mpc512x/clock: fix clk_get logic

mpc512x/clock: fix clk_get logic

[Wolfram Sang]

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

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

Re: mpc512x/clock: fix clk_get logic

[Mark Brown]

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.

Re: mpc512x/clock: fix clk_get logic

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

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?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: mpc512x/clock: fix clk_get logic

[Mark Brown]

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.

[PATCH V2] mpc512x/clock: fix clk_get logic

[Wolfram Sang]

The current matching logic returns a clock even if only one out of two
arguments matches. This is wrong as devices may utilize more than one clock, so
only the first clock out of those is returned if the dev-match alone is
considered sufficent (noticed while working on the CAN driver). The proposed
new method will:

- return -EINVAL if both arguments are NULL
- skip the relevant check if one argument is NULL (first match wins)
- otherwise both arguments need to match

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Mark Brown <broonie@sirena.org.uk>
Cc: Roel Kluin <roel.kluin@gmail.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
---

After Mark's valid comment, I'll try harder ;)

 arch/powerpc/platforms/512x/clock.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/512x/clock.c b/arch/powerpc/platforms/512x/clock.c
index 84544d0..4168457 100644
--- a/arch/powerpc/platforms/512x/clock.c
+++ b/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;
+	/* If one argument is not given, skip its match */
+	bool id_matched = !id;
+	bool dev_matched = !dev;
 
-	if (dev == NULL || id == NULL)
-		return NULL;
+	/* We need at least one argument */
+	if (!dev && !id)
+		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&clocks_mutex);
 	list_for_each_entry(p, &clocks, node) {
 		if (dev == p->dev)
-			dev_match++;
-		if (strcmp(id, p->name) == 0)
-			id_match++;
-		if ((dev_match || id_match) && try_module_get(p->owner)) {
+			dev_matched = true;
+		if (id && strcmp(id, p->name) == 0)
+			id_matched = true;
+		if (dev_matched && id_matched && try_module_get(p->owner)) {
 			clk = p;
 			break;
 		}
-- 
1.6.3.3

Re: mpc512x/clock: fix clk_get logic

[Benjamin Herrenschmidt]

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

Re: mpc512x/clock: fix clk_get logic

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: mpc512x/clock: fix clk_get logic

[Benjamin Herrenschmidt]

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.

Re: mpc512x/clock: fix clk_get logic

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

> Sure I don't have major objections against your patch (though who is
> formally mpc512x maintainer to ack it ?),

Grant is maintainer for MPC5xxx.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] mpc512x/clocks: initialize CAN clocks

[Wolfram Sang]

Signed-off-by: John Rigby <jrigby@freescale.com>
Signed-off-by: Chen Hongjun <hong-jun.chen@freescale.com>
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
---

Should come after the fix for clk_get to be usable for the upcoming CAN driver:
http://patchwork.ozlabs.org/patch/37342/

 arch/powerpc/platforms/512x/clock.c |   74 +++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/512x/clock.c b/arch/powerpc/platforms/512x/clock.c
index 4168457..2d3a5ef 100644
--- a/arch/powerpc/platforms/512x/clock.c
+++ b/arch/powerpc/platforms/512x/clock.c
@@ -50,6 +50,8 @@ struct clk {
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
+struct clk mscan_clks[4];
+
 static struct clk *mpc5121_clk_get(struct device *dev, const char *id)
 {
 	struct clk *p, *clk = ERR_PTR(-ENOENT);
@@ -119,6 +121,8 @@ struct mpc512x_clockctl {
 	u32 spccr;		/* SPDIF Clk Ctrl Reg */
 	u32 cccr;		/* CFM Clk Ctrl Reg */
 	u32 dccr;		/* DIU Clk Cnfg Reg */
+	/* rev2+ only regs */
+	u32 mccr[4];		/* MSCAN Clk Ctrl Reg 1-4 */
 };
 
 struct mpc512x_clockctl __iomem *clockctl;
@@ -688,6 +692,72 @@ static void psc_clks_init(void)
 	}
 }
 
