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

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

[Jon Smirl]

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/4] 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.
---

 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..76f48be 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..56d2dca 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/4] 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
---

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


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3ace747..e4c766e 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,9 +342,13 @@ 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;
 
 		i2c_register_board_info(bus_num, &info, 1);
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/4] 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.
---

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


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index e4c766e..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 d8de4ac..bb6284e 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
@@ -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,15 @@ 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;
+	if ((result = i2c_wait(i2c, timeout, 1)) < 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;
+		if ((result = i2c_wait(i2c, timeout, 1)) < 0)
+			return result;
 	}
 
 	return 0;
@@ -210,7 +210,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 +221,8 @@ 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;
+	if ((result = i2c_wait(i2c, timeout, 1)) < 0)
+		return result;
 
 	if (length) {
 		if (length == 1)
@@ -234,8 +234,8 @@ 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;
+		if ((result = i2c_wait(i2c, timeout, 0)) < 0)
+			return result;
 
 		/* Generate txack on next to last byte */
 		if (i == length - 2)
@@ -312,105 +312,166 @@ 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;
 
-	if (!(i2c = kzalloc(sizeof(*i2c), GFP_KERNEL))) {
+	if (!(i2c = kzalloc(sizeof(*i2c), GFP_KERNEL)))
 		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;
+	}
+	
+	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_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;
+	if ((result = i2c_add_adapter(&i2c->adap)) < 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 != 0)
+	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;
+	}
+	return 0;
 }
 
-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 4/4] 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.
---

 drivers/rtc/rtc-pcf8563.c |  114 +++++++++++++--------------------------------
 1 files changed, 32 insertions(+), 82 deletions(-)


diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 0242d80..20b1acf 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/4] Series to add device tree naming to i2c

[Olof Johansson]

On Mon, Dec 03, 2007 at 04:20:32PM -0500, Jon Smirl wrote:
> 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. 

May I ask why you want to modify the i2c layer instead of keeping the
OF->i2c driver mapping in PPC code? It seems simpler to keep it in the
PPC-specific code, since otherwise you might end up with confused i2c
driver writers that make up their own OF names without knowing for sure
that's what will be used. No?

I recently posted (and asked Paulus to pull) a patch where I consolidate
the fsl_soc mapping code so I can also use that on pasemi, and modified
the pasemi platform code to setup the board_info from the device tree
accordingly.

Finally, whitespace is broken in several of your patches.


-Olof

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

[Scott Wood]

Olof Johansson wrote:
> On Mon, Dec 03, 2007 at 04:20:32PM -0500, Jon Smirl wrote:
>> 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. 
> 
> May I ask why you want to modify the i2c layer instead of keeping the
> OF->i2c driver mapping in PPC code?

Because it doesn't belong there -- at the least, it should be in 
drivers/of.  But putting it in the driver is better, IMHO.

> It seems simpler to keep it in the
> PPC-specific code, since otherwise you might end up with confused i2c
> driver writers that make up their own OF names without knowing for sure
> that's what will be used. No?

How is this different from drivers that have of_platform bindings?

That said, there should probably be some sort of tag to indicate the 
namespace being matched against (OF, Linux, etc), to avoid matching an 
OF device with a non-OF name (or vice versa).

> I recently posted (and asked Paulus to pull) a patch where I consolidate
> the fsl_soc mapping code so I can also use that on pasemi, and modified
> the pasemi platform code to setup the board_info from the device tree
> accordingly.

Having just one bit i2c glue code for arch/powerpc is certainly an 
improvement over the current situation, but it's not really powerpc 
specific.  Just because other architectures don't use it now doesn't 
mean they won't in the future -- and I don't like the "we'll move it 
when they do" argument because I want to make it easy for other 
architectures to decide to use it. :-)

-Scott

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

[Jon Smirl]

On 12/3/07, Olof Johansson <olof@lixom.net> wrote:
> On Mon, Dec 03, 2007 at 04:20:32PM -0500, Jon Smirl wrote:
> > 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.
>
> May I ask why you want to modify the i2c layer instead of keeping the
> OF->i2c driver mapping in PPC code? It seems simpler to keep it in the
> PPC-specific code, since otherwise you might end up with confused i2c
> driver writers that make up their own OF names without knowing for sure
> that's what will be used. No?

