[RFC/PATCH 0/3] CLK: add more devm_* APIs

[RFC/PATCH 0/3] CLK: add more devm_* APIs

[Dmitry Torokhov]

Hi,

When looking at recent driver conversions to managed resources (devm_*) there
is no devm_clk_prepare() and similar functions, which forces mixing of 2
resource management styles (managed/classic) in the same driver, which is not
great.

This patch series adds more devm_* managed APIs to the CLK subsystem so that
driver conversions can be "pure".

Not tested as I do not have relevant hardware, so testing is appreciated.

Thanks!

-- 
Dmitry

[PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

We'll need to invoke clk_unprepare() via a pointer in our devm_*
conversion so let's uninline the pair.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/clk/clk.c   |  4 ++++
 include/linux/clk.h | 68 +++++++++++++++++++++++++----------------------------
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 56e4495e..1b642f2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -374,6 +374,7 @@ struct clk *__clk_lookup(const char *name)
 
 void __clk_unprepare(struct clk *clk)
 {
+#ifdef CONFIG_HAVE_CLK_PREPARE
 	if (!clk)
 		return;
 
@@ -389,6 +390,7 @@ void __clk_unprepare(struct clk *clk)
 		clk->ops->unprepare(clk->hw);
 
 	__clk_unprepare(clk->parent);
+#endif
 }
 
 /**
@@ -412,6 +414,7 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
 
 int __clk_prepare(struct clk *clk)
 {
+#ifdef CONFIG_HAVE_CLK_PREPARE
 	int ret = 0;
 
 	if (!clk)
@@ -432,6 +435,7 @@ int __clk_prepare(struct clk *clk)
 	}
 
 	clk->prepare_count++;
+#endif
 
 	return 0;
 }
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b3ac22d..f8204c3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -84,42 +84,6 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
 
 #endif
 
-/**
- * clk_prepare - prepare a clock source
- * @clk: clock source
- *
- * This prepares the clock source for use.
- *
- * Must not be called from within atomic context.
- */
-#ifdef CONFIG_HAVE_CLK_PREPARE
-int clk_prepare(struct clk *clk);
-#else
-static inline int clk_prepare(struct clk *clk)
-{
-	might_sleep();
-	return 0;
-}
-#endif
-
-/**
- * clk_unprepare - undo preparation of a clock source
- * @clk: clock source
- *
- * This undoes a previously prepared clock.  The caller must balance
- * the number of prepare and unprepare calls.
- *
- * Must not be called from within atomic context.
- */
-#ifdef CONFIG_HAVE_CLK_PREPARE
-void clk_unprepare(struct clk *clk);
-#else
-static inline void clk_unprepare(struct clk *clk)
-{
-	might_sleep();
-}
-#endif
-
 #ifdef CONFIG_HAVE_CLK
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -159,6 +123,27 @@ struct clk *clk_get(struct device *dev, const char *id);
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
 /**
+ * clk_prepare - prepare a clock source
+ * @clk: clock source
+ *
+ * This prepares the clock source for use.
+ *
+ * Must not be called from within atomic context.
+ */
+int clk_prepare(struct clk *clk);
+
+/**
+ * clk_unprepare - undo preparation of a clock source
+ * @clk: clock source
+ *
+ * This undoes a previously prepared clock.  The caller must balance
+ * the number of prepare and unprepare calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprepare(struct clk *clk);
+
+/**
  * clk_enable - inform the system when the clock source should be running.
  * @clk: clock source
  *
@@ -292,6 +277,17 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+static inline int clk_prepare(struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void clk_unprepare(struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
-- 
1.7.11.7

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Russell King - ARM Linux]

On Tue, Nov 20, 2012 at 01:22:17AM -0800, Dmitry Torokhov wrote:
> We'll need to invoke clk_unprepare() via a pointer in our devm_*
> conversion so let's uninline the pair.

NAK.  This breaks non-common clock using implementations.

Why do you need to call this function via a pointer?  That sounds absurd.

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Viresh Kumar]

On 20 November 2012 15:02, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Nov 20, 2012 at 01:22:17AM -0800, Dmitry Torokhov wrote:
>> We'll need to invoke clk_unprepare() via a pointer in our devm_*
>> conversion so let's uninline the pair.
>
> NAK.  This breaks non-common clock using implementations.

Hi Russell,

Dummy routines for non-common clock are still there. Sorry, couldn't get
why this will break those platforms:

+static inline int clk_prepare(struct clk *clk)
+{
+       might_sleep();
+       return 0;
+}
+
+static inline void clk_unprepare(struct clk *clk)
+{
+       might_sleep();
+}
+

--
viresh

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

On Tue, Nov 20, 2012 at 09:32:42AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 20, 2012 at 01:22:17AM -0800, Dmitry Torokhov wrote:
> > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > conversion so let's uninline the pair.
> 
> NAK.  This breaks non-common clock using implementations.

As Viresh mentioned I provided stubs for case when we do not have
CONFIG_HAVE_CLK, so I am not sure how I'll break these platforms, but I
am certainly open for suggestions.
 
> 
> Why do you need to call this function via a pointer?  That sounds absurd.

devres framework takes and stores a pointer to a "destructor" which will
be used later.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Viresh Kumar]

On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> We'll need to invoke clk_unprepare() via a pointer in our devm_*
> conversion so let's uninline the pair.

Sorry, but you aren't doing this :(
This routine is already uninlined as it is in clk.c

Instead you are just moving clk_prepare(), etc calls within
#ifdef CONFIG_HAVE_CLK
#else
#endif

I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
earlier. Can they exist without CONFIG_HAVE_CLK

@Mike: ?

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/clk/clk.c   |  4 ++++
>  include/linux/clk.h | 68 +++++++++++++++++++++++++----------------------------
>  2 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 56e4495e..1b642f2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -374,6 +374,7 @@ struct clk *__clk_lookup(const char *name)
>
>  void __clk_unprepare(struct clk *clk)
>  {
> +#ifdef CONFIG_HAVE_CLK_PREPARE

clk.c is compiled if COMMON_CLK is selected. And COMMON_CLK has following:
	select HAVE_CLK_PREPARE

So, these checks you added don't have a meaning.

>         if (!clk)
>                 return;
>
> @@ -389,6 +390,7 @@ void __clk_unprepare(struct clk *clk)
>                 clk->ops->unprepare(clk->hw);
>
>         __clk_unprepare(clk->parent);
> +#endif
>  }
>
>  /**
> @@ -412,6 +414,7 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
>
>  int __clk_prepare(struct clk *clk)
>  {
> +#ifdef CONFIG_HAVE_CLK_PREPARE

ditto.

>         int ret = 0;
>
>         if (!clk)
> @@ -432,6 +435,7 @@ int __clk_prepare(struct clk *clk)
>         }
>
>         clk->prepare_count++;
> +#endif
>
>         return 0;
>  }
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index b3ac22d..f8204c3 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -84,42 +84,6 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
>
>  #endif
>
> -/**
> - * clk_prepare - prepare a clock source
> - * @clk: clock source
> - *
> - * This prepares the clock source for use.
> - *
> - * Must not be called from within atomic context.
> - */
> -#ifdef CONFIG_HAVE_CLK_PREPARE
> -int clk_prepare(struct clk *clk);
> -#else
> -static inline int clk_prepare(struct clk *clk)
> -{
> -       might_sleep();
> -       return 0;
> -}
> -#endif
> -
> -/**
> - * clk_unprepare - undo preparation of a clock source
> - * @clk: clock source
> - *
> - * This undoes a previously prepared clock.  The caller must balance
> - * the number of prepare and unprepare calls.
> - *
> - * Must not be called from within atomic context.
> - */
> -#ifdef CONFIG_HAVE_CLK_PREPARE
> -void clk_unprepare(struct clk *clk);
> -#else
> -static inline void clk_unprepare(struct clk *clk)
> -{
> -       might_sleep();
> -}
> -#endif
> -
>  #ifdef CONFIG_HAVE_CLK
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -159,6 +123,27 @@ struct clk *clk_get(struct device *dev, const char *id);
>  struct clk *devm_clk_get(struct device *dev, const char *id);
>
>  /**
> + * clk_prepare - prepare a clock source
> + * @clk: clock source
> + *
> + * This prepares the clock source for use.
> + *
> + * Must not be called from within atomic context.
> + */
> +int clk_prepare(struct clk *clk);
> +
> +/**
> + * clk_unprepare - undo preparation of a clock source
> + * @clk: clock source
> + *
> + * This undoes a previously prepared clock.  The caller must balance
> + * the number of prepare and unprepare calls.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_unprepare(struct clk *clk);
> +
> +/**
>   * clk_enable - inform the system when the clock source should be running.
>   * @clk: clock source
>   *
> @@ -292,6 +277,17 @@ static inline void clk_put(struct clk *clk) {}
>
>  static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
>
> +static inline int clk_prepare(struct clk *clk)
> +{
> +       might_sleep();
> +       return 0;
> +}
> +
> +static inline void clk_unprepare(struct clk *clk)
> +{
> +       might_sleep();
> +}
> +
>  static inline int clk_enable(struct clk *clk)
>  {
>         return 0;
> --
> 1.7.11.7
>

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Mike Turquette]

Quoting Viresh Kumar (2012-11-20 02:13:55)
> On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > conversion so let's uninline the pair.
> 
> Sorry, but you aren't doing this :(
> This routine is already uninlined as it is in clk.c
> 
> Instead you are just moving clk_prepare(), etc calls within
> #ifdef CONFIG_HAVE_CLK
> #else
> #endif
> 
> I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
> earlier. Can they exist without CONFIG_HAVE_CLK
> 
> @Mike: ?
> 

HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
selecting HAVE_CLK_PREPARE without HAVE_CLK.

Looking through the code I see that this used to be the case.  Commit
93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
commit was authored by you.  Can you elaborate on why that aspect of the
patch was needed?

Thanks,
Mike

> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/clk/clk.c   |  4 ++++
> >  include/linux/clk.h | 68 +++++++++++++++++++++++++----------------------------
> >  2 files changed, 36 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 56e4495e..1b642f2 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -374,6 +374,7 @@ struct clk *__clk_lookup(const char *name)
> >
> >  void __clk_unprepare(struct clk *clk)
> >  {
> > +#ifdef CONFIG_HAVE_CLK_PREPARE
> 
> clk.c is compiled if COMMON_CLK is selected. And COMMON_CLK has following:
>         select HAVE_CLK_PREPARE
> 
> So, these checks you added don't have a meaning.
> 
> >         if (!clk)
> >                 return;
> >
> > @@ -389,6 +390,7 @@ void __clk_unprepare(struct clk *clk)
> >                 clk->ops->unprepare(clk->hw);
> >
> >         __clk_unprepare(clk->parent);
> > +#endif
> >  }
> >
> >  /**
> > @@ -412,6 +414,7 @@ EXPORT_SYMBOL_GPL(clk_unprepare);
> >
> >  int __clk_prepare(struct clk *clk)
> >  {
> > +#ifdef CONFIG_HAVE_CLK_PREPARE
> 
> ditto.
> 
> >         int ret = 0;
> >
> >         if (!clk)
> > @@ -432,6 +435,7 @@ int __clk_prepare(struct clk *clk)
> >         }
> >
> >         clk->prepare_count++;
> > +#endif
> >
> >         return 0;
> >  }
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index b3ac22d..f8204c3 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -84,42 +84,6 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
> >
> >  #endif
> >
> > -/**
> > - * clk_prepare - prepare a clock source
> > - * @clk: clock source
> > - *
> > - * This prepares the clock source for use.
> > - *
> > - * Must not be called from within atomic context.
> > - */
> > -#ifdef CONFIG_HAVE_CLK_PREPARE
> > -int clk_prepare(struct clk *clk);
> > -#else
> > -static inline int clk_prepare(struct clk *clk)
> > -{
> > -       might_sleep();
> > -       return 0;
> > -}
> > -#endif
> > -
> > -/**
> > - * clk_unprepare - undo preparation of a clock source
> > - * @clk: clock source
> > - *
> > - * This undoes a previously prepared clock.  The caller must balance
> > - * the number of prepare and unprepare calls.
> > - *
> > - * Must not be called from within atomic context.
> > - */
> > -#ifdef CONFIG_HAVE_CLK_PREPARE
> > -void clk_unprepare(struct clk *clk);
> > -#else
> > -static inline void clk_unprepare(struct clk *clk)
> > -{
> > -       might_sleep();
> > -}
> > -#endif
> > -
> >  #ifdef CONFIG_HAVE_CLK
> >  /**
> >   * clk_get - lookup and obtain a reference to a clock producer.
> > @@ -159,6 +123,27 @@ struct clk *clk_get(struct device *dev, const char *id);
> >  struct clk *devm_clk_get(struct device *dev, const char *id);
> >
> >  /**
> > + * clk_prepare - prepare a clock source
> > + * @clk: clock source
> > + *
> > + * This prepares the clock source for use.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +int clk_prepare(struct clk *clk);
> > +
> > +/**
> > + * clk_unprepare - undo preparation of a clock source
> > + * @clk: clock source
> > + *
> > + * This undoes a previously prepared clock.  The caller must balance
> > + * the number of prepare and unprepare calls.
> > + *
> > + * Must not be called from within atomic context.
> > + */
> > +void clk_unprepare(struct clk *clk);
> > +
> > +/**
> >   * clk_enable - inform the system when the clock source should be running.
> >   * @clk: clock source
> >   *
> > @@ -292,6 +277,17 @@ static inline void clk_put(struct clk *clk) {}
> >
> >  static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
> >
> > +static inline int clk_prepare(struct clk *clk)
> > +{
> > +       might_sleep();
> > +       return 0;
> > +}
> > +
> > +static inline void clk_unprepare(struct clk *clk)
> > +{
> > +       might_sleep();
> > +}
> > +
> >  static inline int clk_enable(struct clk *clk)
> >  {
> >         return 0;
> > --
> > 1.7.11.7
> >

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
> Quoting Viresh Kumar (2012-11-20 02:13:55)
> > On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > > conversion so let's uninline the pair.
> > 
> > Sorry, but you aren't doing this :(
> > This routine is already uninlined as it is in clk.c
> > 
> > Instead you are just moving clk_prepare(), etc calls within
> > #ifdef CONFIG_HAVE_CLK
> > #else
> > #endif
> > 
> > I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
> > earlier. Can they exist without CONFIG_HAVE_CLK
> > 
> > @Mike: ?
> > 
> 
> HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> selecting HAVE_CLK_PREPARE without HAVE_CLK.
> 
> Looking through the code I see that this used to be the case.  Commit
> 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> commit was authored by you.  Can you elaborate on why that aspect of the
> patch was needed?
> 

BTW, it looks like the only place where we select HAVE_CLK_PREPARE is
IMX platform and it also selects COMMON_CLK so I think HAVE_CLK_PREPARE
can be removed now.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Russell King - ARM Linux]

On Wed, Nov 21, 2012 at 12:54:24PM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
> > Quoting Viresh Kumar (2012-11-20 02:13:55)
> > > On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > > > conversion so let's uninline the pair.
> > > 
> > > Sorry, but you aren't doing this :(
> > > This routine is already uninlined as it is in clk.c
> > > 
> > > Instead you are just moving clk_prepare(), etc calls within
> > > #ifdef CONFIG_HAVE_CLK
> > > #else
> > > #endif
> > > 
> > > I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
> > > earlier. Can they exist without CONFIG_HAVE_CLK
> > > 
> > > @Mike: ?
> > > 
> > 
> > HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> > selecting HAVE_CLK_PREPARE without HAVE_CLK.
> > 
> > Looking through the code I see that this used to be the case.  Commit
> > 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> > clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> > commit was authored by you.  Can you elaborate on why that aspect of the
> > patch was needed?
> > 
> 
> BTW, it looks like the only place where we select HAVE_CLK_PREPARE is
> IMX platform and it also selects COMMON_CLK so I think HAVE_CLK_PREPARE
> can be removed now.

You've checked non-ARM architectures too?

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

On Wed, Nov 21, 2012 at 10:38:59PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 21, 2012 at 12:54:24PM -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
> > > Quoting Viresh Kumar (2012-11-20 02:13:55)
> > > > On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > > > > conversion so let's uninline the pair.
> > > > 
> > > > Sorry, but you aren't doing this :(
> > > > This routine is already uninlined as it is in clk.c
> > > > 
> > > > Instead you are just moving clk_prepare(), etc calls within
> > > > #ifdef CONFIG_HAVE_CLK
> > > > #else
> > > > #endif
> > > > 
> > > > I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
> > > > earlier. Can they exist without CONFIG_HAVE_CLK
> > > > 
> > > > @Mike: ?
> > > > 
> > > 
> > > HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> > > selecting HAVE_CLK_PREPARE without HAVE_CLK.
> > > 
> > > Looking through the code I see that this used to be the case.  Commit
> > > 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> > > clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> > > commit was authored by you.  Can you elaborate on why that aspect of the
> > > patch was needed?
> > > 
> > 
> > BTW, it looks like the only place where we select HAVE_CLK_PREPARE is
> > IMX platform and it also selects COMMON_CLK so I think HAVE_CLK_PREPARE
> > can be removed now.
> 
> You've checked non-ARM architectures too?

Yes:

[dtor@dtor-d630 linux-next]$ grep -r HAVE_CLK_PREPARE .
./arch/arm/Kconfig:     select HAVE_CLK_PREPARE
Binary file ./.git/objects/pack/pack-7dad5ee164f601f1327dc78648fa317772c2d872.pack matches
./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
./drivers/clk/Kconfig:config HAVE_CLK_PREPARE
./drivers/clk/Kconfig:  select HAVE_CLK_PREPARE

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

On Wed, Nov 21, 2012 at 06:17:50PM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 21, 2012 at 10:38:59PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Nov 21, 2012 at 12:54:24PM -0800, Dmitry Torokhov wrote:
> > > On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
> > > > Quoting Viresh Kumar (2012-11-20 02:13:55)
> > > > > On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > > > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > > > > > conversion so let's uninline the pair.
> > > > > 
> > > > > Sorry, but you aren't doing this :(
> > > > > This routine is already uninlined as it is in clk.c
> > > > > 
> > > > > Instead you are just moving clk_prepare(), etc calls within
> > > > > #ifdef CONFIG_HAVE_CLK
> > > > > #else
> > > > > #endif
> > > > > 
> > > > > I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
> > > > > earlier. Can they exist without CONFIG_HAVE_CLK
> > > > > 
> > > > > @Mike: ?
> > > > > 
> > > > 
> > > > HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> > > > selecting HAVE_CLK_PREPARE without HAVE_CLK.
> > > > 
> > > > Looking through the code I see that this used to be the case.  Commit
> > > > 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> > > > clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> > > > commit was authored by you.  Can you elaborate on why that aspect of the
> > > > patch was needed?
> > > > 
> > > 
> > > BTW, it looks like the only place where we select HAVE_CLK_PREPARE is
> > > IMX platform and it also selects COMMON_CLK so I think HAVE_CLK_PREPARE
> > > can be removed now.
> > 
> > You've checked non-ARM architectures too?
> 
> Yes:
> 
> [dtor@dtor-d630 linux-next]$ grep -r HAVE_CLK_PREPARE .
> ./arch/arm/Kconfig:     select HAVE_CLK_PREPARE
> Binary file ./.git/objects/pack/pack-7dad5ee164f601f1327dc78648fa317772c2d872.pack matches
> ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> ./drivers/clk/Kconfig:config HAVE_CLK_PREPARE
> ./drivers/clk/Kconfig:  select HAVE_CLK_PREPARE
> 
> Thanks.
> 

So how about the one blow?

-- 
Dmitry

CLK: get rid of HAVE_CLK_PREPARE

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

HAVE_CLK_PREPARE is automatically selected by COMMON_CLK and the only
platform that explicitly selects HAVE_CLK_PREPARE is MXS which has been
switched to common clk framework, so we can delete this option now.

As part of this change we move declarations of clk_prepare() and
clk_unprepare() under HAVE_CLK and provide stubs if this option is not
enabled.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/clk/Kconfig |    4 ---
 include/linux/clk.h |   68 ++++++++++++++++++++++++---------------------------
 2 files changed, 32 insertions(+), 40 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index bace9e9..639ee9d 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -3,15 +3,11 @@ config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
 
-config HAVE_CLK_PREPARE
-	bool
-
 config HAVE_MACH_CLKDEV
 	bool
 
 config COMMON_CLK
 	bool
-	select HAVE_CLK_PREPARE
 	select CLKDEV_LOOKUP
 	---help---
 	  The common clock framework is a single definition of struct
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b3ac22d..f8204c3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -84,42 +84,6 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
 
 #endif
 
-/**
- * clk_prepare - prepare a clock source
- * @clk: clock source
- *
- * This prepares the clock source for use.
- *
- * Must not be called from within atomic context.
- */
-#ifdef CONFIG_HAVE_CLK_PREPARE
-int clk_prepare(struct clk *clk);
-#else
-static inline int clk_prepare(struct clk *clk)
-{
-	might_sleep();
-	return 0;
-}
-#endif
-
-/**
- * clk_unprepare - undo preparation of a clock source
- * @clk: clock source
- *
- * This undoes a previously prepared clock.  The caller must balance
- * the number of prepare and unprepare calls.
- *
- * Must not be called from within atomic context.
- */
-#ifdef CONFIG_HAVE_CLK_PREPARE
-void clk_unprepare(struct clk *clk);
-#else
-static inline void clk_unprepare(struct clk *clk)
-{
-	might_sleep();
-}
-#endif
-
 #ifdef CONFIG_HAVE_CLK
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -159,6 +123,27 @@ struct clk *clk_get(struct device *dev, const char *id);
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
 /**
+ * clk_prepare - prepare a clock source
+ * @clk: clock source
+ *
+ * This prepares the clock source for use.
+ *
+ * Must not be called from within atomic context.
+ */
+int clk_prepare(struct clk *clk);
+
+/**
+ * clk_unprepare - undo preparation of a clock source
+ * @clk: clock source
+ *
+ * This undoes a previously prepared clock.  The caller must balance
+ * the number of prepare and unprepare calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprepare(struct clk *clk);
+
+/**
  * clk_enable - inform the system when the clock source should be running.
  * @clk: clock source
  *
@@ -292,6 +277,17 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+static inline int clk_prepare(struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void clk_unprepare(struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Mike Turquette]

Quoting Dmitry Torokhov (2012-11-21 19:11:17)
> On Wed, Nov 21, 2012 at 06:17:50PM -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 21, 2012 at 10:38:59PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Nov 21, 2012 at 12:54:24PM -0800, Dmitry Torokhov wrote:
> > > > On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
> > > > > Quoting Viresh Kumar (2012-11-20 02:13:55)
> > > > > > On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > > > > We'll need to invoke clk_unprepare() via a pointer in our devm_*
> > > > > > > conversion so let's uninline the pair.
> > > > > > 
> > > > > > Sorry, but you aren't doing this :(
> > > > > > This routine is already uninlined as it is in clk.c
> > > > > > 
> > > > > > Instead you are just moving clk_prepare(), etc calls within
> > > > > > #ifdef CONFIG_HAVE_CLK
> > > > > > #else
> > > > > > #endif
> > > > > > 
> > > > > > I doubt why they have been added under #ifdef CONFIG_HAVE_CLK_PREPARE
> > > > > > earlier. Can they exist without CONFIG_HAVE_CLK
> > > > > > 
> > > > > > @Mike: ?
> > > > > > 
> > > > > 
> > > > > HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> > > > > selecting HAVE_CLK_PREPARE without HAVE_CLK.
> > > > > 
> > > > > Looking through the code I see that this used to be the case.  Commit
> > > > > 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> > > > > clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> > > > > commit was authored by you.  Can you elaborate on why that aspect of the
> > > > > patch was needed?
> > > > > 
> > > > 
> > > > BTW, it looks like the only place where we select HAVE_CLK_PREPARE is
> > > > IMX platform and it also selects COMMON_CLK so I think HAVE_CLK_PREPARE
> > > > can be removed now.
> > > 
> > > You've checked non-ARM architectures too?
> > 
> > Yes:
> > 
> > [dtor@dtor-d630 linux-next]$ grep -r HAVE_CLK_PREPARE .
> > ./arch/arm/Kconfig:     select HAVE_CLK_PREPARE
> > Binary file ./.git/objects/pack/pack-7dad5ee164f601f1327dc78648fa317772c2d872.pack matches
> > ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> > ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> > ./drivers/clk/Kconfig:config HAVE_CLK_PREPARE
> > ./drivers/clk/Kconfig:  select HAVE_CLK_PREPARE
> > 
> > Thanks.
> > 
> 
> So how about the one blow?
> 
> -- 
> Dmitry
> 
> CLK: get rid of HAVE_CLK_PREPARE
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> HAVE_CLK_PREPARE is automatically selected by COMMON_CLK and the only
> platform that explicitly selects HAVE_CLK_PREPARE is MXS which has been
> switched to common clk framework, so we can delete this option now.
> 
> As part of this change we move declarations of clk_prepare() and
> clk_unprepare() under HAVE_CLK and provide stubs if this option is not
> enabled.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Looks right to me.  Just need to remove the select from IMX as well.

Regards,
Mike

> ---
>  drivers/clk/Kconfig |    4 ---
>  include/linux/clk.h |   68 ++++++++++++++++++++++++---------------------------
>  2 files changed, 32 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index bace9e9..639ee9d 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -3,15 +3,11 @@ config CLKDEV_LOOKUP
>         bool
>         select HAVE_CLK
>  
> -config HAVE_CLK_PREPARE
> -       bool
> -
>  config HAVE_MACH_CLKDEV
>         bool
>  
>  config COMMON_CLK
>         bool
> -       select HAVE_CLK_PREPARE
>         select CLKDEV_LOOKUP
>         ---help---
>           The common clock framework is a single definition of struct
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index b3ac22d..f8204c3 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -84,42 +84,6 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
>  
>  #endif
>  
> -/**
> - * clk_prepare - prepare a clock source
> - * @clk: clock source
> - *
> - * This prepares the clock source for use.
> - *
> - * Must not be called from within atomic context.
> - */
> -#ifdef CONFIG_HAVE_CLK_PREPARE
> -int clk_prepare(struct clk *clk);
> -#else
> -static inline int clk_prepare(struct clk *clk)
> -{
> -       might_sleep();
> -       return 0;
> -}
> -#endif
> -
> -/**
> - * clk_unprepare - undo preparation of a clock source
> - * @clk: clock source
> - *
> - * This undoes a previously prepared clock.  The caller must balance
> - * the number of prepare and unprepare calls.
> - *
> - * Must not be called from within atomic context.
> - */
> -#ifdef CONFIG_HAVE_CLK_PREPARE
> -void clk_unprepare(struct clk *clk);
> -#else
> -static inline void clk_unprepare(struct clk *clk)
> -{
> -       might_sleep();
> -}
> -#endif
> -
>  #ifdef CONFIG_HAVE_CLK
>  /**
>   * clk_get - lookup and obtain a reference to a clock producer.
> @@ -159,6 +123,27 @@ struct clk *clk_get(struct device *dev, const char *id);
>  struct clk *devm_clk_get(struct device *dev, const char *id);
>  
>  /**
> + * clk_prepare - prepare a clock source
> + * @clk: clock source
> + *
> + * This prepares the clock source for use.
> + *
> + * Must not be called from within atomic context.
> + */
> +int clk_prepare(struct clk *clk);
> +
> +/**
> + * clk_unprepare - undo preparation of a clock source
> + * @clk: clock source
> + *
> + * This undoes a previously prepared clock.  The caller must balance
> + * the number of prepare and unprepare calls.
> + *
> + * Must not be called from within atomic context.
> + */
> +void clk_unprepare(struct clk *clk);
> +
> +/**
>   * clk_enable - inform the system when the clock source should be running.
>   * @clk: clock source
>   *
> @@ -292,6 +277,17 @@ static inline void clk_put(struct clk *clk) {}
>  
>  static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
>  
> +static inline int clk_prepare(struct clk *clk)
> +{
> +       might_sleep();
> +       return 0;
> +}
> +
> +static inline void clk_unprepare(struct clk *clk)
> +{
> +       might_sleep();
> +}
> +
>  static inline int clk_enable(struct clk *clk)
>  {
>         return 0;

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Russell King - ARM Linux]

On Wed, Nov 21, 2012 at 07:24:07PM -0800, Mike Turquette wrote:
> Looks right to me.  Just need to remove the select from IMX as well.

Then you don't understand HAVE_CLK_PREPARE.

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Viresh Kumar]

On 22 November 2012 08:41, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> CLK: get rid of HAVE_CLK_PREPARE
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> HAVE_CLK_PREPARE is automatically selected by COMMON_CLK and the only
> platform that explicitly selects HAVE_CLK_PREPARE is MXS which has been
> switched to common clk framework, so we can delete this option now.
>
> As part of this change we move declarations of clk_prepare() and
> clk_unprepare() under HAVE_CLK and provide stubs if this option is not
> enabled.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/clk/Kconfig |    4 ---
>  include/linux/clk.h |   68 ++++++++++++++++++++++++---------------------------

Fix imx select line as pointed out by Mike and add:

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Russell King - ARM Linux]

On Wed, Nov 21, 2012 at 06:17:50PM -0800, Dmitry Torokhov wrote:
> > You've checked non-ARM architectures too?
> 
> Yes:
> 
> [dtor@dtor-d630 linux-next]$ grep -r HAVE_CLK_PREPARE .
> ./arch/arm/Kconfig:     select HAVE_CLK_PREPARE
> Binary file ./.git/objects/pack/pack-7dad5ee164f601f1327dc78648fa317772c2d872.pack matches
> ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> ./drivers/clk/Kconfig:config HAVE_CLK_PREPARE
> ./drivers/clk/Kconfig:  select HAVE_CLK_PREPARE

Err, no you haven't, not with that grep.  What you've found are the places
which enable this, and say "yes, I have clk_prepare".

What HAVE_CLK_PREPARE is about though is providing a transition path between
drivers using clk_prepare() to platforms which _don't_ have a clk_prepare()
implementation - and when it's unset, it provides a default implementation.

So, finding all those places where the symbol exists is the exact opposite
of what you need to be doing.  You need to find those platforms which have
CLK support, but which don't have HAVE_CLK_PREPARE selected.

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Russell King - ARM Linux]

On Thu, Nov 22, 2012 at 09:30:33AM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 21, 2012 at 06:17:50PM -0800, Dmitry Torokhov wrote:
> > > You've checked non-ARM architectures too?
> > 
> > Yes:
> > 
> > [dtor@dtor-d630 linux-next]$ grep -r HAVE_CLK_PREPARE .
> > ./arch/arm/Kconfig:     select HAVE_CLK_PREPARE
> > Binary file ./.git/objects/pack/pack-7dad5ee164f601f1327dc78648fa317772c2d872.pack matches
> > ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> > ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> > ./drivers/clk/Kconfig:config HAVE_CLK_PREPARE
> > ./drivers/clk/Kconfig:  select HAVE_CLK_PREPARE
> 
> Err, no you haven't, not with that grep.  What you've found are the places
> which enable this, and say "yes, I have clk_prepare".
> 
> What HAVE_CLK_PREPARE is about though is providing a transition path between
> drivers using clk_prepare() to platforms which _don't_ have a clk_prepare()
> implementation - and when it's unset, it provides a default implementation.
> 
> So, finding all those places where the symbol exists is the exact opposite
> of what you need to be doing.  You need to find those platforms which have
> CLK support, but which don't have HAVE_CLK_PREPARE selected.

Right, according to my greps under arch/arm, the following define a
clk_enable() function but do not define a clk_prepare() function:

arch/arm/mach-w90x900/clock.c
arch/arm/mach-ep93xx/clock.c
arch/arm/plat-omap/clock.c
arch/arm/plat-samsung/clock.c
arch/arm/mach-lpc32xx/clock.c
arch/arm/mach-msm/clock.c
arch/arm/mach-mmp/clock.c
arch/arm/mach-sa1100/clock.c
arch/arm/mach-at91/clock.c
arch/arm/mach-at91/at91x40.c
arch/arm/mach-pxa/clock.c
arch/arm/plat-versatile/clock.c
arch/arm/mach-davinci/clock.c

This list gets over twice as big if you widen the search to the arch/
subtree.

If any of these makes use of a driver which makes a call to clk_prepare(),
removing HAVE_CLK_PREPARE will break all those platforms.

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Viresh Kumar]

Hi Russell,

On 22 November 2012 15:00, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> Err, no you haven't, not with that grep.  What you've found are the places
> which enable this, and say "yes, I have clk_prepare".
>
> What HAVE_CLK_PREPARE is about though is providing a transition path between
> drivers using clk_prepare() to platforms which _don't_ have a clk_prepare()
> implementation - and when it's unset, it provides a default implementation.

Just to make it more clear:

Categories of platforms:
- COMMON_CLK=y: For them it is mandatory to have clk_[un]prepare
- COMMON_CLK=n:
   - HAVE_CLK=n: dummy implementation suggested in this patch is enough for it.
      Even existing implementation too.
   - HAVE_CLK=y:
       - HAVE_CLK_PREPARE=y: Platforms must have their own implementation of
          this routine and so a prototype is enough in clk.h
       - HAVE_CLK_PREPARE=n: This is the problematic place. Who will provide
         implementation of dummy routine here? With current patch
Neither platform
         nor clk.h is providing that.

Sorry for not reviewing it properly :(

--
viresh

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

On Thu, Nov 22, 2012 at 09:30:33AM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 21, 2012 at 06:17:50PM -0800, Dmitry Torokhov wrote:
> > > You've checked non-ARM architectures too?
> > 
> > Yes:
> > 
> > [dtor@dtor-d630 linux-next]$ grep -r HAVE_CLK_PREPARE .
> > ./arch/arm/Kconfig:     select HAVE_CLK_PREPARE
> > Binary file ./.git/objects/pack/pack-7dad5ee164f601f1327dc78648fa317772c2d872.pack matches
> > ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> > ./include/linux/clk.h:#ifdef CONFIG_HAVE_CLK_PREPARE
> > ./drivers/clk/Kconfig:config HAVE_CLK_PREPARE
> > ./drivers/clk/Kconfig:  select HAVE_CLK_PREPARE
> 
> Err, no you haven't, not with that grep.  What you've found are the places
> which enable this, and say "yes, I have clk_prepare".
> 
> What HAVE_CLK_PREPARE is about though is providing a transition path between
> drivers using clk_prepare() to platforms which _don't_ have a clk_prepare()
> implementation - and when it's unset, it provides a default implementation.

Ahh, I see. Then I think my first patch was correct albeit it had bad changelog
message. If provided stubs for clk_prepare()/clk_unprepare() for
platforms that did not define HAVE_CLK and pushed the check for
HAVE_CLK_PREPARE down into drivers/clk/clk.c so __clk_prepare() would
either call platform implementation or just be an empty function.

Am I correct or I am still missing something?

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Viresh Kumar]

On 23 November 2012 12:49, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Ahh, I see. Then I think my first patch was correct albeit it had bad changelog
> message. If provided stubs for clk_prepare()/clk_unprepare() for
> platforms that did not define HAVE_CLK and pushed the check for
> HAVE_CLK_PREPARE down into drivers/clk/clk.c so __clk_prepare() would
> either call platform implementation or just be an empty function.
>
> Am I correct or I am still missing something?

I believe you are still missing it :)

clk.c will only be compiled when we have COMMON_CLK and
COMMON_CLK selects HAVE_CLK_PREPARE.

So, using HAVE_CLK_PREPARE in clk.c is useless, as its always true.
I feel, the best solution would be to simply drop patch 1 and apply others.

--
viresh

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Dmitry Torokhov]

On Fri, Nov 23, 2012 at 12:57:54PM +0530, Viresh Kumar wrote:
> On 23 November 2012 12:49, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > Ahh, I see. Then I think my first patch was correct albeit it had bad changelog
> > message. If provided stubs for clk_prepare()/clk_unprepare() for
> > platforms that did not define HAVE_CLK and pushed the check for
> > HAVE_CLK_PREPARE down into drivers/clk/clk.c so __clk_prepare() would
> > either call platform implementation or just be an empty function.
> >
> > Am I correct or I am still missing something?
> 
> I believe you are still missing it :)
> 
> clk.c will only be compiled when we have COMMON_CLK and
> COMMON_CLK selects HAVE_CLK_PREPARE.
> 
> So, using HAVE_CLK_PREPARE in clk.c is useless, as its always true.
> I feel, the best solution would be to simply drop patch 1 and apply others.

Right... OK, I'll drop the first patch.

-- 
Dmitry

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Shawn Guo]

On Fri, Nov 23, 2012 at 12:08:58AM -0800, Dmitry Torokhov wrote:
> On Fri, Nov 23, 2012 at 12:57:54PM +0530, Viresh Kumar wrote:
> > On 23 November 2012 12:49, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > Ahh, I see. Then I think my first patch was correct albeit it had bad changelog
> > > message. If provided stubs for clk_prepare()/clk_unprepare() for
> > > platforms that did not define HAVE_CLK and pushed the check for
> > > HAVE_CLK_PREPARE down into drivers/clk/clk.c so __clk_prepare() would
> > > either call platform implementation or just be an empty function.
> > >
> > > Am I correct or I am still missing something?
> > 
> > I believe you are still missing it :)
> > 
> > clk.c will only be compiled when we have COMMON_CLK and
> > COMMON_CLK selects HAVE_CLK_PREPARE.
> > 
> > So, using HAVE_CLK_PREPARE in clk.c is useless, as its always true.
> > I feel, the best solution would be to simply drop patch 1 and apply others.
> 
> Right... OK, I'll drop the first patch.
> 
Removing HAVE_CLK_PREPARE from ARCH_MXS stands valid though.  I will
send another patch to do that.

Shawn

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Viresh Kumar]

On 22 November 2012 02:13, Mike Turquette <mturquette@ti.com> wrote:
> HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> selecting HAVE_CLK_PREPARE without HAVE_CLK.
>
> Looking through the code I see that this used to be the case.  Commit
> 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> commit was authored by you.  Can you elaborate on why that aspect of the
> patch was needed?

Haha... Caught red handed :(

Before this commit, nothing was enclosed within CONFIG_HAVE_CLK and
this patch only introduced it. I am not really sure, why i kept
prepare/unprepare
out of it though :(

Maybe because some platform at that time is using it directly, without
CONFIG_HAVE_CLK. Not sure.

--
viresh

Re: [PATCH 1/3] CLK: uninline clk_prepare() and clk_unprepare()

[Mike Turquette]

Quoting Viresh Kumar (2012-11-21 19:34:18)
> On 22 November 2012 02:13, Mike Turquette <mturquette@ti.com> wrote:
> > HAVE_CLK logically wraps HAVE_CLK_PREPARE.  There is no point in
> > selecting HAVE_CLK_PREPARE without HAVE_CLK.
> >
> > Looking through the code I see that this used to be the case.  Commit
> > 93abe8e "clk: add non CONFIG_HAVE_CLK routines" moved the
> > clk_(un)prepare declarations outside of #ifdef CONFIG_HAVE_CLK.  That
> > commit was authored by you.  Can you elaborate on why that aspect of the
> > patch was needed?
> 
> Haha... Caught red handed :(
> 
> Before this commit, nothing was enclosed within CONFIG_HAVE_CLK and
> this patch only introduced it. I am not really sure, why i kept
> prepare/unprepare
> out of it though :(
> 
> Maybe because some platform at that time is using it directly, without
> CONFIG_HAVE_CLK. Not sure.
> 

No worries.  Looks like everything gets sorted out in the end ;)

Regards,
Mike

> --
> viresh

[PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Dmitry Torokhov]

We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
conversion so let's uninline the pair.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/clk/clk.c   | 23 +++++++++++++++++++++++
 include/linux/clk.h | 53 +++++++++++++++++++++++++++++++----------------------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1b642f2..69dc7ba 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -561,6 +561,29 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+int clk_prepare_enable(struct clk *clk)
+{
+	int ret;
+
+	ret = clk_prepare(clk);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(clk);
+	if (ret)
+		clk_unprepare(clk);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare_enable);
+
+void clk_disable_unprepare(struct clk *clk)
+{
+	clk_disable(clk);
+	clk_unprepare(clk);
+}
+EXPORT_SYMBOL_GPL(clk_disable_unprepare);
+
 /**
  * __clk_round_rate - round the given rate for a clk
  * @clk: round the rate of this clock
diff --git a/include/linux/clk.h b/include/linux/clk.h
index f8204c3..8bf149e 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -172,6 +172,26 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_prepare_enable - prepare and enable a clock source
+ * @clk: clock source
+ *
+ * This prepares the clock source for use and enables it.
+ *
+ * Must not be called from within atomic context.
+ */
+int clk_prepare_enable(struct clk *clk);
+
+/**
+ * clk_disable_unprepare - disable and undo preparation of a clock source
+ * @clk: clock source
+ *
+ * This disables and undoes a previously prepared clock.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_disable_unprepare(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
@@ -295,6 +315,17 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline int clk_prepare_enable(struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void clk_disable_unprepare(struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;
@@ -322,28 +353,6 @@ static inline struct clk *clk_get_parent(struct clk *clk)
 
 #endif
 
-/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
-static inline int clk_prepare_enable(struct clk *clk)
-{
-	int ret;
-
-	ret = clk_prepare(clk);
-	if (ret)
-		return ret;
-	ret = clk_enable(clk);
-	if (ret)
-		clk_unprepare(clk);
-
-	return ret;
-}
-
-/* clk_disable_unprepare helps cases using clk_disable in non-atomic context. */
-static inline void clk_disable_unprepare(struct clk *clk)
-{
-	clk_disable(clk);
-	clk_unprepare(clk);
-}
-
 /**
  * clk_add_alias - add a new clock alias
  * @alias: name for clock alias
-- 
1.7.11.7

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Russell King - ARM Linux]

On Tue, Nov 20, 2012 at 01:22:18AM -0800, Dmitry Torokhov wrote:
> We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
> conversion so let's uninline the pair.

Again, what about stuff not using drivers/clk/clk.c ?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Viresh Kumar]

On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
> conversion so let's uninline the pair.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

This one matches its expectations :)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Viresh Kumar]

On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
> conversion so let's uninline the pair.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Mike, are you taking these patches?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Viresh Kumar]

[Adding Linaro id of Mike]

On 16 December 2012 17:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
>> conversion so let's uninline the pair.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Mike, are you taking these patches?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Russell King - ARM Linux]

On Sun, Dec 16, 2012 at 05:10:36PM +0530, Viresh Kumar wrote:
> On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
> > conversion so let's uninline the pair.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Mike, are you taking these patches?

And what about my comments, some of which you've failed to reply to?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Viresh Kumar]

On 16 December 2012 17:27, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Dec 16, 2012 at 05:10:36PM +0530, Viresh Kumar wrote:
>> On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> > We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
>> > conversion so let's uninline the pair.
>> >
>> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> Mike, are you taking these patches?
>
> And what about my comments, some of which you've failed to reply to?

Surprised!! I thought there was nothing missing after the last
discussion i remember.
Dmitry agreed to drop the first patch and all other looked fine, as
nobody objected
to them again. Yes, you did in the beginning but there were valid
replies to them, on
which you never objected.

So, i thought its all good now. Can you point again to the issues still left ?

--
viresh

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Russell King - ARM Linux]

On Sun, Dec 16, 2012 at 05:50:44PM +0530, Viresh Kumar wrote:
> On 16 December 2012 17:27, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Dec 16, 2012 at 05:10:36PM +0530, Viresh Kumar wrote:
> >> On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >> > We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
> >> > conversion so let's uninline the pair.
> >> >
> >> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>
> >> Mike, are you taking these patches?
> >
> > And what about my comments, some of which you've failed to reply to?
> 
> Surprised!! I thought there was nothing missing after the last
> discussion i remember.
> Dmitry agreed to drop the first patch and all other looked fine, as
> nobody objected
> to them again. Yes, you did in the beginning but there were valid
> replies to them, on
> which you never objected.
> 
> So, i thought its all good now. Can you point again to the issues still left ?

Well, there's my comment against patch 2 which never got a reply:

"Again, what about stuff not using drivers/clk/clk.c ?"

Has this been addressed?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Viresh Kumar]

On 16 December 2012 18:10, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> Well, there's my comment against patch 2 which never got a reply:
>
> "Again, what about stuff not using drivers/clk/clk.c ?"
>
> Has this been addressed?

Hmm.. I misread it and thought it is same as breaking other platforms
because there are
no dummy routines. But i was wrong :(

So, the problem is, platform not using common-clock framework uses
this routine, and they
don't want it to be dummy but call prepare & enable..

Because Dmirty requires this one to be non-inline, either he can move
these routines to
drivers/clk/clk-devres.c (which would be wrong) or can add wrappers
over them in clk-devres
file.

--
viresh

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Russell King - ARM Linux]

On Sun, Dec 16, 2012 at 06:35:24PM +0530, Viresh Kumar wrote:
> On 16 December 2012 18:10, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Well, there's my comment against patch 2 which never got a reply:
> >
> > "Again, what about stuff not using drivers/clk/clk.c ?"
> >
> > Has this been addressed?
> 
> Hmm.. I misread it and thought it is same as breaking other platforms
> because there are
> no dummy routines. But i was wrong :(
> 
> So, the problem is, platform not using common-clock framework uses
> this routine, and they
> don't want it to be dummy but call prepare & enable..
> 
> Because Dmirty requires this one to be non-inline, either he can move
> these routines to
> drivers/clk/clk-devres.c (which would be wrong) or can add wrappers
> over them in clk-devres
> file.

The point of the inlines in linux/clk.h is so that people using the clk
API have a way to transition to the new prepare+enable solution without
having their drivers break.  This patch series totally wrecks that by
making clk_prepare() private to the common clock framework.  All the
time that it does that, it's totally and utterly unsuitable for going
into mainline.

Is that strong enough language that my point is properly heard?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Dmitry Torokhov]

Hi Viresh,

On Sun, Dec 16, 2012 at 06:35:24PM +0530, Viresh Kumar wrote:
> On 16 December 2012 18:10, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Well, there's my comment against patch 2 which never got a reply:
> >
> > "Again, what about stuff not using drivers/clk/clk.c ?"
> >
> > Has this been addressed?
> 
> Hmm.. I misread it and thought it is same as breaking other platforms
> because there are
> no dummy routines. But i was wrong :(
> 
> So, the problem is, platform not using common-clock framework uses
> this routine, and they
> don't want it to be dummy but call prepare & enable..
> 
> Because Dmirty requires this one to be non-inline, either he can move
> these routines to
> drivers/clk/clk-devres.c (which would be wrong) or can add wrappers
> over them in clk-devres
> file.

They do not _have_ to be non-inline, I think we should simply drop the
first 2 patches and I will refresh and ressend the 3rd one.

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Viresh Kumar]

On 17 December 2012 11:12, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> They do not _have_ to be non-inline, I think we should simply drop the
> first 2 patches and I will refresh and ressend the 3rd one.

I haven't seen this patch since a long time? Any updates?

Re: [PATCH 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Andy Shevchenko]

Dmitry, what is the status of this patchseries? Are you continue to
make it upstream?

On Mon, Apr 8, 2013 at 1:19 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 17 December 2012 11:12, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> They do not _have_ to be non-inline, I think we should simply drop the
>> first 2 patches and I will refresh and ressend the 3rd one.
>
> I haven't seen this patch since a long time? Any updates?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] CLK: add more managed APIs

[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 all otehr resources.

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_enable()/devm_clk_disable();
- devm_clk_preapre_enable()/devm_clk_diable_unprepare().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/clk/clk-devres.c |  91 +++++++++++++++++++++++++++++++---------
 include/linux/clk.h      | 105 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 177 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..2703fa9 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -9,6 +9,33 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 
+static int devm_clk_match(struct device *dev, void *res, void *data)
+{
+	struct clk **c = res;
+	if (!c || !*c) {
+		WARN_ON(!c || !*c);
+		return 0;
+	}
+	return *c == data;
+}
+
+
+static int devm_clk_create_devres(struct device *dev,
+				  struct clk *clk,
+				  void (*release)(struct device *, void *))
+{
+	struct clk **ptr;
+
+	ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	*ptr = clk;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+
 static void devm_clk_release(struct device *dev, void *res)
 {
 	clk_put(*(struct clk **)res);
@@ -16,34 +43,22 @@ static void devm_clk_release(struct device *dev, void *res)
 
 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);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct clk *clk;
+	int error;
 
 	clk = clk_get(dev, id);
 	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
+		error = devm_clk_create_devres(dev, clk, devm_clk_release);
+		if (error) {
+			clk_put(clk);
+			return ERR_PTR(error);
+		}
 	}
 
 	return clk;
 }
 EXPORT_SYMBOL(devm_clk_get);
 
-static int devm_clk_match(struct device *dev, void *res, void *data)
-{
-	struct clk **c = res;
-	if (!c || !*c) {
-		WARN_ON(!c || !*c);
-		return 0;
-	}
-	return *c == data;
-}
-
 void devm_clk_put(struct device *dev, struct clk *clk)
 {
 	int ret;
@@ -53,3 +68,41 @@ void devm_clk_put(struct device *dev, struct clk *clk)
 	WARN_ON(ret);
 }
 EXPORT_SYMBOL(devm_clk_put);
+
+#define DEFINE_DEVM_CLK_OP(create_op, destroy_op)			\
+static void devm_##destroy_op##_release(struct device *devm, void *res)	\
+{									\
+	destroy_op(*(struct clk **)res);				\
+}									\
+									\
+int devm_##create_op(struct device *dev, struct clk *clk)		\
+{									\
+	int error;							\
+									\
+	error = devm_clk_create_devres(dev, clk,			\
+					devm_##destroy_op##_release);	\
+	if (error)							\
+		return error;						\
+									\
+	error = create_op(clk);						\
+	if (error) {							\
+		WARN_ON(devres_destroy(dev,				\
+					devm_##destroy_op##_release,	\
+					devm_clk_match, clk));		\
+		return error;						\
+	}								\
+									\
+	return 0;							\
+}									\
+EXPORT_SYMBOL(devm_##create_op);					\
+									\
+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_DEVM_CLK_OP(clk_prepare, clk_unprepare);
+DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare);
+DEFINE_DEVM_CLK_OP(clk_enable, clk_disable);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8bf149e..04b6300 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -133,6 +133,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
 int clk_prepare(struct clk *clk);
 
 /**
+ * devm_clk_prepare - prepare a clock source as 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);
+
+/**
  * clk_unprepare - undo preparation of a clock source
  * @clk: clock source
  *
@@ -144,6 +155,18 @@ int clk_prepare(struct clk *clk);
 void clk_unprepare(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 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
  *
@@ -156,6 +179,19 @@ void clk_unprepare(struct clk *clk);
 int clk_enable(struct clk *clk);
 
 /**
+ * devm_clk_enable - enable the clock source as 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
  *
@@ -172,6 +208,18 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * devm_clk_disable - disable managed clock source resource
+ * @dev: device used to enable the clock
+ * @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);
+
+/**
  * clk_prepare_enable - prepare and enable a clock source
  * @clk: clock source
  *
@@ -182,6 +230,17 @@ void clk_disable(struct clk *clk);
 int clk_prepare_enable(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);
+
+/**
  * clk_disable_unprepare - disable and undo preparation of a clock source
  * @clk: clock source
  *
@@ -192,6 +251,17 @@ int clk_prepare_enable(struct clk *clk);
 void clk_disable_unprepare(struct clk *clk);
 
 /**
+ * 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
@@ -303,29 +373,64 @@ static inline int clk_prepare(struct clk *clk)
 	return 0;
 }
 
+static inline int devm_clk_prepare(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline void clk_unprepare(struct clk *clk)
 {
 	might_sleep();
 }
 
+static inline void devm_clk_unprepare(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
 }
 
+static inline int devm_clk_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline void clk_disable(struct clk *clk) {}
 
+static inline void devm_clk_disable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline int clk_prepare_enable(struct clk *clk)
 {
 	might_sleep();
 	return 0;
 }
 
+static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline void clk_disable_unprepare(struct clk *clk)
 {
 	might_sleep();
 }
 
+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;
-- 
1.7.11.7

Re: [PATCH 3/3] CLK: add more managed APIs

[Viresh Kumar]

On 20 November 2012 14:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 all otehr resources.

s/otehr/other

> This adds the following managed APIs:
>
> - devm_clk_prepare()/devm_clk_unprepare();
> - devm_clk_enable()/devm_clk_disable();
> - devm_clk_preapre_enable()/devm_clk_diable_unprepare().
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/clk/clk-devres.c |  91 +++++++++++++++++++++++++++++++---------
>  include/linux/clk.h      | 105 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 177 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 8f57154..2703fa9 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -9,6 +9,33 @@
>  #include <linux/export.h>
>  #include <linux/gfp.h>
>
> +static int devm_clk_match(struct device *dev, void *res, void *data)
> +{
> +       struct clk **c = res;
> +       if (!c || !*c) {
> +               WARN_ON(!c || !*c);

I know, you just moved this routine higher in the file. But above two
lines look duplicated. Can we just use WARN(); here?

> +               return 0;
> +       }
> +       return *c == data;
> +}
> +
> +
> +static int devm_clk_create_devres(struct device *dev,
> +                                 struct clk *clk,
> +                                 void (*release)(struct device *, void *))

This didn't came in two lines?

> +{

Apart from that, looks fine.

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [RFC/PATCH 0/3] CLK: add more devm_* APIs

[Russell King - ARM Linux]

On Tue, Nov 20, 2012 at 01:22:16AM -0800, Dmitry Torokhov wrote:
> Hi,
> 
> When looking at recent driver conversions to managed resources (devm_*) there
> is no devm_clk_prepare() and similar functions, which forces mixing of 2
> resource management styles (managed/classic) in the same driver, which is not
> great.
> 
> This patch series adds more devm_* managed APIs to the CLK subsystem so that
> driver conversions can be "pure".

So, how do you ensure the correct ordering between clk_unprepare() and
clk_put(), or even clk_disable() and clk_unprepare() ?  I see nothing
here which makes any guarantees as to the ordering of those operations
upon cleanup.

Re: [RFC/PATCH 0/3] CLK: add more devm_* APIs

[Dmitry Torokhov]

On Tue, Nov 20, 2012 at 09:34:45AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 20, 2012 at 01:22:16AM -0800, Dmitry Torokhov wrote:
> > Hi,
> > 
> > When looking at recent driver conversions to managed resources (devm_*) there
> > is no devm_clk_prepare() and similar functions, which forces mixing of 2
> > resource management styles (managed/classic) in the same driver, which is not
> > great.
> > 
> > This patch series adds more devm_* managed APIs to the CLK subsystem so that
> > driver conversions can be "pure".
> 
> So, how do you ensure the correct ordering between clk_unprepare() and
> clk_put(), or even clk_disable() and clk_unprepare() ?  I see nothing
> here which makes any guarantees as to the ordering of those operations
> upon cleanup.

devm_* calls form a stack so if you have

static void xxx_probe()
{
	input = devm_input_allocate_device();

	devm_request_irq();
	...
	devm_clk_prepare()
	...
	devm_clk_enable()
	... 
	input_register_device();
	return 0;
}

and

static int xxx_remove()
{
	return 0;
}

then upon remove we'll execute:

	input_unregister_device();
	devm_clk_disable();
	devm_clk_unprepare();
	devm_free_irq();
	input_free_device();

Thanks.

-- 
Dmitry

[PATCH v2 0/3] CLK: add more devm_* APIs

[Dmitry Torokhov]

Here is the 2nd version of patchset adding more devm_* APIs to CLK subsystem,
hopefully with all issues addressed.

If this is acceptable then I wonder who will carry the patchset? I can
volunteer :) as I have a couple of pending patches that need this
functionality.

Thanks!

-- 
Dmitry

[PATCH v2 1/3] CLK: get rid of HAVE_CLK_PREPARE

[Dmitry Torokhov]

HAVE_CLK_PREPARE is automatically selected by COMMON_CLK and the only
platform that explicitly selects HAVE_CLK_PREPARE is MXS which has been
switched to common clk framework, so we can delete this option now.

As part of this change we move declarations of clk_prepare() and
clk_unprepare() under HAVE_CLK and provide stubs if this option is not
enabled.

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 arch/arm/Kconfig    |  1 -
 drivers/clk/Kconfig |  4 ----
 include/linux/clk.h | 68 +++++++++++++++++++++++++----------------------------
 3 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ade7e92..ae6c1df 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -453,7 +453,6 @@ config ARCH_MXS
 	select CLKSRC_MMIO
 	select COMMON_CLK
 	select GENERIC_CLOCKEVENTS
-	select HAVE_CLK_PREPARE
 	select MULTI_IRQ_HANDLER
 	select PINCTRL
 	select SPARSE_IRQ
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index bace9e9..639ee9d 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -3,15 +3,11 @@ config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
 
-config HAVE_CLK_PREPARE
-	bool
-
 config HAVE_MACH_CLKDEV
 	bool
 
 config COMMON_CLK
 	bool
-	select HAVE_CLK_PREPARE
 	select CLKDEV_LOOKUP
 	---help---
 	  The common clock framework is a single definition of struct
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b3ac22d..f8204c3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -84,42 +84,6 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
 
 #endif
 
-/**
- * clk_prepare - prepare a clock source
- * @clk: clock source
- *
- * This prepares the clock source for use.
- *
- * Must not be called from within atomic context.
- */
-#ifdef CONFIG_HAVE_CLK_PREPARE
-int clk_prepare(struct clk *clk);
-#else
-static inline int clk_prepare(struct clk *clk)
-{
-	might_sleep();
-	return 0;
-}
-#endif
-
-/**
- * clk_unprepare - undo preparation of a clock source
- * @clk: clock source
- *
- * This undoes a previously prepared clock.  The caller must balance
- * the number of prepare and unprepare calls.
- *
- * Must not be called from within atomic context.
- */
-#ifdef CONFIG_HAVE_CLK_PREPARE
-void clk_unprepare(struct clk *clk);
-#else
-static inline void clk_unprepare(struct clk *clk)
-{
-	might_sleep();
-}
-#endif
-
 #ifdef CONFIG_HAVE_CLK
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
@@ -159,6 +123,27 @@ struct clk *clk_get(struct device *dev, const char *id);
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
 /**
+ * clk_prepare - prepare a clock source
+ * @clk: clock source
+ *
+ * This prepares the clock source for use.
+ *
+ * Must not be called from within atomic context.
+ */
+int clk_prepare(struct clk *clk);
+
+/**
+ * clk_unprepare - undo preparation of a clock source
+ * @clk: clock source
+ *
+ * This undoes a previously prepared clock.  The caller must balance
+ * the number of prepare and unprepare calls.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_unprepare(struct clk *clk);
+
+/**
  * clk_enable - inform the system when the clock source should be running.
  * @clk: clock source
  *
@@ -292,6 +277,17 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+static inline int clk_prepare(struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void clk_unprepare(struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
-- 
1.7.11.7

Re: [PATCH v2 1/3] CLK: get rid of HAVE_CLK_PREPARE

[Shawn Guo]

On Wed, Nov 21, 2012 at 09:34:40PM -0800, Dmitry Torokhov wrote:
> HAVE_CLK_PREPARE is automatically selected by COMMON_CLK and the only
> platform that explicitly selects HAVE_CLK_PREPARE is MXS which has been
> switched to common clk framework, so we can delete this option now.
> 
> As part of this change we move declarations of clk_prepare() and
> clk_unprepare() under HAVE_CLK and provide stubs if this option is not
> enabled.
> 
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH v2 1/3] CLK: get rid of HAVE_CLK_PREPARE

[Russell King - ARM Linux]

On Wed, Nov 21, 2012 at 09:34:40PM -0800, Dmitry Torokhov wrote:
> HAVE_CLK_PREPARE is automatically selected by COMMON_CLK and the only
> platform that explicitly selects HAVE_CLK_PREPARE is MXS which has been
> switched to common clk framework, so we can delete this option now.
> 
> As part of this change we move declarations of clk_prepare() and
> clk_unprepare() under HAVE_CLK and provide stubs if this option is not
> enabled.
> 
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

NAK, until some proper investigation about whether this can be removed
has been done.

[PATCH v2 2/3] CLK: uninline clk_prepare_enable() and clk_disable_unprepare()

[Dmitry Torokhov]

We'll need to invoke clk_disable_unprepare() via a pointer in our devm_*
conversion so let's uninline the pair.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/clk/clk.c   | 23 +++++++++++++++++++++++
 include/linux/clk.h | 53 +++++++++++++++++++++++++++++++----------------------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 56e4495e..92bdafd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -557,6 +557,29 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+int clk_prepare_enable(struct clk *clk)
+{
+	int ret;
+
+	ret = clk_prepare(clk);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(clk);
+	if (ret)
+		clk_unprepare(clk);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(clk_prepare_enable);
+
+void clk_disable_unprepare(struct clk *clk)
+{
+	clk_disable(clk);
+	clk_unprepare(clk);
+}
+EXPORT_SYMBOL_GPL(clk_disable_unprepare);
+
 /**
  * __clk_round_rate - round the given rate for a clk
  * @clk: round the rate of this clock
diff --git a/include/linux/clk.h b/include/linux/clk.h
index f8204c3..8bf149e 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -172,6 +172,26 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_prepare_enable - prepare and enable a clock source
+ * @clk: clock source
+ *
+ * This prepares the clock source for use and enables it.
+ *
+ * Must not be called from within atomic context.
+ */
+int clk_prepare_enable(struct clk *clk);
+
+/**
+ * clk_disable_unprepare - disable and undo preparation of a clock source
+ * @clk: clock source
+ *
+ * This disables and undoes a previously prepared clock.
+ *
+ * Must not be called from within atomic context.
+ */
+void clk_disable_unprepare(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
@@ -295,6 +315,17 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline int clk_prepare_enable(struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void clk_disable_unprepare(struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;
@@ -322,28 +353,6 @@ static inline struct clk *clk_get_parent(struct clk *clk)
 
 #endif
 
-/* clk_prepare_enable helps cases using clk_enable in non-atomic context. */
-static inline int clk_prepare_enable(struct clk *clk)
-{
-	int ret;
-
-	ret = clk_prepare(clk);
-	if (ret)
-		return ret;
-	ret = clk_enable(clk);
-	if (ret)
-		clk_unprepare(clk);
-
-	return ret;
-}
-
-/* clk_disable_unprepare helps cases using clk_disable in non-atomic context. */
-static inline void clk_disable_unprepare(struct clk *clk)
-{
-	clk_disable(clk);
-	clk_unprepare(clk);
-}
-
 /**
  * clk_add_alias - add a new clock alias
  * @alias: name for clock alias
-- 
1.7.11.7

[PATCH v2 3/3] CLK: add more managed APIs

[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 all other resources.

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_enable()/devm_clk_disable();
- devm_clk_preapre_enable()/devm_clk_diable_unprepare().

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/clk/clk-devres.c |  90 +++++++++++++++++++++++++++++++---------
 include/linux/clk.h      | 105 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..3a2286b 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -9,6 +9,32 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 
+static int devm_clk_match(struct device *dev, void *res, void *data)
+{
+	struct clk **c = res;
+
+	if (WARN_ON(!c || !*c))
+		return 0;
+
+	return *c == data;
+}
+
+
+static int devm_clk_create_devres(struct device *dev, struct clk *clk,
+				  void (*release)(struct device *, void *))
+{
+	struct clk **ptr;
+
+	ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	*ptr = clk;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+
 static void devm_clk_release(struct device *dev, void *res)
 {
 	clk_put(*(struct clk **)res);
@@ -16,34 +42,22 @@ static void devm_clk_release(struct device *dev, void *res)
 
 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);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct clk *clk;
+	int error;
 
 	clk = clk_get(dev, id);
 	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
+		error = devm_clk_create_devres(dev, clk, devm_clk_release);
+		if (error) {
+			clk_put(clk);
+			return ERR_PTR(error);
+		}
 	}
 
 	return clk;
 }
 EXPORT_SYMBOL(devm_clk_get);
 
-static int devm_clk_match(struct device *dev, void *res, void *data)
-{
-	struct clk **c = res;
-	if (!c || !*c) {
-		WARN_ON(!c || !*c);
-		return 0;
-	}
-	return *c == data;
-}
-
 void devm_clk_put(struct device *dev, struct clk *clk)
 {
 	int ret;
@@ -53,3 +67,41 @@ void devm_clk_put(struct device *dev, struct clk *clk)
 	WARN_ON(ret);
 }
 EXPORT_SYMBOL(devm_clk_put);
+
+#define DEFINE_DEVM_CLK_OP(create_op, destroy_op)			\
+static void devm_##destroy_op##_release(struct device *devm, void *res)	\
+{									\
+	destroy_op(*(struct clk **)res);				\
+}									\
+									\
+int devm_##create_op(struct device *dev, struct clk *clk)		\
+{									\
+	int error;							\
+									\
+	error = devm_clk_create_devres(dev, clk,			\
+					devm_##destroy_op##_release);	\
+	if (error)							\
+		return error;						\
+									\
+	error = create_op(clk);						\
+	if (error) {							\
+		WARN_ON(devres_destroy(dev,				\
+					devm_##destroy_op##_release,	\
+					devm_clk_match, clk));		\
+		return error;						\
+	}								\
+									\
+	return 0;							\
+}									\
+EXPORT_SYMBOL(devm_##create_op);					\
+									\
+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_DEVM_CLK_OP(clk_prepare, clk_unprepare);
+DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare);
+DEFINE_DEVM_CLK_OP(clk_enable, clk_disable);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8bf149e..04b6300 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -133,6 +133,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
 int clk_prepare(struct clk *clk);
 
 /**
+ * devm_clk_prepare - prepare a clock source as 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);
+
+/**
  * clk_unprepare - undo preparation of a clock source
  * @clk: clock source
  *
@@ -144,6 +155,18 @@ int clk_prepare(struct clk *clk);
 void clk_unprepare(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 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
  *
@@ -156,6 +179,19 @@ void clk_unprepare(struct clk *clk);
 int clk_enable(struct clk *clk);
 
 /**
+ * devm_clk_enable - enable the clock source as 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
  *
@@ -172,6 +208,18 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * devm_clk_disable - disable managed clock source resource
+ * @dev: device used to enable the clock
+ * @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);
+
+/**
  * clk_prepare_enable - prepare and enable a clock source
  * @clk: clock source
  *
@@ -182,6 +230,17 @@ void clk_disable(struct clk *clk);
 int clk_prepare_enable(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);
+
+/**
  * clk_disable_unprepare - disable and undo preparation of a clock source
  * @clk: clock source
  *
@@ -192,6 +251,17 @@ int clk_prepare_enable(struct clk *clk);
 void clk_disable_unprepare(struct clk *clk);
 
 /**
+ * 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
@@ -303,29 +373,64 @@ static inline int clk_prepare(struct clk *clk)
 	return 0;
 }
 
+static inline int devm_clk_prepare(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline void clk_unprepare(struct clk *clk)
 {
 	might_sleep();
 }
 
+static inline void devm_clk_unprepare(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
 }
 
+static inline int devm_clk_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline void clk_disable(struct clk *clk) {}
 
+static inline void devm_clk_disable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline int clk_prepare_enable(struct clk *clk)
 {
 	might_sleep();
 	return 0;
 }
 
+static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
 static inline void clk_disable_unprepare(struct clk *clk)
 {
 	might_sleep();
 }
 
+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;
-- 
1.7.11.7

Re: [v2,3/3] CLK: add more managed APIs

[Guenter Roeck]

On Wed, Nov 21, 2012 at 09:34:42PM -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 all other resources.
> 
> This adds the following managed APIs:
> 
> - devm_clk_prepare()/devm_clk_unprepare();
> - devm_clk_enable()/devm_clk_disable();
> - devm_clk_preapre_enable()/devm_clk_diable_unprepare().

s/devm_clk_preapre_enable/devm_clk_prepare_enable/
s//devm_clk_diable_unprepare//devm_clk_disable_unprepare/
> 
> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

What happened with this patch ? I find it highly inconventient having to add
devm_add_action_or_reset() for pretty much every call to clk_prepare_enable().

Another odd one is that there is a devm_clk_get(), but no devm_of_clk_get()
or devm_of_clk_get_by_name().

Thanks,
Guenter

> ---
>  drivers/clk/clk-devres.c |  90 +++++++++++++++++++++++++++++++---------
>  include/linux/clk.h      | 105 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 176 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 8f57154..3a2286b 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -9,6 +9,32 @@
>  #include <linux/export.h>
>  #include <linux/gfp.h>
>  
> +static int devm_clk_match(struct device *dev, void *res, void *data)
> +{
> +	struct clk **c = res;
> +
> +	if (WARN_ON(!c || !*c))
> +		return 0;
> +
> +	return *c == data;
> +}
> +
> +
> +static int devm_clk_create_devres(struct device *dev, struct clk *clk,
> +				  void (*release)(struct device *, void *))
> +{
> +	struct clk **ptr;
> +
> +	ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	*ptr = clk;
> +	devres_add(dev, ptr);
> +
> +	return 0;
> +}
> +
>  static void devm_clk_release(struct device *dev, void *res)
>  {
>  	clk_put(*(struct clk **)res);
> @@ -16,34 +42,22 @@ static void devm_clk_release(struct device *dev, void *res)
>  
>  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);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct clk *clk;
> +	int error;
>  
>  	clk = clk_get(dev, id);
>  	if (!IS_ERR(clk)) {
> -		*ptr = clk;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> +		error = devm_clk_create_devres(dev, clk, devm_clk_release);
> +		if (error) {
> +			clk_put(clk);
> +			return ERR_PTR(error);
> +		}
>  	}
>  
>  	return clk;
>  }
>  EXPORT_SYMBOL(devm_clk_get);
>  
> -static int devm_clk_match(struct device *dev, void *res, void *data)
> -{
> -	struct clk **c = res;
> -	if (!c || !*c) {
> -		WARN_ON(!c || !*c);
> -		return 0;
> -	}
> -	return *c == data;
> -}
> -
>  void devm_clk_put(struct device *dev, struct clk *clk)
>  {
>  	int ret;
> @@ -53,3 +67,41 @@ void devm_clk_put(struct device *dev, struct clk *clk)
>  	WARN_ON(ret);
>  }
>  EXPORT_SYMBOL(devm_clk_put);
> +
> +#define DEFINE_DEVM_CLK_OP(create_op, destroy_op)			\
> +static void devm_##destroy_op##_release(struct device *devm, void *res)	\
> +{									\
> +	destroy_op(*(struct clk **)res);				\
> +}									\
> +									\
> +int devm_##create_op(struct device *dev, struct clk *clk)		\
> +{									\
> +	int error;							\
> +									\
> +	error = devm_clk_create_devres(dev, clk,			\
> +					devm_##destroy_op##_release);	\
> +	if (error)							\
> +		return error;						\
> +									\
> +	error = create_op(clk);						\
> +	if (error) {							\
> +		WARN_ON(devres_destroy(dev,				\
> +					devm_##destroy_op##_release,	\
> +					devm_clk_match, clk));		\
> +		return error;						\
> +	}								\
> +									\
> +	return 0;							\
> +}									\
> +EXPORT_SYMBOL(devm_##create_op);					\
> +									\
> +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_DEVM_CLK_OP(clk_prepare, clk_unprepare);
> +DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare);
> +DEFINE_DEVM_CLK_OP(clk_enable, clk_disable);
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 8bf149e..04b6300 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -133,6 +133,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id);
>  int clk_prepare(struct clk *clk);
>  
>  /**
> + * devm_clk_prepare - prepare a clock source as 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);
> +
> +/**
>   * clk_unprepare - undo preparation of a clock source
>   * @clk: clock source
>   *
> @@ -144,6 +155,18 @@ int clk_prepare(struct clk *clk);
>  void clk_unprepare(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 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
>   *
> @@ -156,6 +179,19 @@ void clk_unprepare(struct clk *clk);
>  int clk_enable(struct clk *clk);
>  
>  /**
> + * devm_clk_enable - enable the clock source as 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
>   *
> @@ -172,6 +208,18 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * devm_clk_disable - disable managed clock source resource
> + * @dev: device used to enable the clock
> + * @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);
> +
> +/**
>   * clk_prepare_enable - prepare and enable a clock source
>   * @clk: clock source
>   *
> @@ -182,6 +230,17 @@ void clk_disable(struct clk *clk);
>  int clk_prepare_enable(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);
> +
> +/**
>   * clk_disable_unprepare - disable and undo preparation of a clock source
>   * @clk: clock source
>   *
> @@ -192,6 +251,17 @@ int clk_prepare_enable(struct clk *clk);
>  void clk_disable_unprepare(struct clk *clk);
>  
>  /**
> + * 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
> @@ -303,29 +373,64 @@ static inline int clk_prepare(struct clk *clk)
>  	return 0;
>  }
>  
> +static inline int devm_clk_prepare(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +	return 0;
> +}
> +
>  static inline void clk_unprepare(struct clk *clk)
>  {
>  	might_sleep();
>  }
>  
> +static inline void devm_clk_unprepare(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +}
> +
>  static inline int clk_enable(struct clk *clk)
>  {
>  	return 0;
>  }
>  
> +static inline int devm_clk_enable(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +	return 0;
> +}
> +
>  static inline void clk_disable(struct clk *clk) {}
>  
> +static inline void devm_clk_disable(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +	return 0;
> +}
> +
>  static inline int clk_prepare_enable(struct clk *clk)
>  {
>  	might_sleep();
>  	return 0;
>  }
>  
> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +	return 0;
> +}
> +
>  static inline void clk_disable_unprepare(struct clk *clk)
>  {
>  	might_sleep();
>  }
>  
> +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;

Re: [PATCH v2 0/3] CLK: add more devm_* APIs

[Viresh Kumar]

On 22 November 2012 11:04, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Here is the 2nd version of patchset adding more devm_* APIs to CLK subsystem,
> hopefully with all issues addressed.
>
> If this is acceptable then I wonder who will carry the patchset? I can
> volunteer :) as I have a couple of pending patches that need this
> functionality.

Looks good.

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>