Re: [PATCH 0/5] Version 17, series to add device tree naming to i2c

Re: [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jon Smirl]

On 12/19/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> Another rework of the i2c for powerpc device tree patch. This version implements standard alias naming only on the powerpc platform and only for the device tree names. The old naming mechanism of i2c_client.name,driver_name is left in place and not changed for non-powerpc platforms. This patch is fully capable of dynamically loading the i2c modules. You can modprobe in the i2c-mpc driver and the i2c modules described in the device tree will be automatically loaded. Modules also work if compiled in.


Are there any further objections to this patch? Can it all go in
through the powerpc trees or do the i2c people want to send it on?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jean Delvare]

Hi John,

Le 27/12/2007, Jon Smirl écrit:
>On 12/19/07, Jon Smirl <jonsmirl@gmail.com> wrote:
>> Another rework of the i2c for powerpc device tree patch. This version implements standard alias naming only on the powerpc platform and only for the device tree names. The old naming mechanism of i2c_client.name,driver_name is left in place and not changed for non-powerpc platforms. This patch is fully capable of dynamically loading the i2c modules. You can modprobe in the i2c-mpc driver and the i2c modules described in the device tree will be automatically loaded. Modules also work if compiled in.
>
>Are there any further objections to this patch? Can it all go in
>through the powerpc trees or do the i2c people want to send it on?

I am on vacation until January 4th. I will review your patchset the week
following my return.

Thanks for your patience,
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jon Smirl]

What is the review status of this series, should I post it again?

On 12/19/07, Jon Smirl <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Since copying i2c-mpc.c to maintain support for the ppc architecture seems to be an issue; instead rework i2c-mpc.c to use CONFIG_PPC_MERGE #ifdefs to support both the ppc and powerpc architecture. When ppc is deleted in six months these #ifdefs will need to be removed.
>
> Another rework of the i2c for powerpc device tree patch. This version implements standard alias naming only on the powerpc platform and only for the device tree names. The old naming mechanism of i2c_client.name,driver_name is left in place and not changed for non-powerpc platforms. This patch is fully capable of dynamically loading the i2c modules. You can modprobe in the i2c-mpc driver and the i2c modules described in the device tree will be automatically loaded. Modules also work if compiled in.
>
> The follow on patch to module-init-tools is also needed since the i2c subsystem has never implemented dynamic loading.
>
> The following series implements standard linux module aliasing for i2c modules on arch=powerpc. It then converts the mpc i2c driver from being a platform driver to an open firmware one. I2C device names are picked up from the device tree. Module aliasing is used to translate from device tree names into to linux kernel names. Several i2c drivers are updated to use the new aliasing.
>
> --
> Jon Smirl
> jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jean Delvare]

On Thu, 10 Jan 2008 09:14:26 -0500, Jon Smirl wrote:
> What is the review status of this series, should I post it again?

No please! /o\

I'll go through your numerous past posts now, stay tuned.

-- 
Jean Delvare

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jean Delvare]

Hi Jon,

On Wed, 19 Dec 2007 23:41:36 -0500, Jon Smirl wrote:
> Since copying i2c-mpc.c to maintain support for the ppc architecture seems to be an issue; instead rework i2c-mpc.c to use CONFIG_PPC_MERGE #ifdefs to support both the ppc and powerpc architecture. When ppc is deleted in six months these #ifdefs will need to be removed.
> 
> Another rework of the i2c for powerpc device tree patch. This version implements standard alias naming only on the powerpc platform and only for the device tree names. The old naming mechanism of i2c_client.name,driver_name is left in place and not changed for non-powerpc platforms. This patch is fully capable of dynamically loading the i2c modules. You can modprobe in the i2c-mpc driver and the i2c modules described in the device tree will be automatically loaded. Modules also work if compiled in.
> 
> The follow on patch to module-init-tools is also needed since the i2c subsystem has never implemented dynamic loading.
> 
> The following series implements standard linux module aliasing for i2c modules on arch=powerpc. It then converts the mpc i2c driver from being a platform driver to an open firmware one. I2C device names are picked up from the device tree. Module aliasing is used to translate from device tree names into to linux kernel names. Several i2c drivers are updated to use the new aliasing. 

Now that I have read all the previous versions of this patch series
and, more importantly, all objections that were raised on the way, I
can start reviewing the latest iteration of your patches. I'll also do
some testing, although I have no powerpc stuff here, but at least I
want to make sure that there are no regressions introduced by your
patches on x86.

-- 
Jean Delvare

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jon Smirl]

On 1/11/08, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Jon,
>
> On Wed, 19 Dec 2007 23:41:36 -0500, Jon Smirl wrote:
> > Since copying i2c-mpc.c to maintain support for the ppc architecture seems to be an issue; instead rework i2c-mpc.c to use CONFIG_PPC_MERGE #ifdefs to support both the ppc and powerpc architecture. When ppc is deleted in six months these #ifdefs will need to be removed.
> >
> > Another rework of the i2c for powerpc device tree patch. This version implements standard alias naming only on the powerpc platform and only for the device tree names. The old naming mechanism of i2c_client.name,driver_name is left in place and not changed for non-powerpc platforms. This patch is fully capable of dynamically loading the i2c modules. You can modprobe in the i2c-mpc driver and the i2c modules described in the device tree will be automatically loaded. Modules also work if compiled in.
> >
> > The follow on patch to module-init-tools is also needed since the i2c subsystem has never implemented dynamic loading.
> >
> > The following series implements standard linux module aliasing for i2c modules on arch=powerpc. It then converts the mpc i2c driver from being a platform driver to an open firmware one. I2C device names are picked up from the device tree. Module aliasing is used to translate from device tree names into to linux kernel names. Several i2c drivers are updated to use the new aliasing.
>
> Now that I have read all the previous versions of this patch series
> and, more importantly, all objections that were raised on the way, I
> can start reviewing the latest iteration of your patches. I'll also do
> some testing, although I have no powerpc stuff here, but at least I
> want to make sure that there are no regressions introduced by your
> patches on x86.


Various people were worried about x86. Around version 15 I altered the
patches so that they only impacted PowerPC. If they impact x86 in
current form that is a bug.

