[PATCH 1/1] soundwire: use devm_clk_get_enabled() in qcom_swrm_probe()

[PATCH 1/1] soundwire: use devm_clk_get_enabled() in qcom_swrm_probe()

[Yuanjun Gong]

in qcom_swrm_probe(), the return value of function
clk_prepare_enable() should be checked, since it may fail.
using devm_clk_get_enabled() instead of devm_clk_get() and
clk_prepare_enable() can avoid this problem.

Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com>
---
 drivers/soundwire/qcom.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 7970fdb27ba0..04bc917b7a70 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -1549,14 +1549,12 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		goto err_init;
 	}
 
-	ctrl->hclk = devm_clk_get(dev, "iface");
+	ctrl->hclk = devm_clk_get_enabled(dev, "iface");
 	if (IS_ERR(ctrl->hclk)) {
 		ret = PTR_ERR(ctrl->hclk);
 		goto err_init;
 	}
 
-	clk_prepare_enable(ctrl->hclk);
-
 	ctrl->dev = dev;
 	dev_set_drvdata(&pdev->dev, ctrl);
 	mutex_init(&ctrl->port_lock);
@@ -1570,7 +1568,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
-		goto err_clk;
+		goto err_init;
 
 	params = &ctrl->bus.params;
 	params->max_dr_freq = DEFAULT_CLK_FREQ;
@@ -1598,7 +1596,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 					"soundwire", ctrl);
 	if (ret) {
 		dev_err(dev, "Failed to request soundwire irq\n");
-		goto err_clk;
+		goto err_init;
 	}
 
 	ctrl->wake_irq = of_irq_get(dev->of_node, 1);
@@ -1617,7 +1615,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "Failed to register Soundwire controller (%d)\n",
 			ret);
-		goto err_clk;
+		goto err_init;
 	}
 
 	qcom_swrm_init(ctrl);
@@ -1647,8 +1645,6 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 
 err_master_add:
 	sdw_bus_master_delete(&ctrl->bus);
-err_clk:
-	clk_disable_unprepare(ctrl->hclk);
 err_init:
 	return ret;
 }
-- 
2.17.1

Re: [PATCH 1/1] soundwire: use devm_clk_get_enabled() in qcom_swrm_probe()

[Bjorn Andersson]

On Thu, Jul 20, 2023 at 10:08:45PM +0800, Yuanjun Gong wrote:
> in qcom_swrm_probe(), the return value of function
> clk_prepare_enable() should be checked, since it may fail.
> using devm_clk_get_enabled() instead of devm_clk_get() and
> clk_prepare_enable() can avoid this problem.
> 
> Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com>
> ---
>  drivers/soundwire/qcom.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 7970fdb27ba0..04bc917b7a70 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -1549,14 +1549,12 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		goto err_init;
>  	}
>  
> -	ctrl->hclk = devm_clk_get(dev, "iface");
> +	ctrl->hclk = devm_clk_get_enabled(dev, "iface");

This gives the impression that hclk is to be enabled from probe() until
cleanup, but hclk is toggled during suspend/resume. So I'm not entirely
sure this is good practice.

If we choose to use devm_clk_get_enabled() then you need to remove the
clk_disable_unprepare() from qcom_swrm_remove().

If you choose to solve your problem by checking the return value of
clk_prepare_enable(), note that you have one in swrm_runtime_resume() as
well.

Regards,
Bjorn

[PATCH v2 1/1] Soundwire: fix the return value checks of clk_prepare_enable()

[Yuanjun Gong]

in qcom_swrm_probe() and swrm_runtime_resume(), the return value
of function clk_prepare_enable() should be checked, since it may
 fail.

Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com>
---
 drivers/soundwire/qcom.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 7970fdb27ba0..cf1e13a35023 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -1555,7 +1555,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		goto err_init;
 	}
 
-	clk_prepare_enable(ctrl->hclk);
+	ret = clk_prepare_enable(ctrl->hclk);
+	if (ret)
+		goto err_init;
 
 	ctrl->dev = dev;
 	dev_set_drvdata(&pdev->dev, ctrl);
@@ -1673,7 +1675,9 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
 			disable_irq_nosync(ctrl->wake_irq);
 	}
 
-	clk_prepare_enable(ctrl->hclk);
+	ret = clk_prepare_enable(ctrl->hclk);
+	if (ret)
+		return ret;
 
 	if (ctrl->clock_stop_not_supported) {
 		reinit_completion(&ctrl->enumeration);
-- 
2.17.1

Re: [PATCH v2 1/1] Soundwire: fix the return value checks of clk_prepare_enable()

[Bjorn Andersson]

On Sat, Jul 22, 2023 at 09:24:19PM +0800, Yuanjun Gong wrote:
> in qcom_swrm_probe() and swrm_runtime_resume(), the return value
> of function clk_prepare_enable() should be checked, since it may
>  fail.
> 
> Signed-off-by: Yuanjun Gong <ruc_gongyuanjun@163.com>
> ---
>  drivers/soundwire/qcom.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 7970fdb27ba0..cf1e13a35023 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -1555,7 +1555,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  		goto err_init;
>  	}
>  
> -	clk_prepare_enable(ctrl->hclk);
> +	ret = clk_prepare_enable(ctrl->hclk);
> +	if (ret)
> +		goto err_init;

FWIW, this is correct.

>  
>  	ctrl->dev = dev;
>  	dev_set_drvdata(&pdev->dev, ctrl);
> @@ -1673,7 +1675,9 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
>  			disable_irq_nosync(ctrl->wake_irq);
>  	}
>  
> -	clk_prepare_enable(ctrl->hclk);
> +	ret = clk_prepare_enable(ctrl->hclk);
> +	if (ret)
> +		return ret;

On the lines above this the wakeup IRQ is disabled, if you return here
without re-enabling that, this device will no longer be able to
wake the system up.

The significance of the ordering here (and the irq handler also
disabling of the irq) is a little bit unclear to me, so I'd like to hear
what Vinod and Srinivas has to say, before you send another version.

Regards,
Bjorn