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

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

[Uwe Kleine-König]

Hello,

after Stephen signaled to accept the idea, this is a rework of v8[1] with
the following changes:

 - Drop the follow up conversions. I will resend them individually per
   subsystem once the preconditions from this v9 are in. I only kept one
   clk patch that will go in via the clk tree anyhow. I trimmed the Cc:
   list accordingly.

 - (trivially) rebased to v5.18-rc1

 - Introduce a new commit that first improves the documention of
   devm_clk_get() and devm_clk_get_optional() before (mostly)
   duplicating these for the new functions.

 - Make the new functions use a GPL export. (Note the existing functions
   use a plain export, I didn't change that.)

 - Drop a bogus empty line that was cut-n-pasted into several functions.

Thanks for feedback by Stephen and Jonathan.

@Russell: Stephen wrote in v8: "I'm largely waiting for Russell to ack
the clk.h change [...]". Would be great if you looked at the series and
tell us your thoughts.

@Stephen: You asked to add the acks from v8. There were however only
acks for the followup conversions. So no changes here.
(Andy Shevchenko replied to patch 0, but his Reviewed-by: only affected
the patch "iio: Make use of devm_clk_get_enabled()".)

Best regards
Uwe

[1] https://lore.kernel.org/linux-clk/20220314141643.22184-1-u.kleine-koenig@pengutronix.de

Uwe Kleine-König (4):
  clk: Improve documentation for devm_clk_get() and its optional variant
  clk: generalize devm_clk_get() a bit
  clk: Provide new devm_clk helpers for prepared and enabled clocks
  clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()

 drivers/clk/clk-devres.c      |  91 +++++++++++++++++++----
 drivers/clk/meson/axg-audio.c |  36 +--------
 include/linux/clk.h           | 134 ++++++++++++++++++++++++++++++++--
 3 files changed, 207 insertions(+), 54 deletions(-)


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

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

[Uwe Kleine-König]

Hello,

after Stephen signaled to accept the idea, this is a rework of v8[1] with
the following changes:

 - Drop the follow up conversions. I will resend them individually per
   subsystem once the preconditions from this v9 are in. I only kept one
   clk patch that will go in via the clk tree anyhow. I trimmed the Cc:
   list accordingly.

 - (trivially) rebased to v5.18-rc1

 - Introduce a new commit that first improves the documention of
   devm_clk_get() and devm_clk_get_optional() before (mostly)
   duplicating these for the new functions.

 - Make the new functions use a GPL export. (Note the existing functions
   use a plain export, I didn't change that.)

 - Drop a bogus empty line that was cut-n-pasted into several functions.

Thanks for feedback by Stephen and Jonathan.

@Russell: Stephen wrote in v8: "I'm largely waiting for Russell to ack
the clk.h change [...]". Would be great if you looked at the series and
tell us your thoughts.

@Stephen: You asked to add the acks from v8. There were however only
acks for the followup conversions. So no changes here.
(Andy Shevchenko replied to patch 0, but his Reviewed-by: only affected
the patch "iio: Make use of devm_clk_get_enabled()".)

Best regards
Uwe

[1] https://lore.kernel.org/linux-clk/20220314141643.22184-1-u.kleine-koenig@pengutronix.de

Uwe Kleine-König (4):
  clk: Improve documentation for devm_clk_get() and its optional variant
  clk: generalize devm_clk_get() a bit
  clk: Provide new devm_clk helpers for prepared and enabled clocks
  clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()

 drivers/clk/clk-devres.c      |  91 +++++++++++++++++++----
 drivers/clk/meson/axg-audio.c |  36 +--------
 include/linux/clk.h           | 134 ++++++++++++++++++++++++++++++++--
 3 files changed, 207 insertions(+), 54 deletions(-)


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v9 1/4] clk: Improve documentation for devm_clk_get() and its optional variant

[Uwe Kleine-König]

Make use of "Context:" and "Return:". Mention that the clk is not to be
expected to be prepared, previously only not being enabled was mentioned
which probably dates from the times when the concept of clk preparation
wasn't invented yet.

Also describe devm_clk_get_optional() fully instead of just referencing
devm_clk_get().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/linux/clk.h | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 39faa54efe88..c8fc398d2ad7 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -443,15 +443,16 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  * @dev: device for clock "consumer"
  * @id: clock consumer ID
  *
- * Returns a struct clk corresponding to the clock producer, or
+ * Context: May sleep.
+ *
+ * Return: 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.)
  *