When x86 is ready for it I do think dynamic module loading should be
implemented there also.
>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jochen Friedrich]

Hi Jon,

>>> The following series implements standard linux module aliasing for i2c modules on arch=powerpc. It then converts the mpc i2c driver from being a platform driver to an open firmware one. I2C device names are picked up from the device tree. Module aliasing is used to translate from device tree names into to linux kernel names. Several i2c drivers are updated to use the new aliasing.

>> Now that I have read all the previous versions of this patch series
>> and, more importantly, all objections that were raised on the way, I
>> can start reviewing the latest iteration of your patches. I'll also do
>> some testing, although I have no powerpc stuff here, but at least I
>> want to make sure that there are no regressions introduced by your
>> patches on x86.

> Various people were worried about x86. Around version 15 I altered the
> patches so that they only impacted PowerPC. If they impact x86 in
> current form that is a bug.

I can only second this. The latest version of i2c-cpm
(http://patchwork.ozlabs.org/linuxppc/patch?person=1023&id=15902) makes
use of this patch, as well. On the dbox2, loading and unloading of
modules in any order just works fine.

Thanks,
Jochen

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jean Delvare]

On Fri, 11 Jan 2008 10:52:56 -0500, Jon Smirl wrote:
> On 1/11/08, Jean Delvare wrote:
> > Now that I have read all the previous versions of this patch series
> > and, more importantly, all objections that were raised on the way, I
> > can start reviewing the latest iteration of your patches. I'll also do
> > some testing, although I have no powerpc stuff here, but at least I
> > want to make sure that there are no regressions introduced by your
> > patches on x86.
> 
> 
> Various people were worried about x86. Around version 15 I altered the
> patches so that they only impacted PowerPC. If they impact x86 in
> current form that is a bug.
> 
> When x86 is ready for it I do think dynamic module loading should be
> implemented there also.

I agree, and I am doing some testing on x86 to make sure that your
patch will work fine there as well once we decide to go that way.

Your patch set really contains two different parts which should be
clearly identified and discussed separately. Firstly, it lets i2c
drivers export module aliases so that the rest of the world knows which
devices they support. This part I think everybody agrees is needed, so
that platform code no longer needs to specify the driver name for every
I2C device.

Secondly, it promotes OF device names as acceptable aliases. This I
don't think I agree with. While I see some value in moving the OF name
-> Linux name translation to the drivers themselves (even though I
don't see this as a mandatory move either), this doesn't imply that OF
names should be used as aliases. I don't like the idea that different
architectures will name the same device differently in a visible way.
This could easily break user-space code that makes assumptions on the
device names (libsensors comes to mind.) So, I think that this part
will need some more discussion.

-- 
Jean Delvare

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jon Smirl]

On 1/11/08, Jean Delvare <khali@linux-fr.org> wrote:
> Secondly, it promotes OF device names as acceptable aliases. This I
> don't think I agree with. While I see some value in moving the OF name
> -> Linux name translation to the drivers themselves (even though I
> don't see this as a mandatory move either), this doesn't imply that OF
> names should be used as aliases. I don't like the idea that different
> architectures will name the same device differently in a visible way.
> This could easily break user-space code that makes assumptions on the
> device names (libsensors comes to mind.) So, I think that this part
> will need some more discussion.

They're aliases.  On the x86 my e1000 Ethernet driver loads using this
alias name:
"pci:v00008086d00001010sv*sd*bc*sc*i*"
In fact, the e1000 driver has 63 alias names in addition to "e1000"

But it's still the e1000 driver after it is loaded.
jonsmirl@terra:/home/linux/drivers/net/e1000$ lsmod | grep e1000
e1000                 115968  0

Loading a I2C driver with an OF alias name is not going to change the
module name after it is loaded. In fact, once the module is in memory
there's no way to tell what name was used to load it.

OF device names are set by the Open Firmware committee. It is not
reasonable to force the Linux names back into Open Firmware since this
would force the other operating systems using Open Firmware to adopt
the Linux names.

This issue hasn't been visible before since there was a global table
in the PowerPC code mapping all known Open Firmware names into linux
names. Keeping this as a global table doesn't scale. The mapping needs
to be done by each device individually.

>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jean Delvare]

On Fri, 11 Jan 2008 15:16:57 -0500, Jon Smirl wrote:
> On 1/11/08, Jean Delvare <khali@linux-fr.org> wrote:
> > Secondly, it promotes OF device names as acceptable aliases. This I
> > don't think I agree with. While I see some value in moving the OF name
> > -> Linux name translation to the drivers themselves (even though I
> > don't see this as a mandatory move either), this doesn't imply that OF
> > names should be used as aliases. I don't like the idea that different
> > architectures will name the same device differently in a visible way.
> > This could easily break user-space code that makes assumptions on the
> > device names (libsensors comes to mind.) So, I think that this part
> > will need some more discussion.
> 
> They're aliases.  On the x86 my e1000 Ethernet driver loads using this
> alias name:
> "pci:v00008086d00001010sv*sd*bc*sc*i*"
> In fact, the e1000 driver has 63 alias names in addition to "e1000"
> 
> But it's still the e1000 driver after it is loaded.
> jonsmirl@terra:/home/linux/drivers/net/e1000$ lsmod | grep e1000
> e1000                 115968  0
> 
> Loading a I2C driver with an OF alias name is not going to change the
> module name after it is loaded. In fact, once the module is in memory
> there's no way to tell what name was used to load it.

Of course. That's not what I was worried about... what I was worried
about is something your patch set doesn't do but I misread the code and
I thought it was doing. I'll read it again before I make more comments
on this.

> OF device names are set by the Open Firmware committee. It is not
> reasonable to force the Linux names back into Open Firmware since this
> would force the other operating systems using Open Firmware to adopt
> the Linux names.

I never meant to force the Linux names into Open Firmware. It wouldn't
make sense especially when the Linux names are invented by random
contributors with no specific rules, and can even change over time.

What I meant is that the translation from Open Firmware device name to
Linux device name could happen in different ways. Making module aliases
out of the is one possibility but this is not the only one.

I am curious why the translation could not happen "offline". As I
understand it, you're getting the device names from these .dts files.
However you're not parsing them in the kernel directly, are you? I
presume that you have some tool that converts these files into C code
that the kernel can use? This conversion tool could translate the names.

> This issue hasn't been visible before since there was a global table
> in the PowerPC code mapping all known Open Firmware names into linux
> names. Keeping this as a global table doesn't scale. The mapping needs
> to be done by each device individually.

Looking at your patch set, I see only 11 entries in the table (in
arch/powerpc/sysdev/fsl_soc.c) that patch #2 deletes. Are there more in
other files? I'm asking because 11 entries hardly qualifies as "doesn't
scale". I sure hope that you're not doing all this for the sole purpose
of getting rid of this 11-element table.

-- 
Jean Delvare

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jon Smirl]

