[PATCH] of: i2c: improve last resort compatible entry selection

[PATCH] of: i2c: improve last resort compatible entry selection

[Anton Vorontsov]

Currently of_i2c will select first compatible property as a last resort
option. This isn't best choice though, because generic compatible entries
are listed last, not first. For example, two compatible entries given for
the MCU node:

"fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";

Since no sane driver will ever match specific devices, what we want is
to select most generic option (last). Then driver may call
of_device_is_compatible() if it is really interested in details.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

Other options are: start filling the exceptions list, or add  "linux,..."
compatible entry to the device tree.

 drivers/of/of_i2c.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index b2ccdcb..0d35a0a 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -29,6 +29,7 @@ static int of_find_i2c_driver(struct device_node *node,
 	int i, cplen;
 	const char *compatible;
 	const char *p;
+	const char *last_wcomma = NULL;
 
 	/* 1. search for exception list entry */
 	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
@@ -45,7 +46,7 @@ static int of_find_i2c_driver(struct device_node *node,
 	if (!compatible)
 		return -ENODEV;
 
-	/* 2. search for linux,<i2c-type> entry */
+	/* 2. search for linux,<i2c-type> entry, or remember last w/ comma */
 	p = compatible;
 	while (cplen > 0) {
 		if (!strncmp(p, "linux,", 6)) {
@@ -54,6 +55,12 @@ static int of_find_i2c_driver(struct device_node *node,
 				    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
 				return -ENOMEM;
 			return 0;
+		} else {
+			const char *comma;
+
+			comma = strchr(p, ',');
+			if (comma)
+				last_wcomma = comma + 1;
 		}
 
 		i = strlen(p) + 1;
@@ -61,12 +68,10 @@ static int of_find_i2c_driver(struct device_node *node,
 		cplen -= i;
 	}
 
-	/* 3. take fist compatible entry and strip manufacturer */
-	p = strchr(compatible, ',');
-	if (!p)
+	/* 3. take last compatible entry w/ comma, manufacturer stripped */
+	if (!last_wcomma)
 		return -ENODEV;
-	p++;
-	if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
+	if (strlcpy(info->type, last_wcomma, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
 		return -ENOMEM;
 	return 0;
 }
-- 
1.5.5.4

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jochen Friedrich]

Hi Anton,

> Since no sane driver will ever match specific devices, what we want is
> to select most generic option (last). Then driver may call
> of_device_is_compatible() if it is really interested in details.

My original intention was to have alias entries for specific devices in the
i2c device drivers. Later, Jean decided to only have the most generic names
in there (like in http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;f=drivers/media/video/tvaudio.c;h=c77914d99d15c5972c94e9763a08b5789098e90a;hp=6f9945b04e1f2bcd676f0ed8dc910994b29ed300;hb=ae429083efe996ca2c569c44fd6fea440676dc33;hpb=60b129d7bfa3e20450816983bd52c49bb0bc1c21)
So your patch is correct.

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: Jochen Friedrich <jochen@scram.de>

Thanks,
Jochen

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jon Smirl]

On 7/15/08, Jochen Friedrich <jochen@scram.de> wrote:
> Hi Anton,
>
>
>  > Since no sane driver will ever match specific devices, what we want is
>  > to select most generic option (last). Then driver may call
>  > of_device_is_compatible() if it is really interested in details.
>
>
> My original intention was to have alias entries for specific devices in the
>  i2c device drivers. Later, Jean decided to only have the most generic names
>  in there (like in http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;f=drivers/media/video/tvaudio.c;h=c77914d99d15c5972c94e9763a08b5789098e90a;hp=6f9945b04e1f2bcd676f0ed8dc910994b29ed300;hb=ae429083efe996ca2c569c44fd6fea440676dc33;hpb=60b129d7bfa3e20450816983bd52c49bb0bc1c21)
>  So your patch is correct.

Why aren't we listing the chip names in the driver's id section? Large
chip id tables are not unusual in the kernel, the e1000 driver has
about 70 entries.

Taking the chip id table out of the driver just means we have to build
it somewhere else. It doesn't save space, the tables just get moved.
Device firmware has the chip names in it so there has to be code in
the kernel somewhere to do the mapping from chip id to linux driver
name.

Pushing the linux driver name down into the firmware is even crazier.
+  /* 2. search for linux,<i2c-type> entry */
Now if the linux driver gets renamed everyone on the planet needs a
firmware update.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jean Delvare]

On Tue, 15 Jul 2008 09:40:08 -0400, Jon Smirl wrote:
> On 7/15/08, Jochen Friedrich <jochen@scram.de> wrote:
> > Hi Anton,
> >
> >
> >  > Since no sane driver will ever match specific devices, what we want is
> >  > to select most generic option (last). Then driver may call
> >  > of_device_is_compatible() if it is really interested in details.
> >
> >
> > My original intention was to have alias entries for specific devices in the
> >  i2c device drivers. Later, Jean decided to only have the most generic names
> >  in there (like in http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blobdiff;f=drivers/media/video/tvaudio.c;h=c77914d99d15c5972c94e9763a08b5789098e90a;hp=6f9945b04e1f2bcd676f0ed8dc910994b29ed300;hb=ae429083efe996ca2c569c44fd6fea440676dc33;hpb=60b129d7bfa3e20450816983bd52c49bb0bc1c21)
> >  So your patch is correct.

Eeeek. The patch you mention here is only the conversion of ONE driver.
It is absolutely not relevant as to what the general rule is.

Background:
Some media drivers have been relying on autodetection for years. We
will convert them to the new model as much as possible, however the
knowledge of what chip is present on each board has sometimes been
lost, so switching to the new i2c model isn't that easy. So, as a
transition step, for a few media drivers, we decided to not enumerate
all the supported chips (nobody would be able to instantiate any of
them specifically anyway). This might change in the future, as
developers gain knowledge about the hardware again.

Jochen, I am very surprised that you dare drawing conclusions based on
one random patch of mine. And I am unhappy that you even claim that I
took some decision when I definitely did not.

> Why aren't we listing the chip names in the driver's id section? Large
> chip id tables are not unusual in the kernel, the e1000 driver has
> about 70 entries.
> 
> Taking the chip id table out of the driver just means we have to build
> it somewhere else. It doesn't save space, the tables just get moved.
> Device firmware has the chip names in it so there has to be code in
> the kernel somewhere to do the mapping from chip id to linux driver
> name.
> 
> Pushing the linux driver name down into the firmware is even crazier.
> +  /* 2. search for linux,<i2c-type> entry */
> Now if the linux driver gets renamed everyone on the planet needs a
> firmware update.

I can't comment on the specific issue at hand as I am not familiar with
it, but overall Jon appears to be right. Listing individual chips in
id_table is the standard way to go. That's even the very reason why we
decided to add this id_table to i2c_driver, instead of matching on the
driver name as we were doing before.

-- 
Jean Delvare

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jochen Friedrich]

Hi Jean,

> Eeeek. The patch you mention here is only the conversion of ONE driver.
> It is absolutely not relevant as to what the general rule is.

Sorry, i must have misunderstood you then.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=af294867a52bf718df835a688e8c786d550bee26#patch9
is the same, my original patch listed all four supported chips in there
(saa7126, saa7127, saa7128 and saa7129) while only one made it into the driver...

> Jochen, I am very surprised that you dare drawing conclusions based on
> one random patch of mine. And I am unhappy that you even claim that I
> took some decision when I definitely did not.

Maybe I draw wrong conclusions from the discussion with Jon Smirl then.

> I can't comment on the specific issue at hand as I am not familiar with
> it, but overall Jon appears to be right. Listing individual chips in
> id_table is the standard way to go. That's even the very reason why we
> decided to add this id_table to i2c_driver, instead of matching on the
> driver name as we were doing before.

I definitely agree here.

Thanks,
Jochen

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jean Delvare]

