[PATCH 0/2] clk: Add a devm variant of clk_rate_exclusive_get()

[PATCH 0/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Uwe Kleine-König]

Hello,

patch #1 adds a new function devm_clk_rate_exclusive_get() that
simplifies usage of clk_rate_exclusive_get() in probe functions.

One downside is that a caller of clk_rate_exclusive_get() doesn't need
to check the return value, devm_clk_rate_exclusive_get() can fail
however. So to benefit from the new function you usually need to add a
check for devm_clk_rate_exclusive_get() failing in return to simplifying
error paths in .probe() and .remove().

Patch #2 shows an example conversion. Note that without
devm_clk_rate_exclusive_get() the pwm-xilinx driver cannot benefit from
devm_pwmchip_add().

This series bases on
https://lore.kernel.org/linux-clk/cover.1702400947.git.u.kleine-koenig@pengutronix.de/T/#t
. That's not a "hard" dependency, it can easily be rebased to next.
There is only a trivial context conflict in include/linux/clk.h.

Regarding how to merge this: I suggest to take patch #1 via clk. When
this is merged I will take care for the pwm-xilinx driver (and others
that might benefit).

Best regards
Uwe

Uwe Kleine-König (2):
  clk: Add a devm variant of clk_rate_exclusive_get()
  pwm: xilinx: Simplify using devm functions

 drivers/clk/clk.c        | 15 +++++++++++++++
 drivers/pwm/pwm-xilinx.c | 25 ++++++-------------------
 include/linux/clk.h      | 12 ++++++++++++
 3 files changed, 33 insertions(+), 19 deletions(-)


base-commit: bbd220ce4e29ed55ab079007cff0b550895258eb
prerequisite-patch-id: 5a986d744a054000998ce06e3dbaaedede71c8ac
prerequisite-patch-id: cdb26f72d1fbd7fef4a9ef4476e0218dd3fcba22
prerequisite-patch-id: 838e3cb639cf13c7b571d77c1dea3d4ec479465d
prerequisite-patch-id: d5059c23f752a4861e5bbfe70b8ab771727bf68d
prerequisite-patch-id: c7ca9e00334dabe41f39e006ceb2b7d1afddcaa5
-- 
2.42.0

[PATCH 1/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Uwe Kleine-König]

