[PATCH 0/6] dma:Use devm_clk_get_enabled() helpers

[PATCH 0/6] dma:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

The devm_clk_get_enabled() helpers:
    - call devm_clk_get()
    - call clk_prepare_enable() and register what is needed in order to
     call clk_disable_unprepare() when needed, as a managed resource.

This simplifies the code and avoids the calls to clk_disable_unprepare().

Liao Yuanhong (6):
  dma:at_hdmac:Use devm_clk_get_enabled() helpers
  dma:dma-jz4780:Use devm_clk_get_enabled() helpers
  dma:imx-dma:Use devm_clk_get_enabled() helpers
  dma:imx-sdma:Use devm_clk_get_enabled() helpers
  dma:milbeaut-hdmac:Use devm_clk_get_enabled() helpers
  dma:uniphier-mdmac:Use devm_clk_get_enabled() helpers

 drivers/dma/at_hdmac.c       | 22 ++++----------
 drivers/dma/dma-jz4780.c     | 18 ++++--------
 drivers/dma/imx-dma.c        | 38 ++++++++----------------
 drivers/dma/imx-sdma.c       | 57 ++++--------------------------------
 drivers/dma/milbeaut-hdmac.c | 20 ++++---------
 drivers/dma/uniphier-mdmac.c | 20 ++++---------
 6 files changed, 41 insertions(+), 134 deletions(-)

-- 
2.25.1

[PATCH 1/6] dma:at_hdmac:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in at_hdmac.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
 drivers/dma/at_hdmac.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index 40052d1bd0b5..b1e10541cb12 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -337,7 +337,6 @@ static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
  * struct at_dma - internal representation of an Atmel HDMA Controller
  * @dma_device: dmaengine dma_device object members
  * @regs: memory mapped register base
- * @clk: dma controller clock
  * @save_imr: interrupt mask register that is saved on suspend/resume cycle
  * @all_chan_mask: all channels availlable in a mask
  * @lli_pool: hw lli table
@@ -347,7 +346,6 @@ static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
 struct at_dma {
 	struct dma_device	dma_device;
 	void __iomem		*regs;
-	struct clk		*clk;
 	u32			save_imr;
 
 	u8			all_chan_mask;
@@ -1942,6 +1940,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
 	int			err;
 	int			i;
 	const struct at_dma_platform_data *plat_dat;
+	struct clk	*clk;
 
 	/* setup platform data for each SoC */
 	dma_cap_set(DMA_MEMCPY, at91sam9rl_config.cap_mask);
@@ -1975,20 +1974,16 @@ static int __init at_dma_probe(struct platform_device *pdev)
 	atdma->dma_device.cap_mask = plat_dat->cap_mask;
 	atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
 
-	atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
-	if (IS_ERR(atdma->clk))
-		return PTR_ERR(atdma->clk);
-
-	err = clk_prepare_enable(atdma->clk);
-	if (err)
-		return err;
+	clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	/* force dma off, just in case */
 	at_dma_off(atdma);
 
 	err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
 	if (err)
-		goto err_irq;
+		return err;
 
 	platform_set_drvdata(pdev, atdma);
 
@@ -2105,8 +2100,6 @@ static int __init at_dma_probe(struct platform_device *pdev)
 	dma_pool_destroy(atdma->lli_pool);
 err_desc_pool_create:
 	free_irq(platform_get_irq(pdev, 0), atdma);
-err_irq:
-	clk_disable_unprepare(atdma->clk);
 	return err;
 }
 
@@ -2130,8 +2123,6 @@ static void at_dma_remove(struct platform_device *pdev)
 		atc_disable_chan_irq(atdma, chan->chan_id);
 		list_del(&chan->device_node);
 	}
-
-	clk_disable_unprepare(atdma->clk);
 }
 
 static void at_dma_shutdown(struct platform_device *pdev)
@@ -2139,7 +2130,6 @@ static void at_dma_shutdown(struct platform_device *pdev)
 	struct at_dma	*atdma = platform_get_drvdata(pdev);
 
 	at_dma_off(platform_get_drvdata(pdev));
-	clk_disable_unprepare(atdma->clk);
 }
 
 static int at_dma_prepare(struct device *dev)
@@ -2194,7 +2184,6 @@ static int at_dma_suspend_noirq(struct device *dev)
 
 	/* disable DMA controller */
 	at_dma_off(atdma);
-	clk_disable_unprepare(atdma->clk);
 	return 0;
 }
 
@@ -2223,7 +2212,6 @@ static int at_dma_resume_noirq(struct device *dev)
 	struct dma_chan *chan, *_chan;
 
 	/* bring back DMA controller */
-	clk_prepare_enable(atdma->clk);
 	dma_writel(atdma, EN, AT_DMA_ENABLE);
 
 	/* clear any pending interrupt */
-- 
2.25.1

Re: [PATCH 1/6] dma:at_hdmac:Use devm_clk_get_enabled() helpers

[Jonathan Cameron]

On Fri, 23 Aug 2024 18:19:28 +0800
Liao Yuanhong <liaoyuanhong@vivo.com> wrote:

> Use devm_clk_get_enabled() instead of clk functions in at_hdmac.

Doesn't this stop the clock being turned of in suspend?

Or is there some magic handling that which I'm not aware of?


> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
> ---
>  drivers/dma/at_hdmac.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 40052d1bd0b5..b1e10541cb12 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -337,7 +337,6 @@ static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
>   * struct at_dma - internal representation of an Atmel HDMA Controller
>   * @dma_device: dmaengine dma_device object members
>   * @regs: memory mapped register base
> - * @clk: dma controller clock
>   * @save_imr: interrupt mask register that is saved on suspend/resume cycle
>   * @all_chan_mask: all channels availlable in a mask
>   * @lli_pool: hw lli table
> @@ -347,7 +346,6 @@ static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
>  struct at_dma {
>  	struct dma_device	dma_device;
>  	void __iomem		*regs;
> -	struct clk		*clk;
>  	u32			save_imr;
>  
>  	u8			all_chan_mask;
> @@ -1942,6 +1940,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	int			err;
>  	int			i;
>  	const struct at_dma_platform_data *plat_dat;
> +	struct clk	*clk;
>  
>  	/* setup platform data for each SoC */
>  	dma_cap_set(DMA_MEMCPY, at91sam9rl_config.cap_mask);
> @@ -1975,20 +1974,16 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	atdma->dma_device.cap_mask = plat_dat->cap_mask;
>  	atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
>  
> -	atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
> -	if (IS_ERR(atdma->clk))
> -		return PTR_ERR(atdma->clk);
> -
> -	err = clk_prepare_enable(atdma->clk);
> -	if (err)
> -		return err;
> +	clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
>  
>  	/* force dma off, just in case */
>  	at_dma_off(atdma);
>  
>  	err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
>  	if (err)
> -		goto err_irq;
> +		return err;
>  
>  	platform_set_drvdata(pdev, atdma);
>  
> @@ -2105,8 +2100,6 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	dma_pool_destroy(atdma->lli_pool);
>  err_desc_pool_create:
>  	free_irq(platform_get_irq(pdev, 0), atdma);
> -err_irq:
> -	clk_disable_unprepare(atdma->clk);
>  	return err;
>  }
>  
> @@ -2130,8 +2123,6 @@ static void at_dma_remove(struct platform_device *pdev)
>  		atc_disable_chan_irq(atdma, chan->chan_id);
>  		list_del(&chan->device_node);
>  	}
> -
> -	clk_disable_unprepare(atdma->clk);
>  }
>  
>  static void at_dma_shutdown(struct platform_device *pdev)
> @@ -2139,7 +2130,6 @@ static void at_dma_shutdown(struct platform_device *pdev)
>  	struct at_dma	*atdma = platform_get_drvdata(pdev);
>  
>  	at_dma_off(platform_get_drvdata(pdev));
> -	clk_disable_unprepare(atdma->clk);
>  }
>  
>  static int at_dma_prepare(struct device *dev)
> @@ -2194,7 +2184,6 @@ static int at_dma_suspend_noirq(struct device *dev)
>  
>  	/* disable DMA controller */
>  	at_dma_off(atdma);
> -	clk_disable_unprepare(atdma->clk);
>  	return 0;
>  }
>  
> @@ -2223,7 +2212,6 @@ static int at_dma_resume_noirq(struct device *dev)
>  	struct dma_chan *chan, *_chan;
>  
>  	/* bring back DMA controller */
> -	clk_prepare_enable(atdma->clk);
>  	dma_writel(atdma, EN, AT_DMA_ENABLE);
>  
>  	/* clear any pending interrupt */

