diff mbox series

[1/2] usb: dwc3: Use helper function devm_clk_get_enabled()

Message ID 20240902123020.29267-2-zhangzekun11@huawei.com (mailing list archive)
State New
Headers show
Series Use helper function devm_clk_get_enabled() | expand

Commit Message

zhangzekun (A) Sept. 2, 2024, 12:30 p.m. UTC 
devm_clk_get() and clk_prepare_enable() can be replaced by helper
function devm_clk_get_enabled(). Let's use devm_clk_get_enabled() to
simplify code and avoid calling clk_disable_unprepare().

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/usb/dwc3/dwc3-imx8mp.c | 47 ++++++++--------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

Comments

Thinh Nguyen Sept. 3, 2024, 10:06 p.m. UTC | #1 
On Mon, Sep 02, 2024, Zhang Zekun wrote:
> devm_clk_get() and clk_prepare_enable() can be replaced by helper
> function devm_clk_get_enabled(). Let's use devm_clk_get_enabled() to
> simplify code and avoid calling clk_disable_unprepare().
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  drivers/usb/dwc3/dwc3-imx8mp.c | 47 ++++++++--------------------------
>  1 file changed, 11 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c b/drivers/usb/dwc3/dwc3-imx8mp.c
> index 392fa1232788..a7e5ee797ae7 100644
> --- a/drivers/usb/dwc3/dwc3-imx8mp.c
> +++ b/drivers/usb/dwc3/dwc3-imx8mp.c
> @@ -178,37 +178,20 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
>  			return PTR_ERR(dwc3_imx->glue_base);
>  	}
>  
> -	dwc3_imx->hsio_clk = devm_clk_get(dev, "hsio");
> -	if (IS_ERR(dwc3_imx->hsio_clk)) {
> -		err = PTR_ERR(dwc3_imx->hsio_clk);
> -		dev_err(dev, "Failed to get hsio clk, err=%d\n", err);
> -		return err;
> -	}
> +	dwc3_imx->hsio_clk = devm_clk_get_enabled(dev, "hsio");
> +	if (IS_ERR(dwc3_imx->hsio_clk))
> +		return dev_err_probe(dev, PTR_ERR(dwc3_imx->hsio_clk),
> +				     "Failed to get and enable hsio clk\n");
>  
> -	err = clk_prepare_enable(dwc3_imx->hsio_clk);
> -	if (err) {
> -		dev_err(dev, "Failed to enable hsio clk, err=%d\n", err);
> -		return err;
> -	}
> -
> -	dwc3_imx->suspend_clk = devm_clk_get(dev, "suspend");
> -	if (IS_ERR(dwc3_imx->suspend_clk)) {
> -		err = PTR_ERR(dwc3_imx->suspend_clk);
> -		dev_err(dev, "Failed to get suspend clk, err=%d\n", err);
> -		goto disable_hsio_clk;
> -	}
> -
> -	err = clk_prepare_enable(dwc3_imx->suspend_clk);
> -	if (err) {
> -		dev_err(dev, "Failed to enable suspend clk, err=%d\n", err);
> -		goto disable_hsio_clk;
> -	}
> +	dwc3_imx->suspend_clk = devm_clk_get_enabled(dev, "suspend");
> +	if (IS_ERR(dwc3_imx->suspend_clk))
> +		return dev_err_probe(dev, PTR_ERR(dwc3_imx->suspend_clk),
> +				     "Failed to get and enable suspend clk\n");
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0) {
> -		err = irq;
> -		goto disable_clks;
> -	}
> +	if (irq < 0)
> +		return irq;
> +
>  	dwc3_imx->irq = irq;
>  
>  	imx8mp_configure_glue(dwc3_imx);
> @@ -259,25 +242,17 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
>  disable_rpm:
>  	pm_runtime_disable(dev);
>  	pm_runtime_put_noidle(dev);
> -disable_clks:
> -	clk_disable_unprepare(dwc3_imx->suspend_clk);
> -disable_hsio_clk:
> -	clk_disable_unprepare(dwc3_imx->hsio_clk);
>  
>  	return err;
>  }
>  
>  static void dwc3_imx8mp_remove(struct platform_device *pdev)
>  {
> -	struct dwc3_imx8mp *dwc3_imx = platform_get_drvdata(pdev);
>  	struct device *dev = &pdev->dev;
>  
>  	pm_runtime_get_sync(dev);
>  	of_platform_depopulate(dev);
>  
> -	clk_disable_unprepare(dwc3_imx->suspend_clk);
> -	clk_disable_unprepare(dwc3_imx->hsio_clk);
> -
>  	pm_runtime_disable(dev);
>  	pm_runtime_put_noidle(dev);
>  }
> -- 
> 2.17.1
> 

Krzysztof Kozlowski already submitted some cleanup related to this:
https://lore.kernel.org/linux-usb/20240827012651.j2chqblkjha2vene@synopsys.com/T/#u