This allows to simplify drivers that use clk_rate_exclusive_get()
in their probe routine as calling clk_rate_exclusive_put() is cared for
automatically.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk.c   | 15 +++++++++++++++
 include/linux/clk.h | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af2011c2a93b..78249ca2341c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -937,6 +937,21 @@ void clk_rate_exclusive_get(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
 
+static void devm_clk_rate_exclusive_put(void *data)
+{
+	struct clk *clk = data;
+
+	clk_rate_exclusive_put(clk);
+}
+
+int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk)
+{
+	clk_rate_exclusive_get(clk);
+
+	return devm_add_action_or_reset(dev, devm_clk_rate_exclusive_put, clk);
+}
+EXPORT_SYMBOL_GPL(devm_clk_rate_exclusive_get);
+
 static void clk_core_unprepare(struct clk_core *core)
 {
 	lockdep_assert_held(&prepare_lock);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index f88c407925f8..5a749459c3a3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -199,6 +199,18 @@ bool clk_is_match(const struct clk *p, const struct clk *q);
  */
 void clk_rate_exclusive_get(struct clk *clk);
 
+/**
+ * devm_clk_rate_exclusive_get - devm variant of clk_rate_exclusive_get
+ * @dev: device the exclusivity is bound to
+ * @clk: clock source
+ *
+ * Calls clk_rate_exclusive_get() on @clk and registers a devm cleanup handler
+ * on @dev to cal clk_rate_exclusive_put().
+ *
+ * Must not be called from within atomic context.
+ */
+int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk);
+
 /**
  * clk_rate_exclusive_put - release exclusivity over the rate control of a
  *                          producer
-- 
2.42.0

Re: [PATCH 1/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Stephen Boyd]

Quoting Uwe Kleine-König (2023-12-12 10:09:42)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index af2011c2a93b..78249ca2341c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -937,6 +937,21 @@ void clk_rate_exclusive_get(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
>  
> +static void devm_clk_rate_exclusive_put(void *data)
> +{
> +       struct clk *clk = data;
> +
> +       clk_rate_exclusive_put(clk);
> +}
> +
> +int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk)
> +{
> +       clk_rate_exclusive_get(clk);

It seems the other thread wants this to return an error value.

> +
> +       return devm_add_action_or_reset(dev, devm_clk_rate_exclusive_put, clk);
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_rate_exclusive_get);
> +
>  static void clk_core_unprepare(struct clk_core *core)
>  {
>         lockdep_assert_held(&prepare_lock);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index f88c407925f8..5a749459c3a3 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -199,6 +199,18 @@ bool clk_is_match(const struct clk *p, const struct clk *q);
>   */
>  void clk_rate_exclusive_get(struct clk *clk);
>  
> +/**
> + * devm_clk_rate_exclusive_get - devm variant of clk_rate_exclusive_get
> + * @dev: device the exclusivity is bound to
> + * @clk: clock source
> + *
> + * Calls clk_rate_exclusive_get() on @clk and registers a devm cleanup handler
> + * on @dev to cal clk_rate_exclusive_put().

call

> + *
> + * Must not be called from within atomic context.
> + */
> +int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk);
> +
>  /**
>   * clk_rate_exclusive_put - release exclusivity over the rate control of a
>   *                          producer

Re: [PATCH 1/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

[Cc += Maxime]

Hello Stephen,

On Sun, Dec 17, 2023 at 04:17:41PM -0800, Stephen Boyd wrote:
> Quoting Uwe Kleine-König (2023-12-12 10:09:42)
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index af2011c2a93b..78249ca2341c 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -937,6 +937,21 @@ void clk_rate_exclusive_get(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
> >  
> > +static void devm_clk_rate_exclusive_put(void *data)
> > +{
> > +       struct clk *clk = data;
> > +
> > +       clk_rate_exclusive_put(clk);
> > +}
> > +
> > +int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk)
> > +{
> > +       clk_rate_exclusive_get(clk);
> 
> It seems the other thread wants this to return an error value.

The status quo is that clk_rate_exclusive_get() always returns zero.
Some users do error handling (which is dead code until Maxime reworks
the call that it might return something non-zero), others just call it
without checking.

If you don't require to add something like:

	ret = clk_rate_exclusive_get(clk);
	if (ret)
		return ret;

where we currently have just

	clk_rate_exclusive_get(clk);

the patch can just be applied (using git am -3) not even hitting a merge
conflict without that other series.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

Hello Stephen,

On Mon, Dec 18, 2023 at 02:01:41PM +0100, Uwe Kleine-König wrote:
> [Cc += Maxime]
> 
> Hello Stephen,
> 
> On Sun, Dec 17, 2023 at 04:17:41PM -0800, Stephen Boyd wrote:
> > Quoting Uwe Kleine-König (2023-12-12 10:09:42)
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af2011c2a93b..78249ca2341c 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -937,6 +937,21 @@ void clk_rate_exclusive_get(struct clk *clk)
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
> > >  
> > > +static void devm_clk_rate_exclusive_put(void *data)
> > > +{
> > > +       struct clk *clk = data;
> > > +
> > > +       clk_rate_exclusive_put(clk);
> > > +}
> > > +
> > > +int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk)
> > > +{
> > > +       clk_rate_exclusive_get(clk);
> > 
> > It seems the other thread wants this to return an error value.
> 
> The status quo is that clk_rate_exclusive_get() always returns zero.
> Some users do error handling (which is dead code until Maxime reworks
> the call that it might return something non-zero), others just call it
> without checking.
> 
> If you don't require to add something like:
> 
> 	ret = clk_rate_exclusive_get(clk);
> 	if (ret)
> 		return ret;
> 
> where we currently have just
> 
> 	clk_rate_exclusive_get(clk);
> 
> the patch can just be applied (using git am -3) not even hitting a merge
> conflict without that other series.

I wonder what you think about this. This devm_clk_rate_exclusive_get()
would be very useful and simplify a few more drivers.

Do you intend to take the patch as is, or should I rework it to check
for the zero it returns?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Stephen Boyd]

Quoting Uwe Kleine-König (2024-01-04 10:06:29)
> Hello Stephen,
> 
> On Mon, Dec 18, 2023 at 02:01:41PM +0100, Uwe Kleine-K�nig wrote:
> > [Cc += Maxime]
> > 
> > Hello Stephen,
> > 
> > On Sun, Dec 17, 2023 at 04:17:41PM -0800, Stephen Boyd wrote:
> > > Quoting Uwe Kleine-K�nig (2023-12-12 10:09:42)
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index af2011c2a93b..78249ca2341c 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -937,6 +937,21 @@ void clk_rate_exclusive_get(struct clk *clk)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(clk_rate_exclusive_get);
> > > >  
> > > > +static void devm_clk_rate_exclusive_put(void *data)
> > > > +{
> > > > +       struct clk *clk = data;
> > > > +
> > > > +       clk_rate_exclusive_put(clk);
> > > > +}
> > > > +
> > > > +int devm_clk_rate_exclusive_get(struct device *dev, struct clk *clk)
> > > > +{
> > > > +       clk_rate_exclusive_get(clk);
> > > 
> > > It seems the other thread wants this to return an error value.
> > 
> > The status quo is that clk_rate_exclusive_get() always returns zero.
> > Some users do error handling (which is dead code until Maxime reworks
> > the call that it might return something non-zero), others just call it
> > without checking.
> > 
> > If you don't require to add something like:
> > 
> >       ret = clk_rate_exclusive_get(clk);
> >       if (ret)
> >               return ret;
> > 
> > where we currently have just
> > 
> >       clk_rate_exclusive_get(clk);
> > 
> > the patch can just be applied (using git am -3) not even hitting a merge
> > conflict without that other series.
> 
> I wonder what you think about this. This devm_clk_rate_exclusive_get()
> would be very useful and simplify a few more drivers.
> 
> Do you intend to take the patch as is, or should I rework it to check
> for the zero it returns?
> 

Please check the return value even if it is always zero. The discussion
about handling the return value can continue in parallel.

Re: [PATCH 1/2] clk: Add a devm variant of clk_rate_exclusive_get()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Hello Stephen,

On Thu, Jan 04, 2024 at 01:38:27PM -0800, Stephen Boyd wrote:
> Quoting Uwe Kleine-König (2024-01-04 10:06:29)
> > On Mon, Dec 18, 2023 at 02:01:41PM +0100, Uwe Kleine-K�nig wrote:
> > > If you don't require to add something like:
> > > 
> > >       ret = clk_rate_exclusive_get(clk);
> > >       if (ret)
> > >               return ret;
> > > 
> > > where we currently have just
> > > 
> > >       clk_rate_exclusive_get(clk);
> > > 
> > > the patch can just be applied (using git am -3) not even hitting a merge
> > > conflict without that other series.
> > 
> > I wonder what you think about this. This devm_clk_rate_exclusive_get()
> > would be very useful and simplify a few more drivers.
> > 
> > Do you intend to take the patch as is, or should I rework it to check
> > for the zero it returns?
> 
> Please check the return value even if it is always zero. The discussion
> about handling the return value can continue in parallel.

The discussion in the other thread died, but maybe that's because of the
holidays. Anyhow, I sent a v2 that checks the return value and intend to
rebase and resend my series making clk_rate_exclusive_get() return void
if there is no further contribution by Maxime in a few months.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] pwm: xilinx: Simplify using devm functions

[Uwe Kleine-König]

devm_clk_get() + clk_prepare_enable() can be simplified to
devm_clk_get_enabled(). Both clk_rate_exclusive_get() and pwmchip_add()
have devm variants.

This allows to completely drop the remove callback and the error path in
the probe function.

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

diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
index 5f3c2a6fed11..19a2a496d555 100644
--- a/drivers/pwm/pwm-xilinx.c
+++ b/drivers/pwm/pwm-xilinx.c
@@ -268,38 +268,26 @@ 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,
+				     "Could not get exclusive control over clock\n");
 
 	xilinx_pwm->chip.dev = dev;
 	xilinx_pwm->chip.ops = &xilinx_pwm_ops;
 	xilinx_pwm->chip.npwm = 1;
-	ret = pwmchip_add(&xilinx_pwm->chip);
-	if (ret) {
-		clk_rate_exclusive_put(priv->clk);
-		clk_disable_unprepare(priv->clk);
+	ret = devm_pwmchip_add(dev, &xilinx_pwm->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 xilinx_pwm_device *xilinx_pwm = platform_get_drvdata(pdev);
-
-	pwmchip_remove(&xilinx_pwm->chip);
-	clk_rate_exclusive_put(xilinx_pwm->priv.clk);
-	clk_disable_unprepare(xilinx_pwm->priv.clk);
-}
-
 static const struct of_device_id xilinx_pwm_of_match[] = {
 	{ .compatible = "xlnx,xps-timer-1.00.a", },
 	{},
@@ -308,7 +296,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),
-- 
2.42.0