diff mbox series

pwm: xilinx: Simplify using devm_ functions

Message ID 20240628063524.92907-2-u.kleine-koenig@baylibre.com
State Accepted
Headers show
Series pwm: xilinx: Simplify using devm_ functions | expand

Commit Message

Uwe Kleine-König June 28, 2024, 6:35 a.m. UTC
There are devm variants for clk_prepare_enable() and pwmchip_add(); and
clk_prepare_enable() can be done together with devm_clk_get(). This
allows to simplify the error paths in .probe() and drop .remove()
completely.

With the remove callback gone, the last user of platform_get_drvdata()
is gone and so the call to platform_set_drvdata() can be dropped, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-xilinx.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)


base-commit: 642a16ca7994a50d7de85715996a8ce171a5bdfb

Comments

Sean Anderson June 28, 2024, 9:04 p.m. UTC | #1
On 6/28/24 02:35, Uwe Kleine-König wrote:
> There are devm variants for clk_prepare_enable() and pwmchip_add(); and
> clk_prepare_enable() can be done together with devm_clk_get(). This
> allows to simplify the error paths in .probe() and drop .remove()
> completely.
>
> With the remove callback gone, the last user of platform_get_drvdata()
> is gone and so the call to platform_set_drvdata() can be dropped, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/pwm-xilinx.c | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
> index 3a7deebb0d0c..52c241982807 100644
> --- a/drivers/pwm/pwm-xilinx.c
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -224,7 +224,6 @@ static int xilinx_pwm_probe(struct platform_device *pdev)
>       if (IS_ERR(chip))
>               return PTR_ERR(chip);
>       priv = xilinx_pwm_chip_to_priv(chip);
> -     platform_set_drvdata(pdev, chip);
>
>       regs = devm_platform_ioremap_resource(pdev, 0);
>       if (IS_ERR(regs))
> @@ -263,37 +262,24 @@ static int xilinx_pwm_probe(struct platform_device *pdev)
>        * alas, such properties are not allowed to be used.
>        */
>
> -     priv->clk = devm_clk_get(dev, "s_axi_aclk");
> +     priv->clk = devm_clk_get_enabled(dev, "s_axi_aclk");
>       if (IS_ERR(priv->clk))
>               return dev_err_probe(dev, PTR_ERR(priv->clk),
>                                    "Could not get clock\n");
>
> -     ret = clk_prepare_enable(priv->clk);
> +     ret = devm_clk_rate_exclusive_get(dev, priv->clk);
>       if (ret)
> -             return dev_err_probe(dev, ret, "Clock enable failed\n");
> -     clk_rate_exclusive_get(priv->clk);
> +             return dev_err_probe(dev, ret,
> +                                  "Failed to lock clock rate\n");

Isn't this actually "failed to allocate memory"? clk_rate_exclusive_get can't fail.

