From: Jon Smirl <hidden> Date: 2007-12-20 04:42:20
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@gmail.com
From: Jon Smirl <hidden> Date: 2007-12-20 04:41:55
Return errors that were being ignored in the mpc-i2c driver
Signed-off-by: Jon Smirl <redacted>
---
drivers/i2c/busses/i2c-mpc.c | 30 +++++++++++++++++-------------
1 files changed, 17 insertions(+), 13 deletions(-)
@@ -192,15 +192,17 @@ static int mpc_write(struct mpc_i2c *i2c, int target,/* Write target byte */writeb((target<<1),i2c->base+MPC_I2C_DR);-if(i2c_wait(i2c,timeout,1)<0)-return-1;+result=i2c_wait(i2c,timeout,1);+if(result<0)+returnresult;for(i=0;i<length;i++){/* Write data byte */writeb(data[i],i2c->base+MPC_I2C_DR);-if(i2c_wait(i2c,timeout,1)<0)-return-1;+result=i2c_wait(i2c,timeout,1);+if(result<0)+returnresult;}return0;
@@ -210,7 +212,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,u8*data,intlength,intrestart){unsignedtimeout=i2c->adap.timeout;-inti;+inti,result;u32flags=restart?CCR_RSTA:0;/* Start with MEN */
@@ -221,8 +223,9 @@ static int mpc_read(struct mpc_i2c *i2c, int target,/* Write target address byte - this time with the read flag set */writeb((target<<1)|1,i2c->base+MPC_I2C_DR);-if(i2c_wait(i2c,timeout,1)<0)-return-1;+result=i2c_wait(i2c,timeout,1);+if(result<0)+returnresult;if(length){if(length==1)
@@ -234,8 +237,9 @@ static int mpc_read(struct mpc_i2c *i2c, int target,}for(i=0;i<length;i++){-if(i2c_wait(i2c,timeout,0)<0)-return-1;+result=i2c_wait(i2c,timeout,0);+if(result<0)+returnresult;/* Generate txack on next to last byte */if(i==length-2)
@@ -321,9 +325,9 @@ static int fsl_i2c_probe(struct platform_device *pdev)pdata=(structfsl_i2c_platform_data*)pdev->dev.platform_data;-if(!(i2c=kzalloc(sizeof(*i2c),GFP_KERNEL))){+i2c=kzalloc(sizeof(*i2c),GFP_KERNEL);+if(!i2c)return-ENOMEM;-}i2c->irq=platform_get_irq(pdev,0);if(i2c->irq<0){
@@ -381,7 +385,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)i2c_del_adapter(&i2c->adap);platform_set_drvdata(pdev,NULL);-if(i2c->irq!=0)+if(i2c->irq!=NO_IRQ)free_irq(i2c->irq,i2c);iounmap(i2c->base);
From: Jon Smirl <hidden> Date: 2007-12-20 04:42:35
Convert pfc8563 i2c driver from old style to new style. The
driver is also modified to support device tree names via the
i2c mod alias mechanism.
Signed-off-by: Jon Smirl <redacted>
---
drivers/rtc/rtc-pcf8563.c | 107 +++++++++++----------------------------------
1 files changed, 27 insertions(+), 80 deletions(-)
From: Jon Smirl <hidden> Date: 2007-12-20 04:42:51
This patch modifies the ds1307, ds1374, and rs5c372 i2c drivers to support
device tree names using the new i2c mod alias support
Signed-off-by: Jon Smirl <redacted>
---
arch/powerpc/sysdev/fsl_soc.c | 44 ++++----------------------------
drivers/rtc/rtc-ds1307.c | 20 +++++++++++++-
drivers/rtc/rtc-ds1374.c | 9 ++++++
drivers/rtc/rtc-m41t80.c | 57 ++++++++++++++++++++++++++++-------------
drivers/rtc/rtc-rs5c372.c | 16 ++++++++++--
5 files changed, 85 insertions(+), 61 deletions(-)
@@ -773,17 +786,24 @@ static int m41t80_probe(struct i2c_client *client)dev_info(&client->dev,"chip found, driver version "DRV_VERSION"\n");-chip=NULL;-for(i=0;i<ARRAY_SIZE(m41t80_chip_info_tbl);i++){-if(!strcmp(m41t80_chip_info_tbl[i].name,client->name)){-chip=&m41t80_chip_info_tbl[i];-break;+if(id)+features=id->driver_data;+else{+conststructm41t80_chip_info*chip;++chip=NULL;+for(i=0;i<ARRAY_SIZE(m41t80_chip_info_tbl);i++){+if(!strcmp(m41t80_chip_info_tbl[i].name,client->name)){+chip=&m41t80_chip_info_tbl[i];+break;+}}-}-if(!chip){-dev_err(&client->dev,"%s is not supported\n",client->name);-rc=-ENODEV;-gotoexit;+if(!chip){+dev_err(&client->dev,"%s is not supported\n",client->name);+rc=-ENODEV;+gotoexit;+}+features=chip->features;}clientdata=kzalloc(sizeof(*clientdata),GFP_KERNEL);
@@ -801,7 +821,7 @@ static int m41t80_probe(struct i2c_client *client)}clientdata->rtc=rtc;-clientdata->chip=chip;+clientdata->features=features;i2c_set_clientdata(client,clientdata);/* Make sure HT (Halt Update) bit is cleared */
@@ -810,7 +830,7 @@ static int m41t80_probe(struct i2c_client *client)gotoht_err;if(rc&M41T80_ALHOUR_HT){-if(chip->features&M41T80_FEATURE_HT){+if(features&M41T80_FEATURE_HT){m41t80_get_datetime(client,&tm);dev_info(&client->dev,"HT bit was set!\n");dev_info(&client->dev,
@@ -842,7 +862,7 @@ static int m41t80_probe(struct i2c_client *client)gotoexit;#ifdef CONFIG_RTC_DRV_M41T80_WDT-if(chip->features&M41T80_FEATURE_HT){+if(features&M41T80_FEATURE_HT){rc=misc_register(&wdt_dev);if(rc)gotoexit;
@@ -878,7 +898,7 @@ static int m41t80_remove(struct i2c_client *client)structrtc_device*rtc=clientdata->rtc;#ifdef CONFIG_RTC_DRV_M41T80_WDT-if(clientdata->chip->features&M41T80_FEATURE_HT){+if(clientdata->features&M41T80_FEATURE_HT){misc_deregister(&wdt_dev);unregister_reboot_notifier(&wdt_notifier);}
From: Jon Smirl <hidden> Date: 2007-12-20 04:43:12
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.
Signed-off-by: Jon Smirl <redacted>
---
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(-)
@@ -47,10 +47,25 @@ static DEFINE_IDR(i2c_adapter_idr);/* ------------------------------------------------------------------------- */-staticinti2c_device_match(structdevice*dev,structdevice_driver*drv)+staticconststructi2c_device_id*i2c_device_match(conststructi2c_device_id*id,structi2c_client*client)+{+/* only powerpc drivers implement the id_table,+*itisemptyonotherplatforms*/+if(id){+while(id->name[0]){+if(strcmp(client->driver_name,id->name)==0)+returnid;+id++;+}+}+returnNULL;+}++staticinti2c_bus_match(structdevice*dev,structdevice_driver*drv){structi2c_client*client=to_i2c_client(dev);structi2c_driver*driver=to_i2c_driver(drv);+conststructi2c_device_id*found_id;/* make legacy i2c drivers bypass driver model probing entirely;*suchdriversscaneachi2cadapter/busthemselves.
@@ -58,9 +73,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)if(!is_newstyle_driver(driver))return0;-/* new style drivers use the same kind of driver matching policy-*asplatformdevicesorSPI:comparedeviceanddriverIDs.-*/+/* match on an id table if there is one */+found_id=i2c_device_match(driver->id_table,client);+if(found_id)+return1;+returnstrcmp(client->driver_name,drv->name)==0;}
@@ -89,12 +106,15 @@ static int i2c_device_probe(struct device *dev){structi2c_client*client=to_i2c_client(dev);structi2c_driver*driver=to_i2c_driver(dev->driver);+conststructi2c_device_id*id;if(!driver->probe)return-ENODEV;client->driver=driver;dev_dbg(dev,"probe\n");-returndriver->probe(client);++id=i2c_device_match(driver->id_table,client);+returndriver->probe(client,id);}staticinti2c_device_remove(structdevice*dev)
@@ -367,4 +367,24 @@ struct virtio_device_id {};#define VIRTIO_DEV_ANY_ID 0xffffffff+/* i2c */++/* These defines are used to separate PowerPC open firmware+*driversintotheirownnamespace*/+#define I2C_NAME_SIZE (16*3)+#define I2C_MODULE_PREFIX "i2c:N"+#ifdef CONFIG_OF+#define OF_I2C_PREFIX "OF,"+#define I2C_OF_MODULE_PREFIX I2C_MODULE_PREFIX OF_I2C_PREFIX+#define OF_I2C_ID(s,d) {OF_I2C_PREFIX s, (d) },+#else+#define OF_I2C_ID(s,d)+#endif++structi2c_device_id{+charname[I2C_NAME_SIZE];+kernel_ulong_tdriver_data;/* Data private to the driver */+};++#endif /* LINUX_MOD_DEVICETABLE_H */
From: Jon Smirl <hidden> Date: 2007-12-20 04:43:30
Convert MPC i2c driver from being a platform_driver to an open firmware version. Error returns were improved. Routine names were changed from fsl_ to mpc_ to make them match the file name.
Signed-off-by: Jon Smirl <redacted>
---
arch/powerpc/sysdev/fsl_soc.c | 96 ----------------------
drivers/i2c/busses/i2c-mpc.c | 183 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 180 insertions(+), 99 deletions(-)
From: David Gibson <hidden> Date: 2007-12-20 05:16:27
On Wed, Dec 19, 2007 at 11:41:44PM -0500, Jon Smirl wrote:
Convert MPC i2c driver from being a platform_driver to an open
firmware version. Error returns were improved. Routine names were
changed from fsl_ to mpc_ to make them match the file name.
In discussions BenH and I have had, we've actually concluded that
moving this from platform drivers to of_platform drives is not
actually a good idea.
In fact we're planning to move away from of_platform devices and
drivers and instead develop a framework for instantiating platform
devices or i2c devices or whatever devices from the device tree nodes.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
From: Olof Johansson <hidden> Date: 2007-12-20 05:55:09
On Thu, Dec 20, 2007 at 04:16:18PM +1100, David Gibson wrote:
On Wed, Dec 19, 2007 at 11:41:44PM -0500, Jon Smirl wrote:
quoted
Convert MPC i2c driver from being a platform_driver to an open
firmware version. Error returns were improved. Routine names were
changed from fsl_ to mpc_ to make them match the file name.
In discussions BenH and I have had, we've actually concluded that
moving this from platform drivers to of_platform drives is not
actually a good idea.
In fact we're planning to move away from of_platform devices and
drivers and instead develop a framework for instantiating platform
devices or i2c devices or whatever devices from the device tree nodes.
There's been talk about that for a long time. Whenever that framework
is done, all the other drivers that have been converted (or written)
must/can be converted back as well.
Meanwhile, this should go in so those of us who can make use of the
other improvements the series brings can make use of them.
Thanks,
Olof
From: Stefan Roese <sr@denx.de> Date: 2007-12-20 06:04:43
On Thursday 20 December 2007, David Gibson wrote:
On Wed, Dec 19, 2007 at 11:41:44PM -0500, Jon Smirl wrote:
quoted
Convert MPC i2c driver from being a platform_driver to an open
firmware version. Error returns were improved. Routine names were
changed from fsl_ to mpc_ to make them match the file name.
In discussions BenH and I have had, we've actually concluded that
moving this from platform drivers to of_platform drives is not
actually a good idea.
In fact we're planning to move away from of_platform devices and
drivers and instead develop a framework for instantiating platform
devices or i2c devices or whatever devices from the device tree nodes.
Now that is interesting news. I like this idea.
But what should be done to support the still missing devices in the 4xx
arch/powerpc tree, like I2C, NAND etc.? Should we wait with those driver till
this framework is available?
Thanks.
Cheers,
Stefan
From: Jon Smirl <hidden> Date: 2007-12-20 15:56:52
On 12/20/07, David Gibson [off-list ref] wrote:
On Wed, Dec 19, 2007 at 11:41:44PM -0500, Jon Smirl wrote:
quoted
Convert MPC i2c driver from being a platform_driver to an open
firmware version. Error returns were improved. Routine names were
changed from fsl_ to mpc_ to make them match the file name.
In discussions BenH and I have had, we've actually concluded that
moving this from platform drivers to of_platform drives is not
actually a good idea.
In fact we're planning to move away from of_platform devices and
drivers and instead develop a framework for instantiating platform
devices or i2c devices or whatever devices from the device tree nodes.
I have to work with what is in the kernel tree so this patch is based
the current kernel. A lot of time and testing has going into all the
revisions of this patch. It has been posted in various forms for the
last month and a half.
As for platform vs of_platform, IMHO of_platform should have been
derived from platform instead of being an unrelated structure. In the
current scheme of unrelated structures it makes more sense to use one
or the other and not mix them. I am in agreement with Olof that if and
when a new scheme is adopted the drivers should be converted then.
In my local builds I have disabled platform and use only of_platform.
An of_platform only system works fine and is capable of instantiating
everything from the device tree as this patch illustrates. Once this
patch goes in I'll post another series that shows how I went
of_platform only on mpc5200.
Everyone seems to be focusing on a small piece of driver
initialization code and ignoring the much more complicated part which
implements dynamic i2c driver loading. It's the complicated part of
the code that needs review.
Ben, I know we talked about not putting the alias names into their own
namespace. But the i2c people don't want to move onto the standard
kernel aliasing scheme for other platforms and asked for #ifdef
powerpc. In version 15 I removed the cross platform support and made
everything powerpc specific. .
--
Jon Smirl
jonsmirl@gmail.com
From: Jon Smirl <hidden> Date: 2007-12-20 23:59:34
Are there any other objections to this patch? If not, can it be
targeted for 2.6.25?
On 12/19/07, Jon Smirl [off-list ref] 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@gmail.com
--
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/
From: Jon Smirl <hidden> Date: 2007-12-27 16:47:21
On 12/19/07, Jon Smirl [off-list ref] 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
From: Jean Delvare <hidden> Date: 2007-12-28 12:14:47
Hi John,
Le 27/12/2007, Jon Smirl =E9crit:
On 12/19/07, Jon Smirl [off-list ref] wrote:
quoted
Another rework of the i2c for powerpc device tree patch. This version impl=
ements standard alias naming only on the powerpc platform and only for the de=
vice tree names. The old naming mechanism of i2c_client.name,driver_name is l=
eft in place and not changed for non-powerpc platforms. This patch is fully c=
apable of dynamically loading the i2c modules. You can modprobe in the i2c-mp=
c driver and the i2c modules described in the device tree will be automatical=
ly 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
From: Jochen Friedrich <jochen@scram.de> Date: 2008-01-02 19:40:49
Hi Jon,
Convert MPC i2c driver from being a platform_driver to an open firmware version. Error returns were improved. Routine names were changed from fsl_ to mpc_ to make them match the file name
I did the same for my i2c-cpm. Tested with the frontprocessor driver (dbox2 specific) converted to new style. I'll post the new i2c-cpm, as well.
Should this go into some central file? Else we would have to copy it in any i2c bus driver that supports powerpc.
This would at least be i2c-cpm and i2c-gpio.
IMHO, there should be a node attribute to override i2c_adapter.class. Legacy i2c drivers (in particular v4l and dvb drivers) use this class to decide which adapter to bind to. dbox2 needs I2C_CLASS_TV_DIGITAL (4).
Thanks,
Jochen
From: Jon Smirl <hidden> Date: 2008-01-02 19:49:59
On 1/2/08, Jochen Friedrich [off-list ref] wrote:
Should this go into some central file? Else we would have to copy it in any i2c bus driver that supports powerpc.
This would at least be i2c-cpm and i2c-gpio.
I can change that in the next rev. I'm waiting to see if there are any
more changes.
IMHO, there should be a node attribute to override i2c_adapter.class. Legacy i2c drivers (in particular v4l and dvb drivers) use this class to decide which adapter to bind to. dbox2 needs I2C_CLASS_TV_DIGITAL (4).
I hadn't paid any attention to i2c_adapter.class. Now that you mention
it I wonder why i2c has it. It appears to be a holdover from the
older mechanism of searching for devices and it is used as a filter to
reduce the number of device being searched. I would think a new style
driver could just ignore it.
It is probably best to fix it in a successive patch.
--
Jon Smirl
jonsmirl@gmail.com
From: Jon Smirl <hidden> Date: 2008-01-10 14:14:27
What is the review status of this series, should I post it again?
On 12/19/07, Jon Smirl [off-list ref] 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@gmail.com
--
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/
From: Jean Delvare <hidden> Date: 2008-01-11 08:56:55
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
From: Jon Smirl <hidden> Date: 2008-01-11 15:53:09
On 1/11/08, Jean Delvare [off-list ref] wrote:
Hi Jon,
On Wed, 19 Dec 2007 23:41:36 -0500, Jon Smirl wrote:
quoted
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.
From: Jochen Friedrich <jochen@scram.de> Date: 2008-01-11 16:05:46
Hi Jon,
quoted
quoted
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.
quoted
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.
From: Jean Delvare <hidden> Date: 2008-01-11 19:16:01
On Fri, 11 Jan 2008 10:52:56 -0500, Jon Smirl wrote:
On 1/11/08, Jean Delvare wrote:
quoted
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
From: Jean Delvare <hidden> Date: 2008-01-11 19:20:27
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.
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.
And this function would be named i2c_device_match (i.e. don't change
the name.)
quoted hunk
{
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.
@@ -367,4 +367,24 @@ struct virtio_device_id {};#define VIRTIO_DEV_ANY_ID 0xffffffff+/* i2c */++/* These defines are used to separate PowerPC open firmware+*driversintotheirownnamespace*/+#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.
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.
From: Jon Smirl <hidden> Date: 2008-01-11 20:17:16
On 1/11/08, Jean Delvare [off-list ref] 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.
From: Jean Delvare <hidden> Date: 2008-01-12 08:47:18
Hi Jon,
On Fri, 11 Jan 2008 20:20:15 +0100, Jean Delvare wrote:
quoted
+{
+ /* 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
From: Jean Delvare <hidden> Date: 2008-01-12 09:08:56
On Fri, 11 Jan 2008 15:16:57 -0500, Jon Smirl wrote:
On 1/11/08, Jean Delvare [off-list ref] wrote:
quoted
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
From: Jon Smirl <hidden> Date: 2008-01-12 16:00:42
On 1/12/08, Jean Delvare [off-list ref] wrote:
On Fri, 11 Jan 2008 15:16:57 -0500, Jon Smirl wrote:
quoted
On 1/11/08, Jean Delvare [off-list ref] wrote:
quoted
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.
quoted
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.
quoted
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
From: Jon Smirl <hidden> Date: 2008-01-12 16:26:45
On 1/12/08, Jean Delvare [off-list ref] wrote:
Hi Jon,
On Fri, 11 Jan 2008 20:20:15 +0100, Jean Delvare wrote:
quoted
quoted
+{
+ /* 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
From: Jean Delvare <hidden> Date: 2008-01-13 14:41:26
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
From: Jean Delvare <hidden> Date: 2008-01-13 15:09:56
Hi Jon,
On Sat, 12 Jan 2008 11:00:31 -0500, Jon Smirl wrote:
On 1/12/08, Jean Delvare wrote:
quoted
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
From: Jon Smirl <hidden> Date: 2008-01-13 16:24:51
On 1/13/08, Jean Delvare [off-list ref] wrote:
Hi Jon,
On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
quoted
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.
quoted
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
From: Jean Delvare <hidden> Date: 2008-01-13 17:40:29
On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
On 1/13/08, Jean Delvare wrote:
quoted
On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
quoted
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
From: Jon Smirl <hidden> Date: 2008-01-13 18:01:18
On 1/13/08, Jean Delvare [off-list ref] wrote:
On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
quoted
On 1/13/08, Jean Delvare wrote:
quoted
On Sat, 12 Jan 2008 11:26:34 -0500, Jon Smirl wrote:
quoted
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.
quoted
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.
quoted
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
From: Jean Delvare <hidden> Date: 2008-01-13 18:45:39
Hi Jon,
On Sun, 13 Jan 2008 13:01:06 -0500, Jon Smirl wrote:
On 1/13/08, Jean Delvare wrote:
quoted
On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
quoted
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.
quoted
quoted
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
From: Jon Smirl <hidden> Date: 2008-01-13 18:50:56
On 1/13/08, Jean Delvare [off-list ref] wrote:
Hi Jon,
On Sun, 13 Jan 2008 13:01:06 -0500, Jon Smirl wrote:
quoted
On 1/13/08, Jean Delvare wrote:
quoted
On Sun, 13 Jan 2008 11:24:29 -0500, Jon Smirl wrote:
quoted
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.
quoted
quoted
quoted
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.
quoted
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@gmail.com
From: Jean Delvare <hidden> Date: 2008-01-13 19:05:22
On Sun, 13 Jan 2008 13:50:46 -0500, Jon Smirl wrote:
On 1/13/08, Jean Delvare [off-list ref] wrote:
quoted
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