[PATCH 0/5] Series to add device tree naming to i2c

[PATCH 0/5] Series to add device tree naming to i2c

[Jon Smirl]

Respin to split error return fixups out of mpc-i2c to of_platform change

The following series implements standard linux module aliasing for i2c modules
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 

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

[Jon Smirl]

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 <jonsmirl@gmail.com>
---

 drivers/i2c/i2c-core.c          |   34 +++++++++++++++++++++++++++-------
 include/linux/i2c.h             |    5 ++---
 include/linux/mod_devicetable.h |    9 +++++++++
 3 files changed, 38 insertions(+), 10 deletions(-)


diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b5e13e4..6fa6bab 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -47,10 +47,26 @@ static DEFINE_IDR(i2c_adapter_idr);
 
 /* ------------------------------------------------------------------------- */
 
-static int i2c_device_match(struct device *dev, struct device_driver *drv)
+static const struct i2c_device_id *i2c_device_match(const struct i2c_device_id *id, struct i2c_client *client)
+{
+	/* new style drivers use the same kind of driver matching policy
+	 * as platform devices or SPI:  compare device and driver IDs.
+	 */
+	if (id) {
+		while (id->name[0]) {
+			if (strcmp(client->driver_name, id->name) == 0)
+				return id;
+			id++;
+		}
+	}
+	return NULL;
+}
+
+static int i2c_bus_match(struct device *dev, struct device_driver *drv)
 {
 	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,10 +74,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.
-	 */
-	return strcmp(client->driver_name, drv->name) == 0;
+	found_id = i2c_device_match(driver->id_table, client);
+	if (found_id)
+		return 1;
+
+    return 0;
 }
 
 #ifdef	CONFIG_HOTPLUG
@@ -89,12 +106,15 @@ static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
+	const struct i2c_device_id *id;
 
 	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+
+    id = i2c_device_match(driver->id_table, client);
+	return driver->probe(client, id);
 }
 
 static int i2c_device_remove(struct device *dev)
