| Message ID | 1507796693-14268-1-git-send-email-geert+renesas@glider.be |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | ravb: Consolidate clock handling | expand |
On Thu, Oct 12, 2017 at 10:24:53AM +0200, Geert Uytterhoeven wrote: > The module clock is used for two purposes: > - Wake-on-LAN (WoL), which is optional, > - gPTP Timer Increment (GTI) configuration, which is mandatory. > > As the clock is needed for GTI configuration anyway, WoL is always > available. Hence remove duplication and repeated obtaining of the clock > by making GTI use the stored clock for WoL use. Hi Geert, I understand from the statements above that the clock must be present, but I'm most sure that I understand why that means that WoL is always available. Assuming your assertion that WoL is correct the code changes look good to me.
Hi Simon, On Thu, Oct 12, 2017 at 11:55 AM, Simon Horman <horms@verge.net.au> wrote: > On Thu, Oct 12, 2017 at 10:24:53AM +0200, Geert Uytterhoeven wrote: >> The module clock is used for two purposes: >> - Wake-on-LAN (WoL), which is optional, >> - gPTP Timer Increment (GTI) configuration, which is mandatory. >> >> As the clock is needed for GTI configuration anyway, WoL is always >> available. Hence remove duplication and repeated obtaining of the clock >> by making GTI use the stored clock for WoL use. > > Hi Geert, > > I understand from the statements above that the clock must be present, > but I'm most sure that I understand why that means that WoL is always > available. That refers to the part below: WoL was considered available iff the clock is available. @@ -2073,10 +2058,11 @@ static int ravb_probe(struct platform_device *pdev) priv->chip_id = chip_id; - /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */ priv->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(priv->clk)) - priv->clk = NULL; + if (IS_ERR(priv->clk)) { + error = PTR_ERR(priv->clk); + goto out_release; + } But the clock must always be available, else GTI configuration would fail, and ravb initialization would return with an error. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Grate patch! Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> On 2017-10-12 10:24:53 +0200, Geert Uytterhoeven wrote: > The module clock is used for two purposes: > - Wake-on-LAN (WoL), which is optional, > - gPTP Timer Increment (GTI) configuration, which is mandatory. > > As the clock is needed for GTI configuration anyway, WoL is always > available. Hence remove duplication and repeated obtaining of the clock > by making GTI use the stored clock for WoL use. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/net/ethernet/renesas/ravb_main.c | 35 +++++++++----------------------- > 1 file changed, 10 insertions(+), 25 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index fdf30bfa403bf416..8fad62f08e85e3ac 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1337,20 +1337,15 @@ static void ravb_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol) > { > struct ravb_private *priv = netdev_priv(ndev); > > - wol->supported = 0; > - wol->wolopts = 0; > - > - if (priv->clk) { > - wol->supported = WAKE_MAGIC; > - wol->wolopts = priv->wol_enabled ? WAKE_MAGIC : 0; > - } > + wol->supported = WAKE_MAGIC; > + wol->wolopts = priv->wol_enabled ? WAKE_MAGIC : 0; > } > > static int ravb_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol) > { > struct ravb_private *priv = netdev_priv(ndev); > > - if (!priv->clk || wol->wolopts & ~WAKE_MAGIC) > + if (wol->wolopts & ~WAKE_MAGIC) > return -EOPNOTSUPP; > > priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC); > @@ -1912,22 +1907,12 @@ MODULE_DEVICE_TABLE(of, ravb_match_table); > > static int ravb_set_gti(struct net_device *ndev) > { > - > + struct ravb_private *priv = netdev_priv(ndev); > struct device *dev = ndev->dev.parent; > - struct device_node *np = dev->of_node; > unsigned long rate; > - struct clk *clk; > uint64_t inc; > > - clk = of_clk_get(np, 0); > - if (IS_ERR(clk)) { > - dev_err(dev, "could not get clock\n"); > - return PTR_ERR(clk); > - } > - > - rate = clk_get_rate(clk); > - clk_put(clk); > - > + rate = clk_get_rate(priv->clk); > if (!rate) > return -EINVAL; > > @@ -2073,10 +2058,11 @@ static int ravb_probe(struct platform_device *pdev) > > priv->chip_id = chip_id; > > - /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */ > priv->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(priv->clk)) > - priv->clk = NULL; > + if (IS_ERR(priv->clk)) { > + error = PTR_ERR(priv->clk); > + goto out_release; > + } > > /* Set function */ > ndev->netdev_ops = &ravb_netdev_ops; > @@ -2144,8 +2130,7 @@ static int ravb_probe(struct platform_device *pdev) > if (error) > goto out_napi_del; > > - if (priv->clk) > - device_set_wakeup_capable(&pdev->dev, 1); > + device_set_wakeup_capable(&pdev->dev, 1); > > /* Print device information */ > netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n", > -- > 2.7.4 >
Hello! On 10/12/2017 11:24 AM, Geert Uytterhoeven wrote: > The module clock is used for two purposes: > - Wake-on-LAN (WoL), which is optional, > - gPTP Timer Increment (GTI) configuration, which is mandatory. > > As the clock is needed for GTI configuration anyway, WoL is always > available. Hence remove duplication and repeated obtaining of the clock > by making GTI use the stored clock for WoL use. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> MBR, Sergei
From: Geert Uytterhoeven <geert+renesas@glider.be> Date: Thu, 12 Oct 2017 10:24:53 +0200 > The module clock is used for two purposes: > - Wake-on-LAN (WoL), which is optional, > - gPTP Timer Increment (GTI) configuration, which is mandatory. > > As the clock is needed for GTI configuration anyway, WoL is always > available. Hence remove duplication and repeated obtaining of the clock > by making GTI use the stored clock for WoL use. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Applied.
On Thu, Oct 12, 2017 at 02:20:45PM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Thu, Oct 12, 2017 at 11:55 AM, Simon Horman <horms@verge.net.au> wrote: > > On Thu, Oct 12, 2017 at 10:24:53AM +0200, Geert Uytterhoeven wrote: > >> The module clock is used for two purposes: > >> - Wake-on-LAN (WoL), which is optional, > >> - gPTP Timer Increment (GTI) configuration, which is mandatory. > >> > >> As the clock is needed for GTI configuration anyway, WoL is always > >> available. Hence remove duplication and repeated obtaining of the clock > >> by making GTI use the stored clock for WoL use. > > > > Hi Geert, > > > > I understand from the statements above that the clock must be present, > > but I'm most sure that I understand why that means that WoL is always > > available. > > That refers to the part below: WoL was considered available iff the clock > is available. > > @@ -2073,10 +2058,11 @@ static int ravb_probe(struct platform_device *pdev) > > priv->chip_id = chip_id; > > - /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */ > priv->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(priv->clk)) > - priv->clk = NULL; > + if (IS_ERR(priv->clk)) { > + error = PTR_ERR(priv->clk); > + goto out_release; > + } > > But the clock must always be available, else GTI configuration > would fail, and ravb initialization would return with an error. Thanks, that was the information I had missed. Belated: Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index fdf30bfa403bf416..8fad62f08e85e3ac 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1337,20 +1337,15 @@ static void ravb_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol) { struct ravb_private *priv = netdev_priv(ndev); - wol->supported = 0; - wol->wolopts = 0; - - if (priv->clk) { - wol->supported = WAKE_MAGIC; - wol->wolopts = priv->wol_enabled ? WAKE_MAGIC : 0; - } + wol->supported = WAKE_MAGIC; + wol->wolopts = priv->wol_enabled ? WAKE_MAGIC : 0; } static int ravb_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol) { struct ravb_private *priv = netdev_priv(ndev); - if (!priv->clk || wol->wolopts & ~WAKE_MAGIC) + if (wol->wolopts & ~WAKE_MAGIC) return -EOPNOTSUPP; priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC); @@ -1912,22 +1907,12 @@ MODULE_DEVICE_TABLE(of, ravb_match_table); static int ravb_set_gti(struct net_device *ndev) { - + struct ravb_private *priv = netdev_priv(ndev); struct device *dev = ndev->dev.parent; - struct device_node *np = dev->of_node; unsigned long rate; - struct clk *clk; uint64_t inc; - clk = of_clk_get(np, 0); - if (IS_ERR(clk)) { - dev_err(dev, "could not get clock\n"); - return PTR_ERR(clk); - } - - rate = clk_get_rate(clk); - clk_put(clk); - + rate = clk_get_rate(priv->clk); if (!rate) return -EINVAL; @@ -2073,10 +2058,11 @@ static int ravb_probe(struct platform_device *pdev) priv->chip_id = chip_id; - /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */ priv->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(priv->clk)) - priv->clk = NULL; + if (IS_ERR(priv->clk)) { + error = PTR_ERR(priv->clk); + goto out_release; + } /* Set function */ ndev->netdev_ops = &ravb_netdev_ops; @@ -2144,8 +2130,7 @@ static int ravb_probe(struct platform_device *pdev) if (error) goto out_napi_del; - if (priv->clk) - device_set_wakeup_capable(&pdev->dev, 1); + device_set_wakeup_capable(&pdev->dev, 1); /* Print device information */ netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n",
The module clock is used for two purposes: - Wake-on-LAN (WoL), which is optional, - gPTP Timer Increment (GTI) configuration, which is mandatory. As the clock is needed for GTI configuration anyway, WoL is always available. Hence remove duplication and repeated obtaining of the clock by making GTI use the stored clock for WoL use. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/net/ethernet/renesas/ravb_main.c | 35 +++++++++----------------------- 1 file changed, 10 insertions(+), 25 deletions(-)