Re: [PATCH 1/6] dma:at_hdmac:Use devm_clk_get_enabled() helpers

[Nicolas Ferre]

On 23/08/2024 at 12:19, Liao Yuanhong wrote:
> Use devm_clk_get_enabled() instead of clk functions in at_hdmac.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>

With this patch, you remove the possibility to stop the clock during a 
suspend/resume cycle: why avoid to gain power during... low power phases?

NACK.

Regards,
   Nicolas

> ---
>   drivers/dma/at_hdmac.c | 22 +++++-----------------
>   1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 40052d1bd0b5..b1e10541cb12 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -337,7 +337,6 @@ static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
>    * struct at_dma - internal representation of an Atmel HDMA Controller
>    * @dma_device: dmaengine dma_device object members
>    * @regs: memory mapped register base
> - * @clk: dma controller clock
>    * @save_imr: interrupt mask register that is saved on suspend/resume cycle
>    * @all_chan_mask: all channels availlable in a mask
>    * @lli_pool: hw lli table
> @@ -347,7 +346,6 @@ static inline u8 convert_buswidth(enum dma_slave_buswidth addr_width)
>   struct at_dma {
>          struct dma_device       dma_device;
>          void __iomem            *regs;
> -       struct clk              *clk;
>          u32                     save_imr;
> 
>          u8                      all_chan_mask;
> @@ -1942,6 +1940,7 @@ static int __init at_dma_probe(struct platform_device *pdev)
>          int                     err;
>          int                     i;
>          const struct at_dma_platform_data *plat_dat;
> +       struct clk      *clk;
> 
>          /* setup platform data for each SoC */
>          dma_cap_set(DMA_MEMCPY, at91sam9rl_config.cap_mask);
> @@ -1975,20 +1974,16 @@ static int __init at_dma_probe(struct platform_device *pdev)
>          atdma->dma_device.cap_mask = plat_dat->cap_mask;
>          atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
> 
> -       atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
> -       if (IS_ERR(atdma->clk))
> -               return PTR_ERR(atdma->clk);
> -
> -       err = clk_prepare_enable(atdma->clk);
> -       if (err)
> -               return err;
> +       clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> 
>          /* force dma off, just in case */
>          at_dma_off(atdma);
> 
>          err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
>          if (err)
> -               goto err_irq;
> +               return err;
> 
>          platform_set_drvdata(pdev, atdma);
> 
> @@ -2105,8 +2100,6 @@ static int __init at_dma_probe(struct platform_device *pdev)
>          dma_pool_destroy(atdma->lli_pool);
>   err_desc_pool_create:
>          free_irq(platform_get_irq(pdev, 0), atdma);
> -err_irq:
> -       clk_disable_unprepare(atdma->clk);
>          return err;
>   }
> 
> @@ -2130,8 +2123,6 @@ static void at_dma_remove(struct platform_device *pdev)
>                  atc_disable_chan_irq(atdma, chan->chan_id);
>                  list_del(&chan->device_node);
>          }
> -
> -       clk_disable_unprepare(atdma->clk);
>   }
> 
>   static void at_dma_shutdown(struct platform_device *pdev)
> @@ -2139,7 +2130,6 @@ static void at_dma_shutdown(struct platform_device *pdev)
>          struct at_dma   *atdma = platform_get_drvdata(pdev);
> 
>          at_dma_off(platform_get_drvdata(pdev));
> -       clk_disable_unprepare(atdma->clk);
>   }
> 
>   static int at_dma_prepare(struct device *dev)
> @@ -2194,7 +2184,6 @@ static int at_dma_suspend_noirq(struct device *dev)
> 
>          /* disable DMA controller */
>          at_dma_off(atdma);
> -       clk_disable_unprepare(atdma->clk);
>          return 0;
>   }
> 
> @@ -2223,7 +2212,6 @@ static int at_dma_resume_noirq(struct device *dev)
>          struct dma_chan *chan, *_chan;
> 
>          /* bring back DMA controller */
> -       clk_prepare_enable(atdma->clk);
>          dma_writel(atdma, EN, AT_DMA_ENABLE);
> 
>          /* clear any pending interrupt */
> --
> 2.25.1
> 
> 

[PATCH 2/6] dma:dma-jz4780:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in dma-jz4780.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
 drivers/dma/dma-jz4780.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index c9cfa341db51..151a85516419 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -149,7 +149,6 @@ struct jz4780_dma_dev {
 	struct dma_device dma_device;
 	void __iomem *chn_base;
 	void __iomem *ctrl_base;
-	struct clk *clk;
 	unsigned int irq;
 	const struct jz4780_dma_soc_data *soc_data;
 
@@ -857,6 +856,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 	struct dma_device *dd;
 	struct resource *res;
 	int i, ret;
+	struct clk *clk;
 
 	if (!dev->of_node) {
 		dev_err(dev, "This driver must be probed from devicetree\n");
@@ -896,15 +896,13 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	jzdma->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(jzdma->clk)) {
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk)) {
 		dev_err(dev, "failed to get clock\n");
-		ret = PTR_ERR(jzdma->clk);
+		ret = PTR_ERR(clk);
 		return ret;
 	}
 
-	clk_prepare_enable(jzdma->clk);
-
 	/* Property is optional, if it doesn't exist the value will remain 0. */
 	of_property_read_u32_index(dev->of_node, "ingenic,reserved-channels",
 				   0, &jzdma->chan_reserved);
@@ -972,7 +970,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_disable_clk;
+		return ret;
 
 	jzdma->irq = ret;
 
@@ -980,7 +978,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 			  jzdma);
 	if (ret) {
 		dev_err(dev, "failed to request IRQ %u!\n", jzdma->irq);
-		goto err_disable_clk;
+		return ret;
 	}
 
 	ret = dmaenginem_async_device_register(dd);
@@ -1002,9 +1000,6 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 
 err_free_irq:
 	free_irq(jzdma->irq, jzdma);
-
-err_disable_clk:
-	clk_disable_unprepare(jzdma->clk);
 	return ret;
 }
 
