[PATCH] ravb: Consolidate clock handling

[PATCH] ravb: Consolidate clock handling

[Geert Uytterhoeven]

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

Re: [PATCH] ravb: Consolidate clock handling

[Simon Horman]

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.

Re: [PATCH] ravb: Consolidate clock handling

[Geert Uytterhoeven]

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

Re: [PATCH] ravb: Consolidate clock handling

[Niklas Söderlund]

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
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] ravb: Consolidate clock handling

[Sergei Shtylyov]

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

Re: [PATCH] ravb: Consolidate clock handling

[David Miller]

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.

Re: [PATCH] ravb: Consolidate clock handling

[Simon Horman]

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>