[PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Bence Csókás]

Clean up error handling by using devm functions
and dev_err_probe(). This should make it easier
to add new code, as we can eliminate the "goto
ladder" in probe().

Suggested-by: Chen-Yu Tsai <wens@kernel.org>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---

Notes:
    Changes in v2:
    * rebase on current next
    Changes in v3:
    * rebase on current next
    * collect Jernej's tag
    Changes in v4:
    * rebase on current next
    * collect Chen-Yu's tag

 drivers/dma/sun4i-dma.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
index 24796aaaddfa..b10639720efd 100644
--- a/drivers/dma/sun4i-dma.c
+++ b/drivers/dma/sun4i-dma.c
@@ -1249,10 +1249,9 @@ static int sun4i_dma_probe(struct platform_device *pdev)
 	if (priv->irq < 0)
 		return priv->irq;
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
-		dev_err(&pdev->dev, "No clock specified\n");
-		return PTR_ERR(priv->clk);
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), "Couldn't start the clock");
 	}
 
 	if (priv->cfg->has_reset) {
@@ -1328,12 +1327,6 @@ static int sun4i_dma_probe(struct platform_device *pdev)
 		vchan_init(&vchan->vc, &priv->slave);
 	}
 
-	ret = clk_prepare_enable(priv->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Couldn't enable the clock\n");
-		return ret;
-	}
-
 	/*
 	 * Make sure the IRQs are all disabled and accounted for. The bootloader
 	 * likes to leave these dirty
@@ -1344,32 +1337,23 @@ static int sun4i_dma_probe(struct platform_device *pdev)
 	ret = devm_request_irq(&pdev->dev, priv->irq, sun4i_dma_interrupt,
 			       0, dev_name(&pdev->dev), priv);
 	if (ret) {
-		dev_err(&pdev->dev, "Cannot request IRQ\n");
-		goto err_clk_disable;
+		return dev_err_probe(&pdev->dev, ret, "Cannot request IRQ");
 	}
 
-	ret = dma_async_device_register(&priv->slave);
+	ret = dmaenginem_async_device_register(&priv->slave);
 	if (ret) {
-		dev_warn(&pdev->dev, "Failed to register DMA engine device\n");
-		goto err_clk_disable;
+		return dev_err_probe(&pdev->dev, ret, "Failed to register DMA engine device");
 	}
 
 	ret = of_dma_controller_register(pdev->dev.of_node, sun4i_dma_of_xlate,
 					 priv);
 	if (ret) {
-		dev_err(&pdev->dev, "of_dma_controller_register failed\n");
-		goto err_dma_unregister;
+		return dev_err_probe(&pdev->dev, ret, "Failed to register translation function");
 	}
 
 	dev_dbg(&pdev->dev, "Successfully probed SUN4I_DMA\n");
 
 	return 0;
-
-err_dma_unregister:
-	dma_async_device_unregister(&priv->slave);
-err_clk_disable:
-	clk_disable_unprepare(priv->clk);
-	return ret;
 }
 
 static void sun4i_dma_remove(struct platform_device *pdev)
@@ -1380,9 +1364,6 @@ static void sun4i_dma_remove(struct platform_device *pdev)
 	disable_irq(priv->irq);
 
 	of_dma_controller_free(pdev->dev.of_node);
-	dma_async_device_unregister(&priv->slave);
-
-	clk_disable_unprepare(priv->clk);
 }
 
 static struct sun4i_dma_config sun4i_a10_dma_cfg = {
-- 
2.48.1

Re: [PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Markus Elfring]

> Clean up error handling by using devm functions
> and dev_err_probe(). This should make it easier
…

You may occasionally put more than 47 characters into text lines
of such a change description.

Regards,
Markus

Re: [PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Markus Elfring]

> Clean up error handling by using devm functions
> and dev_err_probe(). …

How good does such a change combination fit to the patch requirement
according to separation of concerns?
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81

Regards,
Markus

Re: [PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Csókás Bence]

Dear Markus,

On 2025. 03. 11. 20:33, Markus Elfring wrote:
 >> Clean up error handling by using devm functions
 >> and dev_err_probe(). This should make it easier
 > …
 >
 > You may occasionally put more than 47 characters into text lines
 > of such a change description.

It was an old patch I hadn't yet reformatted to 75 cols. I used 50-60 
cols before, because my mail client's preview panel is very narrow, so 
anything more than ~65 characters will wrap. If there will be a v5, I'll 
reformat it to 75 cols as well, as per the style guidelines.

> How good does such a change combination fit to the patch requirement
> according to separation of concerns?
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81

It is a general refactor patch, it shouldn't change any functionality. I 
could split it to one part introducing `devm_clk_get_enabled()` and the 
other `dmaenginem_async_device_register()`, but I don't feel that to be 
necessary, nor does it bring any advantages I believe.

> Regards,
> Markus

Bence

Re: [v4] dma-engine: sun4i: Use devm functions in probe()

[Markus Elfring]

>> How good does such a change combination fit to the patch requirement
>> according to separation of concerns?
>> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81
>
> It is a general refactor patch, it shouldn't change any functionality. I could split it to one part introducing `devm_clk_get_enabled()` and the other `dmaenginem_async_device_register()`, but I don't feel that to be necessary, nor does it bring any advantages I believe.
Can it matter a bit more to separate changes for the application of devm functions
and the adjustment of corresponding exception handling with dev_err_probe() calls?

Regards,
Markus

Re: [v4] dma-engine: sun4i: Use devm functions in probe()

[Csókás Bence]

Hi,

On 2025. 03. 12. 12:44, Markus Elfring wrote:
>>> How good does such a change combination fit to the patch requirement
>>> according to separation of concerns?
>>> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81
>>
>> It is a general refactor patch, it shouldn't change any functionality. I could split it to one part introducing `devm_clk_get_enabled()` and the other `dmaenginem_async_device_register()`, but I don't feel that to be necessary, nor does it bring any advantages I believe.
> Can it matter a bit more to separate changes for the application of devm functions
> and the adjustment of corresponding exception handling with dev_err_probe() calls?

The change in error handling is just the result of switching to devm 
functions, because it is no longer needed to separately dev_err(), store 
the error code to `ret` and goto a cleanup phase (as the whole point of 
using devm functions is to have auto-cleanup), you can just return with 
the error code (which dev_err_probe() returns for us) right away. The 
devm functions are used precisely _because_ they allow us to simplify 
this error handling.

> Regards,
> Markus

Bence

Re: [PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Csókás Bence]

Hi all,

On 2025. 03. 11. 19:02, Bence Csókás wrote:
> Clean up error handling by using devm functions
> and dev_err_probe(). This should make it easier
> to add new code, as we can eliminate the "goto
> ladder" in probe().
> 
> Suggested-by: Chen-Yu Tsai <wens@kernel.org>
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>

So, will this be merged for the MW of 6.15? I just looked at the history 
of this patch, and there haven't been any major changes since v1, which 
was sent on 8 Dec last year. Despite this, it didn't make it into 6.14, 
probably because it was forgot amid the MW rush, so now I'm anxious as 
to whether it will happen again.

Bence

Re: [PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Christophe JAILLET]

Le 11/03/2025 à 19:02, Bence Csókás a écrit :
> Clean up error handling by using devm functions
> and dev_err_probe(). This should make it easier
> to add new code, as we can eliminate the "goto
> ladder" in probe().
> 
> Suggested-by: Chen-Yu Tsai <wens@kernel.org>
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> ---
> 
> Notes:
>      Changes in v2:
>      * rebase on current next
>      Changes in v3:
>      * rebase on current next
>      * collect Jernej's tag
>      Changes in v4:
>      * rebase on current next
>      * collect Chen-Yu's tag
> 
>   drivers/dma/sun4i-dma.c | 31 ++++++-------------------------
>   1 file changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
> index 24796aaaddfa..b10639720efd 100644
> --- a/drivers/dma/sun4i-dma.c
> +++ b/drivers/dma/sun4i-dma.c
> @@ -1249,10 +1249,9 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>   	if (priv->irq < 0)
>   		return priv->irq;
>   
> -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>   	if (IS_ERR(priv->clk)) {
> -		dev_err(&pdev->dev, "No clock specified\n");
> -		return PTR_ERR(priv->clk);
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), "Couldn't start the clock");

Why removing the trailing \n everywhere?

Any reference esxplaing why it is correct?

CJ

>   	}
>   
>   	if (priv->cfg->has_reset) {
> @@ -1328,12 +1327,6 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>   		vchan_init(&vchan->vc, &priv->slave);
>   	}
>   
> -	ret = clk_prepare_enable(priv->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Couldn't enable the clock\n");
> -		return ret;
> -	}
> -
>   	/*
>   	 * Make sure the IRQs are all disabled and accounted for. The bootloader
>   	 * likes to leave these dirty
> @@ -1344,32 +1337,23 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>   	ret = devm_request_irq(&pdev->dev, priv->irq, sun4i_dma_interrupt,
>   			       0, dev_name(&pdev->dev), priv);
>   	if (ret) {
> -		dev_err(&pdev->dev, "Cannot request IRQ\n");
> -		goto err_clk_disable;
> +		return dev_err_probe(&pdev->dev, ret, "Cannot request IRQ");
>   	}
>   
> -	ret = dma_async_device_register(&priv->slave);
> +	ret = dmaenginem_async_device_register(&priv->slave);
>   	if (ret) {
> -		dev_warn(&pdev->dev, "Failed to register DMA engine device\n");
> -		goto err_clk_disable;
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register DMA engine device");
>   	}
>   
>   	ret = of_dma_controller_register(pdev->dev.of_node, sun4i_dma_of_xlate,
>   					 priv);
>   	if (ret) {
> -		dev_err(&pdev->dev, "of_dma_controller_register failed\n");
> -		goto err_dma_unregister;
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register translation function");
>   	}
>   
>   	dev_dbg(&pdev->dev, "Successfully probed SUN4I_DMA\n");
>   
>   	return 0;
> -
> -err_dma_unregister:
> -	dma_async_device_unregister(&priv->slave);
> -err_clk_disable:
> -	clk_disable_unprepare(priv->clk);
> -	return ret;
>   }
>   
>   static void sun4i_dma_remove(struct platform_device *pdev)
> @@ -1380,9 +1364,6 @@ static void sun4i_dma_remove(struct platform_device *pdev)
>   	disable_irq(priv->irq);
>   
>   	of_dma_controller_free(pdev->dev.of_node);
> -	dma_async_device_unregister(&priv->slave);
> -
> -	clk_disable_unprepare(priv->clk);
>   }
>   
>   static struct sun4i_dma_config sun4i_a10_dma_cfg = {

Re: [PATCH v4] dma-engine: sun4i: Use devm functions in probe()

[Csókás Bence]

Hi,

On 2025. 03. 22. 20:13, Christophe JAILLET wrote:
> Le 11/03/2025 à 19:02, Bence Csókás a écrit :
>> Clean up error handling by using devm functions
>> and dev_err_probe(). This should make it easier
>> to add new code, as we can eliminate the "goto
>> ladder" in probe().
>>
>> Suggested-by: Chen-Yu Tsai <wens@kernel.org>
>> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Acked-by: Chen-Yu Tsai <wens@csie.org>
>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      * rebase on current next
>>      Changes in v3:
>>      * rebase on current next
>>      * collect Jernej's tag
>>      Changes in v4:
>>      * rebase on current next
>>      * collect Chen-Yu's tag
>>
>>   drivers/dma/sun4i-dma.c | 31 ++++++-------------------------
>>   1 file changed, 6 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
>> index 24796aaaddfa..b10639720efd 100644
>> --- a/drivers/dma/sun4i-dma.c
>> +++ b/drivers/dma/sun4i-dma.c
>> @@ -1249,10 +1249,9 @@ static int sun4i_dma_probe(struct 
>> platform_device *pdev)
>>       if (priv->irq < 0)
>>           return priv->irq;
>> -    priv->clk = devm_clk_get(&pdev->dev, NULL);
>> +    priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>>       if (IS_ERR(priv->clk)) {
>> -        dev_err(&pdev->dev, "No clock specified\n");
>> -        return PTR_ERR(priv->clk);
>> +        return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), 
>> "Couldn't start the clock");
> 
> Why removing the trailing \n everywhere?
> 
> Any reference esxplaing why it is correct?
> 
> CJ

Well, printk() will add a newline unless LOG_CONT is used. However, 
while looking up this feature, it seems that the consensus is to keep 
the `\n`s, even though they are not strictly needed anymore. I'll update 
this.

Bence