On 1/12/08, Jean Delvare <khali@linux-fr.org> wrote:
> On Fri, 11 Jan 2008 15:16:57 -0500, Jon Smirl wrote:
> > On 1/11/08, Jean Delvare <khali@linux-fr.org> wrote:
> > > Secondly, it promotes OF device names as acceptable aliases. This I
> > > don't think I agree with. While I see some value in moving the OF name
> > > -> Linux name translation to the drivers themselves (even though I
> > > don't see this as a mandatory move either), this doesn't imply that OF
> > > names should be used as aliases. I don't like the idea that different
> > > architectures will name the same device differently in a visible way.
> > > This could easily break user-space code that makes assumptions on the
> > > device names (libsensors comes to mind.) So, I think that this part
> > > will need some more discussion.
> >
> > They're aliases.  On the x86 my e1000 Ethernet driver loads using this
> > alias name:
> > "pci:v00008086d00001010sv*sd*bc*sc*i*"
> > In fact, the e1000 driver has 63 alias names in addition to "e1000"
> >
> > But it's still the e1000 driver after it is loaded.
> > jonsmirl@terra:/home/linux/drivers/net/e1000$ lsmod | grep e1000
> > e1000                 115968  0
> >
> > Loading a I2C driver with an OF alias name is not going to change the
> > module name after it is loaded. In fact, once the module is in memory
> > there's no way to tell what name was used to load it.
>
> Of course. That's not what I was worried about... what I was worried
> about is something your patch set doesn't do but I misread the code and
> I thought it was doing. I'll read it again before I make more comments
> on this.
>
> > OF device names are set by the Open Firmware committee. It is not
> > reasonable to force the Linux names back into Open Firmware since this
> > would force the other operating systems using Open Firmware to adopt
> > the Linux names.
>
> I never meant to force the Linux names into Open Firmware. It wouldn't
> make sense especially when the Linux names are invented by random
> contributors with no specific rules, and can even change over time.
>
> What I meant is that the translation from Open Firmware device name to
> Linux device name could happen in different ways. Making module aliases
> out of the is one possibility but this is not the only one.
>
> I am curious why the translation could not happen "offline". As I
> understand it, you're getting the device names from these .dts files.
> However you're not parsing them in the kernel directly, are you? I
> presume that you have some tool that converts these files into C code
> that the kernel can use? This conversion tool could translate the names.

Those dts files are for embedded devices that were specifically
developed for Linux. All of the PowerPC Macs in the world have a
device tree in ROM that was developed by Apple following the Open
Firmware standard. Same thing for Sun boxes, but I'm not working on
those.

The kernel has an existing mechanism for handling translations like
these, the alias scheme.


> > This issue hasn't been visible before since there was a global table
> > in the PowerPC code mapping all known Open Firmware names into linux
> > names. Keeping this as a global table doesn't scale. The mapping needs
> > to be done by each device individually.
>
> Looking at your patch set, I see only 11 entries in the table (in
> arch/powerpc/sysdev/fsl_soc.c) that patch #2 deletes. Are there more in
> other files? I'm asking because 11 entries hardly qualifies as "doesn't
> scale". I sure hope that you're not doing all this for the sole purpose
> of getting rid of this 11-element table.

Currently developers add entries to the table in their private builds
for the i2c devices they are using. Another way to avoid adding a
table entry is to create a platform device in the platform code. But
this support is being extended to audio codecs too. There are hundreds
of audio codecs.

The whole purpose of this code is to dynamically load the correct i2c
and audio drivers by reading the device tree instead of having static
i2s/codec devices for every possible platform combination.


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH 0/5] Version 17, series to add device tree naming to i2c

[Jean Delvare]

Hi Jon,

On Sat, 12 Jan 2008 11:00:31 -0500, Jon Smirl wrote:
> On 1/12/08, Jean Delvare wrote:
> > What I meant is that the translation from Open Firmware device name to
> > Linux device name could happen in different ways. Making module aliases
> > out of the is one possibility but this is not the only one.
> >
> > I am curious why the translation could not happen "offline". As I
> > understand it, you're getting the device names from these .dts files.
> > However you're not parsing them in the kernel directly, are you? I
> > presume that you have some tool that converts these files into C code
> > that the kernel can use? This conversion tool could translate the names.
> 
> Those dts files are for embedded devices that were specifically
> developed for Linux. All of the PowerPC Macs in the world have a
> device tree in ROM that was developed by Apple following the Open
> Firmware standard. Same thing for Sun boxes, but I'm not working on
> those.

OK. So basically we have to handle two different cases here, trees that
come from the .dts files and trees that are read from ROMs, right?
Does this mean that .dts files are compiled to some binary format to
look like what is in the ROMs? Is there kernel code that parses this?
Please explain how both types are handled by the kernel. I need to
understand how this works before I can decide where the OF names ->
Linux names translation can happen.

> The kernel has an existing mechanism for handling translations like
> these, the alias scheme.

That we agree on. My concern here is that you want to replace the Linux
names of i2c devices by OF names, without realizing that the Linux
names have a use outside of the kernel. We can't just replace them like
that, it would break some user-space applications. That's the reason
why I believe that it would make more sense to translate from OF names
to Linux names early in the process, so that the kernel, and thus
user-space applications, always handle and see the Linux names,
independently of the platform. I'm asking questions in order to figure
out whether and how this could be achieved.

