| Message ID | 1353403339-11679-2-git-send-email-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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 >
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 > >
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.
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?
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.
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
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
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.
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.
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
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.
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
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.
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
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;
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(-)