@@ -1015,7 +1010,6 @@ static void jz4780_dma_remove(struct platform_device *pdev)
 
 	of_dma_controller_free(pdev->dev.of_node);
 
-	clk_disable_unprepare(jzdma->clk);
 	free_irq(jzdma->irq, jzdma);
 
 	for (i = 0; i < jzdma->soc_data->nb_channels; i++)
-- 
2.25.1

Re: [PATCH 2/6] dma:dma-jz4780:Use devm_clk_get_enabled() helpers

[Jonathan Cameron]

On Fri, 23 Aug 2024 18:19:29 +0800
Liao Yuanhong <liaoyuanhong@vivo.com> wrote:

> Use devm_clk_get_enabled() instead of clk functions in dma-jz4780.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
Original code has some odd ordering.  The error path order
and remove order should be the same and aren't.

Even with that tidied up this reorders clock disable and tasklet
killing. So I think this is fine, but needs more eyes on it.

> ---
>  drivers/dma/dma-jz4780.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index c9cfa341db51..151a85516419 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -149,7 +149,6 @@ struct jz4780_dma_dev {
>  	struct dma_device dma_device;
>  	void __iomem *chn_base;
>  	void __iomem *ctrl_base;
> -	struct clk *clk;
>  	unsigned int irq;
>  	const struct jz4780_dma_soc_data *soc_data;
>  
> @@ -857,6 +856,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  	struct dma_device *dd;
>  	struct resource *res;
>  	int i, ret;
> +	struct clk *clk;
>  
>  	if (!dev->of_node) {
>  		dev_err(dev, "This driver must be probed from devicetree\n");
> @@ -896,15 +896,13 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	jzdma->clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(jzdma->clk)) {
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk)) {
>  		dev_err(dev, "failed to get clock\n");
> -		ret = PTR_ERR(jzdma->clk);
> +		ret = PTR_ERR(clk);
>  		return ret;
>  	}
>  
> -	clk_prepare_enable(jzdma->clk);
> -
>  	/* Property is optional, if it doesn't exist the value will remain 0. */
>  	of_property_read_u32_index(dev->of_node, "ingenic,reserved-channels",
>  				   0, &jzdma->chan_reserved);
> @@ -972,7 +970,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  
>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
> -		goto err_disable_clk;
> +		return ret;
>  
>  	jzdma->irq = ret;
>  
> @@ -980,7 +978,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  			  jzdma);
>  	if (ret) {
>  		dev_err(dev, "failed to request IRQ %u!\n", jzdma->irq);
> -		goto err_disable_clk;
> +		return ret;
>  	}
>  
>  	ret = dmaenginem_async_device_register(dd);
> @@ -1002,9 +1000,6 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  
>  err_free_irq:
>  	free_irq(jzdma->irq, jzdma);
> -
> -err_disable_clk:
> -	clk_disable_unprepare(jzdma->clk);
>  	return ret;
>  }
>  
> @@ -1015,7 +1010,6 @@ static void jz4780_dma_remove(struct platform_device *pdev)
>  
>  	of_dma_controller_free(pdev->dev.of_node);
>  
> -	clk_disable_unprepare(jzdma->clk);
>  	free_irq(jzdma->irq, jzdma);
Hmm. Ordering wise this is not great.

>  
>  	for (i = 0; i < jzdma->soc_data->nb_channels; i++)

[PATCH 3/6] dma:imx-dma:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in imx-dma.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
 drivers/dma/imx-dma.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index ebf7c115d553..1ef926304d0e 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -1039,6 +1039,8 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	struct imxdma_engine *imxdma;
 	int ret, i;
 	int irq, irq_err;
+	struct clk *dma_ahb;
+	struct clk *dma_ipg;
 
 	imxdma = devm_kzalloc(&pdev->dev, sizeof(*imxdma), GFP_KERNEL);
 	if (!imxdma)
@@ -1055,20 +1057,13 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	imxdma->dma_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(imxdma->dma_ipg))
-		return PTR_ERR(imxdma->dma_ipg);
+	dma_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
+	if (IS_ERR(dma_ipg))
+		return PTR_ERR(dma_ipg);
 
-	imxdma->dma_ahb = devm_clk_get(&pdev->dev, "ahb");
-	if (IS_ERR(imxdma->dma_ahb))
-		return PTR_ERR(imxdma->dma_ahb);
-
-	ret = clk_prepare_enable(imxdma->dma_ipg);
-	if (ret)
-		return ret;
-	ret = clk_prepare_enable(imxdma->dma_ahb);
-	if (ret)
-		goto disable_dma_ipg_clk;
+	dma_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
+	if (IS_ERR(dma_ahb))
+		return PTR_ERR(dma_ahb);
 
 	/* reset DMA module */
 	imx_dmav1_writel(imxdma, DCR_DRST, DMA_DCR);
@@ -1078,21 +1073,21 @@ static int __init imxdma_probe(struct platform_device *pdev)
 				       dma_irq_handler, 0, "DMA", imxdma);
 		if (ret) {
 			dev_warn(imxdma->dev, "Can't register IRQ for DMA\n");
-			goto disable_dma_ahb_clk;
+			return ret;
 		}
 		imxdma->irq = irq;
 
 		irq_err = platform_get_irq(pdev, 1);
 		if (irq_err < 0) {
 			ret = irq_err;
-			goto disable_dma_ahb_clk;
+			return ret;
 		}
 
 		ret = devm_request_irq(&pdev->dev, irq_err,
 				       imxdma_err_handler, 0, "DMA", imxdma);
 		if (ret) {
 			dev_warn(imxdma->dev, "Can't register ERRIRQ for DMA\n");
-			goto disable_dma_ahb_clk;
+			return ret;
 		}
 		imxdma->irq_err = irq_err;
 	}
@@ -1130,7 +1125,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 				dev_warn(imxdma->dev, "Can't register IRQ %d "
 					 "for DMA channel %d\n",
 					 irq + i, i);
-				goto disable_dma_ahb_clk;
+				return ret;
 			}
 
 			imxdmac->irq = irq + i;
@@ -1174,7 +1169,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	ret = dma_async_device_register(&imxdma->dma_device);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register\n");
-		goto disable_dma_ahb_clk;
+		return ret;
 	}
 
 	if (pdev->dev.of_node) {
@@ -1190,10 +1185,6 @@ static int __init imxdma_probe(struct platform_device *pdev)
 
 err_of_dma_controller:
 	dma_async_device_unregister(&imxdma->dma_device);
-disable_dma_ahb_clk:
-	clk_disable_unprepare(imxdma->dma_ahb);
-disable_dma_ipg_clk:
-	clk_disable_unprepare(imxdma->dma_ipg);
 	return ret;
 }
 
@@ -1226,9 +1217,6 @@ static void imxdma_remove(struct platform_device *pdev)
 
 	if (pdev->dev.of_node)
 		of_dma_controller_free(pdev->dev.of_node);