On Tue, 15 Jul 2008 16:52:01 +0200, Jochen Friedrich wrote:
> Hi Jean,
> 
> > Eeeek. The patch you mention here is only the conversion of ONE driver.
> > It is absolutely not relevant as to what the general rule is.
> 
> Sorry, i must have misunderstood you then.
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=af294867a52bf718df835a688e8c786d550bee26#patch9
> is the same, my original patch listed all four supported chips in there
> (saa7126, saa7127, saa7128 and saa7129) while only one made it into the driver...

As I recall, your patch was done quickly and without knowledge of the
chips in question. I did mine in close collaboration with Hans Verkuil
who knows these chips very well, to make sure I wouldn't break
anything. With success, as far as I can tell.

Honestly, I can't remember why we decided to have a single chip name
for all 4 variants. It might have been a shortcut to complete the
conversion in time. Or, more likely, I didn't notice the other types
because the driver was originally using the same name "saa7127" for all
devices. If that is the case I'll update the driver to behave more in
compliance with the new i2c device/driver matching scheme. I'll discuss
this with Hans to make sure it's OK.

So, again, please don't take this (nor any other) media driver
conversion patch as an example of what should be done. The proper
conversion of all media drivers will take a lot of time because of the
history behind these drivers.

-- 
Jean Delvare

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Grant Likely]

