[RFC] Rework of i2c-mpc.c - Freescale i2c driver

[RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

This is my first pass at reworking the Freescale i2c driver. It
switches the driver from being a platform driver to an open firmware
one. I've checked it out on my hardware and it is working.

You can specific i2c devices on a bus by adding child nodes for them
in the device tree. It does not know how to autoload the i2c chip
driver modules.

i2c@3d40 {
	device_type = "i2c";
	compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
	cell-index = <1>;
	reg = <3d40 40>;
	interrupts = <2 10 0>;
	interrupt-parent = <&mpc5200_pic>;
	fsl5200-clocking;

	rtc@32 {
		device_type = "rtc";
		compatible = "epson,pcf8564";
		reg = <51>;
	};
};

One side effect is that legacy style i2c drivers won't work anymore.
The rtc driver in the patch has been converted from legacy i2c style
to new i2c style. It took about ten minutes to change it and it
eliminates a lot of code. The patch uses the broad_info support in the
i2c core which does not support legacy style drivers.

The driver contains a table for mapping device tree style names to
linux kernel ones.

+static struct i2c_driver_device i2c_devices[] = {
+	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
+	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
+	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
+	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
+	{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
+};

I'd like to get rid of this table.  There are two obvious solutions,
use names in the device tree that match up with the linux device
driver names. Or push these strings into the chip drivers and modify
i2c-core to also match on the device tree style names. Without one of
these changes the table is going to need a mapping for every i2c
device on the market.

If someone can supply a fix for this I will add it:

On 10/26/07, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> On Fri, 2007-10-26 at 11:53 +0200, Jean Delvare wrote:
> > > 2) mpc_read(), according to the comment below it sends a STOP condition here but
> > >    this function does not known if this is the last read or not. mpc_xfer is
> > >    the one that knows when the transaction is over and should send the stop,
> > >    which it already does.
> > >
> > >  /* Generate stop on last byte */
> > >   if (i == length - 1)
> > >        writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
> >
> > Probably correct, although I am not familiar with this specific
> > hardware. I guess that the same is true of mpc_write as well, which is
> > even worse because write + read combined transactions are very common
> > (while read + write are not.)
>
> Don't think write is a problem, only read. I would have to look at the
> HW spec to make sure though.

Convert i2c to of_platform_driver from platform_driver

From: Jon Smirl <jonsmirl@gmail.com>


---

 arch/powerpc/sysdev/fsl_soc.c |  116 -------------------------
 drivers/i2c/busses/i2c-mpc.c  |  193 +++++++++++++++++++++++++++++++----------
 drivers/rtc/rtc-pcf8563.c     |  103 ++++------------------
 3 files changed, 166 insertions(+), 246 deletions(-)


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..6f80216 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -306,122 +306,6 @@ err:

 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",},
-};
-
-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;
-		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
-		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
-		return 0;
-	}
-	return -ENODEV;
-}
-
-static void __init of_register_i2c_devices(struct device_node
*adap_node, int bus_num)
-{
-	struct device_node *node = NULL;
-
-	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_ioc.c: invalid i2c device entry\n");
-			continue;
-		}
-
-		info.irq = irq_of_parse_and_map(node, 0);
-		if (info.irq == NO_IRQ)
-			info.irq = -1;
-
-		if (of_find_i2c_driver(node, &info) < 0)
-			continue;
-
-		info.platform_data = NULL;
-		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..5ceb936 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 <asm/of_platform.h>

 #include <asm/io.h>
 #include <linux/fsl_devices.h>
@@ -25,6 +25,8 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>

+#define DRV_NAME "mpc-i2c"
+
 #define MPC_I2C_ADDR  0x00
 #define MPC_I2C_FDR 	0x04
 #define MPC_I2C_CR	0x08
@@ -180,7 +182,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 +194,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 +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,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 +236,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,74 +314,151 @@ 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 struct i2c_driver_device i2c_devices[] = {
+	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
+	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
+	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
+	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
+	{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
+};
+
+static int 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;
+		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
+		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		printk("i2c driver is %s\n", info->driver_name);
+		return 0;
+	}
+	return -ENODEV;
+}
+
+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;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+			printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
+			continue;
+		}
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		if (of_find_i2c_driver(node, &info) < 0)
+			continue;
+
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		i2c_new_device(adap, &info);
+	}
+}
+
+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);
+	struct resource mem;
+	const u32 *index;
+	const unsigned char *flags = NULL;

-	pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
+	memset(&mem, 0, sizeof(mem));
+	result = of_address_to_resource(op->node, 0, &mem);
+	if (result)
+		return result;

+	index = of_get_property(op->node, "cell-index", NULL);
+    if (!index || *index > 5) {
+            printk(KERN_ERR "mpc_i2c_probe: Device node %s has invalid "
+                            "cell-index property\n", op->node->full_name);
+            return -EINVAL;
+    }
+
 	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);
+	flags = of_get_property(op->node, "dfsrr", NULL);
+	if (flags)
+		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;

-	i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
+	flags = of_get_property(op->node, "fsl5200-clocking", NULL);
+	if (flags)
+		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
+
+	init_waitqueue_head(&i2c->queue);

+	i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
 	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 < 0) {
+		result = -ENXIO;
+		goto fail_irq;
+	}
+	
 	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");
+					  	IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
+			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
 			goto fail_irq;
 		}

 	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->adap.nr = *index;
 	i2c_set_adapdata(&i2c->adap, i2c);
-	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.dev.parent = &op->dev;
 	if ((result = i2c_add_numbered_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:
+fail_add:
 	if (i2c->irq != 0)
 		free_irq(i2c->irq, i2c);
-      fail_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)
 		free_irq(i2c->irq, i2c);
@@ -389,28 +468,46 @@ static int fsl_i2c_remove(struct platform_device *pdev)
 	return 0;
 };

+static struct of_device_id mpc_i2c_of_match[] = {
+	{
+		.type		= "i2c",
+		.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 = {
+	.owner		= THIS_MODULE,
+	.name		= DRV_NAME,
+	.match_table	= mpc_i2c_of_match,
+	.probe		= mpc_i2c_probe,
+	.remove		= __devexit_p(mpc_i2c_remove),
+	.driver		= {
+		.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
diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 0242d80..b778d35 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,44 @@ 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 int pcf8563_probe(struct i2c_client *client);
+
 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,
 };

-static int pcf8563_probe(struct i2c_adapter *adapter, int address, int kind)
+static int pcf8563_probe(struct i2c_client *client)
 {
-	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)
		
-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Matt Sealey]

Jon Smirl wrote:

> i2c@3d40 {
> 	device_type = "i2c";
> 	compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
> 	cell-index = <1>;
> 	reg = <3d40 40>;
> 	interrupts = <2 10 0>;
> 	interrupt-parent = <&mpc5200_pic>;
> 	fsl5200-clocking;
> 
> 	rtc@32 {
> 		device_type = "rtc";
> 		compatible = "epson,pcf8564";
> 		reg = <51>;
> 	};
> };

My only comment would be that the fsl5200-clocking property is
totally redundant.

Drivers can look at the compatible property (mpc5200b-i2c and
mpc5200-i2c) to match up what special needs the driver may need.
Even if it was just fsl-i2c, it could/should be implicit that
this device is the onboard i2c and the parent node is ostensibly
going to be marked as an MPC52xx SoC.. or it can look for the
mpc5200-cdm node. There is no reason to invent a property just
so you can do a property search when it replaces code of the
same size to do a node or compatible search..

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jon Smirl wrote:
> This is my first pass at reworking the Freescale i2c driver. It
> switches the driver from being a platform driver to an open firmware
> one. I've checked it out on my hardware and it is working.

We may want to hold off on this until arch/ppc goes away (or at least 
all users of this driver in arch/ppc).

> You can specific i2c devices on a bus by adding child nodes for them
> in the device tree. It does not know how to autoload the i2c chip
> driver modules.
> 
> i2c@3d40 {
> 	device_type = "i2c";
> 	compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";

dtc supports the "mpc5200b-i2c", "mpc5200-i2c", "fsl-i2c" syntax.

> 	cell-index = <1>;

What is cell-index for?

> 	fsl5200-clocking;

As Matt pointed out, this is redundant.

> 	rtc@32 {
> 		device_type = "rtc";

This isn't really necessary.

> One side effect is that legacy style i2c drivers won't work anymore.

If you mean legacy-style client drivers, why not?

> The driver contains a table for mapping device tree style names to
> linux kernel ones.

Can we please put this in common code instead?

> +static struct i2c_driver_device i2c_devices[] = {
> +	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
> +	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
> +	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
> +	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
> +	{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
> +};

At the very least, include the entries that are already being used with 
this driver in fsl_soc.c.

> I'd like to get rid of this table.  There are two obvious solutions,
> use names in the device tree that match up with the linux device
> driver names.

This was proposed and rejected a while ago.  For one thing, some drivers 
handle multiple chips, and we shouldn't base device tree bindings on 
what groupings Linux happens to make in what driver supports what.

> Or push these strings into the chip drivers and modify
> i2c-core to also match on the device tree style names.

That would be ideal; it just hasn't been done yet.

A middle ground for now would be to keep one table in drivers/of or 
something.

> Without one of
> these changes the table is going to need a mapping for every i2c
> device on the market.

Nah, only one for every i2c device Linux supports. :-)

> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 1cf29c9..6f80216 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -306,122 +306,6 @@ err:
> 
>  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",},
> -};

This is obviously not based on head-of-tree -- there are several more 
entries in this table.

> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index d8de4ac..5ceb936 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 <asm/of_platform.h>

Should be linux/of_platform.h

> @@ -180,7 +182,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 +194,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;

This is a separate change from the OF-ization -- at least note it in the 
changelog.

> +	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> +		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
> +			continue;
> +		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
> +		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> +		printk("i2c driver is %s\n", info->driver_name);

Should be KERN_DEBUG, if it stays at all.

> +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;
> +		int len;
> +
> +		addr = of_get_property(node, "reg", &len);
> +		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
> +			printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
> +			continue;
> +		}
> +
> +		info.irq = irq_of_parse_and_map(node, 0);
> +		if (info.irq == NO_IRQ)
> +			info.irq = -1;
> +
> +		if (of_find_i2c_driver(node, &info) < 0)
> +			continue;
> +
> +		info.platform_data = NULL;
> +		info.addr = *addr;
> +
> +		i2c_new_device(adap, &info);
> +	}
> +}

