clk: add more managed APIs

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_enable()/devm_clk_disable();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Quoted 9 lines · Dmitry Torokhov

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_enable()/devm_clk_disable();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Does it make any sense what so ever to have devm_clk_enable() and devm_clk_disable()?

Take a moment to think about where you use all of these. The devm_* functions are there to be used in probe functions so that cleanup paths can be streamlined and less erroneous. They aren't for general use throughout the driver.

Given that, there are two operations that you may wish to do in the probe path:

1. prepare a clock (avoiding the enable because you want to perform the enable elsewhere in the driver.)
2. prepare and enable a clock

So, does having devm_clk_enable() really make sense? I don't think it does, and I suspect they'll get very little if any use. So, I think best not add them until someone comes up with a good and wide-spread use case.

Quoted 22 lines · Russell King - ARM Linux

Quoted 9 lines · Dmitry Torokhov

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_enable()/devm_clk_disable();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Does it make any sense what so ever to have devm_clk_enable() and devm_clk_disable()?

Take a moment to think about where you use all of these. The devm_* functions are there to be used in probe functions so that cleanup paths can be streamlined and less erroneous. They aren't for general use throughout the driver.

Given that, there are two operations that you may wish to do in the probe path:

Quoted 8 lines

1. prepare a clock (avoiding the enable because you want to perform the enable elsewhere in the driver.)
2. prepare and enable a clock

So, does having devm_clk_enable() really make sense? I don't think it does, and I suspect they'll get very little if any use. So, I think best not add them until someone comes up with a good and wide-spread use case.

That makes sense.

Guenter, I know you are a coccinelle wizard, can you cook a script that can find current users of clk_enable() in probe paths? Then we can make informed decision on devm_clk_enable.

Quoted 4 lines · Dmitry Torokhov

Guenter, I know you are a coccinelle wizard, can you cook a script that can find current users of clk_enable() in probe paths? Then we can make informed decision on devm_clk_enable.

Questionable use:
drivers/input/keyboard/st-keyscan.c

clk_enable() in probe, clk_disable() in remove:
drivers/i2c/busses/i2c-tegra.c

drivers/media/platform/exynos4-is/fimc-core.c drivers/media/platform/exynos4-is/mipi-csis.c

drivers/thermal/spear_thermal.c

drivers/usb/musb/am35x.c

drivers/usb/musb/davinci.c

Not that many. A quick browse suggests that clk_enable()/clk_disable() is more commonly used to temporarily enable the clock while needed.

Quoted 8 lines · Guenter Roeck

Quoted 4 lines · Dmitry Torokhov

Guenter, I know you are a coccinelle wizard, can you cook a script that can find current users of clk_enable() in probe paths? Then we can make informed decision on devm_clk_enable.

Questionable use:
drivers/input/keyboard/st-keyscan.c

clk_enable() without preceding clk_prepare() - buggy.

Quoted 3 lines

clk_enable() in probe, clk_disable() in remove:
drivers/i2c/busses/i2c-tegra.c

Could be converted to use clk_prepare_enable() or clk_prepare() depending on i2c_dev->is_multimaster_mode in the initialisation path (and their devm_* equivalents.)

Quoted 2 lines

drivers/media/platform/exynos4-is/fimc-core.c drivers/media/platform/exynos4-is/mipi-csis.c

Looks like it does a sequence of:

	devm_clk_get()
	clk_prepare()
	clk_set_rate()
	clk_enable()

which seems a little wrong - clk_set_rate() should be before clk_prepare() if you care about not having the clock output. Remember that clk_prepare() _may_ result in the clock output being enabled. So these two look buggy, and when fixed could use devm_clk_prepare_enable().

Quoted 1 line

drivers/thermal/spear_thermal.c

clk_enable() without a preceding clk_prepare(). Buggy driver.

Quoted 1 line

drivers/usb/musb/am35x.c

Ditto.

Quoted 1 line

drivers/usb/musb/davinci.c

Ditto.

Quoted 2 lines

Not that many. A quick browse suggests that clk_enable()/clk_disable() is more commonly used to temporarily enable the clock while needed.

So out of all those, there's probably only _one_ which is legit - the rest are all technically buggy.

Given that, I'd say there's real reason _not_ to provide devm_clk_enable() to persuade people to use the correct interfaces in the probe path.

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 <[email protected]>

---

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(-)

drivers/clk/clk-devres.c +66 -32

index 3a218c3a06ae..2ff94ffe11d3 100644
@@ -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;

include/linux/clk.h +68 -0

index e9d36b3e49de..413dc8f636bd 100644
@@ -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().

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines

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.

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. ]

Quoted 10 lines · Guenter Roeck

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.

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.

Quoted 10 lines · Guenter Roeck

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.

Quoted 3 lines · Russell King - ARM Linux

Quoted 2 lines · Guenter Roeck

Maybe the additional calls make sense; I can imagine they would. However, I personally would be a bit wary of changing the initializati=

Quoted 1 line

Quoted 1 line

order of multi-clock initializations, and I am not sure how a single c=

Quoted 1 line

Quoted 1 line

could address setting the rate ([devm_]clk_get_setrate_prepare_enable(=

Quoted 16 lines

Quoted 6 lines

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.

Quoted 2 lines

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=

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 the=

is _no_ unified API (ie a complete mess of per-clock-driver calls all ove=

the place), I consider this as at least as or even more important than it=

power savings potential. In this use case, one would normally both get an=

enable the clock (or, much more likely, clocks) in the probe function. One would also need remove functions, since interfaces in a high end swit=

are typically OIR capable.

For my part, I stumbled over the lack of devm functions for clock APIs re=

when trying to convert watchdog drivers to use devm functions where possi=

Many watchdog drivers do use the clock API to only enable the clock when =

watchdog is actually running. However, there are several which prepare an=

enable the clock at probe time, and disable/unprepare on remove. Would it=

possible to convert those to only prepare/enable the clocks if the watchd=

is actually enabled ? Possibly, but I am sure that there are cases where =

is not possible, or feasible. Either way, watchdog drivers are usually on=

loaded when actually used, so trying to optimize clock usage would often =

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 th=

clock APIs would be a much better solution. As it turns out, devm_add_act=

and devm_add_action_or_reset() is already being used by various drivers t=

work around the lack of devm functions for the clock API. With that in mi=

have a choice to make - we can continue forcing people to do that, or we =

introduce devm functions. My vote is for the latter.

Quoted 17 lines · Stephen Boyd

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Quoted 8 lines

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. ]

Quoted 4 lines

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.

Quoted 54 lines · Guenter Roeck

Quoted 17 lines · Stephen Boyd

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Quoted 8 lines

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. ]

Quoted 4 lines

Is there any other subsystem that has similar functionality? Regulators? GPIOs? Resets? I'm just curious if those subsystems also need similar changes.

Quoted 6 lines

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().

Quoted 62 lines · Dmitry Torokhov

Quoted 60 lines · Guenter Roeck

Quoted 17 lines · Stephen Boyd

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Quoted 8 lines

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. ]

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 63 lines · Guenter Roeck

Quoted 62 lines · Dmitry Torokhov

Quoted 60 lines · Guenter Roeck

Quoted 17 lines · Stephen Boyd

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Quoted 8 lines

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. ]

Quoted 4 lines

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.

Quoted 6 lines

Quoted 4 lines

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().

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 clock prepared and enabled state 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().

Reviewed-by Guenter Roeck <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

v3: adjusted commit message, added Guenter's reviewed-by
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(-)

drivers/clk/clk-devres.c +66 -32

index 3a218c3a06ae..2ff94ffe11d3 100644
@@ -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;

include/linux/clk.h +68 -0

index e9d36b3e49de..413dc8f636bd 100644
@@ -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;

Quoted 6 lines · Dmitry Torokhov

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 clock prepared and enabled state in the same way we manage many other resources.

This adds the following managed APIs:

Quoted 5 lines

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Reviewed-by Guenter Roeck <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

It would be awesome if we could get it into 4.11...

Quoted 7 lines · Dmitry Torokhov

Quoted 6 lines · Dmitry Torokhov

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 clock prepared and enabled state in the same way we manage many other resources.

This adds the following managed APIs:

Quoted 7 lines

Quoted 5 lines

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Reviewed-by Guenter Roeck <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

It would be awesome if we could get it into 4.11...

I'd prefer we didn't do this. Instead, make clk_put() drop any prepare or enables that were done via that clk pointer. Mike started to do this before[1], but we have some code that assumes it can do:

        clk = clk_get(...)
        clk_prepare_enable(clk)
        clk_put(clk)

and have the clk stay on. Those would need to be changed.

We would also need Russell's approval to update the clk_put() documentation to describe this change in behavior.

[1] http://lkml.kernel.org/r/[email protected]

Quoted 21 lines · Stephen Boyd

Quoted 7 lines · Dmitry Torokhov

Quoted 6 lines · Dmitry Torokhov

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 clock prepared and enabled state in the same way we manage many other resources.

This adds the following managed APIs:

Quoted 7 lines

Quoted 5 lines

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Reviewed-by Guenter Roeck <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

It would be awesome if we could get it into 4.11...

I'd prefer we didn't do this. Instead, make clk_put() drop any prepare or enables that were done via that clk pointer. Mike started to do this before[1], but we have some code that assumes it can do:

Quoted 5 lines

        clk = clk_get(...)
        clk_prepare_enable(clk)
        clk_put(clk)

and have the clk stay on. Those would need to be changed.

That means we'd need to audit entire code base ;(

Quoted 6 lines

We would also need Russell's approval to update the clk_put() documentation to describe this change in behavior.

[1] http://lkml.kernel.org/r/[email protected]

Note that devm* APIs do not preclude from changing clk_put() behavior down the road and it is extremely easy to go and s/devm_clk_prepare_enable/clk_prepare_enable/ once cleanup is automatic.

Having devm now will help make driver code better (because right now we either need to add wrappers so devm_add_action_or_reset() can be used, or continue mixing devm* and goto cleanups, which are often wrong).

I'd prefer we didn't do this. Instead, make clk_put() drop any prepare or enables that were done via that clk pointer. Mike started to do this before[1], but we have some code that assumes it can do:

	clk = clk_get(...)
	clk_prepare_enable(clk)
	clk_put(clk)

and have the clk stay on. Those would need to be changed.