[PATCH] powerpc: Add of_register_i2c_devices()

[PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

Add of_register_i2c_devices(), which scans the children of the specified
I2C adapter node, and registers them with the I2C code.

Signed-off-by: Scott Wood <scottwood@freescale.com>
Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/prom_parse.c
index 3786dcc..9caf96d 100644
--- a/arch/powerpc/kernel/prom_parse.c
+++ b/arch/powerpc/kernel/prom_parse.c
@@ -1067,3 +1067,49 @@ void __iomem *of_iomap(struct device_node *np, int index)
 	return ioremap(res.start, 1 + res.end - res.start);
 }
 EXPORT_SYMBOL(of_iomap);
+
+#ifdef CONFIG_I2C
+#include <linux/i2c.h>
+
+void of_register_i2c_devices(struct device_node *adap_node, int bus_num)
+{
+	struct device_node *node = NULL;
+
+	while ((node = of_get_next_child(adap_node, node))) {
+		struct i2c_board_info info;
+		const u32 *addr;
+		const char *name, *model;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > 0xffff)
+			continue;
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		name = of_get_property(node, "compatible", NULL);
+		if (!name)
+			name = node->name;
+		if (!name)
+			continue;
+
+		model = of_get_property(node, "model", NULL);
+		if (!model)
+			model = name;
+
+		/* FIXME: the i2c code should allow drivers to specify
+		 * multiple match names; board code shouldn't need to
+		 * know what driver will handle a given type.
+		 */
+
+		snprintf(info.driver_name, KOBJ_NAME_LEN, name);
+		snprintf(info.type, KOBJ_NAME_LEN, model);
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		i2c_register_board_info(bus_num, &info, 1);
+	}
+}
+#endif /* CONFIG_I2C */
diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
index 6845af9..ed085e1 100644
--- a/include/asm-powerpc/prom.h
+++ b/include/asm-powerpc/prom.h
@@ -305,6 +305,7 @@ extern int of_irq_map_raw(struct device_node *parent, const u32 *intspec,
 			  u32 ointsize, const u32 *addr,
 			  struct of_irq *out_irq);
 
+void of_register_i2c_devices(struct device_node *adap_node, int bus_num);
 
 /**
  * of_irq_map_one - Resolve an interrupt for a device

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Kumar Gala]

On Jun 29, 2007, at 2:20 PM, Guennadi Liakhovetski wrote:

> Add of_register_i2c_devices(), which scans the children of the  
> specified
> I2C adapter node, and registers them with the I2C code.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

I'm still against this patch since it means we are stating that I2C  
devices in the device tree must us the linux driver names.  The two  
should be de-coupled.

- k

>
> diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/ 
> prom_parse.c
> index 3786dcc..9caf96d 100644
> --- a/arch/powerpc/kernel/prom_parse.c
> +++ b/arch/powerpc/kernel/prom_parse.c
> @@ -1067,3 +1067,49 @@ void __iomem *of_iomap(struct device_node  
> *np, int index)
>  	return ioremap(res.start, 1 + res.end - res.start);
>  }
>  EXPORT_SYMBOL(of_iomap);
> +
> +#ifdef CONFIG_I2C
> +#include <linux/i2c.h>
> +
> +void of_register_i2c_devices(struct device_node *adap_node, int  
> bus_num)
> +{
> +	struct device_node *node = NULL;
> +
> +	while ((node = of_get_next_child(adap_node, node))) {
> +		struct i2c_board_info info;
> +		const u32 *addr;
> +		const char *name, *model;
> +		int len;
> +
> +		addr = of_get_property(node, "reg", &len);
> +		if (!addr || len < sizeof(int) || *addr > 0xffff)
> +			continue;
> +
> +		info.irq = irq_of_parse_and_map(node, 0);
> +		if (info.irq == NO_IRQ)
> +			info.irq = -1;
> +
> +		name = of_get_property(node, "compatible", NULL);
> +		if (!name)
> +			name = node->name;
> +		if (!name)
> +			continue;
> +
> +		model = of_get_property(node, "model", NULL);
> +		if (!model)
> +			model = name;
> +
> +		/* FIXME: the i2c code should allow drivers to specify
> +		 * multiple match names; board code shouldn't need to
> +		 * know what driver will handle a given type.
> +		 */
> +
> +		snprintf(info.driver_name, KOBJ_NAME_LEN, name);
> +		snprintf(info.type, KOBJ_NAME_LEN, model);
> +		info.platform_data = NULL;
> +		info.addr = *addr;
> +
> +		i2c_register_board_info(bus_num, &info, 1);
> +	}
> +}
> +#endif /* CONFIG_I2C */
> diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
> index 6845af9..ed085e1 100644
> --- a/include/asm-powerpc/prom.h
> +++ b/include/asm-powerpc/prom.h
> @@ -305,6 +305,7 @@ extern int of_irq_map_raw(struct device_node  
> *parent, const u32 *intspec,
>  			  u32 ointsize, const u32 *addr,
>  			  struct of_irq *out_irq);
>
> +void of_register_i2c_devices(struct device_node *adap_node, int  
> bus_num);
>
>  /**
>   * of_irq_map_one - Resolve an interrupt for a device
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> Add of_register_i2c_devices(), which scans the children of the
>> specified
>> I2C adapter node, and registers them with the I2C code.
>>
>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>> Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>
>
> I'm still against this patch since it means we are stating that I2C
> devices in the device tree must us the linux driver names.  The two
> should be de-coupled.

Very much so.  More comments as reply to the original post...


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

> +	while ((node = of_get_next_child(adap_node, node))) {

of_for_each_child_node() or whatever it is called?

> +		addr = of_get_property(node, "reg", &len);
> +		if (!addr || len < sizeof(int) || *addr > 0xffff)
> +			continue;

Addresses aren't 16 bit AFAIK?

> +		name = of_get_property(node, "compatible", NULL);
> +		if (!name)
> +			name = node->name;
> +		if (!name)
> +			continue;

Look at "name" first, then look at "compatible", not
the other way around.

> +		model = of_get_property(node, "model", NULL);
> +		if (!model)
> +			model = name;

No way.  "model" and "name" have nothing in common
semantically.

> +		/* FIXME: the i2c code should allow drivers to specify
> +		 * multiple match names; board code shouldn't need to
> +		 * know what driver will handle a given type.
> +		 */