-
-	clk_disable_unprepare(imxdma->dma_ipg);
-	clk_disable_unprepare(imxdma->dma_ahb);
 }
 
 static struct platform_driver imxdma_driver = {
-- 
2.25.1

Re: [PATCH 3/6] dma:imx-dma:Use devm_clk_get_enabled() helpers

[Jonathan Cameron]

On Fri, 23 Aug 2024 18:19:30 +0800
Liao Yuanhong <liaoyuanhong@vivo.com> wrote:

> Use devm_clk_get_enabled() instead of clk functions in imx-dma.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
Straight forward case, but nice to combine this with
use of return dev_err_probe() in the paths where you now have
direct returns.  Other comments below.

 
> ---
>  drivers/dma/imx-dma.c | 38 +++++++++++++-------------------------
>  1 file changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
> index ebf7c115d553..1ef926304d0e 100644
> --- a/drivers/dma/imx-dma.c
> +++ b/drivers/dma/imx-dma.c
> @@ -1039,6 +1039,8 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  	struct imxdma_engine *imxdma;
>  	int ret, i;
>  	int irq, irq_err;
> +	struct clk *dma_ahb;
> +	struct clk *dma_ipg;
	struct clk *dma_ahb, *dma_ipg;
should be fine.
>  
>  	imxdma = devm_kzalloc(&pdev->dev, sizeof(*imxdma), GFP_KERNEL);
>  	if (!imxdma)
> @@ -1055,20 +1057,13 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> -	imxdma->dma_ipg = devm_clk_get(&pdev->dev, "ipg");
> -	if (IS_ERR(imxdma->dma_ipg))
> -		return PTR_ERR(imxdma->dma_ipg);
> +	dma_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> +	if (IS_ERR(dma_ipg))
> +		return PTR_ERR(dma_ipg);
>  
> -	imxdma->dma_ahb = devm_clk_get(&pdev->dev, "ahb");
> -	if (IS_ERR(imxdma->dma_ahb))
> -		return PTR_ERR(imxdma->dma_ahb);
> -
> -	ret = clk_prepare_enable(imxdma->dma_ipg);
> -	if (ret)
> -		return ret;
> -	ret = clk_prepare_enable(imxdma->dma_ahb);
> -	if (ret)
> -		goto disable_dma_ipg_clk;
> +	dma_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
> +	if (IS_ERR(dma_ahb))
> +		return PTR_ERR(dma_ahb);
>  
>  	/* reset DMA module */
>  	imx_dmav1_writel(imxdma, DCR_DRST, DMA_DCR);
> @@ -1078,21 +1073,21 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  				       dma_irq_handler, 0, "DMA", imxdma);
>  		if (ret) {
>  			dev_warn(imxdma->dev, "Can't register IRQ for DMA\n");
> -			goto disable_dma_ahb_clk;
> +			return ret;
Odd not to make that a dev_error given driver fails to probe as a result.
I'd switch to return dev_err_rpobe()
>  		}
>  		imxdma->irq = irq;
>  
>  		irq_err = platform_get_irq(pdev, 1);
>  		if (irq_err < 0) {
>  			ret = irq_err;
> -			goto disable_dma_ahb_clk;
> +			return ret;
>  		}
>  
>  		ret = devm_request_irq(&pdev->dev, irq_err,
>  				       imxdma_err_handler, 0, "DMA", imxdma);
>  		if (ret) {
>  			dev_warn(imxdma->dev, "Can't register ERRIRQ for DMA\n");
> -			goto disable_dma_ahb_clk;
> +			return ret;
Here as well.

>  		}
>  		imxdma->irq_err = irq_err;
>  	}
> @@ -1130,7 +1125,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  				dev_warn(imxdma->dev, "Can't register IRQ %d "
>  					 "for DMA channel %d\n",
>  					 irq + i, i);
> -				goto disable_dma_ahb_clk;
> +				return ret;
and here.

>  			}
>  
>  			imxdmac->irq = irq + i;
> @@ -1174,7 +1169,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  	ret = dma_async_device_register(&imxdma->dma_device);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to register\n");
> -		goto disable_dma_ahb_clk;
> +		return ret;
and finaly here.

>  	}
>  
>  	if (pdev->dev.of_node) {
> @@ -1190,10 +1185,6 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  
>  err_of_dma_controller:
>  	dma_async_device_unregister(&imxdma->dma_device);
Maybe use a local callback and 
devm_add_action_or_reset() to get automate handling of this call as well.

> -disable_dma_ahb_clk:
> -	clk_disable_unprepare(imxdma->dma_ahb);
> -disable_dma_ipg_clk:
> -	clk_disable_unprepare(imxdma->dma_ipg);
>  	return ret;
>  }
>  
> @@ -1226,9 +1217,6 @@ static void imxdma_remove(struct platform_device *pdev)
>  
>  	if (pdev->dev.of_node)
>  		of_dma_controller_free(pdev->dev.of_node);
The ordering of the two items above here looks suspicious as it doesn't
reverse order of probably. Maybe worth cleaning that up whilst here.

> -
> -	clk_disable_unprepare(imxdma->dma_ipg);
> -	clk_disable_unprepare(imxdma->dma_ahb);
>  }
>  
>  static struct platform_driver imxdma_driver = {

[PATCH v2 3/6] dma:imx-dma:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in imx-dma.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
v2:use dev_err_probe() instead of warn msg and return value.
---
 drivers/dma/imx-dma.c | 59 +++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index ebf7c115d553..a6ad50734f2a 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -1039,6 +1039,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	struct imxdma_engine *imxdma;
 	int ret, i;
 	int irq, irq_err;
+	struct clk *dma_ahb, *dma_ipg;
 
 	imxdma = devm_kzalloc(&pdev->dev, sizeof(*imxdma), GFP_KERNEL);
 	if (!imxdma)
@@ -1055,20 +1056,13 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	imxdma->dma_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(imxdma->dma_ipg))
-		return PTR_ERR(imxdma->dma_ipg);
+	dma_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
+	if (IS_ERR(dma_ipg))
+		return PTR_ERR(dma_ipg);
 
-	imxdma->dma_ahb = devm_clk_get(&pdev->dev, "ahb");
-	if (IS_ERR(imxdma->dma_ahb))
-		return PTR_ERR(imxdma->dma_ahb);
-
-	ret = clk_prepare_enable(imxdma->dma_ipg);
-	if (ret)
-		return ret;
-	ret = clk_prepare_enable(imxdma->dma_ahb);
-	if (ret)
-		goto disable_dma_ipg_clk;
+	dma_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
+	if (IS_ERR(dma_ahb))
+		return PTR_ERR(dma_ahb);
 
 	/* reset DMA module */
 	imx_dmav1_writel(imxdma, DCR_DRST, DMA_DCR);
@@ -1076,24 +1070,22 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	if (is_imx1_dma(imxdma)) {
 		ret = devm_request_irq(&pdev->dev, irq,
 				       dma_irq_handler, 0, "DMA", imxdma);
-		if (ret) {
-			dev_warn(imxdma->dev, "Can't register IRQ for DMA\n");
-			goto disable_dma_ahb_clk;
-		}
+		if (ret)
+			return dev_err_probe(imxdma->dev, ret, "Can't register IRQ for DMA\n");
+
 		imxdma->irq = irq;
 
 		irq_err = platform_get_irq(pdev, 1);
 		if (irq_err < 0) {
 			ret = irq_err;
-			goto disable_dma_ahb_clk;
+			return ret;
 		}
 
 		ret = devm_request_irq(&pdev->dev, irq_err,
 				       imxdma_err_handler, 0, "DMA", imxdma);
-		if (ret) {
-			dev_warn(imxdma->dev, "Can't register ERRIRQ for DMA\n");
-			goto disable_dma_ahb_clk;
-		}
+		if (ret)
+			return dev_err_probe(imxdma->dev, ret, "Can't register ERRIRQ for DMA\n");
+
 		imxdma->irq_err = irq_err;
 	}
 
