| Message ID | 1405328538-29153-1-git-send-email-neidhard.kim@lge.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 07/14/2014 02:32 PM, Jongsung Kim wrote: > This patch enables the ethtool utility to control the WOL function > of the PHY connected to the GEM/MACB. (if supported) > > Signed-off-by: Jongsung Kim <neidhard.kim@lge.com> > --- > drivers/net/ethernet/cadence/macb.c | 26 ++++++++++++++++++++++++++ > 1 files changed, 26 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > index e9daa07..7ad8909 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -1742,11 +1742,37 @@ static void macb_get_regs(struct net_device *dev, struct ethtool_regs *regs, > } > } > > +static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > +{ > + struct macb *bp = netdev_priv(netdev); > + struct phy_device *phydev = bp->phy_dev; > + > + wol->supported = 0; > + wol->wolopts = 0; > + > + if (phydev) > + phy_ethtool_get_wol(phydev, wol); > +} > + > +static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > +{ > + struct macb *bp = netdev_priv(netdev); > + struct phy_device *phydev = bp->phy_dev; > + int err = -ENODEV; > + > + if (phydev) > + err = phy_ethtool_set_wol(phydev, wol); > + > + return err; > +} > + I think we can do in this way: if (phydev) return phy_ethtool_set_wol(phydev, wol); else return -ENODEV; we can save err. What do you say ...? > const struct ethtool_ops macb_ethtool_ops = { > .get_settings = macb_get_settings, > .set_settings = macb_set_settings, > .get_regs_len = macb_get_regs_len, > .get_regs = macb_get_regs, > + .get_wol = macb_get_wol, > + .set_wol = macb_set_wol, > .get_link = ethtool_op_get_link, > .get_ts_info = ethtool_op_get_ts_info, > };
From: Varka Bhadram > On 07/14/2014 02:32 PM, Jongsung Kim wrote: > > This patch enables the ethtool utility to control the WOL function > > of the PHY connected to the GEM/MACB. (if supported) ... > > +static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > > +{ > > + struct macb *bp = netdev_priv(netdev); > > + struct phy_device *phydev = bp->phy_dev; > > + int err = -ENODEV; > > + > > + if (phydev) > > + err = phy_ethtool_set_wol(phydev, wol); > > + > > + return err; > > +} > > + > > I think we can do in this way: > > if (phydev) > return phy_ethtool_set_wol(phydev, wol); > else > return -ENODEV; > > > we can save err. What do you say ...? I would do: if (!phydev) return -ENODEV; return phy_ethtool_set_wol(phydev, wol); Although it might even be worth moving the NULL test into the function. (sort of depends on style and the number of callers who need to do the test.) David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/14/2014 04:19 PM, David Laight wrote: > From: Varka Bhadram >> On 07/14/2014 02:32 PM, Jongsung Kim wrote: >>> This patch enables the ethtool utility to control the WOL function >>> of the PHY connected to the GEM/MACB. (if supported) > ... >>> +static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) >>> +{ >>> + struct macb *bp = netdev_priv(netdev); >>> + struct phy_device *phydev = bp->phy_dev; >>> + int err = -ENODEV; >>> + >>> + if (phydev) >>> + err = phy_ethtool_set_wol(phydev, wol); >>> + >>> + return err; >>> +} >>> + >> I think we can do in this way: >> >> if (phydev) >> return phy_ethtool_set_wol(phydev, wol); >> else >> return -ENODEV; >> >> >> we can save err. What do you say ...? > I would do: > if (!phydev) > return -ENODEV; > return phy_ethtool_set_wol(phydev, wol); I will agree with this.... :-) if (!phydev) { netdev_err("bla bla..."); return -ENODEV; } return phy_ethtool_set_wol(phydev, wol);
On 07/14/2014 07:49 PM, David Laight wrote: > From: Varka Bhadram >> On 07/14/2014 02:32 PM, Jongsung Kim wrote: >>> This patch enables the ethtool utility to control the WOL function >>> of the PHY connected to the GEM/MACB. (if supported) > ... >>> +static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) >>> +{ >>> + struct macb *bp = netdev_priv(netdev); >>> + struct phy_device *phydev = bp->phy_dev; >>> + int err = -ENODEV; >>> + >>> + if (phydev) >>> + err = phy_ethtool_set_wol(phydev, wol); >>> + >>> + return err; >>> +} >>> + >> >> I think we can do in this way: >> >> if (phydev) >> return phy_ethtool_set_wol(phydev, wol); >> else >> return -ENODEV; >> >> >> we can save err. What do you say ...? > > I would do: > if (!phydev) > return -ENODEV; > return phy_ethtool_set_wol(phydev, wol); > > Although it might even be worth moving the NULL test into the function. > (sort of depends on style and the number of callers who need to do the test.) > Totally agreed. I'd be better to submit a patch about it first. > David > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/14/2014 07:15 PM, Varka Bhadram wrote: > > I think we can do in this way: > > if (phydev) > return phy_ethtool_set_wol(phydev, wol); > else > return -ENODEV; > > > we can save err. What do you say ...? > Thank you for your comment. I just wanted the function to return at one position. However, I'll think about your point.. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index e9daa07..7ad8909 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -1742,11 +1742,37 @@ static void macb_get_regs(struct net_device *dev, struct ethtool_regs *regs, } } +static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) +{ + struct macb *bp = netdev_priv(netdev); + struct phy_device *phydev = bp->phy_dev; + + wol->supported = 0; + wol->wolopts = 0; + + if (phydev) + phy_ethtool_get_wol(phydev, wol); +} + +static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) +{ + struct macb *bp = netdev_priv(netdev); + struct phy_device *phydev = bp->phy_dev; + int err = -ENODEV; + + if (phydev) + err = phy_ethtool_set_wol(phydev, wol); + + return err; +} + const struct ethtool_ops macb_ethtool_ops = { .get_settings = macb_get_settings, .set_settings = macb_set_settings, .get_regs_len = macb_get_regs_len, .get_regs = macb_get_regs, + .get_wol = macb_get_wol, + .set_wol = macb_set_wol, .get_link = ethtool_op_get_link, .get_ts_info = ethtool_op_get_ts_info, };
This patch enables the ethtool utility to control the WOL function of the PHY connected to the GEM/MACB. (if supported) Signed-off-by: Jongsung Kim <neidhard.kim@lge.com> --- drivers/net/ethernet/cadence/macb.c | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-)