[PATCH 0/2] Use helper function devm_clk_get_enabled()

[PATCH 0/2] Use helper function devm_clk_get_enabled()

[Zhang Zekun]

devm_clk_get() and clk_prepare_enable() can be replaced by helper function
devm_clk_get_enabled(). Use helper function devm_clk_get_enabled() to
simplify code.

Zhang Zekun (2):
  usb: dwc3: Use helper function devm_clk_get_enabled()
  usb: ohci-nxp: Use helper function devm_clk_get_enabled()

 drivers/usb/dwc3/dwc3-imx8mp.c | 47 ++++++++--------------------------
 drivers/usb/host/ohci-nxp.c    | 18 +++----------
 2 files changed, 15 insertions(+), 50 deletions(-)

-- 
2.17.1

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

[Zhang Zekun]

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

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

[Thinh Nguyen]

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

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

[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

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

[zhangzekun (A)]

在 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

[PATCH 2/2] usb: ohci-nxp: Use helper function devm_clk_get_enabled()

[Zhang Zekun]

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/host/ohci-nxp.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
index 8264c454f6bd..5b775e1ea527 100644
--- a/drivers/usb/host/ohci-nxp.c
+++ b/drivers/usb/host/ohci-nxp.c
@@ -51,8 +51,6 @@ static struct hc_driver __read_mostly ohci_nxp_hc_driver;
 
 static struct i2c_client *isp1301_i2c_client;
 
-static struct clk *usb_host_clk;
-
 static void isp1301_configure_lpc32xx(void)
 {
 	/* LPC32XX only supports DAT_SE0 USB mode */
@@ -155,6 +153,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret = 0, irq;
 	struct device_node *isp1301_node;
+	struct clk *usb_host_clk;
 
 	if (pdev->dev.of_node) {
 		isp1301_node = of_parse_phandle(pdev->dev.of_node,
@@ -180,26 +179,20 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	}
 
 	/* Enable USB host clock */
-	usb_host_clk = devm_clk_get(&pdev->dev, NULL);
+	usb_host_clk = devm_clk_get_enabled(&pdev->dev, NULL);
 	if (IS_ERR(usb_host_clk)) {
-		dev_err(&pdev->dev, "failed to acquire USB OHCI clock\n");
+		dev_err(&pdev->dev, "failed to acquire and start USB OHCI clock\n");
 		ret = PTR_ERR(usb_host_clk);
 		goto fail_disable;
 	}
 
-	ret = clk_prepare_enable(usb_host_clk);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to start USB OHCI clock\n");
-		goto fail_disable;
-	}
-
 	isp1301_configure();
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd) {
 		dev_err(&pdev->dev, "Failed to allocate HC buffer\n");
 		ret = -ENOMEM;
-		goto fail_hcd;
+		goto fail_disable;
 	}
 
 	hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
@@ -229,8 +222,6 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	ohci_nxp_stop_hc();
 fail_resource:
 	usb_put_hcd(hcd);
-fail_hcd:
-	clk_disable_unprepare(usb_host_clk);
 fail_disable:
 	isp1301_i2c_client = NULL;
 	return ret;
@@ -243,7 +234,6 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
 	usb_remove_hcd(hcd);
 	ohci_nxp_stop_hc();
 	usb_put_hcd(hcd);
-	clk_disable_unprepare(usb_host_clk);
 	isp1301_i2c_client = NULL;
 }
 
-- 
2.17.1

Re: [PATCH 2/2] usb: ohci-nxp: Use helper function devm_clk_get_enabled()

[Alan Stern]

On Mon, Sep 02, 2024 at 08:30:20PM +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>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/host/ohci-nxp.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-nxp.c b/drivers/usb/host/ohci-nxp.c
> index 8264c454f6bd..5b775e1ea527 100644
> --- a/drivers/usb/host/ohci-nxp.c
> +++ b/drivers/usb/host/ohci-nxp.c
> @@ -51,8 +51,6 @@ static struct hc_driver __read_mostly ohci_nxp_hc_driver;
>  
>  static struct i2c_client *isp1301_i2c_client;
>  
> -static struct clk *usb_host_clk;
> -
>  static void isp1301_configure_lpc32xx(void)
>  {
>  	/* LPC32XX only supports DAT_SE0 USB mode */
> @@ -155,6 +153,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	int ret = 0, irq;
>  	struct device_node *isp1301_node;
> +	struct clk *usb_host_clk;
>  
>  	if (pdev->dev.of_node) {
>  		isp1301_node = of_parse_phandle(pdev->dev.of_node,
> @@ -180,26 +179,20 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Enable USB host clock */
> -	usb_host_clk = devm_clk_get(&pdev->dev, NULL);
> +	usb_host_clk = devm_clk_get_enabled(&pdev->dev, NULL);
>  	if (IS_ERR(usb_host_clk)) {
> -		dev_err(&pdev->dev, "failed to acquire USB OHCI clock\n");
> +		dev_err(&pdev->dev, "failed to acquire and start USB OHCI clock\n");
>  		ret = PTR_ERR(usb_host_clk);
>  		goto fail_disable;
>  	}
>  
> -	ret = clk_prepare_enable(usb_host_clk);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "failed to start USB OHCI clock\n");
> -		goto fail_disable;
> -	}
> -
>  	isp1301_configure();
>  
>  	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>  	if (!hcd) {
>  		dev_err(&pdev->dev, "Failed to allocate HC buffer\n");
>  		ret = -ENOMEM;
> -		goto fail_hcd;
> +		goto fail_disable;
>  	}
>  
>  	hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> @@ -229,8 +222,6 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
>  	ohci_nxp_stop_hc();
>  fail_resource:
>  	usb_put_hcd(hcd);
> -fail_hcd:
> -	clk_disable_unprepare(usb_host_clk);
>  fail_disable:
>  	isp1301_i2c_client = NULL;
>  	return ret;
> @@ -243,7 +234,6 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
>  	usb_remove_hcd(hcd);
>  	ohci_nxp_stop_hc();
>  	usb_put_hcd(hcd);
> -	clk_disable_unprepare(usb_host_clk);
>  	isp1301_i2c_client = NULL;
>  }
>  
> -- 
> 2.17.1
>