@@ -1126,12 +1118,10 @@ static int __init imxdma_probe(struct platform_device *pdev)
 		if (!is_imx1_dma(imxdma)) {
 			ret = devm_request_irq(&pdev->dev, irq + i,
 					dma_irq_handler, 0, "DMA", imxdma);
-			if (ret) {
-				dev_warn(imxdma->dev, "Can't register IRQ %d "
-					 "for DMA channel %d\n",
-					 irq + i, i);
-				goto disable_dma_ahb_clk;
-			}
+			if (ret)
+				return dev_err_probe(imxdma->dev, ret,
+					"Can't register IRQ %d for DMA channel %d\n",
+					irq + i, i);
 
 			imxdmac->irq = irq + i;
 			timer_setup(&imxdmac->watchdog, imxdma_watchdog, 0);
@@ -1172,10 +1162,8 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	dma_set_max_seg_size(imxdma->dma_device.dev, 0xffffff);
 
 	ret = dma_async_device_register(&imxdma->dma_device);
-	if (ret) {
-		dev_err(&pdev->dev, "unable to register\n");
-		goto disable_dma_ahb_clk;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "unable to register\n");
 
 	if (pdev->dev.of_node) {
 		ret = of_dma_controller_register(pdev->dev.of_node,
@@ -1190,10 +1178,6 @@ static int __init imxdma_probe(struct platform_device *pdev)
 
 err_of_dma_controller:
 	dma_async_device_unregister(&imxdma->dma_device);
-disable_dma_ahb_clk:
-	clk_disable_unprepare(imxdma->dma_ahb);
-disable_dma_ipg_clk:
-	clk_disable_unprepare(imxdma->dma_ipg);
 	return ret;
 }
 
@@ -1226,9 +1210,6 @@ static void imxdma_remove(struct platform_device *pdev)
 
 	if (pdev->dev.of_node)
 		of_dma_controller_free(pdev->dev.of_node);
-
-	clk_disable_unprepare(imxdma->dma_ipg);
-	clk_disable_unprepare(imxdma->dma_ahb);
 }
 
 static struct platform_driver imxdma_driver = {
-- 
2.25.1

Re: [PATCH v2 3/6] dma:imx-dma:Use devm_clk_get_enabled() helpers

[Vinod Koul]

On 26-08-24, 20:21, Liao Yuanhong wrote:
> Use devm_clk_get_enabled() instead of clk functions in imx-dma.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
> ---
> v2:use dev_err_probe() instead of warn msg and return value.

This is not a way to post the v2, please post a new series.
While at it, please se dmaengine: imx...: Use devm... Please pay
attention to spaces and formatting as well

-- 
~Vinod

[PATCH 4/6] dma:imx-sdma:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in imx-sdma.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
 drivers/dma/imx-sdma.c | 57 ++++--------------------------------------
 1 file changed, 5 insertions(+), 52 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 72299a08af44..af972a4b6ce1 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1493,24 +1493,11 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 	sdmac->event_id0 = data->dma_request;
 	sdmac->event_id1 = data->dma_request2;
 
-	ret = clk_enable(sdmac->sdma->clk_ipg);
-	if (ret)
-		return ret;
-	ret = clk_enable(sdmac->sdma->clk_ahb);
-	if (ret)
-		goto disable_clk_ipg;
-
 	ret = sdma_set_channel_priority(sdmac, prio);
 	if (ret)
-		goto disable_clk_ahb;
+		return ret;
 
 	return 0;
-
-disable_clk_ahb:
-	clk_disable(sdmac->sdma->clk_ahb);
-disable_clk_ipg:
-	clk_disable(sdmac->sdma->clk_ipg);
-	return ret;
 }
 
 static void sdma_free_chan_resources(struct dma_chan *chan)
@@ -1530,9 +1517,6 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 	sdmac->event_id1 = 0;
 
 	sdma_set_channel_priority(sdmac, 0);
-
-	clk_disable(sdma->clk_ipg);
-	clk_disable(sdma->clk_ahb);
 }
 
 static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
@@ -2015,14 +1999,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context)
 	addr = (void *)header + header->script_addrs_start;
 	ram_code = (void *)header + header->ram_code_start;
 
-	clk_enable(sdma->clk_ipg);
-	clk_enable(sdma->clk_ahb);
 	/* download the RAM image for SDMA */
 	sdma_load_script(sdma, ram_code,
 			 header->ram_code_size,
 			 addr->ram_code_start_addr);
-	clk_disable(sdma->clk_ipg);
-	clk_disable(sdma->clk_ahb);
 
 	sdma_add_scripts(sdma, addr);
 
@@ -2119,13 +2099,6 @@ static int sdma_init(struct sdma_engine *sdma)
 	dma_addr_t ccb_phys;
 	int ccbsize;
 
-	ret = clk_enable(sdma->clk_ipg);
-	if (ret)
-		return ret;
-	ret = clk_enable(sdma->clk_ahb);
-	if (ret)
-		goto disable_clk_ipg;
-
 	if (sdma->drvdata->check_ratio &&
 	    (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg)))
 		sdma->clk_ratio = 1;
@@ -2180,15 +2153,9 @@ static int sdma_init(struct sdma_engine *sdma)
 	/* Initializes channel's priorities */
 	sdma_set_channel_priority(&sdma->channel[0], 7);
 
-	clk_disable(sdma->clk_ipg);
-	clk_disable(sdma->clk_ahb);
-
 	return 0;
 
 err_dma_alloc:
-	clk_disable(sdma->clk_ahb);
-disable_clk_ipg:
-	clk_disable(sdma->clk_ipg);
 	dev_err(sdma->dev, "initialisation failed with %d\n", ret);
 	return ret;
 }
@@ -2266,33 +2233,25 @@ static int sdma_probe(struct platform_device *pdev)
 	if (IS_ERR(sdma->regs))
 		return PTR_ERR(sdma->regs);
 
-	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	sdma->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
 	if (IS_ERR(sdma->clk_ipg))
 		return PTR_ERR(sdma->clk_ipg);
 
-	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
+	sdma->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
 	if (IS_ERR(sdma->clk_ahb))
 		return PTR_ERR(sdma->clk_ahb);
 
-	ret = clk_prepare(sdma->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare(sdma->clk_ahb);
-	if (ret)
-		goto err_clk;
-
 	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
 				dev_name(&pdev->dev), sdma);
 	if (ret)
-		goto err_irq;
+		return ret;
 
 	sdma->irq = irq;
 
 	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
 	if (!sdma->script_addrs) {
 		ret = -ENOMEM;
-		goto err_irq;
+		return ret;
 	}
 
 	/* initially no scripts available */
@@ -2407,10 +2366,6 @@ static int sdma_probe(struct platform_device *pdev)
 	dma_async_device_unregister(&sdma->dma_device);
 err_init:
 	kfree(sdma->script_addrs);
-err_irq:
-	clk_unprepare(sdma->clk_ahb);
-err_clk:
-	clk_unprepare(sdma->clk_ipg);
 	return ret;
 }
 
