Patchwork mpc512x/clock: fix clk_get logic

login
register
mail settings
Submitter Wolfram Sang
Date Oct. 30, 2009, 9:17 a.m.
Message ID <1256894234-11264-1-git-send-email-w.sang@pengutronix.de>
Download mbox | patch
Permalink /patch/37264/
State Changes Requested
Delegated to: Grant Likely
Headers show

Comments

Wolfram Sang - Oct. 30, 2009, 9:17 a.m.
The matching logic returns a clock even if only the dev-part matches. This is
wrong as devices may utilize more than one clock, so the wrong clock may be
returned due to dev being not unique (noticed while working on the CAN driver).
The proposed new method will:

- require the id field (as _this_ is the unique identifier)
- dev need not be given; if NULL, it will match any device.
  if given, it has to match the dev of the clock
- using the above rules, both fields need to match in order to claim the clock

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 arch/powerpc/platforms/512x/clock.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
Mark Brown - Oct. 30, 2009, 10:54 a.m.
On Fri, Oct 30, 2009 at 10:17:14AM +0100, Wolfram Sang wrote:
> The matching logic returns a clock even if only the dev-part matches. This is
> wrong as devices may utilize more than one clock, so the wrong clock may be
> returned due to dev being not unique (noticed while working on the CAN driver).
> The proposed new method will:

> - require the id field (as _this_ is the unique identifier)

NULL id fields are supposed to be supported in the cannonical clkdev
API, unfortunately.
Wolfram Sang - Oct. 30, 2009, 11:36 a.m.
On Fri, Oct 30, 2009 at 10:54:14AM +0000, Mark Brown wrote:
> On Fri, Oct 30, 2009 at 10:17:14AM +0100, Wolfram Sang wrote:
> > The matching logic returns a clock even if only the dev-part matches. This is
> > wrong as devices may utilize more than one clock, so the wrong clock may be
> > returned due to dev being not unique (noticed while working on the CAN driver).
> > The proposed new method will:
> 
> > - require the id field (as _this_ is the unique identifier)
> 
> NULL id fields are supposed to be supported in the cannonical clkdev
> API, unfortunately.

Hmm, ok, thanks for the hint. Where is this documented, I can't find it?
Mark Brown - Oct. 30, 2009, 12:02 p.m.
On Fri, Oct 30, 2009 at 12:36:44PM +0100, Wolfram Sang wrote:
> On Fri, Oct 30, 2009 at 10:54:14AM +0000, Mark Brown wrote:

> > > - require the id field (as _this_ is the unique identifier)

> > NULL id fields are supposed to be supported in the cannonical clkdev
> > API, unfortunately.

> Hmm, ok, thanks for the hint. Where is this documented, I can't find it?

I'm not aware of any particular documentation on it but searching for
mailing list posts from rmk ought to turn up some posts on the issue.
Benjamin Herrenschmidt - Oct. 30, 2009, 9:54 p.m.
On Fri, 2009-10-30 at 10:17 +0100, Wolfram Sang wrote:
> The matching logic returns a clock even if only the dev-part matches. This is
> wrong as devices may utilize more than one clock, so the wrong clock may be
> returned due to dev being not unique (noticed while working on the CAN driver).
> The proposed new method will:
> 
> - require the id field (as _this_ is the unique identifier)
> - dev need not be given; if NULL, it will match any device.
>   if given, it has to match the dev of the clock
> - using the above rules, both fields need to match in order to claim the clock

Have you considered switching to my proposed device-tree based clock
reprentation ?

Cheers,
Ben.

> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
>  arch/powerpc/platforms/512x/clock.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> Index: .kernel/arch/powerpc/platforms/512x/clock.c
> ===================================================================
> --- .kernel.orig/arch/powerpc/platforms/512x/clock.c
> +++ .kernel/arch/powerpc/platforms/512x/clock.c
> @@ -53,19 +53,21 @@ static DEFINE_MUTEX(clocks_mutex);
>  static struct clk *mpc5121_clk_get(struct device *dev, const char *id)
>  {
>  	struct clk *p, *clk = ERR_PTR(-ENOENT);
> -	int dev_match = 0;
> -	int id_match = 0;
> +	bool id_match = false;
> +	/* Match any device if no dev given */
> +	bool dev_match = !dev;
>  
> -	if (dev == NULL || id == NULL)
> +	/* We need the unique identifier */
> +	if (id == NULL)
>  		return NULL;
>  
>  	mutex_lock(&clocks_mutex);
>  	list_for_each_entry(p, &clocks, node) {
>  		if (dev == p->dev)
> -			dev_match++;
> +			dev_match = true;
>  		if (strcmp(id, p->name) == 0)
> -			id_match++;
> -		if ((dev_match || id_match) && try_module_get(p->owner)) {
> +			id_match = true;
> +		if (dev_match && id_match && try_module_get(p->owner)) {
>  			clk = p;
>  			break;
>  		}
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
Wolfram Sang - Oct. 31, 2009, 1:01 p.m.
Hello Ben,

> Have you considered switching to my proposed device-tree based clock
> reprentation ?

You mean this one?

http://patchwork.ozlabs.org/patch/31551/

Sorry, I have missed it up to now :(

While I think (after one glimpse) this moves things into the right direction,
my priority at the moment is to get the mscan driver working on 512x (and then
to mainline). For that, I needed the patch I posted. I hope I can have another
look at your proposal later on, it looks really worth it.

Until then I'd propose to consider this patch as it fixes a bug in clock
assignment. But I leave the final decision to you maintainers, of course.

Regards,

   Wolfram
Benjamin Herrenschmidt - Oct. 31, 2009, 8:48 p.m.
On Sat, 2009-10-31 at 14:01 +0100, Wolfram Sang wrote:
> Hello Ben,
> 
> > Have you considered switching to my proposed device-tree based clock
> > reprentation ?
> 
> You mean this one?
> 
> http://patchwork.ozlabs.org/patch/31551/
> 
> Sorry, I have missed it up to now :(
> 
> While I think (after one glimpse) this moves things into the right direction,
> my priority at the moment is to get the mscan driver working on 512x (and then
> to mainline). For that, I needed the patch I posted. I hope I can have another
> look at your proposal later on, it looks really worth it.
> 
> Until then I'd propose to consider this patch as it fixes a bug in clock
> assignment. But I leave the final decision to you maintainers, of course.

Sure I don't have major objections against your patch (though who is
formally mpc512x maintainer to ack it ?), I just wanted to make sure you
had a look at my proposal since this is almost the only platform to use
the clock API today in arch/powerpc.

Cheers,
Ben.
Wolfram Sang - Nov. 2, 2009, 2:22 p.m.
> Sure I don't have major objections against your patch (though who is
> formally mpc512x maintainer to ack it ?),

Grant is maintainer for MPC5xxx.
Grant Likely - Nov. 2, 2009, 4:37 p.m.
On Mon, Nov 2, 2009 at 7:22 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> Sure I don't have major objections against your patch (though who is
>> formally mpc512x maintainer to ack it ?),
>
> Grant is maintainer for MPC5xxx.

Is this patch intended for mainline?  I've received so many 5121
patches that only apply against a Denx tree that I'm starting to
ignore them.  Until the bulk of the out-of-tree 5121 support is
merged, it would be helpful to specifically mention in the patch
description whether or not it should be merged upstream.

Cheers,
g.
Wolfram Sang - Nov. 2, 2009, 5:18 p.m.
> Is this patch intended for mainline?  I've received so many 5121

Yes, my branch is based on linus-git as of today. The CAN driver addition was
just posted to socketcan-devel. It is also independent of the denx-patches[1]
and will later be send to netdev, too.

[1] Of course, the FEC patches are still very handy to bring up a board ;)

Patch

Index: .kernel/arch/powerpc/platforms/512x/clock.c
===================================================================
--- .kernel.orig/arch/powerpc/platforms/512x/clock.c
+++ .kernel/arch/powerpc/platforms/512x/clock.c
@@ -53,19 +53,21 @@  static DEFINE_MUTEX(clocks_mutex);
 static struct clk *mpc5121_clk_get(struct device *dev, const char *id)
 {
 	struct clk *p, *clk = ERR_PTR(-ENOENT);
-	int dev_match = 0;
-	int id_match = 0;
+	bool id_match = false;
+	/* Match any device if no dev given */
+	bool dev_match = !dev;
 
-	if (dev == NULL || id == NULL)
+	/* We need the unique identifier */
+	if (id == NULL)
 		return NULL;
 
 	mutex_lock(&clocks_mutex);
 	list_for_each_entry(p, &clocks, node) {
 		if (dev == p->dev)
-			dev_match++;
+			dev_match = true;
 		if (strcmp(id, p->name) == 0)
-			id_match++;
-		if ((dev_match || id_match) && try_module_get(p->owner)) {
+			id_match = true;
+		if (dev_match && id_match && try_module_get(p->owner)) {
 			clk = p;
 			break;
 		}