Most of this code should be made generic and placed in drivers/of.

> +	index = of_get_property(op->node, "cell-index", NULL);
> +    if (!index || *index > 5) {
> +            printk(KERN_ERR "mpc_i2c_probe: Device node %s has invalid "
> +                            "cell-index property\n", op->node->full_name);
> +            return -EINVAL;
> +    }
> +

We might as well just use i2c_new_device() instead of messing around 
with bus numbers.  Note that this is technically no longer platform 
code, so it's harder to justify claiming the static numberspace.

> +	i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
>  	if (!i2c->base) {
>  		printk(KERN_ERR "i2c-mpc - failed to map controller\n");

Use of_iomap().

>  	if (i2c->irq != 0)

if (i2c->irq != NO_IRQ)

> +static struct of_device_id mpc_i2c_of_match[] = {
> +	{
> +		.type		= "i2c",
> +		.compatible	= "fsl-i2c",
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);

Let's take this opportunity to stop matching on device_type here 
(including updating booting-without-of.txt).

> +static struct of_platform_driver mpc_i2c_driver = {
> +	.owner		= THIS_MODULE,
> +	.name		= DRV_NAME,
> +	.match_table	= mpc_i2c_of_match,
> +	.probe		= mpc_i2c_probe,
> +	.remove		= __devexit_p(mpc_i2c_remove),
> +	.driver		= {
> +		.name	= DRV_NAME,
>  	},
>  };

Do we still need .name if we have .driver.name?

> diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> index 0242d80..b778d35 100644
> --- a/drivers/rtc/rtc-pcf8563.c
> +++ b/drivers/rtc/rtc-pcf8563.c

This should be a separate patch.

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Matt Sealey <matt@genesi-usa.com> wrote:
> Jon Smirl wrote:
>
> > i2c@3d40 {
> >       device_type = "i2c";
> >       compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
> >       cell-index = <1>;
> >       reg = <3d40 40>;
> >       interrupts = <2 10 0>;
> >       interrupt-parent = <&mpc5200_pic>;
> >       fsl5200-clocking;
> >
> >       rtc@32 {
> >               device_type = "rtc";
> >               compatible = "epson,pcf8564";
> >               reg = <51>;
> >       };
> > };
>
> My only comment would be that the fsl5200-clocking property is
> totally redundant.
>
> Drivers can look at the compatible property (mpc5200b-i2c and
> mpc5200-i2c) to match up what special needs the driver may need.
> Even if it was just fsl-i2c, it could/should be implicit that
> this device is the onboard i2c and the parent node is ostensibly
> going to be marked as an MPC52xx SoC.. or it can look for the
> mpc5200-cdm node. There is no reason to invent a property just
> so you can do a property search when it replaces code of the
> same size to do a node or compatible search..

fsl5200-clocking is used to set FSL_I2C_DEV_CLOCK_5200

	} else if (i2c->flags & FSL_I2C_DEV_CLOCK_5200)
		writeb(0x3f, i2c->base + MPC_I2C_FDR);
	else
		writel(0x1031, i2c->base + MPC_I2C_FDR);

I can change it to remove fsl5200-clocking if someone can tell me if
this is only needed on the mpc5200b. This driver also supports the
MPC107/Tsi107/MPC8240/MPC8245 and           MPC85xx/MPC8641. Do any of
these other chips need fsl5200-clocking?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jon Smirl wrote:
> I can change it to remove fsl5200-clocking if someone can tell me if
> this is only needed on the mpc5200b.

Well, it appears to be needed on the mpc5200 (no 'b')...

> This driver also supports the
> MPC107/Tsi107/MPC8240/MPC8245 and           MPC85xx/MPC8641.

You forgot mpc83xx. :-)

> Do any of
> these other chips need fsl5200-clocking?

None of them have it set in arch/powerpc/boot/dts.

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Grant Likely]

On 11/5/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> This is my first pass at reworking the Freescale i2c driver. It
> switches the driver from being a platform driver to an open firmware
> one. I've checked it out on my hardware and it is working.

Isn't this device core also used in the fsl coldfire socs?

If so, it needs to have both platform bus and of_platform bus
bindings.  This is easy to do as long as you keep device probing and
initialization in separate routines.  See drivers/serial/uartlite.c
for an example.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Jon Smirl wrote:
> > I can change it to remove fsl5200-clocking if someone can tell me if
> > this is only needed on the mpc5200b.
>
> Well, it appears to be needed on the mpc5200 (no 'b')...


What is the recommend way to check for a mpc5200 or mpc5200b?


>
> > This driver also supports the
> > MPC107/Tsi107/MPC8240/MPC8245 and           MPC85xx/MPC8641.
>
> You forgot mpc83xx. :-)
>
> > Do any of
> > these other chips need fsl5200-clocking?
>
> None of them have it set in arch/powerpc/boot/dts.
>
> -Scott
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jon Smirl wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
>> Jon Smirl wrote:
>>> I can change it to remove fsl5200-clocking if someone can tell me if
>>> this is only needed on the mpc5200b.
>> Well, it appears to be needed on the mpc5200 (no 'b')...
> 
> What is the recommend way to check for a mpc5200 or mpc5200b?

Just check for mpc5200-i2c -- it's listed in the mpc5200b dts as well.

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Grant Likely]

On 11/5/07, Matt Sealey <matt@genesi-usa.com> wrote:
> Jon Smirl wrote:
>
> > i2c@3d40 {
> >       device_type = "i2c";
> >       compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
> >       cell-index = <1>;
> >       reg = <3d40 40>;
> >       interrupts = <2 10 0>;
> >       interrupt-parent = <&mpc5200_pic>;
> >       fsl5200-clocking;
> >
> >       rtc@32 {
> >               device_type = "rtc";
> >               compatible = "epson,pcf8564";
> >               reg = <51>;
> >       };
> > };
>
> My only comment would be that the fsl5200-clocking property is
> totally redundant.
>
> Drivers can look at the compatible property (mpc5200b-i2c and
> mpc5200-i2c) to match up what special needs the driver may need.
> Even if it was just fsl-i2c, it could/should be implicit that
> this device is the onboard i2c and the parent node is ostensibly
> going to be marked as an MPC52xx SoC.. or it can look for the
> mpc5200-cdm node. There is no reason to invent a property just
> so you can do a property search when it replaces code of the
> same size to do a node or compatible search..

Yeah, I agree.  Drop the fsl-clocking property.  The hardware is
adequately described without it.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Jon Smirl wrote:
> > This is my first pass at reworking the Freescale i2c driver. It
> > switches the driver from being a platform driver to an open firmware
> > one. I've checked it out on my hardware and it is working.
>
> We may want to hold off on this until arch/ppc goes away (or at least
> all users of this driver in arch/ppc).

How about renaming the old driver file and leaving it hooked to ppc?
Then it would get deleted when ppc goes away. That would let work
progress on the powerpc version.

I'm working on this because I'm adding codecs under powerpc that use
i2c and I want consistent device tree use.