- * Drivers must assume that the clock source is not enabled.
- *
- * devm_clk_get should not be called from within interrupt context.
+ * Drivers must assume that the clock source is neither prepared nor
+ * enabled.
  *
  * The clock will automatically be freed when the device is unbound
  * from the bus.
@@ -464,8 +465,20 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
  * @dev: device for clock "consumer"
  * @id: clock consumer ID
  *
- * Behaves the same as devm_clk_get() except where there is no clock producer.
- * In this case, instead of returning -ENOENT, the function returns NULL.
+ * Context: May sleep.
+ *
+ * Return: 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.  If no such clk is found, it returns NULL
+ * which serves as a dummy clk.  That's the only difference compared
+ * to devm_clk_get().
+ *
+ * Drivers must assume that the clock source is neither prepared nor
+ * enabled.
+ *
+ * The clock will automatically be freed when the device is unbound
+ * from the bus.
  */
 struct clk *devm_clk_get_optional(struct device *dev, const char *id);
 
-- 
2.35.1

[PATCH v9 2/4] 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>
Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk-devres.c | 66 +++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index f9d5b7334341..c822f4ef1584 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -4,39 +4,71 @@
 #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);
+}
+
+struct clk *devm_clk_get(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get);
 
 struct clk *devm_clk_get_optional(struct device *dev, const char *id)
 {
-	struct clk *clk = devm_clk_get(dev, id);
-
-	if (clk == ERR_PTR(-ENOENT))
-		return NULL;
-
-	return clk;
+	return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get_optional);
 
-- 
2.35.1