> Currently developers add entries to the table in their private builds
> for the i2c devices they are using. Another way to avoid adding a
> table entry is to create a platform device in the platform code. But
> this support is being extended to audio codecs too. There are hundreds
> of audio codecs.
> 
> The whole purpose of this code is to dynamically load the correct i2c
> and audio drivers by reading the device tree instead of having static
> i2s/codec devices for every possible platform combination.

I2C driver autoloading is already implemented, and works. Just not the
way you expected, but it works.

Replacing this mechanism with standard aliases is IMHO a good idea, it
makes the code cleaner and also more similar to what the rest of the
kernel does, which is always nice.

However, having a module aliasing mechanism for i2c drivers does NOT
require that OF names are used. We could implement aliasing using Linux
device names. Note that I have no problem with using OF names for
aliasing, however it should not break applications that currently know
the I2C devices by their Linux name.

-- 
Jean Delvare

Re: [i2c] [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jean Delvare]

Hi Jon,

On Wed, 19 Dec 2007 23:41:38 -0500, Jon Smirl wrote:
> This patch allows new style i2c chip drivers to have alias names using
> the official kernel aliasing system and MODULE_DEVICE_TABLE(). I've
> tested it on PowerPC and x86. This change is required for PowerPC
> device tree support.

Your patch adds compilation warnings on several i2c drivers:

drivers/hwmon/f75375s.c:135: warning: initialization from incompatible pointer type
drivers/i2c/chips/ds1682.c:241: warning: initialization from incompatible pointer type
drivers/i2c/chips/tps65010.c:590: warning: initialization from incompatible pointer type
drivers/i2c/chips/tsl2550.c:461: warning: initialization from incompatible pointer type
drivers/rtc/rtc-ds1307.c:538: warning: initialization from incompatible pointer type
drivers/rtc/rtc-ds1374.c:430: warning: initialization from incompatible pointer type
drivers/rtc/rtc-m41t80.c:897: warning: initialization from incompatible pointer type
drivers/rtc/rtc-rs5c372.c:652: warning: initialization from incompatible pointer type

And there may be more drivers affected that just happen to not build on
x86_64 so I did not spot them. Please check this and fix them all.

I see that 4 of these warnings are fixed in the next patch of this
series, but that's not sufficient: each patch must be correct by
itself, so that bisections can be performed safely.

> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
> 
>  drivers/i2c/i2c-core.c          |   32 ++++++++++++++++++++++++++------
>  include/linux/i2c.h             |    9 ++++-----
>  include/linux/mod_devicetable.h |   20 ++++++++++++++++++++
>  scripts/mod/file2alias.c        |   19 +++++++++++++++++++
>  4 files changed, 69 insertions(+), 11 deletions(-)
> 
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b5e13e4..fce06fd 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -47,10 +47,25 @@ static DEFINE_IDR(i2c_adapter_idr);
>  
>  /* ------------------------------------------------------------------------- */
>  
> -static int i2c_device_match(struct device *dev, struct device_driver *drv)
> +static const struct i2c_device_id *i2c_device_match(const struct i2c_device_id *id, struct i2c_client *client)

Line too long, please fold.

Following the pci and usb examples, this function would be named
i2c_match_id.

> +{
> +	/* only powerpc drivers implement the id_table,
> +	 * it is empty on other platforms */
> +	if (id) {
> +		while (id->name[0]) {
> +			if (strcmp(client->driver_name, id->name) == 0)

This doesn't look right to me. You should be comparing client->name,
not client->driver_name, with id->name. Where id_table is implemented,
client->driver_name might not even be set. I see that the next patch in
the series makes use of client->driver_name as well, so your code
"works"... but this ain't correct still.

> +				return id;
> +			id++;
> +		}
> +	}
> +	return NULL;
> +}
> +
> +static int i2c_bus_match(struct device *dev, struct device_driver *drv)

And this function would be named i2c_device_match (i.e. don't change
the name.)