> > You can specific i2c devices on a bus by adding child nodes for them
> > in the device tree. It does not know how to autoload the i2c chip
> > driver modules.
> >
> > i2c@3d40 {
> >       device_type = "i2c";
> >       compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
>
> dtc supports the "mpc5200b-i2c", "mpc5200-i2c", "fsl-i2c" syntax.
>
> >       cell-index = <1>;
>
> What is cell-index for?

I was using it to control the bus number, is that the wrong attribute?

> >       fsl5200-clocking;
>
> As Matt pointed out, this is redundant.
>
> >       rtc@32 {
> >               device_type = "rtc";
>
> This isn't really necessary.

Code doesn't access it. I copied it from a pre-existing device tree.

> > One side effect is that legacy style i2c drivers won't work anymore.
>
> If you mean legacy-style client drivers, why not?

i2c_new_device() doesn't work with legacy-style client drivers.

>
> > The driver contains a table for mapping device tree style names to
> > linux kernel ones.
>
> Can we please put this in common code instead?
>
> > +static struct i2c_driver_device i2c_devices[] = {
> > +     {"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
> > +     {"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
> > +     {"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
> > +     {"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
> > +     {"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
> > +};
>
> At the very least, include the entries that are already being used with
> this driver in fsl_soc.c.

This is the same table from fsl_soc.c it has been moved. The data
really belongs in the i2c drivers if I can figure out how to get it
there.


> > I'd like to get rid of this table.  There are two obvious solutions,
> > use names in the device tree that match up with the linux device
> > driver names.
>
> This was proposed and rejected a while ago.  For one thing, some drivers
> handle multiple chips, and we shouldn't base device tree bindings on
> what groupings Linux happens to make in what driver supports what.
>
> > Or push these strings into the chip drivers and modify
> > i2c-core to also match on the device tree style names.
>
> That would be ideal; it just hasn't been done yet.

This is not hard to do but the i2c people will have to agree. I need
to change the i2c_driver structure to include the additional names.

> A middle ground for now would be to keep one table in drivers/of or
> something.
>
> > Without one of
> > these changes the table is going to need a mapping for every i2c
> > device on the market.
>
> Nah, only one for every i2c device Linux supports. :-)
>
> > diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> > index 1cf29c9..6f80216 100644
> > --- a/arch/powerpc/sysdev/fsl_soc.c
> > +++ b/arch/powerpc/sysdev/fsl_soc.c
> > @@ -306,122 +306,6 @@ err:
> >
> >  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",},
> > -};
>
> This is obviously not based on head-of-tree -- there are several more
> entries in this table.

It is based on 2.6.23. Head of tree hasn't been working very well for
me. I'll rebase it when I can get things working again.

> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index d8de4ac..5ceb936 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 <asm/of_platform.h>
>
> Should be linux/of_platform.h
changed
>
> > @@ -180,7 +182,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 +194,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;
>
> This is a separate change from the OF-ization -- at least note it in the
> changelog.
done
>
> > +     for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> > +             if (!of_device_is_compatible(node, i2c_devices[i].of_device))
> > +                     continue;
> > +             strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
> > +             strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
> > +             printk("i2c driver is %s\n", info->driver_name);
>
> Should be KERN_DEBUG, if it stays at all.
deleted
>
> > +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;
> > +             int len;
> > +
> > +             addr = of_get_property(node, "reg", &len);
> > +             if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
> > +                     printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
> > +                     continue;
> > +             }
> > +
> > +             info.irq = irq_of_parse_and_map(node, 0);
> > +             if (info.irq == NO_IRQ)
> > +                     info.irq = -1;
> > +
> > +             if (of_find_i2c_driver(node, &info) < 0)
> > +                     continue;
> > +
> > +             info.platform_data = NULL;
> > +             info.addr = *addr;
> > +
> > +             i2c_new_device(adap, &info);
> > +     }
> > +}
>
> Most of this code should be made generic and placed in drivers/of.

How so, it is specific to adding i2c drivers.

>
> > +     index = of_get_property(op->node, "cell-index", NULL);
> > +    if (!index || *index > 5) {
> > +            printk(KERN_ERR "mpc_i2c_probe: Device node %s has invalid "
> > +                            "cell-index property\n", op->node->full_name);
> > +            return -EINVAL;
> > +    }
> > +
>
> We might as well just use i2c_new_device() instead of messing around
> with bus numbers.  Note that this is technically no longer platform
> code, so it's harder to justify claiming the static numberspace.

I was allowing control of the bus number with "cell-index" and
i2c_add_numbered_adapter().
Should I get rid of this and switch to i2c_add_adapter()?

>
> > +     i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
> >       if (!i2c->base) {
> >               printk(KERN_ERR "i2c-mpc - failed to map controller\n");
>
> Use of_iomap().

I didn't write this, how should it be done? MPC_I2C_REGION can be
eliminated by using mem.start - mem.end.

>
> >       if (i2c->irq != 0)
>
> if (i2c->irq != NO_IRQ)
>
> > +static struct of_device_id mpc_i2c_of_match[] = {
> > +     {
> > +             .type           = "i2c",
> > +             .compatible     = "fsl-i2c",
> > +     },
> > +};
> > +MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
>
> Let's take this opportunity to stop matching on device_type here
> (including updating booting-without-of.txt).

Remove the .type line and leave .compatible?

>
> > +static struct of_platform_driver mpc_i2c_driver = {
> > +     .owner          = THIS_MODULE,
> > +     .name           = DRV_NAME,
> > +     .match_table    = mpc_i2c_of_match,
> > +     .probe          = mpc_i2c_probe,
> > +     .remove         = __devexit_p(mpc_i2c_remove),
> > +     .driver         = {
> > +             .name   = DRV_NAME,
> >       },
> >  };
>
> Do we still need .name if we have .driver.name?

Yes, i2c-core is matching on mpc_i2c_driver.name, i2c-core would need
to be changed to use mpc_i2c_driver.driver.name.

>
> > diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> > index 0242d80..b778d35 100644
> > --- a/drivers/rtc/rtc-pcf8563.c
> > +++ b/drivers/rtc/rtc-pcf8563.c
>
> This should be a separate patch.

It will be in final form.

>
> -Scott
>

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> This is my first pass at reworking the Freescale i2c driver. It
> switches the driver from being a platform driver to an open firmware
> one. I've checked it out on my hardware and it is working.

Is there any way to have the i2c chip modules match on the device tree
and be automatically loaded? This is complicated by the fact that
these modules are used on other platforms.

If there is a way to do this for the i2c bus I can apply the same code
to the audio bus as well.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jon Smirl wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
>> Jon Smirl wrote:
>>> This is my first pass at reworking the Freescale i2c driver. It
>>> switches the driver from being a platform driver to an open firmware
>>> one. I've checked it out on my hardware and it is working.
>> We may want to hold off on this until arch/ppc goes away (or at least
>> all users of this driver in arch/ppc).
> 
> How about renaming the old driver file and leaving it hooked to ppc?
> Then it would get deleted when ppc goes away. That would let work
> progress on the powerpc version.

Or we could have one driver that has two probe methods.  I don't like 
forking the driver.

> I'm working on this because I'm adding codecs under powerpc that use
> i2c and I want consistent device tree use.

We already support i2c clients in the device tree, via the code in 
fsl_soc.c.

>>>       cell-index = <1>;
>> What is cell-index for?
> 
> I was using it to control the bus number, is that the wrong attribute?

It shouldn't be specified at all -- the hardware has no concept of a 
device number.

>>>       rtc@32 {
>>>               device_type = "rtc";
>> This isn't really necessary.
> 
> Code doesn't access it. I copied it from a pre-existing device tree.

I'm just trying to keep the damage from spreading. :-)

>>> One side effect is that legacy style i2c drivers won't work anymore.
>> If you mean legacy-style client drivers, why not?
> 
> i2c_new_device() doesn't work with legacy-style client drivers.

No, but they should still work the old way.

>>> Or push these strings into the chip drivers and modify
>>> i2c-core to also match on the device tree style names.
>> That would be ideal; it just hasn't been done yet.
> 
> This is not hard to do but the i2c people will have to agree. I need
> to change the i2c_driver structure to include the additional names.

I got a fair bit of resistance from them on the topic of multiple match 
names for i2c clients.

>> Most of this code should be made generic and placed in drivers/of.
> 
> How so, it is specific to adding i2c drivers.

I meant generic with respect to the type of i2c controller, not generic 
to all device types. :-)

It could be drivers/of/i2c.c, or drivers/i2c/of.c, etc.

>> We might as well just use i2c_new_device() instead of messing around
>> with bus numbers.  Note that this is technically no longer platform
>> code, so it's harder to justify claiming the static numberspace.
> 
> I was allowing control of the bus number with "cell-index" and
> i2c_add_numbered_adapter().
> Should I get rid of this and switch to i2c_add_adapter()?

Yes.

>>> +     i2c->base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
>>>       if (!i2c->base) {
>>>               printk(KERN_ERR "i2c-mpc - failed to map controller\n");
>> Use of_iomap().
> 
> I didn't write this, how should it be done? MPC_I2C_REGION can be
> eliminated by using mem.start - mem.end.

i2c->base = of_iomap(op->node, 0);

of_address_to_resource() and ioremap() are combined into one.

>> Let's take this opportunity to stop matching on device_type here
>> (including updating booting-without-of.txt).
> 
> Remove the .type line and leave .compatible?

Yes.

>>> +static struct of_platform_driver mpc_i2c_driver = {
>>> +     .owner          = THIS_MODULE,
>>> +     .name           = DRV_NAME,
>>> +     .match_table    = mpc_i2c_of_match,
>>> +     .probe          = mpc_i2c_probe,
>>> +     .remove         = __devexit_p(mpc_i2c_remove),
>>> +     .driver         = {
>>> +             .name   = DRV_NAME,
>>>       },
>>>  };
>> Do we still need .name if we have .driver.name?
> 
> Yes, i2c-core is matching on mpc_i2c_driver.name, i2c-core would need
> to be changed to use mpc_i2c_driver.driver.name.

How can i2c-core be matching on a field in an OF-specific struct?

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Matt Sealey]

Scott Wood wrote:
> Jon Smirl wrote:

>>>>       cell-index = <1>;
>>> What is cell-index for?
>> I was using it to control the bus number, is that the wrong attribute?
> 
> It shouldn't be specified at all -- the hardware has no concept of a 
> device number.

Well, all i2c devices have a chip id you can probe for, as for buses I
think cell-index is a holdover from the way the PSC code is organised
on the MPC5200 for example - if you have multiple buses which use the
same registers, for example. It's redundant on the PSC's for programming
because they all use different register offsets but if you move to
other devices like the GPTs, then it is then useful for debugging (it
is far more interesting to say GPT1 than GPT @ offset to match the)
and in general for tweaking OTHER parts of the chip (for instance
the CDM - very relevant!) which use single registers to control entire
swathes of units.

This way if you are in any doubt you can tell which one you should
be futzing with in other parts of the chip without complicated logic
based on MBAR offsets from the manual (magic numbers hinder the
maintainability and portability of the code). It's not relevant for
i2c but like I said, still valid, useful information..

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Matt Sealey wrote:
> Scott Wood wrote:
>> Jon Smirl wrote:
> 
>>>>> cell-index = <1>;
>>>> What is cell-index for?
>>> I was using it to control the bus number, is that the wrong
>>> attribute?
>> 
>> It shouldn't be specified at all -- the hardware has no concept of
>> a device number.
> 
> Well, all i2c devices have a chip id you can probe for,

I meant a controller device number (a.k.a. bus number), which (outside 
of documentation) is purely a Linux invention, and which is what 
cell-index was being used for above.

> as for buses I think cell-index is a holdover from the way the PSC
> code is organised on the MPC5200 for example - if you have multiple
> buses which use the same registers, for example. It's redundant on
> the PSC's for programming because they all use different register
> offsets but if you move to other devices like the GPTs, then it is
> then useful for debugging (it is far more interesting to say GPT1
> than GPT @ offset to match the) and in general for tweaking OTHER
> parts of the chip (for instance the CDM - very relevant!) which use
> single registers to control entire swathes of units.

Right, that's what cell-index is for.  This is different. :-)

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Grant Likely]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Jon Smirl wrote:
> > On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> >> Jon Smirl wrote:
> >>> This is my first pass at reworking the Freescale i2c driver. It
> >>> switches the driver from being a platform driver to an open firmware
> >>> one. I've checked it out on my hardware and it is working.
> >> We may want to hold off on this until arch/ppc goes away (or at least
> >> all users of this driver in arch/ppc).
> >
> > How about renaming the old driver file and leaving it hooked to ppc?
> > Then it would get deleted when ppc goes away. That would let work
> > progress on the powerpc version.
>
> Or we could have one driver that has two probe methods.  I don't like
> forking the driver.

I agree.  This driver can and should have multiple bus bindings.

> >>>       cell-index = <1>;
> >> What is cell-index for?
> >
> > I was using it to control the bus number, is that the wrong attribute?
>
> It shouldn't be specified at all -- the hardware has no concept of a
> device number.

cell-index is important.  It describes the hardware, or more
specifically the layout of the SoC.  The SoC has 2 i2c busses which
are numbered 0 and 1.  This property should stay for the 5200.
However, that is the only purpose of it.  cell-index does *not*
describe the system level bus number.

> > I was allowing control of the bus number with "cell-index" and
> > i2c_add_numbered_adapter().
> > Should I get rid of this and switch to i2c_add_adapter()?
>
> Yes.

Yes, the purpose of cell-index is not to give an i2c bus number enumeration.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Grant Likely]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> Matt Sealey wrote:
> > Scott Wood wrote:
> >> Jon Smirl wrote:
> >
> >>>>> cell-index = <1>;
> >>>> What is cell-index for?
> >>> I was using it to control the bus number, is that the wrong
> >>> attribute?
> >>
> >> It shouldn't be specified at all -- the hardware has no concept of
> >> a device number.
> >
> > Well, all i2c devices have a chip id you can probe for,
>
> I meant a controller device number (a.k.a. bus number), which (outside
> of documentation) is purely a Linux invention, and which is what
> cell-index was being used for above.
>
> > as for buses I think cell-index is a holdover from the way the PSC
> > code is organised on the MPC5200 for example - if you have multiple
> > buses which use the same registers, for example. It's redundant on
> > the PSC's for programming because they all use different register
> > offsets but if you move to other devices like the GPTs, then it is
> > then useful for debugging (it is far more interesting to say GPT1
> > than GPT @ offset to match the)

Actually, it is not intended for this.  cell-index is not intended to
be able to say GPT1 instead of GPT@xxx (while that may be possible, it
is not the intent).  Nor is it a holdover from the PSC code design.
It is designed to describe the internal structure of the SoC.  The
5200 has 6 PSCs, and there are some chip registers which are shared
between all the PSCs (for clocking, IO mode, etc).  It is not
sufficient to simply plop down a device tree node for each PSC because
it doesn't give enough information about which bits to use in the
shared regs.  (For example, the port_config register)

> > and in general for tweaking OTHER
> > parts of the chip (for instance the CDM - very relevant!) which use
> > single registers to control entire swathes of units.
>
> Right, that's what cell-index is for.  This is different. :-)

Yes, this is correct.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > Jon Smirl wrote:
> > > On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > >> Jon Smirl wrote:
> > >>> This is my first pass at reworking the Freescale i2c driver. It
> > >>> switches the driver from being a platform driver to an open firmware
> > >>> one. I've checked it out on my hardware and it is working.
> > >> We may want to hold off on this until arch/ppc goes away (or at least
> > >> all users of this driver in arch/ppc).
> > >
> > > How about renaming the old driver file and leaving it hooked to ppc?
> > > Then it would get deleted when ppc goes away. That would let work
> > > progress on the powerpc version.
> >
> > Or we could have one driver that has two probe methods.  I don't like
> > forking the driver.
>
> I agree.  This driver can and should have multiple bus bindings.

What non-of platform uses this driver?

I believe this is the last struct platform driver left for the
mpc5200. Fixing this one allows the platform bus to be removed. With a
device tree there isn't any reason to keep a platform bus, everything
should be on the of_platform bus.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> >>> One side effect is that legacy style i2c drivers won't work anymore.
> >> If you mean legacy-style client drivers, why not?
> >
> > i2c_new_device() doesn't work with legacy-style client drivers.
>
> No, but they should still work the old way.

I'm not in favor trying to support both legacy and new style i2c
drivers.  It took me all of five minutes to convert an existing legacy
driver to the new style. Pretty much all you need to do is delete code
(about 100 lines). So I'd recommend converting the drivers we are
interest in instead of trying to support both types.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > +static struct of_platform_driver mpc_i2c_driver = {
> > +     .owner          = THIS_MODULE,
> > +     .name           = DRV_NAME,
> > +     .match_table    = mpc_i2c_of_match,
> > +     .probe          = mpc_i2c_probe,
> > +     .remove         = __devexit_p(mpc_i2c_remove),
> > +     .driver         = {
> > +             .name   = DRV_NAME,
> >       },
> >  };
>
> Do we still need .name if we have .driver.name?

This is a general question, if of_platform_driver doesn't need .name
it should be removed from the structure.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Stephen Rothwell]

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

