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

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

[Jon Smirl]

Updated to reflect comments in:
http://lkml.org/lkml/2008/1/11/272

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.
http://lkml.org/lkml/2007/12/17/493

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 19 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/hwmon/f75375s.c         |    4 ++--
 drivers/i2c/chips/ds1682.c      |    2 +-
 drivers/i2c/chips/menelaus.c    |    2 +-
 drivers/i2c/chips/tps65010.c    |    2 +-
 drivers/i2c/chips/tsl2550.c     |    2 +-
 drivers/i2c/i2c-core.c          |   24 +++++++++++++++++++++++-
 drivers/rtc/rtc-ds1307.c        |    2 +-
 drivers/rtc/rtc-ds1374.c        |    2 +-
 drivers/rtc/rtc-m41t80.c        |    2 +-
 drivers/rtc/rtc-rs5c372.c       |    2 +-
 include/linux/i2c.h             |    5 ++---
 include/linux/mod_devicetable.h |   19 +++++++++++++++++++
 scripts/mod/file2alias.c        |   14 ++++++++++++++
 13 files changed, 68 insertions(+), 14 deletions(-)


diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
index 6892f76..2247de6 100644
--- a/drivers/hwmon/f75375s.c
+++ b/drivers/hwmon/f75375s.c
@@ -117,7 +117,7 @@ struct f75375_data {
 static int f75375_attach_adapter(struct i2c_adapter *adapter);
 static int f75375_detect(struct i2c_adapter *adapter, int address, int kind);
 static int f75375_detach_client(struct i2c_client *client);
-static int f75375_probe(struct i2c_client *client);
+static int f75375_probe(struct i2c_client *client, const struct i2c_device_id *id);
 static int f75375_remove(struct i2c_client *client);
 
 static struct i2c_driver f75375_legacy_driver = {
@@ -628,7 +628,7 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
 
 }
 
-static int f75375_probe(struct i2c_client *client)
+static int f75375_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct f75375_data *data = i2c_get_clientdata(client);
 	struct f75375s_platform_data *f75375s_pdata = client->dev.platform_data;
diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
index 9e94542..93c0441 100644
--- a/drivers/i2c/chips/ds1682.c
+++ b/drivers/i2c/chips/ds1682.c
@@ -200,7 +200,7 @@ static struct bin_attribute ds1682_eeprom_attr = {
 /*
  * Called when a ds1682 device is matched with this driver
  */
-static int ds1682_probe(struct i2c_client *client)
+static int ds1682_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int rc;
 
diff --git a/drivers/i2c/chips/menelaus.c b/drivers/i2c/chips/menelaus.c
index 2dea012..89ef9b6 100644
--- a/drivers/i2c/chips/menelaus.c
+++ b/drivers/i2c/chips/menelaus.c
@@ -1149,7 +1149,7 @@ static inline void menelaus_rtc_init(struct menelaus_chip *m)
 
 static struct i2c_driver menelaus_i2c_driver;
 
-static int menelaus_probe(struct i2c_client *client)
+static int menelaus_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct menelaus_chip	*menelaus;
 	int			rev = 0, val;
diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
index e320994..6b13642 100644
--- a/drivers/i2c/chips/tps65010.c
+++ b/drivers/i2c/chips/tps65010.c
@@ -465,7 +465,7 @@ static int __exit tps65010_remove(struct i2c_client *client)
 	return 0;
 }
 
-static int tps65010_probe(struct i2c_client *client)
+static int tps65010_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct tps65010		*tps;
 	int			status;
diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/i2c/chips/tsl2550.c
index 3de4b19..27c553d 100644
--- a/drivers/i2c/chips/tsl2550.c
+++ b/drivers/i2c/chips/tsl2550.c
@@ -364,7 +364,7 @@ static int tsl2550_init_client(struct i2c_client *client)
  */
 
 static struct i2c_driver tsl2550_driver;
-static int __devinit tsl2550_probe(struct i2c_client *client)
+static int __devinit tsl2550_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct tsl2550_data *data;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b5e13e4..5f62230 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -47,6 +47,19 @@ static DEFINE_IDR(i2c_adapter_idr);
 
 /* ------------------------------------------------------------------------- */
 
+static const struct i2c_device_id *i2c_match_id(
+		const struct i2c_device_id *id, struct i2c_client *client)
+{
+	/* only powerpc drivers implement the id_table,
+	 * it is empty on other platforms */
+	while (id->name[0]) {
+		if (strcmp(client->name, id->name) == 0)
+			return id;
+		id++;
+	}
+	return NULL;
+}
+
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
@@ -58,6 +71,10 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 	if (!is_newstyle_driver(driver))
 		return 0;
 
+	/* match on an id table if there is one */
+	if (driver->id_table)
+		return i2c_match_id(driver->id_table, client) != NULL;
+
 	/* new style drivers use the same kind of driver matching policy
 	 * as platform devices or SPI:  compare device and driver IDs.
 	 */
@@ -89,12 +106,17 @@ 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 = NULL;
 
 	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+
+	if (driver->id_table)
+		id = i2c_match_id(driver->id_table, client);
+	
+	return driver->probe(client, id);
 }
 
 static int i2c_device_remove(struct device *dev)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index bc1c7fe..9b0eab9 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -326,7 +326,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;
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 45bda18..dab6008 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -355,7 +355,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;
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 1cb33ca..457f2ca 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -756,7 +756,7 @@ static struct notifier_block wdt_notifier = {
  *
  *****************************************************************************
  */
-static int m41t80_probe(struct i2c_client *client)
+static int m41t80_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int i, rc = 0;
 	struct rtc_device *rtc = NULL;
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 6b67b50..25de13b 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -494,7 +494,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;
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..f3aef9f 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -367,4 +367,23 @@ 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)  /* Needs to be large enough to hold device tree style names */
+#define I2C_MODULE_PREFIX "i2c:"
+#ifdef CONFIG_OF
+#define OF_I2C_PREFIX "OF,"
+#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..90a8d2c 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -539,6 +539,16 @@ static int do_virtio_entry(const char *filename, struct virtio_device_id *id,
 	return 1;
 }
 