Audio codecs have the same problem. That table is could end up with
hundreds of entries. The right place to store the mappings is in the
device driver for the device.

A side effect of moving the mappings into the device drivers is that
the correct i2c drivers can be automatically loaded by the kernel.
This lets you make distro CDs with all of the i2c drivers on disk and
the device tree will cause insmod of the correct ones.

I'm working on a parallel patch for Alsa SOC but it isn't ready yet.

>
> I recently posted (and asked Paulus to pull) a patch where I consolidate
> the fsl_soc mapping code so I can also use that on pasemi, and modified
> the pasemi platform code to setup the board_info from the device tree
> accordingly.
>
> Finally, whitespace is broken in several of your patches.
>
>
> -Olof
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Olof Johansson]

On Mon, Dec 03, 2007 at 06:52:06PM -0500, Jon Smirl wrote:
> On 12/3/07, Olof Johansson <olof@lixom.net> wrote:
> > On Mon, Dec 03, 2007 at 04:20:32PM -0500, Jon Smirl wrote:
> > > 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.
> >
> > May I ask why you want to modify the i2c layer instead of keeping the
> > OF->i2c driver mapping in PPC code? It seems simpler to keep it in the
> > PPC-specific code, since otherwise you might end up with confused i2c
> > driver writers that make up their own OF names without knowing for sure
> > that's what will be used. No?
> 
> Audio codecs have the same problem. That table is could end up with
> hundreds of entries. The right place to store the mappings is in the
> device driver for the device.
> 
> A side effect of moving the mappings into the device drivers is that
> the correct i2c drivers can be automatically loaded by the kernel.
> This lets you make distro CDs with all of the i2c drivers on disk and
> the device tree will cause insmod of the correct ones.

Ok, good enough reasons. I'll back out my i2c commits and wait for yours
to settle, then add the pasemi stuff in the same way.

(Whitespace comments still apply).


-Olof

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

[Jon Smirl]

What is the status of this series, is there anything I can do to help
get this into the i2c subsystem?

On 12/3/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> 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
>
> _______________________________________________
> i2c mailing list
> i2c@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Olof Johansson]

On Sun, Dec 09, 2007 at 03:24:55PM -0500, Jon Smirl wrote:
> What is the status of this series, is there anything I can do to help
> get this into the i2c subsystem?

I never saw the comments about whitespace cleanups being addressed. If
you run them through checkpatch you'll see quite a few things that needs
fixups. Care to do the round of cleanups and repost the series?

Besides that I'm fine with the approach of moving the matching to the
i2c layer, it does seem like a reasonable thing to do.


-Olof

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

[Benjamin Herrenschmidt]

On Sun, 2007-12-09 at 15:24 -0500, Jon Smirl wrote:
> What is the status of this series, is there anything I can do to help
> get this into the i2c subsystem?

I think there were a few comments such as whitespace issues and Scott
had a comment about a tag to differenciate matching type.

Thus the expectation is that you'll respin the serie with those
addressed.

Ben.

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

[Jon Smirl]

On 12/9/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sun, 2007-12-09 at 15:24 -0500, Jon Smirl wrote:
> > What is the status of this series, is there anything I can do to help
> > get this into the i2c subsystem?
>
> I think there were a few comments such as whitespace issues and Scott
> had a comment about a tag to differenciate matching type.

Are there technical concerns with this series? The white space can be
fixed in a few minutes.

Adding a tag to differentiate matching types has implications that are
broader than just i2c. Shouldn't we do this first with the existing
scheme and then change the tagging process with later patches?

>
> Thus the expectation is that you'll respin the serie with those
> addressed.
>
> Ben.
>
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Benjamin Herrenschmidt]

On Sun, 2007-12-09 at 15:57 -0500, Jon Smirl wrote:
> 
> Are there technical concerns with this series? The white space can be
> fixed in a few minutes.
> 
> Adding a tag to differentiate matching types has implications that are
> broader than just i2c. Shouldn't we do this first with the existing
> scheme and then change the tagging process with later patches?

No, we should decide on what to do with the tagging process (or not do)
first, don't you think ? (If we need a tagging process, Scott had a
concern but it might be moot, let's discuss that first).

Ben.

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

[Jon Smirl]