>
>       chip->ops = &xilinx_pwm_ops;
> -     ret = pwmchip_add(chip);
> -     if (ret) {
> -             clk_rate_exclusive_put(priv->clk);
> -             clk_disable_unprepare(priv->clk);
> +     ret = devm_pwmchip_add(dev, chip);
> +     if (ret)
>               return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> -     }
>
>       return 0;
>  }
>
> -static void xilinx_pwm_remove(struct platform_device *pdev)
> -{
> -     struct pwm_chip *chip = platform_get_drvdata(pdev);
> -     struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> -
> -     pwmchip_remove(chip);
> -     clk_rate_exclusive_put(priv->clk);
> -     clk_disable_unprepare(priv->clk);
> -}
> -
>  static const struct of_device_id xilinx_pwm_of_match[] = {
>       { .compatible = "xlnx,xps-timer-1.00.a", },
>       {},
> @@ -302,7 +288,6 @@ MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match);
>
>  static struct platform_driver xilinx_pwm_driver = {
>       .probe = xilinx_pwm_probe,
> -     .remove_new = xilinx_pwm_remove,
>       .driver = {
>               .name = "xilinx-pwm",
>               .of_match_table = of_match_ptr(xilinx_pwm_of_match),
>
> base-commit: 642a16ca7994a50d7de85715996a8ce171a5bdfb

Anyway,

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

[StudioX, SECO SpA]<https://www.seco.com/applications/studiox>
Uwe Kleine-König June 29, 2024, 8:17 a.m. UTC | #2
Hello Sean,

On Fri, Jun 28, 2024 at 05:04:47PM -0400, Sean Anderson wrote:
> On 6/28/24 02:35, Uwe Kleine-König wrote:
> > -     ret = clk_prepare_enable(priv->clk);
> > +     ret = devm_clk_rate_exclusive_get(dev, priv->clk);
> >       if (ret)
> > -             return dev_err_probe(dev, ret, "Clock enable failed\n");
> > -     clk_rate_exclusive_get(priv->clk);
> > +             return dev_err_probe(dev, ret,
> > +                                  "Failed to lock clock rate\n");
> 
> Isn't this actually "failed to allocate memory"? clk_rate_exclusive_get can't fail.

There was a thread about clk_rate_exclusive_get() and its return code
some time ago[1]. I intend to pick up the discussion from there, this is
a preparation for it.

So yes, devm_clk_rate_exclusive_get() can only fail with -ENOMEM, but as
this isn't obvious for a casual reader, I like having the error handling
using dev_err_probe() in place. And since commit 2f3cfd2f4b7c ("driver
core: Make dev_err_probe() silent for -ENOMEM") this is also fine (at
least IMHO, I don't expect to have the agreement from everyone).

> Anyway,
> 
> Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Thanks!

Best regards
Uwe

[1] https://lore.kernel.org/all/cover.1702400947.git.u.kleine-koenig@pengutronix.de
Uwe Kleine-König July 5, 2024, 10:25 a.m. UTC | #3
Hello,

On Fri, Jun 28, 2024 at 08:35:23AM +0200, Uwe Kleine-König wrote:
> There are devm variants for clk_prepare_enable() and pwmchip_add(); and
> clk_prepare_enable() can be done together with devm_clk_get(). This
> allows to simplify the error paths in .probe() and drop .remove()
> completely.
> 
> With the remove callback gone, the last user of platform_get_drvdata()
> is gone and so the call to platform_set_drvdata() can be dropped, too.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Applied to pwm/for-next with Sean's R-b.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
index 3a7deebb0d0c..52c241982807 100644
--- a/drivers/pwm/pwm-xilinx.c
+++ b/drivers/pwm/pwm-xilinx.c
@@ -224,7 +224,6 @@  static int xilinx_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 	priv = xilinx_pwm_chip_to_priv(chip);
-	platform_set_drvdata(pdev, chip);
 
 	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
@@ -263,37 +262,24 @@  static int xilinx_pwm_probe(struct platform_device *pdev)
 	 * alas, such properties are not allowed to be used.
 	 */
 
-	priv->clk = devm_clk_get(dev, "s_axi_aclk");
+	priv->clk = devm_clk_get_enabled(dev, "s_axi_aclk");
 	if (IS_ERR(priv->clk))
 		return dev_err_probe(dev, PTR_ERR(priv->clk),
 				     "Could not get clock\n");
 
-	ret = clk_prepare_enable(priv->clk);
+	ret = devm_clk_rate_exclusive_get(dev, priv->clk);
 	if (ret)
-		return dev_err_probe(dev, ret, "Clock enable failed\n");
-	clk_rate_exclusive_get(priv->clk);
+		return dev_err_probe(dev, ret,
+				     "Failed to lock clock rate\n");
 
 	chip->ops = &xilinx_pwm_ops;
-	ret = pwmchip_add(chip);
-	if (ret) {
-		clk_rate_exclusive_put(priv->clk);
-		clk_disable_unprepare(priv->clk);
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret)
 		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
-	}
 
 	return 0;
 }
 
-static void xilinx_pwm_remove(struct platform_device *pdev)
-{
-	struct pwm_chip *chip = platform_get_drvdata(pdev);
-	struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
-
-	pwmchip_remove(chip);
-	clk_rate_exclusive_put(priv->clk);
-	clk_disable_unprepare(priv->clk);
-}
-
 static const struct of_device_id xilinx_pwm_of_match[] = {
 	{ .compatible = "xlnx,xps-timer-1.00.a", },
 	{},
@@ -302,7 +288,6 @@  MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match);
 
 static struct platform_driver xilinx_pwm_driver = {
 	.probe = xilinx_pwm_probe,
-	.remove_new = xilinx_pwm_remove,
 	.driver = {
 		.name = "xilinx-pwm",
 		.of_match_table = of_match_ptr(xilinx_pwm_of_match),