On Mon, 5 Nov 2007 20:34:51 -0500 "Jon Smirl" <jonsmirl@gmail.com> wrote:
>
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > > +static struct of_platform_driver mpc_i2c_driver = {
> > > +     .owner          = THIS_MODULE,
> > > +     .name           = DRV_NAME,
> > > +     .match_table    = mpc_i2c_of_match,
> > > +     .probe          = mpc_i2c_probe,
> > > +     .remove         = __devexit_p(mpc_i2c_remove),
> > > +     .driver         = {
> > > +             .name   = DRV_NAME,
> > >       },
> > >  };
> >
> > Do we still need .name if we have .driver.name?
> 
> This is a general question, if of_platform_driver doesn't need .name
> it should be removed from the structure.

I am in the process of doing that.  However there a quite a few drivers
that need to be fixed first.  In the meantime,
of_register_platform_driver will copy which ever of the name and owner
fields you fill in to the others, so we can convert over time.  For new
drivers (and changing), please use the name and owner fields in the
embedded device_driver struct. (this is the same as done by the platform
drivers currently ...)

-- 
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: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> >>> Or push these strings into the chip drivers and modify
> >>> i2c-core to also match on the device tree style names.
> >> That would be ideal; it just hasn't been done yet.
> >
> > This is not hard to do but the i2c people will have to agree. I need
> > to change the i2c_driver structure to include the additional names.
>
> I got a fair bit of resistance from them on the topic of multiple match
> names for i2c clients.

Here's a first pass at pushing the strings back into the i2c drivers.
If this looks reasonable it can be optimized a lot more.  A more
advanced version of this code would combine the alias, name, and
driver_name fields. The existing pairing of driver_name/name could be
merged into the concept of alias names for the driver.

Extend i2c-core to support lists of device tree compatible names when
matching drivers

From: Jon Smirl <jonsmirl@gmail.com>


---

 drivers/i2c/busses/i2c-mpc.c |   35 ++++-------------------------------
 drivers/i2c/i2c-core.c       |   17 +++++++++++++++--
 drivers/rtc/rtc-pcf8563.c    |    1 +
 drivers/rtc/rtc-rs5c372.c    |    1 +
 include/linux/i2c.h          |   11 +++++++++--
 5 files changed, 30 insertions(+), 35 deletions(-)


diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4ddebe4..6313631 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -312,34 +312,6 @@ static struct i2c_adapter mpc_ops =3D {
 =09.retries =3D 1
 };

