linux-kernel - Re: [PATCH 2/6] dma:dma-jz4780:Use devm_clk_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240823130117.00007e25@Huawei.com>
Date: Fri, 23 Aug 2024 13:01:17 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Liao Yuanhong <liaoyuanhong@...o.com>
CC: <vkoul@...nel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<dmaengine@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/6] dma:dma-jz4780:Use devm_clk_get_enabled() helpers

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

> Use devm_clk_get_enabled() instead of clk functions in dma-jz4780.
> 
> Signed-off-by: Liao Yuanhong <liaoyuanhong@...o.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++)