| Message ID | 20240628063524.92907-2-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: xilinx: Simplify using devm_ functions | expand |
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>
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
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 --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),
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