diff mbox series

[v4,6/6] spi: davinci: Simplify using devm_clk_get_enabled()

Message ID 20210330181755.204339-7-u.kleine-koenig@pengutronix.de (mailing list archive)
State New
Headers show
Series clk: provide new devm helpers for prepared and enabled clocks | expand

Commit Message

Uwe Kleine-König March 30, 2021, 6:17 p.m. UTC
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(-)

Comments

Mark Brown March 31, 2021, 12:02 p.m. UTC | #1
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>
Uwe Kleine-König April 6, 2021, 6:57 a.m. UTC | #2
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
Geert Uytterhoeven April 7, 2021, 7 a.m. UTC | #3
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
Uwe Kleine-König April 7, 2021, 7:24 a.m. UTC | #4
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
Mark Brown April 7, 2021, 11:02 a.m. UTC | #5
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.
diff mbox series

Patch

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