Date:   Wed, 24 Aug 2022 13:27:02 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Conor.Dooley@...rochip.com, Daire.McNamara@...rochip.com,
        a.zummo@...ertech.it, alexandre.belloni@...tlin.com
Cc:     linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-rtc@...r.kernel.org
Subject: Re: [PATCH] rtc: mpfs: Use devm_clk_get_enabled() helper

Le 24/08/2022 à 11:58, Conor.Dooley@...rochip.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@...adoo.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@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

Powered by blists - more mailing lists