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

[Christophe JAILLET]

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