On Mon, Jul 14, 2008 at 09:54:37PM +0400, Anton Vorontsov wrote:
> Currently of_i2c will select first compatible property as a last resort
> option. This isn't best choice though, because generic compatible entries
> are listed last, not first. For example, two compatible entries given for
> the MCU node:
> 
> "fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";
> 
> Since no sane driver will ever match specific devices, what we want is
> to select most generic option (last). Then driver may call
> of_device_is_compatible() if it is really interested in details.
 
I highly suspect that this will actually be a rare condition and that
most of the time the driver you want will bind against the first entry
in the list (I'm basing this on what discussion I've seen on the list
and it seems to me that Jiri does want i2c devices to list the exact set
of chips that each driver binds against).

I'm inclined to leave it as is for now and instead handle the mpc837x
case with the fixup table in this file.  (Actually, this function has
been moved to of/base.c; so handle it in that function)

For the longer term, I think that this whole match method is a stop gap
solution until we figure out a tidy way to bind and provide device tree
data to i2c, SPI and platform devices.

g.

> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> Other options are: start filling the exceptions list, or add  "linux,..."
> compatible entry to the device tree.
> 
>  drivers/of/of_i2c.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index b2ccdcb..0d35a0a 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -29,6 +29,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  	int i, cplen;
>  	const char *compatible;
>  	const char *p;
> +	const char *last_wcomma = NULL;
>  
>  	/* 1. search for exception list entry */
>  	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> @@ -45,7 +46,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  	if (!compatible)
>  		return -ENODEV;
>  
> -	/* 2. search for linux,<i2c-type> entry */
> +	/* 2. search for linux,<i2c-type> entry, or remember last w/ comma */
>  	p = compatible;
>  	while (cplen > 0) {
>  		if (!strncmp(p, "linux,", 6)) {
> @@ -54,6 +55,12 @@ static int of_find_i2c_driver(struct device_node *node,
>  				    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  				return -ENOMEM;
>  			return 0;
> +		} else {
> +			const char *comma;
> +
> +			comma = strchr(p, ',');
> +			if (comma)
> +				last_wcomma = comma + 1;
>  		}
>  
>  		i = strlen(p) + 1;
> @@ -61,12 +68,10 @@ static int of_find_i2c_driver(struct device_node *node,
>  		cplen -= i;
>  	}
>  
> -	/* 3. take fist compatible entry and strip manufacturer */
> -	p = strchr(compatible, ',');
> -	if (!p)
> +	/* 3. take last compatible entry w/ comma, manufacturer stripped */
> +	if (!last_wcomma)
>  		return -ENODEV;
> -	p++;
> -	if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
> +	if (strlcpy(info->type, last_wcomma, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  		return -ENOMEM;
>  	return 0;
>  }
> -- 
> 1.5.5.4

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jon Smirl]

On 7/26/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jul 14, 2008 at 09:54:37PM +0400, Anton Vorontsov wrote:
>  > Currently of_i2c will select first compatible property as a last resort
>  > option. This isn't best choice though, because generic compatible entries
>  > are listed last, not first. For example, two compatible entries given for
>  > the MCU node:
>  >
>  > "fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";
>  >
>  > Since no sane driver will ever match specific devices, what we want is
>  > to select most generic option (last). Then driver may call
>  > of_device_is_compatible() if it is really interested in details.
>
>
> I highly suspect that this will actually be a rare condition and that
>  most of the time the driver you want will bind against the first entry
>  in the list (I'm basing this on what discussion I've seen on the list
>  and it seems to me that Jiri does want i2c devices to list the exact set
>  of chips that each driver binds against).

Can we put a loop on request_module() and have it try each one down
the list until something matches? request_module() returns errors, but
I can't tell from the source if one of those errors is "no matching
module found" since it invokes a user space helper.

That would work for this compatible string:
compatible = "atmel,24c32wp", "24c32", "eeprom";

request_module will always fail for the first entry. If you have at24
in your system the second one will succeed. If you have eeprom the
third one works. All of those names are valid in a device tree.

If all three fail, create a device with the last entry since someone
may modprobe the driver in later.

>  I'm inclined to leave it as is for now and instead handle the mpc837x
>  case with the fixup table in this file.  (Actually, this function has
>  been moved to of/base.c; so handle it in that function)
>
>  For the longer term, I think that this whole match method is a stop gap
>  solution until we figure out a tidy way to bind and provide device tree
>  data to i2c, SPI and platform devices.
>
>
>  g.
>
>  >
>  > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>  > ---
>  >
>  > Other options are: start filling the exceptions list, or add  "linux,..."
>  > compatible entry to the device tree.
>  >
>  >  drivers/of/of_i2c.c |   17 +++++++++++------
>  >  1 files changed, 11 insertions(+), 6 deletions(-)
>  >
>  > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>  > index b2ccdcb..0d35a0a 100644
>  > --- a/drivers/of/of_i2c.c
>  > +++ b/drivers/of/of_i2c.c
>  > @@ -29,6 +29,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  >       int i, cplen;
>  >       const char *compatible;
>  >       const char *p;
>  > +     const char *last_wcomma = NULL;
>  >
>  >       /* 1. search for exception list entry */
>  >       for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
>  > @@ -45,7 +46,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  >       if (!compatible)
>  >               return -ENODEV;
>  >
>  > -     /* 2. search for linux,<i2c-type> entry */
>  > +     /* 2. search for linux,<i2c-type> entry, or remember last w/ comma */
>  >       p = compatible;
>  >       while (cplen > 0) {
>  >               if (!strncmp(p, "linux,", 6)) {
>  > @@ -54,6 +55,12 @@ static int of_find_i2c_driver(struct device_node *node,
>  >                                   I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  >                               return -ENOMEM;
>  >                       return 0;
>  > +             } else {
>  > +                     const char *comma;
>  > +
>  > +                     comma = strchr(p, ',');
>  > +                     if (comma)
>  > +                             last_wcomma = comma + 1;
>  >               }
>  >
>  >               i = strlen(p) + 1;
>  > @@ -61,12 +68,10 @@ static int of_find_i2c_driver(struct device_node *node,
>  >               cplen -= i;
>  >       }
>  >
>  > -     /* 3. take fist compatible entry and strip manufacturer */
>  > -     p = strchr(compatible, ',');
>  > -     if (!p)
>  > +     /* 3. take last compatible entry w/ comma, manufacturer stripped */
>  > +     if (!last_wcomma)
>  >               return -ENODEV;
>  > -     p++;
>  > -     if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  > +     if (strlcpy(info->type, last_wcomma, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  >               return -ENOMEM;
>  >       return 0;
>  >  }
>  > --
>  > 1.5.5.4
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Grant Likely]

On Sun, Jul 27, 2008 at 1:05 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/26/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Mon, Jul 14, 2008 at 09:54:37PM +0400, Anton Vorontsov wrote:
>>  > Currently of_i2c will select first compatible property as a last resort
>>  > option. This isn't best choice though, because generic compatible entries
>>  > are listed last, not first. For example, two compatible entries given for
>>  > the MCU node:
>>  >
>>  > "fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";
>>  >
>>  > Since no sane driver will ever match specific devices, what we want is
>>  > to select most generic option (last). Then driver may call
>>  > of_device_is_compatible() if it is really interested in details.
>>
>>
>> I highly suspect that this will actually be a rare condition and that
>>  most of the time the driver you want will bind against the first entry
>>  in the list (I'm basing this on what discussion I've seen on the list
>>  and it seems to me that Jiri does want i2c devices to list the exact set
>>  of chips that each driver binds against).
>
> Can we put a loop on request_module() and have it try each one down
> the list until something matches? request_module() returns errors, but
> I can't tell from the source if one of those errors is "no matching
> module found" since it invokes a user space helper.

What will request_module() do if the modules is compiled in
statically?  If it is workable then I'm not opposed to this approach.

> That would work for this compatible string:
> compatible = "atmel,24c32wp", "24c32", "eeprom";
>
> request_module will always fail for the first entry. If you have at24
> in your system the second one will succeed. If you have eeprom the
> third one works. All of those names are valid in a device tree.

I know this is just an example; but to keep thinks clear, the second
and third values in this compatible property are completely bogus (for
device trees).  The manufacturer prefix needs to be present and
'eeprom' is far to vague.

g.


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

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jon Smirl]

On 7/27/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sun, Jul 27, 2008 at 1:05 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
>  > On 7/26/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>  >> On Mon, Jul 14, 2008 at 09:54:37PM +0400, Anton Vorontsov wrote:
>  >>  > Currently of_i2c will select first compatible property as a last resort
>  >>  > option. This isn't best choice though, because generic compatible entries
>  >>  > are listed last, not first. For example, two compatible entries given for
>  >>  > the MCU node:
>  >>  >
>  >>  > "fsl,mc9s08qg8-mpc837xrdb", "fsl,mcu-mpc8349emitx";
>  >>  >
>  >>  > Since no sane driver will ever match specific devices, what we want is
>  >>  > to select most generic option (last). Then driver may call
>  >>  > of_device_is_compatible() if it is really interested in details.
>  >>
>  >>
>  >> I highly suspect that this will actually be a rare condition and that
>  >>  most of the time the driver you want will bind against the first entry
>  >>  in the list (I'm basing this on what discussion I've seen on the list
>  >>  and it seems to me that Jiri does want i2c devices to list the exact set
>  >>  of chips that each driver binds against).
>  >
>  > Can we put a loop on request_module() and have it try each one down
>  > the list until something matches? request_module() returns errors, but
>  > I can't tell from the source if one of those errors is "no matching
>  > module found" since it invokes a user space helper.
>
>
> What will request_module() do if the modules is compiled in
>  statically?  If it is workable then I'm not opposed to this approach.

I'm no expert on request_module, from the comments:

It appears to be synchronous with user space...
	call_usermodehelper wait flag, and remove exec_usermodehelper.
	Rusty Russell <rusty@rustcorp.com.au>  Jan 2003

 * Load a module using the user mode module loader. The function returns
 * zero on success or a negative errno code on failure. Note that a
 * successful module load does not mean the module did not then unload
 * and exit on an error of its own. Callers must check that the service
 * they requested is now available not blindly invoke it.

Is this really a problem? If the specific driver gets loaded and then
errors out, then something must be wrong. The appropriate action
probably should not be to load the next driver on the list.

It looks like it should work. Compiled in moduled and modules that are
already loaded are essentially the same so they should return 0 from
request module.

>
>
>  > That would work for this compatible string:
>  > compatible = "atmel,24c32wp", "24c32", "eeprom";
>  >
>  > request_module will always fail for the first entry. If you have at24
>  > in your system the second one will succeed. If you have eeprom the
>  > third one works. All of those names are valid in a device tree.
>
>
> I know this is just an example; but to keep thinks clear, the second
>  and third values in this compatible property are completely bogus (for
>  device trees).  The manufacturer prefix needs to be present and
>  'eeprom' is far to vague.

Isn't 24c32 a generic, cross manufacturer term used for these devices?
What if I have a socket and use a different vendor's chip each week?

eeprom is the vague Linuxism that at24 is attempting to correct.
eeprom just goes and searches for anything resembling an eeprom. It
will trigger on chips that aren't eeproms.

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


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Segher Boessenkool]

>>> compatible = "atmel,24c32wp", "24c32", "eeprom";

>> I know this is just an example; but to keep thinks clear, the second
>>  and third values in this compatible property are completely bogus 
>> (for
>>  device trees).  The manufacturer prefix needs to be present and
>>  'eeprom' is far to vague.
>
> Isn't 24c32 a generic, cross manufacturer term used for these devices?

Sure it is.  But "compatible" values are a global namespace so care
needs to be taken not to cause collisions.  One mechanism for that
is to use vendor prefixes (and that just shifts the problem so it
is less global); another is to choose good names that have a lower
chance to collide with the name for another device.  And the most
important way to prevent collisions is to write up a binding, so
everyone knows you have claimed that name.  It still needs to be
a good name, of course.

> What if I have a socket and use a different vendor's chip each week?

You use sockets for your seeproms?  Wow :-)  But yes, it shouldn't
be necessary to put the exact make of the device in the device
tree, for such generic devices.  It certainly doesn't hurt to do
so though (if the exact model is known).

A reasonable "compatible" value would be something like 
"serial-eeprom-24c32".
You can go a little bit more generic than that, if you write up in
your binding how the driver should figure out the device size and
the protocol used.

> eeprom is the vague Linuxism that at24 is attempting to correct.
> eeprom just goes and searches for anything resembling an eeprom. It
> will trigger on chips that aren't eeproms.

Yeah.  And no driver should need to probe _anything_ if it has a
device tree node describing the device -- certainly it shouldn't
probe for what kind of device it is!


Segher

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jon Smirl]