@@ -2422,8 +2377,6 @@ static void sdma_remove(struct platform_device *pdev)
 	devm_free_irq(&pdev->dev, sdma->irq, sdma);
 	dma_async_device_unregister(&sdma->dma_device);
 	kfree(sdma->script_addrs);
-	clk_unprepare(sdma->clk_ahb);
-	clk_unprepare(sdma->clk_ipg);
 	/* Kill the tasklet */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];
-- 
2.25.1

Re: [PATCH 4/6] dma:imx-sdma:Use devm_clk_get_enabled() helpers

[Jonathan Cameron]

On Fri, 23 Aug 2024 18:19:31 +0800
Liao Yuanhong <liaoyuanhong@vivo.com> wrote:

> Use devm_clk_get_enabled() instead of clk functions in imx-sdma.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
No.
Consider why the clocks are adjusted where they are in existing code
before 'cleaning' it up.

> ---
>  drivers/dma/imx-sdma.c | 57 ++++--------------------------------------
>  1 file changed, 5 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 72299a08af44..af972a4b6ce1 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1493,24 +1493,11 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
>  	sdmac->event_id0 = data->dma_request;
>  	sdmac->event_id1 = data->dma_request2;
>  
> -	ret = clk_enable(sdmac->sdma->clk_ipg);
> -	if (ret)
> -		return ret;
> -	ret = clk_enable(sdmac->sdma->clk_ahb);
> -	if (ret)
> -		goto disable_clk_ipg;
> -
>  	ret = sdma_set_channel_priority(sdmac, prio);
>  	if (ret)
> -		goto disable_clk_ahb;
> +		return ret;
>  
>  	return 0;
> -
> -disable_clk_ahb:
> -	clk_disable(sdmac->sdma->clk_ahb);
> -disable_clk_ipg:
> -	clk_disable(sdmac->sdma->clk_ipg);
> -	return ret;
>  }
>  
>  static void sdma_free_chan_resources(struct dma_chan *chan)
> @@ -1530,9 +1517,6 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
>  	sdmac->event_id1 = 0;
>  
>  	sdma_set_channel_priority(sdmac, 0);
> -
> -	clk_disable(sdma->clk_ipg);
> -	clk_disable(sdma->clk_ahb);
>  }
>  
>  static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
> @@ -2015,14 +1999,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context)
>  	addr = (void *)header + header->script_addrs_start;
>  	ram_code = (void *)header + header->ram_code_start;
>  
> -	clk_enable(sdma->clk_ipg);
> -	clk_enable(sdma->clk_ahb);
>  	/* download the RAM image for SDMA */
>  	sdma_load_script(sdma, ram_code,
>  			 header->ram_code_size,
>  			 addr->ram_code_start_addr);
> -	clk_disable(sdma->clk_ipg);
> -	clk_disable(sdma->clk_ahb);
Why do you think it is suddenly fine to leave the locks on here and it
wasn't before?

Check all the paths.

>  
>  	sdma_add_scripts(sdma, addr);
>  
> @@ -2119,13 +2099,6 @@ static int sdma_init(struct sdma_engine *sdma)
>  	dma_addr_t ccb_phys;
>  	int ccbsize;
>  
> -	ret = clk_enable(sdma->clk_ipg);
> -	if (ret)
> -		return ret;
> -	ret = clk_enable(sdma->clk_ahb);
> -	if (ret)
> -		goto disable_clk_ipg;
> -
>  	if (sdma->drvdata->check_ratio &&
>  	    (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg)))
>  		sdma->clk_ratio = 1;
> @@ -2180,15 +2153,9 @@ static int sdma_init(struct sdma_engine *sdma)
>  	/* Initializes channel's priorities */
>  	sdma_set_channel_priority(&sdma->channel[0], 7);
>  
> -	clk_disable(sdma->clk_ipg);
> -	clk_disable(sdma->clk_ahb);
> -
>  	return 0;
>  
>  err_dma_alloc:
> -	clk_disable(sdma->clk_ahb);
> -disable_clk_ipg:
> -	clk_disable(sdma->clk_ipg);
>  	dev_err(sdma->dev, "initialisation failed with %d\n", ret);
>  	return ret;
>  }
> @@ -2266,33 +2233,25 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (IS_ERR(sdma->regs))
>  		return PTR_ERR(sdma->regs);
>  
> -	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	sdma->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
>  	if (IS_ERR(sdma->clk_ipg))
>  		return PTR_ERR(sdma->clk_ipg);
>  
> -	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	sdma->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
>  	if (IS_ERR(sdma->clk_ahb))
>  		return PTR_ERR(sdma->clk_ahb);
>  
> -	ret = clk_prepare(sdma->clk_ipg);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare(sdma->clk_ahb);
> -	if (ret)
> -		goto err_clk;
> -
>  	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
>  				dev_name(&pdev->dev), sdma);
>  	if (ret)
> -		goto err_irq;
> +		return ret;
>  
>  	sdma->irq = irq;
>  
>  	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
>  	if (!sdma->script_addrs) {
>  		ret = -ENOMEM;
> -		goto err_irq;
> +		return ret;
>  	}
>  
>  	/* initially no scripts available */
> @@ -2407,10 +2366,6 @@ static int sdma_probe(struct platform_device *pdev)
>  	dma_async_device_unregister(&sdma->dma_device);
>  err_init:
>  	kfree(sdma->script_addrs);
> -err_irq:
> -	clk_unprepare(sdma->clk_ahb);
> -err_clk:
> -	clk_unprepare(sdma->clk_ipg);
>  	return ret;
>  }
>  
> @@ -2422,8 +2377,6 @@ static void sdma_remove(struct platform_device *pdev)
>  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
>  	dma_async_device_unregister(&sdma->dma_device);
>  	kfree(sdma->script_addrs);
> -	clk_unprepare(sdma->clk_ahb);
> -	clk_unprepare(sdma->clk_ipg);
>  	/* Kill the tasklet */
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];

Re: [PATCH 4/6] dma:imx-sdma:Use devm_clk_get_enabled() helpers

[Frank Li]

