Messages in this thread | | | Date | Wed, 24 Aug 2022 13:27:02 +0200 | Subject | Re: [PATCH] rtc: mpfs: Use devm_clk_get_enabled() helper | From | Christophe JAILLET <> |
| |
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 >
| |