On 7/27/08, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> >
> > >
> > > > compatible = "atmel,24c32wp", "24c32", "eeprom";
> > > >
> > >
> >
>
>
> >
> > > I know this is just an example; but to keep thinks clear, the second
> > >  and third values in this compatible property are completely bogus (for
> > >  device trees).  The manufacturer prefix needs to be present and
> > >  'eeprom' is far to vague.
> > >
> >
> > Isn't 24c32 a generic, cross manufacturer term used for these devices?
> >
>
>  Sure it is.  But "compatible" values are a global namespace so care
>  needs to be taken not to cause collisions.  One mechanism for that
>  is to use vendor prefixes (and that just shifts the problem so it
>  is less global); another is to choose good names that have a lower
>  chance to collide with the name for another device.  And the most
>  important way to prevent collisions is to write up a binding, so
>  everyone knows you have claimed that name.  It still needs to be
>  a good name, of course.
>
>
> > What if I have a socket and use a different vendor's chip each week?
> >
>
>  You use sockets for your seeproms?  Wow :-)  But yes, it shouldn't
>  be necessary to put the exact make of the device in the device
>  tree, for such generic devices.  It certainly doesn't hurt to do
>  so though (if the exact model is known).
>
>  A reasonable "compatible" value would be something like
> "serial-eeprom-24c32".
>  You can go a little bit more generic than that, if you write up in
>  your binding how the driver should figure out the device size and
>  the protocol used.