On Fri, Aug 23, 2024 at 01:08:35PM +0100, Jonathan Cameron wrote:
> On Fri, 23 Aug 2024 18:19:31 +0800
> Liao Yuanhong <liaoyuanhong@vivo.com> wrote:
>
> > Use devm_clk_get_enabled() instead of clk functions in imx-sdma.
> >
> > Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
> No.
> Consider why the clocks are adjusted where they are in existing code
> before 'cleaning' it up.
>
> > ---
> >  drivers/dma/imx-sdma.c | 57 ++++--------------------------------------
> >  1 file changed, 5 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 72299a08af44..af972a4b6ce1 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -1493,24 +1493,11 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
> >  	sdmac->event_id0 = data->dma_request;
> >  	sdmac->event_id1 = data->dma_request2;
> >
> > -	ret = clk_enable(sdmac->sdma->clk_ipg);
> > -	if (ret)
> > -		return ret;
> > -	ret = clk_enable(sdmac->sdma->clk_ahb);
> > -	if (ret)
> > -		goto disable_clk_ipg;
> > -
> >  	ret = sdma_set_channel_priority(sdmac, prio);
> >  	if (ret)
> > -		goto disable_clk_ahb;
> > +		return ret;
> >
> >  	return 0;
> > -
> > -disable_clk_ahb:
> > -	clk_disable(sdmac->sdma->clk_ahb);
> > -disable_clk_ipg:
> > -	clk_disable(sdmac->sdma->clk_ipg);
> > -	return ret;
> >  }
> >
> >  static void sdma_free_chan_resources(struct dma_chan *chan)
> > @@ -1530,9 +1517,6 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
> >  	sdmac->event_id1 = 0;
> >
> >  	sdma_set_channel_priority(sdmac, 0);
> > -
> > -	clk_disable(sdma->clk_ipg);
> > -	clk_disable(sdma->clk_ahb);
> >  }
> >
> >  static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
> > @@ -2015,14 +1999,10 @@ static void sdma_load_firmware(const struct firmware *fw, void *context)
> >  	addr = (void *)header + header->script_addrs_start;
> >  	ram_code = (void *)header + header->ram_code_start;
> >
> > -	clk_enable(sdma->clk_ipg);
> > -	clk_enable(sdma->clk_ahb);
> >  	/* download the RAM image for SDMA */
> >  	sdma_load_script(sdma, ram_code,
> >  			 header->ram_code_size,
> >  			 addr->ram_code_start_addr);
> > -	clk_disable(sdma->clk_ipg);
> > -	clk_disable(sdma->clk_ahb);
> Why do you think it is suddenly fine to leave the locks on here and it
> wasn't before?
>
> Check all the paths.
>
> >
> >  	sdma_add_scripts(sdma, addr);
> >
> > @@ -2119,13 +2099,6 @@ static int sdma_init(struct sdma_engine *sdma)
> >  	dma_addr_t ccb_phys;
> >  	int ccbsize;
> >
> > -	ret = clk_enable(sdma->clk_ipg);
> > -	if (ret)
> > -		return ret;
> > -	ret = clk_enable(sdma->clk_ahb);
> > -	if (ret)
> > -		goto disable_clk_ipg;
> > -
> >  	if (sdma->drvdata->check_ratio &&
> >  	    (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg)))
> >  		sdma->clk_ratio = 1;
> > @@ -2180,15 +2153,9 @@ static int sdma_init(struct sdma_engine *sdma)
> >  	/* Initializes channel's priorities */
> >  	sdma_set_channel_priority(&sdma->channel[0], 7);
> >
> > -	clk_disable(sdma->clk_ipg);
> > -	clk_disable(sdma->clk_ahb);
> > -
> >  	return 0;
> >
> >  err_dma_alloc:
> > -	clk_disable(sdma->clk_ahb);
> > -disable_clk_ipg:
> > -	clk_disable(sdma->clk_ipg);
> >  	dev_err(sdma->dev, "initialisation failed with %d\n", ret);
> >  	return ret;
> >  }
> > @@ -2266,33 +2233,25 @@ static int sdma_probe(struct platform_device *pdev)
> >  	if (IS_ERR(sdma->regs))
> >  		return PTR_ERR(sdma->regs);
> >
> > -	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > +	sdma->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> >  	if (IS_ERR(sdma->clk_ipg))
> >  		return PTR_ERR(sdma->clk_ipg);
> >
> > -	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> > +	sdma->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");

Supposed it should be devm_clk_get_prepared() here because below code use
clk_prepare().

Please cc to imx@lists.linux.dev next time.

Frank

> >  	if (IS_ERR(sdma->clk_ahb))
> >  		return PTR_ERR(sdma->clk_ahb);
> >
> > -	ret = clk_prepare(sdma->clk_ipg);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = clk_prepare(sdma->clk_ahb);
> > -	if (ret)
> > -		goto err_clk;
> > -
> >  	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
> >  				dev_name(&pdev->dev), sdma);
> >  	if (ret)
> > -		goto err_irq;
> > +		return ret;
> >
> >  	sdma->irq = irq;
> >
> >  	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
> >  	if (!sdma->script_addrs) {
> >  		ret = -ENOMEM;
> > -		goto err_irq;
> > +		return ret;
> >  	}
> >
> >  	/* initially no scripts available */
> > @@ -2407,10 +2366,6 @@ static int sdma_probe(struct platform_device *pdev)
> >  	dma_async_device_unregister(&sdma->dma_device);
> >  err_init:
> >  	kfree(sdma->script_addrs);
> > -err_irq:
> > -	clk_unprepare(sdma->clk_ahb);
> > -err_clk:
> > -	clk_unprepare(sdma->clk_ipg);
> >  	return ret;
> >  }
> >
> > @@ -2422,8 +2377,6 @@ static void sdma_remove(struct platform_device *pdev)
> >  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
> >  	dma_async_device_unregister(&sdma->dma_device);
> >  	kfree(sdma->script_addrs);
> > -	clk_unprepare(sdma->clk_ahb);
> > -	clk_unprepare(sdma->clk_ipg);
> >  	/* Kill the tasklet */
> >  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> >  		struct sdma_channel *sdmac = &sdma->channel[i];
>

[PATCH v2 4/6] dma:imx-sdma:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in imx-sdma.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
v2:Remove all enable related modifications, replace clk_prepare() with
devm_clk_get_prepared(), and configure COMPILE_TEST on the corresponding
node in Kconfig for easy compilation and debugging.
---
 drivers/dma/Kconfig    |  4 ++--
 drivers/dma/imx-sdma.c | 22 ++++------------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index cc0a62c34861..c4ffb5d3e321 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -264,7 +264,7 @@ config IMG_MDC_DMA
 
 config IMX_DMA
 	tristate "i.MX DMA support"
-	depends on ARCH_MXC
+	depends on ARCH_MXC || COMPILE_TEST
 	select DMA_ENGINE
 	help
 	  Support the i.MX DMA engine. This engine is integrated into
@@ -272,7 +272,7 @@ config IMX_DMA
 
 config IMX_SDMA
 	tristate "i.MX SDMA support"
-	depends on ARCH_MXC
+	depends on ARCH_MXC || COMPILE_TEST
 	select DMA_ENGINE
 	select DMA_VIRTUAL_CHANNELS
 	help
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 72299a08af44..07a017c40a82 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2266,33 +2266,25 @@ static int sdma_probe(struct platform_device *pdev)
 	if (IS_ERR(sdma->regs))
 		return PTR_ERR(sdma->regs);
 
-	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	sdma->clk_ipg = devm_clk_get_prepared(&pdev->dev, "ipg");
 	if (IS_ERR(sdma->clk_ipg))
 		return PTR_ERR(sdma->clk_ipg);
 
-	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
+	sdma->clk_ahb = devm_clk_get_prepared(&pdev->dev, "ahb");
 	if (IS_ERR(sdma->clk_ahb))
 		return PTR_ERR(sdma->clk_ahb);
 
-	ret = clk_prepare(sdma->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare(sdma->clk_ahb);
-	if (ret)
-		goto err_clk;
-
 	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
 				dev_name(&pdev->dev), sdma);
 	if (ret)
-		goto err_irq;
+		return ret;
 
 	sdma->irq = irq;
 
 	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
 	if (!sdma->script_addrs) {
 		ret = -ENOMEM;
-		goto err_irq;
+		return ret;
 	}
 
 	/* initially no scripts available */
