Re: [PATCH 5/7] watchdog: dw_wdt: Support devices with asynch clocks

[Guenter Roeck]

On 4/13/20 1:52 PM, Stephen Boyd wrote:
> Quoting Sergey Semin (2020-04-10 11:59:34)
>> Michael, Stephen, could you take a look at the issue we've got here?
>>
>> Guenter, my comment is below.
>>
>> On Sun, Mar 15, 2020 at 07:22:07AM -0700, Guenter Roeck wrote:
>>> On Fri, Mar 06, 2020 at 04:27:45PM +0300, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
>>>> @@ -358,10 +375,27 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>>             goto out_disable_clk;
>>>>     }
>>>>  
>>>> +   /*
>>>> +    * Request APB clocks if device is configured with async clocks mode.
>>>> +    * In this case both tclk and pclk clocks are supposed to be specified.
>>>> +    * Alas we can't know for sure whether async mode was really activated,
>>>> +    * so the pclk reference is left optional. If it it's failed to be
>>>> +    * found we consider the device configured in synchronous clocks mode.
>>>> +    */
>>>> +   dw_wdt->pclk = devm_clk_get_optional(dev, "pclk");
>>>> +   if (IS_ERR(dw_wdt->pclk)) {
>>>> +           ret = PTR_ERR(dw_wdt->pclk);
>>>> +           goto out_disable_clk;
>>>> +   }
>>>> +
>>>> +   ret = clk_prepare_enable(dw_wdt->pclk);
>>>
>>> Not every implementation of clk_enable() checks for a NULL parameter.
>>> Some return an error. This can not be trusted to work on all platforms /
>>> architectures.
>>
>> Hm, this was unexpected twist. I've submitted not a single patch with optional
>> clock API usage. It was first time I've got a comment like this, that the
>> API isn't cross-platform. As I see it this isn't the patch problem, but the
>> platforms/common clock bug. The platforms code must have been submitted before
>> the optional clock API was introduced or the API hasn't been properly
>> implemented or we don't understand something.
>>
>> Stephen, Michael could you clarify the situation with the
>> cross-platformness of the optional clock API.
>>
> 
> NULL is a valid clk to return from clk_get(). And the documentation of
> clk_enable() says that "If the clock can not be enabled/disabled, this
> should return success". Given that a NULL pointer can't do much of
> anything I think any platform that returns an error in this situation is
> deviating from the documentation of the clk API.
> 

This is not about returning an error; some platforms don't check for NULL.

> Does any platform that uses this driver use one of these non-common clk
> framework implementations? All of this may not matter if they all use
> the CCF.
> 

Currently the driver is only used on arm and arm64. Maybe those are safe ?

Also, it looks like clk_enable() exists in the non-standard implementations,
but clk_prepare (and thus clk_prepare_enable) only exist in the standard
implementation. With that, maybe it is always safe to call
clk_prepare_enable() with a NULL parameter ?

Thanks,
Guenter