[PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Jon Smirl]

Convert i2c-mpc to an of_platform driver. Utilize the code in drivers/of-i2c.c to make i2c modules dynamically loadable by the device tree.

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

 arch/powerpc/sysdev/fsl_soc.c |  122 -----------------------------------------
 drivers/i2c/busses/i2c-mpc.c  |  104 ++++++++++++++++++++---------------
 2 files changed, 60 insertions(+), 166 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3a7054e..ebcec73 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -414,128 +414,6 @@ err:
 
 arch_initcall(gfar_of_init);
 
-#ifdef CONFIG_I2C_BOARDINFO
-#include <linux/i2c.h>
-struct i2c_driver_device {
-	char	*of_device;
-	char	*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] __initdata = {
-	{"ricoh,rs5c372a", "rs5c372a"},
-	{"ricoh,rs5c372b", "rs5c372b"},
-	{"ricoh,rv5c386",  "rv5c386"},
-	{"ricoh,rv5c387a", "rv5c387a"},
-	{"dallas,ds1307",  "ds1307"},
-	{"dallas,ds1337",  "ds1337"},
-	{"dallas,ds1338",  "ds1338"},
-	{"dallas,ds1339",  "ds1339"},
-	{"dallas,ds1340",  "ds1340"},
-	{"stm,m41t00",     "m41t00"},
-	{"dallas,ds1374",  "rtc-ds1374"},
-};
-
-static int __init of_find_i2c_driver(struct device_node *node,
-				     struct i2c_board_info *info)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
-		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
-			continue;
-		if (strlcpy(info->type, i2c_devices[i].i2c_type,
-			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
-			return -ENOMEM;
-		return 0;
-	}
-	return -ENODEV;
-}
-
-static void __init of_register_i2c_devices(struct device_node *adap_node,
-					   int bus_num)
-{
-	struct device_node *node = NULL;
-
-	while ((node = of_get_next_child(adap_node, node))) {
-		struct i2c_board_info info = {};
-		const u32 *addr;
-		int len;
-
-		addr = of_get_property(node, "reg", &len);
-		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
-			printk(KERN_WARNING "fsl_soc.c: invalid i2c device entry\n");
-			continue;
-		}
-
-		info.irq = irq_of_parse_and_map(node, 0);
-		if (info.irq == NO_IRQ)
-			info.irq = -1;
-
-		if (of_find_i2c_driver(node, &info) < 0)
-			continue;
-
-		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 = 0;
-	struct platform_device *i2c_dev;
-	int ret;
-
-	for_each_compatible_node(np, NULL, "fsl-i2c") {
-		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 a076129..4fdfb62 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -17,7 +17,8 @@
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/init.h>
-#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_i2c.h>
 
 #include <asm/io.h>
 #include <linux/fsl_devices.h>
@@ -25,13 +26,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
@@ -315,102 +316,117 @@ static struct i2c_adapter mpc_ops = {
 	.timeout = 1,
 };
 
-static int fsl_i2c_probe(struct platform_device *pdev)
+static int __devinit fsl_i2c_probe(struct of_device *op, const struct of_device_id *match)
 {
 	int result = 0;
 	struct mpc_i2c *i2c;
-	struct fsl_i2c_platform_data *pdata;
-	struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
 
 	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
-	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0)
-		i2c->irq = NO_IRQ; /* Use polling */
+	if (of_get_property(op->node, "dfsrr", NULL))
+		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
 
-	i2c->flags = pdata->device_flags;
-	init_waitqueue_head(&i2c->queue);
+	if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
+			of_device_is_compatible(op->node, "mpc5200-i2c"))
+		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
 
-	i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
+	init_waitqueue_head(&i2c->queue);
 
+	i2c->base = of_iomap(op->node, 0);
 	if (!i2c->base) {
 		printk(KERN_ERR "i2c-mpc - failed to map controller\n");
 		result = -ENOMEM;
 		goto fail_map;
 	}
 
-	if (i2c->irq != NO_IRQ)
-		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
-					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
-			printk(KERN_ERR
-			       "i2c-mpc - failed to attach interrupt\n");
-			goto fail_irq;
+	i2c->irq = irq_of_parse_and_map(op->node, 0);
+	if (i2c->irq != NO_IRQ) { /* i2c->irq = NO_IRQ implies polling */
+		result = request_irq(i2c->irq, mpc_i2c_isr,
+				     IRQF_SHARED, "i2c-mpc", i2c);
+		if (result < 0) {
+			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
+			goto fail_request;
 		}
-
+	}
+	
 	mpc_i2c_setclock(i2c);
-	platform_set_drvdata(pdev, i2c);
+
+	dev_set_drvdata(&op->dev, i2c);
 
 	i2c->adap = mpc_ops;
-	i2c->adap.nr = pdev->id;
 	i2c_set_adapdata(&i2c->adap, i2c);
-	i2c->adap.dev.parent = &pdev->dev;
-	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
+	i2c->adap.dev.parent = &op->dev;
+
+	result = i2c_add_adapter(&i2c->adap);
+	if (result < 0) {
 		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
 		goto fail_add;
 	}
+	of_register_i2c_devices(&i2c->adap, op->node);
 
 	return result;
 
-      fail_add:
-	if (i2c->irq != NO_IRQ)
-		free_irq(i2c->irq, i2c);
-      fail_irq:
-	iounmap(i2c->base);
-      fail_map:
+ fail_add:
+	dev_set_drvdata(&op->dev, NULL);
+	free_irq(i2c->irq, i2c);
+ fail_request:
+	irq_dispose_mapping(i2c->irq);
+ 	iounmap(i2c->base);
+ fail_map:
 	kfree(i2c);
 	return result;
 };
 
-static int fsl_i2c_remove(struct platform_device *pdev)
+static int __devexit fsl_i2c_remove(struct of_device *op)
 {
-	struct mpc_i2c *i2c = platform_get_drvdata(pdev);
+	struct mpc_i2c *i2c = dev_get_drvdata(&op->dev);
 
 	i2c_del_adapter(&i2c->adap);
-	platform_set_drvdata(pdev, NULL);
+	dev_set_drvdata(&op->dev, NULL);
 
 	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
 
+	irq_dispose_mapping(i2c->irq);
 	iounmap(i2c->base);
 	kfree(i2c);
 	return 0;
 };
 
-/* work with hotplug and coldplug */
-MODULE_ALIAS("platform:fsl-i2c");
+static const 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		= fsl_i2c_probe,
+	.remove		= __devexit_p(fsl_i2c_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= DRV_NAME,
 	},
 };
 
 static int __init fsl_i2c_init(void)
 {
-	return platform_driver_register(&fsl_i2c_driver);
+	int rv;
+
+	rv = of_register_platform_driver(&mpc_i2c_driver);
+	if (rv)
+		printk(KERN_ERR DRV_NAME 
+		       " of_register_platform_driver failed (%i)\n", rv);
+	return rv;
 }
 
 static void __exit fsl_i2c_exit(void)
 {
-	platform_driver_unregister(&fsl_i2c_driver);
+	of_unregister_platform_driver(&mpc_i2c_driver);
 }
 
 module_init(fsl_i2c_init);

[PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

Add the of_find_i2c_device_by_node function. This allows you to follow a reference in the device to an i2c device node and then locate the linux device instantiated by the device tree. Example use, an i2s codec controlled by i2c.
---

 drivers/i2c/i2c-core.c |    2 +-
 drivers/of/of_i2c.c    |   37 ++++++++++++++++++++++++++-----------
 include/linux/i2c.h    |    3 +++
 include/linux/of_i2c.h |    2 ++
 4 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d0175f4..e3abe1b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -208,7 +208,7 @@ static struct device_attribute i2c_dev_attrs[] = {
 	{ },
 };
 
-static struct bus_type i2c_bus_type = {
+struct bus_type i2c_bus_type = {
 	.name		= "i2c",
 	.dev_attrs	= i2c_dev_attrs,
 	.match		= i2c_device_match,
diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 715a444..ca69a16 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -75,7 +75,7 @@ static int of_find_i2c_driver(struct device_node *node,
 void of_register_i2c_devices(struct i2c_adapter *adap,
 			     struct device_node *adap_node)
 {
-	void *result;
+	struct i2c_client *i2c_dev;
 	struct device_node *node;
 
 	for_each_child_of_node(adap_node, node) {
@@ -90,29 +90,44 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 			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) {
-			irq_dispose_mapping(info.irq);
+		if (of_find_i2c_driver(node, &info) < 0)
 			continue;
-		}
 
+		info.irq = irq_of_parse_and_map(node, 0);
 		info.addr = *addr;
 
-		request_module(info.type);
+		request_module("%s", info.type);
 
-		result = i2c_new_device(adap, &info);
-		if (result == NULL) {
+		i2c_dev = i2c_new_device(adap, &info);
+		if (i2c_dev == NULL) {
 			printk(KERN_ERR
 			       "of-i2c: Failed to load driver for %s\n",
 			       info.type);
 			irq_dispose_mapping(info.irq);
 			continue;
 		}
+		
+		i2c_dev->dev.archdata.of_node = of_node_get(node);
 	}
 }
 EXPORT_SYMBOL(of_register_i2c_devices);
 
+static int of_dev_node_match(struct device *dev, void *data)
+{
+        return dev->archdata.of_node == data;
+}
+
+struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
+{
+	struct device *dev;
+	
+	dev = bus_find_device(&i2c_bus_type, NULL, node, 
+					 of_dev_node_match);
+	if (!dev)
+		return NULL;
+		
+	return to_i2c_client(dev);
+}
+EXPORT_SYMBOL(of_find_i2c_device_by_node);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fb9af6a..186b22d 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -92,6 +92,9 @@ extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client,
 extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client,
 					  u8 command, u8 length,
 					  const u8 *values);
+					  
+/* Base of the i2c bus */
+extern struct bus_type i2c_bus_type; 
 
 /*
  * A driver is capable of handling one or more physical devices present on
diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
index bd2a870..17d5897 100644
--- a/include/linux/of_i2c.h
+++ b/include/linux/of_i2c.h
@@ -16,5 +16,7 @@
 
 void of_register_i2c_devices(struct i2c_adapter *adap,
 			     struct device_node *adap_node);
+struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
+			     
 
 #endif /* __LINUX_OF_I2C_H */

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

Hi Jon,

On Mon, 30 Jun 2008 19:01:28 -0400, Jon Smirl wrote:
> Add the of_find_i2c_device_by_node function. This allows you to
> follow a reference in the device to an i2c device node and then
> locate the linux device instantiated by the device tree. Example
> use, an i2s codec controlled by i2c.
> ---
> 
>  drivers/i2c/i2c-core.c |    2 +-
>  drivers/of/of_i2c.c    |   37 ++++++++++++++++++++++++++-----------
>  include/linux/i2c.h    |    3 +++
>  include/linux/of_i2c.h |    2 ++
>  4 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d0175f4..e3abe1b 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -208,7 +208,7 @@ static struct device_attribute i2c_dev_attrs[] = {
>  	{ },
>  };
>  
> -static struct bus_type i2c_bus_type = {
> +struct bus_type i2c_bus_type = {
>  	.name		= "i2c",
>  	.dev_attrs	= i2c_dev_attrs,
>  	.match		= i2c_device_match,

What if i2c-core is built as a module? Don't you need to export the
symbol then?

> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index 715a444..ca69a16 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -75,7 +75,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  void of_register_i2c_devices(struct i2c_adapter *adap,
>  			     struct device_node *adap_node)
>  {
> -	void *result;
> +	struct i2c_client *i2c_dev;
>  	struct device_node *node;
>  
>  	for_each_child_of_node(adap_node, node) {
> @@ -90,29 +90,44 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>  			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) {
> -			irq_dispose_mapping(info.irq);
> +		if (of_find_i2c_driver(node, &info) < 0)
>  			continue;
> -		}
>  
> +		info.irq = irq_of_parse_and_map(node, 0);
>  		info.addr = *addr;
>  
> -		request_module(info.type);
> +		request_module("%s", info.type);

A separate fix for this issue was already sent by Maximilian Attems a
few days go:
http://lists.lm-sensors.org/pipermail/i2c/2008-June/004030.html

>  
> -		result = i2c_new_device(adap, &info);
> -		if (result == NULL) {
> +		i2c_dev = i2c_new_device(adap, &info);
> +		if (i2c_dev == NULL) {
>  			printk(KERN_ERR
>  			       "of-i2c: Failed to load driver for %s\n",
>  			       info.type);
>  			irq_dispose_mapping(info.irq);
>  			continue;
>  		}
> +		
> +		i2c_dev->dev.archdata.of_node = of_node_get(node);
>  	}
>  }
>  EXPORT_SYMBOL(of_register_i2c_devices);
>  
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +        return dev->archdata.of_node == data;
> +}
> +
> +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> +{
> +	struct device *dev;
> +	
> +	dev = bus_find_device(&i2c_bus_type, NULL, node, 
> +					 of_dev_node_match);
> +	if (!dev)
> +		return NULL;
> +		
> +	return to_i2c_client(dev);
> +}
> +EXPORT_SYMBOL(of_find_i2c_device_by_node);
> +
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index fb9af6a..186b22d 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -92,6 +92,9 @@ extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client,
>  extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client,
>  					  u8 command, u8 length,
>  					  const u8 *values);
> +					  
> +/* Base of the i2c bus */
> +extern struct bus_type i2c_bus_type; 
>  
>  /*
>   * A driver is capable of handling one or more physical devices present on
> diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
> index bd2a870..17d5897 100644
> --- a/include/linux/of_i2c.h
> +++ b/include/linux/of_i2c.h
> @@ -16,5 +16,7 @@
>  
>  void of_register_i2c_devices(struct i2c_adapter *adap,
>  			     struct device_node *adap_node);
> +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
> +			     
>  
>  #endif /* __LINUX_OF_I2C_H */

I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
It was un-exported only because it had no user left, but it can be
exported again if needed.

I'm not the one to push this upstream though, as the patch is
essentially an openfirmware patch. That would be something for Jochen
Friedrich and Paul Mackerras I guess. Would be nice to have a
MAINTAINERS entry for OF...

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
>  It was un-exported only because it had no user left, but it can be
>  exported again if needed.

Another solution would be to move drivers/of/of_i2c into the i2c
directory and make it part of i2c core on powerpc builds.

>  I'm not the one to push this upstream though, as the patch is
>  essentially an openfirmware patch. That would be something for Jochen
>  Friedrich and Paul Mackerras I guess. Would be nice to have a
>  MAINTAINERS entry for OF...
>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jochen Friedrich]

Hi Jean,

> I'm not the one to push this upstream though, as the patch is
> essentially an openfirmware patch. That would be something for Jochen
> Friedrich and Paul Mackerras I guess. Would be nice to have a
> MAINTAINERS entry for OF...

Nope. I only did a small contribution to the OF stuff. I guess,
Grant Likely or Stephen Rothwell are the defacto maintainers for OF.

Thanks,
Jochen

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Jean Delvare]

Hi Jon,

On Mon, 30 Jun 2008 19:01:26 -0400, Jon Smirl wrote:
> Convert i2c-mpc to an of_platform driver. Utilize the code in drivers/of-i2c.c
> to make i2c modules dynamically loadable by the device tree.
> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
> 
>  arch/powerpc/sysdev/fsl_soc.c |  122 -----------------------------------------
>  drivers/i2c/busses/i2c-mpc.c  |  104 ++++++++++++++++++++---------------
>  2 files changed, 60 insertions(+), 166 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 3a7054e..ebcec73 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -414,128 +414,6 @@ err:
>  
>  arch_initcall(gfar_of_init);
>  
> -#ifdef CONFIG_I2C_BOARDINFO
> -#include <linux/i2c.h>
> -struct i2c_driver_device {
> -	char	*of_device;
> -	char	*i2c_type;
> -};
> -
> -static struct i2c_driver_device i2c_devices[] __initdata = {
> -	{"ricoh,rs5c372a", "rs5c372a"},
> -	{"ricoh,rs5c372b", "rs5c372b"},
> -	{"ricoh,rv5c386",  "rv5c386"},
> -	{"ricoh,rv5c387a", "rv5c387a"},
> -	{"dallas,ds1307",  "ds1307"},
> -	{"dallas,ds1337",  "ds1337"},
> -	{"dallas,ds1338",  "ds1338"},
> -	{"dallas,ds1339",  "ds1339"},
> -	{"dallas,ds1340",  "ds1340"},
> -	{"stm,m41t00",     "m41t00"},
> -	{"dallas,ds1374",  "rtc-ds1374"},
> -};
> -
> -static int __init of_find_i2c_driver(struct device_node *node,
> -				     struct i2c_board_info *info)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> -		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
> -			continue;
> -		if (strlcpy(info->type, i2c_devices[i].i2c_type,
> -			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
> -			return -ENOMEM;
> -		return 0;
> -	}
> -	return -ENODEV;
> -}
> -
> -static void __init of_register_i2c_devices(struct device_node *adap_node,
> -					   int bus_num)
> -{
> -	struct device_node *node = NULL;
> -
> -	while ((node = of_get_next_child(adap_node, node))) {
> -		struct i2c_board_info info = {};
> -		const u32 *addr;
> -		int len;
> -
> -		addr = of_get_property(node, "reg", &len);
> -		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
> -			printk(KERN_WARNING "fsl_soc.c: invalid i2c device entry\n");
> -			continue;
> -		}
> -
> -		info.irq = irq_of_parse_and_map(node, 0);
> -		if (info.irq == NO_IRQ)
> -			info.irq = -1;
> -
> -		if (of_find_i2c_driver(node, &info) < 0)
> -			continue;
> -
> -		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 = 0;
> -	struct platform_device *i2c_dev;
> -	int ret;
> -
> -	for_each_compatible_node(np, NULL, "fsl-i2c") {
> -		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 a076129..4fdfb62 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -17,7 +17,8 @@
>  #include <linux/module.h>
>  #include <linux/sched.h>
>  #include <linux/init.h>
> -#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_i2c.h>
>  
>  #include <asm/io.h>
>  #include <linux/fsl_devices.h>
> @@ -25,13 +26,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
> @@ -315,102 +316,117 @@ static struct i2c_adapter mpc_ops = {
>  	.timeout = 1,
>  };
>  
> -static int fsl_i2c_probe(struct platform_device *pdev)
> +static int __devinit fsl_i2c_probe(struct of_device *op, const struct of_device_id *match)
>  {
>  	int result = 0;
>  	struct mpc_i2c *i2c;
> -	struct fsl_i2c_platform_data *pdata;
> -	struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -	pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
>  
>  	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>  	if (!i2c)
>  		return -ENOMEM;
>  
> -	i2c->irq = platform_get_irq(pdev, 0);
> -	if (i2c->irq < 0)
> -		i2c->irq = NO_IRQ; /* Use polling */
> +	if (of_get_property(op->node, "dfsrr", NULL))
> +		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
>  
> -	i2c->flags = pdata->device_flags;
> -	init_waitqueue_head(&i2c->queue);
> +	if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
> +			of_device_is_compatible(op->node, "mpc5200-i2c"))
> +		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
>  
> -	i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
> +	init_waitqueue_head(&i2c->queue);
>  
> +	i2c->base = of_iomap(op->node, 0);
>  	if (!i2c->base) {
>  		printk(KERN_ERR "i2c-mpc - failed to map controller\n");
>  		result = -ENOMEM;
>  		goto fail_map;
>  	}
>  
> -	if (i2c->irq != NO_IRQ)
> -		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> -					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> -			printk(KERN_ERR
> -			       "i2c-mpc - failed to attach interrupt\n");
> -			goto fail_irq;
> +	i2c->irq = irq_of_parse_and_map(op->node, 0);
> +	if (i2c->irq != NO_IRQ) { /* i2c->irq = NO_IRQ implies polling */
> +		result = request_irq(i2c->irq, mpc_i2c_isr,
> +				     IRQF_SHARED, "i2c-mpc", i2c);
> +		if (result < 0) {
> +			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
> +			goto fail_request;
>  		}
> -
> +	}
> +	
>  	mpc_i2c_setclock(i2c);
> -	platform_set_drvdata(pdev, i2c);
> +
> +	dev_set_drvdata(&op->dev, i2c);
>  
>  	i2c->adap = mpc_ops;
> -	i2c->adap.nr = pdev->id;
>  	i2c_set_adapdata(&i2c->adap, i2c);
> -	i2c->adap.dev.parent = &pdev->dev;
> -	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> +	i2c->adap.dev.parent = &op->dev;
> +
> +	result = i2c_add_adapter(&i2c->adap);
> +	if (result < 0) {
>  		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
>  		goto fail_add;
>  	}
> +	of_register_i2c_devices(&i2c->adap, op->node);
>  
>  	return result;
>  
> -      fail_add:
> -	if (i2c->irq != NO_IRQ)
> -		free_irq(i2c->irq, i2c);
> -      fail_irq:
> -	iounmap(i2c->base);
> -      fail_map:
> + fail_add:
> +	dev_set_drvdata(&op->dev, NULL);
> +	free_irq(i2c->irq, i2c);
> + fail_request:
> +	irq_dispose_mapping(i2c->irq);
> + 	iounmap(i2c->base);
> + fail_map:
>  	kfree(i2c);
>  	return result;
>  };
>  
> -static int fsl_i2c_remove(struct platform_device *pdev)
> +static int __devexit fsl_i2c_remove(struct of_device *op)
>  {
> -	struct mpc_i2c *i2c = platform_get_drvdata(pdev);
> +	struct mpc_i2c *i2c = dev_get_drvdata(&op->dev);
>  
>  	i2c_del_adapter(&i2c->adap);
> -	platform_set_drvdata(pdev, NULL);
> +	dev_set_drvdata(&op->dev, NULL);
>  
>  	if (i2c->irq != NO_IRQ)
>  		free_irq(i2c->irq, i2c);
>  
> +	irq_dispose_mapping(i2c->irq);
>  	iounmap(i2c->base);
>  	kfree(i2c);
>  	return 0;
>  };
>  
> -/* work with hotplug and coldplug */
> -MODULE_ALIAS("platform:fsl-i2c");
> +static const 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		= fsl_i2c_probe,
> +	.remove		= __devexit_p(fsl_i2c_remove),
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= DRV_NAME,
>  	},
>  };
>  
>  static int __init fsl_i2c_init(void)
>  {
> -	return platform_driver_register(&fsl_i2c_driver);
> +	int rv;
> +
> +	rv = of_register_platform_driver(&mpc_i2c_driver);
> +	if (rv)
> +		printk(KERN_ERR DRV_NAME 
> +		       " of_register_platform_driver failed (%i)\n", rv);
> +	return rv;
>  }
>  
>  static void __exit fsl_i2c_exit(void)
>  {
> -	platform_driver_unregister(&fsl_i2c_driver);
> +	of_unregister_platform_driver(&mpc_i2c_driver);
>  }
>  
>  module_init(fsl_i2c_init);

