diff mbox series

[4/9] crypto: qce - shrink code with devres clk helpers

Message ID 20241203-crypto-qce-refactor-v1-4-c5901d2dd45c@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series crypto: qce - refactor the driver | expand

Commit Message

Bartosz Golaszewski Dec. 3, 2024, 9:19 a.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
separately as well as putting the clocks in error path and the remove()
callback.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/crypto/qce/core.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

Comments

Neil Armstrong Dec. 3, 2024, 1:46 p.m. UTC | #1 
On 03/12/2024 10:19, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
> separately as well as putting the clocks in error path and the remove()
> callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/crypto/qce/core.c | 29 ++++-------------------------
>   1 file changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index f9ff1dfc1defe..cdcddf8f9f02b 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -212,15 +212,15 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> -	qce->core = devm_clk_get_optional(qce->dev, "core");
> +	qce->core = devm_clk_get_optional_enabled(qce->dev, "core");
>   	if (IS_ERR(qce->core))
>   		return PTR_ERR(qce->core);
>   
> -	qce->iface = devm_clk_get_optional(qce->dev, "iface");
> +	qce->iface = devm_clk_get_optional_enabled(qce->dev, "iface");
>   	if (IS_ERR(qce->iface))
>   		return PTR_ERR(qce->iface);
>   
> -	qce->bus = devm_clk_get_optional(qce->dev, "bus");
> +	qce->bus = devm_clk_get_optional_enabled(qce->dev, "bus");
>   	if (IS_ERR(qce->bus))
>   		return PTR_ERR(qce->bus);
>   
> @@ -232,21 +232,9 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	ret = clk_prepare_enable(qce->core);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(qce->iface);
> -	if (ret)
> -		goto err_clks_core;
> -
> -	ret = clk_prepare_enable(qce->bus);
> -	if (ret)
> -		goto err_clks_iface;
> -
>   	ret = qce_dma_request(qce->dev, &qce->dma);
>   	if (ret)
> -		goto err_clks;
> +		return ret;
>   
>   	ret = qce_check_version(qce);
>   	if (ret)
> @@ -268,12 +256,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
>   
>   err_dma:
>   	qce_dma_release(&qce->dma);
> -err_clks:
> -	clk_disable_unprepare(qce->bus);
> -err_clks_iface:
> -	clk_disable_unprepare(qce->iface);
> -err_clks_core:
> -	clk_disable_unprepare(qce->core);
>   
>   	return ret;
>   }
> @@ -285,9 +267,6 @@ static void qce_crypto_remove(struct platform_device *pdev)
>   	tasklet_kill(&qce->done_tasklet);
>   	qce_unregister_algs(qce);
>   	qce_dma_release(&qce->dma);
> -	clk_disable_unprepare(qce->bus);
> -	clk_disable_unprepare(qce->iface);
> -	clk_disable_unprepare(qce->core);
>   }
>   
>   static const struct of_device_id qce_crypto_of_match[] = {
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Stephan Gerhold Dec. 3, 2024, 6:32 p.m. UTC | #2 
On Tue, Dec 03, 2024 at 10:19:32AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
> separately as well as putting the clocks in error path and the remove()
> callback.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

FWIW: Ideally, the driver shouldn't keep on the clock all the time in
the first place, since this will prevent reaching deeper low power
states. So while this cleanup is nice, I think it will have to be
reverted again once someone fixes that.

If you're working on refactoring this rarely cared for driver, is there
any chance you could add some form of runtime PM while you're at it? :-)

Thanks,
Stephan
Bartosz Golaszewski Dec. 3, 2024, 8:39 p.m. UTC | #3 
On Tue, Dec 3, 2024 at 7:32 PM Stephan Gerhold
<stephan.gerhold@linaro.org> wrote:
>
> On Tue, Dec 03, 2024 at 10:19:32AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use devm_clk_get_optional_enabled() to avoid having to enable the clocks
> > separately as well as putting the clocks in error path and the remove()
> > callback.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> FWIW: Ideally, the driver shouldn't keep on the clock all the time in
> the first place, since this will prevent reaching deeper low power
> states. So while this cleanup is nice, I think it will have to be
> reverted again once someone fixes that.
>
> If you're working on refactoring this rarely cared for driver, is there
> any chance you could add some form of runtime PM while you're at it? :-)
>

Sure, I will most likely be doing more work on this in the future, I
can think about it. This patch was about code shrink, not functional
changes so I prefer to keep it as is.

Bart
diff mbox series

Patch

diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
index f9ff1dfc1defe..cdcddf8f9f02b 100644
--- a/drivers/crypto/qce/core.c
+++ b/drivers/crypto/qce/core.c
@@ -212,15 +212,15 @@  static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	qce->core = devm_clk_get_optional(qce->dev, "core");
+	qce->core = devm_clk_get_optional_enabled(qce->dev, "core");
 	if (IS_ERR(qce->core))
 		return PTR_ERR(qce->core);
 
-	qce->iface = devm_clk_get_optional(qce->dev, "iface");
+	qce->iface = devm_clk_get_optional_enabled(qce->dev, "iface");
 	if (IS_ERR(qce->iface))
 		return PTR_ERR(qce->iface);
 
-	qce->bus = devm_clk_get_optional(qce->dev, "bus");
+	qce->bus = devm_clk_get_optional_enabled(qce->dev, "bus");
 	if (IS_ERR(qce->bus))
 		return PTR_ERR(qce->bus);
 
@@ -232,21 +232,9 @@  static int qce_crypto_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(qce->core);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(qce->iface);
-	if (ret)
-		goto err_clks_core;
-
-	ret = clk_prepare_enable(qce->bus);
-	if (ret)
-		goto err_clks_iface;
-
 	ret = qce_dma_request(qce->dev, &qce->dma);
 	if (ret)
-		goto err_clks;
+		return ret;
 
 	ret = qce_check_version(qce);
 	if (ret)
@@ -268,12 +256,6 @@  static int qce_crypto_probe(struct platform_device *pdev)
 
 err_dma:
 	qce_dma_release(&qce->dma);
-err_clks:
-	clk_disable_unprepare(qce->bus);
-err_clks_iface:
-	clk_disable_unprepare(qce->iface);
-err_clks_core:
-	clk_disable_unprepare(qce->core);
 
 	return ret;
 }
@@ -285,9 +267,6 @@  static void qce_crypto_remove(struct platform_device *pdev)
 	tasklet_kill(&qce->done_tasklet);
 	qce_unregister_algs(qce);
 	qce_dma_release(&qce->dma);
-	clk_disable_unprepare(qce->bus);
-	clk_disable_unprepare(qce->iface);
-	clk_disable_unprepare(qce->core);
 }
 
 static const struct of_device_id qce_crypto_of_match[] = {