[PATCH v9 3/4] 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>
Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk-devres.c |  27 ++++++++++
 include/linux/clk.h      | 109 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index c822f4ef1584..43ccd20e0298 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -66,12 +66,39 @@ 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_GPL(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_GPL(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_GPL(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_GPL(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 c8fc398d2ad7..c13061cabdfc 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -459,6 +459,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
+ *
+ * Context: May sleep.
+ *
+ * Return: 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.
+ *
+ * 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
+ *
+ * Context: May sleep.
+ *
+ * Return: 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.
+ *
+ * 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.
@@ -482,6 +523,50 @@ 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
+ *
+ * Context: May sleep.
+ *
+ * Return: 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.  If no such clk is found, it returns NULL
+ * which serves as a dummy clk.  That's the only difference compared
+ * to devm_clk_get_prepared().
+ *
+ * The returned clk (if valid) is prepared. Drivers must however
+ * assume that the clock is not enabled.
+ *
+ * The clock will automatically be unprepared and freed when the
+ * device is unbound from the bus.
+ */
+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
+ *
+ * Context: May sleep.
+ *
+ * Return: 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.  If no such clk is found, it returns NULL
+ * which serves as a dummy clk.  That's the only difference compared
+ * to devm_clk_get_enabled().
+ *
+ * The returned clk (if valid) is prepared and enabled.
+ *
+ * The clock will automatically be disabled, unprepared and freed
+ * when the device is unbound from the bus.
+ */
+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.
@@ -826,12 +911,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.35.1

[PATCH v9 4/4] clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()

[Uwe Kleine-König]

The clk API just got a function with a slightly different name and
the same functionality. Remove the duplication.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/meson/axg-audio.c | 36 ++++-------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
index bfe36bd41339..5016682e47c8 100644
--- a/drivers/clk/meson/axg-audio.c
+++ b/drivers/clk/meson/axg-audio.c
@@ -1657,35 +1657,6 @@ static struct clk_regmap *const sm1_clk_regmaps[] = {
 	&sm1_sysclk_b_en,
 };
 
-static int devm_clk_get_enable(struct device *dev, char *id)
-{
-	struct clk *clk;
-	int ret;
-
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		dev_err_probe(dev, ret, "failed to get %s", id);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "failed to enable %s", id);
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(dev,
-				       (void(*)(void *))clk_disable_unprepare,
-				       clk);
-	if (ret) {
-		dev_err(dev, "failed to add reset action on %s", id);
-		return ret;
-	}
-
-	return 0;
-}
-
 struct axg_audio_reset_data {
 	struct reset_controller_dev rstc;
 	struct regmap *map;
@@ -1787,6 +1758,7 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)
 	struct regmap *map;
 	void __iomem *regs;
 	struct clk_hw *hw;
+	struct clk *clk;
 	int ret, i;
 
 	data = of_device_get_match_data(dev);
@@ -1804,9 +1776,9 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)
 	}
 
 	/* Get the mandatory peripheral clock */
-	ret = devm_clk_get_enable(dev, "pclk");
-	if (ret)
-		return ret;
+	clk = devm_clk_get_enabled(dev, "pclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	ret = device_reset(dev);
 	if (ret) {
-- 
2.35.1

[PATCH v9 4/4] clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()

[Uwe Kleine-König]

The clk API just got a function with a slightly different name and
the same functionality. Remove the duplication.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/meson/axg-audio.c | 36 ++++-------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
index bfe36bd41339..5016682e47c8 100644
--- a/drivers/clk/meson/axg-audio.c
+++ b/drivers/clk/meson/axg-audio.c
@@ -1657,35 +1657,6 @@ static struct clk_regmap *const sm1_clk_regmaps[] = {
 	&sm1_sysclk_b_en,
 };
 
-static int devm_clk_get_enable(struct device *dev, char *id)
-{
-	struct clk *clk;
-	int ret;
-
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk)) {
-		ret = PTR_ERR(clk);
-		dev_err_probe(dev, ret, "failed to get %s", id);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "failed to enable %s", id);
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(dev,
-				       (void(*)(void *))clk_disable_unprepare,
-				       clk);
-	if (ret) {
-		dev_err(dev, "failed to add reset action on %s", id);
-		return ret;
-	}
-
-	return 0;
-}
-
 struct axg_audio_reset_data {
 	struct reset_controller_dev rstc;
 	struct regmap *map;
@@ -1787,6 +1758,7 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)
 	struct regmap *map;
 	void __iomem *regs;
 	struct clk_hw *hw;
+	struct clk *clk;
 	int ret, i;
 
 	data = of_device_get_match_data(dev);
@@ -1804,9 +1776,9 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)
 	}
 
 	/* Get the mandatory peripheral clock */
