Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4 0/6] clk: provide new devm helpers for prepared and enabled clocks
@ 2021-03-30 18:17 Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 1/6] clk: generalize devm_clk_get() a bit Uwe Kleine-König
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-03-30 18:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-clk, kernel, Claudiu Beznea, Thierry Reding, Lee Jones,
	Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-pwm,
	linux-arm-kernel, Alessandro Zummo, linux-rtc, Mark Brown,
	linux-spi

Hello,

this series contains new helpers for devm managed clocks. Since v3 this
is a series that also contains four example conversions to show the
simplification that can be achieved. It was not hard to find these
candidates, there are drivers all over that can benefit.

The idea to provide these helpers is already quite old, I sent v1 back
in October and unfortunately didn't receive any feedback from the clk
maintainers yet on any of the patch series. It would be great if this
series is considered obviously good enough to consider it.

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: at91sma9: Simplify using devm_clk_get_enabled()
  i2c: imx: Simplify using devm_clk_get_enableded()
  spi: davinci: Simplify using devm_clk_get_enabled()

 drivers/clk/clk-devres.c     | 96 ++++++++++++++++++++++++++++++------
 drivers/i2c/busses/i2c-imx.c | 11 +----
 drivers/pwm/pwm-atmel.c      | 15 +-----
 drivers/rtc/rtc-at91sam9.c   | 22 ++-------
 drivers/spi/spi-davinci.c    | 11 +----
 include/linux/clk.h          | 87 +++++++++++++++++++++++++++++++-
 6 files changed, 176 insertions(+), 66 deletions(-)

base-commit: a38fd8748464831584a19438cbb3082b5a2dab15
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/6] clk: generalize devm_clk_get() a bit
  2021-03-30 18:17 [PATCH v4 0/6] clk: provide new devm helpers for prepared and enabled clocks Uwe Kleine-König