Applied, after fixing the patch so that it applies, fixing it again so
that it is correct in the polling case, and fixing it again to make it
pass checkpatch.pl.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Jon,
>
>
>  On Mon, 30 Jun 2008 19:01:28 -0400, Jon Smirl wrote:
>  > Add the of_find_i2c_device_by_node function. This allows you to
>  > follow a reference in the device to an i2c device node and then
>  > locate the linux device instantiated by the device tree. Example
>  > use, an i2s codec controlled by i2c.
>  > ---
>  >
>  >  drivers/i2c/i2c-core.c |    2 +-
>  >  drivers/of/of_i2c.c    |   37 ++++++++++++++++++++++++++-----------
>  >  include/linux/i2c.h    |    3 +++
>  >  include/linux/of_i2c.h |    2 ++
>  >  4 files changed, 32 insertions(+), 12 deletions(-)
>  >
>  > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>  > index d0175f4..e3abe1b 100644
>  > --- a/drivers/i2c/i2c-core.c
>  > +++ b/drivers/i2c/i2c-core.c
>  > @@ -208,7 +208,7 @@ static struct device_attribute i2c_dev_attrs[] = {
>  >       { },
>  >  };
>  >
>  > -static struct bus_type i2c_bus_type = {
>  > +struct bus_type i2c_bus_type = {
>  >       .name           = "i2c",
>  >       .dev_attrs      = i2c_dev_attrs,
>  >       .match          = i2c_device_match,
>
>
> What if i2c-core is built as a module? Don't you need to export the
>  symbol then?

Jean, can you re-export i2c_bus_type and then I'll drop that part from
the patch? That way the patch won't hit multiple maintainers.

>  > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>  > index 715a444..ca69a16 100644
>  > --- a/drivers/of/of_i2c.c
>  > +++ b/drivers/of/of_i2c.c
>  > @@ -75,7 +75,7 @@ static int of_find_i2c_driver(struct device_node *node,
>  >  void of_register_i2c_devices(struct i2c_adapter *adap,
>  >                            struct device_node *adap_node)
>  >  {
>  > -     void *result;
>  > +     struct i2c_client *i2c_dev;
>  >       struct device_node *node;
>  >
>  >       for_each_child_of_node(adap_node, node) {
>  > @@ -90,29 +90,44 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>  >                       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) {
>  > -                     irq_dispose_mapping(info.irq);
>  > +             if (of_find_i2c_driver(node, &info) < 0)
>  >                       continue;
>  > -             }
>  >
>  > +             info.irq = irq_of_parse_and_map(node, 0);
>  >               info.addr = *addr;
>  >
>  > -             request_module(info.type);
>  > +             request_module("%s", info.type);
>
>
> A separate fix for this issue was already sent by Maximilian Attems a
>  few days go:
>  http://lists.lm-sensors.org/pipermail/i2c/2008-June/004030.html
>
>
>  >
>  > -             result = i2c_new_device(adap, &info);
>  > -             if (result == NULL) {
>  > +             i2c_dev = i2c_new_device(adap, &info);
>  > +             if (i2c_dev == NULL) {
>  >                       printk(KERN_ERR
>  >                              "of-i2c: Failed to load driver for %s\n",
>  >                              info.type);
>  >                       irq_dispose_mapping(info.irq);
>  >                       continue;
>  >               }
>  > +
>  > +             i2c_dev->dev.archdata.of_node = of_node_get(node);
>  >       }
>  >  }
>  >  EXPORT_SYMBOL(of_register_i2c_devices);
>  >
>  > +static int of_dev_node_match(struct device *dev, void *data)
>  > +{
>  > +        return dev->archdata.of_node == data;
>  > +}
>  > +
>  > +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
>  > +{
>  > +     struct device *dev;
>  > +
>  > +     dev = bus_find_device(&i2c_bus_type, NULL, node,
>  > +                                      of_dev_node_match);
>  > +     if (!dev)
>  > +             return NULL;
>  > +
>  > +     return to_i2c_client(dev);
>  > +}
>  > +EXPORT_SYMBOL(of_find_i2c_device_by_node);
>  > +
>  >  MODULE_LICENSE("GPL");
>  > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>  > index fb9af6a..186b22d 100644
>  > --- a/include/linux/i2c.h
>  > +++ b/include/linux/i2c.h
>  > @@ -92,6 +92,9 @@ extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client,
>  >  extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client,
>  >                                         u8 command, u8 length,
>  >                                         const u8 *values);
>  > +
>  > +/* Base of the i2c bus */
>  > +extern struct bus_type i2c_bus_type;
>  >
>  >  /*
>  >   * A driver is capable of handling one or more physical devices present on
>  > diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
>  > index bd2a870..17d5897 100644
>  > --- a/include/linux/of_i2c.h
>  > +++ b/include/linux/of_i2c.h
>  > @@ -16,5 +16,7 @@
>  >
>  >  void of_register_i2c_devices(struct i2c_adapter *adap,
>  >                            struct device_node *adap_node);
>  > +struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
>  > +
>  >
>  >  #endif /* __LINUX_OF_I2C_H */
>
>
> I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
>  It was un-exported only because it had no user left, but it can be
>  exported again if needed.
>
>  I'm not the one to push this upstream though, as the patch is
>  essentially an openfirmware patch. That would be something for Jochen
>  Friedrich and Paul Mackerras I guess. Would be nice to have a
>  MAINTAINERS entry for OF...
>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
> >  It was un-exported only because it had no user left, but it can be
> >  exported again if needed.
> 
> Another solution would be to move drivers/of/of_i2c into the i2c
> directory and make it part of i2c core on powerpc builds.

I don't think this is a good idea. Merging arch-specific code (or
half-arch-specific code in this case) into arch-neutral drivers ends up
being a pain to maintain. People will keep sending me patches for stuff
I don't know anything about and can't help with. Having of-specific
stuff in just one directory as is the case now sounds much better to
me. All it's missing is a MAINTAINERS entry.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 12:29:29 -0400, Jon Smirl wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Jon,
> >
> >
> >  On Mon, 30 Jun 2008 19:01:28 -0400, Jon Smirl wrote:
> >  > Add the of_find_i2c_device_by_node function. This allows you to
> >  > follow a reference in the device to an i2c device node and then
> >  > locate the linux device instantiated by the device tree. Example
> >  > use, an i2s codec controlled by i2c.
> >  > ---
> >  >
> >  >  drivers/i2c/i2c-core.c |    2 +-
> >  >  drivers/of/of_i2c.c    |   37 ++++++++++++++++++++++++++-----------
> >  >  include/linux/i2c.h    |    3 +++
> >  >  include/linux/of_i2c.h |    2 ++
> >  >  4 files changed, 32 insertions(+), 12 deletions(-)
> >  >
> >  > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >  > index d0175f4..e3abe1b 100644
> >  > --- a/drivers/i2c/i2c-core.c
> >  > +++ b/drivers/i2c/i2c-core.c
> >  > @@ -208,7 +208,7 @@ static struct device_attribute i2c_dev_attrs[] = {
> >  >       { },
> >  >  };
> >  >
> >  > -static struct bus_type i2c_bus_type = {
> >  > +struct bus_type i2c_bus_type = {
> >  >       .name           = "i2c",
> >  >       .dev_attrs      = i2c_dev_attrs,
> >  >       .match          = i2c_device_match,
> >
> >
> > What if i2c-core is built as a module? Don't you need to export the
> >  symbol then?
> 
> Jean, can you re-export i2c_bus_type and then I'll drop that part from
> the patch? That way the patch won't hit multiple maintainers.

Just send me a patch doing just that and I will be glad to push early in
the 2.6.27 merge window.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote:
>  > On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
>  > > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
>  > >  It was un-exported only because it had no user left, but it can be
>  > >  exported again if needed.
>  >
>  > Another solution would be to move drivers/of/of_i2c into the i2c
>  > directory and make it part of i2c core on powerpc builds.
>
>
> I don't think this is a good idea. Merging arch-specific code (or
>  half-arch-specific code in this case) into arch-neutral drivers ends up
>  being a pain to maintain. People will keep sending me patches for stuff
>  I don't know anything about and can't help with. Having of-specific
>  stuff in just one directory as is the case now sounds much better to
>  me. All it's missing is a MAINTAINERS entry.

A side effect of this is that the small pieces of code in drivers/of
have to be compiled into stand alone modules and they may need access
to internal symbols from the subsystem. If they were directly linked
into the subsystems you wouldn't need to make the internal symbols
visible. Now the subsystems have to be careful about breaking the
in-kernel, external users of the symbols and we've made it possible
for out of tree drivers to get to internal structures.




>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Grant Likely]

On Tue, Jul 01, 2008 at 11:12:58AM -0400, Jon Smirl wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
> >  It was un-exported only because it had no user left, but it can be
> >  exported again if needed.
> 
> Another solution would be to move drivers/of/of_i2c into the i2c
> directory and make it part of i2c core on powerpc builds.

My preference is for things like of_spi and of_i2c to go with the
related busses; I think it makes more sense to keep all the I2C stuff
together, but I've already lost that battle once.

g.

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 12:38:05 -0400, Jon Smirl wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote:
> >  > On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> >  > > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
> >  > >  It was un-exported only because it had no user left, but it can be
> >  > >  exported again if needed.
> >  >
> >  > Another solution would be to move drivers/of/of_i2c into the i2c
> >  > directory and make it part of i2c core on powerpc builds.
> >
> > I don't think this is a good idea. Merging arch-specific code (or
> >  half-arch-specific code in this case) into arch-neutral drivers ends up
> >  being a pain to maintain. People will keep sending me patches for stuff
> >  I don't know anything about and can't help with. Having of-specific
> >  stuff in just one directory as is the case now sounds much better to
> >  me. All it's missing is a MAINTAINERS entry.
> 
> A side effect of this is that the small pieces of code in drivers/of
> have to be compiled into stand alone modules and they may need access
> to internal symbols from the subsystem. If they were directly linked
> into the subsystems you wouldn't need to make the internal symbols
> visible.