-	ret = devm_clk_get_enable(dev, "pclk");
-	if (ret)
-		return ret;
+	clk = devm_clk_get_enabled(dev, "pclk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	ret = device_reset(dev);
 	if (ret) {
-- 
2.35.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1696 bytes --]

Hello,

On Fri, May 20, 2022 at 09:57:33AM +0200, Uwe Kleine-König wrote:
> after Stephen signaled to accept the idea, this is a rework of v8[1] with
> the following changes:
> 
>  - Drop the follow up conversions. I will resend them individually per
>    subsystem once the preconditions from this v9 are in. I only kept one
>    clk patch that will go in via the clk tree anyhow. I trimmed the Cc:
>    list accordingly.
> 
>  - (trivially) rebased to v5.18-rc1
> 
>  - Introduce a new commit that first improves the documention of
>    devm_clk_get() and devm_clk_get_optional() before (mostly)
>    duplicating these for the new functions.
> 
>  - Make the new functions use a GPL export. (Note the existing functions
>    use a plain export, I didn't change that.)
> 
>  - Drop a bogus empty line that was cut-n-pasted into several functions.
> 
> Thanks for feedback by Stephen and Jonathan.
> 
> @Russell: Stephen wrote in v8: "I'm largely waiting for Russell to ack
> the clk.h change [...]". Would be great if you looked at the series and
> tell us your thoughts.

That didn't happen :-\

> @Stephen: You asked to add the acks from v8. There were however only
> acks for the followup conversions. So no changes here.
> (Andy Shevchenko replied to patch 0, but his Reviewed-by: only affected
> the patch "iio: Make use of devm_clk_get_enabled()".)

Hmm, I somehow expected this series to go into v5.19-rc1, but it didn't
:-\. Is there anything still needed from my side?

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Uwe Kleine-König]

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

Hello,

On Fri, May 20, 2022 at 09:57:33AM +0200, Uwe Kleine-König wrote:
> after Stephen signaled to accept the idea, this is a rework of v8[1] with
> the following changes:
> 
>  - Drop the follow up conversions. I will resend them individually per
>    subsystem once the preconditions from this v9 are in. I only kept one
>    clk patch that will go in via the clk tree anyhow. I trimmed the Cc:
>    list accordingly.
> 
>  - (trivially) rebased to v5.18-rc1
> 
>  - Introduce a new commit that first improves the documention of
>    devm_clk_get() and devm_clk_get_optional() before (mostly)
>    duplicating these for the new functions.
> 
>  - Make the new functions use a GPL export. (Note the existing functions
>    use a plain export, I didn't change that.)
> 
>  - Drop a bogus empty line that was cut-n-pasted into several functions.
> 
> Thanks for feedback by Stephen and Jonathan.
> 
> @Russell: Stephen wrote in v8: "I'm largely waiting for Russell to ack
> the clk.h change [...]". Would be great if you looked at the series and
> tell us your thoughts.

That didn't happen :-\

> @Stephen: You asked to add the acks from v8. There were however only
> acks for the followup conversions. So no changes here.
> (Andy Shevchenko replied to patch 0, but his Reviewed-by: only affected
> the patch "iio: Make use of devm_clk_get_enabled()".)

Hmm, I somehow expected this series to go into v5.19-rc1, but it didn't
:-\. Is there anything still needed from my side?

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: [PATCH v9 0/4] clk: provide new devm helpers for prepared and enabled clocks

[Russell King (Oracle)]

On Tue, Jun 07, 2022 at 01:08:16PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, May 20, 2022 at 09:57:33AM +0200, Uwe Kleine-König wrote:
> > after Stephen signaled to accept the idea, this is a rework of v8[1] with
> > the following changes:
> > 
> >  - Drop the follow up conversions. I will resend them individually per
> >    subsystem once the preconditions from this v9 are in. I only kept one
> >    clk patch that will go in via the clk tree anyhow. I trimmed the Cc:
> >    list accordingly.
> > 
> >  - (trivially) rebased to v5.18-rc1
> > 
> >  - Introduce a new commit that first improves the documention of
> >    devm_clk_get() and devm_clk_get_optional() before (mostly)
> >    duplicating these for the new functions.
> > 
> >  - Make the new functions use a GPL export. (Note the existing functions
> >    use a plain export, I didn't change that.)
> > 
> >  - Drop a bogus empty line that was cut-n-pasted into several functions.
> > 
> > Thanks for feedback by Stephen and Jonathan.
> > 
> > @Russell: Stephen wrote in v8: "I'm largely waiting for Russell to ack
> > the clk.h change [...]". Would be great if you looked at the series and
> > tell us your thoughts.
> 
> That didn't happen :-\

Sorry, I missed this patch set. I haven't been reading lakml very much
for *all* of May - commitments have meant I haven't had very little
time to look at stuff on the mailing list, and due to the volume of
email, I don't do "catch up" anymore.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Tue, Jun 07, 2022 at 01:08:16PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, May 20, 2022 at 09:57:33AM +0200, Uwe Kleine-König wrote:
> > after Stephen signaled to accept the idea, this is a rework of v8[1] with
> > the following changes:
> > 
> >  - Drop the follow up conversions. I will resend them individually per
> >    subsystem once the preconditions from this v9 are in. I only kept one
> >    clk patch that will go in via the clk tree anyhow. I trimmed the Cc:
> >    list accordingly.
> > 
> >  - (trivially) rebased to v5.18-rc1
> > 
> >  - Introduce a new commit that first improves the documention of
> >    devm_clk_get() and devm_clk_get_optional() before (mostly)
> >    duplicating these for the new functions.
> > 
> >  - Make the new functions use a GPL export. (Note the existing functions
> >    use a plain export, I didn't change that.)
> > 
> >  - Drop a bogus empty line that was cut-n-pasted into several functions.
> > 
> > Thanks for feedback by Stephen and Jonathan.
> > 
> > @Russell: Stephen wrote in v8: "I'm largely waiting for Russell to ack
> > the clk.h change [...]". Would be great if you looked at the series and
> > tell us your thoughts.
> 
> That didn't happen :-\

Sorry, I missed this patch set. I haven't been reading lakml very much
for *all* of May - commitments have meant I haven't had very little
time to look at stuff on the mailing list, and due to the volume of
email, I don't do "catch up" anymore.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 1/4] clk: Improve documentation for devm_clk_get() and its optional variant

[Russell King (Oracle)]

On Fri, May 20, 2022 at 09:57:34AM +0200, Uwe Kleine-König wrote:
> Make use of "Context:" and "Return:". Mention that the clk is not to be
> expected to be prepared, previously only not being enabled was mentioned
> which probably dates from the times when the concept of clk preparation
> wasn't invented yet.
> 
> Also describe devm_clk_get_optional() fully instead of just referencing
> devm_clk_get().
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v9 1/4] clk: Improve documentation for devm_clk_get() and its optional variant

[Stephen Boyd]

Quoting Uwe Kleine-König (2022-05-20 00:57:34)
> Make use of "Context:" and "Return:". Mention that the clk is not to be
> expected to be prepared, previously only not being enabled was mentioned
> which probably dates from the times when the concept of clk preparation
> wasn't invented yet.
> 
> Also describe devm_clk_get_optional() fully instead of just referencing
> devm_clk_get().
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Applied to clk-next

Re: [PATCH v9 2/4] clk: generalize devm_clk_get() a bit

[Stephen Boyd]

Quoting Uwe Kleine-König (2022-05-20 00:57:35)
> 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>
> Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Applied to clk-next

Re: [PATCH v9 3/4] clk: Provide new devm_clk helpers for prepared and enabled clocks

[Stephen Boyd]

Quoting Uwe Kleine-König (2022-05-20 00:57:36)
> 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>
> Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/clk/clk-devres.c |  27 ++++++++++
>  include/linux/clk.h      | 109 +++++++++++++++++++++++++++++++++++++++

I was looking for Russell's ack here so tentatively applied to clk-next.

Re: [PATCH v9 4/4] clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()

[Stephen Boyd]

Quoting Uwe Kleine-König (2022-05-20 00:57:37)
> The clk API just got a function with a slightly different name and
> the same functionality. Remove the duplication.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Applied to clk-next

Re: [PATCH v9 4/4] clk: meson: axg-audio: Don't duplicate devm_clk_get_enabled()

[Stephen Boyd]

Quoting Uwe Kleine-König (2022-05-20 00:57:37)
> The clk API just got a function with a slightly different name and
> the same functionality. Remove the duplication.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Applied to clk-next

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 2/4] clk: generalize devm_clk_get() a bit

[Marek Szyprowski]

Hi Uwe,

On 20.05.2022 09:57, Uwe Kleine-König wrote:
> 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>
> Reviewed-by: Alexandru Ardelean <aardelean@deviqon.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This patch landed in linux next-20220620 as commit abae8e57e49a ("clk: 
generalize devm_clk_get() a bit"). Unfortunately it has ugly issue, see 
my comment in the code below.

> ---
>   drivers/clk/clk-devres.c | 66 +++++++++++++++++++++++++++++-----------
>   1 file changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index f9d5b7334341..c822f4ef1584 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -4,39 +4,71 @@
>   #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;

This should be:

struct devm_clk_state *state = res;

otherwise it nukes badly during cleanup:

8<--- cut here ---
Unable to handle kernel paging request at virtual address c254f810
[c254f810] *pgd=4241141e(bad)
Internal error: Oops: 8000000d [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.19.0-rc1-00002-gabae8e57e49a #5239
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at 0xc254f810
LR is at devm_clk_release+0x1c/0x28
pc : [<c254f810>]    lr : [<c05c93f4>]    psr: a0000053
...
Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000404a  DAC: 00000051
...
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xf0845dd0 to 0xf0846000)
...
  devm_clk_release from release_nodes+0x5c/0x80
  release_nodes from devres_release_all+0x7c/0xc0
  devres_release_all from device_unbind_cleanup+0xc/0x60
  device_unbind_cleanup from really_probe+0x150/0x404
  really_probe from __driver_probe_device+0xa0/0x208
  __driver_probe_device from driver_probe_device+0x34/0xc4
  driver_probe_device from __driver_attach+0xf0/0x1e4
  __driver_attach from bus_for_each_dev+0x70/0xb0
  bus_for_each_dev from bus_add_driver+0x174/0x218
  bus_add_driver from driver_register+0x88/0x11c
  driver_register from do_one_initcall+0x64/0x380
  do_one_initcall from kernel_init_freeable+0x1c0/0x224
  kernel_init_freeable from kernel_init+0x18/0x12c
  kernel_init from ret_from_fork+0x14/0x2c
Exception stack(0xf0845fb0 to 0xf0845ff8)
...
---[ end trace 0000000000000000 ]---


> +
> +	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);
> +}
> +
> +struct clk *devm_clk_get(struct device *dev, const char *id)
> +{
> +	return __devm_clk_get(dev, id, clk_get, NULL, NULL);
>   }
>   EXPORT_SYMBOL(devm_clk_get);
>   
>   struct clk *devm_clk_get_optional(struct device *dev, const char *id)
>   {
> -	struct clk *clk = devm_clk_get(dev, id);
> -
> -	if (clk == ERR_PTR(-ENOENT))
> -		return NULL;
> -
> -	return clk;
> +	return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
>   }
>   EXPORT_SYMBOL(devm_clk_get_optional);
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH] clk: Fix pointer casting to prevent oops in devm_clk_release()