-struct i2c_driver_device {
-=09char=09*of_device;
-=09char=09*i2c_driver;
-=09char=09*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] =3D {
-=09{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
-=09{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
-=09{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
-=09{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
-=09{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
-};
-
-static int of_find_i2c_driver(struct device_node *node, struct
i2c_board_info *info)
-{
-=09int i;
-
-=09for (i =3D 0; i < ARRAY_SIZE(i2c_devices); i++) {
-=09=09if (!of_device_is_compatible(node, i2c_devices[i].of_device))
-=09=09=09continue;
-=09=09strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN)=
;
-=09=09strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
-=09=09return 0;
-=09}
-=09return -ENODEV;
-}
-
 static void of_register_i2c_devices(struct i2c_adapter *adap, struct
device_node *adap_node)
 {
 =09struct device_node *node =3D NULL;
@@ -347,6 +319,7 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
 =09while ((node =3D of_get_next_child(adap_node, node))) {
 =09=09struct i2c_board_info info;
 =09=09const u32 *addr;
+=09=09const char *compatible;
 =09=09int len;

 =09=09addr =3D of_get_property(node, "reg", &len);
@@ -359,9 +332,9 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
 =09=09if (info.irq =3D=3D NO_IRQ)
 =09=09=09info.irq =3D -1;

-=09=09if (of_find_i2c_driver(node, &info) < 0)
-=09=09=09continue;
-
+=09=09compatible =3D of_get_property(node, "compatible", &len);
+=09=09strlcpy(info.driver_name, compatible, len);
+=09=09info.type[0] =3D '\0';
 =09=09info.platform_data =3D NULL;
 =09=09info.addr =3D *addr;

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d663e69..4128cd1 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -17,7 +17,7 @@
     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.=09=09     */
 /* -----------------------------------------------------------------------=
-- */

-/* With some changes from Ky=F6sti M=E4lkki <kmalkki@cc.hut.fi>.
+/* With some changes from Ky=EF=BF=9Csti M=EF=BF=9Clkki <kmalkki@cc.hut.fi=
>.
    All SMBus-related things are written by Frodo Looijaard <frodol@dds.nl>
    SMBus 2.0 support by Mark Studebaker <mdsxyz123@yahoo.com> and
    Jean Delvare <khali@linux-fr.org> */
@@ -51,6 +51,7 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
 {
 =09struct i2c_client=09*client =3D to_i2c_client(dev);
 =09struct i2c_driver=09*driver =3D to_i2c_driver(drv);
+=09char **alias;

 =09/* make legacy i2c drivers bypass driver model probing entirely;
 =09 * such drivers scan each i2c adapter/bus themselves.
@@ -61,7 +62,19 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
 =09/* new style drivers use the same kind of driver matching policy
 =09 * as platform devices or SPI:  compare device and driver IDs.
 =09 */
-=09return strcmp(client->driver_name, drv->name) =3D=3D 0;
+=09if (strcmp(client->driver_name, drv->name) =3D=3D 0)
+=09=09return true;
+
+=09/* Match against alias device tree names */
+=09if (!driver->alias)
+=09=09return 0;
+=09alias =3D driver->alias;
+=09while (*alias) {
+=09=09if (strcmp(client->driver_name, *alias) =3D=3D 0)
+=09=09=09return true;
+=09=09alias++;
+=09}
+=09return 0;
 }

 #ifdef=09CONFIG_HOTPLUG
diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index b778d35..7c8caf5 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -266,6 +266,7 @@ static struct i2c_driver pcf8563_driver =3D {
 =09.driver=09=09=3D {
 =09=09.name=09=3D "rtc-pcf8563",
 =09},
+=09.alias=09=3D (char *[]){"epson,pcf8564", 0},
 =09.id=09=09=3D I2C_DRIVERID_PCF8563,
 =09.probe =3D &pcf8563_probe,
 =09.remove =3D &pcf8563_remove,
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 6b67b50..0f37d04 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -649,6 +649,7 @@ static struct i2c_driver rs5c372_driver =3D {
 =09.driver=09=09=3D {
 =09=09.name=09=3D "rtc-rs5c372",
 =09},
+=09.alias=09=3D (char
*[]){"ricoh,rs5c372a","ricoh,rs5c372b","ricoh,rv5c386","ricoh,rv5c387a",0},
 =09.probe=09=09=3D rs5c372_probe,
 =09.remove=09=09=3D rs5c372_remove,
 };
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2a32f2f..d55748c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -107,6 +107,13 @@ extern s32 i2c_smbus_write_i2c_block_data(struct
i2c_client * client,
 struct i2c_driver {
 =09int id;
 =09unsigned int class;
+=09
+=09/* Alias name for the driver. Used to support device trees on
+=09 * the PowerPC architecture. Device tree names take the form of
+=09 * vendor,chip. For example "epson,pcf8564". Alias is a list of
+=09 * strings terminated by a zero entry.
+=09 */
+=09char **alias;

 =09/* Notifies the driver that a new bus has appeared. This routine
 =09 * can be used by the driver to test if the bus meets its conditions
@@ -146,7 +153,7 @@ struct i2c_driver {
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)

-#define I2C_NAME_SIZE=0920
+#define I2C_NAME_SIZE=0940

 /**
  * struct i2c_client - represent an I2C slave device
@@ -225,7 +232,7 @@ static inline void i2c_set_clientdata (struct
i2c_client *dev, void *data)
  * with the adapter already known.
  */
 struct i2c_board_info {
-=09char=09=09driver_name[KOBJ_NAME_LEN];
+=09char=09=09driver_name[I2C_NAME_SIZE];
 =09char=09=09type[I2C_NAME_SIZE];
 =09unsigned short=09flags;
 =09unsigned short=09addr;


--=20
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Stephen Rothwell]

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

Hi Jon,

On Mon, 5 Nov 2007 23:25:35 -0500 "Jon Smirl" <jonsmirl@gmail.com> wrote:
>
> +		compatible = of_get_property(node, "compatible", &len);
> +		strlcpy(info.driver_name, compatible, len);

Of course, you will check len against sizeof(info.driver_name), right?

> -/* With some changes from Kyösti Mälkki <kmalkki@cc.hut.fi>.
> +/* With some changes from Kyï¿œsti Mï¿œlkki <kmalkki@cc.hut.fi>.

Did you mean to change this?

> +	char **alias;

const char ** would be nicer.

>  struct i2c_driver {
>  	int id;
>  	unsigned int class;
> +	
> +	/* Alias name for the driver. Used to support device trees on
> +	 * the PowerPC architecture. Device tree names take the form of
> +	 * vendor,chip. For example "epson,pcf8564". Alias is a list of
> +	 * strings terminated by a zero entry.
> +	 */
> +	char **alias;

const char ** would be nicer;

-- 
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: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jon Smirl wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
>>>>> One side effect is that legacy style i2c drivers won't work anymore.
>>>> If you mean legacy-style client drivers, why not?
>>> i2c_new_device() doesn't work with legacy-style client drivers.
>> No, but they should still work the old way.
> 
> I'm not in favor trying to support both legacy and new style i2c
> drivers.

I don't understand what it is that you did that would break support for 
legacy clients, though.

>  It took me all of five minutes to convert an existing legacy
> driver to the new style. Pretty much all you need to do is delete code
> (about 100 lines). So I'd recommend converting the drivers we are
> interest in instead of trying to support both types.

Sure, conversion is good, but that doesn't mean we want things to 
suddenly break for users.

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jean Delvare]

Hi Scott, Jon,

On Mon, 05 Nov 2007 14:51:51 -0600, Scott Wood wrote:
> Jon Smirl wrote:
> > How about renaming the old driver file and leaving it hooked to ppc?
> > Then it would get deleted when ppc goes away. That would let work
> > progress on the powerpc version.
> 
> Or we could have one driver that has two probe methods.  I don't like 
> forking the driver.

I agree with Scott here, I don't want to fork the drivers. It is
possible (and easy) to support both methods in the same module, let's
just to that. See for example David Brownell's work on the lm75 driver:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-September/021270.html

> > i2c_new_device() doesn't work with legacy-style client drivers.
> 
> No, but they should still work the old way.

Definitely.

> > This is not hard to do but the i2c people will have to agree. I need
> > to change the i2c_driver structure to include the additional names.
> 
> I got a fair bit of resistance from them on the topic of multiple match 
> names for i2c clients.

Really? All I said is that you were a bit late in the game because this
had been discussed before. I know that David Brownell doesn't agree
with you (he designed what we have now), but me, I am still open to
discussing the matter, especially when more people complain about the
situation every month.

> >> We might as well just use i2c_new_device() instead of messing around
> >> with bus numbers.  Note that this is technically no longer platform
> >> code, so it's harder to justify claiming the static numberspace.
> > 
> > I was allowing control of the bus number with "cell-index" and
> > i2c_add_numbered_adapter().
> > Should I get rid of this and switch to i2c_add_adapter()?
> 
> Yes.

No! If you don't call i2c_add_numbered_adapter() then new-style i2c
clients will never work on your i2c adapter.

-- 
Jean Delvare

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jean Delvare]

On Mon, 05 Nov 2007 21:52:06 +0000, Matt Sealey wrote:
> Well, all i2c devices have a chip id you can probe for (...)

This statement is completely incorrect. I2C devices do NOT have
standard ID registers. Some devices have proprietary ID registers, some
don't, it's really up to the manfacturer.

-- 
Jean Delvare

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jean Delvare wrote:
>>>> We might as well just use i2c_new_device() instead of messing around
>>>> with bus numbers.  Note that this is technically no longer platform
>>>> code, so it's harder to justify claiming the static numberspace.
>>> I was allowing control of the bus number with "cell-index" and
>>> i2c_add_numbered_adapter().
>>> Should I get rid of this and switch to i2c_add_adapter()?
>> Yes.
> 
> No! If you don't call i2c_add_numbered_adapter() then new-style i2c
> clients will never work on your i2c adapter.

I thought that was what i2c_new_device() was for?

By handling all the device tree stuff in the driver, it acts more like 
an add-on adapter than a platform device.

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

On 11/6/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Scott, Jon,
>
> On Mon, 05 Nov 2007 14:51:51 -0600, Scott Wood wrote:
> > Jon Smirl wrote:
> > > How about renaming the old driver file and leaving it hooked to ppc?
> > > Then it would get deleted when ppc goes away. That would let work
> > > progress on the powerpc version.
> >
> > Or we could have one driver that has two probe methods.  I don't like
> > forking the driver.
>
> I agree with Scott here, I don't want to fork the drivers. It is
> possible (and easy) to support both methods in the same module, let's
> just to that. See for example David Brownell's work on the lm75 driver:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2007-September/021270.html

I agree that it is easy to make make a chip driver support both new
and old style.

But when I call i2c_new_device() on an old style chip driver it exits
saying that it doesn't work for the old style adapters. Checks for
is_newstyle_driver() are in the i2c_new_device code. That's what
caused me to rewrite the rtc-pcf8563 driver for the new style. This
probably related to probing, I have to pass the address in struct
i2c_board_info. The old style drivers don't support having their
address passed in.

This may be complicated by the fact that the rtc drivers I'm working
on are not probable. That's why I want to add device tree support for
them.

If this is going to work on an old style driver, how do I get the address to it?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jean Delvare]

Hi Scott,

On Tue, 06 Nov 2007 11:36:23 -0600, Scott Wood wrote:
> Jean Delvare wrote:
> >>>> We might as well just use i2c_new_device() instead of messing around
> >>>> with bus numbers.  Note that this is technically no longer platform
> >>>> code, so it's harder to justify claiming the static numberspace.
> >>> I was allowing control of the bus number with "cell-index" and
> >>> i2c_add_numbered_adapter().
> >>> Should I get rid of this and switch to i2c_add_adapter()?
> >> Yes.
> > 
> > No! If you don't call i2c_add_numbered_adapter() then new-style i2c
> > clients will never work on your i2c adapter.
> 
> I thought that was what i2c_new_device() was for?

Sorry, I've not been completely clear. Yes, you can use
i2c_new_device() on an adapter that has been added with
i2c_add_adapter(). However, this requires that you have a reference to
that i2c_adapter, which is usually not the case with system-wide I2C
buses. Embedded platforms would rather use i2c_add_numbered_adapter(),
give a list of chips to i2c_register_board_info() and let i2c-core
instantiate them. i2c_new_device was primarily meant for multimedia
adapters.

> By handling all the device tree stuff in the driver, it acts more like 
> an add-on adapter than a platform device.

-- 
Jean Delvare

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jean Delvare]

Hi Jon,

On Tue, 6 Nov 2007 12:45:24 -0500, Jon Smirl wrote:
> On 11/6/07, Jean Delvare wrote:
> > I agree with Scott here, I don't want to fork the drivers. It is
> > possible (and easy) to support both methods in the same module, let's
> > just to that. See for example David Brownell's work on the lm75 driver:
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2007-September/021270.html
> 
> I agree that it is easy to make make a chip driver support both new
> and old style.
> 
> But when I call i2c_new_device() on an old style chip driver it exits
> saying that it doesn't work for the old style adapters. Checks for
> is_newstyle_driver() are in the i2c_new_device code. That's what
> caused me to rewrite the rtc-pcf8563 driver for the new style. This
> probably related to probing, I have to pass the address in struct
> i2c_board_info. The old style drivers don't support having their
> address passed in.

I know that. The trick is to register two struct i2c_driver (again see
the lm75 example), one old-style, one new-style. I agree it's not very
elegant, but it works. Hopefully we can get rid of the old-style one
after some time, and it allows for a smooth transition.

> This may be complicated by the fact that the rtc drivers I'm working
> on are not probable. That's why I want to add device tree support for
> them.
> 
> If this is going to work on an old style driver, how do I get the
> address to it?

Old-style drivers probe for supported chips on all possible addresses
(for the chip in question). If the chip can't be probed, then module
parameters must be used. That's not terribly convenient, and new-style
drivers are much preferred in this case.

-- 
Jean Delvare

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Grant Likely]

On 11/6/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Scott,
>
> On Tue, 06 Nov 2007 11:36:23 -0600, Scott Wood wrote:
> > Jean Delvare wrote:
> > >>>> We might as well just use i2c_new_device() instead of messing around
> > >>>> with bus numbers.  Note that this is technically no longer platform
> > >>>> code, so it's harder to justify claiming the static numberspace.
> > >>> I was allowing control of the bus number with "cell-index" and
> > >>> i2c_add_numbered_adapter().
> > >>> Should I get rid of this and switch to i2c_add_adapter()?
> > >> Yes.
> > >
> > > No! If you don't call i2c_add_numbered_adapter() then new-style i2c
> > > clients will never work on your i2c adapter.
> >
> > I thought that was what i2c_new_device() was for?
>
> Sorry, I've not been completely clear. Yes, you can use
> i2c_new_device() on an adapter that has been added with
> i2c_add_adapter(). However, this requires that you have a reference to
> that i2c_adapter, which is usually not the case with system-wide I2C
> buses. Embedded platforms would rather use i2c_add_numbered_adapter(),
> give a list of chips to i2c_register_board_info() and let i2c-core
> instantiate them. i2c_new_device was primarily meant for multimedia
> adapters.

*Some* embedded platforms would rather use i2c_add_numbered_adapter().  :-)

On powerpc, and other platforms which have a device tree, we don't
need to define a table of devices in the platform code because we've
already got a rich structure for describing such things.  The i2c
busses and i2c devices are grouped together in the device tree, so
when the i2c bus is probed, it should call out to common i2c device
tree parsing code to instantiate all the devices described in the
tree.

It would be awkward to describe the i2c bus in the device tree but
still have to use a static structure to describe the devices on that
bus.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Grant Likely]

On 11/6/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 11/6/07, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Scott,
> >
> > On Tue, 06 Nov 2007 11:36:23 -0600, Scott Wood wrote:
> > > Jean Delvare wrote:
> > > >>>> We might as well just use i2c_new_device() instead of messing around
> > > >>>> with bus numbers.  Note that this is technically no longer platform
> > > >>>> code, so it's harder to justify claiming the static numberspace.
> > > >>> I was allowing control of the bus number with "cell-index" and
> > > >>> i2c_add_numbered_adapter().
> > > >>> Should I get rid of this and switch to i2c_add_adapter()?
> > > >> Yes.
> > > >
> > > > No! If you don't call i2c_add_numbered_adapter() then new-style i2c
> > > > clients will never work on your i2c adapter.
> > >
> > > I thought that was what i2c_new_device() was for?
> >
> > Sorry, I've not been completely clear. Yes, you can use
> > i2c_new_device() on an adapter that has been added with
> > i2c_add_adapter(). However, this requires that you have a reference to
> > that i2c_adapter, which is usually not the case with system-wide I2C
> > buses. Embedded platforms would rather use i2c_add_numbered_adapter(),
> > give a list of chips to i2c_register_board_info() and let i2c-core
> > instantiate them. i2c_new_device was primarily meant for multimedia
> > adapters.
>
> *Some* embedded platforms would rather use i2c_add_numbered_adapter().  :-)
>
> On powerpc, and other platforms which have a device tree, we don't

Specifically; an OF style device tree.  :-)

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Scott Wood]

