| Message ID | 20210330181755.204339-7-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: provide new devm helpers for prepared and enabled clocks | expand |
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>
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
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
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
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 --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);
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(-)