| Message ID | e55c959f2821a2c367a4c5de529a638b1cc6b8cd.1661329086.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rtc: mpfs: Use devm_clk_get_enabled() helper | expand |
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@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. 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
On 24/08/2022 09:58:35+0000, Conor.Dooley@microchip.com wrote: > 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. This would be very surprising that it doesn't keep the time with the AHB clock disabled, this would mean the RTC loses the time when the SoC is not powered. or is the AHB clock also in the always-on domain? > > 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. > > 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 >
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 >
On 24/08/2022 12:27, Christophe JAILLET wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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... >> >>> - 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. > I am doing some more testing rn, before replying to Alexandre - I guess I am just wondering if actually disabling the clock prior to to removing the driver makes any sense. Maybe the lock just needs to be explicitly marked as critical, in which case this patch makes complete sense to me. Thanks, Conor.
On 24/08/2022 11:53, Alexandre Belloni wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 24/08/2022 09:58:35+0000, Conor.Dooley@microchip.com wrote: >> 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. > > This would be very surprising that it doesn't keep the time with the AHB > clock disabled, this would mean the RTC loses the time when the SoC is > not powered. or is the AHB clock also in the always-on domain? If the SoC is completely unpowered, the reference clock will be gone too - not just the AHB clock. In low power states, or with the cpus disabled etc, the rtc should keep counting. Is there something I have missed & should have either documented, explained or set when first submitting the driver? I remember reading in the docs about the rtc class framework supporting systems where the battery backed rtc was external & the SoC had an RTC with potentially more features etc. Maybe I misunderstood? This is a "hardened" version of an FPGA core & AFAIK the clock that clocks the AHB interface/wrapper also clocks the logic in the RTC. The docs are very scant, so I will try to get my hands on the RTL. The clocks themselves should be always-on.. I did some brief testing just now, and if I disable the AHB interface time keeping is lost. I am inclined to say that this patch is valid & the underlying clock needs to be marked as critical - unless I have missed something.. Applying this patch seems like it will have no functional change since the clock is already being disabled in the remove callback so: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> If my conclusions here are correct, I'll submit a patch for the clock driver marking the rtc's AHB interface clock as critical. > >> >> 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. >> >> 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 >> > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On 24/08/2022 13:27:02+0200, Christophe JAILLET wrote: > 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. > BTW, I thought you actually tested your changes on the other patch I took, not that you were doing a blanket conversion of the subsystem. This is the kind of info that must appear in the commit log. I would definitively not have taken the patch.
Le 24/08/2022 à 14:28, Alexandre Belloni a écrit : > > BTW, I thought you actually tested your changes on the other patch I > took, not that you were doing a blanket conversion of the subsystem. > This is the kind of info that must appear in the commit log. I would > definitively not have taken the patch. > Ok, noted for future contribution. In fact I first sent only one patch to see if it got some interest for such transformation. I only sent some other after your Ack. Nothing is never trivial, but such patches looks fine to me. It saves some LoC, reduce the size of the .o and slightly saves some runtime memory. And unless, I missed something, the order of operation remains the same, both when resources are allocated and freed. Why wouldn't you have taken such a patch? (just for my understanding and in order to avoid spamming others with useless/risky stuff) CJ
On Wed, 24 Aug 2022 10:18:25 +0200, Christophe JAILLET wrote: > 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(). > > [...] Applied, thanks! [1/1] rtc: mpfs: Use devm_clk_get_enabled() helper commit: 24fb316155a5f6ba278a8b110c60e67b79900356 Best regards,
On 13/10/2022 22:32, Alexandre Belloni wrote: > On Wed, 24 Aug 2022 10:18:25 +0200, Christophe JAILLET wrote: >> 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(). >> >> [...] > > Applied, thanks! > > [1/1] rtc: mpfs: Use devm_clk_get_enabled() helper > commit: 24fb316155a5f6ba278a8b110c60e67b79900356 > > Best regards, > While this is on my mind, making the rtc's ahb clock critical came up in previous discussion about this patch. A fix for that was applied to v6.0. Thanks, Conor.
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); - 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);
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. 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(-)