Jean Delvare wrote:
> Sorry, I've not been completely clear. Yes, you can use
> i2c_new_device() on an adapter that has been added with
> i2c_add_adapter(). However, this requires that you have a reference to
> that i2c_adapter, which is usually not the case with system-wide I2C
> buses.

But it is the case here, because the i2c driver knows about the device 
tree, and thus can pass the device tree node and the adapter struct to 
the enumeration function.

The driver should still do i2c_add_numbered_adapter() when using the 
non-OF platform device binding, in which case it gets the bus number 
from the platform data.

-Scott

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Matt Sealey]

Jean Delvare wrote:
> On Mon, 05 Nov 2007 21:52:06 +0000, Matt Sealey wrote:
>> Well, all i2c devices have a chip id you can probe for (...)
> 
> This statement is completely incorrect. I2C devices do NOT have
> standard ID registers. Some devices have proprietary ID registers, some
> don't, it's really up to the manfacturer.

All I2C slave devices have to have a 7- or 10-bit address to identify them
by. They *may* not report what they ARE, but this is 9 times out of
10 a hardware design decision of soldering the chip to a board and
the address is then coded into device trees or hardcoded into drivers.

Whoever designed the board and has the datasheets knows the address
they're supposed to be at, and the device can accept this.

You simply cannot entertain an i2c bus with "anonymous and unnumbered
devices", every one has to have an address it responds to, however
it is defined, or it just does not work.

WRT cell-index this is an index of the bus on the chip (not the logical
i2c bus but the physical difference between two i2c controllers) and
then any i2c devices which need to be communicated with would be
child nodes, their reg property reflecting their slave address, is
that not correct?

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

