Date:   Wed, 24 Aug 2022 09:58:35 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <christophe.jaillet@...adoo.fr>, <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

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.

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.

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