Re: [PATCH] rtc: mpfs: Use devm_clk_get_enabled() helper

[<Conor.Dooley@microchip.com>]

On 24/08/2022 12:27, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Le 24/08/2022 à 11:58, Conor.Dooley@microchip.com a écrit :
>> Hey Christope,
>> Thanks for your patch.
>>
>> On 24/08/2022 09:18, Christophe JAILLET wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> The devm_clk_get_enabled() helper:
>>>      - calls devm_clk_get()
>>>      - calls clk_prepare_enable() and registers what is needed in order to
>>>        call clk_disable_unprepare() when needed, as a managed resource.
>>>
>>> This simplifies the code, the error handling paths and avoid the need of
>>> a dedicated function used with devm_add_action_or_reset().
>>>
>>> That said, mpfs_rtc_init_clk() is the same as devm_clk_get_enabled(), so
>>> use this function directly instead.
>>
>> Firstly, I think something is missing from the commit description here.
>> devm_clk_get_enabled() is not just a blanket "use this instead of get(),
>> prepare_enable()" & is only intended for cases where the clock would
>> be kept prepared or enabled for the whole lifetime of the driver. I think
>> it is worth pointing that out to help people who do not keep up with
>> every helper that is added.
> 
> Ok, I'll update my commit log for other similar patches or should a v2
> be needed.
> 
>>
>> I had a bit of a look through the documentation to see if the block would
>> keep track of time without the AHB clock enabled, but it does not seem to.
>> There is no reason to turn off the clock here (in fact it would seem
>> counter productive to disable it..) so it looks like the shoe fits in that
>> regard.
>>
>> However...
>>

>>> -       devm_add_action_or_reset(dev, (void (*) (void *))clk_disable_unprepare, clk);
>>
>> ... this bit here concerns me a little. I don't think we should be
>> registering a callback here at all - if we power down Linux this is
>> going to end up stopping the RTC isn't it?
>>
>> I think this is left over from the v1 driver submission that reset
>> the block during probe & should be removed.
> 
> My point is only that what is done must be undone at some point.
> 
> What if an error occurs in the probe after the clk_get("rtc")?
> Is there any point keeping it prepared and enabled?
> 
> 
> There is a .remove function in this driver, so, it looks that it is
> expected that it can be unloaded.
> 
> So undoing this clk operations via a managed resource looks the correct
> thing to do.
> 
> Just my 2c, you must know this driver and the expected behavior better
> than me.
>

I am doing some more testing rn, before replying to Alexandre - I guess
I am just wondering if actually disabling the clock prior to to removing
the driver makes any sense. Maybe the lock just needs to be explicitly
marked as critical, in which case this patch makes complete sense to me.

Thanks,
Conor.