Do you think you'll need something else than i2c_bus_type? That I don't
consider an i2c-core internal. As I said, the lack of export was simply
due to a lack of user.

> Now the subsystems have to be careful about breaking the
> in-kernel, external users of the symbols

Same holds if the code is merged into i2c-core.

> and we've made it possible for out of tree drivers to get to internal
> structures.

Hopefully the namespaced exports which some kernel developers are
working on, will become a reality soon, to mitigate that kind of issue.
That being said...

As an upstream kernel developer / maintainer, I don't care about that.
If external modules make use of internals they shouldn't touch, and
later changes cause them to break, that's none of my business. I leave
this to distribution maintainers.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Grant Likely]

On Tue, Jul 01, 2008 at 06:29:49PM +0200, Jean Delvare wrote:
> On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote:
> > On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
> > >  It was un-exported only because it had no user left, but it can be
> > >  exported again if needed.
> > 
> > Another solution would be to move drivers/of/of_i2c into the i2c
> > directory and make it part of i2c core on powerpc builds.
> 
> I don't think this is a good idea. Merging arch-specific code (or
> half-arch-specific code in this case) into arch-neutral drivers ends up
> being a pain to maintain. People will keep sending me patches for stuff
> I don't know anything about and can't help with. Having of-specific
> stuff in just one directory as is the case now sounds much better to
> me. All it's missing is a MAINTAINERS entry.

