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  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(>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 

Re: [PATCH] ravb: Consolidate clock handling

[David Miller]

From: Geert Uytterhoeven 
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 

Applied.

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 


Reviewed-by: Sergei Shtylyov 

MBR, Sergei

Re: [PATCH] ravb: Consolidate clock handling

[Geert Uytterhoeven]

Hi Simon,

On Thu, Oct 12, 2017 at 11:55 AM, Simon Horman  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(>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 -- ge...@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

[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.