@@ -189,7 +209,7 @@ static struct device_attribute i2c_dev_attrs[] = {
 static struct bus_type i2c_bus_type = {
 	.name		= "i2c",
 	.dev_attrs	= i2c_dev_attrs,
-	.match		= i2c_device_match,
+	.match		= i2c_bus_match,
 	.uevent		= i2c_device_uevent,
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a100c9f..0ca1a59 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -126,7 +126,7 @@ struct i2c_driver {
 	 * With the driver model, device enumeration is NEVER done by drivers;
 	 * it's done by infrastructure.  (NEW STYLE DRIVERS ONLY)
 	 */
-	int (*probe)(struct i2c_client *);
+	int (*probe)(struct i2c_client *, const struct i2c_device_id *id);
 	int (*remove)(struct i2c_client *);
 
 	/* driver model interfaces that don't relate to enumeration  */
@@ -141,11 +141,10 @@ struct i2c_driver {
 
 	struct device_driver driver;
 	struct list_head list;
+	struct i2c_device_id *id_table;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
-#define I2C_NAME_SIZE	20
-
 /**
  * struct i2c_client - represent an I2C slave device
  * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index e9fddb4..688fad6 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -367,4 +367,13 @@ struct virtio_device_id {
 };
 #define VIRTIO_DEV_ANY_ID	0xffffffff
 
+/* i2c */
+
+#define I2C_NAME_SIZE	40
+struct i2c_device_id {
+	char name[I2C_NAME_SIZE];
+	kernel_ulong_t driver_data;	/* Data private to the driver */
+};
+
+
 #endif /* LINUX_MOD_DEVICETABLE_H */

[PATCH 2/5] Modify several rtc drivers to use the alias names list property of i2c

[Jon Smirl]

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 <jonsmirl@gmail.com>
---

 arch/powerpc/sysdev/fsl_soc.c |   44 ++++++-----------------------------------
 drivers/rtc/rtc-ds1307.c      |   38 +++++++++++++++++++----------------
 drivers/rtc/rtc-ds1374.c      |   11 +++++++++-
 drivers/rtc/rtc-rs5c372.c     |   31 ++++++++++++++++-------------
 4 files changed, 53 insertions(+), 71 deletions(-)


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3ace747..268638a 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -320,48 +320,12 @@ arch_initcall(gfar_of_init);
 
 #ifdef CONFIG_I2C_BOARDINFO
 #include <linux/i2c.h>
-struct i2c_driver_device {
-	char	*of_device;
-	char	*i2c_driver;
-	char	*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] __initdata = {
-	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
-	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
-	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
-	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
-	{"dallas,ds1307",  "rtc-ds1307",  "ds1307",},
-	{"dallas,ds1337",  "rtc-ds1307",  "ds1337",},
-	{"dallas,ds1338",  "rtc-ds1307",  "ds1338",},
-	{"dallas,ds1339",  "rtc-ds1307",  "ds1339",},
-	{"dallas,ds1340",  "rtc-ds1307",  "ds1340",},
-	{"stm,m41t00",     "rtc-ds1307",  "m41t00"},
-	{"dallas,ds1374",  "rtc-ds1374",  "rtc-ds1374",},
-};
-
-static int __init of_find_i2c_driver(struct device_node *node,
-				     struct i2c_board_info *info)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
-		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
-			continue;
-		if (strlcpy(info->driver_name, i2c_devices[i].i2c_driver,
-			    KOBJ_NAME_LEN) >= KOBJ_NAME_LEN ||
-		    strlcpy(info->type, i2c_devices[i].i2c_type,
-			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
-			return -ENOMEM;
-		return 0;
-	}
-	return -ENODEV;
-}
 
 static void __init of_register_i2c_devices(struct device_node *adap_node,
 					   int bus_num)
 {
 	struct device_node *node = NULL;
+	const char *compatible;
 
 	while ((node = of_get_next_child(adap_node, node))) {
 		struct i2c_board_info info = {};
@@ -378,8 +342,12 @@ static void __init of_register_i2c_devices(struct device_node *adap_node,
 		if (info.irq == NO_IRQ)
 			info.irq = -1;
 
-		if (of_find_i2c_driver(node, &info) < 0)
+		compatible = of_get_property(node, "compatible", &len);
+		if (!compatible) {
+			printk(KERN_WARNING "i2c-mpc.c: invalid entry, missing compatible attribute\n");
 			continue;
+		}
+		strncpy(info.driver_name, compatible, sizeof(info.driver_name));
 
 		info.addr = *addr;
 
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index bc1c7fe..a4dec4b 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -99,45 +99,46 @@ struct ds1307 {
 };
 
 struct chip_desc {
-	char			name[9];
 	unsigned		nvram56:1;
 	unsigned		alarm:1;
 	enum ds_type		type;
 };
 
 static const struct chip_desc chips[] = { {
-	.name		= "ds1307",
 	.type		= ds_1307,
 	.nvram56	= 1,
 }, {
-	.name		= "ds1337",
 	.type		= ds_1337,
 	.alarm		= 1,
 }, {
-	.name		= "ds1338",
 	.type		= ds_1338,
 	.nvram56	= 1,
 }, {
-	.name		= "ds1339",
 	.type		= ds_1339,
 	.alarm		= 1,
 }, {
-	.name		= "ds1340",
 	.type		= ds_1340,
 }, {
-	.name		= "m41t00",
 	.type		= m41t00,
 }, };
 
-static inline const struct chip_desc *find_chip(const char *s)
-{
-	unsigned i;
-
-	for (i = 0; i < ARRAY_SIZE(chips); i++)
-		if (strnicmp(s, chips[i].name, sizeof chips[i].name) == 0)
-			return &chips[i];
-	return NULL;
-}
+static struct i2c_device_id ds1307_id[] = {
+	{"rtc-ds1307", ds_1307},
+	{"ds1307", ds_1307},
+	{"ds1337", ds_1337},
+	{"ds1338", ds_1338},
+	{"ds1339", ds_1339},
+	{"ds1340", ds_1340},
+	{"m41t00", m41t00},
+	{"dallas,ds1307", ds_1307},
+	{"dallas,ds1337", ds_1337},
+	{"dallas,ds1338", ds_1338},
+	{"dallas,ds1339", ds_1339},
+	{"dallas,ds1340", ds_1340},
+	{"stm,m41t00", m41t00},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ds1307_id);
 
 static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 {
@@ -326,7 +327,7 @@ static struct bin_attribute nvram = {
 
 static struct i2c_driver ds1307_driver;
 
-static int __devinit ds1307_probe(struct i2c_client *client)
+static int __devinit ds1307_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
@@ -334,7 +335,7 @@ static int __devinit ds1307_probe(struct i2c_client *client)
 	const struct chip_desc	*chip;
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 
-	chip = find_chip(client->name);
+	chip = &chips[id->driver_data];
 	if (!chip) {
 		dev_err(&client->dev, "unknown chip type '%s'\n",
 				client->name);
@@ -537,6 +538,7 @@ static struct i2c_driver ds1307_driver = {
 	},
 	.probe		= ds1307_probe,
 	.remove		= __devexit_p(ds1307_remove),
+	.id_table	= ds1307_id,
 };
 
 static int __init ds1307_init(void)
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 45bda18..2b852f3 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -41,6 +41,14 @@
 #define DS1374_REG_SR_AF	0x01 /* Alarm Flag */
 #define DS1374_REG_TCR		0x09 /* Trickle Charge */
 
+static struct i2c_device_id ds1374_id[] = {
+	{"rtc-ds1374", 0},
+	{"ds1374", 0},
+	{"dallas,ds1374", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ds1374_id);
+
 struct ds1374 {
 	struct i2c_client *client;
 	struct rtc_device *rtc;
@@ -355,7 +363,7 @@ static const struct rtc_class_ops ds1374_rtc_ops = {
 	.ioctl = ds1374_ioctl,
 };
 
-static int ds1374_probe(struct i2c_client *client)
+static int ds1374_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct ds1374 *ds1374;
 	int ret;
@@ -429,6 +437,7 @@ static struct i2c_driver ds1374_driver = {
 	},
 	.probe = ds1374_probe,
 	.remove = __devexit_p(ds1374_remove),
+	.id_table = ds1374_id,
 };
 
 static int __init ds1374_init(void)
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 6b67b50..de0d458 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -62,13 +62,26 @@
 
 
 enum rtc_type {
-	rtc_undef = 0,
 	rtc_rs5c372a,
 	rtc_rs5c372b,
 	rtc_rv5c386,
 	rtc_rv5c387a,
 };
 
+static struct i2c_device_id rs5c372_id[] = {
+	{"rtc-rs5c372", rtc_rs5c372a},
+	{"rs5c372a", rtc_rs5c372a},
+	{"rs5c372b", rtc_rs5c372b},
+	{"rv5c386", rtc_rv5c386},
+	{"rv5c387a", rtc_rv5c387a},
+	{"ricoh,rs5c372a", rtc_rs5c372a},
+	{"ricoh,rs5c372b", rtc_rs5c372b},
+	{"ricoh,rv5c386", rtc_rv5c386},
+	{"ricoh,rv5c387a", rtc_rv5c387a},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, rs5c372_id);
+
 /* REVISIT:  this assumes that:
  *  - we're in the 21st century, so it's safe to ignore the century
  *    bit for rv5c38[67] (REG_MONTH bit 7);
@@ -494,7 +507,7 @@ static void rs5c_sysfs_unregister(struct device *dev)
 
 static struct i2c_driver rs5c372_driver;
 
-static int rs5c372_probe(struct i2c_client *client)
+static int rs5c372_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int err = 0;
 	struct rs5c372 *rs5c372;
@@ -522,18 +535,7 @@ static int rs5c372_probe(struct i2c_client *client)
 	if (err < 0)
 		goto exit_kfree;
 
-	if (strcmp(client->name, "rs5c372a") == 0)
-		rs5c372->type = rtc_rs5c372a;
-	else if (strcmp(client->name, "rs5c372b") == 0)
-		rs5c372->type = rtc_rs5c372b;
-	else if (strcmp(client->name, "rv5c386") == 0)
-		rs5c372->type = rtc_rv5c386;
-	else if (strcmp(client->name, "rv5c387a") == 0)
-		rs5c372->type = rtc_rv5c387a;
-	else {
-		rs5c372->type = rtc_rs5c372b;
-		dev_warn(&client->dev, "assuming rs5c372b\n");
-	}
+	rs5c372->type = id->driver_data;
 
 	/* clock may be set for am/pm or 24 hr time */
 	switch (rs5c372->type) {
@@ -651,6 +653,7 @@ static struct i2c_driver rs5c372_driver = {
 	},
 	.probe		= rs5c372_probe,
 	.remove		= rs5c372_remove,
+	.id_table	= rs5c372_id,
 };
 
 static __init int rs5c372_init(void)

[PATCH 3/5] Clean up error returns

[Jon Smirl]

Return errors that were being ignored in the mpc-i2c driver

Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
---

 drivers/i2c/busses/i2c-mpc.c |   30 +++++++++++++++++-------------
 1 files changed, 17 insertions(+), 13 deletions(-)


diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d8de4ac..7c35a8f 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -180,7 +180,7 @@ static void mpc_i2c_stop(struct mpc_i2c *i2c)
 static int mpc_write(struct mpc_i2c *i2c, int target,
 		     const u8 * data, int length, int restart)
 {
-	int i;
+	int i, result;
 	unsigned timeout = i2c->adap.timeout;
 	u32 flags = restart ? CCR_RSTA : 0;
 
@@ -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)
+		return result;
 
 	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)
+			return result;
 	}
 
 	return 0;
@@ -210,7 +212,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 		    u8 * data, int length, int restart)
 {
 	unsigned timeout = i2c->adap.timeout;
-	int i;
+	int i, result;
 	u32 flags = 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)
+		return result;
 
 	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)
+			return result;
 
 		/* 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 = (struct fsl_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);

[PATCH 4/5] Convert PowerPC MPC i2c to of_platform_driver from platform_driver

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

Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
---

 arch/powerpc/sysdev/fsl_soc.c |   96 -------------------------
 drivers/i2c/busses/i2c-mpc.c  |  158 +++++++++++++++++++++++++++++------------
 2 files changed, 110 insertions(+), 144 deletions(-)


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 268638a..d6ef264 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -318,102 +318,6 @@ err:
 
 arch_initcall(gfar_of_init);
 
-#ifdef CONFIG_I2C_BOARDINFO
-#include <linux/i2c.h>
-
-static void __init of_register_i2c_devices(struct device_node *adap_node,
-					   int bus_num)
-{
-	struct device_node *node = NULL;
-	const char *compatible;
-
-	while ((node = of_get_next_child(adap_node, node))) {
-		struct i2c_board_info info = {};
-		const u32 *addr;
-		int len;
-
-		addr = of_get_property(node, "reg", &len);
-		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
-			printk(KERN_WARNING "fsl_soc.c: invalid i2c device entry\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_WARNING "i2c-mpc.c: invalid entry, missing compatible attribute\n");
-			continue;
-		}
-		strncpy(info.driver_name, compatible, sizeof(info.driver_name));
-
-		info.addr = *addr;
-
-		i2c_register_board_info(bus_num, &info, 1);
-	}
-}
-
-static int __init fsl_i2c_of_init(void)
-{
-	struct device_node *np;
-	unsigned int i;
-	struct platform_device *i2c_dev;
-	int ret;
-
-	for (np = NULL, i = 0;
-	     (np = of_find_compatible_node(np, "i2c", "fsl-i2c")) != NULL;
-	     i++) {
-		struct resource r[2];
-		struct fsl_i2c_platform_data i2c_data;
-		const unsigned char *flags = NULL;
-
-		memset(&r, 0, sizeof(r));
-		memset(&i2c_data, 0, sizeof(i2c_data));
-
-		ret = of_address_to_resource(np, 0, &r[0]);
-		if (ret)
-			goto err;
-
-		of_irq_to_resource(np, 0, &r[1]);
-
-		i2c_dev = platform_device_register_simple("fsl-i2c", i, r, 2);
-		if (IS_ERR(i2c_dev)) {
-			ret = PTR_ERR(i2c_dev);
-			goto err;
-		}
-
-		i2c_data.device_flags = 0;
-		flags = of_get_property(np, "dfsrr", NULL);
-		if (flags)
-			i2c_data.device_flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
-
-		flags = of_get_property(np, "fsl5200-clocking", NULL);
-		if (flags)
-			i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
-
-		ret =
-		    platform_device_add_data(i2c_dev, &i2c_data,
-					     sizeof(struct
-						    fsl_i2c_platform_data));
-		if (ret)
-			goto unreg;
-
-		of_register_i2c_devices(np, i);
-	}
-
-	return 0;
-
-unreg:
-	platform_device_unregister(i2c_dev);
-err:
-	return ret;
-}
-
-arch_initcall(fsl_i2c_of_init);
-#endif
-
 #ifdef CONFIG_PPC_83xx
 static int __init mpc83xx_wdt_init(void)
 {
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 7c35a8f..6f080d4 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -17,7 +17,7 @@
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/init.h>
-#include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 #include <asm/io.h>
 #include <linux/fsl_devices.h>
@@ -25,13 +25,13 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 
-#define MPC_I2C_ADDR  0x00
+#define DRV_NAME "mpc-i2c"
+
 #define MPC_I2C_FDR 	0x04
 #define MPC_I2C_CR	0x08
 #define MPC_I2C_SR	0x0c
 #define MPC_I2C_DR	0x10
 #define MPC_I2C_DFSRR 0x14
-#define MPC_I2C_REGION 0x20
 
 #define CCR_MEN  0x80
 #define CCR_MIEN 0x40
@@ -316,105 +316,167 @@ static struct i2c_adapter mpc_ops = {
 	.retries = 1
 };
 
-static int fsl_i2c_probe(struct platform_device *pdev)
+struct i2c_driver_device {
+	char	*of_device;
+	char	*i2c_driver;
+	char	*i2c_type;
+};
+
+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;
+		}
+		strncpy(info.driver_name, compatible, sizeof(info.driver_name));
+		info.type[0] = '\0';
+
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		if (!i2c_new_device(adap, &info)) {
+			printk(KERN_ERR "i2c-mpc.c: Failed to load driver for %s\n", info.driver_name);
+			continue;
+		}
+	}
+}
+
+static int mpc_i2c_probe(struct of_device *op, const struct of_device_id *match)
 {
 	int result = 0;
 	struct mpc_i2c *i2c;
-	struct fsl_i2c_platform_data *pdata;
-	struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
 
 	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
-	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0) {
-		result = -ENXIO;
-		goto fail_get_irq;
-	}
-	i2c->flags = pdata->device_flags;
-	init_waitqueue_head(&i2c->queue);
+	if (of_get_property(op->node, "dfsrr", NULL))
+		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
 
-	i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
+	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;
 	}
 
-	if (i2c->irq != 0)
-		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
-					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
-			printk(KERN_ERR
-			       "i2c-mpc - failed to attach interrupt\n");
-			goto fail_irq;
-		}
+	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);
-	platform_set_drvdata(pdev, i2c);
+
+	dev_set_drvdata(&op->dev, i2c);
 
 	i2c->adap = mpc_ops;
-	i2c->adap.nr = pdev->id;
 	i2c_set_adapdata(&i2c->adap, i2c);
-	i2c->adap.dev.parent = &pdev->dev;
-	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
+	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;
 
-      fail_add:
-	if (i2c->irq != 0)
-		free_irq(i2c->irq, i2c);
-      fail_irq:
+fail_add:
+	free_irq(i2c->irq, i2c);
+fail_request:
+	irq_dispose_mapping(i2c->irq);
+fail_irq:
 	iounmap(i2c->base);
-      fail_map:
-      fail_get_irq:
+fail_map:
 	kfree(i2c);
 	return result;
 };
 
-static int fsl_i2c_remove(struct platform_device *pdev)
+static int mpc_i2c_remove(struct of_device *op)
 {
-	struct mpc_i2c *i2c = platform_get_drvdata(pdev);
+	struct mpc_i2c *i2c = dev_get_drvdata(&op->dev);
 
 	i2c_del_adapter(&i2c->adap);
-	platform_set_drvdata(pdev, NULL);
+	dev_set_drvdata(&op->dev, NULL);
 
 	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
 
+	irq_dispose_mapping(i2c->irq);
 	iounmap(i2c->base);
 	kfree(i2c);
 	return 0;
 };
 
+static struct of_device_id mpc_i2c_of_match[] = {
+	{
+		.compatible	= "fsl-i2c",
+	},
+};
+MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
+
+
 /* Structure for a device driver */
-static struct platform_driver fsl_i2c_driver = {
-	.probe = fsl_i2c_probe,
-	.remove = fsl_i2c_remove,
-	.driver	= {
-		.owner = THIS_MODULE,
-		.name = "fsl-i2c",
+static struct of_platform_driver mpc_i2c_driver = {
+	.match_table	= mpc_i2c_of_match,
+	.probe		= mpc_i2c_probe,
+	.remove		= __devexit_p(mpc_i2c_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= DRV_NAME,
 	},
 };
 
-static int __init fsl_i2c_init(void)
+static int __init mpc_i2c_init(void)
 {
-	return platform_driver_register(&fsl_i2c_driver);
+	int rv;
+
+	rv = of_register_platform_driver(&mpc_i2c_driver);
+	if (rv)
+		printk(KERN_ERR DRV_NAME " of_register_platform_driver failed (%i)\n", rv);
+	return rv;
 }
 
-static void __exit fsl_i2c_exit(void)
+static void __exit mpc_i2c_exit(void)
 {
-	platform_driver_unregister(&fsl_i2c_driver);
+	of_unregister_platform_driver(&mpc_i2c_driver);
 }
 
-module_init(fsl_i2c_init);
-module_exit(fsl_i2c_exit);
+module_init(mpc_i2c_init);
+module_exit(mpc_i2c_exit);
 
 MODULE_AUTHOR("Adrian Cox <adrian@humboldt.co.uk>");
 MODULE_DESCRIPTION

[PATCH 5/5] Convert pfc8563 i2c driver from old style to new style

[Jon Smirl]

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 <jonsmirl@gmail.com>
---

 drivers/rtc/rtc-pcf8563.c |  110 ++++++++++++---------------------------------
 1 files changed, 30 insertions(+), 80 deletions(-)


diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 0242d80..7da2cd0 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -25,10 +25,6 @@
  * located at 0x51 will pass the validation routine due to
  * the way the registers are implemented.
  */
-static unsigned short normal_i2c[] = { I2C_CLIENT_END };
-
-/* Module parameters */
-I2C_CLIENT_INSMOD;
 
 #define PCF8563_REG_ST1		0x00 /* status */
 #define PCF8563_REG_ST2		0x01
@@ -72,9 +68,6 @@ struct pcf8563 {
 	int c_polarity;	/* 0: MO_C=1 means 19xx, otherwise MO_C=1 means 20xx */
 };
 
-static int pcf8563_probe(struct i2c_adapter *adapter, int address, int kind);
-static int pcf8563_detach(struct i2c_client *client);
-
 /*
  * In the routines that deal directly with the pcf8563 hardware, we use
  * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
@@ -257,98 +250,55 @@ static const struct rtc_class_ops pcf8563_rtc_ops = {
 	.set_time	= pcf8563_rtc_set_time,
 };
 
-static int pcf8563_attach(struct i2c_adapter *adapter)
+static int pcf8563_remove(struct i2c_client *client)
 {
-	return i2c_probe(adapter, &addr_data, pcf8563_probe);
+	struct rtc_device *rtc = i2c_get_clientdata(client);
+
+	if (rtc)
+		rtc_device_unregister(rtc);
+
+	return 0;
 }
 
+static struct i2c_device_id pcf8563_id[] = {
+	{"rtc-pcf8563", 0},
+	{"pcf8563", 0},
+	{"philips,pcf8563", 0},
+	{"rtc8564", 0},
+	{"epson,rtc8564", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, pcf8563_id);
+
+static int pcf8563_probe(struct i2c_client *client, const struct i2c_device_id *id);
+
 static struct i2c_driver pcf8563_driver = {
 	.driver		= {
-		.name	= "pcf8563",
+		.name	= "rtc-pcf8563",
 	},
 	.id		= I2C_DRIVERID_PCF8563,
-	.attach_adapter = &pcf8563_attach,
-	.detach_client	= &pcf8563_detach,
+	.probe = &pcf8563_probe,
+	.remove = &pcf8563_remove,
+	.id_table	= pcf8563_id,
 };
 
-static int pcf8563_probe(struct i2c_adapter *adapter, int address, int kind)
+static int pcf8563_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
-	struct pcf8563 *pcf8563;
-	struct i2c_client *client;
+	int result;
 	struct rtc_device *rtc;
 
-	int err = 0;
-
-	dev_dbg(&adapter->dev, "%s\n", __FUNCTION__);
-
-	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
-		err = -ENODEV;
-		goto exit;
-	}
-
-	if (!(pcf8563 = kzalloc(sizeof(struct pcf8563), GFP_KERNEL))) {
-		err = -ENOMEM;
-		goto exit;
-	}
-
-	client = &pcf8563->client;
-	client->addr = address;
-	client->driver = &pcf8563_driver;
-	client->adapter	= adapter;
-
-	strlcpy(client->name, pcf8563_driver.driver.name, I2C_NAME_SIZE);
-
-	/* Verify the chip is really an PCF8563 */
-	if (kind < 0) {
-		if (pcf8563_validate_client(client) < 0) {
-			err = -ENODEV;
-			goto exit_kfree;
-		}
-	}
-
-	/* Inform the i2c layer */
-	if ((err = i2c_attach_client(client)))
-		goto exit_kfree;
-
-	dev_info(&client->dev, "chip found, driver version " DRV_VERSION "\n");
+	result = pcf8563_validate_client(client);
+	if (result)
+		return result;
 
 	rtc = rtc_device_register(pcf8563_driver.driver.name, &client->dev,
 				&pcf8563_rtc_ops, THIS_MODULE);
-
-	if (IS_ERR(rtc)) {
-		err = PTR_ERR(rtc);
-		goto exit_detach;
-	}
+	if (IS_ERR(rtc))
+		return PTR_ERR(rtc);
 
 	i2c_set_clientdata(client, rtc);
 
 	return 0;
-
-exit_detach:
-	i2c_detach_client(client);
-
-exit_kfree:
-	kfree(pcf8563);
-
-exit:
-	return err;
-}
-
-static int pcf8563_detach(struct i2c_client *client)
-{
-	struct pcf8563 *pcf8563 = container_of(client, struct pcf8563, client);
-	int err;
-	struct rtc_device *rtc = i2c_get_clientdata(client);
-
-	if (rtc)
-		rtc_device_unregister(rtc);
-
-	if ((err = i2c_detach_client(client)))
-		return err;
-
-	kfree(pcf8563);
-
-	return 0;
 }
 
 static int __init pcf8563_init(void)

Re: [PATCH 0/5] Series to add device tree naming to i2c

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1644 bytes --]

On Mon, 10 Dec 2007, Jon Smirl wrote:
> Respin to split error return fixups out of mpc-i2c to of_platform change
> 
> The following series implements standard linux module aliasing for i2c modules
> 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. 

Is there any relationship of these patches with commit
cee37ae4071740cb190d1ac4ddb7aa77484aa7b3?

Author: Jean Delvare <khali@linux-fr.org>
Date:   Sat Oct 13 23:56:29 2007 +0200

    i2c: Kill struct i2c_device_id
    
    I2C devices do not have any form of ID as PCI or USB devices have.
    No driver uses "MODULE_DEVICE_TABLE(i2c, ...)" because it doesn't
    make sense. So we can get rid of struct i2c_device_id and the
    associated support code.
    
    Signed-off-by: Jean Delvare <khali@linux-fr.org>
    Cc: Greg KH <greg@kroah.com>

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [PATCH 0/5] Series to add device tree naming to i2c

[Jon Smirl]

On 12/11/07, Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> On Mon, 10 Dec 2007, Jon Smirl wrote:
> > Respin to split error return fixups out of mpc-i2c to of_platform chang=
e
> >
> > The following series implements standard linux module aliasing for i2c =
modules
> > 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. Modu=
le
> > aliasing is used to translate from device tree names into to linux kern=
el
> > names. Several i2c drivers are updated to use the new aliasing.
>
> Is there any relationship of these patches with commit
> cee37ae4071740cb190d1ac4ddb7aa77484aa7b3?

That commit appears to be removing an entry for i2c in
mod_devicetable.h. But there was never any code in place to use that
struct which is why it was removed.

I have added the struct back with a name field instead of an id. My
patches also include the code necessary to make it all work.

>
> Author: Jean Delvare <khali@linux-fr.org>
> Date:   Sat Oct 13 23:56:29 2007 +0200
>
>     i2c: Kill struct i2c_device_id
>
>     I2C devices do not have any form of ID as PCI or USB devices have.
>     No driver uses "MODULE_DEVICE_TABLE(i2c, ...)" because it doesn't
>     make sense. So we can get rid of struct i2c_device_id and the
>     associated support code.
>
>     Signed-off-by: Jean Delvare <khali@linux-fr.org>
>     Cc: Greg KH <greg@kroah.com>
>
> With kind regards,
>
> Geert Uytterhoeven
> Software Architect
>
> Sony Network and Software Technology Center Europe
> The Corporate Village =B7 Da Vincilaan 7-D1 =B7 B-1935 Zaventem =B7 Belgi=
um
>
> Phone:    +32 (0)2 700 8453
> Fax:      +32 (0)2 700 8622
> E-mail:   Geert.Uytterhoeven@sonycom.com
> Internet: http://www.sony-europe.com/
>
> Sony Network and Software Technology Center Europe
> A division of Sony Service Centre (Europe) N.V.
> Registered office: Technologielaan 7 =B7 B-1840 Londerzeel =B7 Belgium
> VAT BE 0413.825.160 =B7 RPR Brussels
> Fortis Bank Zaventem =B7 Swift GEBABEBB08A =B7 IBAN BE39001382358619


--=20
Jon Smirl
jonsmirl@gmail.com

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

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

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 

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

[Jon Smirl]

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 <jonsmirl@gmail.com>
---

 drivers/i2c/i2c-core.c          |   32 ++++++++++++++++++++++++++------
 include/linux/i2c.h             |    9 ++++-----
 include/linux/mod_devicetable.h |   20 ++++++++++++++++++++
 scripts/mod/file2alias.c        |   19 +++++++++++++++++++
 4 files changed, 69 insertions(+), 11 deletions(-)


diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b5e13e4..fce06fd 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -47,10 +47,25 @@ static DEFINE_IDR(i2c_adapter_idr);
 
 /* ------------------------------------------------------------------------- */
 
-static int i2c_device_match(struct device *dev, struct device_driver *drv)
+static const struct i2c_device_id *i2c_device_match(const struct i2c_device_id *id, struct i2c_client *client)
+{
+	/* 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)
+				return id;
+			id++;
+		}
+	}
+	return NULL;
+}
+
+static int i2c_bus_match(struct device *dev, struct device_driver *drv)
 {
 	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.
-	 */
+	/* match on an id table if there is one */
+	found_id = i2c_device_match(driver->id_table, client);
+	if (found_id)
+		return 1;
+
 	return strcmp(client->driver_name, drv->name) == 0;
 }
 
@@ -89,12 +106,15 @@ static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
+	const struct i2c_device_id *id;
 
 	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+
+	id = i2c_device_match(driver->id_table, client);
+	return driver->probe(client, id);
 }
 
 static int i2c_device_remove(struct device *dev)
@@ -189,7 +209,7 @@ static struct device_attribute i2c_dev_attrs[] = {
 static struct bus_type i2c_bus_type = {
 	.name		= "i2c",
 	.dev_attrs	= i2c_dev_attrs,
-	.match		= i2c_device_match,
+	.match		= i2c_bus_match,
 	.uevent		= i2c_device_uevent,
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index a100c9f..49fc682 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -126,7 +126,7 @@ struct i2c_driver {
 	 * With the driver model, device enumeration is NEVER done by drivers;
 	 * it's done by infrastructure.  (NEW STYLE DRIVERS ONLY)
 	 */
-	int (*probe)(struct i2c_client *);
+	int (*probe)(struct i2c_client *, const struct i2c_device_id *id);
 	int (*remove)(struct i2c_client *);
 
 	/* driver model interfaces that don't relate to enumeration  */
@@ -141,11 +141,10 @@ struct i2c_driver {
 
 	struct device_driver driver;
 	struct list_head list;
+	struct i2c_device_id *id_table;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
-#define I2C_NAME_SIZE	20
-
 /**
  * struct i2c_client - represent an I2C slave device
  * @flags: I2C_CLIENT_TEN indicates the device uses a ten bit chip address;
@@ -179,7 +178,7 @@ struct i2c_client {
 					/* to the client		*/
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device (or -1) */
-	char driver_name[KOBJ_NAME_LEN];
+	char driver_name[I2C_NAME_SIZE];
 	struct list_head list;
 	struct completion released;
 };
@@ -223,7 +222,7 @@ static inline void i2c_set_clientdata (struct i2c_client *dev, void *data)
  * with the adapter already known.
  */
 struct i2c_board_info {
-	char		driver_name[KOBJ_NAME_LEN];
+	char		driver_name[I2C_NAME_SIZE];
 	char		type[I2C_NAME_SIZE];
 	unsigned short	flags;
 	unsigned short	addr;
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index e9fddb4..d66038a 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -367,4 +367,24 @@ struct virtio_device_id {
 };
 #define VIRTIO_DEV_ANY_ID	0xffffffff
 
+/* i2c */
+
+/* These defines are used to separate PowerPC open firmware
+ * drivers into their own namespace */
+#define I2C_NAME_SIZE	(16*3)
+#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
+
+struct i2c_device_id {
+	char name[I2C_NAME_SIZE];
+	kernel_ulong_t driver_data;	/* Data private to the driver */
+};
+
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index d802b5a..da43742 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -539,6 +539,21 @@ static int do_virtio_entry(const char *filename, struct virtio_device_id *id,
 	return 1;
 }
 
+/* Looks like: i2c:Ns */
+static int do_i2c_entry(const char *filename, struct i2c_device_id *id,
+			   char *alias)
+{
+    char *tmp;
+    sprintf (alias, I2C_MODULE_PREFIX "%s", id->name);
+
+    /* Replace all whitespace with underscores */
+    for (tmp = alias; tmp && *tmp; tmp++)
+        if (isspace (*tmp))
+            *tmp = '_';
+
+    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);
 }
 

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

[Jean Delvare]

Hi Jon,

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

Your patch adds compilation warnings on several i2c drivers:

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

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

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

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

Line too long, please fold.

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

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

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

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

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

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

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

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

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

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

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

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

Ditto.

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

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

> +#define I2C_MODULE_PREFIX "i2c:N"

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

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

This one isn't used anywhere?

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

Coding style: space afte opening curly brace.

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

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

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

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

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

-- 
Jean Delvare

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

[Jean Delvare]

Hi Jon,

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

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

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

-- 
Jean Delvare

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

[Jon Smirl]

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

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

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



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


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Jean Delvare]

Hi Jon,

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

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

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

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

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

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

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

-- 
Jean Delvare

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

[Jon Smirl]

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

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

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

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

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

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

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


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Jean Delvare]

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

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

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

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

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

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

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

-- 
Jean Delvare

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

[Jon Smirl]

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

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

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

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

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

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

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


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Jean Delvare]

Hi Jon,

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

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

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

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

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

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

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

-- 
Jean Delvare

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

[Jon Smirl]

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

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

-- 
Jon Smirl
jonsmirl@gmail.com

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

[Jean Delvare]

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

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

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

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

-- 
Jean Delvare