BR,
Thinh
Greg KH Sept. 4, 2024, 8:49 a.m. UTC | #2 
On Mon, Sep 02, 2024 at 08:30:19PM +0800, Zhang Zekun wrote:
> devm_clk_get() and clk_prepare_enable() can be replaced by helper
> function devm_clk_get_enabled(). Let's use devm_clk_get_enabled() to
> simplify code and avoid calling clk_disable_unprepare().
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

Also, have you tested that this actually works?  using devm can have
tricky sync issues when shutting down so I'm going to start requiring
that any conversions like this be proven to work properly on real
hardware.

thanks,

greg k-h
zhangzekun (A) Sept. 5, 2024, 1:13 a.m. UTC | #3 
在 2024/9/4 16:49, Greg KH 写道:
> On Mon, Sep 02, 2024 at 08:30:19PM +0800, Zhang Zekun wrote:
>> devm_clk_get() and clk_prepare_enable() can be replaced by helper
>> function devm_clk_get_enabled(). Let's use devm_clk_get_enabled() to
>> simplify code and avoid calling clk_disable_unprepare().
>>
>> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> 
> Also, have you tested that this actually works?  using devm can have
> tricky sync issues when shutting down so I'm going to start requiring
> that any conversions like this be proven to work properly on real
> hardware.
> 
> thanks,
> 
> greg k-h

Hi, greg,

I make a compile test and have not test on a real hardware. I have read 
through the code logic but did not find a obvious problem.

Best Regards,
Zekun
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c b/drivers/usb/dwc3/dwc3-imx8mp.c
index 392fa1232788..a7e5ee797ae7 100644
--- a/drivers/usb/dwc3/dwc3-imx8mp.c
+++ b/drivers/usb/dwc3/dwc3-imx8mp.c
@@ -178,37 +178,20 @@  static int dwc3_imx8mp_probe(struct platform_device *pdev)
 			return PTR_ERR(dwc3_imx->glue_base);
 	}
 
-	dwc3_imx->hsio_clk = devm_clk_get(dev, "hsio");
-	if (IS_ERR(dwc3_imx->hsio_clk)) {
-		err = PTR_ERR(dwc3_imx->hsio_clk);
-		dev_err(dev, "Failed to get hsio clk, err=%d\n", err);
-		return err;
-	}
+	dwc3_imx->hsio_clk = devm_clk_get_enabled(dev, "hsio");
+	if (IS_ERR(dwc3_imx->hsio_clk))
+		return dev_err_probe(dev, PTR_ERR(dwc3_imx->hsio_clk),
+				     "Failed to get and enable hsio clk\n");
 
-	err = clk_prepare_enable(dwc3_imx->hsio_clk);
-	if (err) {
-		dev_err(dev, "Failed to enable hsio clk, err=%d\n", err);
-		return err;
-	}
-
-	dwc3_imx->suspend_clk = devm_clk_get(dev, "suspend");
-	if (IS_ERR(dwc3_imx->suspend_clk)) {
-		err = PTR_ERR(dwc3_imx->suspend_clk);
-		dev_err(dev, "Failed to get suspend clk, err=%d\n", err);
-		goto disable_hsio_clk;
-	}
-
-	err = clk_prepare_enable(dwc3_imx->suspend_clk);
-	if (err) {
-		dev_err(dev, "Failed to enable suspend clk, err=%d\n", err);
-		goto disable_hsio_clk;
-	}
+	dwc3_imx->suspend_clk = devm_clk_get_enabled(dev, "suspend");
+	if (IS_ERR(dwc3_imx->suspend_clk))
+		return dev_err_probe(dev, PTR_ERR(dwc3_imx->suspend_clk),
+				     "Failed to get and enable suspend clk\n");
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		err = irq;
-		goto disable_clks;
-	}
+	if (irq < 0)
+		return irq;
+
 	dwc3_imx->irq = irq;
 
 	imx8mp_configure_glue(dwc3_imx);
@@ -259,25 +242,17 @@  static int dwc3_imx8mp_probe(struct platform_device *pdev)
 disable_rpm:
 	pm_runtime_disable(dev);
 	pm_runtime_put_noidle(dev);
-disable_clks:
-	clk_disable_unprepare(dwc3_imx->suspend_clk);
-disable_hsio_clk:
-	clk_disable_unprepare(dwc3_imx->hsio_clk);
 
 	return err;
 }
 
 static void dwc3_imx8mp_remove(struct platform_device *pdev)
 {
-	struct dwc3_imx8mp *dwc3_imx = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
 
 	pm_runtime_get_sync(dev);
 	of_platform_depopulate(dev);
 
-	clk_disable_unprepare(dwc3_imx->suspend_clk);
-	clk_disable_unprepare(dwc3_imx->hsio_clk);
-
 	pm_runtime_disable(dev);
 	pm_runtime_put_noidle(dev);
 }