Second pass at extending i2c core to accept strings of aliases for the
module. This version eliminate the need for separate name and type
fields when selecting a driver. PowerPC has to have a mapping from
device tree names to the i2c drivers, it makes sense to keep this
mapping inside the i2c driver.

Extend i2c-core to support lists of device tree compatible names when
matching drivers

From: Jon Smirl <jonsmirl@gmail.com>


---

 drivers/i2c/busses/i2c-mpc.c |   37 +++++++------------------------------
 drivers/i2c/i2c-core.c       |   35 ++++++++++++++++++-----------------
 drivers/rtc/rtc-pcf8563.c    |    1 +
 drivers/rtc/rtc-rs5c372.c    |    3 ++-
 include/linux/i2c.h          |   13 +++++++++----
 5 files changed, 37 insertions(+), 52 deletions(-)


diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4ddebe4..30420ad 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -312,34 +312,6 @@ static struct i2c_adapter mpc_ops =3D {
 =09.retries =3D 1
 };

-struct i2c_driver_device {
-=09char=09*of_device;
-=09char=09*i2c_driver;
-=09char=09*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] =3D {
-=09{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
-=09{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
-=09{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
-=09{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
-=09{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
-};
-
-static int of_find_i2c_driver(struct device_node *node, struct
i2c_board_info *info)
-{
-=09int i;
-
-=09for (i =3D 0; i < ARRAY_SIZE(i2c_devices); i++) {
-=09=09if (!of_device_is_compatible(node, i2c_devices[i].of_device))
-=09=09=09continue;
-=09=09strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN)=
;
-=09=09strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
-=09=09return 0;
-=09}
-=09return -ENODEV;
-}
-
 static void of_register_i2c_devices(struct i2c_adapter *adap, struct
device_node *adap_node)
 {
 =09struct device_node *node =3D NULL;
@@ -347,11 +319,12 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
 =09while ((node =3D of_get_next_child(adap_node, node))) {
 =09=09struct i2c_board_info info;
 =09=09const u32 *addr;
+=09=09const char *compatible;
 =09=09int len;

 =09=09addr =3D of_get_property(node, "reg", &len);
 =09=09if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
-=09=09=09printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
+=09=09=09printk(KERN_WARNING "i2c-mpc.c: invalid entry, missing reg attrib=
ute\n");
 =09=09=09continue;
 =09=09}

@@ -359,8 +332,12 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
 =09=09if (info.irq =3D=3D NO_IRQ)
 =09=09=09info.irq =3D -1;

-=09=09if (of_find_i2c_driver(node, &info) < 0)
+=09=09compatible =3D of_get_property(node, "compatible", &len);
+=09=09if (!compatible) {
+=09=09=09printk(KERN_WARNING "i2c-mpc.c: invalid entry, missing compatible
attribute\n");
 =09=09=09continue;
+=09=09}
+=09=09strncpy(info.name, compatible, sizeof(info.name));

 =09=09info.platform_data =3D NULL;
 =09=09info.addr =3D *addr;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d663e69..d9a70c2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -17,7 +17,7 @@
     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.=09=09     */
 /* -----------------------------------------------------------------------=
-- */

-/* With some changes from Ky=F6sti M=E4lkki <kmalkki@cc.hut.fi>.
+/* With some changes from Ky=C3=B6sti M=C3=80lkki <kmalkki@cc.hut.fi>.
    All SMBus-related things are written by Frodo Looijaard <frodol@dds.nl>
    SMBus 2.0 support by Mark Studebaker <mdsxyz123@yahoo.com> and
    Jean Delvare <khali@linux-fr.org> */
@@ -51,6 +51,7 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
 {
 =09struct i2c_client=09*client =3D to_i2c_client(dev);
 =09struct i2c_driver=09*driver =3D to_i2c_driver(drv);
+=09char const **alias;

 =09/* make legacy i2c drivers bypass driver model probing entirely;
 =09 * such drivers scan each i2c adapter/bus themselves.
@@ -60,8 +61,18 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)

 =09/* new style drivers use the same kind of driver matching policy
 =09 * as platform devices or SPI:  compare device and driver IDs.
+=09 * Match against arrary of alias device tree names. When a match
+=09 * is found change the reference to point at the copy inside the
+=09 * chip driver allowing the caller's string to be freed.
 =09 */
-=09return strcmp(client->driver_name, drv->name) =3D=3D 0;
+=09alias =3D driver->aliases;
+=09while (*alias) {
+=09=09if (strcmp(client->name, *alias) =3D=3D 0) {
+=09=09=09return true;
+=09=09}
+=09=09alias++;
+=09}
+=09return 0;
 }

 #ifdef=09CONFIG_HOTPLUG
@@ -74,11 +85,11 @@ static int i2c_device_uevent(struct device *dev,
char **envp, int num_envp,
 =09int=09=09=09i =3D 0, length =3D 0;

 =09/* by definition, legacy drivers can't hotplug */
-=09if (dev->driver || !client->driver_name)
+=09if (dev->driver || !client->name)
 =09=09return 0;

 =09if (add_uevent_var(envp, num_envp, &i, buffer, buffer_size, &length,
-=09=09=09"MODALIAS=3D%s", client->driver_name))
+=09=09=09"MODALIAS=3D%s", client->name))
 =09=09return -ENOMEM;
 =09envp[i] =3D NULL;
 =09dev_dbg(dev, "uevent\n");
@@ -169,22 +180,15 @@ static void i2c_client_dev_release(struct device *dev=
)
 =09kfree(to_i2c_client(dev));
 }

-static ssize_t show_client_name(struct device *dev, struct
device_attribute *attr, char *buf)
-{
-=09struct i2c_client *client =3D to_i2c_client(dev);
-=09return sprintf(buf, "%s\n", client->name);
-}
-
 static ssize_t show_modalias(struct device *dev, struct
device_attribute *attr, char *buf)
 {
 =09struct i2c_client *client =3D to_i2c_client(dev);
-=09return client->driver_name
-=09=09? sprintf(buf, "%s\n", client->driver_name)
+=09return client->name
+=09=09? sprintf(buf, "%s\n", client->name)
 =09=09: 0;
 }

 static struct device_attribute i2c_dev_attrs[] =3D {
-=09__ATTR(name, S_IRUGO, show_client_name, NULL),
 =09/* modalias helps coldplug:  modprobe $(cat .../modalias) */
 =09__ATTR(modalias, S_IRUGO, show_modalias, NULL),
 =09{ },
@@ -233,10 +237,7 @@ i2c_new_device(struct i2c_adapter *adap, struct
i2c_board_info const *info)
 =09client->flags =3D info->flags;
 =09client->addr =3D info->addr;
 =09client->irq =3D info->irq;
-
-=09strlcpy(client->driver_name, info->driver_name,
-=09=09sizeof(client->driver_name));
-=09strlcpy(client->name, info->type, sizeof(client->name));
+=09strlcpy(client->name, info->name, sizeof(info->name));

 =09/* a new style driver may be bound to this device when we
 =09 * return from this function, or any later moment (e.g. maybe
diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index b778d35..8162f77 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -266,6 +266,7 @@ static struct i2c_driver pcf8563_driver =3D {
 =09.driver=09=09=3D {
 =09=09.name=09=3D "rtc-pcf8563",
 =09},
+=09.aliases =3D (char const *[]){"philips,pcf8563", "epson,rtc8564", 0},
 =09.id=09=09=3D I2C_DRIVERID_PCF8563,
 =09.probe =3D &pcf8563_probe,
 =09.remove =3D &pcf8563_remove,
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 6b67b50..b2da981 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -62,11 +62,11 @@


 enum rtc_type {
-=09rtc_undef =3D 0,
 =09rtc_rs5c372a,
 =09rtc_rs5c372b,
 =09rtc_rv5c386,
 =09rtc_rv5c387a,
+=09rtc_undef,
 };

 /* REVISIT:  this assumes that:
@@ -649,6 +649,7 @@ static struct i2c_driver rs5c372_driver =3D {
 =09.driver=09=09=3D {
 =09=09.name=09=3D "rtc-rs5c372",
 =09},
+=09.aliases=09=3D (char const
*[]){"ricoh,rs5c372a","ricoh,rs5c372b","ricoh,rv5c386","ricoh,rv5c387a",=09=
0},
 =09.probe=09=09=3D rs5c372_probe,
 =09.remove=09=09=3D rs5c372_remove,
 };
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2a32f2f..4384cc1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -107,6 +107,13 @@ extern s32 i2c_smbus_write_i2c_block_data(struct
i2c_client * client,
 struct i2c_driver {
 =09int id;
 =09unsigned int class;
+=09
+=09/* Alias names for the driver. Used to support device trees on
+=09 * the PowerPC architecture. Device tree names take the form of
+=09 * vendor,chip. For example "epson,rtc8564". Alias is a list of
+=09 * strings terminated by a zero entry.
+=09 */
+=09char const **aliases;

 =09/* Notifies the driver that a new bus has appeared. This routine
 =09 * can be used by the driver to test if the bus meets its conditions
@@ -146,7 +153,7 @@ struct i2c_driver {
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)

-#define I2C_NAME_SIZE=0920
+#define I2C_NAME_SIZE=0940

 /**
  * struct i2c_client - represent an I2C slave device
@@ -181,7 +188,6 @@ struct i2c_client {
 =09=09=09=09=09/* to the client=09=09*/
 =09struct device dev;=09=09/* the device structure=09=09*/
 =09int irq;=09=09=09/* irq issued by device (or -1) */
-=09char driver_name[KOBJ_NAME_LEN];
 =09struct list_head list;
 =09struct completion released;
 };
@@ -225,8 +231,7 @@ static inline void i2c_set_clientdata (struct
i2c_client *dev, void *data)
  * with the adapter already known.
  */
 struct i2c_board_info {
-=09char=09=09driver_name[KOBJ_NAME_LEN];
-=09char=09=09type[I2C_NAME_SIZE];
+=09char =09=09name[I2C_NAME_SIZE];
 =09unsigned short=09flags;
 =09unsigned short=09addr;
 =09void=09=09*platform_data;

--=20
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jon Smirl]

Second pass on rework of mpc-i2c.c. This doesn't include code for
supporting both old and new style chip drivers. I'm still not a fan of
cluttering up mpc-i2c.c to support old style drivers since it so easy
to convert them to the new style. I'd rather just fix up the i2c
drivers used by chips on the PowerPC platform.

Convert i2c to of_platform_driver from platform_driver

From: Jon Smirl <jonsmirl@gmail.com>

Improve error returns
---

 arch/powerpc/sysdev/fsl_soc.c |  116 ---------------------------
 drivers/i2c/busses/i2c-mpc.c  |  178 +++++++++++++++++++++++++++++------------
 2 files changed, 126 insertions(+), 168 deletions(-)


diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..6f80216 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -306,122 +306,6 @@ err:

 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",},
-};
-
-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;
-		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
-		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
-		return 0;
-	}
-	return -ENODEV;
-}
-
-static void __init of_register_i2c_devices(struct device_node
*adap_node, int bus_num)
-{
-	struct device_node *node = NULL;
-
-	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_ioc.c: invalid i2c device entry\n");
-			continue;
-		}
-
-		info.irq = irq_of_parse_and_map(node, 0);
-		if (info.irq == NO_IRQ)
-			info.irq = -1;
-
-		if (of_find_i2c_driver(node, &info) < 0)
-			continue;
-
-		info.platform_data = NULL;
-		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..4ddebe4 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,74 +312,132 @@ 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 struct i2c_driver_device i2c_devices[] = {
+	{"ricoh,rs5c372a", "rtc-rs5c372", "rs5c372a",},
+	{"ricoh,rs5c372b", "rtc-rs5c372", "rs5c372b",},
+	{"ricoh,rv5c386",  "rtc-rs5c372", "rv5c386",},
+	{"ricoh,rv5c387a", "rtc-rs5c372", "rv5c387a",},
+	{"epson,pcf8564", "rtc-pcf8563", "pcf8564",},
+};
+
+static int 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;
+		strncpy(info->driver_name, i2c_devices[i].i2c_driver, KOBJ_NAME_LEN);
+		strncpy(info->type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+		return 0;
+	}
+	return -ENODEV;
+}
+
+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;
+		int len;
+
+		addr = of_get_property(node, "reg", &len);
+		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
+			printk(KERN_WARNING "i2c-mpc.c: invalid i2c device entry\n");
+			continue;
+		}
+
+		info.irq = irq_of_parse_and_map(node, 0);
+		if (info.irq == NO_IRQ)
+			info.irq = -1;
+
+		if (of_find_i2c_driver(node, &info) < 0)
+			continue;
+
+		info.platform_data = NULL;
+		info.addr = *addr;
+
+		i2c_new_device(adap, &info);
+	}
+}
+
+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))) {
 		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;
 	}