@@ -2407,10 +2399,6 @@ static int sdma_probe(struct platform_device *pdev)
 	dma_async_device_unregister(&sdma->dma_device);
 err_init:
 	kfree(sdma->script_addrs);
-err_irq:
-	clk_unprepare(sdma->clk_ahb);
-err_clk:
-	clk_unprepare(sdma->clk_ipg);
 	return ret;
 }
 
@@ -2422,8 +2410,6 @@ static void sdma_remove(struct platform_device *pdev)
 	devm_free_irq(&pdev->dev, sdma->irq, sdma);
 	dma_async_device_unregister(&sdma->dma_device);
 	kfree(sdma->script_addrs);
-	clk_unprepare(sdma->clk_ahb);
-	clk_unprepare(sdma->clk_ipg);
 	/* Kill the tasklet */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];
-- 
2.25.1

[PATCH 5/6] dma:milbeaut-hdmac:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in milbeaut-hdmac.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
 drivers/dma/milbeaut-hdmac.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/milbeaut-hdmac.c b/drivers/dma/milbeaut-hdmac.c
index 7b41c670970a..b188bfa9613a 100644
--- a/drivers/dma/milbeaut-hdmac.c
+++ b/drivers/dma/milbeaut-hdmac.c
@@ -75,7 +75,6 @@ struct milbeaut_hdmac_chan {
 
 struct milbeaut_hdmac_device {
 	struct dma_device ddev;
-	struct clk *clk;
 	void __iomem *reg_base;
 	struct milbeaut_hdmac_chan channels[];
 };
@@ -458,6 +457,7 @@ static int milbeaut_hdmac_probe(struct platform_device *pdev)
 	struct milbeaut_hdmac_device *mdev;
 	struct dma_device *ddev;
 	int nr_chans, ret, i;
+	struct clk *clk;
 
 	nr_chans = platform_irq_count(pdev);
 	if (nr_chans < 0)
@@ -476,16 +476,12 @@ static int milbeaut_hdmac_probe(struct platform_device *pdev)
 	if (IS_ERR(mdev->reg_base))
 		return PTR_ERR(mdev->reg_base);
 
-	mdev->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(mdev->clk)) {
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk)) {
 		dev_err(dev, "failed to get clock\n");
-		return PTR_ERR(mdev->clk);
+		return PTR_ERR(clk);
 	}
 
-	ret = clk_prepare_enable(mdev->clk);
-	if (ret)
-		return ret;
-
 	ddev = &mdev->ddev;
 	ddev->dev = dev;
 	dma_cap_set(DMA_SLAVE, ddev->cap_mask);
@@ -507,12 +503,12 @@ static int milbeaut_hdmac_probe(struct platform_device *pdev)
 	for (i = 0; i < nr_chans; i++) {
 		ret = milbeaut_hdmac_chan_init(pdev, mdev, i);
 		if (ret)
-			goto disable_clk;
+			return ret;
 	}
 
 	ret = dma_async_device_register(ddev);
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	ret = of_dma_controller_register(dev->of_node,
 					 milbeaut_hdmac_xlate, mdev);
@@ -525,9 +521,6 @@ static int milbeaut_hdmac_probe(struct platform_device *pdev)
 
 unregister_dmac:
 	dma_async_device_unregister(ddev);
-disable_clk:
-	clk_disable_unprepare(mdev->clk);
-
 	return ret;
 }
 
@@ -560,7 +553,6 @@ static void milbeaut_hdmac_remove(struct platform_device *pdev)
 
 	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&mdev->ddev);
-	clk_disable_unprepare(mdev->clk);
 }
 
 static const struct of_device_id milbeaut_hdmac_match[] = {
-- 
2.25.1

Re: [PATCH 5/6] dma:milbeaut-hdmac:Use devm_clk_get_enabled() helpers

[Jonathan Cameron]

On Fri, 23 Aug 2024 18:19:32 +0800
Liao Yuanhong <liaoyuanhong@vivo.com> wrote:

> Use devm_clk_get_enabled() instead of clk functions in milbeaut-hdmac.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
This one is fine I think
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 6/6] dma:uniphier-mdmac:Use devm_clk_get_enabled() helpers

[Liao Yuanhong]

Use devm_clk_get_enabled() instead of clk functions in uniphier-mdmac.

Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
---
 drivers/dma/uniphier-mdmac.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/uniphier-mdmac.c b/drivers/dma/uniphier-mdmac.c
index ad7125f6e2ca..6b3570440b70 100644
--- a/drivers/dma/uniphier-mdmac.c
+++ b/drivers/dma/uniphier-mdmac.c
@@ -66,7 +66,6 @@ struct uniphier_mdmac_chan {
 
 struct uniphier_mdmac_device {
 	struct dma_device ddev;
-	struct clk *clk;
 	void __iomem *reg_base;
 	struct uniphier_mdmac_chan channels[];
 };
@@ -383,6 +382,7 @@ static int uniphier_mdmac_probe(struct platform_device *pdev)
 	struct uniphier_mdmac_device *mdev;
 	struct dma_device *ddev;
 	int nr_chans, ret, i;
+	struct clk *clk;
 
 	nr_chans = platform_irq_count(pdev);
 	if (nr_chans < 0)
@@ -401,16 +401,12 @@ static int uniphier_mdmac_probe(struct platform_device *pdev)
 	if (IS_ERR(mdev->reg_base))
 		return PTR_ERR(mdev->reg_base);
 
-	mdev->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(mdev->clk)) {
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk)) {
 		dev_err(dev, "failed to get clock\n");
-		return PTR_ERR(mdev->clk);
+		return PTR_ERR(clk);
 	}
 
-	ret = clk_prepare_enable(mdev->clk);
-	if (ret)
-		return ret;
-
 	ddev = &mdev->ddev;
 	ddev->dev = dev;
 	dma_cap_set(DMA_PRIVATE, ddev->cap_mask);
@@ -429,12 +425,12 @@ static int uniphier_mdmac_probe(struct platform_device *pdev)
 	for (i = 0; i < nr_chans; i++) {
 		ret = uniphier_mdmac_chan_init(pdev, mdev, i);
 		if (ret)
-			goto disable_clk;
+			return ret;
 	}
 
 	ret = dma_async_device_register(ddev);
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	ret = of_dma_controller_register(dev->of_node, of_dma_xlate_by_chan_id,
 					 ddev);
@@ -447,9 +443,6 @@ static int uniphier_mdmac_probe(struct platform_device *pdev)
 
 unregister_dmac:
 	dma_async_device_unregister(ddev);
-disable_clk:
-	clk_disable_unprepare(mdev->clk);
-
 	return ret;
 }
 
@@ -482,7 +475,6 @@ static void uniphier_mdmac_remove(struct platform_device *pdev)
 
 	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&mdev->ddev);
-	clk_disable_unprepare(mdev->clk);
 }
 
 static const struct of_device_id uniphier_mdmac_match[] = {
-- 
2.25.1

Re: [PATCH 6/6] dma:uniphier-mdmac:Use devm_clk_get_enabled() helpers

[Jonathan Cameron]

On Fri, 23 Aug 2024 18:19:33 +0800
Liao Yuanhong <liaoyuanhong@vivo.com> wrote:

> Use devm_clk_get_enabled() instead of clk functions in uniphier-mdmac.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@vivo.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>