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
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:
quoted
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
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] 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: ?
From: Mike Turquette <hidden> Date: 2012-11-21 20:43:44
Quoting Viresh Kumar (2012-11-20 02:13:55)
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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
On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
Quoting Viresh Kumar (2012-11-20 02:13:55)
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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
From: Russell King - ARM Linux <hidden> Date: 2012-11-22 22:31:53
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:
quoted
Quoting Viresh Kumar (2012-11-20 02:13:55)
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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.
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:
quoted
On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
quoted
Quoting Viresh Kumar (2012-11-20 02:13:55)
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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.
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:
quoted
On Wed, Nov 21, 2012 at 12:54:24PM -0800, Dmitry Torokhov wrote:
quoted
On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
quoted
Quoting Viresh Kumar (2012-11-20 02:13:55)
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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.
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(-)
From: Mike Turquette <hidden> Date: 2012-11-22 18:57:58
Quoting Dmitry Torokhov (2012-11-21 19:11:17)
On Wed, Nov 21, 2012 at 06:17:50PM -0800, Dmitry Torokhov wrote:
quoted
On Wed, Nov 21, 2012 at 10:38:59PM +0000, Russell King - ARM Linux wrote:
quoted
On Wed, Nov 21, 2012 at 12:54:24PM -0800, Dmitry Torokhov wrote:
quoted
On Wed, Nov 21, 2012 at 12:43:24PM -0800, Mike Turquette wrote:
quoted
Quoting Viresh Kumar (2012-11-20 02:13:55)
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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.
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
On 22 November 2012 08:41, Dmitry Torokhov [off-list ref] 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>
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.
Hi Russell,
On 22 November 2012 15:00, Russell King - ARM Linux
[off-list ref] 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
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.
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
On 23 November 2012 12:49, Dmitry Torokhov [off-list ref] 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 [off-list ref] wrote:
quoted
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.
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:
quoted
On 23 November 2012 12:49, Dmitry Torokhov [off-list ref] wrote:
quoted
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
On 22 November 2012 02:13, Mike Turquette [off-list ref] 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
From: Mike Turquette <hidden> Date: 2012-11-22 19:05:05
Quoting Viresh Kumar (2012-11-21 19:34:18)
On 22 November 2012 02:13, Mike Turquette [off-list ref] wrote:
quoted
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
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(-)
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] 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.
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] 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>
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] 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>
[Adding Linaro id of Mike]
On 16 December 2012 17:10, Viresh Kumar [off-list ref] wrote:
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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>
From: Russell King - ARM Linux <hidden> Date: 2012-12-16 11:58:14
On Sun, Dec 16, 2012 at 05:10:36PM +0530, Viresh Kumar wrote:
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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?
On 16 December 2012 17:27, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Dec 16, 2012 at 05:10:36PM +0530, Viresh Kumar wrote:
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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
From: Russell King - ARM Linux <hidden> Date: 2012-12-16 12:40:44
On Sun, Dec 16, 2012 at 05:50:44PM +0530, Viresh Kumar wrote:
On 16 December 2012 17:27, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Sun, Dec 16, 2012 at 05:10:36PM +0530, Viresh Kumar wrote:
quoted
On 20 November 2012 14:52, Dmitry Torokhov [off-list ref] wrote:
quoted
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?
On 16 December 2012 18:10, Russell King - ARM Linux
[off-list ref] 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
From: Russell King - ARM Linux <hidden> Date: 2012-12-16 13:10:04
On Sun, Dec 16, 2012 at 06:35:24PM +0530, Viresh Kumar wrote:
On 16 December 2012 18:10, Russell King - ARM Linux
[off-list ref] wrote:
quoted
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?
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
[off-list ref] wrote:
quoted
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
From: Andy Shevchenko <hidden> Date: 2014-01-13 14:06:24
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 [off-list ref] wrote:
On 17 December 2012 11:12, Dmitry Torokhov [off-list ref] wrote:
quoted
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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Russell King - ARM Linux <hidden> Date: 2012-11-20 09:37:00
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.
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:
quoted
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
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
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(-)
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().
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
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(-)
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>
From: Russell King - ARM Linux <hidden> Date: 2012-11-22 22:32:52
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.
On 22 November 2012 11:04, Dmitry Torokhov [off-list ref] 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.