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

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

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

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