This should be handled by the OF matching stuff, there
is no direct connection between device tree naming and
Linux driver naming.  I don't want to see this patch
going in without this problem being fixed first.


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

Hi Segher,

On Mon, 2 Jul 2007, Segher Boessenkool wrote:

> > +	while ((node = of_get_next_child(adap_node, node))) {
> 
> of_for_each_child_node() or whatever it is called?

Sure, would be nice, only I cannot find anything like that. I seem to 
remember seeing a patch like that, but maybe I'me mistaken... Do you 
remember details / can you find it?

> > +		addr = of_get_property(node, "reg", &len);
> > +		if (!addr || len < sizeof(int) || *addr > 0xffff)
> > +			continue;
> 
> Addresses aren't 16 bit AFAIK?

Maximum 10 bits. So, yes, it can become 0x3ff.

> > +		name = of_get_property(node, "compatible", NULL);
> > +		if (!name)
> > +			name = node->name;
> > +		if (!name)
> > +			continue;
> 
> Look at "name" first, then look at "compatible", not
> the other way around.

ok

> > +		model = of_get_property(node, "model", NULL);
> > +		if (!model)
> > +			model = name;
> 
> No way.  "model" and "name" have nothing in common
> semantically.

ok

> > +		/* FIXME: the i2c code should allow drivers to specify
> > +		 * multiple match names; board code shouldn't need to
> > +		 * know what driver will handle a given type.
> > +		 */
> 
> This should be handled by the OF matching stuff, there
> is no direct connection between device tree naming and
> Linux driver naming.  I don't want to see this patch
> going in without this problem being fixed first.

Right, this one is interesting. But this time I cannot even vaguely 
remember seeing any "OF matching" code. I can only remember each driver 
(or OF glue to a generic driver) parsing compatible of entries, like 
legacy_serial.c. Or you mean we need to write such a matching layer?

AFAIK, i2c is not specified by OF standard. So, we are free to chose any 
"compatible" fields for our dts. Which we then would need to match against 
drivers. So, why not make this a 1-to-1 match? I.e.,
[OF]			[I2C]
compatible	<->	driver_name
model		<->	type

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Scott Wood]

On Mon, Jul 02, 2007 at 10:11:04PM +0200, Guennadi Liakhovetski wrote:
> AFAIK, i2c is not specified by OF standard. So, we are free to chose any 
> "compatible" fields for our dts. Which we then would need to match against 
> drivers. So, why not make this a 1-to-1 match? I.e.,
> [OF]			[I2C]
> compatible	<->	driver_name
> model		<->	type

If we do this, then the compatible field should probably be preceded with
"linux,", which would get stripped by of_register_i2c_devices().  This
would only be an interim measure until we can figure out some sort of
name registry via power.org.  Once that happens, non-"linux," compatible
fields could be looked up in an OF-to-linux table, or there could be a
way for i2c devices to specify OF matches in addition to driver name.

-Scott

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> AFAIK, i2c is not specified by OF standard. So, we are free to chose 
>> any
>> "compatible" fields for our dts. Which we then would need to match 
>> against
>> drivers. So, why not make this a 1-to-1 match? I.e.,
>> [OF]			[I2C]
>> compatible	<->	driver_name
>> model		<->	type
>
> If we do this, then the compatible field should probably be preceded 
> with
> "linux,", which would get stripped by of_register_i2c_devices().  This
> would only be an interim measure until we can figure out some sort of
> name registry via power.org.  Once that happens, non-"linux," 
> compatible
> fields could be looked up in an OF-to-linux table, or there could be a
> way for i2c devices to specify OF matches in addition to driver name.

Why even think of doing this?  Just put the bloody name of
the device in the "compatible" property and match on that.
What is so hard about this?

If the answer is "the Linux I2C stuff is badly structured",
well, you know what to do :-)


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> of_for_each_child_node() or whatever it is called?
>
> Sure, would be nice, only I cannot find anything like that. I seem to
> remember seeing a patch like that, but maybe I'me mistaken... Do you
> remember details / can you find it?

Not right now, sorry.  Use grep?  :-)

>>> +		name = of_get_property(node, "compatible", NULL);
>>> +		if (!name)
>>> +			name = node->name;
>>> +		if (!name)
>>> +			continue;
>>
>> Look at "name" first, then look at "compatible", not
>> the other way around.
>
> ok

Which btw is what the "is_compatible()" stuff does
(or, ought to do).

>>> +		/* FIXME: the i2c code should allow drivers to specify
>>> +		 * multiple match names; board code shouldn't need to
>>> +		 * know what driver will handle a given type.
>>> +		 */
>>
>> This should be handled by the OF matching stuff, there
>> is no direct connection between device tree naming and
>> Linux driver naming.  I don't want to see this patch
>> going in without this problem being fixed first.
>
> Right, this one is interesting. But this time I cannot even vaguely
> remember seeing any "OF matching" code. I can only remember each driver
> (or OF glue to a generic driver) parsing compatible of entries, like
> legacy_serial.c. Or you mean we need to write such a matching layer?

The OF glue parsing stuff and setting up the data structures
the generic drivers want to see.  I say make it work for your
board first, then when later more boards need it we have a
better idea about what is needed from a more generic I2C glue
layer.

> AFAIK, i2c is not specified by OF standard.

Yes.

> So, we are free to chose any "compatible" fields for our dts.

Within bounds.  What goes into "compatible" properties is
well-defined for _any_ device.

> Which we then would need to match against
> drivers. So, why not make this a 1-to-1 match?

Because the device tree describes the hardware, not what
Linux (or anything else) is going to do with it.  Let me
give you an example of why this is a good thing for _you_:
say a future version of Linux drops the I2C driver for a
device you have, since some other driver should be used
preferably instead (maybe it is just a better driver for
the same device, maybe it supports more devices, whatever).
Suddenly boards with your old device tree cannot run that
new Linux version anymore.  Ouch.

Just use "compatible" exactly the way it is meant to be
used and your life will be so much simpler.  And don't
even think about using a very generic property like "model"
for a purpose totally different from its intended purpose.


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Tue, 3 Jul 2007, Segher Boessenkool wrote:

> Just use "compatible" exactly the way it is meant to be
> used and your life will be so much simpler.  And don't
> even think about using a very generic property like "model"
> for a purpose totally different from its intended purpose.

AFAIU, the only way currently to attach an i2c device to a driver is to 
match against driver's name. Let's take the rtc-rs5c372.c example. It 
services rs5c372a, rs5c372b, rv5c386, rv5c387a. And it registers itself 
with the name "rtc-rs5c372". We want to just specify in our dts, e.g., 
'compatible = "rs5c372b";', right? And ask the i2c layer to find a 
suitable driver for us. Which means a pretty big change to i2c core. Is 
this what you think is the proper solution? I could try to think about it 
/ discuss on i2c and eventually cook up a patch, just want to make sure I 
understand you right.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> Just use "compatible" exactly the way it is meant to be
>> used and your life will be so much simpler.  And don't
>> even think about using a very generic property like "model"
>> for a purpose totally different from its intended purpose.
>
> AFAIU, the only way currently to attach an i2c device to a driver  
> is to
> match against driver's name. Let's take the rtc-rs5c372.c example. It
> services rs5c372a, rs5c372b, rv5c386, rv5c387a. And it registers  
> itself
> with the name "rtc-rs5c372". We want to just specify in our dts, e.g.,
> 'compatible = "rs5c372b";', right? And ask the i2c layer to find a
> suitable driver for us. Which means a pretty big change to i2c  
> core. Is
> this what you think is the proper solution? I could try to think  
> about it
> / discuss on i2c and eventually cook up a patch, just want to make  
> sure I
> understand you right.

Your device is an rs5c372b.  So, that's what you put in
your device tree.  Simple so far, right?

Now some OF I2C code goes looking for IIC devices in the
device tree.  It finds this thing, and from a table or
something it derives that it has to tell the kernel I2C
layer this is an "rtc-rs5c372".  [It would be nicer if it
could just instantiate the correct driver directly, but
if that's how the Linux I2C layer works, so be it].

No change in the I2C "core" needed, just an OF "compatible"
matching thing like is needed *everywhere else* too.


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Wed, 4 Jul 2007, Segher Boessenkool wrote:

> Your device is an rs5c372b.  So, that's what you put in
> your device tree.  Simple so far, right?

Yep.

> Now some OF I2C code goes looking for IIC devices in the
> device tree.  It finds this thing, and from a table or
> something it derives that it has to tell the kernel I2C
> layer this is an "rtc-rs5c372".

(I2C ML cc'ed.)

This is where I WOULD disagree. These tables would rather live inside the 
i2c layer, be filled by respective drivers themselves. Noone except the 
rs5c372 driver can know which devices it can handle. Using the very same 
your argument - what if in a future version this driver disappears and 
another one will be used for these devices? Then that driver will have to 
register support for this device.

For this to work i2c would need something similar to what pci, usb do - 
register supported device ids. The only difference is that instead of 
numerical IDs we have to use plain text names for i2c devices...

> [It would be nicer if it
> could just instantiate the correct driver directly, but
> if that's how the Linux I2C layer works, so be it].
> 
> No change in the I2C "core" needed, just an OF "compatible"
> matching thing like is needed *everywhere else* too.

Yes, this is why I put "would". Looks like this is the common powerpc 
practice ATM - to make such a glue to map arbitrary "OF names" to what 
respective drivers react to. Like in the case of the serial driver. But - 
i2c is much more diverse and dynamic than serial, so, maybe it is worth 
thinking about "fixing" i2c? Once again my offer for the i2c folks - I 
could try to think about a design for this, but I would happily accept 
anybody else more proficient with i2c do it.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> Now some OF I2C code goes looking for IIC devices in the
>> device tree.  It finds this thing, and from a table or
>> something it derives that it has to tell the kernel I2C
>> layer this is an "rtc-rs5c372".
>
> (I2C ML cc'ed.)
>
> This is where I WOULD disagree. These tables would rather live  
> inside the
> i2c layer,

Physical location doesn't matter, logical location is
a separate layer.

> be filled by respective drivers themselves.

That would be nicer yes, but a bigger change.

> Noone except the
> rs5c372 driver can know which devices it can handle.

I don't really agree but that's more a philosophical than
a technical argument.

> Using the very same
> your argument - what if in a future version this driver disappears and
> another one will be used for these devices? Then that driver will  
> have to
> register support for this device.

And it's all in the same kernel source tree so it would be
a trivial fixup -- quite different from keeping OF device
trees and Linux kernels in synch (which is pretty much
impossible).

> For this to work i2c would need something similar to what pci, usb  
> do -
> register supported device ids. The only difference is that instead of
> numerical IDs we have to use plain text names for i2c devices...

Yeah, that would be nice.  Are you suggesting the Linux I2C
layer should use "OF-style" names as its "native" naming
scheme?  I'd rather keep the namespaces separate, coupling
them always seems like a great idea and always turns out a
disaster.

>> [It would be nicer if it
>> could just instantiate the correct driver directly, but
>> if that's how the Linux I2C layer works, so be it].
>>
>> No change in the I2C "core" needed, just an OF "compatible"
>> matching thing like is needed *everywhere else* too.
>
> Yes, this is why I put "would". Looks like this is the common powerpc
> practice ATM - to make such a glue to map arbitrary "OF names" to what
> respective drivers react to. Like in the case of the serial driver.

Yes.  This is the most flexible scheme possible and allows
for all kinds of fixups/workarounds in case of broken device
trees (or broken kernel code, for that matter).

> But -
> i2c is much more diverse and dynamic than serial, so, maybe it is  
> worth
> thinking about "fixing" i2c?

Be my guest :-)  I care more about the OF side of things, but
let me ask anyway -- what do you see as "broken" in the Linux
IIC "core" that needs fixing here?


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Wed, 4 Jul 2007, Segher Boessenkool wrote:

> > be filled by respective drivers themselves.
> 
> That would be nicer yes, but a bigger change.

Aha, but nicer!:-)

> > For this to work i2c would need something similar to what pci, usb do -
> > register supported device ids. The only difference is that instead of
> > numerical IDs we have to use plain text names for i2c devices...
> 
> Yeah, that would be nice.  Are you suggesting the Linux I2C
> layer should use "OF-style" names as its "native" naming
> scheme?  I'd rather keep the namespaces separate, coupling
> them always seems like a great idea and always turns out a
> disaster.

Sorry, I'm probably missing something here. What's the principal 
difference between the "OF-style" and "native" names? Wouldn't "rs5c372b" 
be a natural name for both the DT "compatible" property and the list of 
driver supported devices in i2c?

The only thing that I can see here is devices with not so obvious names. 
OTOH, we do want all .dts to use the same name for the same chip, soooo...

> > Yes, this is why I put "would". Looks like this is the common powerpc
> > practice ATM - to make such a glue to map arbitrary "OF names" to what
> > respective drivers react to. Like in the case of the serial driver.
> 
> Yes.  This is the most flexible scheme possible and allows
> for all kinds of fixups/workarounds in case of broken device
> trees (or broken kernel code, for that matter).

Well, I think, I might bite. But - mostly for the above argument, i.e., 
just because it offers slightly more flexibility, doesn't require changes 
to existing drivers / subsystems...

> > But -
> > i2c is much more diverse and dynamic than serial, so, maybe it is worth
> > thinking about "fixing" i2c?
> 
> Be my guest :-)  I care more about the OF side of things, but
> let me ask anyway -- what do you see as "broken" in the Linux
> IIC "core" that needs fixing here?

No, I put "fixing" in quotes, i.e., I didn't really mean it was some 
brokenness. Just nobody needed this until now. Just that device - driver 
matching in i2c happens based on driver id, not device id, like in pci, 
usb, maybe others.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Wed, 4 Jul 2007, Segher Boessenkool wrote:

> Your device is an rs5c372b.  So, that's what you put in
> your device tree.  Simple so far, right?
> 
> Now some OF I2C code goes looking for IIC devices in the
> device tree.  It finds this thing, and from a table or
> something it derives that it has to tell the kernel I2C
> layer this is an "rtc-rs5c372".  [It would be nicer if it
> could just instantiate the correct driver directly, but
> if that's how the Linux I2C layer works, so be it].
> 
> No change in the I2C "core" needed, just an OF "compatible"
> matching thing like is needed *everywhere else* too.

How about the patch below?

Thanks
Guennadi
---
Guennadi Liakhovetski

Scan the device tree for i2c devices, check their "compatible" property 
against a hard-coded table, and, if found, register with i2c boardinfo.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 9588b60..c3c7eba 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -296,6 +296,62 @@ err:
 
 arch_initcall(gfar_of_init);
 
+#ifdef CONFIG_I2C_BOARDINFO
+#include <linux/i2c.h>
+struct i2c_driver_device {
+	char	*of_device;
+	char	*i2c_driver;
+	char	*i2c_type;
+};
+
+static struct i2c_driver_device i2c_devices[] = {
+	{"rs5c372a", "rtc-rs5c372", "rs5c372a",},
+	{"rs5c372b", "rtc-rs5c372", "rs5c372b",},
+	{"rv5c386",  "rtc-rs5c372", "rv5c386",},
+	{"rv5c387a", "rtc-rs5c372", "rv5c387a",},
+};
+
+static int of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
+		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
+			continue;
+		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
+		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		return 0;
+	}
+	return -ENODEV;
+}
+
+static void of_register_i2c_devices(struct device_node *adap_node, int bus_num)
+{
+	struct device_node *node = NULL;
+
+	while ((node = of_get_next_child(adap_node, node))) {
+		struct i2c_board_info info;
+		const u32 *addr;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > 0xffff)
+			continue;
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		if (of_find_i2c_driver(node, &info) < 0)
+			continue;
+
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		i2c_register_board_info(bus_num, &info, 1);
+	}
+}
+
 static int __init fsl_i2c_of_init(void)
 {
 	struct device_node *np;
@@ -340,6 +396,8 @@ static int __init fsl_i2c_of_init(void)
 						    fsl_i2c_platform_data));
 		if (ret)
 			goto unreg;
+
+		of_register_i2c_devices(np, i);
 	}
 
 	return 0;
@@ -351,6 +409,7 @@ err:
 }
 
 arch_initcall(fsl_i2c_of_init);
+#endif
 
 #ifdef CONFIG_PPC_83xx
 static int __init mpc83xx_wdt_init(void)

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> Your device is an rs5c372b.  So, that's what you put in
>> your device tree.  Simple so far, right?
>>
>> Now some OF I2C code goes looking for IIC devices in the
>> device tree.  It finds this thing, and from a table or
>> something it derives that it has to tell the kernel I2C
>> layer this is an "rtc-rs5c372".  [It would be nicer if it
>> could just instantiate the correct driver directly, but
>> if that's how the Linux I2C layer works, so be it].
>>
>> No change in the I2C "core" needed, just an OF "compatible"
>> matching thing like is needed *everywhere else* too.
>
> How about the patch below?

Looks good.  It should later be moved to a more generic
I2C OF matching layer, but fine for now.  Thanks!

You might want to put vendor names in the "compatible"
entries, dunno though, maybe these are "cross-vendor"
ICs?


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Thu, 19 Jul 2007, Segher Boessenkool wrote:

> > > Your device is an rs5c372b.  So, that's what you put in
> > > your device tree.  Simple so far, right?
> > > 
> > > Now some OF I2C code goes looking for IIC devices in the
> > > device tree.  It finds this thing, and from a table or
> > > something it derives that it has to tell the kernel I2C
> > > layer this is an "rtc-rs5c372".  [It would be nicer if it
> > > could just instantiate the correct driver directly, but
> > > if that's how the Linux I2C layer works, so be it].
> > > 
> > > No change in the I2C "core" needed, just an OF "compatible"
> > > matching thing like is needed *everywhere else* too.
> > 
> > How about the patch below?
> 
> Looks good.  It should later be moved to a more generic
> I2C OF matching layer, but fine for now.  Thanks!
> 
> You might want to put vendor names in the "compatible"
> entries, dunno though, maybe these are "cross-vendor"
> ICs?

You mean like

	compatible = "ricoh,rs5c372a"

etc? With this change, plus __init(data) attributes for functions and the 
array, attached below is version 2 of the patch. Corresponding 
modifications to the kurobox*.dts will follow separately.

Thanks
Guennadi
---
Guennadi Liakhovetski

Scan the device tree for i2c devices, check their "compatible" property 
against a hard-coded table, and, if found, register with i2c boardinfo.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3289fab..e07d031 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -305,6 +305,62 @@ err:
 
 arch_initcall(gfar_of_init);
 
+#ifdef CONFIG_I2C_BOARDINFO
+#include <linux/i2c.h>
+struct i2c_driver_device {
+	char	*of_device;
+	char	*i2c_driver;
+	char	*i2c_type;
+};
+
+static struct i2c_driver_device i2c_devices[] __initdata = {
+	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
+	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
+	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
+	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
+};
+
+static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
+		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
+			continue;
+		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
+		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		return 0;
+	}
+	return -ENODEV;
+}
+
+static void __init of_register_i2c_devices(struct device_node *adap_node, int bus_num)
+{
+	struct device_node *node = NULL;
+
+	while ((node = of_get_next_child(adap_node, node))) {
+		struct i2c_board_info info;
+		const u32 *addr;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > 0xffff)
+			continue;
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		if (of_find_i2c_driver(node, &info) < 0)
+			continue;
+
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		i2c_register_board_info(bus_num, &info, 1);
+	}
+}
+
 static int __init fsl_i2c_of_init(void)
 {
 	struct device_node *np;
@@ -349,6 +405,8 @@ static int __init fsl_i2c_of_init(void)
 						    fsl_i2c_platform_data));
 		if (ret)
 			goto unreg;
+
+		of_register_i2c_devices(np, i);
 	}
 
 	return 0;
@@ -360,6 +418,7 @@ err:
 }
 
 arch_initcall(fsl_i2c_of_init);
+#endif
 
 #ifdef CONFIG_PPC_83xx
 static int __init mpc83xx_wdt_init(void)

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>> You might want to put vendor names in the "compatible"
>> entries, dunno though, maybe these are "cross-vendor"
>> ICs?
>
> You mean like
>
> 	compatible = "ricoh,rs5c372a"

Yeah, like that.  I'm not sure it is needed for these,
but it won't hurt either.

> +		strncpy(info->driver_name, i2c_devices[i].i2c_driver,  
> KOBJ_NAME_LEN);
> +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);

Why not just strcpy(), btw?

> +		addr = of_get_property(node, "reg", &len);
> +		if (!addr || len < sizeof(int) || *addr > 0xffff)
> +			continue;

Give a warning when the addr won't fit in 16 bits?


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Fri, 20 Jul 2007, Segher Boessenkool wrote:

> > +		strncpy(info->driver_name, i2c_devices[i].i2c_driver,
> > KOBJ_NAME_LEN);
> > +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> 
> Why not just strcpy(), btw?

Because target strings are finite length, and sources are just pointers to 
some constant strings, which one might make arbitrarily long.

> > +		addr = of_get_property(node, "reg", &len);
> > +		if (!addr || len < sizeof(int) || *addr > 0xffff)
> > +			continue;
> 
> Give a warning when the addr won't fit in 16 bits?

Ok, version 3, hopefully last, below.

Thanks
Guennadi
---
Guennadi Liakhovetski

Scan the device tree for i2c devices, check their "compatible" property
against a hard-coded table, and, if found, register with i2c boardinfo.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3289fab..727453d 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -305,6 +305,64 @@ err:
 
 arch_initcall(gfar_of_init);
 
+#ifdef CONFIG_I2C_BOARDINFO
+#include <linux/i2c.h>
+struct i2c_driver_device {
+	char	*of_device;
+	char	*i2c_driver;
+	char	*i2c_type;
+};
+
+static struct i2c_driver_device i2c_devices[] __initdata = {
+	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
+	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
+	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
+	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
+};
+
+static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
+		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
+			continue;
+		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
+		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		return 0;
+	}
+	return -ENODEV;
+}
+
+static void __init of_register_i2c_devices(struct device_node *adap_node, int bus_num)
+{
+	struct device_node *node = NULL;
+
+	while ((node = of_get_next_child(adap_node, node))) {
+		struct i2c_board_info info;
+		const u32 *addr;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+			printk(KERN_WARNING "fsl_ioc.c: invalid i2c device entry\n");
+			continue;
+		}
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		if (of_find_i2c_driver(node, &info) < 0)
+			continue;
+
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		i2c_register_board_info(bus_num, &info, 1);
+	}
+}
+
 static int __init fsl_i2c_of_init(void)
 {
 	struct device_node *np;
@@ -349,6 +407,8 @@ static int __init fsl_i2c_of_init(void)
 						    fsl_i2c_platform_data));
 		if (ret)
 			goto unreg;
+
+		of_register_i2c_devices(np, i);
 	}
 
 	return 0;
@@ -360,6 +420,7 @@ err:
 }
 
 arch_initcall(fsl_i2c_of_init);
+#endif
 
 #ifdef CONFIG_PPC_83xx
 static int __init mpc83xx_wdt_init(void)

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>>> +		strncpy(info->driver_name, i2c_devices[i].i2c_driver,
>>> KOBJ_NAME_LEN);
>>> +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
>>
>> Why not just strcpy(), btw?
>
> Because target strings are finite length, and sources are just 
> pointers to
> some constant strings, which one might make arbitrarily long.

So it's no problem if the name or type string gets cut short?
Just checking :-)


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Wed, 1 Aug 2007, Segher Boessenkool wrote:

> > > > +		strncpy(info->driver_name, i2c_devices[i].i2c_driver,
> > > > KOBJ_NAME_LEN);
> > > > +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> > > 
> > > Why not just strcpy(), btw?
> > 
> > Because target strings are finite length, and sources are just pointers to
> > some constant strings, which one might make arbitrarily long.
> 
> So it's no problem if the name or type string gets cut short?
> Just checking :-)

Then it just won't match. Which is also the expected behaviour, IMHO, if 
someone specifies a name longer than possible maximum length of the 
variable it should match.

Have I passed or failed?:-)

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>>>>> +		strncpy(info->driver_name, i2c_devices[i].i2c_driver,
>>>>> KOBJ_NAME_LEN);
>>>>> +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
>>>>
>>>> Why not just strcpy(), btw?
>>>
>>> Because target strings are finite length, and sources are just 
>>> pointers to
>>> some constant strings, which one might make arbitrarily long.
>>
>> So it's no problem if the name or type string gets cut short?
>> Just checking :-)
>
> Then it just won't match.

strncpy() won't put a terminating zero on there, is everything
that uses the resulting string okay with that?  Also, if the
name gets cut short, it might match some _other_ expected name.

> Have I passed or failed?:-)

Dunno, what is this test, anyway?  :-)


Segher

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Guennadi Liakhovetski]

On Wed, 1 Aug 2007, Segher Boessenkool wrote:

> > > > > > +		strncpy(info->driver_name, i2c_devices[i].i2c_driver,
> > > > > > KOBJ_NAME_LEN);
> > > > > > +		strncpy(info->type, i2c_devices[i].i2c_type,
> > > > > > I2C_NAME_SIZE);
> > > > > 
> > > > > Why not just strcpy(), btw?
> > > > 
> > > > Because target strings are finite length, and sources are just pointers
> > > > to
> > > > some constant strings, which one might make arbitrarily long.
> > > 
> > > So it's no problem if the name or type string gets cut short?
> > > Just checking :-)
> > 
> > Then it just won't match.
> 
> strncpy() won't put a terminating zero on there, is everything
> that uses the resulting string okay with that?

Ok, this might be a problem, I guess. E.g., in 
drivers/i2c/i2c-core.c::i2c_device_uevent():

	if (add_uevent_var(envp, num_envp, &i, buffer, buffer_size, &length,
			"MODALIAS=%s", client->driver_name))

and in a couple more places.

> Also, if the
> name gets cut short, it might match some _other_ expected name.

Don't think this could happen. At least the generic strcmp compares the 
'\0' too.

Now, the bad news (for me at least, as well as for some on #mklinux). To 
fix the bug I wrote the patch below. Yes, it is lacking a "Signed-..." - 
on purpose. Here's why. Having written it, I verified, that

	c[4] = "01234";

causes a nice compiler warning like:

warning: initializer-string for array of chars is too long

Then I asked myself what happens if you do

	c[4] = "0123";

? To my surprise, there was no warning. So, for example, declaring a 
struct like

struct t {
	char c[4];
	int i;
} z;

and doing

	strcpy(z.c, c);

silently corrupts z.i. Verified with gcc 3.3.x, 3.4.5, 4.1.2. 4.2 Does 
emit a warning... Now, am I right that there are lots of places in 
the kernel where we just initialize arrays of chars like above and then 
use ctrcpy to copy it to another equally-long string? Which means, if the 
initialization string is exactly the array length - without the '\0' - we 
get random memory corruption... And the only safe way is 

	strncpy(z.c, c, 3);
	z.c[3] = '\0';

with compilers < 4.2?...

Thanks
Guennadi
---

Fix a bug introduced by my earlier patch, whereby strncpy of too long a 
line could leave the result unterminated. Instead, just use fixed-size 
arrays and let the compiler verify the length for us. No need to try to 
save a few bytes of __initdata anyway. Thanks to Segher Boessenkool for 
pointing out.

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..1e567d5 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -309,8 +309,8 @@ arch_initcall(gfar_of_init);
 #include <linux/i2c.h>
 struct i2c_driver_device {
 	char	*of_device;
-	char	*i2c_driver;
-	char	*i2c_type;
+	char	i2c_driver[KOBJ_NAME_LEN];
+	char	i2c_type[I2C_NAME_SIZE];
 };
 
 static struct i2c_driver_device i2c_devices[] __initdata = {
@@ -327,8 +327,8 @@ static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_
 	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
 		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
 			continue;
-		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
-		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		strcpy(info->driver_name, i2c_devices[i].i2c_driver);
+		strcpy(info->type, i2c_devices[i].i2c_type);
 		return 0;
 	}
 	return -ENODEV;

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Scott Wood]

Guennadi Liakhovetski wrote:
> And the only safe way is 
> 
> 	strncpy(z.c, c, 3);
> 	z.c[3] = '\0';
> 
> with compilers < 4.2?...

You could use strlcpy() instead, which always leaves a zero terminator.

-Scott

[PATCH] powerpc: fix i2c device string format

[Guennadi Liakhovetski]

On Wed, 1 Aug 2007, Segher Boessenkool wrote:

> strncpy() won't put a terminating zero on there, is everything
> that uses the resulting string okay with that?  Also, if the
> name gets cut short, it might match some _other_ expected name.

On Wed, 1 Aug 2007, Scott Wood wrote:

> You could use strlcpy() instead, which always leaves a zero terminator.

The patch below does exactly this - uses strlcpy() to guarantee strings in 
i2c device type and driver_name fields are 0-terminated.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

---

Trying to follow the "canonical" patch format: Paul, please queue this 
patch for 2.6.24. It is not critical, as there no strings at the moment in 
fsl_soc.c, that exceed i2c limits. So, it's just a precaution measure.

Thanks
Guennadi

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..2ef7036 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -327,8 +327,8 @@ static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_
 	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
 		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
 			continue;
-		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
-		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		strlcpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
+		strlcpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
 		return 0;
 	}
 	return -ENODEV;

Re: [PATCH] powerpc: fix i2c device string format

[Segher Boessenkool]

>> strncpy() won't put a terminating zero on there, is everything
>> that uses the resulting string okay with that?  Also, if the
>> name gets cut short, it might match some _other_ expected name.
>
> On Wed, 1 Aug 2007, Scott Wood wrote:
>
>> You could use strlcpy() instead, which always leaves a zero 
>> terminator.
>
> The patch below does exactly this - uses strlcpy() to guarantee 
> strings in
> i2c device type and driver_name fields are 0-terminated.

You're not checking the return values of these calls.  This would
be a good function to put attribute warn_unused_result on :-)


Segher

Re: [PATCH] powerpc: fix i2c device string format

[Guennadi Liakhovetski]

On Thu, 9 Aug 2007, Segher Boessenkool wrote:

> > > strncpy() won't put a terminating zero on there, is everything
> > > that uses the resulting string okay with that?  Also, if the
> > > name gets cut short, it might match some _other_ expected name.
> > 
> > On Wed, 1 Aug 2007, Scott Wood wrote:
> > 
> > > You could use strlcpy() instead, which always leaves a zero terminator.
> > 
> > The patch below does exactly this - uses strlcpy() to guarantee strings in
> > i2c device type and driver_name fields are 0-terminated.
> 
> You're not checking the return values of these calls.  This would
> be a good function to put attribute warn_unused_result on :-)

hm... Well, the worst that could happen, if an "evil" programmer defines 
too long a name, it gets truncated, and then binds to a wrong driver, 
well, the worst that can happen is that your hardware gets damaged, not a 
big thing. However, some might disagree, so, below is a new version... 
Wrap some long lines while at that.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..c0d66df 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -320,21 +320,26 @@ static struct i2c_driver_device i2c_devices[] __initdata = {
 	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
 };
 
-static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
+static int __init of_find_i2c_driver(struct device_node *node,
+				     struct i2c_board_info *info)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
 		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
 			continue;
-		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
-		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		if (strlcpy(info->driver_name, i2c_devices[i].i2c_driver,
+			    KOBJ_NAME_LEN) >= KOBJ_NAME_LEN ||
+		    strlcpy(info->type, i2c_devices[i].i2c_type,
+			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
+			return -ENOMEM;
 		return 0;
 	}
 	return -ENODEV;
 }
 
-static void __init of_register_i2c_devices(struct device_node *adap_node, int bus_num)
+static void __init of_register_i2c_devices(struct device_node *adap_node,
+					   int bus_num)
 {
 	struct device_node *node = NULL;
 

Re: [PATCH] powerpc: fix i2c device string format

[Segher Boessenkool]

> hm... Well, the worst that could happen, if an "evil" programmer 
> defines
> too long a name, it gets truncated, and then binds to a wrong driver,
> well, the worst that can happen is that your hardware gets damaged, 
> not a
> big thing.

:-)

> However, some might disagree, so, below is a new version...
> Wrap some long lines while at that.
>
> Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

Looks good to me, thank you!


Segher

Re: [PATCH] powerpc: fix i2c device string format

[Paul Mackerras]

Guennadi Liakhovetski writes:

> On Thu, 9 Aug 2007, Segher Boessenkool wrote:
> 
> > > > strncpy() won't put a terminating zero on there, is everything
> > > > that uses the resulting string okay with that?  Also, if the
> > > > name gets cut short, it might match some _other_ expected name.
> > > 
> > > On Wed, 1 Aug 2007, Scott Wood wrote:
> > > 
> > > > You could use strlcpy() instead, which always leaves a zero terminator.
> > > 
> > > The patch below does exactly this - uses strlcpy() to guarantee strings in
> > > i2c device type and driver_name fields are 0-terminated.
> > 
> > You're not checking the return values of these calls.  This would
> > be a good function to put attribute warn_unused_result on :-)
> 
> hm... Well, the worst that could happen, if an "evil" programmer defines 
> too long a name, it gets truncated, and then binds to a wrong driver, 
> well, the worst that can happen is that your hardware gets damaged, not a 
> big thing. However, some might disagree, so, below is a new version... 
> Wrap some long lines while at that.