Matching on "serial-eeprom-24c32" requires me to convince the at24
authors to add that string as an alias binding for their driver. How
about "serial-eeprom,24c32" or "generic,24x32"?

>
> > eeprom is the vague Linuxism that at24 is attempting to correct.
> > eeprom just goes and searches for anything resembling an eeprom. It
> > will trigger on chips that aren't eeproms.
> >
>
>  Yeah.  And no driver should need to probe _anything_ if it has a
>  device tree node describing the device -- certainly it shouldn't
>  probe for what kind of device it is!
>
>
>  Segher
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[M. Warner Losh]

In message: <9e4733910807271500l23fd2b12n940197474a5291df@mail.gmail.com>
            "Jon Smirl" <jonsmirl@gmail.com> writes:
: On 7/27/08, Segher Boessenkool <segher@kernel.crashing.org> wrote:
: > >
: > > >
: > > > > compatible = "atmel,24c32wp", "24c32", "eeprom";
: > > > >
: > > >
: > >
: >
: >
: > >
: > > > I know this is just an example; but to keep thinks clear, the second
: > > >  and third values in this compatible property are completely bogus (for
: > > >  device trees).  The manufacturer prefix needs to be present and
: > > >  'eeprom' is far to vague.
: > > >
: > >
: > > Isn't 24c32 a generic, cross manufacturer term used for these devices?
: > >
: >
: >  Sure it is.  But "compatible" values are a global namespace so care
: >  needs to be taken not to cause collisions.  One mechanism for that
: >  is to use vendor prefixes (and that just shifts the problem so it
: >  is less global); another is to choose good names that have a lower
: >  chance to collide with the name for another device.  And the most
: >  important way to prevent collisions is to write up a binding, so
: >  everyone knows you have claimed that name.  It still needs to be
: >  a good name, of course.
: >
: >
: > > What if I have a socket and use a different vendor's chip each week?
: > >
: >
: >  You use sockets for your seeproms?  Wow :-)  But yes, it shouldn't
: >  be necessary to put the exact make of the device in the device
: >  tree, for such generic devices.  It certainly doesn't hurt to do
: >  so though (if the exact model is known).
: >
: >  A reasonable "compatible" value would be something like
: > "serial-eeprom-24c32".
: >  You can go a little bit more generic than that, if you write up in
: >  your binding how the driver should figure out the device size and
: >  the protocol used.
: 
: Matching on "serial-eeprom-24c32" requires me to convince the at24
: authors to add that string as an alias binding for their driver. How
: about "serial-eeprom,24c32" or "generic,24x32"?

