| Message ID | 20210510174142.986250-1-u.kleine-koenig@pengutronix.de |
|---|---|
| Headers | show |
| Series | clk: provide new devm helpers for prepared and enabled clocks | expand |
On Mon, 10 May 2021 at 20:41, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > compared to v6 I rebased to v5.13-rc1 (which resulted in a conflict in > the pwm-atmel patch), reformated the doc comments in patch 2 (as > suggested by Jonathan Cameron) and added the two Reviewed-by tags for > Jonathan Cameron. This will definitely cleanup a lot of boiler-plate code for probe()/remove(). What I'm noticing, is that some remove hooks [in the drivers in this series] are being slightly re-ordered; i.e. the clk_disable/unprepare may happen in a slightly different order, but it doesn't look like those would be problematic. So, for the series: Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com> Thanks Alex > > Best regards > Uwe > > Uwe Kleine-König (6): > clk: generalize devm_clk_get() a bit > clk: Provide new devm_clk_helpers for prepared and enabled clocks > pwm: atmel: Simplify using devm_clk_get_prepared() > rtc: at91sam9: Simplify using devm_clk_get_enabled() > i2c: imx: Simplify using devm_clk_get_enabled() > spi: davinci: Simplify using devm_clk_get_enabled() > > drivers/clk/clk-devres.c | 96 ++++++++++++++++++++++++++++++------ > drivers/i2c/busses/i2c-imx.c | 12 +---- > drivers/pwm/pwm-atmel.c | 15 +----- > drivers/rtc/rtc-at91sam9.c | 22 ++------- > drivers/spi/spi-davinci.c | 11 +---- > include/linux/clk.h | 90 ++++++++++++++++++++++++++++++++- > 6 files changed, 179 insertions(+), 67 deletions(-) > > > base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 > -- > 2.30.2 >
On Mon, May 10, 2021 at 07:41:36PM +0200, Uwe Kleine-König wrote: > compared to v6 I rebased to v5.13-rc1 (which resulted in a conflict in > the pwm-atmel patch), reformated the doc comments in patch 2 (as > suggested by Jonathan Cameron) and added the two Reviewed-by tags for > Jonathan Cameron. Another two weeks without maintainer feedback. I didn't find a single mail by either Michael Turquette nor by Stephen Boyd on the linux-clk list on lore dating from this month. This patch set didn't get a reply since more than half a year. Is the clk tree still maintained? Would a pull request help? There are several people who expressed interest in this series and the cleanup it allows. @Andrew: Would you be willing to take the first two patches if Michael and Stephen don't react in the near future? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Stephen,
given that I don't succeed in getting any feedback for my patch set, I'm
trying with a pull request today. It would be really great if this pull
request made it finally in for the next merge window.
The changes are not as bad or complex as the diffstat suggests. The
first patch contains all the complexity and only has
1 file changed, 50 insertions(+), 17 deletions(-)
. The second patch makes use of this and just adds kernel-doc, four
functions that are one-line wrappers around the newly introduced
__devm_clk_get() function in the first patch and dummy implementations
for the !CONFIG_HAVE_CLK case.
The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:
Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)
are available in the Git repository at:
https://git.pengutronix.de/git/ukl/linux tags/devm-clk-get-enabled
for you to fetch changes up to fec74d434d6f6016b6b2d5ab13aa28a0c657f5fb:
clk: Provide new devm_clk_helpers for prepared and enabled clocks (2021-05-11 14:20:13 +0200)
----------------------------------------------------------------
New variants of devm_clk_get() for prepared and enabled clocks
These two patches create a set of new devm helpers that return clocks
already prepared or prepared-and-enabled. The automatic cleanup cares
for unpreparing and disabling+unpreparing respectively.
This allows to simplify various drivers as was demonstrated with
additional patches sent with the various revisions of this patch set.
See
https://lore.kernel.org/r/20210510174142.986250-1-u.kleine-koenig@pengutronix.de
for the last submission round. This pull request doesn't contain these
patches though.
----------------------------------------------------------------
Uwe Kleine-König (2):
clk: generalize devm_clk_get() a bit
clk: Provide new devm_clk_helpers for prepared and enabled clocks
drivers/clk/clk-devres.c | 96 ++++++++++++++++++++++++++++++++++++++++--------
include/linux/clk.h | 90 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 169 insertions(+), 17 deletions(-)
Hello Stephen, On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote: > given that I don't succeed in getting any feedback for my patch set, I'm > trying with a pull request today. It would be really great if this pull > request made it finally in for the next merge window. It seems sending a pull request didn't help either :-\ I'm waiting since October for feedback, several people expressed to like this series and I want to make use of it to simplify a few drivers. I'm quite annoyed that your missing feedback blocks me from further improving stuff. > The changes are not as bad or complex as the diffstat suggests. The > first patch contains all the complexity and only has > 1 file changed, 50 insertions(+), 17 deletions(-) > . The second patch makes use of this and just adds kernel-doc, four > functions that are one-line wrappers around the newly introduced > __devm_clk_get() function in the first patch and dummy implementations > for the !CONFIG_HAVE_CLK case. > > The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5: > > Linux 5.13-rc1 (2021-05-09 14:17:44 -0700) > > are available in the Git repository at: > > https://git.pengutronix.de/git/ukl/linux tags/devm-clk-get-enabled > > for you to fetch changes up to fec74d434d6f6016b6b2d5ab13aa28a0c657f5fb: > > clk: Provide new devm_clk_helpers for prepared and enabled clocks (2021-05-11 14:20:13 +0200) > > ---------------------------------------------------------------- > New variants of devm_clk_get() for prepared and enabled clocks > > These two patches create a set of new devm helpers that return clocks > already prepared or prepared-and-enabled. The automatic cleanup cares > for unpreparing and disabling+unpreparing respectively. > > This allows to simplify various drivers as was demonstrated with > additional patches sent with the various revisions of this patch set. > See > https://lore.kernel.org/r/20210510174142.986250-1-u.kleine-koenig@pengutronix.de > for the last submission round. This pull request doesn't contain these > patches though. > > ---------------------------------------------------------------- > Uwe Kleine-König (2): > clk: generalize devm_clk_get() a bit > clk: Provide new devm_clk_helpers for prepared and enabled clocks > > drivers/clk/clk-devres.c | 96 ++++++++++++++++++++++++++++++++++++++++-------- > include/linux/clk.h | 90 ++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 169 insertions(+), 17 deletions(-) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Stephen, On Fri, Jun 25, 2021 at 07:14:34PM +0200, Uwe Kleine-König wrote: > On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote: > > given that I don't succeed in getting any feedback for my patch set, I'm > > trying with a pull request today. It would be really great if this pull > > request made it finally in for the next merge window. > > It seems sending a pull request didn't help either :-\ > > I'm waiting since October for feedback, several people expressed to like > this series and I want to make use of it to simplify a few drivers. I'm > quite annoyed that your missing feedback blocks me from further > improving stuff. There is still no feedback, not even something like: "I saw your nagging, sorry. I'm drown in other missions, please have some more patience." I assume it's not to much to expect at least such a reply after more than 8 months? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Stephen, On Mon, Jul 05, 2021 at 10:01:44AM +0200, Uwe Kleine-König wrote: > On Fri, Jun 25, 2021 at 07:14:34PM +0200, Uwe Kleine-König wrote: > > On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote: > > > given that I don't succeed in getting any feedback for my patch set, I'm > > > trying with a pull request today. It would be really great if this pull > > > request made it finally in for the next merge window. > > > > It seems sending a pull request didn't help either :-\ > > > > I'm waiting since October for feedback, several people expressed to like > > this series and I want to make use of it to simplify a few drivers. I'm > > quite annoyed that your missing feedback blocks me from further > > improving stuff. > > There is still no feedback, not even something like: "I saw your > nagging, sorry. I'm drown in other missions, please have some more > patience." > > I assume it's not to much to expect at least such a reply after more > than 8 months? The next merge window is over now. The pull request still merges fine into v5.14-rc2. I'm still convinced it adds some benefit and I want to use it to simplify a bunch of drivers. But I cannot without this being merged. Do I have to consider creating these functions in the pwm namespace to continue here? This cannot be the right thing to do?! Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
> The next merge window is over now. The pull request still merges fine > into v5.14-rc2. I'm still convinced it adds some benefit and I want to > use it to simplify a bunch of drivers. But I cannot without this being > merged. > > Do I have to consider creating these functions in the pwm namespace to > continue here? This cannot be the right thing to do?! What about adding gkh to the list explaining the situation to him?
Hey Wolfram, On Thu, Jul 22, 2021 at 09:40:03AM +0200, Wolfram Sang wrote: > > > The next merge window is over now. The pull request still merges fine > > into v5.14-rc2. I'm still convinced it adds some benefit and I want to > > use it to simplify a bunch of drivers. But I cannot without this being > > merged. > > > > Do I have to consider creating these functions in the pwm namespace to > > continue here? This cannot be the right thing to do?! > > What about adding gkh to the list explaining the situation to him? Greg doesn't like devm_ stuff. I already asked Arnd who doesn't want to interfere and akpm who didn't react either up to now. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
> > What about adding gkh to the list explaining the situation to him? > > Greg doesn't like devm_ stuff. > > I already asked Arnd who doesn't want to interfere and akpm who didn't > react either up to now. Wow, okay, that is frustrating.
Hello, [adding Linus and lkml to Cc: and adding some more context] On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote: > given that I don't succeed in getting any feedback for my patch set, I'm > trying with a pull request today. This is for a series that is currently in v7 and didn't get any feedback at all yet. The history is: v1: 2020-10-13, https://lore.kernel.org/linux-clk/20201013082132.661993-1-u.kleine-koenig@pengutronix.de no feedback at all v2: 2021-03-01, https://lore.kernel.org/linux-clk/20210301110821.1445756-1-uwe@kleine-koenig.org kernel test robot identified some issues v3: 2021-03-01, https://lore.kernel.org/linux-clk/20210301135053.1462168-1-u.kleine-koenig@pengutronix.de I added a few driver patches to show the benefit. (However in a way that the autobuilders don't understand, so there were some false positive build failure reports.) v4: 2021-03-30, https://lore.kernel.org/linux-clk/20210330181755.204339-1-u.kleine-koenig@pengutronix.de Got some feedback for the converted drivers by the respective maintainers. Some were indifferent, some found it good v5: 2021-04-22, https://lore.kernel.org/linux-clk/20210422065726.1646742-1-u.kleine-koenig@pengutronix.de Fixed a problem in one of the driver changes (i2c-imx), no feedback apart from pointing out a few typos, silence from the clk maintainers v6: 2021-04-26, https://lore.kernel.org/linux-clk/20210426141730.2826832-1-u.kleine-koenig@pengutronix.de Just the typos fixed, no feedback v6 resend: 2021-05-10, https://lore.kernel.org/linux-clk/20210510061724.940447-1-u.kleine-koenig@pengutronix.de no changes in code. Got some feedback from Jonathan Cameron v7: 2021-05-10, https://lore.kernel.org/linux-clk/20210510174142.986250-1-u.kleine-koenig@pengutronix.de Adress Jonathan's feedback, recieved some more acks from non-clk people pull request: 2021-07-09, https://lore.kernel.org/linux-clk/20210609202123.u5rmw7al4x3rrvun@pengutronix.de On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote: > On Thursday, July 22, 2021, Wolfram Sang <wsa@kernel.org> wrote: > > > > > > > What about adding gkh to the list explaining the situation to him? > > > > > > Greg doesn't like devm_ stuff. > > > > > > I already asked Arnd who doesn't want to interfere and akpm who didn't > > > react either up to now. > > > > Wow, okay, that is frustrating. > > The situation simply shows the process gap and One Maintainer nowadays is > far from enough to satisfy demands. Technically there are two maintainers for drivers/clk, Michael Turquette and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the capacity to address all requests. > What I think about is that we need to escalate this to Linus and > others and elaborate the mechanisms how to squeeze a new (additional) > maintainer when the original one is not responsive. Let’s say some > procedural steps. Otherwise we doomed because of human factor. Assuming there was some process for this, is there someone who is willing to take responsibility here? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On 23.07.2021 12:13, Uwe Kleine-König wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > ForwardedMessage.eml > > Subject: > Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks > enabled clocks > From: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Date: > 23.07.2021, 12:13 > > To: > Andy Shevchenko <andy.shevchenko@gmail.com> > CC: > Wolfram Sang <wsa@kernel.org>, Stephen Boyd <sboyd@kernel.org>, > "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>, > "linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>, Alexandre Belloni > <alexandre.belloni@bootlin.com>, Claudiu Beznea > <claudiu.beznea@microchip.com>, Alessandro Zummo <a.zummo@towertech.it>, > Michael Turquette <mturquette@baylibre.com>, Nicolas Ferre > <nicolas.ferre@microchip.com>, "linux-spi@vger.kernel.org" > <linux-spi@vger.kernel.org>, Oleksij Rempel <o.rempel@pengutronix.de>, > Ludovic Desroches <ludovic.desroches@microchip.com>, Mark Brown > <broonie@kernel.org>, Thierry Reding <thierry.reding@gmail.com>, Alexandru > Ardelean <aardelean@deviqon.com>, "kernel@pengutronix.de" > <kernel@pengutronix.de>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, > Andrew Morton <akpm@linux-foundation.org>, Lee Jones > <lee.jones@linaro.org>, "linux-clk@vger.kernel.org" > <linux-clk@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" > <linux-arm-kernel@lists.infradead.org>, Linus Torvalds > <torvalds@linux-foundation.org>, <linux-kernel@vger.kernel.org> > > > Hello, > > [adding Linus and lkml to Cc: and adding some more context] > > On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote: >> given that I don't succeed in getting any feedback for my patch set, I'm >> trying with a pull request today. > This is for a series that is currently in v7 and didn't get any feedback > at all yet. The history is: > > v1: 2020-10-13, https://lore.kernel.org/linux-clk/20201013082132.661993-1-u.kleine-koenig@pengutronix.de > no feedback at all > > v2: 2021-03-01, https://lore.kernel.org/linux-clk/20210301110821.1445756-1-uwe@kleine-koenig.org > kernel test robot identified some issues > > v3: 2021-03-01, https://lore.kernel.org/linux-clk/20210301135053.1462168-1-u.kleine-koenig@pengutronix.de > I added a few driver patches to show the benefit. (However in a way > that the autobuilders don't understand, so there were some false > positive build failure reports.) > > v4: 2021-03-30, https://lore.kernel.org/linux-clk/20210330181755.204339-1-u.kleine-koenig@pengutronix.de > Got some feedback for the converted drivers by the respective > maintainers. Some were indifferent, some found it good > > v5: 2021-04-22, https://lore.kernel.org/linux-clk/20210422065726.1646742-1-u.kleine-koenig@pengutronix.de > Fixed a problem in one of the driver changes (i2c-imx), no feedback > apart from pointing out a few typos, silence from the clk > maintainers > > v6: 2021-04-26, https://lore.kernel.org/linux-clk/20210426141730.2826832-1-u.kleine-koenig@pengutronix.de > Just the typos fixed, no feedback > > v6 resend: 2021-05-10, https://lore.kernel.org/linux-clk/20210510061724.940447-1-u.kleine-koenig@pengutronix.de > no changes in code. Got some feedback from Jonathan Cameron > > v7: 2021-05-10, https://lore.kernel.org/linux-clk/20210510174142.986250-1-u.kleine-koenig@pengutronix.de > Adress Jonathan's feedback, recieved some more acks from non-clk > people > > pull request: 2021-07-09, https://lore.kernel.org/linux-clk/20210609202123.u5rmw7al4x3rrvun@pengutronix.de > > On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote: >> On Thursday, July 22, 2021, Wolfram Sang <wsa@kernel.org> wrote: >> >>>>> What about adding gkh to the list explaining the situation to him? >>>> Greg doesn't like devm_ stuff. >>>> >>>> I already asked Arnd who doesn't want to interfere and akpm who didn't >>>> react either up to now. >>> Wow, okay, that is frustrating. >> The situation simply shows the process gap and One Maintainer nowadays is >> far from enough to satisfy demands. > Technically there are two maintainers for drivers/clk, Michael Turquette > and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the > capacity to address all requests. > >> What I think about is that we need to escalate this to Linus and >> others and elaborate the mechanisms how to squeeze a new (additional) >> maintainer when the original one is not responsive. Let’s say some >> procedural steps. Otherwise we doomed because of human factor. > Assuming there was some process for this, is there someone who is > willing to take responsibility here? Hi, In the last year I worked on AT91 clock drivers and I would be available for taking responsibility beyond AT91 clocks (if everyone's OK with this), in whatever form the current maintainers and people in the audience would agree, if any (co-maintainer or other forms that could be useful). The idea is to help things progress as I also have patches waiting for feedback on clock mailing list for almost 6 months. Let me know if I can be helpful. Thank you, Claudiu Beznea > > Best regards > Uwe > > -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | > https://www.pengutronix.de/ | >
On Mon, Jul 26, 2021 at 12:18 PM <Claudiu.Beznea@microchip.com> wrote: > On 23.07.2021 12:13, Uwe Kleine-König wrote: > > From: > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Date: > > 23.07.2021, 12:13 > > On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote: > >> given that I don't succeed in getting any feedback for my patch set, I'm > >> trying with a pull request today. > > This is for a series that is currently in v7 and didn't get any feedback > > at all yet. The history is: > > > > v1: 2020-10-13, https://lore.kernel.org/linux-clk/20201013082132.661993-1-u.kleine-koenig@pengutronix.de > > no feedback at all > > > > v2: 2021-03-01, https://lore.kernel.org/linux-clk/20210301110821.1445756-1-uwe@kleine-koenig.org > > kernel test robot identified some issues > > > > v3: 2021-03-01, https://lore.kernel.org/linux-clk/20210301135053.1462168-1-u.kleine-koenig@pengutronix.de > > I added a few driver patches to show the benefit. (However in a way > > that the autobuilders don't understand, so there were some false > > positive build failure reports.) > > > > v4: 2021-03-30, https://lore.kernel.org/linux-clk/20210330181755.204339-1-u.kleine-koenig@pengutronix.de > > Got some feedback for the converted drivers by the respective > > maintainers. Some were indifferent, some found it good > > > > v5: 2021-04-22, https://lore.kernel.org/linux-clk/20210422065726.1646742-1-u.kleine-koenig@pengutronix.de > > Fixed a problem in one of the driver changes (i2c-imx), no feedback > > apart from pointing out a few typos, silence from the clk > > maintainers > > > > v6: 2021-04-26, https://lore.kernel.org/linux-clk/20210426141730.2826832-1-u.kleine-koenig@pengutronix.de > > Just the typos fixed, no feedback > > > > v6 resend: 2021-05-10, https://lore.kernel.org/linux-clk/20210510061724.940447-1-u.kleine-koenig@pengutronix.de > > no changes in code. Got some feedback from Jonathan Cameron > > > > v7: 2021-05-10, https://lore.kernel.org/linux-clk/20210510174142.986250-1-u.kleine-koenig@pengutronix.de > > Adress Jonathan's feedback, recieved some more acks from non-clk > > people > > > > pull request: 2021-07-09, https://lore.kernel.org/linux-clk/20210609202123.u5rmw7al4x3rrvun@pengutronix.de > > > > On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote: > >> On Thursday, July 22, 2021, Wolfram Sang <wsa@kernel.org> wrote: > >> > >>>>> What about adding gkh to the list explaining the situation to him? > >>>> Greg doesn't like devm_ stuff. > >>>> > >>>> I already asked Arnd who doesn't want to interfere and akpm who didn't > >>>> react either up to now. > >>> Wow, okay, that is frustrating. > >> The situation simply shows the process gap and One Maintainer nowadays is > >> far from enough to satisfy demands. > > Technically there are two maintainers for drivers/clk, Michael Turquette > > and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the > > capacity to address all requests. > > > >> What I think about is that we need to escalate this to Linus and > >> others and elaborate the mechanisms how to squeeze a new (additional) > >> maintainer when the original one is not responsive. Let’s say some > >> procedural steps. Otherwise we doomed because of human factor. > > Assuming there was some process for this, is there someone who is > > willing to take responsibility here? > > Hi, > > In the last year I worked on AT91 clock drivers and I would be available > for taking responsibility beyond AT91 clocks (if everyone's OK with this), > in whatever form the current maintainers and people in the audience would > agree, if any (co-maintainer or other forms that could be useful). The idea > is to help things progress as I also have patches waiting for feedback on > clock mailing list for almost 6 months. > > Let me know if I can be helpful. I think so. Many subsystems relatively recently (in the last couple of years or so) enforced that new drivers have to have official maintainers. Besides that it's warmly welcome to register existing drivers in the MAINTAINERS database. I would tell you go ahead and become a maintainer of AT91 clocks and it will definitely reduce the burden on Stephan's shoulders. The idea is that you will send a PR to CCF maintainers instead of patches, although it's expected that patches appear in the mailing list beforehand anyway. -- With Best Regards, Andy Shevchenko
> The idea is that you will send a PR to CCF maintainers instead of > patches, although it's expected that patches appear in the mailing > list beforehand anyway. Depends a little. For me, a Rev-by from the driver maintainer is enough, and I'll pick them. That lowers the burden on the drivers maintainer side. May not work for high volumes of patches, but for I2C this works enough.
On Mon, Jul 26, 2021 at 3:33 PM Wolfram Sang <wsa@kernel.org> wrote: > > > > The idea is that you will send a PR to CCF maintainers instead of > > patches, although it's expected that patches appear in the mailing > > list beforehand anyway. > > Depends a little. For me, a Rev-by from the driver maintainer is enough, > and I'll pick them. That lowers the burden on the drivers maintainer > side. May not work for high volumes of patches, but for I2C this works > enough. AFAICT in practice it's a mandatory requirement in I²C subsys (in the past you hadn't accepted a patch from us without a tag from the maintainer) which makes it equal to sending PR by a maintainer. PR makes less burden since subsys maintainer don't need to run many tools or a tool many times to get the pile of patches. -- With Best Regards, Andy Shevchenko
> AFAICT in practice it's a mandatory requirement in I²C subsys (in the > past you hadn't accepted a patch from us without a tag from the > maintainer) which makes it equal to sending PR by a maintainer. PR Right. I require a tag from the driver maintainer. > makes less burden since subsys maintainer don't need to run many tools > or a tool many times to get the pile of patches. I had driver maintainers who found it difficult to have a public tree to pull from or forgot how to send properly prepared pull requests. They were happy to send Rev-by, though. And I am happy with that, too. At least in I2C, picking up patches is small work compared to the actual review.
On Wed, Jul 28, 2021 at 10:25:47PM +0200, Uwe Kleine-König wrote: > I adapted the Subject in the hope to catch Stephen's and Michael's > attention. My impression is that this thread isn't on their radar yet, > but the topic here seems important enough to get a matching Subject. Have you thought about sending your pull request to the clk API maintainer (iow, me) ? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Quoting Russell King (Oracle) (2021-07-28 13:40:34) > > I adapted the Subject in the hope to catch Stephen's and Michael's > > attention. My impression is that this thread isn't on their radar yet, > > but the topic here seems important enough to get a matching Subject. The thread is on my radar but... > > Have you thought about sending your pull request to the clk API > maintainer (iow, me) ? > +1 This patch doesn't fall under CCF maintainer. $ ./scripts/get_maintainer.pl -f include/linux/clk.h Russell King <linux@armlinux.org.uk> (maintainer:CLK API) linux-clk@vger.kernel.org (open list:CLK API) linux-kernel@vger.kernel.org (open list) Finally, this sort of patch has been discussed for years and I didn't see any mention of those previous attempts in the patch series. Has something changed since that time? I think we've got a bunch of hand rolled devm things in the meantime but not much else. I still wonder if it would be better if we had some sort of DT binding that said "turn this clk on when the driver probes this device and keep it on until the driver is unbound". That would probably work well for quite a few drivers that don't want to ever call clk_get() or clk_prepare_enable() and could tie into the assigned-clock-rates property in a way that let us keep the platform details out of the drivers. We could also go one step further and make a clk pm domain when this new property is present and then have the clk be enabled based on runtime PM of the device (and if runtime PM is disabled then just enable it at driver probe time).
On Sat, Jul 31, 2021 at 10:41 AM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Russell King (Oracle) (2021-07-28 13:40:34) > > > I adapted the Subject in the hope to catch Stephen's and Michael's > > > attention. My impression is that this thread isn't on their radar yet, > > > but the topic here seems important enough to get a matching Subject. > I still wonder if it would be better if we had some sort of DT binding > that said "turn this clk on when the driver probes this device and keep > it on until the driver is unbound". DT is not the only way the clocks are established in the kernel. > That would probably work well for > quite a few drivers that don't want to ever call clk_get() or > clk_prepare_enable() and could tie into the assigned-clock-rates > property in a way that let us keep the platform details out of the > drivers. > We could also go one step further and make a clk pm domain when > this new property is present and then have the clk be enabled based on > runtime PM of the device (and if runtime PM is disabled then just enable > it at driver probe time). This sounds like a good idea from a usage perspective. -- With Best Regards, Andy Shevchenko
Hi Russell, hi Stephen, On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote: > Quoting Russell King (Oracle) (2021-07-28 13:40:34) > > > I adapted the Subject in the hope to catch Stephen's and Michael's > > > attention. My impression is that this thread isn't on their radar yet, > > > but the topic here seems important enough to get a matching Subject. > > The thread is on my radar but... > > > > > Have you thought about sending your pull request to the clk API > > maintainer (iow, me) ? I wasn't really aware that Russell has the clk API hat (or that this separate hat actually exists and this isn't purely a CCF topic). I assume I only did $ scripts/get_maintainer.pl -f drivers/clk/clk-devres.c which is where the current and new code implementing devm_clk_get et al lives. @Russell: What is your position here, do you like the approach of devm_clk_get_enabled? I can send a new pull request in your direction if you like it and are willing to take it. > +1 This patch doesn't fall under CCF maintainer. Given that CCF is the only implementer of devm_clk_get at least an Ack from your side would still be good I guess? > Finally, this sort of patch has been discussed for years and I didn't > see any mention of those previous attempts in the patch series. Has > something changed since that time? I think we've got a bunch of hand > rolled devm things in the meantime but not much else. I found a patch set adding devm variants of clk_enable (e.g. https://lore.kernel.org/patchwork/patch/755667/) but this approach is different as it also contains clk_get which IMHO makes more sense The discussion considered wrapping get+enable at one point, but I didn't find a followup. > I still wonder if it would be better if we had some sort of DT binding > that said "turn this clk on when the driver probes this device and keep > it on until the driver is unbound". This doesn't sound like a hardware property and so I don't think this belongs into DT and I would be surprised if the dt maintainers would be willing to accept an idea with this semantic. > That would probably work well for quite a few drivers that don't want > to ever call clk_get() or clk_prepare_enable() and could tie into the > assigned-clock-rates property in a way that let us keep the platform > details out of the drivers. We could also go one step further and make > a clk pm domain when this new property is present and then have the > clk be enabled based on runtime PM of the device (and if runtime PM is > disabled then just enable it at driver probe time). clk pm domain sounds good, but introducing devm_clk_get_enabled() is much easier and converting to it can be done without dt changes and more or less mechanically. So I consider the cost-usage-value of devm_clk_get_enabled() much better. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On Sat, 31 Jul 2021 14:00:04 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hi Russell, hi Stephen, > > On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote: > > Quoting Russell King (Oracle) (2021-07-28 13:40:34) > > > > I adapted the Subject in the hope to catch Stephen's and Michael's > > > > attention. My impression is that this thread isn't on their radar yet, > > > > but the topic here seems important enough to get a matching Subject. > > > > The thread is on my radar but... > > > > > > > > Have you thought about sending your pull request to the clk API > > > maintainer (iow, me) ? > > I wasn't really aware that Russell has the clk API hat (or that this > separate hat actually exists and this isn't purely a CCF topic). I > assume I only did > > $ scripts/get_maintainer.pl -f drivers/clk/clk-devres.c > > which is where the current and new code implementing devm_clk_get et al > lives. > > @Russell: What is your position here, do you like the approach of > devm_clk_get_enabled? I can send a new pull request in your direction if > you like it and are willing to take it. > > > +1 This patch doesn't fall under CCF maintainer. > > Given that CCF is the only implementer of devm_clk_get at least an Ack > from your side would still be good I guess? > > > Finally, this sort of patch has been discussed for years and I didn't > > see any mention of those previous attempts in the patch series. Has > > something changed since that time? I think we've got a bunch of hand > > rolled devm things in the meantime but not much else. > > I found a patch set adding devm variants of clk_enable (e.g. > https://lore.kernel.org/patchwork/patch/755667/) but this approach is > different as it also contains clk_get which IMHO makes more sense > The discussion considered wrapping get+enable at one point, but I didn't > find a followup. > > > I still wonder if it would be better if we had some sort of DT binding > > that said "turn this clk on when the driver probes this device and keep > > it on until the driver is unbound". > > This doesn't sound like a hardware property and so I don't think this > belongs into DT and I would be surprised if the dt maintainers would be > willing to accept an idea with this semantic. Agreed. It's not unheard of to have a driver start out just enabing clock at probe and dropping it at remove. When the driver gets more sophisticated it will then manage the clock more frequently. Whilst that's often tied to runtime_pm I'm not sure it always is. Given the mess that would be involved in having a property that we need to later ignore for particular drivers, I'd keep this management explicit in the driver. This series makes that trivial to do for these easy cases. Jonathan > > > That would probably work well for quite a few drivers that don't want > > to ever call clk_get() or clk_prepare_enable() and could tie into the > > assigned-clock-rates property in a way that let us keep the platform > > details out of the drivers. We could also go one step further and make > > a clk pm domain when this new property is present and then have the > > clk be enabled based on runtime PM of the device (and if runtime PM is > > disabled then just enable it at driver probe time). > > clk pm domain sounds good, but introducing devm_clk_get_enabled() is > much easier and converting to it can be done without dt changes and more > or less mechanically. So I consider the cost-usage-value of > devm_clk_get_enabled() much better. > > Best regards > Uwe >
On Sat, Jul 31, 2021 at 02:00:04PM +0200, Uwe Kleine-König wrote: > Hi Russell, hi Stephen, > > On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote: > > +1 This patch doesn't fall under CCF maintainer. > > Given that CCF is the only implementer of devm_clk_get at least an Ack > from your side would still be good I guess? I think devm_clk_get() should not be part of CCF but should be part of the interface level - it's silly to have devm_clk_get() being CCF but not clk_get(). The same should go for the other devm wrappers around the plain clk_* interfaces. > I found a patch set adding devm variants of clk_enable (e.g. > https://lore.kernel.org/patchwork/patch/755667/) but this approach is > different as it also contains clk_get which IMHO makes more sense > The discussion considered wrapping get+enable at one point, but I didn't > find a followup. There have been several different approaches to wrapping things up, but here's a question: should we make it easier to do the lazy thing (get+enable) or should we make it easier to be power efficient? Shouldn't we be encouraging people to write power efficient drivers? > > I still wonder if it would be better if we had some sort of DT binding > > that said "turn this clk on when the driver probes this device and keep > > it on until the driver is unbound". > > This doesn't sound like a hardware property and so I don't think this > belongs into DT and I would be surprised if the dt maintainers would be > willing to accept an idea with this semantic. I really don't like that idea - enabling or disabling a clock is an implementation decision, one which can change over time. Even if a clock is required to be on for e.g. accessing device registers, we may decide that, if we want maximum power savings, to disable that clock when the device is not being used, but an initial driver implementation may not do that. If we encourage people to throw a property in DT, then the driver's runtime behaviour becomes a combination of the DT being used and the driver implementation. Let's keep that to a minimum. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hello Russell, On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote: > On Sat, Jul 31, 2021 at 02:00:04PM +0200, Uwe Kleine-König wrote: > > Hi Russell, hi Stephen, > > > > On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote: > > > +1 This patch doesn't fall under CCF maintainer. > > > > Given that CCF is the only implementer of devm_clk_get at least an Ack > > from your side would still be good I guess? > > I think devm_clk_get() should not be part of CCF but should be > part of the interface level - it's silly to have devm_clk_get() > being CCF but not clk_get(). The same should go for the other > devm wrappers around the plain clk_* interfaces. What is the practical difference between "Function X is part of CCF" and "Function X is part of the clk interface and there is only CCF who implements it"? > > I found a patch set adding devm variants of clk_enable (e.g. > > https://lore.kernel.org/patchwork/patch/755667/) but this approach is > > different as it also contains clk_get which IMHO makes more sense > > The discussion considered wrapping get+enable at one point, but I didn't > > find a followup. > > There have been several different approaches to wrapping things up, > but here's a question: should we make it easier to do the lazy thing > (get+enable) or should we make it easier to be power efficient? > Shouldn't we be encouraging people to write power efficient drivers? Yeah, sounds compelling, but I wonder if that's of practical importance. How many driver authors do you expect to lure into making a better driver just because devm_clk_get_prepared() doesn't exist? In contrast: How many drivers become simpler with devm_clk_get_prepared() and so it becomes easier to maintain them and easier to spot bugs? In the absence of devm_clk_get_prepared(), is it better that several frameworks (or drivers) open code it? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-König wrote: > Hello Russell, > > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote: > > I think devm_clk_get() should not be part of CCF but should be > > part of the interface level - it's silly to have devm_clk_get() > > being CCF but not clk_get(). The same should go for the other > > devm wrappers around the plain clk_* interfaces. > > What is the practical difference between "Function X is part of CCF" and > "Function X is part of the clk interface and there is only CCF who > implements it"? clkdev is maintained by me as part of the API, and provides clk_get() functionality for all clk implementations. To then have devm_clk_get(), which merely provides a wrapper around clk_get() adding the devm semantics being part of CCF is not sane - devm_clk_get() isn't specific to CCF or in fact any clk API implementation. > > There have been several different approaches to wrapping things up, > > but here's a question: should we make it easier to do the lazy thing > > (get+enable) or should we make it easier to be power efficient? > > Shouldn't we be encouraging people to write power efficient drivers? > > Yeah, sounds compelling, but I wonder if that's of practical importance. > How many driver authors do you expect to lure into making a better > driver just because devm_clk_get_prepared() doesn't exist? In contrast: > How many drivers become simpler with devm_clk_get_prepared() and so > it becomes easier to maintain them and easier to spot bugs? > In the absence of devm_clk_get_prepared(), is it better that several > frameworks (or drivers) open code it? It probably depends on where you stand on power management and power efficiency issues. Personally, I would like to see more effort put into drivers to make them more power efficient, and I believe in the coming years, power efficiency is going to become a big issue. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Aug 2, 2021 at 7:38 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-König wrote: > > Hello Russell, > > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote: ... > > > There have been several different approaches to wrapping things up, > > > but here's a question: should we make it easier to do the lazy thing > > > (get+enable) or should we make it easier to be power efficient? > > > Shouldn't we be encouraging people to write power efficient drivers? > > > > Yeah, sounds compelling, but I wonder if that's of practical importance. > > How many driver authors do you expect to lure into making a better > > driver just because devm_clk_get_prepared() doesn't exist? In contrast: > > How many drivers become simpler with devm_clk_get_prepared() and so > > it becomes easier to maintain them and easier to spot bugs? > > In the absence of devm_clk_get_prepared(), is it better that several > > frameworks (or drivers) open code it? > > It probably depends on where you stand on power management and power > efficiency issues. Personally, I would like to see more effort put > into drivers to make them more power efficient, and I believe in the > coming years, power efficiency is going to become a big issue. While in the ideal world I 100% agree with the approach, IRL we have to deal with constantly degrading quality of the code and instead of thinking about power management and efficiency the absence of APIs such as discussed provokes not only creating the power management inefficient code, but also memory leaks here and there. -- With Best Regards, Andy Shevchenko
On Mon, Aug 02, 2021 at 08:13:05PM +0300, Andy Shevchenko wrote: > On Mon, Aug 2, 2021 at 7:38 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > It probably depends on where you stand on power management and power > > efficiency issues. Personally, I would like to see more effort put > > into drivers to make them more power efficient, and I believe in the > > coming years, power efficiency is going to become a big issue. > > While in the ideal world I 100% agree with the approach, IRL we have > to deal with constantly degrading quality of the code and instead of > thinking about power management and efficiency the absence of APIs > such as discussed provokes not only creating the power management > inefficient code, but also memory leaks here and there. The point of my previous reply that you quoted above was to make a prediction, it wasn't a rejection of the approach. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Quoting Russell King (Oracle) (2021-08-02 02:48:10) > > > > I still wonder if it would be better if we had some sort of DT binding > > > that said "turn this clk on when the driver probes this device and keep > > > it on until the driver is unbound". > > > > This doesn't sound like a hardware property and so I don't think this > > belongs into DT and I would be surprised if the dt maintainers would be > > willing to accept an idea with this semantic. > > I really don't like that idea - enabling or disabling a clock is > an implementation decision, one which can change over time. Even > if a clock is required to be on for e.g. accessing device registers, > we may decide that, if we want maximum power savings, to disable > that clock when the device is not being used, but an initial driver > implementation may not do that. If we encourage people to throw a > property in DT, then the driver's runtime behaviour becomes a > combination of the DT being used and the driver implementation. > Let's keep that to a minimum. > I suspect that sometimes we want to express that some clk is on all the time in DT because there isn't any sort of consumer driver for it. We have fixed rate clks that sort of do that, but I'm thinking about something like a GPIO clk gate that is downstream of some clk source, and this gpio gate is enabled once at boot and then forgotten about. I suppose in this case we could have a property in the clk gpio binding that expresses this property of the hardware so it's best to not make something more generic that could be abused.
Quoting Russell King (Oracle) (2021-08-02 09:38:24) > On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-Konig wrote: > > Hello Russell, > > > > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote: > > > > There have been several different approaches to wrapping things up, > > > but here's a question: should we make it easier to do the lazy thing > > > (get+enable) or should we make it easier to be power efficient? > > > Shouldn't we be encouraging people to write power efficient drivers? > > > > Yeah, sounds compelling, but I wonder if that's of practical importance. > > How many driver authors do you expect to lure into making a better > > driver just because devm_clk_get_prepared() doesn't exist? In contrast: > > How many drivers become simpler with devm_clk_get_prepared() and so > > it becomes easier to maintain them and easier to spot bugs? > > In the absence of devm_clk_get_prepared(), is it better that several > > frameworks (or drivers) open code it? > > It probably depends on where you stand on power management and power > efficiency issues. Personally, I would like to see more effort put > into drivers to make them more power efficient, and I believe in the > coming years, power efficiency is going to become a big issue. > I agree we should put more effort into power efficiency in the kernel. I've occasionally heard from driver writers that they never will turn the clk off even in low power modes though. They feel like it's a nuisance to have to do anything with the clk framework in their driver. When I say "why not use runtime PM?" I get told that they're not turning the clk off because it needs to be on all the time, so using runtime PM makes the driver more complicated, not less, and adds no value. I think some touchscreens are this way, and watchdogs too. Looking at the drivers being converted in this series I suspect RTC is one of those sorts of devices as well. But SPI and I2C most likely could benefit from using runtime PM and so those ones don't feel appropriate to convert. Maybe this series would be more compelling if those various drivers that are hand rolling the devm action were converted to the consolidated official devm function. The truth is it's already happening in various subsystems so consolidating that logic into one place would be a win code size wise and very hard to ignore. Doing $ git grep devm_add_action | grep clk seems to catch quite a few of them.
On Tue, Aug 03, 2021 at 01:11:54AM -0700, Stephen Boyd wrote: > Quoting Russell King (Oracle) (2021-08-02 09:38:24) > > On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-Konig wrote: > > > Hello Russell, > > > > > > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote: > > > > > > There have been several different approaches to wrapping things up, > > > > but here's a question: should we make it easier to do the lazy thing > > > > (get+enable) or should we make it easier to be power efficient? > > > > Shouldn't we be encouraging people to write power efficient drivers? > > > > > > Yeah, sounds compelling, but I wonder if that's of practical importance. > > > How many driver authors do you expect to lure into making a better > > > driver just because devm_clk_get_prepared() doesn't exist? In contrast: > > > How many drivers become simpler with devm_clk_get_prepared() and so > > > it becomes easier to maintain them and easier to spot bugs? > > > In the absence of devm_clk_get_prepared(), is it better that several > > > frameworks (or drivers) open code it? > > > > It probably depends on where you stand on power management and power > > efficiency issues. Personally, I would like to see more effort put > > into drivers to make them more power efficient, and I believe in the > > coming years, power efficiency is going to become a big issue. > > > > I agree we should put more effort into power efficiency in the kernel. > I've occasionally heard from driver writers that they never will turn > the clk off even in low power modes though. They feel like it's a > nuisance to have to do anything with the clk framework in their driver. > When I say "why not use runtime PM?" I get told that they're not turning > the clk off because it needs to be on all the time, so using runtime PM > makes the driver more complicated, not less, and adds no value. I think > some touchscreens are this way, and watchdogs too. Looking at the > drivers being converted in this series I suspect RTC is one of those > sorts of devices as well. But SPI and I2C most likely could benefit from > using runtime PM and so those ones don't feel appropriate to convert. > > Maybe this series would be more compelling if those various drivers that > are hand rolling the devm action were converted to the consolidated > official devm function. The truth is it's already happening in various > subsystems so consolidating that logic into one place would be a win > code size wise and very hard to ignore. > > Doing > > $ git grep devm_add_action | grep clk > > seems to catch quite a few of them. Another upside is that grepping for these drivers with a potential for further improvement become easier to grep for as devm_clk_get_{prepared,enabled} is a much better hint :-) The changes to these drivers probably won't go through a clk tree, so adding these patches before adding devm_clk_get_enabled() would only help for the warm and cozy feeling that it is right to do so, correct? As my focus is limited to (mostly) drivers/pwm and I already have quite some other patch quests on my list: So can I lure you in merging the new functions and I will create a kernel janitor task to convert more existing drivers? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Quoting Uwe Kleine-König (2021-08-03 03:40:12) > On Tue, Aug 03, 2021 at 01:11:54AM -0700, Stephen Boyd wrote: > > > > Maybe this series would be more compelling if those various drivers that > > are hand rolling the devm action were converted to the consolidated > > official devm function. The truth is it's already happening in various > > subsystems so consolidating that logic into one place would be a win > > code size wise and very hard to ignore. > > > > Doing > > > > $ git grep devm_add_action | grep clk > > > > seems to catch quite a few of them. > > Another upside is that grepping for these drivers with a potential for > further improvement become easier to grep for as > devm_clk_get_{prepared,enabled} is a much better hint :-) Sorry, but that's a pretty weak argument. I'd think grepping for the absence of pm_ops in drivers would be the same hint. > > The changes to these drivers probably won't go through a clk tree, so > adding these patches before adding devm_clk_get_enabled() would only > help for the warm and cozy feeling that it is right to do so, correct? It isn't to feel warm and cozy. It's to demonstrate the need for consolidating code. Converting the i2c and spi drivers to use this is actively damaging the cause though. Those driver frameworks are more likely to encourage proper power management around bus transfers, so converting them to use the devm API moves them away from power management, not closer to it. This proves why this topic is always contentious. It's too easy to blindly convert drivers to get the clk and leave it enabled forever and then they never use power management. The janitors win and nobody else. Is there some way to avoid that trap? Maybe through some combination of a device PM function that indicates the driver has no runtime PM callbacks (pm_runtime_no_callbacks() perhaps?) and devm_clk_get_enabled() checking for that and returning the clk only when that call has been made (i.e. pm_runtime_has_no_callbacks())? This approach would fail to catch the case where system wide suspend/resume could turn the clk off but the driver doesn't do it. I'm not sure how much we care about that case though. > > As my focus is limited to (mostly) drivers/pwm and I already have quite > some other patch quests on my list: Don't we all? :)
On Thu, Aug 05, 2021 at 05:26:16PM -0700, Stephen Boyd wrote: > Quoting Uwe Kleine-König (2021-08-03 03:40:12) > > On Tue, Aug 03, 2021 at 01:11:54AM -0700, Stephen Boyd wrote: > > > > > > Maybe this series would be more compelling if those various drivers that > > > are hand rolling the devm action were converted to the consolidated > > > official devm function. The truth is it's already happening in various > > > subsystems so consolidating that logic into one place would be a win > > > code size wise and very hard to ignore. > > > > > > Doing > > > > > > $ git grep devm_add_action | grep clk > > > > > > seems to catch quite a few of them. Will do. > > Another upside is that grepping for these drivers with a potential for > > further improvement become easier to grep for as > > devm_clk_get_{prepared,enabled} is a much better hint :-) > > Sorry, but that's a pretty weak argument. I'd think grepping for the > absence of pm_ops in drivers would be the same hint. To be honest: Yes, it's a weak argument, but grepping for drivers without pm_ops is a tad more complicated and yields a different set of drivers. For example take the i2c-imx driver (drivers/i2c/busses/i2c-imx.c) which has a pm_ops but still can make use of devm_clk_get_enabled. > > The changes to these drivers probably won't go through a clk tree, so > > adding these patches before adding devm_clk_get_enabled() would only > > help for the warm and cozy feeling that it is right to do so, correct? > > It isn't to feel warm and cozy. It's to demonstrate the need for > consolidating code. Converting the i2c and spi drivers to use this is > actively damaging the cause though. Those driver frameworks are more > likely to encourage proper power management around bus transfers, so > converting them to use the devm API moves them away from power > management, not closer to it. Well I think one could disagree here. Today these drivers are not power efficient as they just enable the clock in their probe routine and keep it on even though it might not be needed. My patch still is beneficial as it simplifies the drivers without making them worse. Agreed, this isn't the best optimisation to the drivers (assuming it is possible to disable the clocks while the device isn't in use). > This proves why this topic is always contentious. It's too easy to > blindly convert drivers to get the clk and leave it enabled forever and > then they never use power management. The janitors win and nobody else. If the janitors win and nobody else looses anything, this is fine for me. And if in the future someone turns up who cares enough to improve the converted drivers to a more efficient clock usage, they will probably not stop their efforts just because then the driver uses devm_clk_get_enabled. > Is there some way to avoid that trap? Maybe through some combination of > a device PM function that indicates the driver has no runtime PM > callbacks (pm_runtime_no_callbacks() perhaps?) and > devm_clk_get_enabled() checking for that and returning the clk only when > that call has been made (i.e. pm_runtime_has_no_callbacks())? This > approach would fail to catch the case where system wide suspend/resume > could turn the clk off but the driver doesn't do it. I'm not sure how > much we care about that case though. > > > As my focus is limited to (mostly) drivers/pwm and I already have quite > > some other patch quests on my list: > > Don't we all? :) Might be. The patches in your queue are however not a reason to drop my efforts to make my queue shorter :-P Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Sep 14, 2021 at 03:22:56PM +0200, Uwe Kleine-König wrote: > On Thu, Aug 05, 2021 at 05:26:16PM -0700, Stephen Boyd wrote: > > This proves why this topic is always contentious. It's too easy to > > blindly convert drivers to get the clk and leave it enabled forever and > > then they never use power management. The janitors win and nobody else. > If the janitors win and nobody else looses anything, this is fine for > me. And if in the future someone turns up who cares enough to improve > the converted drivers to a more efficient clock usage, they will > probably not stop their efforts just because then the driver uses > devm_clk_get_enabled. The patterns that concern me are people either blindly converting to devm without checking for other usage and causing problems as a result (some of the janitorial stuff is done very mechanically) or thinking it's important to keep devm_ used (or not thinking through the interaction) and trying to do that while doing something more active.