lkml.org 
[lkml]   [2022]   [Aug]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] rtc: mpfs: Use devm_clk_get_enabled() helper
From
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...
>
>>
>> This also fixes an (unlikely) unchecked devm_add_action_or_reset() error.
>>
>> Based on my test with allyesconfig, this reduces the .o size from:
>> text data bss dec hex filename
>> 5330 2208 0 7538 1d72 drivers/rtc/rtc-mpfs.o
>> down to:
>> 5074 2208 0 7282 1c72 drivers/rtc/rtc-mpfs.o
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> devm_clk_get_enabled() is new and is part of 6.0-rc1
>> ---
>> drivers/rtc/rtc-mpfs.c | 19 +------------------
>> 1 file changed, 1 insertion(+), 18 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-mpfs.c b/drivers/rtc/rtc-mpfs.c
>> index 944ad1036516..2a479d44f198 100644
>> --- a/drivers/rtc/rtc-mpfs.c
>> +++ b/drivers/rtc/rtc-mpfs.c
>> @@ -193,23 +193,6 @@ static int mpfs_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>> return 0;
>> }
>>
>> -static inline struct clk *mpfs_rtc_init_clk(struct device *dev)
>> -{
>> - struct clk *clk;
>> - int ret;
>> -
>> - clk = devm_clk_get(dev, "rtc");
>> - if (IS_ERR(clk))
>> - return clk;
>> -
>> - ret = clk_prepare_enable(clk);
>> - if (ret)
>> - return ERR_PTR(ret);
>> -
>> - 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.

CJ

>
> Thanks,
> Conor.
>
>> - return clk;
>> -}
>> -
>> static irqreturn_t mpfs_rtc_wakeup_irq_handler(int irq, void *dev)
>> {
>> struct mpfs_rtc_dev *rtcdev = dev;
>> @@ -251,7 +234,7 @@ static int mpfs_rtc_probe(struct platform_device *pdev)
>> /* range is capped by alarm max, lower reg is 31:0 & upper is 10:0 */
>> rtcdev->rtc->range_max = GENMASK_ULL(42, 0);
>>
>> - clk = mpfs_rtc_init_clk(&pdev->dev);
>> + clk = devm_clk_get_enabled(&pdev->dev, "rtc");
>> if (IS_ERR(clk))
>> return PTR_ERR(clk);
>>
>> --
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

\
 
 \ /
  Last update: 2022-08-24 13:28    [W:0.113 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site