On 12/9/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Sun, 2007-12-09 at 15:57 -0500, Jon Smirl wrote:
> >
> > Are there technical concerns with this series? The white space can be
> > fixed in a few minutes.
> >
> > Adding a tag to differentiate matching types has implications that are
> > broader than just i2c. Shouldn't we do this first with the existing
> > scheme and then change the tagging process with later patches?
>
> No, we should decide on what to do with the tagging process (or not do)
> first, don't you think ? (If we need a tagging process, Scott had a
> concern but it might be moot, let's discuss that first).

Right now the tags are simply strings. The second parameter is driver specific.

+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},
+       {},
+};

The current mechanism is simple string matching there are no platform
specific namespaces.

We could wrap the device tree style names in a macro that adds a
non-printable character to the front.

Something like this:
#define DT_NAMESPACE "\1"
#define DT_NAME(x) (DT_NAMESPACE x)
+       {DT_NAME("ricoh,rv5c386"), rtc_rv5c386},

And then modify the mpc i2c driver to insert the DT_NAMESPACE in front
of the string.

Another solution would be to make the names disappear on non-device
tree platforms

in mod_devicetable.h:
#ifdef USING_DEVICE_TREES
#define DT_NAME(x) x
#else
#define DTNAME(x)
#endif

+static struct i2c_device_id rs5c372_id[] = {
+       {"rtc-rs5c372", rtc_rs5c372a},
+       {"rs5c372a", rtc_rs5c372a},
+       {"rs5c372b", rtc_rs5c372b},
+       {"rv5c386", rtc_rv5c386},
+       {"rv5c387a", rtc_rv5c387a},
+       DT_NAME({"ricoh,rs5c372a", rtc_rs5c372a},)
+       DT_NAME({"ricoh,rs5c372b", rtc_rs5c372b},)
+       DT_NAME({"ricoh,rv5c386", rtc_rv5c386},)
+       DT_NAME({"ricoh,rv5c387a", rtc_rv5c387a},)
+       {},

But what's the point in making these names specific to device trees?
They are perfectly valid names for the devices that could be used from
any platform.

-- 
Jon Smirl
jonsmirl@gmail.com

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

[Benjamin Herrenschmidt]

> +static struct i2c_device_id rs5c372_id[] = {
> +       {"rtc-rs5c372", rtc_rs5c372a},
> +       {"rs5c372a", rtc_rs5c372a},
> +       {"rs5c372b", rtc_rs5c372b},
> +       {"rv5c386", rtc_rv5c386},
> +       {"rv5c387a", rtc_rv5c387a},
> +       DT_NAME({"ricoh,rs5c372a", rtc_rs5c372a},)
> +       DT_NAME({"ricoh,rs5c372b", rtc_rs5c372b},)
> +       DT_NAME({"ricoh,rv5c386", rtc_rv5c386},)
> +       DT_NAME({"ricoh,rv5c387a", rtc_rv5c387a},)
> +       {},
> 
> But what's the point in making these names specific to device trees?
> They are perfectly valid names for the devices that could be used from
> any platform.

The more I think about it, the more I tend to agree that tagging isn't
necessary and you are right. We should just match the name against the
"compatible" property of the OF nodes (which mean we need to support
multiple matches though since "compatible" is a list of strings).

Now, I have a question about your example: Why do you have both
"rs5c372a" and "ricoh,rs5c372a" ?

I would argue that we should keep only the later...

Cheers,
Ben.

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

[Jon Smirl]

On 12/9/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > +static struct i2c_device_id rs5c372_id[] = {
> > +       {"rtc-rs5c372", rtc_rs5c372a},
> > +       {"rs5c372a", rtc_rs5c372a},
> > +       {"rs5c372b", rtc_rs5c372b},
> > +       {"rv5c386", rtc_rv5c386},
> > +       {"rv5c387a", rtc_rv5c387a},
> > +       DT_NAME({"ricoh,rs5c372a", rtc_rs5c372a},)
> > +       DT_NAME({"ricoh,rs5c372b", rtc_rs5c372b},)
> > +       DT_NAME({"ricoh,rv5c386", rtc_rv5c386},)
> > +       DT_NAME({"ricoh,rv5c387a", rtc_rv5c387a},)
> > +       {},
> >
> > But what's the point in making these names specific to device trees?
> > They are perfectly valid names for the devices that could be used from
> > any platform.
>
> The more I think about it, the more I tend to agree that tagging isn't
> necessary and you are right. We should just match the name against the
> "compatible" property of the OF nodes (which mean we need to support
> multiple matches though since "compatible" is a list of strings).
>
> Now, I have a question about your example: Why do you have both
> "rs5c372a" and "ricoh,rs5c372a" ?

The "rs5c372a" is unrelated to the device tree changes. In the
existing i2c driver code the driver is named rtc-rs5c372. But this
driver supports five different devices. A secondary i2c parameter
(driver_name, name) is used to tell the rtc-rs5c372 driver that it is
being loaded for use on a rs5c372a, rv5c387a, etc. When I fixed i2c to
support device tree name aliases I also fixed it to use kernel
aliasing to support these drivers that support multiple devices.

>
> I would argue that we should keep only the later...
>
> Cheers,
> Ben.
>
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Olof Johansson]

On Mon, Dec 10, 2007 at 08:38:46AM +1100, Benjamin Herrenschmidt wrote:
> 
> > +static struct i2c_device_id rs5c372_id[] = {
> > +       {"rtc-rs5c372", rtc_rs5c372a},
> > +       {"rs5c372a", rtc_rs5c372a},
> > +       {"rs5c372b", rtc_rs5c372b},
> > +       {"rv5c386", rtc_rv5c386},
> > +       {"rv5c387a", rtc_rv5c387a},
> > +       DT_NAME({"ricoh,rs5c372a", rtc_rs5c372a},)
> > +       DT_NAME({"ricoh,rs5c372b", rtc_rs5c372b},)
> > +       DT_NAME({"ricoh,rv5c386", rtc_rv5c386},)
> > +       DT_NAME({"ricoh,rv5c387a", rtc_rv5c387a},)
> > +       {},
> > 
> > But what's the point in making these names specific to device trees?
> > They are perfectly valid names for the devices that could be used from
> > any platform.
> 
> The more I think about it, the more I tend to agree that tagging isn't
> necessary and you are right. We should just match the name against the
> "compatible" property of the OF nodes (which mean we need to support
> multiple matches though since "compatible" is a list of strings).
> 
> Now, I have a question about your example: Why do you have both
> "rs5c372a" and "ricoh,rs5c372a" ?
> 
> I would argue that we should keep only the later...

I think existing platforms register with the simpler name, so they would
need to be changed. Having both shouldn't do harm though, we tend to
sometimes do the same with of_platform drivers for legacy reasons.

Unless someone adds a ridicously simple match name (by mistake or
whatever), it shouldn't be a problem. And if someone does it to some
dts/firmware, then we'll just need to add device tree fixups to set the
"vendor,product" string instead, yet again similar to how we have to do
sometimes with other drivers/devices.


-Olof

[PATCH 4/4] 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: [i2c] [PATCH 0/4] Series to add device tree naming to i2c

[Scott Wood]

On Mon, Dec 10, 2007 at 08:38:46AM +1100, Benjamin Herrenschmidt wrote:
> The more I think about it, the more I tend to agree that tagging isn't
> necessary and you are right. We should just match the name against the
> "compatible" property of the OF nodes (which mean we need to support
> multiple matches though since "compatible" is a list of strings).

It may not be strictly necessary, but I think it's a good idea not just for
safety reasons, but as an indication to the driver what additional
information it has access to.  We could put a match data pointer in the i2c
device, and have it be a valid node pointer if the match was an OF one (and
a device-specific struct for a straight platform device, etc).  This could
be useful if a device needs to have more properties than standard
address/type/interrupt for some reason.

-Scott

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

[Jon Smirl]

On 12/10/07, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, Dec 10, 2007 at 08:38:46AM +1100, Benjamin Herrenschmidt wrote:
> > The more I think about it, the more I tend to agree that tagging isn't
> > necessary and you are right. We should just match the name against the
> > "compatible" property of the OF nodes (which mean we need to support
> > multiple matches though since "compatible" is a list of strings).
>
> It may not be strictly necessary, but I think it's a good idea not just for
> safety reasons, but as an indication to the driver what additional
> information it has access to.  We could put a match data pointer in the i2c
> device, and have it be a valid node pointer if the match was an OF one (and
> a device-specific struct for a straight platform device, etc).  This could
> be useful if a device needs to have more properties than standard
> address/type/interrupt for some reason.

I can't see an easy way to do this. The basic problem is that the i2c
drivers are assumed to be cross platform. I would need to add a path
through the i2c core for getting a void pointer from the bus to the
device But then when the device code gets this pointer it has no way
of knowing what it was. Assuming the void is a pointer to an of_node
would make the driver for the i2c device platform specific.

Another way that would work cross platform would be for the module to
have module parameters for the extra attributes. The bus code could
then look in the of_node for extra attributes and use them to set the
module parameters. I know this can be done, but doing it is a little
above my understanding of the module code. You need to ask the module
it's parameter names, addresses and types and then match them to
attributes in the of_node and do the copy.

>
> -Scott
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Scott Wood]

Jon Smirl wrote:
> I can't see an easy way to do this. The basic problem is that the i2c
>  drivers are assumed to be cross platform.

It'd be a small binding-specific portion, similar to an of_platform stub
on a generic driver.  It could probably wait until an actual need
arises, though.

> I would need to add a path through the i2c core for getting a void
> pointer from the bus to the device But then when the device code gets
> this pointer it has no way of knowing what it was.

It'd need to know which binding/name combination it matched against
(similar to how of_platform does it).

> Another way that would work cross platform would be for the module to
>  have module parameters for the extra attributes.

Ick.  Module parameters are a PITA, and have to be duplicated with 
command line parameters if you want to support non-modular builds.

-Scott

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

[Jon Smirl]

On 12/10/07, Scott Wood <scottwood@freescale.com> wrote:
> Jon Smirl wrote:
> > I can't see an easy way to do this. The basic problem is that the i2c
> >  drivers are assumed to be cross platform.
>
> It'd be a small binding-specific portion, similar to an of_platform stub
> on a generic driver.  It could probably wait until an actual need
> arises, though.
>
> > I would need to add a path through the i2c core for getting a void
> > pointer from the bus to the device But then when the device code gets
> > this pointer it has no way of knowing what it was.
>
> It'd need to know which binding/name combination it matched against
> (similar to how of_platform does it).
>
> > Another way that would work cross platform would be for the module to
> >  have module parameters for the extra attributes.
>
> Ick.  Module parameters are a PITA, and have to be duplicated with
> command line parameters if you want to support non-modular builds.

You don't have to duplicate module parameters in non-modular builds
(you needed to a couple of years ago). Even if the module is linked in
you can still set the parameters from the kernel command line without
doing anything special. Just use module.param = value.

>
> -Scott
>


-- 
Jon Smirl
jonsmirl@gmail.com

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

[Benjamin Herrenschmidt]

On Mon, 2007-12-10 at 10:42 -0600, Scott Wood wrote:
> On Mon, Dec 10, 2007 at 08:38:46AM +1100, Benjamin Herrenschmidt wrote:
> > The more I think about it, the more I tend to agree that tagging isn't
> > necessary and you are right. We should just match the name against the
> > "compatible" property of the OF nodes (which mean we need to support
> > multiple matches though since "compatible" is a list of strings).
> 
> It may not be strictly necessary, but I think it's a good idea not just for
> safety reasons, but as an indication to the driver what additional
> information it has access to.  We could put a match data pointer in the i2c
> device, and have it be a valid node pointer if the match was an OF one (and
> a device-specific struct for a straight platform device, etc).  This could
> be useful if a device needs to have more properties than standard
> address/type/interrupt for some reason.

You don't need that much... On ppc64, all devices have an optional
device node pointer in struct device via the archdata, an we should do
that on ppc32 too (and will soon in order to merge some of the PCI & DMA
stuff anyway).

Ben.

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

[Benjamin Herrenschmidt]

On Mon, 2007-12-10 at 13:06 -0500, Jon Smirl wrote:
> 
> I can't see an easy way to do this. The basic problem is that the i2c
> drivers are assumed to be cross platform. I would need to add a path
> through the i2c core for getting a void pointer from the bus to the
> device But then when the device code gets this pointer it has no way
> of knowing what it was. Assuming the void is a pointer to an of_node
> would make the driver for the i2c device platform specific.

As I said, there's an arch data structure that can be added to -any-
struct device and that we use for, among others, an optional device tree
node pointer on ppc64. We should do that on ppc32 too (and will soon for
other reasons). In this case, the i2c core can be modified on powerpc so
that when it instanciate an i2c device from the device-tree, it fills
that field.

Some device drivers can have powerpc specific code that make good use of
that for things like calibration infos etc...

Ben.