+
+/*
+ * mscan clock rate calculation
+ */
+static unsigned long mscan_calc_rate(struct device_node *np, int mscannum)
+{
+	unsigned long mscanclk_src, mscanclk_div;
+	u32 *mccr = &clockctl->mccr[mscannum];
+
+	/*
+	 * If the divider is the reset default of all 1's then
+	 * we know u-boot and/or board setup has not
+	 * done anything so set up a sane default
+	 */
+	if (((in_be32(mccr) >> 17) & 0x7fff) == 0x7fff) {
+		/* disable */
+		out_be32(mccr, 0);
+		/* src is sysclk, divider is 4 */
+		out_be32(mccr, (0x3 << 17) | 0x10000);
+	}
+
+	switch ((in_be32(mccr) >> 14) & 0x3) {
+	case 0:
+		mscanclk_src = sys_clk.rate;
+		break;
+	case 1:
+		mscanclk_src = ref_clk.rate;
+		break;
+	case 2:
+		mscanclk_src = psc_mclk_in.rate;
+		break;
+	case 3:
+		mscanclk_src = spdif_txclk.rate;
+		break;
+	}
+
+	mscanclk_src = roundup(mscanclk_src, 1000000);
+	mscanclk_div = ((in_be32(mccr) >> 17) & 0x7fff) + 1;
+	return mscanclk_src / mscanclk_div;
+}
+
+/*
+ * Find all silicon rev2 mscan nodes in device tree and assign a clock
+ * with name "mscan%d_clk" and dev pointing at the device
+ * returned from of_find_device_by_node
+ */
+static void mscan_clks_init(void)
+{
+	struct device_node *np;
+	struct of_device *ofdev;
+	const u32 *cell_index;
+
+	for_each_compatible_node(np, NULL, "fsl,mpc5121-mscan") {
+		cell_index = of_get_property(np, "cell-index", NULL);
+		if (cell_index) {
+			struct clk *clk = &mscan_clks[*cell_index];
+			clk->flags = CLK_HAS_RATE;
+			ofdev = of_find_device_by_node(np);
+			clk->dev = &ofdev->dev;
+			clk->rate = mscan_calc_rate(np, *cell_index);
+			sprintf(clk->name, "mscan%d_clk", *cell_index);
+			clk_register(clk);
+		}
+	}
+}
+
 static struct clk_interface mpc5121_clk_functions = {
 	.clk_get		= mpc5121_clk_get,
 	.clk_enable		= mpc5121_clk_enable,
@@ -719,6 +789,10 @@ mpc5121_clk_init(void)
 	rate_clks_init();
 	psc_clks_init();
 
+	/* Setup per-controller CAN clocks for Rev2 and later */
+	if (((mfspr(SPRN_SVR) >> 4) & 0xF) > 1)
+		mscan_clks_init();
+
 	/* leave clockctl mapped forever */
 	/*iounmap(clockctl); */
 	DEBUG_CLK_DUMP();
-- 
1.6.3.3

Re: mpc512x/clock: fix clk_get logic

[Grant Likely]

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: mpc512x/clock: fix clk_get logic

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH V2] mpc512x/clock: fix clk_get logic

[Grant Likely]

On Fri, Oct 30, 2009 at 10:53 AM, Wolfram Sang <w.sang@pengutronix.de> wrot=
e:
> + =A0 =A0 =A0 bool id_matched =3D !id;
> + =A0 =A0 =A0 bool dev_matched =3D !dev;
[...]
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_matched =3D true;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (id && strcmp(id, p->name) =3D=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 id_matched =3D true;

Using bool/true/false doesn't seem to be a common pattern in the
kernel.  Anyone know what the winds of prevailing opinion are
regarding 'bool' in kernel code?

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] mpc512x/clocks: initialize CAN clocks

[Grant Likely]

Hi Wolfram,

Comments below

