Version 17, series to add device tree naming to i2c

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 <[email protected]>

---

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

drivers/i2c/i2c-core.c +0 -0

index b5e13e4..fce06fd 100644

drivers/i2c/i2c-core.c +1 -0

 {
 	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.

drivers/i2c/i2c-core.c +6 -2

+
 	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,

include/linux/i2c.h +2 -3

index a100c9f..49fc682 100644
@@ -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;

include/linux/i2c.h +0 -0

 	char		type[I2C_NAME_SIZE];
 	unsigned short	flags;
 	unsigned short	addr;

include/linux/mod_devicetable.h +0 -0

index e9fddb4..d66038a 100644

include/linux/mod_devicetable.h +10 -0

+#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 */

scripts/mod/file2alias.c +0 -0

index d802b5a..da43742 100644

scripts/mod/file2alias.c +8 -0

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

Quoted 4 lines

drivers/i2c/i2c-core.c +4 -0

+{
+	/* only powerpc drivers implement the id_table,
+	 * it is empty on other platforms */
+	if (id) {

drivers/i2c/i2c-core.c static DEFINE_IDR(i2c_adapter_idr); · line 56

Quoted 2 lines

drivers/i2c/i2c-core.c +2 -0

+		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.

Hi Jon,

Quoted 7 lines · Jean Delvare

drivers/i2c/i2c-core.c static DEFINE_IDR(i2c_adapter_idr); · line 56

Quoted 6 lines

drivers/i2c/i2c-core.c +6 -0

+{
+   /* 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)

Quoted 5 lines

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

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.

Hi Jon,

Quoted 7 lines · Jon Smirl

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 3 lines

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

Quoted 8 lines · Jean Delvare

Quoted 3 lines · Jon Smirl

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.

Quoted 16 lines · Jon Smirl

Quoted 8 lines · Jean Delvare

Quoted 3 lines · Jon Smirl

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.

Quoted 4 lines

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 3 lines

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

Quoted 3 lines

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?

Quoted 3 lines

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

Quoted 5 lines · Jean Delvare

Quoted 3 lines · Jon Smirl

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.

Quoted 3 lines

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 6 lines

Quoted 2 lines

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?

Hi Jon,

Quoted 6 lines · Jon Smirl

Quoted 5 lines · Jean Delvare

Quoted 3 lines · Jon Smirl

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.

Quoted 7 lines

Quoted 3 lines

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 9 lines

Quoted 6 lines

Quoted 2 lines

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 3 lines

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.

Quoted 10 lines · Jean Delvare

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.

Quoted 6 lines

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.

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.

Quoted 3 lines · Jon Smirl

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.

Quoted 3 lines · Jon Smirl

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.

Quoted 3 lines · Jon Smirl

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.

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

drivers/i2c/busses/i2c-mpc.c +35 -0

+static void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node)
+{
+	struct device_node *node = NULL;
+
+	while ((node = of_get_next_child(adap_node, node))) {
+		struct i2c_board_info info;
+		const u32 *addr;
+		const char *compatible;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+			printk(KERN_ERR "i2c-mpc.c: invalid entry, missing reg attribute\n");
+			continue;
+		}
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		compatible = of_get_property(node, "compatible", &len);
+		if (!compatible) {
+			printk(KERN_ERR "i2c-mpc.c: invalid entry, missing compatible attribute\n");
+			continue;
+		}
+		
+		/* need full alias i2c:NOF,vendor,device */
+		strcpy(info.driver_name, I2C_OF_MODULE_PREFIX);
+		strncat(info.driver_name, compatible, sizeof(info.driver_name));
+		request_module(info.driver_name);
+		
+		/* need module alias OF,vendor,device */
+		strcpy(info.driver_name, OF_I2C_PREFIX);
+		strncat(info.driver_name, compatible, sizeof(info.driver_name));
+		

drivers/i2c/busses/i2c-mpc.c +54 -0

+static int mpc_i2c_probe(struct of_device *op, const struct of_device_id *match)
+{
+	int result = 0;
+	struct mpc_i2c *i2c;
+
+	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	if (of_get_property(op->node, "dfsrr", NULL))
+		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
+
+	if (of_device_is_compatible(op->node, "mpc5200-i2c"))
+		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
+
+	init_waitqueue_head(&i2c->queue);
+
+	i2c->base = of_iomap(op->node, 0);
+	if (!i2c->base) {
+		printk(KERN_ERR "i2c-mpc - failed to map controller\n");
+		result = -ENOMEM;
+		goto fail_map;
+	}
+
+	i2c->irq = irq_of_parse_and_map(op->node, 0);
+	if (i2c->irq == NO_IRQ) {
+		result = -ENXIO;
+		goto fail_irq;
+	}
+
+	result = request_irq(i2c->irq, mpc_i2c_isr, IRQF_SHARED, "i2c-mpc", i2c);
+	if (result < 0) {
+		printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
+		goto fail_request;
+	}
+
+	mpc_i2c_setclock(i2c);
+
+	dev_set_drvdata(&op->dev, i2c);
+
+	i2c->adap = mpc_ops;
+	i2c_set_adapdata(&i2c->adap, i2c);
+	i2c->adap.dev.parent = &op->dev;
+
+	result = i2c_add_adapter(&i2c->adap);
+	if (result < 0) {
+		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
+		goto fail_add;
+	}
+
+	of_register_i2c_devices(&i2c->adap, op->node);
+
+	return result;
+

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

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
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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.

Quoted 1 line

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?

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
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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.

Hi Jon,

Quoted 6 lines · Jon Smirl

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.

Quoted 1 line

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

Quoted 8 lines

Quoted 1 line

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.

Quoted 6 lines · Jean Delvare

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.

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

Quoted 9 lines · Jean Delvare

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.

Quoted 16 lines · Jon Smirl

Quoted 9 lines · Jean Delvare

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"

Quoted 7 lines

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 4 lines

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.

Quoted 4 lines

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.

Quoted 9 lines · Jean Delvare

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.

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.