>  {
>  	struct i2c_client	*client = to_i2c_client(dev);
>  	struct i2c_driver	*driver = to_i2c_driver(drv);
> +	const struct i2c_device_id *found_id;
>  
>  	/* make legacy i2c drivers bypass driver model probing entirely;
>  	 * such drivers scan each i2c adapter/bus themselves.
> @@ -58,9 +73,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
>  	if (!is_newstyle_driver(driver))
>  		return 0;
>  
> -	/* new style drivers use the same kind of driver matching policy
> -	 * as platform devices or SPI:  compare device and driver IDs.
> -	 */

This comment still applies to the last part of the function.

> +	/* match on an id table if there is one */
> +	found_id = i2c_device_match(driver->id_table, client);
> +	if (found_id)
> +		return 1;

If the driver has an id_table but the device doesn't match any entry,
you fallback to the string comparison below. Is this really what you
want? Why not return 0 right away instead? Again, client->driver_name
might not even be set.

> +
>  	return strcmp(client->driver_name, drv->name) == 0;
>  }
>  
> @@ -89,12 +106,15 @@ static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = to_i2c_client(dev);
>  	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
> +	const struct i2c_device_id *id;
>  
>  	if (!driver->probe)
>  		return -ENODEV;
>  	client->driver = driver;
>  	dev_dbg(dev, "probe\n");
> -	return driver->probe(client);
> +
> +	id = i2c_device_match(driver->id_table, client);
> +	return driver->probe(client, id);
>  }
>  
>  static int i2c_device_remove(struct device *dev)
> @@ -189,7 +209,7 @@ static struct device_attribute i2c_dev_attrs[] = {
>  static struct bus_type i2c_bus_type = {
>  	.name		= "i2c",
>  	.dev_attrs	= i2c_dev_attrs,
> -	.match		= i2c_device_match,
> +	.match		= i2c_bus_match,
>  	.uevent		= i2c_device_uevent,
>  	.probe		= i2c_device_probe,
>  	.remove		= i2c_device_remove,
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index a100c9f..49fc682 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -126,7 +126,7 @@ struct i2c_driver {
>  	 * With the driver model, device enumeration is NEVER done by drivers;
>  	 * it's done by infrastructure.  (NEW STYLE DRIVERS ONLY)
>  	 */
> -	int (*probe)(struct i2c_client *);
> +	int (*probe)(struct i2c_client *, const struct i2c_device_id *id);
>  	int (*remove)(struct i2c_client *);
>  
>  	/* driver model interfaces that don't relate to enumeration  */
> @@ -141,11 +141,10 @@ struct i2c_driver {
>  
>  	struct device_driver driver;
>  	struct list_head list;
> +	struct i2c_device_id *id_table;
>  };
>  #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
>  
> -#define I2C_NAME_SIZE	20
> -
>  /**
>   * struct i2c_client - represent an I2C slave device
>   * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
> @@ -179,7 +178,7 @@ struct i2c_client {
>  					/* to the client		*/
>  	struct device dev;		/* the device structure		*/
>  	int irq;			/* irq issued by device (or -1) */
> -	char driver_name[KOBJ_NAME_LEN];
> +	char driver_name[I2C_NAME_SIZE];

Rationale please? I can't see why this change would be needed.

>  	struct list_head list;
>  	struct completion released;
>  };
> @@ -223,7 +222,7 @@ static inline void i2c_set_clientdata (struct i2c_client *dev, void *data)
>   * with the adapter already known.
>   */
>  struct i2c_board_info {
> -	char		driver_name[KOBJ_NAME_LEN];
> +	char		driver_name[I2C_NAME_SIZE];

Ditto.

>  	char		type[I2C_NAME_SIZE];
>  	unsigned short	flags;
>  	unsigned short	addr;
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index e9fddb4..d66038a 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -367,4 +367,24 @@ struct virtio_device_id {
>  };
>  #define VIRTIO_DEV_ANY_ID	0xffffffff
>  
> +/* i2c */
> +
> +/* These defines are used to separate PowerPC open firmware
> + * drivers into their own namespace */
> +#define I2C_NAME_SIZE	(16*3)

Rationale for this value? 48 bytes seems quite large, the longest
string you handle in the next patch is only 15-bytes long. 32 would
seem to be a more reasonable compromise (but see also my other reply to
this thread for why you probably shouldn't change I2C_NAME_SIZE at all.)

> +#define I2C_MODULE_PREFIX "i2c:N"

This "N" shouldn't be part of the prefix... if it should be there at
all. It makes the aliases harder to read and I don't see what it is
good for.

> +#ifdef CONFIG_OF
> +#define OF_I2C_PREFIX "OF,"
> +#define I2C_OF_MODULE_PREFIX I2C_MODULE_PREFIX OF_I2C_PREFIX

This one isn't used anywhere?

> +#define OF_I2C_ID(s,d) {OF_I2C_PREFIX s, (d) },

Coding style: space afte opening curly brace.

> +#else
> +#define OF_I2C_ID(s,d)
> +#endif
> +
> +struct i2c_device_id {
> +	char name[I2C_NAME_SIZE];
> +	kernel_ulong_t driver_data;	/* Data private to the driver */
> +};
> +
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index d802b5a..da43742 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -539,6 +539,21 @@ static int do_virtio_entry(const char *filename, struct virtio_device_id *id,
>  	return 1;
>  }
>  
> +/* Looks like: i2c:Ns */
> +static int do_i2c_entry(const char *filename, struct i2c_device_id *id,
> +			   char *alias)
> +{
> +    char *tmp;
> +    sprintf (alias, I2C_MODULE_PREFIX "%s", id->name);

Coding style: no space before opening parenthesis. Also please use tabs
for indentation.

> +
> +    /* Replace all whitespace with underscores */
> +    for (tmp = alias; tmp && *tmp; tmp++)
> +        if (isspace (*tmp))
> +            *tmp = '_';

Is this needed? Are there really OF (or other) device names that contain
whitespaces? None of the examples in this patch series do, and I can't
think of any.

> +
> +    return 1;
> +}
> +
>  /* Ignore any prefix, eg. v850 prepends _ */
>  static inline int sym_is(const char *symbol, const char *name)
>  {
> @@ -669,6 +684,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
>  		do_table(symval, sym->st_size,
>  			 sizeof(struct virtio_device_id), "virtio",
>  			 do_virtio_entry, mod);
> +	else if (sym_is(symname, "__mod_i2c_device_table"))
> +		do_table(symval, sym->st_size,
> +			 sizeof(struct i2c_device_id), "i2c",
> +			 do_i2c_entry, mod);
>  	free(zeros);
>  }

-- 
Jean Delvare

Re: [i2c] [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jean Delvare]

Hi Jon,

On Fri, 11 Jan 2008 20:20:15 +0100, Jean Delvare wrote:
> > +{
> > +	/* only powerpc drivers implement the id_table,
> > +	 * it is empty on other platforms */
> > +	if (id) {
> > +		while (id->name[0]) {
> > +			if (strcmp(client->driver_name, id->name) == 0)
> 
> This doesn't look right to me. You should be comparing client->name,
> not client->driver_name, with id->name. Where id_table is implemented,
> client->driver_name might not even be set. I see that the next patch in
> the series makes use of client->driver_name as well, so your code
> "works"... but this ain't correct still.

Err, scratch this (and related comments), I just realized what you were
trying to do. That's different from what I had in mind and so I read
your code wrong. I'll read it (and test it) again not making this
incorrect assumption and my comments will likely be different after
that. Well, I still think that it needs to be changed a bit, but
probably not in the direction I suggested at first (which, I realize
now, has its own share of issues - so it's not fair to me to point you
there.)

Sorry for the trouble. I'll post updated comments later today, but I'm
also pretty busy with other issues, some of which need to be solved
before 2.6.24 is released so I can't really delay them.

-- 
Jean Delvare

Re: [i2c] [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jon Smirl]

On 1/12/08, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Jon,
>
> On Fri, 11 Jan 2008 20:20:15 +0100, Jean Delvare wrote:
> > > +{
> > > +   /* only powerpc drivers implement the id_table,
> > > +    * it is empty on other platforms */
> > > +   if (id) {
> > > +           while (id->name[0]) {
> > > +                   if (strcmp(client->driver_name, id->name) == 0)
> >
> > This doesn't look right to me. You should be comparing client->name,
> > not client->driver_name, with id->name. Where id_table is implemented,
> > client->driver_name might not even be set. I see that the next patch in
> > the series makes use of client->driver_name as well, so your code
> > "works"... but this ain't correct still.
>
> Err, scratch this (and related comments), I just realized what you were
> trying to do. That's different from what I had in mind and so I read
> your code wrong. I'll read it (and test it) again not making this
> incorrect assumption and my comments will likely be different after
> that. Well, I still think that it needs to be changed a bit, but
> probably not in the direction I suggested at first (which, I realize
> now, has its own share of issues - so it's not fair to me to point you
> there.)

The common scheme used elsewhere in the kernel for handling more than
one device in a single driver is aliases. The i2c code's existing
driver_name/type combination is a different way of implementing the
same feature. But there is no real need for driver_name/type on any
platform if aliases are used. Back in version 10 or 11 I had code in
there which replaced the two fields with aliases on all platforms but
too many people objected so I removed it..

IMHO, driver_name/type should be removed in new style drivers and
replaced with aliases on all platforms since aliases are the standard
kernel mechanism.



>
> Sorry for the trouble. I'll post updated comments later today, but I'm
> also pretty busy with other issues, some of which need to be solved
> before 2.6.24 is released so I can't really delay them.
>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jean Delvare]

Hi Jon,

On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
> The common scheme used elsewhere in the kernel for handling more than
> one device in a single driver is aliases. The i2c code's existing
> driver_name/type combination is a different way of implementing the
> same feature. But there is no real need for driver_name/type on any
> platform if aliases are used. Back in version 10 or 11 I had code in
> there which replaced the two fields with aliases on all platforms but
> too many people objected so I removed it..

While I agree that aliases make i2c_client.driver_name obsolete,
i2c_client.type is still needed. Not for device/driver matching in the
kernel, granted, but for device identification from userspace. This is
a first problem your patch has: when using your aliasing mechanism, the
type string is left empty. i2c-core exports this value to user-space
via the "name" sysfs attribute, and some libraries and applications
make use of it. I know of libsensors at least, but I guess there are
more. I can't apply your patch until this problem is solved, otherwise
we would break some user-space applications.

> IMHO, driver_name/type should be removed in new style drivers and
> replaced with aliases on all platforms since aliases are the standard
> kernel mechanism.

I agree. But we can take your aliasing code now (once you have
addressed the issues I raised) and convert the users of driver_name
later; it doesn't have to be done all at once.

The second problem I have with your patch is that you make use of the
driver_name field, while I ultimately want to get rid of it. I'd rather
see you use a different field for aliases, so that the later removal of
the driver_name field and the associated mechanism is easier.

A third, related problem, is the contents of the modalias file when
using your patch. When I tested on my ADM1032 evaluation board, the
modalias contained "adm1032". This isn't a valid module alias string:
"modprobe adm1032" doesn't work. What works is "modprobe i2c:Nadm1032"
so the modalias file should contain "i2c:Nadm1032". Just take a look at
all modalias files in /sys, they all include the subsystem prefix and a
simple modprobe `cat modalias` loads the required driver. I fail to see
why the i2c subsystem would be different.

I said this is related to the second problem because right now,
i2c-core can't easily differentiate between driver names and aliases,
as both are stored in i2c_client.driver_name. Having separate fields
would make it possible (and relatively easy) to add the required prefix
before aliases but not before driver names. The only drawback is that
it will increase the size of the i2c_client structure, but I do not
care that much given that it is only temporary.

-- 
Jean Delvare

Re: [i2c] [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jon Smirl]

On 1/13/08, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Jon,
>
> On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
> > The common scheme used elsewhere in the kernel for handling more than
> > one device in a single driver is aliases. The i2c code's existing
> > driver_name/type combination is a different way of implementing the
> > same feature. But there is no real need for driver_name/type on any
> > platform if aliases are used. Back in version 10 or 11 I had code in
> > there which replaced the two fields with aliases on all platforms but
> > too many people objected so I removed it..
>
> While I agree that aliases make i2c_client.driver_name obsolete,
> i2c_client.type is still needed. Not for device/driver matching in the
> kernel, granted, but for device identification from userspace. This is
> a first problem your patch has: when using your aliasing mechanism, the
> type string is left empty. i2c-core exports this value to user-space
> via the "name" sysfs attribute, and some libraries and applications
> make use of it. I know of libsensors at least, but I guess there are
> more. I can't apply your patch until this problem is solved, otherwise
> we would break some user-space applications.
>
> > IMHO, driver_name/type should be removed in new style drivers and
> > replaced with aliases on all platforms since aliases are the standard
> > kernel mechanism.
>
> I agree. But we can take your aliasing code now (once you have
> addressed the issues I raised) and convert the users of driver_name
> later; it doesn't have to be done all at once.

GregKH, adding a new dynamically loadable subsystem is not something
that happens every day, can you check to make sure all of the standard
kernels mechanisms are being used? I'm not totally sure how the
modalias naming code is supposed to be done. The subsystem core code
in these patches needs review.

Jean, could you take over the i2c core portion of the patch? That will
let you decide exactly how you want the driver_name/name fields to be
dealt with. After you get standard naming support into i2c core I'll
rework the rest of the patch to use your new code.

I don't think driver_name/name fields should be stored in an i2c
structure at all. They are redundant with the standard mechanism.

The kernel automatically exposes modalias as a sysfs attribute so the
string must be recorded further down in the driver support layers. No
need to keep a copy in the i2c structure.

Standard devices don't export a 'name' attribute. To see the driver
name for a device in sysfs look at the 'driver' link.

> The second problem I have with your patch is that you make use of the
> driver_name field, while I ultimately want to get rid of it. I'd rather
> see you use a different field for aliases, so that the later removal of
> the driver_name field and the associated mechanism is easier.
>
> A third, related problem, is the contents of the modalias file when
> using your patch. When I tested on my ADM1032 evaluation board, the
> modalias contained "adm1032". This isn't a valid module alias string:
> "modprobe adm1032" doesn't work. What works is "modprobe i2c:Nadm1032"
> so the modalias file should contain "i2c:Nadm1032". Just take a look at
> all modalias files in /sys, they all include the subsystem prefix and a
> simple modprobe `cat modalias` loads the required driver. I fail to see
> why the i2c subsystem would be different.
>
> I said this is related to the second problem because right now,
> i2c-core can't easily differentiate between driver names and aliases,
> as both are stored in i2c_client.driver_name. Having separate fields
> would make it possible (and relatively easy) to add the required prefix
> before aliases but not before driver names. The only drawback is that
> it will increase the size of the i2c_client structure, but I do not
> care that much given that it is only temporary.
>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jean Delvare]

On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
> On 1/13/08, Jean Delvare wrote:
> > On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
> > > IMHO, driver_name/type should be removed in new style drivers and
> > > replaced with aliases on all platforms since aliases are the standard
> > > kernel mechanism.
> >
> > I agree. But we can take your aliasing code now (once you have
> > addressed the issues I raised) and convert the users of driver_name
> > later; it doesn't have to be done all at once.
> 
> GregKH, adding a new dynamically loadable subsystem is not something
> that happens every day, can you check to make sure all of the standard
> kernels mechanisms are being used? I'm not totally sure how the
> modalias naming code is supposed to be done. The subsystem core code
> in these patches needs review.
> 
> Jean, could you take over the i2c core portion of the patch? That will
> let you decide exactly how you want the driver_name/name fields to be
> dealt with. After you get standard naming support into i2c core I'll
> rework the rest of the patch to use your new code.

Yes, that could be done, and I agree that it will probably be faster
than iterative review/rework cycles between you and me. I'll free some
cycles next week for that.

> I don't think driver_name/name fields should be stored in an i2c
> structure at all. They are redundant with the standard mechanism.
> 
> The kernel automatically exposes modalias as a sysfs attribute so the
> string must be recorded further down in the driver support layers. No
> need to keep a copy in the i2c structure.

Really? I didn't know that. So that's another thing that the i2c
subsystem is not doing like the rest of the kernel? Can you please
point me to the code that does this?

> Standard devices don't export a 'name' attribute. To see the driver
> name for a device in sysfs look at the 'driver' link.

The driver name and the device name are different things! The "name"
attribute that i2c devices have tells user-space the device name, not
the driver name.

You may not like what the i2c subsystem does but you can't ignore its
history. The name attribute of i2c devices has been there pretty much
forever and user-space relies on it, thus we can't remove it.

-- 
Jean Delvare

Re: [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jon Smirl]

On 1/13/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
> > On 1/13/08, Jean Delvare wrote:
> > > On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
> > > > IMHO, driver_name/type should be removed in new style drivers and
> > > > replaced with aliases on all platforms since aliases are the standard
> > > > kernel mechanism.
> > >
> > > I agree. But we can take your aliasing code now (once you have
> > > addressed the issues I raised) and convert the users of driver_name
> > > later; it doesn't have to be done all at once.
> >
> > GregKH, adding a new dynamically loadable subsystem is not something
> > that happens every day, can you check to make sure all of the standard
> > kernels mechanisms are being used? I'm not totally sure how the
> > modalias naming code is supposed to be done. The subsystem core code
> > in these patches needs review.
> >
> > Jean, could you take over the i2c core portion of the patch? That will
> > let you decide exactly how you want the driver_name/name fields to be
> > dealt with. After you get standard naming support into i2c core I'll
> > rework the rest of the patch to use your new code.
>
> Yes, that could be done, and I agree that it will probably be faster
> than iterative review/rework cycles between you and me. I'll free some
> cycles next week for that.
>
> > I don't think driver_name/name fields should be stored in an i2c
> > structure at all. They are redundant with the standard mechanism.
> >
> > The kernel automatically exposes modalias as a sysfs attribute so the
> > string must be recorded further down in the driver support layers. No
> > need to keep a copy in the i2c structure.
>
> Really? I didn't know that. So that's another thing that the i2c
> subsystem is not doing like the rest of the kernel? Can you please
> point me to the code that does this?

I never noticed it before either. Just do find | grep modalias in /sys
and see that every driver has a modalias attribute. It is probably
implement in drivers/base.

>
> > Standard devices don't export a 'name' attribute. To see the driver
> > name for a device in sysfs look at the 'driver' link.
>
> The driver name and the device name are different things! The "name"
> attribute that i2c devices have tells user-space the device name, not
> the driver name.

For this system my i2c device names are:
0-0050  0-0051  0-0052  0-0053
How does the name=eeprom attribute interact with this? All four of my
devices have name=eeprom. What is the name field used for in user
space?

jonsmirl@terra:/sys/bus/i2c/devices/0-0052$ ls
driver  eeprom  modalias  name  power  subsystem  uevent
jonsmirl@terra:/sys/bus/i2c/devices/0-0052$ cat name
eeprom
jonsmirl@terra:/sys/bus/i2c/devices/0-0052$ ls driver -l
lrwxrwxrwx 1 root root 0 2008-01-13 12:46 driver ->
../../../../../../bus/i2c/drivers/eeprom
jonsmirl@terra:/sys/bus/i2c/devices/0-0052$

jonsmirl@terra:/sys/bus/i2c/drivers$ ls
eeprom
jonsmirl@terra:/sys/bus/i2c/drivers$ ls eeprom
0-0050  0-0051  0-0052  0-0053  bind  module  uevent  unbind

>
> You may not like what the i2c subsystem does but you can't ignore its
> history. The name attribute of i2c devices has been there pretty much
> forever and user-space relies on it, thus we can't remove it.
>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jean Delvare]

Hi Jon,

On Sun, 13 Jan 2008 13:01:06 -0500, Jon Smirl wrote:
> On 1/13/08, Jean Delvare wrote:
> > On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
> > > The kernel automatically exposes modalias as a sysfs attribute so the
> > > string must be recorded further down in the driver support layers. No
> > > need to keep a copy in the i2c structure.
> >
> > Really? I didn't know that. So that's another thing that the i2c
> > subsystem is not doing like the rest of the kernel? Can you please
> > point me to the code that does this?
> 
> I never noticed it before either. Just do find | grep modalias in /sys
> and see that every driver has a modalias attribute. It is probably
> implement in drivers/base.

This doesn't mean that the kernel does this automatically! It could
also be that each subsystem does it on its own. Given that the format of
the modalias depends on the bus type, it wouldn't be all that
surprising. Anyway, I'll go look at how the other subsystems handle it
before going on.

> > > Standard devices don't export a 'name' attribute. To see the driver
> > > name for a device in sysfs look at the 'driver' link.
> >
> > The driver name and the device name are different things! The "name"
> > attribute that i2c devices have tells user-space the device name, not
> > the driver name.
> 
> For this system my i2c device names are:
> 0-0050  0-0051  0-0052  0-0053

These are not device names, these are device bus IDs. They tell you how
to access the devices, but they do not tell you what these devices are.

> How does the name=eeprom attribute interact with this? All four of my
> devices have name=eeprom. What is the name field used for in user
> space?

The eeprom case might be a bit confusing because that i2c driver
supports a single device type, so the driver name is the same as the
device name. Take a look at the hwmon/lm90 driver for a better example:
this device supports 7 different devices. The devices are mostly
compatible so it made sense to have a single driver for them, but they
all differ in some way. For example, the LM90 doesn't support PEC,
while the ADM1032 does. User-space needs to be able to distinguish
between the various types. That's the reason why we export the device
name through sysfs.

Most i2c sensor drivers support several chip types, and libsensors has
been relying heavily on the name attribute. Less so with the lm-sensors
3.0.0 rewrite, where most things are automatically detected, but if
nothing else, giving humans a way to distinguish between the different
sensor chip types is very useful. Also, not everyone will upgrade to
lm-sensors 3.0.0 so we need to keep supporting the previous versions.

-- 
Jean Delvare

Re: [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jon Smirl]

On 1/13/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Jon,
>
> On Sun, 13 Jan 2008 13:01:06 -0500, Jon Smirl wrote:
> > On 1/13/08, Jean Delvare wrote:
> > > On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
> > > > The kernel automatically exposes modalias as a sysfs attribute so the
> > > > string must be recorded further down in the driver support layers. No
> > > > need to keep a copy in the i2c structure.
> > >
> > > Really? I didn't know that. So that's another thing that the i2c
> > > subsystem is not doing like the rest of the kernel? Can you please
> > > point me to the code that does this?
> >
> > I never noticed it before either. Just do find | grep modalias in /sys
> > and see that every driver has a modalias attribute. It is probably
> > implement in drivers/base.
>
> This doesn't mean that the kernel does this automatically! It could
> also be that each subsystem does it on its own. Given that the format of
> the modalias depends on the bus type, it wouldn't be all that
> surprising. Anyway, I'll go look at how the other subsystems handle it
> before going on.
>
> > > > Standard devices don't export a 'name' attribute. To see the driver
> > > > name for a device in sysfs look at the 'driver' link.
> > >
> > > The driver name and the device name are different things! The "name"
> > > attribute that i2c devices have tells user-space the device name, not
> > > the driver name.
> >
> > For this system my i2c device names are:
> > 0-0050  0-0051  0-0052  0-0053
>
> These are not device names, these are device bus IDs. They tell you how
> to access the devices, but they do not tell you what these devices are.
>
> > How does the name=eeprom attribute interact with this? All four of my
> > devices have name=eeprom. What is the name field used for in user
> > space?
>
> The eeprom case might be a bit confusing because that i2c driver
> supports a single device type, so the driver name is the same as the
> device name. Take a look at the hwmon/lm90 driver for a better example:
> this device supports 7 different devices. The devices are mostly
> compatible so it made sense to have a single driver for them, but they
> all differ in some way. For example, the LM90 doesn't support PEC,
> while the ADM1032 does. User-space needs to be able to distinguish
> between the various types. That's the reason why we export the device
> name through sysfs.
>
> Most i2c sensor drivers support several chip types, and libsensors has
> been relying heavily on the name attribute. Less so with the lm-sensors
> 3.0.0 rewrite, where most things are automatically detected, but if
> nothing else, giving humans a way to distinguish between the different
> sensor chip types is very useful. Also, not everyone will upgrade to
> lm-sensors 3.0.0 so we need to keep supporting the previous versions.

Another way to handle this is to have the drivers register multiple
times using different names. So LM90 would register as both LM90 and
ADM1032. All the code is shared, you just register it multiple times
under different names.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [PATCH 1/5] Implement module aliasing for i2c to translate from device tree names

[Jean Delvare]

On Sun, 13 Jan 2008 13:50:46 -0500, Jon Smirl wrote:
> On 1/13/08, Jean Delvare <khali@linux-fr.org> wrote:
> > The eeprom case might be a bit confusing because that i2c driver
> > supports a single device type, so the driver name is the same as the
> > device name. Take a look at the hwmon/lm90 driver for a better example:
> > this device supports 7 different devices. The devices are mostly
> > compatible so it made sense to have a single driver for them, but they
> > all differ in some way. For example, the LM90 doesn't support PEC,
> > while the ADM1032 does. User-space needs to be able to distinguish
> > between the various types. That's the reason why we export the device
> > name through sysfs.
> >
> > Most i2c sensor drivers support several chip types, and libsensors has
> > been relying heavily on the name attribute. Less so with the lm-sensors
> > 3.0.0 rewrite, where most things are automatically detected, but if
> > nothing else, giving humans a way to distinguish between the different
> > sensor chip types is very useful. Also, not everyone will upgrade to
> > lm-sensors 3.0.0 so we need to keep supporting the previous versions.
> 
> Another way to handle this is to have the drivers register multiple
> times using different names. So LM90 would register as both LM90 and
> ADM1032. All the code is shared, you just register it multiple times
> under different names.

This won't let us get rid of the name attribute. As I repeatedly
explained, removing that file now would instantly break at least all
versions of lm-sensors before 3.0.0. This alone is enough to make it
unacceptable for the years to come.

Not to mention that this seems like a waste of kernel memory. The name
attribute certainly takes one order of magnitude less memory than
registering the same driver multiple times.

I'm not sure why you want these i2c chip names to go way, given that
the module aliases you'd like to add are almost the same, just in a
different form and using OF names instead of arbitrary names.

-- 
Jean Delvare