[Uwe Kleine-König]

The release function is called with a pointer to the memory returned by
devres_alloc(). I was confused about that by the code before the
generalization that used a struct clk **ptr.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
On Mon, Jun 20, 2022 at 05:26:12PM +0200, Marek Szyprowski wrote:
> > -   clk_put(*(struct clk **)res);
> > +   struct devm_clk_state *state = *(struct devm_clk_state **)res;
> 
> This should be:
> 
> struct devm_clk_state *state = res;
> 
> otherwise it nukes badly during cleanup:
> [...]

How embarrassing. I understood how I confused that, but I wonder how
that didn't pop up earlier.

FTR: I didn't test that now, but assume you did. My focus now was to get
out an applicable patch fast.

Thanks for your report
Uwe

 drivers/clk/clk-devres.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index c822f4ef1584..1bb086695051 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -11,7 +11,7 @@ struct devm_clk_state {
 
 static void devm_clk_release(struct device *dev, void *res)
 {
-	struct devm_clk_state *state = *(struct devm_clk_state **)res;
+	struct devm_clk_state *state = res;
 
 	if (state->exit)
 		state->exit(state->clk);

base-commit: abae8e57e49aa75f6db76aa866c775721523908f
-- 
2.36.1

Re: [PATCH] clk: Fix pointer casting to prevent oops in devm_clk_release()

[Marek Szyprowski]

On 20.06.2022 19:18, Uwe Kleine-König wrote:
> The release function is called with a pointer to the memory returned by
> devres_alloc(). I was confused about that by the code before the
> generalization that used a struct clk **ptr.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> On Mon, Jun 20, 2022 at 05:26:12PM +0200, Marek Szyprowski wrote:
>>> -   clk_put(*(struct clk **)res);
>>> +   struct devm_clk_state *state = *(struct devm_clk_state **)res;
>> This should be:
>>
>> struct devm_clk_state *state = res;
>>
>> otherwise it nukes badly during cleanup:
>> [...]
> How embarrassing. I understood how I confused that, but I wonder how
> that didn't pop up earlier.
>
> FTR: I didn't test that now, but assume you did. My focus now was to get
> out an applicable patch fast.
>
> Thanks for your report
> Uwe
>
>   drivers/clk/clk-devres.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index c822f4ef1584..1bb086695051 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -11,7 +11,7 @@ struct devm_clk_state {
>   
>   static void devm_clk_release(struct device *dev, void *res)
>   {
> -	struct devm_clk_state *state = *(struct devm_clk_state **)res;
> +	struct devm_clk_state *state = res;
>   
>   	if (state->exit)
>   		state->exit(state->clk);
>
> base-commit: abae8e57e49aa75f6db76aa866c775721523908f

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] clk: Fix pointer casting to prevent oops in devm_clk_release()

[Uwe Kleine-König]

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

Hello,

On Tue, Jun 21, 2022 at 08:25:23AM +0200, Marek Szyprowski wrote:
> On 20.06.2022 19:18, Uwe Kleine-König wrote:
> > The release function is called with a pointer to the memory returned by
> > devres_alloc(). I was confused about that by the code before the
> > generalization that used a struct clk **ptr.
> >
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for your confirmation here.

> > ---
> > On Mon, Jun 20, 2022 at 05:26:12PM +0200, Marek Szyprowski wrote:
> >>> -   clk_put(*(struct clk **)res);
> >>> +   struct devm_clk_state *state = *(struct devm_clk_state **)res;
> >> This should be:
> >>
> >> struct devm_clk_state *state = res;
> >>
> >> otherwise it nukes badly during cleanup:
> >> [...]
> > How embarrassing. I understood how I confused that, but I wonder how
> > that didn't pop up earlier.
> >
> > FTR: I didn't test that now, but assume you did. My focus now was to get
> > out an applicable patch fast.

I fear that this issue will break some more tests, so I think it would
be good to get this fix quick into next.

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: [PATCH] clk: Fix pointer casting to prevent oops in devm_clk_release()

[Uwe Kleine-König]

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

Hello,

On Mon, Jun 20, 2022 at 07:18:15PM +0200, Uwe Kleine-König wrote:
> The release function is called with a pointer to the memory returned by
> devres_alloc(). I was confused about that by the code before the
> generalization that used a struct clk **ptr.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Forwarding from https://lore.kernel.org/linux-clk/CA+G9fYtUu2VCZ2NRpKMV4iCimi8koQ3OTeqQ3byZ9W11sE9fSg@mail.gmail.com:

Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>

Up to now there were three reports about this breakage.

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: [PATCH] clk: Fix pointer casting to prevent oops in devm_clk_release()

[Stephen Boyd]

Quoting Uwe Kleine-König (2022-06-20 10:18:15)
> The release function is called with a pointer to the memory returned by
> devres_alloc(). I was confused about that by the code before the
> generalization that used a struct clk **ptr.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: abae8e57e49a ("clk: generalize devm_clk_get() a bit")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Applied to clk-next