[PATCH v7 0/6] clk: provide new devm helpers for prepared and enabled clocks

[PATCH v7 0/6] clk: provide new devm helpers for prepared and enabled clocks

[Uwe Kleine-König]

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.

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

[PATCH v7 1/6] clk: generalize devm_clk_get() a bit

[Uwe Kleine-König]

Allow to add an exit hook to devm managed clocks. Also use
clk_get_optional() in devm_clk_get_optional instead of open coding it.
The generalisation will be used in the next commit to add some more
devm_clk helpers.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk-devres.c | 67 ++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index be160764911b..91c995815b57 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -4,39 +4,72 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 
+struct devm_clk_state {
+	struct clk *clk;
+	void (*exit)(struct clk *clk);
+};
+
 static void devm_clk_release(struct device *dev, void *res)
 {
-	clk_put(*(struct clk **)res);
+	struct devm_clk_state *state = *(struct devm_clk_state **)res;
+
+	if (state->exit)
+		state->exit(state->clk);
+
+	clk_put(state->clk);
 }
 
-struct clk *devm_clk_get(struct device *dev, const char *id)
+static struct clk *__devm_clk_get(struct device *dev, const char *id,
+				  struct clk *(*get)(struct device *dev, const char *id),
+				  int (*init)(struct clk *clk),
+				  void (*exit)(struct clk *clk))
 {
-	struct clk **ptr, *clk;
+	struct devm_clk_state *state;
+	struct clk *clk;
+	int ret;
 
-	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
+	state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
+	if (!state)
 		return ERR_PTR(-ENOMEM);
 
-	clk = clk_get(dev, id);
-	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
+	clk = get(dev, id);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto err_clk_get;
 	}
 
+	if (init) {
+		ret = init(clk);
+		if (ret)
+			goto err_clk_init;
+	}
+
+	state->clk = clk;
+	state->exit = exit;
+
+	devres_add(dev, state);
+
 	return clk;
+
+err_clk_init:
+
+	clk_put(clk);
+err_clk_get:
+
+	devres_free(state);
+	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL(devm_clk_get);
 
-struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-	struct clk *clk = devm_clk_get(dev, id);
+	return __devm_clk_get(dev, id, clk_get, NULL, NULL);
 
-	if (clk == ERR_PTR(-ENOENT))
-		return NULL;
+}
+EXPORT_SYMBOL(devm_clk_get);
 
-	return clk;
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get_optional);
 
-- 
2.30.2

[PATCH v7 2/6] clk: Provide new devm_clk_helpers for prepared and enabled clocks

[Uwe Kleine-König]

When a driver keeps a clock prepared (or enabled) during the whole
lifetime of the driver, these helpers allow to simplify the drivers.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk-devres.c | 31 ++++++++++++++
 include/linux/clk.h      | 90 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 91c995815b57..b54f7f0f2a35 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -67,12 +67,43 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk *devm_clk_get_prepared(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get, clk_prepare, clk_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_prepared);
+
+struct clk *devm_clk_get_enabled(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get,
+			      clk_prepare_enable, clk_disable_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_enabled);
+
 struct clk *devm_clk_get_optional(struct device *dev, const char *id)
 {
 	return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get_optional);
 