@ 2021-03-30 18:17 ` Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 2/6] clk: Provide new devm_clk_helpers for prepared and enabled clocks Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled() Uwe Kleine-König
  2 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-03-30 18:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-clk, kernel, Claudiu Beznea, Thierry Reding, Lee Jones,
	Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-pwm,
	linux-arm-kernel, Alessandro Zummo, linux-rtc, Mark Brown,
	linux-spi

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.

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 2/6] clk: Provide new devm_clk_helpers for prepared and enabled clocks
  2021-03-30 18:17 [PATCH v4 0/6] clk: provide new devm helpers for prepared and enabled clocks Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 1/6] clk: generalize devm_clk_get() a bit Uwe Kleine-König
@ 2021-03-30 18:17 ` Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled() Uwe Kleine-König
  2 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-03-30 18:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-clk, kernel, Claudiu Beznea, Thierry Reding, Lee Jones,
	Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-pwm,
	linux-arm-kernel, Alessandro Zummo, linux-rtc, Mark Brown,
	linux-spi

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

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk-devres.c | 31 ++++++++++++++
 include/linux/clk.h      | 87 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 117 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..b3c5da388b08 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,26 @@ 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 +874,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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled()
  2021-03-30 18:17 [PATCH v4 0/6] clk: provide new devm helpers for prepared and enabled clocks Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 1/6] clk: generalize devm_clk_get() a bit Uwe Kleine-König
  2021-03-30 18:17 ` [PATCH v4 2/6] clk: Provide new devm_clk_helpers for prepared and enabled clocks Uwe Kleine-König
@ 2021-03-30 18:17 ` Uwe Kleine-König
  2021-03-31 12:02   ` Mark Brown
  2 siblings, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2021-03-30 18:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Mark Brown; +Cc: linux-clk, kernel, linux-spi

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.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-davinci.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 7453a1dbbc06..63ee918ecdb0 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -936,14 +936,11 @@ static int davinci_spi_probe(struct platform_device *pdev)
 
 	dspi->bitbang.master = master;
 
-	dspi->clk = devm_clk_get(&pdev->dev, NULL);
+	dspi->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(dspi->clk)) {
 		ret = -ENODEV;
 		goto free_master;
 	}
-	ret = clk_prepare_enable(dspi->clk);
-	if (ret)
-		goto free_master;
 
 	master->use_gpio_descriptors = true;
 	master->dev.of_node = pdev->dev.of_node;
@@ -968,7 +965,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 
 	ret = davinci_spi_request_dma(dspi);
 	if (ret == -EPROBE_DEFER) {
-		goto free_clk;
+		goto free_master;
 	} else if (ret) {
 		dev_info(&pdev->dev, "DMA is not supported (%d)\n", ret);
 		dspi->dma_rx = NULL;
@@ -1012,8 +1009,6 @@ static int davinci_spi_probe(struct platform_device *pdev)
 		dma_release_channel(dspi->dma_rx);
 		dma_release_channel(dspi->dma_tx);
 	}
-free_clk:
-	clk_disable_unprepare(dspi->clk);
 free_master:
 	spi_master_put(master);
 err:
@@ -1039,8 +1034,6 @@ static int davinci_spi_remove(struct platform_device *pdev)
 
 	spi_bitbang_stop(&dspi->bitbang);
 
-	clk_disable_unprepare(dspi->clk);
-
 	if (dspi->dma_rx) {
 		dma_release_channel(dspi->dma_rx);
 		dma_release_channel(dspi->dma_tx);
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled()
  2021-03-30 18:17 ` [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled() Uwe Kleine-König
@ 2021-03-31 12:02   ` Mark Brown
  2021-04-06  6:57     ` Uwe Kleine-König
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2021-03-31 12:02 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Michael Turquette, Stephen Boyd, linux-clk, kernel, linux-spi


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

On Tue, Mar 30, 2021 at 08:17:55PM +0200, Uwe Kleine-König wrote:
> 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: Mark Brown <broonie@kernel.org>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled()
  2021-03-31 12:02   ` Mark Brown
@ 2021-04-06  6:57     ` Uwe Kleine-König
  2021-04-07  7:00       ` Geert Uytterhoeven
  2021-04-07 11:02       ` Mark Brown
  0 siblings, 2 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-04-06  6:57 UTC (permalink / raw)
  To: Mark Brown; +Cc: Stephen Boyd, Michael Turquette, linux-clk, kernel, linux-spi


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

Hello Mark,

On Wed, Mar 31, 2021 at 01:02:12PM +0100, Mark Brown wrote:
> On Tue, Mar 30, 2021 at 08:17:55PM +0200, Uwe Kleine-König wrote:
> > 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: Mark Brown <broonie@kernel.org>

Thanks. I wonder what you think about this series. Is it more "Well, ok,
if you must, the change you did to this spi driver looks correct." or
"This is a good simplification and a similar change for nearly all other
spi drivers that make use of a clk is possible, too. Dear clk
maintainers, please go forward and apply this useful series."?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled()
  2021-04-06  6:57     ` Uwe Kleine-König
@ 2021-04-07  7:00       ` Geert Uytterhoeven
  2021-04-07  7:24         ` Uwe Kleine-König
  2021-04-07 11:02       ` Mark Brown
  1 sibling, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2021-04-07  7:00 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Mark Brown, Stephen Boyd, Michael Turquette, linux-clk,
	Sascha Hauer, linux-spi

Hi Uwe,

I'm not Mark, but I'd like to share my 2€c.

On Tue, Apr 6, 2021 at 3:43 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Mar 31, 2021 at 01:02:12PM +0100, Mark Brown wrote:
> > On Tue, Mar 30, 2021 at 08:17:55PM +0200, Uwe Kleine-König wrote:
> > > 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: Mark Brown <broonie@kernel.org>
>
> Thanks. I wonder what you think about this series. Is it more "Well, ok,
> if you must, the change you did to this spi driver looks correct." or
> "This is a good simplification and a similar change for nearly all other
> spi drivers that make use of a clk is possible, too. Dear clk
> maintainers, please go forward and apply this useful series."?

While this simplifies drivers, this makes it harder to add power
management by controlling the clocks through Runtime PM later, as that
will require reverting the s/devm_clk_get/devm_clk_get_enabled/ again.

At least the Keystone series already uses PM Domains, but I don't
know if that includes clock control.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled()
  2021-04-07  7:00       ` Geert Uytterhoeven
@ 2021-04-07  7:24         ` Uwe Kleine-König
  0 siblings, 0 replies; 9+ messages in thread
From: Uwe Kleine-König @ 2021-04-07  7:24 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Stephen Boyd, Michael Turquette, linux-spi, Mark Brown,
	Sascha Hauer, linux-clk


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

On Wed, Apr 07, 2021 at 09:00:33AM +0200, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> I'm not Mark, but I'd like to share my 2€c.
> 
> On Tue, Apr 6, 2021 at 3:43 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Mar 31, 2021 at 01:02:12PM +0100, Mark Brown wrote:
> > > On Tue, Mar 30, 2021 at 08:17:55PM +0200, Uwe Kleine-König wrote:
> > > > 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: Mark Brown <broonie@kernel.org>
> >
> > Thanks. I wonder what you think about this series. Is it more "Well, ok,
> > if you must, the change you did to this spi driver looks correct." or
> > "This is a good simplification and a similar change for nearly all other
> > spi drivers that make use of a clk is possible, too. Dear clk
> > maintainers, please go forward and apply this useful series."?
> 
> While this simplifies drivers, this makes it harder to add power
> management by controlling the clocks through Runtime PM later, as that
> will require reverting the s/devm_clk_get/devm_clk_get_enabled/ again.

Hmm, if you start with a driver that uses devm_clk_get_enabled() you
have to do:

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 63ee918ecdb0..07855f89290e 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -936,7 +936,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 
 	dspi->bitbang.master = master;
 
-	dspi->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	dspi->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dspi->clk)) {
 		ret = -ENODEV;
 		goto free_master;

(+ adding runtime PM of course). When you start with the previous state
of the driver you have to do:

diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 7453a1dbbc06..07855f89290e 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -941,9 +941,6 @@ static int davinci_spi_probe(struct platform_device *pdev)
 		ret = -ENODEV;
 		goto free_master;
 	}