Many of the serial eeproms have a common way to access them.  There's
a few organizations of eeproms made by a number of different
manufacturers that are actually accessed the same.

Warner

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Segher Boessenkool]

>>  A reasonable "compatible" value would be something like
>> "serial-eeprom-24c32".
>>  You can go a little bit more generic than that, if you write up in
>>  your binding how the driver should figure out the device size and
>>  the protocol used.
>
> Matching on "serial-eeprom-24c32" requires me to convince the at24
> authors to add that string as an alias binding for their driver.

No, it requires the IIC subsystem to get fixed and not use OF
"compatible" values as module alias names.

> How
> about "serial-eeprom,24c32" or "generic,24x32"?

Neither "serial-eeprom" nor "generic" is the name of a vendor, so
no.  The comma has a well-defined meaning.  Why would a comma be
easier than a dash for your device matching code, anyway?


Segher

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Grant Likely]

On Mon, Jul 28, 2008 at 09:47:21AM +0200, Segher Boessenkool wrote:
>>>  A reasonable "compatible" value would be something like
>>> "serial-eeprom-24c32".
>>>  You can go a little bit more generic than that, if you write up in
>>>  your binding how the driver should figure out the device size and
>>>  the protocol used.
>>
>> Matching on "serial-eeprom-24c32" requires me to convince the at24
>> authors to add that string as an alias binding for their driver.
>
> No, it requires the IIC subsystem to get fixed and not use OF
> "compatible" values as module alias names.