+	i2c->irq = irq_of_parse_and_map(op->node, 0);
+	if (i2c->irq < 0) {
+		result = -ENXIO;
+		goto fail_irq;
+	}
+	
 	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");
+					  	IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
+			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
 			goto fail_irq;
 		}

 	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:
+fail_add:
 	if (i2c->irq != 0)
 		free_irq(i2c->irq, i2c);
-      fail_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)
 		free_irq(i2c->irq, i2c);
@@ -389,28 +447,44 @@ static int fsl_i2c_remove(struct platform_device *pdev)
 	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

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jean Delvare]

On Tue, 6 Nov 2007 11:26:14 -0700, Grant Likely wrote:
> On 11/6/07, Jean Delvare <khali@linux-fr.org> wrote:
> > Sorry, I've not been completely clear. Yes, you can use
> > i2c_new_device() on an adapter that has been added with
> > i2c_add_adapter(). However, this requires that you have a reference to
> > that i2c_adapter, which is usually not the case with system-wide I2C
> > buses. Embedded platforms would rather use i2c_add_numbered_adapter(),
> > give a list of chips to i2c_register_board_info() and let i2c-core
> > instantiate them. i2c_new_device was primarily meant for multimedia
> > adapters.
> 
> *Some* embedded platforms would rather use i2c_add_numbered_adapter().  :-)
> 
> On powerpc, and other platforms which have a device tree, we don't
> need to define a table of devices in the platform code because we've
> already got a rich structure for describing such things.  The i2c
> busses and i2c devices are grouped together in the device tree, so
> when the i2c bus is probed, it should call out to common i2c device
> tree parsing code to instantiate all the devices described in the
> tree.
> 
> It would be awkward to describe the i2c bus in the device tree but
> still have to use a static structure to describe the devices on that
> bus.

Ah, OK, thanks for the clarification. Then indeed using
i2c_add_adapter() will work fine, agreed.

-- 
Jean Delvare

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Jean Delvare]

Hi Matt,

On Tue, 06 Nov 2007 18:53:11 +0000, Matt Sealey wrote:
> Jean Delvare wrote:
> > On Mon, 05 Nov 2007 21:52:06 +0000, Matt Sealey wrote:
> >> Well, all i2c devices have a chip id you can probe for (...)
> > 
> > This statement is completely incorrect. I2C devices do NOT have
> > standard ID registers. Some devices have proprietary ID registers, some
> > don't, it's really up to the manfacturer.
> 
> All I2C slave devices have to have a 7- or 10-bit address to identify them
> by. They *may* not report what they ARE, but this is 9 times out of
> 10 a hardware design decision of soldering the chip to a board and
> the address is then coded into device trees or hardcoded into drivers.
> 
> Whoever designed the board and has the datasheets knows the address
> they're supposed to be at, and the device can accept this.
> 
> You simply cannot entertain an i2c bus with "anonymous and unnumbered
> devices", every one has to have an address it responds to, however
> it is defined, or it just does not work.

Of course, but it is all about addressing, NOT identifying.

> WRT cell-index this is an index of the bus on the chip (not the logical
> i2c bus but the physical difference between two i2c controllers) and
> then any i2c devices which need to be communicated with would be
> child nodes, their reg property reflecting their slave address, is
> that not correct?

I am not familiar with the OF tree, I can't tell, sorry.

-- 
Jean Delvare

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[Matt Sealey]

Jean Delvare wrote:
> Hi Matt,
>
>> WRT cell-index this is an index of the bus on the chip (not the logical
>> i2c bus but the physical difference between two i2c controllers) and
>> then any i2c devices which need to be communicated with would be
>> child nodes, their reg property reflecting their slave address, is
>> that not correct?
> 
> I am not familiar with the OF tree, I can't tell, sorry.
> 

Well, it's how board designers tell you what chip is at what address :)

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[David Gibson]

On Mon, Nov 05, 2007 at 03:46:45PM -0700, Grant Likely wrote:
> On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > Jon Smirl wrote:
> > > On 11/5/07, Scott Wood <scottwood@freescale.com> wrote:
> > >> Jon Smirl wrote:
> > >>> This is my first pass at reworking the Freescale i2c driver. It
> > >>> switches the driver from being a platform driver to an open firmware
> > >>> one. I've checked it out on my hardware and it is working.
> > >> We may want to hold off on this until arch/ppc goes away (or at least
> > >> all users of this driver in arch/ppc).
> > >
> > > How about renaming the old driver file and leaving it hooked to ppc?
> > > Then it would get deleted when ppc goes away. That would let work
> > > progress on the powerpc version.
> >
> > Or we could have one driver that has two probe methods.  I don't like
> > forking the driver.
> 
> I agree.  This driver can and should have multiple bus bindings.
> 
> > >>>       cell-index = <1>;
> > >> What is cell-index for?
> > >
> > > I was using it to control the bus number, is that the wrong attribute?
> >
> > It shouldn't be specified at all -- the hardware has no concept of a
> > device number.
> 
> cell-index is important.  It describes the hardware, or more
> specifically the layout of the SoC.  The SoC has 2 i2c busses which
> are numbered 0 and 1.  This property should stay for the 5200.
> However, that is the only purpose of it.  cell-index does *not*
> describe the system level bus number.

cell-index should *only* be used if it's used to index into SoC-shared
registers.  It should *never* be used for logical bus or device
numbering as it's being used here.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver

[David Gibson]

On Tue, Nov 06, 2007 at 02:02:12PM -0500, Jon Smirl wrote:
> Second pass at extending i2c core to accept strings of aliases for
> the
[snip]

> -/* With some changes from Kyösti Mälkki <kmalkki@cc.hut.fi>.
> +/* With some changes from Kyösti MÀlkki <kmalkki@cc.hut.fi>.

This looks like an unrelated change of character encoding has slipped
in here.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson