Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1568994 > unrolled thread

[PATCH] clk: add more managed APIs

Back to article view | Back to linux.kernel

Contents

  [PATCH] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-28 19:50 +0100
    Re: [PATCH] clk: add more managed APIs Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-01-28 20:20 +0100
      Re: [PATCH] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-28 21:00 +0100
        Re: [PATCH] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-28 22:50 +0100
          Re: [PATCH] clk: add more managed APIs Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-01-29 00:50 +0100
            Re: [PATCH] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-29 17:10 +0100
            [PATCH v2] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-29 19:10 +0100
              Re: [PATCH v2] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-29 20:20 +0100

#1568994 — [PATCH] 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().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

This is a resend from about 5 years ago ;) At that time I also tried
uninlining clk_prepare_enable() and clk_disable_unprepare(), but got
lost in the maze of COMMON_CLK/HAVE_CLK/HAVE_CLK_PREPARE. This time
around I left these 2 fucntions alone (inline) and simply adding devm*
equivalents.

Thanks!

 drivers/clk/clk-devres.c |  99 +++++++++++++++++++++++++++++++---------------
 include/linux/clk.h      | 101 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 3a218c3a06ae..ef59eb4f3bc8 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,76 @@ 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_enable, clk_disable);
+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..03c21c5fee4c 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
  *
@@ -279,6 +302,19 @@ struct clk *devm_get_clk_from_child(struct device *dev,
 int clk_enable(struct clk *clk);
 
 /**
+ * devm_clk_enable - enable clock source as a managed resource
+ * @dev: device owning the resource
+ * @clk: clock source
+ *
+ * If the clock can not be enabled/disabled, this should return success.
+ *
+ * May be not called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int devm_clk_enable(struct device *dev, struct clk *clk);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *
@@ -295,6 +331,40 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * devm_clk_disable - disable managed clock source resource
+ * @dev: device owning the clock source
+ * @clk: clock source
+ *
+ * Inform the system that a clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * Must not be called from atomic contexts.
+ */
+void devm_clk_disable(struct device *dev, 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 source
+ * @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 +530,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 +548,26 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline int devm_clk_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void devm_clk_disable(struct device *dev, 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;
-- 
2.11.0.483.g087da7b7c-goog


-- 
Dmitry

[toc] | [next] | [standalone]

#1568998

On Sat, Jan 28, 2017 at 10:40:47AM -0800, 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_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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1569004

On Sat, Jan 28, 2017 at 07:03:10PM +0000, Russell King - ARM Linux wrote:
> On Sat, Jan 28, 2017 at 10:40:47AM -0800, 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_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.

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.

Thanks!

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1569020

On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
> On Sat, Jan 28, 2017 at 07:03:10PM +0000, Russell King - ARM Linux wrote:
>> On Sat, Jan 28, 2017 at 10:40:47AM -0800, 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_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.
>
> 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.
>

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

This does not count drivers which call clk_enable() in probe, but disable
the clock at some point, and only re-enable it when needed.
	
Not that many. A quick browse suggests that clk_enable()/clk_disable()
is more commonly used to temporarily enable the clock while needed.

For clk_prepare(), I get 33 hits in drivers/.

patching file drivers/usb/gadget/udc/at91_udc.c
patching file drivers/i2c/busses/i2c-tegra.c
patching file drivers/pwm/pwm-spear.c
patching file drivers/pinctrl/spear/pinctrl-plgpio.c
patching file drivers/input/keyboard/samsung-keypad.c
patching file drivers/crypto/atmel-aes.c
patching file drivers/usb/gadget/udc/pxa27x_udc.c
patching file drivers/iommu/msm_iommu.c
patching file drivers/i2c/busses/i2c-rk3x.c
patching file drivers/tty/serial/pxa.c
patching file drivers/remoteproc/st_remoteproc.c
patching file drivers/pwm/pwm-tiehrpwm.c
patching file drivers/media/platform/mtk-vpu/mtk_vpu.c
patching file drivers/i2c/busses/i2c-meson.c
patching file drivers/crypto/ux500/hash/hash_core.c
patching file drivers/pwm/pwm-vt8500.c
patching file drivers/pwm/pwm-sti.c
patching file drivers/tty/serial/xilinx_uartps.c
patching file drivers/pwm/pwm-atmel.c
patching file drivers/phy/phy-dm816x-usb.c
patching file drivers/input/keyboard/spear-keyboard.c
patching file drivers/gpu/host1x/mipi.c
patching file drivers/crypto/ux500/cryp/cryp_core.c
patching file drivers/spi/spi-armada-3700.c
patching file drivers/pwm/pwm-mtk-disp.c
patching file drivers/nvmem/mxs-ocotp.c
patching file drivers/media/platform/s5p-g2d/g2d.c
patching file drivers/i2c/busses/i2c-sirf.c
patching file drivers/crypto/atmel-sha.c
patching file drivers/tty/serial/8250/8250_pxa.c
patching file drivers/thermal/samsung/exynos_tmu.c
patching file drivers/media/platform/sti/bdisp/bdisp-v4l2.c
patching file drivers/dma/s3c24xx-dma.c

Those drivers call clk_prepare() in the probe function. The list
may not be complete; my script currently only checks for clk_prepare()
in the probe function of platform, i2c, and spi drivers.

I quick glance through the generated diffs suggests that most
if not all of those call clk_prepare() in probe and clk_unprepare()
in remove.

For clk_prepare_enable() in probe functions, I get 288 hits in drivers/.
I didn't check those for validity - there are just too many. I did check
watchdog and input earlier, though. For those, almost all would be
candidates for devm_clk_prepare_enable().

Overall, I think we should have devm_clk_prepare() and most definitely
devm_clk_prepare_enable(). I am not that sure about clk_enable().

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1569075

On Sat, Jan 28, 2017 at 01:44:35PM -0800, Guenter Roeck wrote:
> On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
> >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.

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

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

> 	drivers/thermal/spear_thermal.c

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

> 	drivers/usb/musb/am35x.c

Ditto.

> 	drivers/usb/musb/davinci.c

Ditto.

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1569224

On 01/28/2017 03:39 PM, Russell King - ARM Linux wrote:
> On Sat, Jan 28, 2017 at 01:44:35PM -0800, Guenter Roeck wrote:
>> On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
>>> 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.
>
>>
>> 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.)
>
>> 	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().
>
>> 	drivers/thermal/spear_thermal.c
>
> clk_enable() without a preceding clk_prepare().  Buggy driver.
>
>> 	drivers/usb/musb/am35x.c
>
> Ditto.
>
>> 	drivers/usb/musb/davinci.c
>
> Ditto.
>
>> 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.
>

I tend to agree, after looking into its existing use.

Do we have agreement on the other two ? I would like to see those in the
next release so we can start using them.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1569256 — [PATCH v2] 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_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(-)

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;

[toc] | [prev] | [next] | [standalone]

#1569265 — Re: [PATCH v2] clk: add more managed APIs

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;
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web