Indeed; the device tree is just a data structure with a well defined
usage model.  It is the kernel's job to adapt that data into a form that
it can use.

>> How
>> about "serial-eeprom,24c32" or "generic,24x32"?
>
> Neither "serial-eeprom" nor "generic" is the name of a vendor, so
> no.  The comma has a well-defined meaning.  Why would a comma be
> easier than a dash for your device matching code, anyway?

Just to add my voice; I 100% agree.  If it is not documented, and it
doesn't fit with established conventions, then it shouldn't be used in
the compatible field.

g.

Re: [PATCH] of: i2c: improve last resort compatible entry selection

[Jon Smirl]

On 7/30/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jul 28, 2008 at 09:47:21AM +0200, Segher Boessenkool wrote:
>  >>>  A reasonable "compatible" value would be something like
>  >>> "serial-eeprom-24c32".
>  >>>  You can go a little bit more generic than that, if you write up in
>  >>>  your binding how the driver should figure out the device size and
>  >>>  the protocol used.
>  >>
>  >> Matching on "serial-eeprom-24c32" requires me to convince the at24
>  >> authors to add that string as an alias binding for their driver.
>  >
>  > No, it requires the IIC subsystem to get fixed and not use OF
>  > "compatible" values as module alias names.
>
>
> Indeed; the device tree is just a data structure with a well defined
>  usage model.  It is the kernel's job to adapt that data into a form that
>  it can use.

Then we're going to have to work on the i2s subsystem more to get them
to allow arbitrary modalias strings like serial-eeprom-24c32.

The current i2c code has linked the use of modalias strings and the
i2c sysfs attribute  'name'. Currently those two always need to be the
same. Existing user space apps are expecting to get the linux name for
the device from the name field.  That linkage needs to be broken.

Then you need entries like this:

static const struct i2c_device_id at24_ids[] = {
	{ "24c01", "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
	{ "24c02", "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
	OF( "serial-eeprom-24c01", "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) ),
	OF( "serial-eeprom-24c02", "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) ),

First column is the modalias, second is the string that is going into
the 'name' attribute. OF() causes the entries to disappear on non-OF
platforms.

We should argue for another macro that makes the non-OF strings
disappear on our platform.

I have submitted a patch like this before and Jean declared that he
doesn't recognize open firmware as a naming authority and NACK'd it.


>
>
>  >> How
>  >> about "serial-eeprom,24c32" or "generic,24x32"?
>  >
>  > Neither "serial-eeprom" nor "generic" is the name of a vendor, so
>  > no.  The comma has a well-defined meaning.  Why would a comma be
>  > easier than a dash for your device matching code, anyway?
>
>
> Just to add my voice; I 100% agree.  If it is not documented, and it
>  doesn't fit with established conventions, then it shouldn't be used in
>  the compatible field.
>
>
>  g.
>


-- 
Jon Smirl
jonsmirl@gmail.com