| Message ID | 20170129180743.GA10917@dtor-ws (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 01/29/2017 10:07 AM, Dmitry Torokhov wrote: > When converting a driver to managed resources it is desirable to be able to > manage all resources in the same fashion. This change allows managing > clocks in the same way we manage many other resources. > > This adds the following managed APIs: > > - devm_clk_prepare()/devm_clk_unprepare(); > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Not really my area of expertise, but LGTM. Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > > v2: dropped devm_clk_enable() and devm_clk_disable() > > > drivers/clk/clk-devres.c | 98 +++++++++++++++++++++++++++++++--------------- > include/linux/clk.h | 68 ++++++++++++++++++++++++++++++++ > 2 files changed, 134 insertions(+), 32 deletions(-) > > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c > index 3a218c3a06ae..2ff94ffe11d3 100644 > --- a/drivers/clk/clk-devres.c > +++ b/drivers/clk/clk-devres.c > @@ -9,30 +9,20 @@ > #include <linux/export.h> > #include <linux/gfp.h> > > -static void devm_clk_release(struct device *dev, void *res) > +static int devm_clk_create_devres(struct device *dev, struct clk *clk, > + void (*release)(struct device *, void *)) > { > - clk_put(*(struct clk **)res); > -} > + struct clk **ptr; > > -struct clk *devm_clk_get(struct device *dev, const char *id) > -{ > - struct clk **ptr, *clk; > - > - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); > + ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL); > if (!ptr) > - return ERR_PTR(-ENOMEM); > + return -ENOMEM; > > - clk = clk_get(dev, id); > - if (!IS_ERR(clk)) { > - *ptr = clk; > - devres_add(dev, ptr); > - } else { > - devres_free(ptr); > - } > + *ptr = clk; > + devres_add(dev, ptr); > > - return clk; > + return 0; > } > -EXPORT_SYMBOL(devm_clk_get); > > static int devm_clk_match(struct device *dev, void *res, void *data) > { > @@ -44,31 +34,75 @@ static int devm_clk_match(struct device *dev, void *res, void *data) > return *c == data; > } > > -void devm_clk_put(struct device *dev, struct clk *clk) > +#define DEFINE_DEVM_CLK_DESTROY_OP(destroy_op) \ > +static void devm_##destroy_op##_release(struct device *dev, void *res) \ > +{ \ > + destroy_op(*(struct clk **)res); \ > +} \ > + \ > +void devm_##destroy_op(struct device *dev, struct clk *clk) \ > +{ \ > + WARN_ON(devres_release(dev, devm_##destroy_op##_release, \ > + devm_clk_match, clk)); \ > +} \ > +EXPORT_SYMBOL(devm_##destroy_op) > + > +#define DEFINE_DEVM_CLK_OP(create_op, destroy_op) \ > +DEFINE_DEVM_CLK_DESTROY_OP(destroy_op); \ > +int devm_##create_op(struct device *dev, struct clk *clk) \ > +{ \ > + int error; \ > + \ > + error = create_op(clk); \ > + if (error) \ > + return error; \ > + \ > + error = devm_clk_create_devres(dev, clk, \ > + devm_##destroy_op##_release); \ > + if (error) { \ > + destroy_op(clk); \ > + return error; \ > + } \ > + \ > + return 0; \ > +} \ > +EXPORT_SYMBOL(devm_##create_op) > + > +DEFINE_DEVM_CLK_DESTROY_OP(clk_put); > +DEFINE_DEVM_CLK_OP(clk_prepare, clk_unprepare); > +DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare); > + > +struct clk *devm_clk_get(struct device *dev, const char *id) > { > - int ret; > + struct clk *clk; > + int error; > > - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk); > + clk = clk_get(dev, id); > + if (!IS_ERR(clk)) { > + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); > + if (error) { > + clk_put(clk); > + return ERR_PTR(error); > + } > + } > > - WARN_ON(ret); > + return clk; > } > -EXPORT_SYMBOL(devm_clk_put); > +EXPORT_SYMBOL(devm_clk_get); > > struct clk *devm_get_clk_from_child(struct device *dev, > struct device_node *np, const char *con_id) > { > - struct clk **ptr, *clk; > - > - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); > - if (!ptr) > - return ERR_PTR(-ENOMEM); > + struct clk *clk; > + int error; > > clk = of_clk_get_by_name(np, con_id); > if (!IS_ERR(clk)) { > - *ptr = clk; > - devres_add(dev, ptr); > - } else { > - devres_free(ptr); > + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); > + if (error) { > + clk_put(clk); > + return ERR_PTR(error); > + } > } > > return clk; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index e9d36b3e49de..413dc8f636bd 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -267,6 +267,29 @@ struct clk *devm_get_clk_from_child(struct device *dev, > struct device_node *np, const char *con_id); > > /** > + * devm_clk_prepare - prepare clock source as a managed resource > + * @dev: device owning the resource > + * @clk: clock source > + * > + * This prepares the clock source for use. > + * > + * Must not be called from within atomic context. > + */ > +int devm_clk_prepare(struct device *dev, struct clk *clk); > + > +/** > + * devm_clk_unprepare - undo preparation of a managed clock source > + * @dev: device used to prepare the clock > + * @clk: clock source > + * > + * This undoes preparation of a clock, previously prepared with a call > + * to devm_clk_pepare(). > + * > + * Must not be called from within atomic context. > + */ > +void devm_clk_unprepare(struct device *dev, struct clk *clk); > + > +/** > * clk_enable - inform the system when the clock source should be running. > * @clk: clock source > * > @@ -295,6 +318,28 @@ int clk_enable(struct clk *clk); > void clk_disable(struct clk *clk); > > /** > + * devm_clk_prepare_enable - prepare and enable a managed clock source > + * @dev: device owning the clock source > + * @clk: clock source > + * > + * This prepares the clock source for use and enables it. > + * > + * Must not be called from within atomic context. > + */ > +int devm_clk_prepare_enable(struct device *dev, struct clk *clk); > + > +/** > + * devm_clk_disable_unprepare - disable and undo preparation of a managed clock > + * @dev: device used to prepare and enable the clock > + * @clk: clock source > + * > + * This disables and undoes a previously prepared clock. > + * > + * Must not be called from within atomic context. > + */ > +void devm_clk_disable_unprepare(struct device *dev, struct clk *clk); > + > +/** > * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. > * This is only valid once the clock source has been enabled. > * @clk: clock source > @@ -460,6 +505,17 @@ static inline void clk_put(struct clk *clk) {} > > static inline void devm_clk_put(struct device *dev, struct clk *clk) {} > > +static inline int devm_clk_prepare(struct device *dev, struct clk *clk) > +{ > + might_sleep(); > + return 0; > +} > + > +static inline void devm_clk_unprepare(struct device *dev, struct clk *clk) > +{ > + might_sleep(); > +} > + > static inline int clk_enable(struct clk *clk) > { > return 0; > @@ -467,6 +523,18 @@ static inline int clk_enable(struct clk *clk) > > static inline void clk_disable(struct clk *clk) {} > > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > +{ > + might_sleep(); > + return 0; > +} > + > +static inline void devm_clk_disable_unprepare(struct device *dev, > + struct clk *clk) > +{ > + might_sleep(); > +} > + > static inline unsigned long clk_get_rate(struct clk *clk) > { > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/29, Dmitry Torokhov wrote: > When converting a driver to managed resources it is desirable to be able to > manage all resources in the same fashion. This change allows managing > clocks in the same way we manage many other resources. Can you please add 'managing clock prepared and enabled state in the same way'? The current wording makes it sound like we don't have devm_clk_get() when we do. > > This adds the following managed APIs: > > - devm_clk_prepare()/devm_clk_unprepare(); > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it be even shorter to have the APIs devm_clk_get_and_prepare()/devm_clk_unprepare_and_put() devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put() instead? Is there any other subsystem that has similar functionality? Regulators? GPIOs? Resets? I'm just curious if those subsystems also need similar changes.
On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote: > On 01/29, Dmitry Torokhov wrote: > > When converting a driver to managed resources it is desirable to be able to > > manage all resources in the same fashion. This change allows managing > > clocks in the same way we manage many other resources. > > Can you please add 'managing clock prepared and enabled state in > the same way'? > > The current wording makes it sound like we don't have > devm_clk_get() when we do. > > > > > This adds the following managed APIs: > > > > - devm_clk_prepare()/devm_clk_unprepare(); > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it > be even shorter to have the APIs > > devm_clk_get_and_prepare()/devm_clk_unprepare_and_put() > devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put() > > instead? > In many cases I see devm_clk_get(clk1); devm_clk_get(clk2); clk_prepare_enable(clk1); clk_prepare_enable(clk2); Sometimes the calls are intertwined with setting the clock rates. devm_clk_get(clk); clk_set_rate(clk, rate); clk_prepare_enable(clk); Maybe the additional calls make sense; I can imagine they would. However, I personally would be a bit wary of changing the initialization order of multi-clock initializations, and I am not sure how a single call could address setting the rate ([devm_]clk_get_setrate_prepare_enable() seems like a bit too much). [ On a side note, why is there no clk_get_prepare_enable() and clk_get_prepare() ? Maybe it would be better to introduce those together with the matching devm_ functions in a separate patch if they are useful. ] > Is there any other subsystem that has similar functionality? > Regulators? GPIOs? Resets? I'm just curious if those subsystems > also need similar changes. > Ultimately yes, and most already do. If I recall correctly, I tried to introduce devm_ functions for regulators some time ago, but that was rejected with the comment that it would invite misuse. At the time I accepted that; today my reaction would be to counter that pretty much everything can be misused, and that the potential for misuse should not penaltize all the valid use cases. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: > Maybe the additional calls make sense; I can imagine they would. > However, I personally would be a bit wary of changing the initialization > order of multi-clock initializations, and I am not sure how a single call > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() > seems like a bit too much). > > [ On a side note, why is there no clk_get_prepare_enable() and > clk_get_prepare() ? Maybe it would be better to introduce those > together with the matching devm_ functions in a separate patch > if they are useful. ] If you take the view that trying to keep clocks disabled is a good way to save power, then you'd have the clk_prepare() or maybe clk_prepare_enable() in your runtime PM resume handler, or maybe even deeper in the driver... the original design goal of the clk API was to allow power saving and clock control. With that in mind, getting and enabling the clock together in the probe function didn't make sense. I feel that aspect has been somewhat lost, and people now regard much of the clk API as a bit of a probe-time nusience.
On Mon, Jan 30, 2017 at 1:42 PM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: >> Maybe the additional calls make sense; I can imagine they would. >> However, I personally would be a bit wary of changing the initialization >> order of multi-clock initializations, and I am not sure how a single call >> could address setting the rate ([devm_]clk_get_setrate_prepare_enable() >> seems like a bit too much). >> >> [ On a side note, why is there no clk_get_prepare_enable() and >> clk_get_prepare() ? Maybe it would be better to introduce those >> together with the matching devm_ functions in a separate patch >> if they are useful. ] > > If you take the view that trying to keep clocks disabled is a good way > to save power, then you'd have the clk_prepare() or maybe > clk_prepare_enable() in your runtime PM resume handler, or maybe even > deeper in the driver... the original design goal of the clk API was to > allow power saving and clock control. > > With that in mind, getting and enabling the clock together in the > probe function didn't make sense. > > I feel that aspect has been somewhat lost, and people now regard much > of the clk API as a bit of a probe-time nusience. It really depends on the driver. Devices that are expected to be used "all the time", like keyboards, that get "opened" very early in the boot process, maybe even by the kernel itself, usually enable hardware in probe as doing it later just makes code more complex. Devices that are "less used" and could provide more power savings (like GPU) may have more elaborate power management with clocks being turned on and off as needed. I would not paint clk API as "probe-time nuisance", but for some class of devices devm CLK API is really helpful. FWIW I do not think that kitchen sink calls, like "devm_clk_get_prepare_set_rate_enable" are helpful. Resource acquisition and use of said resource are logically separate. Thanks.
On Mon, Jan 30, 2017 at 01:58:05PM -0800, Dmitry Torokhov wrote: > FWIW I do not think that kitchen sink calls, like > "devm_clk_get_prepare_set_rate_enable" are helpful. Resource > acquisition and use of said resource are logically separate. That alone is an argument against devm_clk_get_prepare_enable() in so far that drivers should really get all their resources before they start to enable anything.
On Mon, Jan 30, 2017 at 09:42:28PM +0000, Russell King - ARM Linux wrote: > On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: > > Maybe the additional calls make sense; I can imagine they would. > > However, I personally would be a bit wary of changing the initialization > > order of multi-clock initializations, and I am not sure how a single call > > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() > > seems like a bit too much). > > > > [ On a side note, why is there no clk_get_prepare_enable() and > > clk_get_prepare() ? Maybe it would be better to introduce those > > together with the matching devm_ functions in a separate patch > > if they are useful. ] > > If you take the view that trying to keep clocks disabled is a good way > to save power, then you'd have the clk_prepare() or maybe > clk_prepare_enable() in your runtime PM resume handler, or maybe even > deeper in the driver... the original design goal of the clk API was to > allow power saving and clock control. > > With that in mind, getting and enabling the clock together in the > probe function didn't make sense. > > I feel that aspect has been somewhat lost, and people now regard much > of the clk API as a bit of a probe-time nusience. > While I understand what you are saying, I think just focusing on power savings paints a bit of a simplistic view of the clock API and its use. Power saving is not its only use case. In a system where power saving isn't the highest priority (say, in a high end switch), it is still extremely valuable, providing a unified API to the clocks in the system (and there are lots of clocks in a high end switch). Having seen what happens if there is _no_ unified API (ie a complete mess of per-clock-driver calls all over the place), I consider this as at least as or even more important than its power savings potential. In this use case, one would normally both get and enable the clock (or, much more likely, clocks) in the probe function. One would also need remove functions, since interfaces in a high end switch are typically OIR capable. For my part, I stumbled over the lack of devm functions for clock APIs recently when trying to convert watchdog drivers to use devm functions where possible. Many watchdog drivers do use the clock API to only enable the clock when the watchdog is actually running. However, there are several which prepare and enable the clock at probe time, and disable/unprepare on remove. Would it be possible to convert those to only prepare/enable the clocks if the watchdog is actually enabled ? Possibly, but I am sure that there are cases where that is not possible, or feasible. Either way, watchdog drivers are usually only loaded when actually used, so trying to optimize clock usage would often be more pain than it is worth. When I did that conversion, I started out using devm_add_action_or_reset(). While that does work, it was pointed out that using devm functions for the clock APIs would be a much better solution. As it turns out, devm_add_action() and devm_add_action_or_reset() is already being used by various drivers to work around the lack of devm functions for the clock API. With that in mind, have a choice to make - we can continue forcing people to do that, or we can introduce devm functions. My vote is for the latter. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote: > > On 01/29, Dmitry Torokhov wrote: > > > When converting a driver to managed resources it is desirable to be able to > > > manage all resources in the same fashion. This change allows managing > > > clocks in the same way we manage many other resources. > > > > Can you please add 'managing clock prepared and enabled state in > > the same way'? > > > > The current wording makes it sound like we don't have > > devm_clk_get() when we do. > > > > > > > > This adds the following managed APIs: > > > > > > - devm_clk_prepare()/devm_clk_unprepare(); > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > > > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it > > be even shorter to have the APIs > > > > devm_clk_get_and_prepare()/devm_clk_unprepare_and_put() > > devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put() > > > > instead? > > > In many cases I see > > devm_clk_get(clk1); > devm_clk_get(clk2); > clk_prepare_enable(clk1); > clk_prepare_enable(clk2); > > Sometimes the calls are intertwined with setting the clock rates. > > devm_clk_get(clk); > clk_set_rate(clk, rate); > clk_prepare_enable(clk); > > Maybe the additional calls make sense; I can imagine they would. > However, I personally would be a bit wary of changing the initialization > order of multi-clock initializations, and I am not sure how a single call > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() > seems like a bit too much). > > [ On a side note, why is there no clk_get_prepare_enable() and > clk_get_prepare() ? Maybe it would be better to introduce those > together with the matching devm_ functions in a separate patch > if they are useful. ] > > > Is there any other subsystem that has similar functionality? > > Regulators? GPIOs? Resets? I'm just curious if those subsystems > > also need similar changes. > > > Ultimately yes, and most already do. If I recall correctly, I tried to > introduce devm_ functions for regulators some time ago, but that was > rejected with the comment that it would invite misuse. At the time > I accepted that; today my reaction would be to counter that pretty much > everything can be misused, and that the potential for misuse should not > penaltize all the valid use cases. I think we should ping Mark again. The only thing we are achieving is that everyone is using devm_add_action_or_reset() with wrappers around regulator_put(). As I said elsewhere, there are "always used" devices where it isn't worth it to postpone enabling regulators. Thanks.
Hi Günter, On Mon, Jan 30, 2017 at 11:51 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Mon, Jan 30, 2017 at 09:42:28PM +0000, Russell King - ARM Linux wrote: >> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: >> > Maybe the additional calls make sense; I can imagine they would. >> > However, I personally would be a bit wary of changing the initialization >> > order of multi-clock initializations, and I am not sure how a single call >> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() >> > seems like a bit too much). >> > >> > [ On a side note, why is there no clk_get_prepare_enable() and >> > clk_get_prepare() ? Maybe it would be better to introduce those >> > together with the matching devm_ functions in a separate patch >> > if they are useful. ] >> >> If you take the view that trying to keep clocks disabled is a good way >> to save power, then you'd have the clk_prepare() or maybe >> clk_prepare_enable() in your runtime PM resume handler, or maybe even >> deeper in the driver... the original design goal of the clk API was to >> allow power saving and clock control. >> >> With that in mind, getting and enabling the clock together in the >> probe function didn't make sense. >> >> I feel that aspect has been somewhat lost, and people now regard much >> of the clk API as a bit of a probe-time nusience. > > While I understand what you are saying, I think just focusing on power > savings paints a bit of a simplistic view of the clock API and its use. > Power saving is not its only use case. In a system where power saving isn't > the highest priority (say, in a high end switch), it is still extremely > valuable, providing a unified API to the clocks in the system (and there > are lots of clocks in a high end switch). Having seen what happens if there > is _no_ unified API (ie a complete mess of per-clock-driver calls all over > the place), I consider this as at least as or even more important than its > power savings potential. In this use case, one would normally both get and > enable the clock (or, much more likely, clocks) in the probe function. > One would also need remove functions, since interfaces in a high end switch > are typically OIR capable. > > For my part, I stumbled over the lack of devm functions for clock APIs recently > when trying to convert watchdog drivers to use devm functions where possible. > Many watchdog drivers do use the clock API to only enable the clock when the > watchdog is actually running. However, there are several which prepare and > enable the clock at probe time, and disable/unprepare on remove. Would it be > possible to convert those to only prepare/enable the clocks if the watchdog > is actually enabled ? Possibly, but I am sure that there are cases where that > is not possible, or feasible. Either way, watchdog drivers are usually only > loaded when actually used, so trying to optimize clock usage would often be > more pain than it is worth. > > When I did that conversion, I started out using devm_add_action_or_reset(). > While that does work, it was pointed out that using devm functions for the > clock APIs would be a much better solution. As it turns out, devm_add_action() > and devm_add_action_or_reset() is already being used by various drivers to > work around the lack of devm functions for the clock API. With that in mind, > have a choice to make - we can continue forcing people to do that, or we can > introduce devm functions. My vote is for the latter. W.r.t. clocks, there may be multiple reasons why you care about them: 1. You need to control a clock output to a slave device (e.g. SPI, i2c, audio, ...). Here your driver cares about both enabling/disabling the clock, and programming its clock rate. 2. You need to control a clock because synchronous hardware needs a running clock to operate. Here you only care about enabling/disabling the clock, and this falls under the "power saving" umbrella. These days it's typically handled by a clock domain driver implementing a PM Domain using the genpd framework. Hence your driver doesn't need to manage the clock explicitly (no clk_get()/clk_prepare_enable() etc.), but just calls the pm_runtime_*() functions. Using pm_runtime_*() is always a good idea, even if currently there is no clock to control, as your device may end up in a future SoC that does have a clock (or power domains). 3. Combinations of the above. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote: > On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: > > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote: > > > On 01/29, Dmitry Torokhov wrote: > > > > When converting a driver to managed resources it is desirable to be able to > > > > manage all resources in the same fashion. This change allows managing > > > > clocks in the same way we manage many other resources. > > > > > > Can you please add 'managing clock prepared and enabled state in > > > the same way'? > > > > > > The current wording makes it sound like we don't have > > > devm_clk_get() when we do. > > > > > > > > > > > This adds the following managed APIs: > > > > > > > > - devm_clk_prepare()/devm_clk_unprepare(); > > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > > > > > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it > > > be even shorter to have the APIs > > > > > > devm_clk_get_and_prepare()/devm_clk_unprepare_and_put() > > > devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put() > > > > > > instead? > > > > > In many cases I see > > > > devm_clk_get(clk1); > > devm_clk_get(clk2); > > clk_prepare_enable(clk1); > > clk_prepare_enable(clk2); > > > > Sometimes the calls are intertwined with setting the clock rates. > > > > devm_clk_get(clk); > > clk_set_rate(clk, rate); > > clk_prepare_enable(clk); > > > > Maybe the additional calls make sense; I can imagine they would. > > However, I personally would be a bit wary of changing the initialization > > order of multi-clock initializations, and I am not sure how a single call > > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() > > seems like a bit too much). > > > > [ On a side note, why is there no clk_get_prepare_enable() and > > clk_get_prepare() ? Maybe it would be better to introduce those > > together with the matching devm_ functions in a separate patch > > if they are useful. ] > > > > > Is there any other subsystem that has similar functionality? > > > Regulators? GPIOs? Resets? I'm just curious if those subsystems > > > also need similar changes. > > > > > Ultimately yes, and most already do. If I recall correctly, I tried to > > introduce devm_ functions for regulators some time ago, but that was > > rejected with the comment that it would invite misuse. At the time > > I accepted that; today my reaction would be to counter that pretty much > > everything can be misused, and that the potential for misuse should not > > penaltize all the valid use cases. > > I think we should ping Mark again. The only thing we are achieving is > that everyone is using devm_add_action_or_reset() with wrappers around > regulator_put(). > regulator_get() has an equivalent devm_regulator_get(). Maybe it was since added, or I was thinking about a different function. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 31, 2017 at 9:20 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote: >> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: >> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote: >> > > On 01/29, Dmitry Torokhov wrote: >> > > > When converting a driver to managed resources it is desirable to be able to >> > > > manage all resources in the same fashion. This change allows managing >> > > > clocks in the same way we manage many other resources. >> > > >> > > Can you please add 'managing clock prepared and enabled state in >> > > the same way'? >> > > >> > > The current wording makes it sound like we don't have >> > > devm_clk_get() when we do. >> > > >> > > > >> > > > This adds the following managed APIs: >> > > > >> > > > - devm_clk_prepare()/devm_clk_unprepare(); >> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). >> > > >> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it >> > > be even shorter to have the APIs >> > > >> > > devm_clk_get_and_prepare()/devm_clk_unprepare_and_put() >> > > devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put() >> > > >> > > instead? >> > > >> > In many cases I see >> > >> > devm_clk_get(clk1); >> > devm_clk_get(clk2); >> > clk_prepare_enable(clk1); >> > clk_prepare_enable(clk2); >> > >> > Sometimes the calls are intertwined with setting the clock rates. >> > >> > devm_clk_get(clk); >> > clk_set_rate(clk, rate); >> > clk_prepare_enable(clk); >> > >> > Maybe the additional calls make sense; I can imagine they would. >> > However, I personally would be a bit wary of changing the initialization >> > order of multi-clock initializations, and I am not sure how a single call >> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() >> > seems like a bit too much). >> > >> > [ On a side note, why is there no clk_get_prepare_enable() and >> > clk_get_prepare() ? Maybe it would be better to introduce those >> > together with the matching devm_ functions in a separate patch >> > if they are useful. ] >> > >> > > Is there any other subsystem that has similar functionality? >> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems >> > > also need similar changes. >> > > >> > Ultimately yes, and most already do. If I recall correctly, I tried to >> > introduce devm_ functions for regulators some time ago, but that was >> > rejected with the comment that it would invite misuse. At the time >> > I accepted that; today my reaction would be to counter that pretty much >> > everything can be misused, and that the potential for misuse should not >> > penaltize all the valid use cases. >> >> I think we should ping Mark again. The only thing we are achieving is >> that everyone is using devm_add_action_or_reset() with wrappers around >> regulator_put(). >> > regulator_get() has an equivalent devm_regulator_get(). Maybe it was since > added, or I was thinking about a different function. I think we also need devm_regulator_enable(). Thanks.
On Tue, Jan 31, 2017 at 10:26:29AM -0800, Dmitry Torokhov wrote: > On Tue, Jan 31, 2017 at 9:20 AM, Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote: > >> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote: > >> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote: > >> > > On 01/29, Dmitry Torokhov wrote: > >> > > > When converting a driver to managed resources it is desirable to be able to > >> > > > manage all resources in the same fashion. This change allows managing > >> > > > clocks in the same way we manage many other resources. > >> > > > >> > > Can you please add 'managing clock prepared and enabled state in > >> > > the same way'? > >> > > > >> > > The current wording makes it sound like we don't have > >> > > devm_clk_get() when we do. > >> > > > >> > > > > >> > > > This adds the following managed APIs: > >> > > > > >> > > > - devm_clk_prepare()/devm_clk_unprepare(); > >> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > >> > > > >> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it > >> > > be even shorter to have the APIs > >> > > > >> > > devm_clk_get_and_prepare()/devm_clk_unprepare_and_put() > >> > > devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put() > >> > > > >> > > instead? > >> > > > >> > In many cases I see > >> > > >> > devm_clk_get(clk1); > >> > devm_clk_get(clk2); > >> > clk_prepare_enable(clk1); > >> > clk_prepare_enable(clk2); > >> > > >> > Sometimes the calls are intertwined with setting the clock rates. > >> > > >> > devm_clk_get(clk); > >> > clk_set_rate(clk, rate); > >> > clk_prepare_enable(clk); > >> > > >> > Maybe the additional calls make sense; I can imagine they would. > >> > However, I personally would be a bit wary of changing the initialization > >> > order of multi-clock initializations, and I am not sure how a single call > >> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable() > >> > seems like a bit too much). > >> > > >> > [ On a side note, why is there no clk_get_prepare_enable() and > >> > clk_get_prepare() ? Maybe it would be better to introduce those > >> > together with the matching devm_ functions in a separate patch > >> > if they are useful. ] > >> > > >> > > Is there any other subsystem that has similar functionality? > >> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems > >> > > also need similar changes. > >> > > > >> > Ultimately yes, and most already do. If I recall correctly, I tried to > >> > introduce devm_ functions for regulators some time ago, but that was > >> > rejected with the comment that it would invite misuse. At the time > >> > I accepted that; today my reaction would be to counter that pretty much > >> > everything can be misused, and that the potential for misuse should not > >> > penaltize all the valid use cases. > >> > >> I think we should ping Mark again. The only thing we are achieving is > >> that everyone is using devm_add_action_or_reset() with wrappers around > >> regulator_put(). > >> > > regulator_get() has an equivalent devm_regulator_get(). Maybe it was since > > added, or I was thinking about a different function. > > I think we also need devm_regulator_enable(). > Ah, yes, that was it [1]. And, yes, I do see some devm_add_action() calls around regulator_enable(). Guenter --- [1] https://lkml.org/lkml/2014/2/1/131 -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index 3a218c3a06ae..2ff94ffe11d3 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -9,30 +9,20 @@ #include <linux/export.h> #include <linux/gfp.h> -static void devm_clk_release(struct device *dev, void *res) +static int devm_clk_create_devres(struct device *dev, struct clk *clk, + void (*release)(struct device *, void *)) { - clk_put(*(struct clk **)res); -} + struct clk **ptr; -struct clk *devm_clk_get(struct device *dev, const char *id) -{ - struct clk **ptr, *clk; - - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); + ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL); if (!ptr) - return ERR_PTR(-ENOMEM); + return -ENOMEM; - clk = clk_get(dev, id); - if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); - } + *ptr = clk; + devres_add(dev, ptr); - return clk; + return 0; } -EXPORT_SYMBOL(devm_clk_get); static int devm_clk_match(struct device *dev, void *res, void *data) { @@ -44,31 +34,75 @@ static int devm_clk_match(struct device *dev, void *res, void *data) return *c == data; } -void devm_clk_put(struct device *dev, struct clk *clk) +#define DEFINE_DEVM_CLK_DESTROY_OP(destroy_op) \ +static void devm_##destroy_op##_release(struct device *dev, void *res) \ +{ \ + destroy_op(*(struct clk **)res); \ +} \ + \ +void devm_##destroy_op(struct device *dev, struct clk *clk) \ +{ \ + WARN_ON(devres_release(dev, devm_##destroy_op##_release, \ + devm_clk_match, clk)); \ +} \ +EXPORT_SYMBOL(devm_##destroy_op) + +#define DEFINE_DEVM_CLK_OP(create_op, destroy_op) \ +DEFINE_DEVM_CLK_DESTROY_OP(destroy_op); \ +int devm_##create_op(struct device *dev, struct clk *clk) \ +{ \ + int error; \ + \ + error = create_op(clk); \ + if (error) \ + return error; \ + \ + error = devm_clk_create_devres(dev, clk, \ + devm_##destroy_op##_release); \ + if (error) { \ + destroy_op(clk); \ + return error; \ + } \ + \ + return 0; \ +} \ +EXPORT_SYMBOL(devm_##create_op) + +DEFINE_DEVM_CLK_DESTROY_OP(clk_put); +DEFINE_DEVM_CLK_OP(clk_prepare, clk_unprepare); +DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare); + +struct clk *devm_clk_get(struct device *dev, const char *id) { - int ret; + struct clk *clk; + int error; - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk); + clk = clk_get(dev, id); + if (!IS_ERR(clk)) { + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); + if (error) { + clk_put(clk); + return ERR_PTR(error); + } + } - WARN_ON(ret); + return clk; } -EXPORT_SYMBOL(devm_clk_put); +EXPORT_SYMBOL(devm_clk_get); struct clk *devm_get_clk_from_child(struct device *dev, struct device_node *np, const char *con_id) { - struct clk **ptr, *clk; - - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); - if (!ptr) - return ERR_PTR(-ENOMEM); + struct clk *clk; + int error; clk = of_clk_get_by_name(np, con_id); if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); + if (error) { + clk_put(clk); + return ERR_PTR(error); + } } return clk; diff --git a/include/linux/clk.h b/include/linux/clk.h index e9d36b3e49de..413dc8f636bd 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -267,6 +267,29 @@ struct clk *devm_get_clk_from_child(struct device *dev, struct device_node *np, const char *con_id); /** + * devm_clk_prepare - prepare clock source as a managed resource + * @dev: device owning the resource + * @clk: clock source + * + * This prepares the clock source for use. + * + * Must not be called from within atomic context. + */ +int devm_clk_prepare(struct device *dev, struct clk *clk); + +/** + * devm_clk_unprepare - undo preparation of a managed clock source + * @dev: device used to prepare the clock + * @clk: clock source + * + * This undoes preparation of a clock, previously prepared with a call + * to devm_clk_pepare(). + * + * Must not be called from within atomic context. + */ +void devm_clk_unprepare(struct device *dev, struct clk *clk); + +/** * clk_enable - inform the system when the clock source should be running. * @clk: clock source * @@ -295,6 +318,28 @@ int clk_enable(struct clk *clk); void clk_disable(struct clk *clk); /** + * devm_clk_prepare_enable - prepare and enable a managed clock source + * @dev: device owning the clock source + * @clk: clock source + * + * This prepares the clock source for use and enables it. + * + * Must not be called from within atomic context. + */ +int devm_clk_prepare_enable(struct device *dev, struct clk *clk); + +/** + * devm_clk_disable_unprepare - disable and undo preparation of a managed clock + * @dev: device used to prepare and enable the clock + * @clk: clock source + * + * This disables and undoes a previously prepared clock. + * + * Must not be called from within atomic context. + */ +void devm_clk_disable_unprepare(struct device *dev, struct clk *clk); + +/** * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. * @clk: clock source @@ -460,6 +505,17 @@ static inline void clk_put(struct clk *clk) {} static inline void devm_clk_put(struct device *dev, struct clk *clk) {} +static inline int devm_clk_prepare(struct device *dev, struct clk *clk) +{ + might_sleep(); + return 0; +} + +static inline void devm_clk_unprepare(struct device *dev, struct clk *clk) +{ + might_sleep(); +} + static inline int clk_enable(struct clk *clk) { return 0; @@ -467,6 +523,18 @@ static inline int clk_enable(struct clk *clk) static inline void clk_disable(struct clk *clk) {} +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) +{ + might_sleep(); + return 0; +} + +static inline void devm_clk_disable_unprepare(struct device *dev, + struct clk *clk) +{ + might_sleep(); +} + static inline unsigned long clk_get_rate(struct clk *clk) { return 0;
When converting a driver to managed resources it is desirable to be able to manage all resources in the same fashion. This change allows managing clocks in the same way we manage many other resources. This adds the following managed APIs: - devm_clk_prepare()/devm_clk_unprepare(); - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- v2: dropped devm_clk_enable() and devm_clk_disable() drivers/clk/clk-devres.c | 98 +++++++++++++++++++++++++++++++--------------- include/linux/clk.h | 68 ++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 32 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html