That's not a commit message I can use.  Please repost with an
informative commit message that says what the motivation for the
change is, plus anything other information that would be useful for
someone looking at this in a couple of years' time.

Paul.

Re: [PATCH] powerpc: fix i2c device string format

[Guennadi Liakhovetski]

Use strlcpy() to guarantee strings in i2c device type and driver_name 
fields are 0-terminated.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

On Wed, 15 Aug 2007, Paul Mackerras wrote:

> That's not a commit message I can use.  Please repost with an
> informative commit message that says what the motivation for the
> change is, plus anything other information that would be useful for
> someone looking at this in a couple of years' time.

Sure, sorry. This should be fine - below "---" I may write whatever I 
want:-)

Thanks
Guennadi

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..c0d66df 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -320,21 +320,26 @@ static struct i2c_driver_device i2c_devices[] __initdata = {
 	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
 };
 
-static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
+static int __init of_find_i2c_driver(struct device_node *node,
+				     struct i2c_board_info *info)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
 		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
 			continue;
-		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
-		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		if (strlcpy(info->driver_name, i2c_devices[i].i2c_driver,
+			    KOBJ_NAME_LEN) >= KOBJ_NAME_LEN ||
+		    strlcpy(info->type, i2c_devices[i].i2c_type,
+			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
+			return -ENOMEM;
 		return 0;
 	}
 	return -ENODEV;
 }
 
-static void __init of_register_i2c_devices(struct device_node *adap_node, int bus_num)
+static void __init of_register_i2c_devices(struct device_node *adap_node,
+					   int bus_num)
 {
 	struct device_node *node = NULL;
 

Re: [PATCH] powerpc: fix i2c device string format

[Scott Wood]

Guennadi Liakhovetski wrote:
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 727453d..c0d66df 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -320,21 +320,26 @@ static struct i2c_driver_device i2c_devices[] __initdata = {
>  	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
>  };
>  
> -static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
> +static int __init of_find_i2c_driver(struct device_node *node,
> +				     struct i2c_board_info *info)
>  {
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
>  		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
>  			continue;
> -		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
> -		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> +		if (strlcpy(info->driver_name, i2c_devices[i].i2c_driver,
> +			    KOBJ_NAME_LEN) >= KOBJ_NAME_LEN ||
> +		    strlcpy(info->type, i2c_devices[i].i2c_type,
> +			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
> +			return -ENOMEM;
>  		return 0;
>  	}
>  	return -ENODEV;
>  }

BTW, is there any reason this stuff is fsl_soc specific?  I'd think 
prom_parse.c (or better yet, drivers/of/ or drivers/i2c/, now that some 
OF calls have been factored out) would be a better place.

-Scott

Re: [PATCH] powerpc: fix i2c device string format

[Guennadi Liakhovetski]

On Wed, 15 Aug 2007, Scott Wood wrote:

> BTW, is there any reason this stuff is fsl_soc specific?  I'd think
> prom_parse.c (or better yet, drivers/of/ or drivers/i2c/, now that some OF
> calls have been factored out) would be a better place.

Minimal intrusion:-) Initially in your patch you added the 
of_register_i2c_devices() function to prom_parse.c, of_find_i2c_driver() 
is just an abstracted out search function, that is practically inlined in 
the former one. I moved of_register_i2c_devices() into fsl_soc.c because 
that's where its only (ATM) caller fsl_i2c_of_init() is. So, this way it's 
all nicely inlined by the compiler, and we don't need any external 
functions... If we get another user of this function, sure, we can move it 
out. So far no other i2c bus driver is trying to parse OF i2c tree.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] powerpc: fix i2c device string format

[Stephen Rothwell]

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

On Wed, 15 Aug 2007 21:15:03 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 727453d..c0d66df 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -320,21 +320,26 @@ static struct i2c_driver_device i2c_devices[] __initdata = {
>  	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
>  };
>  
> -static int __init of_find_i2c_driver(struct device_node *node, struct i2c_board_info *info)
> +static int __init of_find_i2c_driver(struct device_node *node,
> +				     struct i2c_board_info *info)

Please don't do unrelated formatting changes in a patch.

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

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

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Benjamin Herrenschmidt]

On Mon, 2007-07-02 at 14:03 +0200, Segher Boessenkool wrote:
> > +	while ((node = of_get_next_child(adap_node, node))) {
> 
> of_for_each_child_node() or whatever it is called?
> 
> > +		addr = of_get_property(node, "reg", &len);
> > +		if (!addr || len < sizeof(int) || *addr > 0xffff)
> > +			continue;
> 
> Addresses aren't 16 bit AFAIK?

Some i2c busses support 10 bits addressing...

Ben.

Re: [PATCH] powerpc: Add of_register_i2c_devices()

[Segher Boessenkool]

>>> +		addr = of_get_property(node, "reg", &len);
>>> +		if (!addr || len < sizeof(int) || *addr > 0xffff)
>>> +			continue;
>>
>> Addresses aren't 16 bit AFAIK?
>
> Some i2c busses support 10 bits addressing...

All I2C busses.  All I2C devices, too, it's just that
not all of them can decode 10-bit addresses (but they
are fine with seeing those on the bus, they'll just
never be addressed by it themselves).

Anyway, my point was that 10 isn't the same as 16.


Segher