+struct clk *devm_clk_get_optional_prepared(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get_optional,
+			      clk_prepare, clk_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_optional_prepared);
+
+struct clk *devm_clk_get_optional_enabled(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get_optional,
+			      clk_prepare_enable, clk_disable_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_optional_enabled);
+
 struct clk_bulk_devres {
 	struct clk_bulk_data *clks;
 	int num_clks;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..b011dbba7109 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -449,7 +449,7 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  * the clock producer.  (IOW, @id may be identical strings, but
  * clk_get may return different clock producers depending on @dev.)
  *
- * Drivers must assume that the clock source is not enabled.
+ * Drivers must assume that the clock source is neither prepared nor enabled.
  *
  * devm_clk_get should not be called from within interrupt context.
  *
@@ -458,6 +458,47 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  */
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
+/**
+ * devm_clk_get_prepared - devm_clk_get() + clk_prepare()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno.  The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer.  (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * The returned clk (if valid) is prepared. Drivers must however assume that the
+ * clock is not enabled.
+ *
+ * devm_clk_get_prepared should not be called from within interrupt context.
+ *
+ * The clock will automatically be unprepared and freed when the
+ * device is unbound from the bus.
+ */
+struct clk *devm_clk_get_prepared(struct device *dev, const char *id);
+
+/**
+ * devm_clk_get_enabled - devm_clk_get() + clk_prepare_enable()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or valid IS_ERR()
+ * condition containing errno.  The implementation uses @dev and @id to
+ * determine the clock consumer, and thereby the clock producer.  (IOW, @id may
+ * be identical strings, but clk_get may return different clock producers
+ * depending on @dev.)
+ *
+ * The returned clk (if valid) is prepared and enabled.
+ *
+ * devm_clk_get_prepared should not be called from within interrupt context.
+ *
+ * The clock will automatically be disabled, unprepared and freed when the
+ * device is unbound from the bus.
+ */
+struct clk *devm_clk_get_enabled(struct device *dev, const char *id);
+
 /**
  * devm_clk_get_optional - lookup and obtain a managed reference to an optional
  *			   clock producer.
@@ -469,6 +510,29 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
  */
 struct clk *devm_clk_get_optional(struct device *dev, const char *id);
 
+/**
+ * devm_clk_get_optional_prepared - devm_clk_get_optional() + clk_prepare()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as devm_clk_get_prepared() except where there is no clock
+ * producer.  In this case, instead of returning -ENOENT, the function returns
+ * NULL.
+ */
+struct clk *devm_clk_get_optional_prepared(struct device *dev, const char *id);
+
+/**
+ * devm_clk_get_optional_enabled - devm_clk_get_optional() +
+ *                                 clk_prepare_enable()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as devm_clk_get_enabled() except where there is no clock
+ * producer.  In this case, instead of returning -ENOENT, the function returns
+ * NULL.
+ */
+struct clk *devm_clk_get_optional_enabled(struct device *dev, const char *id);
+
 /**
  * devm_get_clk_from_child - lookup and obtain a managed reference to a
  *			     clock producer from child node.
@@ -813,12 +877,36 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline struct clk *devm_clk_get_prepared(struct device *dev,
+						const char *id)
+{
+	return NULL;
+}
+
+static inline struct clk *devm_clk_get_enabled(struct device *dev,
+					       const char *id)
+{
+	return NULL;
+}
+
 static inline struct clk *devm_clk_get_optional(struct device *dev,
 						const char *id)
 {
 	return NULL;
 }
 
+static inline struct clk *devm_clk_get_optional_prepared(struct device *dev,
+							 const char *id)
+{
+	return NULL;
+}
+
+static inline struct clk *devm_clk_get_optional_enabled(struct device *dev,
+							const char *id)
+{
+	return NULL;
+}
+
 static inline int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
 						 struct clk_bulk_data *clks)
 {
-- 
2.30.2

[PATCH v7 4/6] rtc: at91sam9: Simplify using devm_clk_get_enabled()

[Uwe Kleine-König]

devm_clk_get_enabled() returns the clk already (prepared and) enabled
and the automatically called cleanup cares for disabling (and
unpreparing). So simplify .probe() and .remove() accordingly.

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-at91sam9.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c
index 2216be429ab7..b52e7bd26303 100644
--- a/drivers/rtc/rtc-at91sam9.c
+++ b/drivers/rtc/rtc-at91sam9.c
@@ -374,21 +374,14 @@ static int at91_rtc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	rtc->sclk = devm_clk_get(&pdev->dev, NULL);
+	rtc->sclk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(rtc->sclk))
 		return PTR_ERR(rtc->sclk);
 
-	ret = clk_prepare_enable(rtc->sclk);
-	if (ret) {
-		dev_err(&pdev->dev, "Could not enable slow clock\n");
-		return ret;
-	}
-
 	sclk_rate = clk_get_rate(rtc->sclk);
 	if (!sclk_rate || sclk_rate > AT91_RTT_RTPRES) {
 		dev_err(&pdev->dev, "Invalid slow clock rate\n");
-		ret = -EINVAL;
-		goto err_clk;
+		return -EINVAL;
 	}
 
 	mr = rtt_readl(rtc, MR);
@@ -406,7 +399,7 @@ static int at91_rtc_probe(struct platform_device *pdev)
 	rtc->rtcdev = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc->rtcdev)) {
 		ret = PTR_ERR(rtc->rtcdev);
-		goto err_clk;
+		return ret;
 	}
 
 	rtc->rtcdev->ops = &at91_rtc_ops;
@@ -418,7 +411,7 @@ static int at91_rtc_probe(struct platform_device *pdev)
 			       dev_name(&rtc->rtcdev->dev), rtc);
 	if (ret) {
 		dev_dbg(&pdev->dev, "can't share IRQ %d?\n", rtc->irq);
-		goto err_clk;
+		return ret;
 	}
 
 	/* NOTE:  sam9260 rev A silicon has a ROM bug which resets the
@@ -432,11 +425,6 @@ static int at91_rtc_probe(struct platform_device *pdev)
 			 dev_name(&rtc->rtcdev->dev));
 
 	return devm_rtc_register_device(rtc->rtcdev);
-
-err_clk:
-	clk_disable_unprepare(rtc->sclk);
-
-	return ret;
 }
 
 /*
@@ -450,8 +438,6 @@ static int at91_rtc_remove(struct platform_device *pdev)
 	/* disable all interrupts */
 	rtt_writel(rtc, MR, mr & ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN));
 
-	clk_disable_unprepare(rtc->sclk);
-
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH v7 0/6] clk: provide new devm helpers for prepared and enabled clocks

[Alexandru Ardelean]

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
>

Re: [PATCH v7 0/6] clk: provide new devm helpers for prepared and enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

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(-)

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]


> 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]


> > 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3350 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Claudiu.Beznea]

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/ |
> 

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Andy Shevchenko]

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

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Andy Shevchenko]

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

Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

Hello,

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.

On Mon, Jul 26, 2021 at 09:18:16AM +0000, Claudiu.Beznea@microchip.com wrote:
> > On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote:
> >> On Thursday, July 22, 2021, Wolfram Sang <wsa@kernel.org> wrote:
> >>>>>> [ some frustration for not getting feedback for clk patches ]
> >>>>> 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?
> 
> 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.

Wondering about how we can progress here I think it's crucial that
Stephen and/or Michael share their thoughts about how they intend to
care for drivers/clk in the future.

Do you want to keep the maintainer post long-term? Or only for a
transitional period until someone else can take care? Is your
non-presence only temporal and is it foreseeable that you will increase
your efforts in the next weeks/months again? Do you welcome a
co-maintainer? What kind of involvement would you consider helpful?

Thanks to Claudiu for offering to support here, at least from my side
this is very appreciated.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Russell King (Oracle)]

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!

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Stephen Boyd]

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).

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Andy Shevchenko]

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

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3051 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Jonathan Cameron]

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
> 

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Russell King (Oracle)]

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!

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Russell King (Oracle)]

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!

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Andy Shevchenko]

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

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Russell King (Oracle)]

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!

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Stephen Boyd]

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.

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3481 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Stephen Boyd]

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? :)

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3822 bytes --]

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Stephen Boyd]

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.