On Mon, Nov 2, 2009 at 8:17 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Signed-off-by: John Rigby <jrigby@freescale.com>
> Signed-off-by: Chen Hongjun <hong-jun.chen@freescale.com>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> Should come after the fix for clk_get to be usable for the upcoming CAN d=
river:
> http://patchwork.ozlabs.org/patch/37342/
>
> =A0arch/powerpc/platforms/512x/clock.c | =A0 74 +++++++++++++++++++++++++=
++++++++++
> =A01 files changed, 74 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/platforms/512x/clock.c b/arch/powerpc/platforms=
/512x/clock.c
> index 4168457..2d3a5ef 100644
> --- a/arch/powerpc/platforms/512x/clock.c
> +++ b/arch/powerpc/platforms/512x/clock.c
> @@ -50,6 +50,8 @@ struct clk {
> =A0static LIST_HEAD(clocks);
> =A0static DEFINE_MUTEX(clocks_mutex);
>
> +struct clk mscan_clks[4];
> +

I'd rather not have more globals.  If really needed, should at the
very least be static and prefixed with mpc5121_.

> =A0static struct clk *mpc5121_clk_get(struct device *dev, const char *id)
> =A0{
> =A0 =A0 =A0 =A0struct clk *p, *clk =3D ERR_PTR(-ENOENT);
> @@ -119,6 +121,8 @@ struct mpc512x_clockctl {
> =A0 =A0 =A0 =A0u32 spccr; =A0 =A0 =A0 =A0 =A0 =A0 =A0/* SPDIF Clk Ctrl Re=
g */
> =A0 =A0 =A0 =A0u32 cccr; =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* CFM Clk Ctrl Reg =
*/
> =A0 =A0 =A0 =A0u32 dccr; =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* DIU Clk Cnfg Reg =
*/
> + =A0 =A0 =A0 /* rev2+ only regs */
> + =A0 =A0 =A0 u32 mccr[4]; =A0 =A0 =A0 =A0 =A0 =A0/* MSCAN Clk Ctrl Reg 1=
-4 */
> =A0};
>
> =A0struct mpc512x_clockctl __iomem *clockctl;
> @@ -688,6 +692,72 @@ static void psc_clks_init(void)
> =A0 =A0 =A0 =A0}
> =A0}
>
> +
> +/*
> + * mscan clock rate calculation
> + */
> +static unsigned long mscan_calc_rate(struct device_node *np, int mscannu=
m)
> +{
> + =A0 =A0 =A0 unsigned long mscanclk_src, mscanclk_div;
> + =A0 =A0 =A0 u32 *mccr =3D &clockctl->mccr[mscannum];
> +
> + =A0 =A0 =A0 /*
> + =A0 =A0 =A0 =A0* If the divider is the reset default of all 1's then
> + =A0 =A0 =A0 =A0* we know u-boot and/or board setup has not
> + =A0 =A0 =A0 =A0* done anything so set up a sane default
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 if (((in_be32(mccr) >> 17) & 0x7fff) =3D=3D 0x7fff) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* disable */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(mccr, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* src is sysclk, divider is 4 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(mccr, (0x3 << 17) | 0x10000);
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 switch ((in_be32(mccr) >> 14) & 0x3) {
> + =A0 =A0 =A0 case 0:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mscanclk_src =3D sys_clk.rate;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 case 1:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mscanclk_src =3D ref_clk.rate;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 case 2:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mscanclk_src =3D psc_mclk_in.rate;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 case 3:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mscanclk_src =3D spdif_txclk.rate;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 }

Nit: Table lookup perhaps?

> +
> + =A0 =A0 =A0 mscanclk_src =3D roundup(mscanclk_src, 1000000);
> + =A0 =A0 =A0 mscanclk_div =3D ((in_be32(mccr) >> 17) & 0x7fff) + 1;
> + =A0 =A0 =A0 return mscanclk_src / mscanclk_div;
> +}
> +
> +/*
> + * Find all silicon rev2 mscan nodes in device tree and assign a clock
> + * with name "mscan%d_clk" and dev pointing at the device
> + * returned from of_find_device_by_node
> + */

Comment block doesn't really help me understand what the function does.

> +static void mscan_clks_init(void)
> +{
> + =A0 =A0 =A0 struct device_node *np;
> + =A0 =A0 =A0 struct of_device *ofdev;
> + =A0 =A0 =A0 const u32 *cell_index;
> +
> + =A0 =A0 =A0 for_each_compatible_node(np, NULL, "fsl,mpc5121-mscan") {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cell_index =3D of_get_property(np, "cell-in=
dex", NULL);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (cell_index) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct clk *clk =3D &mscan_=
clks[*cell_index];
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk->flags =3D CLK_HAS_RATE=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ofdev =3D of_find_device_by=
_node(np);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk->dev =3D &ofdev->dev;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk->rate =3D mscan_calc_ra=
te(np, *cell_index);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sprintf(clk->name, "mscan%d=
_clk", *cell_index);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 clk_register(clk);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }
> +}

These clock controllers are 1:1 dedicated to the CAN devices, correct?
 Wouldn't it make more sense to put this code directly into the CAN
bus device driver instead of in common code?  And allocated the clk
structure at driver probe time?  It seems like the only shared bit
seems to be access to the mccr registers.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] mpc512x/clocks: initialize CAN clocks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]


> These clock controllers are 1:1 dedicated to the CAN devices, correct?

Yes.

>  Wouldn't it make more sense to put this code directly into the CAN
> bus device driver instead of in common code?  And allocated the clk
> structure at driver probe time?

Yes, just...

> It seems like the only shared bit seems to be access to the mccr registers.

...we can't access registers from two drivers?? Ah, wait, you are probably
aiming at moving the mscan_init-function to the can-driver and to expose the
mscan_calc_rate function from here? That sounds good in deed, will update!

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] mpc512x/clocks: initialize CAN clocks

[Grant Likely]

On Mon, Nov 2, 2009 at 11:40 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> It seems like the only shared bit seems to be access to the mccr registers.
>
> ...we can't access registers from two drivers?? Ah, wait, you are probably
> aiming at moving the mscan_init-function to the can-driver and to expose the
> mscan_calc_rate function from here? That sounds good in deed, will update!

Yup, that is exactly my thinking.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH V2] mpc512x/clock: fix clk_get logic

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

Hi Grant,

On Mon, 2 Nov 2009 10:48:58 -0700 Grant Likely <grant.likely@secretlab.ca> wrote:
>
> Using bool/true/false doesn't seem to be a common pattern in the
> kernel.  Anyone know what the winds of prevailing opinion are
> regarding 'bool' in kernel code?

Its a good thing.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH V2] mpc512x/clock: fix clk_get logic

[Grant Likely]

On Mon, Nov 2, 2009 at 4:10 PM, Stephen Rothwell <sfr@canb.auug.org.au> wro=
te:
> Hi Grant,
>
> On Mon, 2 Nov 2009 10:48:58 -0700 Grant Likely <grant.likely@secretlab.ca=
> wrote:
>>
>> Using bool/true/false doesn't seem to be a common pattern in the
>> kernel. =A0Anyone know what the winds of prevailing opinion are
>> regarding 'bool' in kernel code?
>
> Its a good thing.

Thanks,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.