| Message ID | 20200519170440.294601-1-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: add api to get clk consummer from clk_hw | expand |
Quoting Jerome Brunet (2020-05-19 10:04:40) > clk_register() is deprecated. Using 'clk' member of struct clk_hw is > discouraged. With this constrainst, it is difficult for driver to s/constrainst/constraint/ > register clocks using the clk_hw API and then use the clock with > the consummer API s/consummer/consumer/ > > This add a simple helper, clk_hw_get_clk(), to get a struct clk from > a struct clk_hw. Like other clk_get() variant, each call to this helper > must be balanced with a call to clk_put(). > > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> I like it! > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 6fd23ce3cb03..d9946e192cbc 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3625,6 +3625,23 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > return clk; > } > > +/** > + * clk_hw_get_clk: get clk consummer given an clk_hw s/consummer/consumer/ > + * @hw: clk_hw associated with the clk being consumed > + * > + * Returns: new clk consummer > + * This is the function to be used by providers which need > + * to get a consummer clk and act on the clock element s/consummer/consumer/ > + * Calls to this function must be balanced with calls clk_put() calls to clk_put() > + */ > +struct clk *clk_hw_get_clk(struct clk_hw *hw) Can it also take a const char *id argument? That will let us "name" the clk structure for situations where we want to keep track of who is using the clk pointer for things. If that doesn't seem useful then I suppose we can pass a string like "clk_hw_get_clk" for con_id below and hope it doesn't become useful later. > +{ > + struct device *dev = hw->core->dev; > + > + return clk_hw_create_clk(dev, hw, dev_name(dev), NULL); > +} > +EXPORT_SYMBOL(clk_hw_get_clk); > + > static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) > { > const char *dst;
Hi Jerome, On Tue, May 19, 2020 at 7:09 PM Jerome Brunet <jbrunet@baylibre.com> wrote: [...] > + * Calls to this function must be balanced with calls clk_put() > + */ > +struct clk *clk_hw_get_clk(struct clk_hw *hw) I haven't looked at it myself yet, but would it be hard to have a devm_ variant of this function as well? a non-devm managed function would add boilerplate to the meson-mx-sdhc-mmc code also this may or may not simplify how to fetch the struct device pointer for this use-case. (that said, I only know about drivers for Amlogic related IP and there the devm_ variant can be used, but I don't know about other potential consumers of this new API) Thank you! Martin
On Wed 27 May 2020 at 11:17, Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Jerome Brunet (2020-05-19 10:04:40) >> clk_register() is deprecated. Using 'clk' member of struct clk_hw is >> discouraged. With this constrainst, it is difficult for driver to > > s/constrainst/constraint/ > >> register clocks using the clk_hw API and then use the clock with >> the consummer API > > s/consummer/consumer/ > >> >> This add a simple helper, clk_hw_get_clk(), to get a struct clk from >> a struct clk_hw. Like other clk_get() variant, each call to this helper >> must be balanced with a call to clk_put(). >> >> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > I like it! > >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 6fd23ce3cb03..d9946e192cbc 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -3625,6 +3625,23 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, >> return clk; >> } >> >> +/** >> + * clk_hw_get_clk: get clk consummer given an clk_hw > > s/consummer/consumer/ > >> + * @hw: clk_hw associated with the clk being consumed >> + * >> + * Returns: new clk consummer >> + * This is the function to be used by providers which need >> + * to get a consummer clk and act on the clock element > > s/consummer/consumer/ > >> + * Calls to this function must be balanced with calls clk_put() > > calls to clk_put() > >> + */ >> +struct clk *clk_hw_get_clk(struct clk_hw *hw) Sorry about all the typos ... > > Can it also take a const char *id argument? That will let us "name" the > clk structure for situations where we want to keep track of who is using > the clk pointer for things. If that doesn't seem useful then I suppose > we can pass a string like "clk_hw_get_clk" for con_id below and hope it > doesn't become useful later. Sure I can add the argument. no worries > >> +{ >> + struct device *dev = hw->core->dev; >> + >> + return clk_hw_create_clk(dev, hw, dev_name(dev), NULL); >> +} >> +EXPORT_SYMBOL(clk_hw_get_clk); >> + >> static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) >> { >> const char *dst;
On Wed 27 May 2020 at 22:07, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Jerome, > > On Tue, May 19, 2020 at 7:09 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > [...] >> + * Calls to this function must be balanced with calls clk_put() >> + */ >> +struct clk *clk_hw_get_clk(struct clk_hw *hw) > I haven't looked at it myself yet, but would it be hard to have a > devm_ variant of this function as well? Seems easy enough. Stephen is this OK with you ? I'm just wondering if this devm_ function should use the device pointer embedded in the clk_hw structure or have it as an argument ? The 1st option seems simpler but I'm not sure it is correct. Thoughts ? > a non-devm managed function would add boilerplate to the meson-mx-sdhc-mmc code > > also this may or may not simplify how to fetch the struct device > pointer for this use-case. > (that said, I only know about drivers for Amlogic related IP and there > the devm_ variant can be used, but I don't know about other potential > consumers of this new API) > > > Thank you! > Martin
Quoting Jerome Brunet (2020-05-28 11:58:45) > > On Wed 27 May 2020 at 22:07, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > > Hi Jerome, > > > > On Tue, May 19, 2020 at 7:09 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > [...] > >> + * Calls to this function must be balanced with calls clk_put() > >> + */ > >> +struct clk *clk_hw_get_clk(struct clk_hw *hw) > > I haven't looked at it myself yet, but would it be hard to have a > > devm_ variant of this function as well? > > Seems easy enough. > Stephen is this OK with you ? > > I'm just wondering if this devm_ function should use the device pointer > embedded in the clk_hw structure or have it as an argument ? > > The 1st option seems simpler but I'm not sure it is correct. > > Thoughts ? > devm API sounds OK to me. For now we can use the one embedded in the clk_hw structure and if we have to we can replace it with the one that the caller passes in. Hopefully we never need to do that because then it means we have drivers passing around clk_hw pointers instead of having the caller use proper clk_get() style APIs.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 6fd23ce3cb03..d9946e192cbc 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3625,6 +3625,23 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, return clk; } +/** + * clk_hw_get_clk: get clk consummer given an clk_hw + * @hw: clk_hw associated with the clk being consumed + * + * Returns: new clk consummer + * This is the function to be used by providers which need + * to get a consummer clk and act on the clock element + * Calls to this function must be balanced with calls clk_put() + */ +struct clk *clk_hw_get_clk(struct clk_hw *hw) +{ + struct device *dev = hw->core->dev; + + return clk_hw_create_clk(dev, hw, dev_name(dev), NULL); +} +EXPORT_SYMBOL(clk_hw_get_clk); + static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) { const char *dst; diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index bd1ee9039558..a8f466bdda1a 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -1088,6 +1088,7 @@ static inline struct clk_hw *__clk_get_hw(struct clk *clk) return (struct clk_hw *)clk; } #endif +struct clk *clk_hw_get_clk(struct clk_hw *hw); unsigned int clk_hw_get_num_parents(const struct clk_hw *hw); struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw); struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,
clk_register() is deprecated. Using 'clk' member of struct clk_hw is discouraged. With this constrainst, it is difficult for driver to register clocks using the clk_hw API and then use the clock with the consummer API This add a simple helper, clk_hw_get_clk(), to get a struct clk from a struct clk_hw. Like other clk_get() variant, each call to this helper must be balanced with a call to clk_put(). Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/clk/clk.c | 17 +++++++++++++++++ include/linux/clk-provider.h | 1 + 2 files changed, 18 insertions(+)