But the other side of the coin is that each driver must have
driver-specific OF code to translate data in the device tree to data
usable by the driver.  It doesn't make any sense to me for that stuff to
live anywhere other that with the driver that it supports.

g.

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Jul 01, 2008 at 11:12:58AM -0400, Jon Smirl wrote:
>  > On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
>  > > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
>  > >  It was un-exported only because it had no user left, but it can be
>  > >  exported again if needed.
>  >
>  > Another solution would be to move drivers/of/of_i2c into the i2c
>  > directory and make it part of i2c core on powerpc builds.
>
>
> My preference is for things like of_spi and of_i2c to go with the
>  related busses; I think it makes more sense to keep all the I2C stuff
>  together, but I've already lost that battle once.
>

This is a similar problem to adding aliases to the i2c driver drivers
for the device tree names of the i2c devices. Instead we have code in
drivers/of/of_i2c.c that tries to guess the translation from device
tree to linux names. Adding aliases to the drivers would eliminate the
need for of_find_i2c_driver().

I've previously posted patches implementing device tree names in the
drivers that used ifdef to only instantiate on powerpc builds. For
example....

diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
index e07274d..9cd1770 100644
--- a/drivers/i2c/chips/tps65010.c
+++ b/drivers/i2c/chips/tps65010.c
@@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = {
        { "tps65011", TPS65011 },
        { "tps65012", TPS65012 },
        { "tps65013", TPS65013 },
+       OF_ID("ti,tps65010", TPS65010)
+       OF_ID("ti,tps65011", TPS65011)
+       OF_ID("ti,tps65012", TPS65012)
+       OF_ID("ti,tps65013", TPS65013)
        { },
 };
 MODULE_DEVICE_TABLE(i2c, tps65010_id);


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 10:45:18 -0600, Grant Likely wrote:
> On Tue, Jul 01, 2008 at 06:29:49PM +0200, Jean Delvare wrote:
> > On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote:
> > > On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > > > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
> > > >  It was un-exported only because it had no user left, but it can be
> > > >  exported again if needed.
> > > 
> > > Another solution would be to move drivers/of/of_i2c into the i2c
> > > directory and make it part of i2c core on powerpc builds.
> > 
> > I don't think this is a good idea. Merging arch-specific code (or
> > half-arch-specific code in this case) into arch-neutral drivers ends up
> > being a pain to maintain. People will keep sending me patches for stuff
> > I don't know anything about and can't help with. Having of-specific
> > stuff in just one directory as is the case now sounds much better to
> > me. All it's missing is a MAINTAINERS entry.
> 
> But the other side of the coin is that each driver must have
> driver-specific OF code to translate data in the device tree to data
> usable by the driver.  It doesn't make any sense to me for that stuff to
> live anywhere other that with the driver that it supports.

This code is glue between OF and subsystems. As with any glue code, you
can argue forever on which side you want to push the code to. Both
answers are valid.

All I see on my personal side is that I don't have any system using OF
and no knowledge about it either, so I can't maintain of_i2c. So having
that file in drivers/of rather than drivers/i2c will make my life
easier for sure. While I'd guess that most (all?) OF-based systems have
an I2C bus, so whoever is responsible for drivers/of should be able to
maintain of_i2c.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 1 Jul 2008 10:45:18 -0600, Grant Likely wrote:
>  > On Tue, Jul 01, 2008 at 06:29:49PM +0200, Jean Delvare wrote:
>  > > On Tue, 1 Jul 2008 11:12:58 -0400, Jon Smirl wrote:
>  > > > On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
>  > > > > I'm fine with this patch. In particular, exporting i2c_bus_type is OK.
>  > > > >  It was un-exported only because it had no user left, but it can be
>  > > > >  exported again if needed.
>  > > >
>  > > > Another solution would be to move drivers/of/of_i2c into the i2c
>  > > > directory and make it part of i2c core on powerpc builds.
>  > >
>  > > I don't think this is a good idea. Merging arch-specific code (or
>  > > half-arch-specific code in this case) into arch-neutral drivers ends up
>  > > being a pain to maintain. People will keep sending me patches for stuff
>  > > I don't know anything about and can't help with. Having of-specific
>  > > stuff in just one directory as is the case now sounds much better to
>  > > me. All it's missing is a MAINTAINERS entry.
>  >
>  > But the other side of the coin is that each driver must have
>  > driver-specific OF code to translate data in the device tree to data
>  > usable by the driver.  It doesn't make any sense to me for that stuff to
>  > live anywhere other that with the driver that it supports.
>
>
> This code is glue between OF and subsystems. As with any glue code, you
>  can argue forever on which side you want to push the code to. Both
>  answers are valid.
>
>  All I see on my personal side is that I don't have any system using OF
>  and no knowledge about it either, so I can't maintain of_i2c. So having
>  that file in drivers/of rather than drivers/i2c will make my life
>  easier for sure. While I'd guess that most (all?) OF-based systems have
>  an I2C bus, so whoever is responsible for drivers/of should be able to
>  maintain of_i2c.

We could modify the Makefile for i2c core to get the source out of
drivers/of and link it into i2c-core. That would remove the need to
export symbols.

Or you could move the file into the i2c directory and just put a note
on it that Grant is the maintainer.


>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 13:00:08 -0400, Jon Smirl wrote:
> On 7/1/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> > My preference is for things like of_spi and of_i2c to go with the
> >  related busses; I think it makes more sense to keep all the I2C stuff
> >  together, but I've already lost that battle once.
> 
> This is a similar problem to adding aliases to the i2c driver drivers
> for the device tree names of the i2c devices. Instead we have code in
> drivers/of/of_i2c.c that tries to guess the translation from device
> tree to linux names. Adding aliases to the drivers would eliminate the
> need for of_find_i2c_driver().
> 
> I've previously posted patches implementing device tree names in the
> drivers that used ifdef to only instantiate on powerpc builds. For
> example....
> 
> diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
> index e07274d..9cd1770 100644
> --- a/drivers/i2c/chips/tps65010.c
> +++ b/drivers/i2c/chips/tps65010.c
> @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = {
>         { "tps65011", TPS65011 },
>         { "tps65012", TPS65012 },
>         { "tps65013", TPS65013 },
> +       OF_ID("ti,tps65010", TPS65010)
> +       OF_ID("ti,tps65011", TPS65011)
> +       OF_ID("ti,tps65012", TPS65012)
> +       OF_ID("ti,tps65013", TPS65013)
>         { },
>  };
>  MODULE_DEVICE_TABLE(i2c, tps65010_id);

Yeah, yeah, you've been asking for this for months already, but it's
just not going to happen, sorry. You want to abuse the standard Linux
alias mechanism for your personal (i.e. openfirmware) use, but that's
bad. Linux drivers shouldn't have to know whether they are used in
openfirmware trees and what device names are used there. And device
names as seen by user-space shouldn't vary depending on whether the
device comes from an openfirmware tree or not - otherwise all
user-space apps need to learn about both naming conversions.

Unsurprisingly, no other subsystem does what you propose.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 1 Jul 2008 13:00:08 -0400, Jon Smirl wrote:
>  > On 7/1/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > > My preference is for things like of_spi and of_i2c to go with the
>  > >  related busses; I think it makes more sense to keep all the I2C stuff
>  > >  together, but I've already lost that battle once.
>  >
>  > This is a similar problem to adding aliases to the i2c driver drivers
>  > for the device tree names of the i2c devices. Instead we have code in
>  > drivers/of/of_i2c.c that tries to guess the translation from device
>  > tree to linux names. Adding aliases to the drivers would eliminate the
>  > need for of_find_i2c_driver().
>  >
>  > I've previously posted patches implementing device tree names in the
>  > drivers that used ifdef to only instantiate on powerpc builds. For
>  > example....
>  >
>  > diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
>  > index e07274d..9cd1770 100644
>  > --- a/drivers/i2c/chips/tps65010.c
>  > +++ b/drivers/i2c/chips/tps65010.c
>  > @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = {
>  >         { "tps65011", TPS65011 },
>  >         { "tps65012", TPS65012 },
>  >         { "tps65013", TPS65013 },
>  > +       OF_ID("ti,tps65010", TPS65010)
>  > +       OF_ID("ti,tps65011", TPS65011)
>  > +       OF_ID("ti,tps65012", TPS65012)
>  > +       OF_ID("ti,tps65013", TPS65013)
>  >         { },
>  >  };
>  >  MODULE_DEVICE_TABLE(i2c, tps65010_id);
>
>
> Yeah, yeah, you've been asking for this for months already, but it's
>  just not going to happen, sorry. You want to abuse the standard Linux
>  alias mechanism for your personal (i.e. openfirmware) use, but that's
>  bad. Linux drivers shouldn't have to know whether they are used in
>  openfirmware trees and what device names are used there. And device
>  names as seen by user-space shouldn't vary depending on whether the
>  device comes from an openfirmware tree or not - otherwise all
>  user-space apps need to learn about both naming conversions.
>
>  Unsurprisingly, no other subsystem does what you propose.

Then what are all of the PCI aliases doing?

The only difference is that you are recognizing the PCI group as a
naming authority and not recognizing the PowerPC device tree group.
But on the PowerPC platform that is our naming authority. That's why I
proposed adding the names on ifdefs so that they disappear on non
PowerPC platforms.

PS - adding an alias to a driver does not change the name of the
driver. My PCI e1000 module has about 100 aliases but it is always
e1000.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jon Smirl]

On 7/1/08, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
>
> > On Tue, 1 Jul 2008 13:00:08 -0400, Jon Smirl wrote:
>  >  > On 7/1/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>  >
>  > > > My preference is for things like of_spi and of_i2c to go with the
>  >  > >  related busses; I think it makes more sense to keep all the I2C stuff
>  >  > >  together, but I've already lost that battle once.
>  >  >
>  >  > This is a similar problem to adding aliases to the i2c driver drivers
>  >  > for the device tree names of the i2c devices. Instead we have code in
>  >  > drivers/of/of_i2c.c that tries to guess the translation from device
>  >  > tree to linux names. Adding aliases to the drivers would eliminate the
>  >  > need for of_find_i2c_driver().
>  >  >
>  >  > I've previously posted patches implementing device tree names in the
>  >  > drivers that used ifdef to only instantiate on powerpc builds. For
>  >  > example....
>  >  >
>  >  > diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
>  >  > index e07274d..9cd1770 100644
>  >  > --- a/drivers/i2c/chips/tps65010.c
>  >  > +++ b/drivers/i2c/chips/tps65010.c
>  >  > @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = {
>  >  >         { "tps65011", TPS65011 },
>  >  >         { "tps65012", TPS65012 },
>  >  >         { "tps65013", TPS65013 },
>  >  > +       OF_ID("ti,tps65010", TPS65010)
>  >  > +       OF_ID("ti,tps65011", TPS65011)
>  >  > +       OF_ID("ti,tps65012", TPS65012)
>  >  > +       OF_ID("ti,tps65013", TPS65013)
>  >  >         { },
>  >  >  };
>  >  >  MODULE_DEVICE_TABLE(i2c, tps65010_id);
>  >
>  >
>  > Yeah, yeah, you've been asking for this for months already, but it's
>  >  just not going to happen, sorry. You want to abuse the standard Linux
>  >  alias mechanism for your personal (i.e. openfirmware) use, but that's
>  >  bad. Linux drivers shouldn't have to know whether they are used in
>  >  openfirmware trees and what device names are used there. And device
>  >  names as seen by user-space shouldn't vary depending on whether the
>  >  device comes from an openfirmware tree or not - otherwise all
>  >  user-space apps need to learn about both naming conversions.
>  >
>  >  Unsurprisingly, no other subsystem does what you propose.
>
>
> Then what are all of the PCI aliases doing?
>
>  The only difference is that you are recognizing the PCI group as a
>  naming authority and not recognizing the PowerPC device tree group.
>  But on the PowerPC platform that is our naming authority. That's why I
>  proposed adding the names on ifdefs so that they disappear on non
>  PowerPC platforms.
>
>  PS - adding an alias to a driver does not change the name of the
>  driver. My PCI e1000 module has about 100 aliases but it is always
>  e1000.

Here's my e1000e sysfs entry:

jonsmirl@terra:/sys/bus/pci/devices/0000:00:19.0$ ls
broken_parity_status  device  local_cpus  power      resource2         uevent
bus                   driver  modalias    resource   subsystem         vendor
class                 enable  msi_bus     resource0  subsystem_device
config                irq     net:eth0    resource1  subsystem_vendor

jonsmirl@terra:/sys/bus/pci/devices/0000:00:19.0$ cat modalias
pci:v00008086d0000104Bsv00001028sd000001DBbc02sc00i00

>>>>  This is the module alias that was used to load the driver.

jonsmirl@terra:/sys/bus/pci/devices/0000:00:19.0$ ls -l driver
lrwxrwxrwx 1 root root 0 2008-07-01 08:52 driver ->
../../../bus/pci/drivers/e1000e

>>>>  The driver is always e1000e no matter which alias was used to load it. "e1000e"  is controled by the name field of the driver structure.  That's the publicly visible name for the driver.

>>>>  Adding the OF aliases would change the modalias entry, not the driver name.

The i2c implementation is adding a field to a device entry that
contains the driver name. No other device drivers I could find do
this.

jonsmirl@terra:/sys/bus/i2c/devices/1-0050$ ls
bus  driver  eeprom  modalias  name  power  subsystem  uevent
jonsmirl@terra:/sys/bus/i2c/devices/1-0050$ cat name
eeprom
jonsmirl@terra:/sys/bus/i2c/devices/1-0050$ ls -l driver
lrwxrwxrwx 1 root root 0 2008-07-01 14:05 driver ->
../../../../bus/i2c/drivers/eeprom
jonsmirl@terra:/sys/bus/i2c/devices/1-0050$ cat modalias

jonsmirl@terra:/sys/bus/i2c/devices/1-0050$

I believe the correct way to get the driver name from sysfs is to
follow the driver link. The name field is probably legacy.  Other
drivers in the system don't have a name entry on the device node.

Is the user space i2c code looking at the modalias entry?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 13:06:37 -0400, Jon Smirl wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > On Tue, 1 Jul 2008 10:45:18 -0600, Grant Likely wrote:
> >  > But the other side of the coin is that each driver must have
> >  > driver-specific OF code to translate data in the device tree to data
> >  > usable by the driver.  It doesn't make any sense to me for that stuff to
> >  > live anywhere other that with the driver that it supports.
> >
> >
> > This code is glue between OF and subsystems. As with any glue code, you
> >  can argue forever on which side you want to push the code to. Both
> >  answers are valid.
> >
> >  All I see on my personal side is that I don't have any system using OF
> >  and no knowledge about it either, so I can't maintain of_i2c. So having
> >  that file in drivers/of rather than drivers/i2c will make my life
> >  easier for sure. While I'd guess that most (all?) OF-based systems have
> >  an I2C bus, so whoever is responsible for drivers/of should be able to
> >  maintain of_i2c.
> 
> We could modify the Makefile for i2c core to get the source out of
> drivers/of and link it into i2c-core. That would remove the need to
> export symbols.

That might be a bit confusing, but could be done. I still don't know
what problem you are trying to solve though. As I repeated, exporting
i2c_bus_type is perfectly fine. All other bus types are always
exported. So if it's all you need from i2c-core, there's no problem and
we can stop the discussion here.

> Or you could move the file into the i2c directory and just put a note
> on it that Grant is the maintainer.

Assuming that developers will read the source code to find out who the
maintainer is, when we have told everyone to search for this
information in MAINTAINERS. I'd rather add an entry in MAINTAINERS, but
even then I have a doubt that people will get it. My experience is that
people map maintainer names to directories and that's about it. That's
one of the reasons why I wanted to move the i2c device drivers from
drivers/i2c/chips to drivers/hwmon, drivers/rtc, drivers/gpio, etc.

-- 
Jean Delvare

Re: [PATCH 2/2] Add the of_find_i2c_device_by_node function, V4

[Jean Delvare]

On Tue, 1 Jul 2008 13:27:57 -0400, Jon Smirl wrote:
> On 7/1/08, Jean Delvare <khali@linux-fr.org> wrote:
> > On Tue, 1 Jul 2008 13:00:08 -0400, Jon Smirl wrote:
> >  > On 7/1/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > > My preference is for things like of_spi and of_i2c to go with the
> >  > >  related busses; I think it makes more sense to keep all the I2C stuff
> >  > >  together, but I've already lost that battle once.
> >  >
> >  > This is a similar problem to adding aliases to the i2c driver drivers
> >  > for the device tree names of the i2c devices. Instead we have code in
> >  > drivers/of/of_i2c.c that tries to guess the translation from device
> >  > tree to linux names. Adding aliases to the drivers would eliminate the
> >  > need for of_find_i2c_driver().
> >  >
> >  > I've previously posted patches implementing device tree names in the
> >  > drivers that used ifdef to only instantiate on powerpc builds. For
> >  > example....
> >  >
> >  > diff --git a/drivers/i2c/chips/tps65010.c b/drivers/i2c/chips/tps65010.c
> >  > index e07274d..9cd1770 100644
> >  > --- a/drivers/i2c/chips/tps65010.c
> >  > +++ b/drivers/i2c/chips/tps65010.c
> >  > @@ -571,6 +571,10 @@ static const struct i2c_device_id tps65010_id[] = {
> >  >         { "tps65011", TPS65011 },
> >  >         { "tps65012", TPS65012 },
> >  >         { "tps65013", TPS65013 },
> >  > +       OF_ID("ti,tps65010", TPS65010)
> >  > +       OF_ID("ti,tps65011", TPS65011)
> >  > +       OF_ID("ti,tps65012", TPS65012)
> >  > +       OF_ID("ti,tps65013", TPS65013)
> >  >         { },
> >  >  };
> >  >  MODULE_DEVICE_TABLE(i2c, tps65010_id);
> >
> >
> > Yeah, yeah, you've been asking for this for months already, but it's
> >  just not going to happen, sorry. You want to abuse the standard Linux
> >  alias mechanism for your personal (i.e. openfirmware) use, but that's
> >  bad. Linux drivers shouldn't have to know whether they are used in
> >  openfirmware trees and what device names are used there. And device
> >  names as seen by user-space shouldn't vary depending on whether the
> >  device comes from an openfirmware tree or not - otherwise all
> >  user-space apps need to learn about both naming conversions.
> >
> >  Unsurprisingly, no other subsystem does what you propose.
> 
> Then what are all of the PCI aliases doing?
> 
> The only difference is that you are recognizing the PCI group as a
> naming authority and not recognizing the PowerPC device tree group.
> But on the PowerPC platform that is our naming authority. That's why I
> proposed adding the names on ifdefs so that they disappear on non
> PowerPC platforms.

You're comparing PCI devices those ID is built-in, with I2C devices
with no ID. This just can't compare.

> PS - adding an alias to a driver does not change the name of the
> driver. My PCI e1000 module has about 100 aliases but it is always
> e1000.

I said device names, not driver names. Linux I2C devices have a name
attribute which contains the (Linux) device name. That's something a
number of user-space applications are relying on.

-- 
Jean Delvare

Re: [i2c] [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Wolfram Sang]

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

Hi Jean,

On Tue, Jul 01, 2008 at 06:14:44PM +0200, Jean Delvare wrote:

> Applied, after fixing the patch so that it applies, fixing it again so
> that it is correct in the polling case, and fixing it again to make it
> pass checkpatch.pl.
So, it cannot be applied directly? :(

Could you repost the correct version perhaps? I'd like to test it, but
can't find it at http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/
(because it is scheduled for 2.6.27?).

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

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

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Jean Delvare]

On Wed, 2 Jul 2008 15:33:43 +0200, Wolfram Sang wrote:
> Hi Jean,
> 
> On Tue, Jul 01, 2008 at 06:14:44PM +0200, Jean Delvare wrote:
> 
> > Applied, after fixing the patch so that it applies, fixing it again so
> > that it is correct in the polling case, and fixing it again to make it
> > pass checkpatch.pl.
> So, it cannot be applied directly? :(
> 
> Could you repost the correct version perhaps? I'd like to test it, but
> can't find it at http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/
> (because it is scheduled for 2.6.27?).

It's at:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-mpc-convert-to-an-of_platform-driver.patch

It was simply a couple days since I had updated this public directory,
sorry.

-- 
Jean Delvare

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Grant Likely]

On Mon, Jun 30, 2008 at 5:01 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
> Convert i2c-mpc to an of_platform driver. Utilize the code in drivers/of-i2c.c to make i2c modules dynamically loadable by the device tree.

Timur, can you please test this one on an 83xx platform?  It works on
5200, but I want to make sure 83xx doesn't break before I pick it up.

Thanks,
g.

>
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
>
>  arch/powerpc/sysdev/fsl_soc.c |  122 -----------------------------------------
>  drivers/i2c/busses/i2c-mpc.c  |  104 ++++++++++++++++++++---------------
>  2 files changed, 60 insertions(+), 166 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 3a7054e..ebcec73 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -414,128 +414,6 @@ err:
>
>  arch_initcall(gfar_of_init);
>
> -#ifdef CONFIG_I2C_BOARDINFO
> -#include <linux/i2c.h>
> -struct i2c_driver_device {
> -       char    *of_device;
> -       char    *i2c_type;
> -};
> -
> -static struct i2c_driver_device i2c_devices[] __initdata = {
> -       {"ricoh,rs5c372a", "rs5c372a"},
> -       {"ricoh,rs5c372b", "rs5c372b"},
> -       {"ricoh,rv5c386",  "rv5c386"},
> -       {"ricoh,rv5c387a", "rv5c387a"},
> -       {"dallas,ds1307",  "ds1307"},
> -       {"dallas,ds1337",  "ds1337"},
> -       {"dallas,ds1338",  "ds1338"},
> -       {"dallas,ds1339",  "ds1339"},
> -       {"dallas,ds1340",  "ds1340"},
> -       {"stm,m41t00",     "m41t00"},
> -       {"dallas,ds1374",  "rtc-ds1374"},
> -};
> -
> -static int __init of_find_i2c_driver(struct device_node *node,
> -                                    struct i2c_board_info *info)
> -{
> -       int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
> -               if (!of_device_is_compatible(node, i2c_devices[i].of_device))
> -                       continue;
> -               if (strlcpy(info->type, i2c_devices[i].i2c_type,
> -                           I2C_NAME_SIZE) >= I2C_NAME_SIZE)
> -                       return -ENOMEM;
> -               return 0;
> -       }
> -       return -ENODEV;
> -}
> -
> -static void __init of_register_i2c_devices(struct device_node *adap_node,
> -                                          int bus_num)
> -{
> -       struct device_node *node = NULL;
> -
> -       while ((node = of_get_next_child(adap_node, node))) {
> -               struct i2c_board_info info = {};
> -               const u32 *addr;
> -               int len;
> -
> -               addr = of_get_property(node, "reg", &len);
> -               if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
> -                       printk(KERN_WARNING "fsl_soc.c: invalid i2c device entry\n");
> -                       continue;
> -               }
> -
> -               info.irq = irq_of_parse_and_map(node, 0);
> -               if (info.irq == NO_IRQ)
> -                       info.irq = -1;
> -
> -               if (of_find_i2c_driver(node, &info) < 0)
> -                       continue;
> -
> -               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 = 0;
> -       struct platform_device *i2c_dev;
> -       int ret;
> -
> -       for_each_compatible_node(np, NULL, "fsl-i2c") {
> -               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 a076129..4fdfb62 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -17,7 +17,8 @@
>  #include <linux/module.h>
>  #include <linux/sched.h>
>  #include <linux/init.h>
> -#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_i2c.h>
>
>  #include <asm/io.h>
>  #include <linux/fsl_devices.h>
> @@ -25,13 +26,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
> @@ -315,102 +316,117 @@ static struct i2c_adapter mpc_ops = {
>        .timeout = 1,
>  };
>
> -static int fsl_i2c_probe(struct platform_device *pdev)
> +static int __devinit fsl_i2c_probe(struct of_device *op, const struct of_device_id *match)
>  {
>        int result = 0;
>        struct mpc_i2c *i2c;
> -       struct fsl_i2c_platform_data *pdata;
> -       struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -       pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;
>
>        i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>        if (!i2c)
>                return -ENOMEM;
>
> -       i2c->irq = platform_get_irq(pdev, 0);
> -       if (i2c->irq < 0)
> -               i2c->irq = NO_IRQ; /* Use polling */
> +       if (of_get_property(op->node, "dfsrr", NULL))
> +               i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
>
> -       i2c->flags = pdata->device_flags;
> -       init_waitqueue_head(&i2c->queue);
> +       if (of_device_is_compatible(op->node, "fsl,mpc5200-i2c") ||
> +                       of_device_is_compatible(op->node, "mpc5200-i2c"))
> +               i2c->flags |= FSL_I2C_DEV_CLOCK_5200;
>
> -       i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
> +       init_waitqueue_head(&i2c->queue);
>
> +       i2c->base = of_iomap(op->node, 0);
>        if (!i2c->base) {
>                printk(KERN_ERR "i2c-mpc - failed to map controller\n");
>                result = -ENOMEM;
>                goto fail_map;
>        }
>
> -       if (i2c->irq != NO_IRQ)
> -               if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> -                                         IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> -                       printk(KERN_ERR
> -                              "i2c-mpc - failed to attach interrupt\n");
> -                       goto fail_irq;
> +       i2c->irq = irq_of_parse_and_map(op->node, 0);
> +       if (i2c->irq != NO_IRQ) { /* i2c->irq = NO_IRQ implies polling */
> +               result = request_irq(i2c->irq, mpc_i2c_isr,
> +                                    IRQF_SHARED, "i2c-mpc", i2c);
> +               if (result < 0) {
> +                       printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
> +                       goto fail_request;
>                }
> -
> +       }
> +
>        mpc_i2c_setclock(i2c);
> -       platform_set_drvdata(pdev, i2c);
> +
> +       dev_set_drvdata(&op->dev, i2c);
>
>        i2c->adap = mpc_ops;
> -       i2c->adap.nr = pdev->id;
>        i2c_set_adapdata(&i2c->adap, i2c);
> -       i2c->adap.dev.parent = &pdev->dev;
> -       if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
> +       i2c->adap.dev.parent = &op->dev;
> +
> +       result = i2c_add_adapter(&i2c->adap);
> +       if (result < 0) {
>                printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
>                goto fail_add;
>        }
> +       of_register_i2c_devices(&i2c->adap, op->node);
>
>        return result;
>
> -      fail_add:
> -       if (i2c->irq != NO_IRQ)
> -               free_irq(i2c->irq, i2c);
> -      fail_irq:
> -       iounmap(i2c->base);
> -      fail_map:
> + fail_add:
> +       dev_set_drvdata(&op->dev, NULL);
> +       free_irq(i2c->irq, i2c);
> + fail_request:
> +       irq_dispose_mapping(i2c->irq);
> +       iounmap(i2c->base);
> + fail_map:
>        kfree(i2c);
>        return result;
>  };
>
> -static int fsl_i2c_remove(struct platform_device *pdev)
> +static int __devexit fsl_i2c_remove(struct of_device *op)
>  {
> -       struct mpc_i2c *i2c = platform_get_drvdata(pdev);
> +       struct mpc_i2c *i2c = dev_get_drvdata(&op->dev);
>
>        i2c_del_adapter(&i2c->adap);
> -       platform_set_drvdata(pdev, NULL);
> +       dev_set_drvdata(&op->dev, NULL);
>
>        if (i2c->irq != NO_IRQ)
>                free_irq(i2c->irq, i2c);
>
> +       irq_dispose_mapping(i2c->irq);
>        iounmap(i2c->base);
>        kfree(i2c);
>        return 0;
>  };
>
> -/* work with hotplug and coldplug */
> -MODULE_ALIAS("platform:fsl-i2c");
> +static const 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          = fsl_i2c_probe,
> +       .remove         = __devexit_p(fsl_i2c_remove),
> +       .driver         = {
> +               .owner  = THIS_MODULE,
> +               .name   = DRV_NAME,
>        },
>  };
>
>  static int __init fsl_i2c_init(void)
>  {
> -       return platform_driver_register(&fsl_i2c_driver);
> +       int rv;
> +
> +       rv = of_register_platform_driver(&mpc_i2c_driver);
> +       if (rv)
> +               printk(KERN_ERR DRV_NAME
> +                      " of_register_platform_driver failed (%i)\n", rv);
> +       return rv;
>  }
>
>  static void __exit fsl_i2c_exit(void)
>  {
> -       platform_driver_unregister(&fsl_i2c_driver);
> +       of_unregister_platform_driver(&mpc_i2c_driver);
>  }
>
>  module_init(fsl_i2c_init);
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Timur Tabi]

On Jul 9, 2008, at 1:22 PM, Grant Likely wrote:

> On Mon, Jun 30, 2008 at 5:01 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
>> Convert i2c-mpc to an of_platform driver. Utilize the code in  
>> drivers/of-i2c.c to make i2c modules dynamically loadable by the  
>> device tree.
>
> Timur, can you please test this one on an 83xx platform?  It works on
> 5200, but I want to make sure 83xx doesn't break before I pick it up.

I'm on vacation this week, so I'll have to deal with it next week,  
after I've handled higher-priority issues.

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Grant Likely]

On Thu, Jul 10, 2008 at 12:36:58PM -0400, Timur Tabi wrote:
>
> On Jul 9, 2008, at 1:22 PM, Grant Likely wrote:
>
>> On Mon, Jun 30, 2008 at 5:01 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
>>> Convert i2c-mpc to an of_platform driver. Utilize the code in  
>>> drivers/of-i2c.c to make i2c modules dynamically loadable by the  
>>> device tree.
>>
>> Timur, can you please test this one on an 83xx platform?  It works on
>> 5200, but I want to make sure 83xx doesn't break before I pick it up.
>
> I'm on vacation this week, so I'll have to deal with it next week, after 
> I've handled higher-priority issues.

Kumar, is there anyone else who can test this out before the merge
window opens?

OTOH, it should be pretty safe.  It's been tested on 5200.  I could just
pick it up without 8xxx testing if you're okay with it.

g.

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Anton Vorontsov]

On Fri, Jul 11, 2008 at 12:12:14PM -0600, Grant Likely wrote:
> On Thu, Jul 10, 2008 at 12:36:58PM -0400, Timur Tabi wrote:
> >
> > On Jul 9, 2008, at 1:22 PM, Grant Likely wrote:
> >
> >> On Mon, Jun 30, 2008 at 5:01 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
> >>> Convert i2c-mpc to an of_platform driver. Utilize the code in  
> >>> drivers/of-i2c.c to make i2c modules dynamically loadable by the  
> >>> device tree.
> >>
> >> Timur, can you please test this one on an 83xx platform?  It works on
> >> 5200, but I want to make sure 83xx doesn't break before I pick it up.
> >
> > I'm on vacation this week, so I'll have to deal with it next week, after 
> > I've handled higher-priority issues.
> 
> Kumar, is there anyone else who can test this out before the merge
> window opens?

FWIW, they seem to work for me. At least RTC still works.


p.s. [1/2] conflicts with fsl_soc.c. And these patches will
conflict a bit more (now with of_i2c.c), if/when
"[PATCH] of/i2c: don't pass -1 to irq_dispose_mapping, otherwise kernel
kernel will oops"

will be applied to 2.6.26.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 1/2] Convert i2c-mpc from a platform driver into a of_platform driver, V4

[Grant Likely]

On Fri, Jul 11, 2008 at 10:45:56PM +0400, Anton Vorontsov wrote:
> On Fri, Jul 11, 2008 at 12:12:14PM -0600, Grant Likely wrote:
> > On Thu, Jul 10, 2008 at 12:36:58PM -0400, Timur Tabi wrote:
> > >
> > > On Jul 9, 2008, at 1:22 PM, Grant Likely wrote:
> > >
> > >> On Mon, Jun 30, 2008 at 5:01 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
> > >>> Convert i2c-mpc to an of_platform driver. Utilize the code in  
> > >>> drivers/of-i2c.c to make i2c modules dynamically loadable by the  
> > >>> device tree.
> > >>
> > >> Timur, can you please test this one on an 83xx platform?  It works on
> > >> 5200, but I want to make sure 83xx doesn't break before I pick it up.
> > >
> > > I'm on vacation this week, so I'll have to deal with it next week, after 
> > > I've handled higher-priority issues.
> > 
> > Kumar, is there anyone else who can test this out before the merge
> > window opens?
> 
> FWIW, they seem to work for me. At least RTC still works.

Good enough for me.  Unless someone screams really loudly I'm going to
go ahead and pick it up.

Cheers,
g.