| Message ID | 744a6371f94fe96f527eea6e52a600914e6fb6b5.1702403904.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: Add a devm variant of clk_rate_exclusive_get() | expand |
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
[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
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
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.
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
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
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(+)