+/* Looks like: i2c:s */
+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);
+
+	return 1;
+}
+
 /* Ignore any prefix, eg. v850 prepends _ */
 static inline int sym_is(const char *symbol, const char *name)
 {
@@ -669,6 +679,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);
 }
 

[PATCH 19 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      |   18 +++++++++++++
 drivers/rtc/rtc-ds1374.c      |    7 +++++
 drivers/rtc/rtc-m41t80.c      |   55 ++++++++++++++++++++++++++++-------------
 drivers/rtc/rtc-rs5c372.c     |   14 ++++++++++
 5 files changed, 81 insertions(+), 57 deletions(-)


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3ace747..94e5c73 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.type, compatible, sizeof(info.type));
 
 		info.addr = *addr;
 
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 9b0eab9..d4874ff 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -139,6 +139,17 @@ static inline const struct chip_desc *find_chip(const char *s)
 	return NULL;
 }
 
+static struct i2c_device_id ds1307_id[] = {
+	OF_I2C_ID("dallas,ds1307", ds_1307)
+	OF_I2C_ID("dallas,ds1337", ds_1337)
+	OF_I2C_ID("dallas,ds1338", ds_1338)
+	OF_I2C_ID("dallas,ds1339", ds_1339)
+	OF_I2C_ID("dallas,ds1340", ds_1340)
+	OF_I2C_ID("stm,m41t00", m41t00)
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ds1307_id);
+
 static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 {
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
@@ -334,7 +345,11 @@ static int __devinit ds1307_probe(struct i2c_client *client, const struct i2c_de
 	const struct chip_desc	*chip;
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 
-	chip = find_chip(client->name);
+	if (id)
+		chip = &chips[id->driver_data];
+	else
+		chip = find_chip(client->name);
+
 	if (!chip) {
 		dev_err(&client->dev, "unknown chip type '%s'\n",
 				client->name);
@@ -537,6 +552,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 dab6008..6dc05c4 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -41,6 +41,12 @@
 #define DS1374_REG_SR_AF	0x01 /* Alarm Flag */
 #define DS1374_REG_TCR		0x09 /* Trickle Charge */
 
+static struct i2c_device_id ds1374_id[] = {
+	OF_I2C_ID("dallas,ds1374", 0)
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ds1374_id);
+
 struct ds1374 {
 	struct i2c_client *client;
 	struct rtc_device *rtc;
@@ -429,6 +435,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-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 457f2ca..fc33f77 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -100,8 +100,21 @@ static const struct m41t80_chip_info m41t80_chip_info_tbl[] = {
 	},
 };
 
+static struct i2c_device_id m41t80_id[] = {
+	OF_I2C_ID("stm,m41t80", 0)
+	OF_I2C_ID("stm,m41t81", M41T80_FEATURE_HT)
+	OF_I2C_ID("stm,m41t81s", M41T80_FEATURE_HT | M41T80_FEATURE_BL)
+	OF_I2C_ID("stm,m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL)
+	OF_I2C_ID("stm,m41t83", M41T80_FEATURE_HT | M41T80_FEATURE_BL)
+	OF_I2C_ID("stm,m41t84", M41T80_FEATURE_HT | M41T80_FEATURE_BL)
+	OF_I2C_ID("stm,m41t85", M41T80_FEATURE_HT | M41T80_FEATURE_BL)
+	OF_I2C_ID("stm,m41t87", M41T80_FEATURE_HT | M41T80_FEATURE_BL)
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, m41t80_id);
+
 struct m41t80_data {
-	const struct m41t80_chip_info *chip;
+	u8 features;
 	struct rtc_device *rtc;
 };
 
@@ -208,7 +221,7 @@ static int m41t80_rtc_proc(struct device *dev, struct seq_file *seq)
 	struct m41t80_data *clientdata = i2c_get_clientdata(client);
 	u8 reg;
 
-	if (clientdata->chip->features & M41T80_FEATURE_BL) {
+	if (clientdata->features & M41T80_FEATURE_BL) {
 		reg = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
 		seq_printf(seq, "battery\t\t: %s\n",
 			   (reg & M41T80_FLAGS_BATT_LOW) ? "exhausted" : "ok");
@@ -761,7 +774,7 @@ static int m41t80_probe(struct i2c_client *client, const struct i2c_device_id *i
 	int i, rc = 0;
 	struct rtc_device *rtc = NULL;
 	struct rtc_time tm;
-	const struct m41t80_chip_info *chip;
+	u8 features;
 	struct m41t80_data *clientdata = NULL;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
@@ -773,17 +786,24 @@ static int m41t80_probe(struct i2c_client *client, const struct i2c_device_id *i
 	dev_info(&client->dev,
 		 "chip found, driver version " DRV_VERSION "\n");
 
-	chip = NULL;
-	for (i = 0; i < ARRAY_SIZE(m41t80_chip_info_tbl); i++) {
-		if (!strcmp(m41t80_chip_info_tbl[i].name, client->name)) {
-			chip = &m41t80_chip_info_tbl[i];
-			break;
+	if (id)
+		features = id->driver_data;
+	else {
+		const struct m41t80_chip_info *chip;
+
+		chip = NULL;
+		for (i = 0; i < ARRAY_SIZE(m41t80_chip_info_tbl); i++) {
+			if (!strcmp(m41t80_chip_info_tbl[i].name, client->name)) {
+				chip = &m41t80_chip_info_tbl[i];
+				break;
+			}
 		}
-	}
-	if (!chip) {
-		dev_err(&client->dev, "%s is not supported\n", client->name);
-		rc = -ENODEV;
-		goto exit;
+		if (!chip) {
+			dev_err(&client->dev, "%s is not supported\n", client->name);
+			rc = -ENODEV;
+			goto exit;
+		}
+		features = chip->features;
 	}
 
 	clientdata = kzalloc(sizeof(*clientdata), GFP_KERNEL);
@@ -801,7 +821,7 @@ static int m41t80_probe(struct i2c_client *client, const struct i2c_device_id *i
 	}
 
 	clientdata->rtc = rtc;
-	clientdata->chip = chip;
+	clientdata->features = features;
 	i2c_set_clientdata(client, clientdata);
 
 	/* Make sure HT (Halt Update) bit is cleared */
@@ -810,7 +830,7 @@ static int m41t80_probe(struct i2c_client *client, const struct i2c_device_id *i
 		goto ht_err;
 
 	if (rc & M41T80_ALHOUR_HT) {
-		if (chip->features & M41T80_FEATURE_HT) {
+		if (features & M41T80_FEATURE_HT) {
 			m41t80_get_datetime(client, &tm);
 			dev_info(&client->dev, "HT bit was set!\n");
 			dev_info(&client->dev,
@@ -842,7 +862,7 @@ static int m41t80_probe(struct i2c_client *client, const struct i2c_device_id *i
 		goto exit;
 
 #ifdef CONFIG_RTC_DRV_M41T80_WDT
-	if (chip->features & M41T80_FEATURE_HT) {
+	if (features & M41T80_FEATURE_HT) {
 		rc = misc_register(&wdt_dev);
 		if (rc)
 			goto exit;
@@ -878,7 +898,7 @@ static int m41t80_remove(struct i2c_client *client)
 	struct rtc_device *rtc = clientdata->rtc;
 
 #ifdef CONFIG_RTC_DRV_M41T80_WDT
-	if (clientdata->chip->features & M41T80_FEATURE_HT) {
+	if (clientdata->features & M41T80_FEATURE_HT) {
 		misc_deregister(&wdt_dev);
 		unregister_reboot_notifier(&wdt_notifier);
 	}
@@ -896,6 +916,7 @@ static struct i2c_driver m41t80_driver = {
 	},
 	.probe = m41t80_probe,
 	.remove = m41t80_remove,
+	.id_table = m41t80_id,
 };
 
 static int __init m41t80_rtc_init(void)
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 25de13b..e2022c0 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -69,6 +69,15 @@ enum rtc_type {
 	rtc_rv5c387a,
 };
 
+static struct i2c_device_id rs5c372_id[] = {
+	OF_I2C_ID("ricoh,rs5c372a", rtc_rs5c372a)
+	OF_I2C_ID("ricoh,rs5c372b", rtc_rs5c372b)
+	OF_I2C_ID("ricoh,rv5c386", rtc_rv5c386)
+	OF_I2C_ID("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);
@@ -522,7 +531,9 @@ static int rs5c372_probe(struct i2c_client *client, const struct i2c_device_id *
 	if (err < 0)
 		goto exit_kfree;
 
-	if (strcmp(client->name, "rs5c372a") == 0)
+	if (id)
+		rs5c372->type = id->driver_data;
+	else if (strcmp(client->name, "rs5c372a") == 0)
 		rs5c372->type = rtc_rs5c372a;
 	else if (strcmp(client->name, "rs5c372b") == 0)
 		rs5c372->type = rtc_rs5c372b;
@@ -651,6 +662,7 @@ static struct i2c_driver rs5c372_driver = {
 	},
 	.probe		= rs5c372_probe,
 	.remove		= rs5c372_remove,
+	.id_table	= rs5c372_id,
 };
 
 static __init int rs5c372_init(void)

[PATCH 19 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 19 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  |  183 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 180 insertions(+), 99 deletions(-)


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 94e5c73..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.type, compatible, sizeof(info.type));
-
-		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..91fa73c 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,6 +316,181 @@ static struct i2c_adapter mpc_ops = {
 	.retries = 1
 };
 
+struct i2c_driver_device {
+	char	*of_device;
+	char	*i2c_driver;
+	char	*i2c_type;
+};
+
+#ifdef CONFIG_PPC_MERGE
+
+static void of_register_i2c_devices(struct i2c_adapter *adap, struct device_node *adap_node)
+{
+	struct device_node *node = NULL;
+
+	while ((node = of_get_next_child(adap_node, node))) {
+		struct i2c_board_info info;
+		const u32 *addr;
+		const char *compatible;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+			printk(KERN_ERR "i2c-mpc.c: invalid entry, missing reg attribute\n");
+			continue;
+		}
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		compatible = of_get_property(node, "compatible", &len);
+		if (!compatible) {
+			printk(KERN_ERR "i2c-mpc.c: invalid entry, missing compatible attribute\n");
+			continue;
+		}
+		
+		/* need full alias i2c:NOF,vendor,device */
+		strcpy(info.type, I2C_OF_MODULE_PREFIX);
+		strncat(info.type, compatible, sizeof(info.type));
+		request_module(info.type);
+		
+		/* need module alias OF,vendor,device */
+		strcpy(info.type, OF_I2C_PREFIX);
+		strncat(info.type, compatible, sizeof(info.type));
+		
+		info.driver_name[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.type);
+			continue;
+		}
+	}
+}
+
+static int mpc_i2c_probe(struct of_device *op, const struct of_device_id *match)
+{
+	int result = 0;
+	struct mpc_i2c *i2c;
+
+	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	if (of_get_property(op->node, "dfsrr", NULL))
+		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
+
+	if (of_device_is_compatible(op->node, "mpc5200-i2c"))
+		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
+
+	init_waitqueue_head(&i2c->queue);
+
+	i2c->base = of_iomap(op->node, 0);
+	if (!i2c->base) {
+		printk(KERN_ERR "i2c-mpc - failed to map controller\n");
+		result = -ENOMEM;
+		goto fail_map;
+	}
+
+	i2c->irq = irq_of_parse_and_map(op->node, 0);
+	if (i2c->irq == NO_IRQ) {
+		result = -ENXIO;
+		goto fail_irq;
+	}
+
+	result = request_irq(i2c->irq, mpc_i2c_isr, IRQF_SHARED, "i2c-mpc", i2c);
+	if (result < 0) {
+		printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
+		goto fail_request;
+	}
+
+	mpc_i2c_setclock(i2c);
+
+	dev_set_drvdata(&op->dev, i2c);
+
+	i2c->adap = mpc_ops;
+	i2c_set_adapdata(&i2c->adap, i2c);
+	i2c->adap.dev.parent = &op->dev;
+
+	result = i2c_add_adapter(&i2c->adap);
+	if (result < 0) {
+		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
+		goto fail_add;
+	}
+
+	of_register_i2c_devices(&i2c->adap, op->node);
+
+	return result;
+
+fail_add:
+	free_irq(i2c->irq, i2c);
+fail_request:
+	irq_dispose_mapping(i2c->irq);
+fail_irq:
+	iounmap(i2c->base);
+fail_map:
+	kfree(i2c);
+	return result;
+};
+
+static int mpc_i2c_remove(struct of_device *op)
+{
+	struct mpc_i2c *i2c = dev_get_drvdata(&op->dev);
+
+	i2c_del_adapter(&i2c->adap);
+	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 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 mpc_i2c_init(void)
+{
+	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 mpc_i2c_exit(void)
+{
+	of_unregister_platform_driver(&mpc_i2c_driver);
+}
+
+module_init(mpc_i2c_init);
+module_exit(mpc_i2c_exit);
+
+#else
+
 static int fsl_i2c_probe(struct platform_device *pdev)
 {
 	int result = 0;
@@ -416,6 +591,8 @@ static void __exit fsl_i2c_exit(void)
 module_init(fsl_i2c_init);
 module_exit(fsl_i2c_exit);
 
+#endif
+
 MODULE_AUTHOR("Adrian Cox <adrian@humboldt.co.uk>");
 MODULE_DESCRIPTION
     ("I2C-Bus adapter for MPC107 bridge and MPC824x/85xx/52xx processors");

[PATCH 19 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 |  107 +++++++++++----------------------------------
 1 files changed, 27 insertions(+), 80 deletions(-)


diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 0242d80..e1ea2a0 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,52 @@ 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[] = {
+	OF_I2C_ID("philips,pcf8563", 0)
+	OF_I2C_ID("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 19 1/5] Implement module aliasing for i2c to translate from device tree names

[Jon Smirl]

Comment was wrong, I2C_OF_MODULE_PREFIX was needed. Add it back.

Implement module aliasing for i2c to translate from device tree names

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/hwmon/f75375s.c         |    4 ++--
 drivers/i2c/chips/ds1682.c      |    2 +-
 drivers/i2c/chips/menelaus.c    |    2 +-
 drivers/i2c/chips/tps65010.c    |    2 +-
 drivers/i2c/chips/tsl2550.c     |    2 +-
 drivers/i2c/i2c-core.c          |   24 +++++++++++++++++++++++-
 drivers/rtc/rtc-ds1307.c        |    2 +-
 drivers/rtc/rtc-ds1374.c        |    2 +-
 drivers/rtc/rtc-m41t80.c        |    2 +-
 drivers/rtc/rtc-rs5c372.c       |    2 +-
 include/linux/i2c.h             |    5 ++---
 include/linux/mod_devicetable.h |   20 ++++++++++++++++++++
 scripts/mod/file2alias.c        |   12 ++++++++++++
 13 files changed, 67 insertions(+), 14 deletions(-)


diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
index 6892f76..2247de6 100644
--- a/drivers/hwmon/f75375s.c
+++ b/drivers/hwmon/f75375s.c
@@ -117,7 +117,7 @@ struct f75375_data {
 static int f75375_attach_adapter(struct i2c_adapter *adapter);
 static int f75375_detect(struct i2c_adapter *adapter, int address, int kind);
 static int f75375_detach_client(struct i2c_client *client);
-static int f75375_probe(struct i2c_client *client);
+static int f75375_probe(struct i2c_client *client, const struct
i2c_device_id *id);
 static int f75375_remove(struct i2c_client *client);

 static struct i2c_driver f75375_legacy_driver = {
@@ -628,7 +628,7 @@ static void f75375_init(struct i2c_client *client,
struct f75375_data *data,

 }

-static int f75375_probe(struct i2c_client *client)
+static int f75375_probe(struct i2c_client *client, const struct
i2c_device_id *id)
 {
 	struct f75375_data *data = i2c_get_clientdata(client);
 	struct f75375s_platform_data *f75375s_pdata = client->dev.platform_data;
diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
index 9e94542..93c0441 100644
--- a/drivers/i2c/chips/ds1682.c
+++ b/drivers/i2c/chips/ds1682.c
@@ -200,7 +200,7 @@ static struct bin_attribute ds1682_eeprom_attr = {
 /*
  * Called when a ds1682 device is matched with this driver
  */
-static int ds1682_probe(struct i2c_client *client)
+static int ds1682_probe(struct i2c_client *client, const struct
i2c_device_id *id)
 {
 	int rc;

diff --git a/drivers/i2c/chips/menelaus.c b/drivers/i2c/chips/menelaus.c
index 2dea012..89ef9b6 100644
--- a/drivers/i2c/chips/menelaus.c
+++ b/drivers/i2c/chips/menelaus.c
@@ -1149,7 +1149,7 @@ static inline void menelaus_rtc_init(struct
menelaus_chip *m)

 static struct i2c_driver menelaus_i2c_driver;

-static int menelaus_probe(struct i2c_client *client)
+static int menelaus_probe(struct i2c_client *client, const struct
i2c_device_id *id)
 {
 	struct menelaus_chip	*menelaus;
 	int			rev = 0, val;
diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
index e320994..6b13642 100644
--- a/drivers/i2c/chips/tps65010.c
+++ b/drivers/i2c/chips/tps65010.c
@@ -465,7 +465,7 @@ static int __exit tps65010_remove(struct i2c_client *client)
 	return 0;
 }

-static int tps65010_probe(struct i2c_client *client)
+static int tps65010_probe(struct i2c_client *client, const struct
i2c_device_id *id)
 {
 	struct tps65010		*tps;
 	int			status;
diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/i2c/chips/tsl2550.c
index 3de4b19..27c553d 100644
--- a/drivers/i2c/chips/tsl2550.c
+++ b/drivers/i2c/chips/tsl2550.c
@@ -364,7 +364,7 @@ static int tsl2550_init_client(struct i2c_client *client)
  */

 static struct i2c_driver tsl2550_driver;
-static int __devinit tsl2550_probe(struct i2c_client *client)
+static int __devinit tsl2550_probe(struct i2c_client *client, const
struct i2c_device_id *id)
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct tsl2550_data *data;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b5e13e4..5f62230 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -47,6 +47,19 @@ static DEFINE_IDR(i2c_adapter_idr);

 /* ------------------------------------------------------------------------- */

+static const struct i2c_device_id *i2c_match_id(
+		const struct i2c_device_id *id, struct i2c_client *client)
+{
+	/* only powerpc drivers implement the id_table,
+	 * it is empty on other platforms */
+	while (id->name[0]) {
+		if (strcmp(client->name, id->name) == 0)
+			return id;
+		id++;
+	}
+	return NULL;
+}
+
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
@@ -58,6 +71,10 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
 	if (!is_newstyle_driver(driver))
 		return 0;

+	/* match on an id table if there is one */
+	if (driver->id_table)
+		return i2c_match_id(driver->id_table, client) != NULL;
+
 	/* new style drivers use the same kind of driver matching policy
 	 * as platform devices or SPI:  compare device and driver IDs.
 	 */
@@ -89,12 +106,17 @@ 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 = NULL;

 	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+
+	if (driver->id_table)
+		id = i2c_match_id(driver->id_table, client);
+	
+	return driver->probe(client, id);
 }

 static int i2c_device_remove(struct device *dev)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index bc1c7fe..9b0eab9 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -326,7 +326,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;
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 45bda18..dab6008 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -355,7 +355,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;
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 1cb33ca..457f2ca 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -756,7 +756,7 @@ static struct notifier_block wdt_notifier = {
  *
  *****************************************************************************
  */
-static int m41t80_probe(struct i2c_client *client)
+static int m41t80_probe(struct i2c_client *client, const struct
i2c_device_id *id)
 {
 	int i, rc = 0;
 	struct rtc_device *rtc = NULL;
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 6b67b50..25de13b 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -494,7 +494,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;
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..612d416 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)  /* Needs to be large enough to hold
device tree style names */
+#define I2C_MODULE_PREFIX "i2c:"
+#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..ac05b04 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -539,6 +539,14 @@ static int do_virtio_entry(const char *filename,
struct virtio_device_id *id,
 	return 1;
 }

+/* Looks like: i2c:s */
+static int do_i2c_entry(const char *filename, struct i2c_device_id *id,
+			   char *alias)
+{
+	sprintf(alias, I2C_MODULE_PREFIX "%s", id->name);
+	return 1;
+}
+
 /* Ignore any prefix, eg. v850 prepends _ */
 static inline int sym_is(const char *symbol, const char *name)
 {
@@ -669,6 +677,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);
 }

-- 
Jon Smirl
jonsmirl@gmail.com

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

Hi Jon,

On Fri, 11 Jan 2008 22:00:42 -0500 "Jon Smirl" <jonsmirl@gmail.com> wrote:
>
> +++ b/drivers/hwmon/f75375s.c
> @@ -117,7 +117,7 @@ struct f75375_data {
>  static int f75375_attach_adapter(struct i2c_adapter *adapter);
>  static int f75375_detect(struct i2c_adapter *adapter, int address, int kind);
>  static int f75375_detach_client(struct i2c_client *client);
> -static int f75375_probe(struct i2c_client *client);
> +static int f75375_probe(struct i2c_client *client, const struct
> i2c_device_id *id);

Looks like your mail client has wrapped this.  Also in various later
spots.

> +++ b/drivers/i2c/i2c-core.c
> @@ -47,6 +47,19 @@ static DEFINE_IDR(i2c_adapter_idr);
> 
>  /* ------------------------------------------------------------------------- */
> 
> +static const struct i2c_device_id *i2c_match_id(
> +		const struct i2c_device_id *id, struct i2c_client *client)

Any reason that the "client" argument is not const as well?

> +++ b/include/linux/i2c.h
> @@ -141,11 +141,10 @@ struct i2c_driver {
> 
>  	struct device_driver driver;
>  	struct list_head list;
> +	struct i2c_device_id *id_table;

Any reason that this is not const?  Making it const would allow divers to
make their tables const.

These are just small things (apart from the wrapping) that can be fixed
up with followup patches.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

Hi Jon,

On Fri, 11 Jan 2008 21:47:46 -0500 Jon Smirl <jonsmirl@gmail.com> wrote:
>
> +++ b/drivers/i2c/busses/i2c-mpc.c

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

We have for_each_child_of_node(adap_node, node) now for this (and it
means you don't need to initialize the "node" above).

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

You may need to clean up from the irq_of_pase_and_map().

> +		}
> +		
> +		/* need full alias i2c:NOF,vendor,device */
> +		strcpy(info.type, I2C_OF_MODULE_PREFIX);
> +		strncat(info.type, compatible, sizeof(info.type));
> +		request_module(info.type);
> +		
> +		/* need module alias OF,vendor,device */
> +		strcpy(info.type, OF_I2C_PREFIX);
> +		strncat(info.type, compatible, sizeof(info.type));
> +		
> +		info.driver_name[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.type);
> +			continue;

And again.

> +		}
> +	}
> +}
> +
> +static int mpc_i2c_probe(struct of_device *op, const struct of_device_id *match)
> +{

> +	dev_set_drvdata(&op->dev, i2c);
> +
> +	i2c->adap = mpc_ops;
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +	i2c->adap.dev.parent = &op->dev;
> +
> +	result = i2c_add_adapter(&i2c->adap);
> +	if (result < 0) {
> +		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
> +		goto fail_add;
> +	}
> +
> +	of_register_i2c_devices(&i2c->adap, op->node);
> +
> +	return result;
> +
> +fail_add:

	dev_set_dvdata(&op->dev, NULL);

> +	free_irq(i2c->irq, i2c);
> +fail_request:
> +	irq_dispose_mapping(i2c->irq);
> +fail_irq:
> +	iounmap(i2c->base);
> +fail_map:
> +	kfree(i2c);
> +	return result;
> +};

> +static struct of_device_id mpc_i2c_of_match[] = {

const, please.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [i2c] [PATCH 19 3/5] Clean up error returns

[Jean Delvare]

Hi Jon,

On Fri, 11 Jan 2008 21:47:43 -0500, Jon Smirl wrote:
> Return errors that were being ignored in the mpc-i2c driver

This wording is a bit excessive. The errors were not being ignored,
only the error code was replaced with a less informative -1. Still,
that's a good fix, although totally unrelated with the patch set it was
hiding into. The only question I have is:

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

Is this last chunk a cleanup or a bugfix? It seems that NO_IRQ can have
value 0 or -1 depending on the architecture, so your change is real on
some architectures.

I have to admit that I'm a bit confused by the way IRQs are handled by
this driver. On the one hand, there is code to handle polled-mode:

static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
{
	(...)
	if (i2c->irq == 0)
	{
		(...)
	} else {
		/* Interrupt mode */

But on the other hand the initialization code bails out if the platform
device doesn't provide an IRQ:

static int fsl_i2c_probe(struct platform_device *pdev)
{
	(...)
	i2c->irq = platform_get_irq(pdev, 0);
	if (i2c->irq < 0) {
		result = -ENXIO;
		goto fail_get_irq;
	}

So it seems to me like the polling mode code is never actually used?
Unless some platforms include an "empty" IRQ in their device
definition. Which indeed seems to be the case... but then they set the
IRQ to 0, NOT to NO_IRQ, so I'm wondering if the change you propose is
really correct.

Either way, there are more places in the driver where the IRQ is
compared to 0, so if your change is correct, it should be applied
consistently. Thus I will revert this part for the time being, if any
change is really needed with regards to interrupts in this driver,
please send a separate patch. But please double-check first, as I said
above it's trickier than it looks.

-- 
Jean Delvare

Re: [i2c] [PATCH 19 3/5] Clean up error returns

[Jon Smirl]

On 1/20/08, Jean Delvare <khali@linux-fr.org> wrote:
> > @@ -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);
> >
> >
>
> Is this last chunk a cleanup or a bugfix? It seems that NO_IRQ can have
> value 0 or -1 depending on the architecture, so your change is real on
> some architectures.

I was confused by this too. Search the ppc list archives and there is
a thread about it where BenH tries to explain the correct way of
fixing it to me.

This is part of the ppc to powerpc conversion that has not been
completely cleaned up in this driver. NO_IRQ = -1 is ppc and NO_IRQ =
0 is powerpc. Since this driver didn't originally use the NO_IRQ
define it didn't get automatically converted. We need to identify the
right places where NO_IRQ should have been used.

>
> I have to admit that I'm a bit confused by the way IRQs are handled by
> this driver. On the one hand, there is code to handle polled-mode:
>
> static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> {
>         (...)
>         if (i2c->irq == 0)

I missed this one, it should have been NO_IRQ.

>         {
>                 (...)
>         } else {
>                 /* Interrupt mode */
>
> But on the other hand the initialization code bails out if the platform
> device doesn't provide an IRQ:

>
> static int fsl_i2c_probe(struct platform_device *pdev)
> {
>         (...)
>         i2c->irq = platform_get_irq(pdev, 0);
>         if (i2c->irq < 0) {
>                 result = -ENXIO;
>                 goto fail_get_irq;

It is only bailing out on an error, not the NO_IRQ case. But maybe the
comparison needs to be with NO_IRQ instead of zero.



>         }
>
> So it seems to me like the polling mode code is never actually used?
> Unless some platforms include an "empty" IRQ in their device
> definition. Which indeed seems to be the case... but then they set the
> IRQ to 0, NOT to NO_IRQ, so I'm wondering if the change you propose is
> really correct.

All of this is very confusing to me, There are physical IRQs and
virtual IRQs. Apparently zero is a legal physical IRQ but it is not a
legal virtual IRQ. We only get virtual IRQs in this code. We need to
get BenH to give us the right answer on these two cases.


>
> Either way, there are more places in the driver where the IRQ is
> compared to 0, so if your change is correct, it should be applied
> consistently. Thus I will revert this part for the time being, if any
> change is really needed with regards to interrupts in this driver,
> please send a separate patch. But please double-check first, as I said
> above it's trickier than it looks.
>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH 19 3/5] Clean up error returns

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

Here' s a version with the compares to zero switched to NO_IRQ. If I
understand how NO_IRQ works it is the correct change. My understanding
is that under ppc IRQ zero was legal and NO_IRQ was -1. But then the
whole kernel switched to NO_IRQ = zero. Powerpc updated to NO_IRQ=0
and used virtual IRQs to move a physical IRQ 0 to another IRQ number.
ppc was not changed. This driver does not appear to have been updated
to track this global change since it didn't initially use the NO_IRQ
define everywhere.

-- 
Jon Smirl
jonsmirl@gmail.com

[-- Attachment #2: jds-fix-err-returns --]
[-- Type: application/octet-stream, Size: 3562 bytes --]

Clean up error returns

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 |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)


diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d8de4ac..a774cdf 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
 	u32 x;
 	int result = 0;
 
-	if (i2c->irq == 0)
+	if (i2c->irq == NO_IRQ)
 	{
 		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
 			schedule();
@@ -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,12 +325,12 @@ 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) {
+	if (i2c->irq < NO_IRQ) {
 		result = -ENXIO;
 		goto fail_get_irq;
 	}
@@ -341,7 +345,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		goto fail_map;
 	}
 
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
 					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
 			printk(KERN_ERR
@@ -364,7 +368,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 	return result;
 
       fail_add:
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
       fail_irq:
 	iounmap(i2c->base);
@@ -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);

Re: [i2c] [PATCH 19 3/5] Clean up error returns

[Jean Delvare]

Hi Jon,

On Sun, 20 Jan 2008 10:39:43 -0500, Jon Smirl wrote:
> Here' s a version with the compares to zero switched to NO_IRQ. If I
> understand how NO_IRQ works it is the correct change. My understanding
> is that under ppc IRQ zero was legal and NO_IRQ was -1. But then the
> whole kernel switched to NO_IRQ = zero. Powerpc updated to NO_IRQ=0
> and used virtual IRQs to move a physical IRQ 0 to another IRQ number.
> ppc was not changed. This driver does not appear to have been updated
> to track this global change since it didn't initially use the NO_IRQ
> define everywhere.

As I have already applied the part of this patch that preserves error
values in error paths, can you please send an incremental patch that
only fixes the IRQ issues? These are separate issues so it's better to
have separate patches anyway.

Thanks,
-- 
Jean Delvare

Re: [i2c] [PATCH 19 3/5] Clean up error returns

[Benjamin Herrenschmidt]

On Mon, 2008-01-21 at 17:10 +0100, Jean Delvare wrote:
> Hi Jon,
> 
> On Sun, 20 Jan 2008 10:39:43 -0500, Jon Smirl wrote:
> > Here' s a version with the compares to zero switched to NO_IRQ. If I
> > understand how NO_IRQ works it is the correct change. My understanding
> > is that under ppc IRQ zero was legal and NO_IRQ was -1. But then the
> > whole kernel switched to NO_IRQ = zero. Powerpc updated to NO_IRQ=0
> > and used virtual IRQs to move a physical IRQ 0 to another IRQ number.
> > ppc was not changed. This driver does not appear to have been updated
> > to track this global change since it didn't initially use the NO_IRQ
> > define everywhere.
> 
> As I have already applied the part of this patch that preserves error
> values in error paths, can you please send an incremental patch that
> only fixes the IRQ issues? These are separate issues so it's better to
> have separate patches anyway.

To be clear, nowadays, checking against 0 is correct unless you intend
the driver to work with arch/ppc (which we'll deprecate soon).

Ben.

Re: [i2c] [PATCH 19 3/5] Clean up error returns

[Benjamin Herrenschmidt]

On Sun, 2008-01-20 at 10:18 -0500, Jon Smirl wrote:
> > So it seems to me like the polling mode code is never actually used?
> > Unless some platforms include an "empty" IRQ in their device
> > definition. Which indeed seems to be the case... but then they set
> the
> > IRQ to 0, NOT to NO_IRQ, so I'm wondering if the change you propose
> is
> > really correct.
> 
> All of this is very confusing to me, There are physical IRQs and
> virtual IRQs. Apparently zero is a legal physical IRQ but it is not a
> legal virtual IRQ. We only get virtual IRQs in this code. We need to
> get BenH to give us the right answer on these two cases.

Testing against NO_IRQ for a linux IRQ number should always be correct.

Physical IRQ numbers are remapped and shouldn't be visible to drivers,
NO_IRQ is a value that never exist for a valid logical interrupt number,
that is 0 for arch/powerpc and -1 for arch/ppc.

Ben.

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

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]

Hi Jon,

On Saturday 12 January 2008 03:47, Jon Smirl wrote:
> 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.

The patch breaks the pfc8563 driver.

The old style driver allocated memory for a pcf8563 structure that
encapsulated an i2c_client instance. Various functions use container_of
on an i2c_client instance to get a pointer to the pcf8563 structure.

The new style driver gets rid of the pcf8563 structure allocation, as the
i2c_client structure is now allocated by I2C code.

Here is an incremental patch that fixes the issue.

From 1eac5a8e10e085c3a77c6ba7ed9dac9a39024915 Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <laurentp@cse-semaphore.com>
Date: Thu, 10 Apr 2008 11:12:25 +0200
Subject: [PATCH] Fix pcf8563 breakage introduced by the conversion from old style to new style

The old style I2C driver used to allocate a pcf8563 instance that encapsulated
an i2c_client instance. The i2c_client instance is now allocated by I2C core.

This patch fixes the driver by storing the pcf8563 instance in the I2C client
data field.

Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
---
 drivers/rtc/rtc-pcf8563.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index e1ea2a0..7aab24e 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -50,7 +50,7 @@
 #define PCF8563_MO_C		0x80 /* century */
 
 struct pcf8563 {
-	struct i2c_client client;
+	struct rtc_device *rtc;
 	/*
 	 * The meaning of MO_C bit varies by the chip type.
 	 * From PCF8563 datasheet: this bit is toggled when the years
@@ -74,7 +74,7 @@ struct pcf8563 {
  */
 static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
-	struct pcf8563 *pcf8563 = container_of(client, struct pcf8563, client);
+	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
 	unsigned char buf[13] = { PCF8563_REG_ST1 };
 
 	struct i2c_msg msgs[] = {
@@ -131,7 +131,7 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 
 static int pcf8563_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
-	struct pcf8563 *pcf8563 = container_of(client, struct pcf8563, client);
+	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
 	int i, err;
 	unsigned char buf[9];
 
@@ -252,10 +252,10 @@ static const struct rtc_class_ops pcf8563_rtc_ops = {
 
 static int pcf8563_remove(struct i2c_client *client)
 {
-	struct rtc_device *rtc = i2c_get_clientdata(client);
+	struct pcf8563 *pcf8563 = i2c_get_clientdata(client);
 
-	if (rtc)
-		rtc_device_unregister(rtc);
+	if (pcf8563->rtc)
+		rtc_device_unregister(pcf8563->rtc);
 
 	return 0;
 }
@@ -274,26 +274,33 @@ static struct i2c_driver pcf8563_driver = {
 		.name	= "rtc-pcf8563",
 	},
 	.id		= I2C_DRIVERID_PCF8563,
-	.probe = &pcf8563_probe,
-	.remove = &pcf8563_remove,
+	.probe		= &pcf8563_probe,
+	.remove		= &pcf8563_remove,
 	.id_table	= pcf8563_id,
 };
 
 static int pcf8563_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
-	int result;
+	struct pcf8563 *pcf8563;
 	struct rtc_device *rtc;
+	int result;
 
 	result = pcf8563_validate_client(client);
 	if (result)
 		return result;
 
+	if ((pcf8563 = kzalloc(sizeof(*pcf8563), GFP_KERNEL)) == NULL)
+		return -ENOMEM;
+
 	rtc = rtc_device_register(pcf8563_driver.driver.name, &client->dev,
 				&pcf8563_rtc_ops, THIS_MODULE);
-	if (IS_ERR(rtc))
+	if (IS_ERR(rtc)) {
+		kfree(pcf8563);
 		return PTR_ERR(rtc);
+	}
 
-	i2c_set_clientdata(client, rtc);
+	pcf8563->rtc = rtc;
+	i2c_set_clientdata(client, pcf8563);
 
 	return 0;
 }
-- 
1.5.0

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]