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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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