-	ret = clk_prepare_enable(dspi->clk);
-	if (ret)
-		goto free_master;
 
 	master->use_gpio_descriptors = true;
 	master->dev.of_node = pdev->dev.of_node;
@@ -968,7 +965,7 @@ static int davinci_spi_probe(struct platform_device *pdev)
 
 	ret = davinci_spi_request_dma(dspi);
 	if (ret == -EPROBE_DEFER) {
-		goto free_clk;
+		goto free_master;
 	} else if (ret) {
 		dev_info(&pdev->dev, "DMA is not supported (%d)\n", ret);
 		dspi->dma_rx = NULL;
@@ -1012,8 +1009,6 @@ static int davinci_spi_probe(struct platform_device *pdev)
 		dma_release_channel(dspi->dma_rx);
 		dma_release_channel(dspi->dma_tx);
 	}
-free_clk:
-	clk_disable_unprepare(dspi->clk);
 free_master:
 	spi_master_put(master);
 err:
@@ -1039,8 +1034,6 @@ static int davinci_spi_remove(struct platform_device *pdev)
 
 	spi_bitbang_stop(&dspi->bitbang);
 
-	clk_disable_unprepare(dspi->clk);
-
 	if (dspi->dma_rx) {
 		dma_release_channel(dspi->dma_rx);
 		dma_release_channel(dspi->dma_tx);

(+ again adding runtime PM of course). Do you really think the latter is
the easier approach? Or what am I missing?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled()
  2021-04-06  6:57     ` Uwe Kleine-König
  2021-04-07  7:00       ` Geert Uytterhoeven
@ 2021-04-07 11:02       ` Mark Brown
  1 sibling, 0 replies; 9+ messages in thread
From: Mark Brown @ 2021-04-07 11:02 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Stephen Boyd, Michael Turquette, linux-clk, kernel, linux-spi


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

On Tue, Apr 06, 2021 at 08:57:27AM +0200, Uwe Kleine-König wrote:

> Thanks. I wonder what you think about this series. Is it more "Well, ok,
> if you must, the change you did to this spi driver looks correct." or
> "This is a good simplification and a similar change for nearly all other
> spi drivers that make use of a clk is possible, too. Dear clk
> maintainers, please go forward and apply this useful series."?

Well, I know there's demand for devm enables of things and for some
simpler drivers it is actually the pattern people use but I find it hard
to get enthusiastic about the pattern, it really only works for the
extremely simple case where things get turned on in probe and never
touched afterwards.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 18:17 [PATCH v4 0/6] clk: provide new devm helpers for prepared and enabled clocks Uwe Kleine-König
2021-03-30 18:17 ` [PATCH v4 1/6] clk: generalize devm_clk_get() a bit Uwe Kleine-König
2021-03-30 18:17 ` [PATCH v4 2/6] clk: Provide new devm_clk_helpers for prepared and enabled clocks Uwe Kleine-König
2021-03-30 18:17 ` [PATCH v4 6/6] spi: davinci: Simplify using devm_clk_get_enabled() Uwe Kleine-König
2021-03-31 12:02   ` Mark Brown
2021-04-06  6:57     ` Uwe Kleine-König
2021-04-07  7:00       ` Geert Uytterhoeven
2021-04-07  7:24         ` Uwe Kleine-König
2021-04-07 11:02       ` Mark Brown

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-spi linux-spi/ https://lore.kernel.org/linux-spi \
		linux-spi@vger.kernel.org
	public-inbox-index linux-spi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-spi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git