[PATCH] i2c-ibm_iic driver

[PATCH] i2c-ibm_iic driver

[Sean MacLennan]

This patch allows the i2c-ibm_iic driver to be built either as an ocp 
driver or an of_platform driver. This allows it to run under the powerpc 
arch but maintains backward compatibility with the ppc arch.

Cheers,
   Sean MacLennan

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
---

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c466c6c..e9e1493 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -241,7 +241,6 @@ config I2C_PIIX4
 
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
-	depends on IBM_OCP
 	help
 	  Say Y here if you want to use IIC peripheral found on 
 	  embedded IBM PPC 4xx based systems. 
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 9b43ff7..98476fc 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -6,6 +6,9 @@
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
  * Based on original work by 
  * 	Ian DaSilva  <idasilva@mvista.com>
  *      Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
 #include <asm/ocp.h>
 #include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
 
 #include "i2c-ibm_iic.h"
 
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
 
 MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
 MODULE_LICENSE("GPL");
@@ -650,13 +658,14 @@ static inline u8 iic_clckdiv(unsigned int opb)
 	opb /= 1000000;
 	
 	if (opb < 20 || opb > 150){
-		printk(KERN_CRIT "ibm-iic: invalid OPB clock frequency %u MHz\n",
+		printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u MHz\n",
 			opb);
 		opb = opb < 20 ? 20 : 150;
 	}
 	return (u8)((opb + 9) / 10 - 1);
 }
 
+#ifdef CONFIG_IBM_OCP
 /*
  * Register single IIC interface
  */
@@ -672,7 +681,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 			ocp->def->index);
 
 	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
-		printk(KERN_CRIT "ibm-iic%d: failed to allocate device data\n",
+		printk(KERN_ERR "ibm-iic%d: failed to allocate device data\n",
 			ocp->def->index);
 		return -ENOMEM;
 	}
@@ -687,7 +696,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 	}
 
 	if (!(dev->vaddr = ioremap(ocp->def->paddr, sizeof(struct iic_regs)))){
-		printk(KERN_CRIT "ibm-iic%d: failed to ioremap device registers\n",
+		printk(KERN_ERR "ibm-iic%d: failed to ioremap device registers\n",
 			dev->idx);
 		ret = -ENXIO;
 		goto fail2;
@@ -746,7 +755,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 	adap->nr = dev->idx >= 0 ? dev->idx : 0;
 
 	if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
-		printk(KERN_CRIT "ibm-iic%d: failed to register i2c adapter\n",
+		printk(KERN_ERR "ibm-iic%d: failed to register i2c adapter\n",
 			dev->idx);
 		goto fail;
 	}
@@ -779,7 +788,7 @@ static void __devexit iic_remove(struct ocp_device *ocp)
 	struct ibm_iic_private* dev = (struct ibm_iic_private*)ocp_get_drvdata(ocp);
 	BUG_ON(dev == NULL);
 	if (i2c_del_adapter(&dev->adap)){
-		printk(KERN_CRIT "ibm-iic%d: failed to delete i2c adapter :(\n",
+		printk(KERN_ERR "ibm-iic%d: failed to delete i2c adapter :(\n",
 			dev->idx);
 		/* That's *very* bad, just shutdown IRQ ... */
 		if (dev->irq >= 0){
@@ -831,3 +840,189 @@ static void __exit iic_exit(void)
 
 module_init(iic_init);
 module_exit(iic_exit);
+#else
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+			       const struct of_device_id *match)
+{
+	static int index = 0;
+	struct device_node *np = ofdev->node;
+	struct ibm_iic_private* dev;
+	struct i2c_adapter* adap;
+	const u32 *indexp, *freq;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		printk(KERN_ERR "ibm-iic: failed to allocate device data\n");
+		return -ENOMEM;
+	}
+
+	/* This assumes we don't mix index and non-index entries. */
+	indexp = of_get_property(np, "index", NULL);
+	dev->idx = indexp ? *indexp : index++;
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	dev->vaddr = of_iomap(np, 0);
+	if (dev->vaddr == NULL) {
+		printk(KERN_ERR "ibm-iic%d: failed to ioremap device registers\n",
+			dev->idx);
+		ret = -ENXIO;
+		goto fail1;
+	}
+
+	init_waitqueue_head(&dev->wq);
+
+	if (iic_force_poll)
+		dev->irq = NO_IRQ;
+	else {
+		dev->irq = irq_of_parse_and_map(np, 0);
+		if (dev->irq == NO_IRQ)
+			printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
+		else {
+			/* Disable interrupts until we finish initialization,
+			   assumes level-sensitive IRQ setup...
+			*/
+			iic_interrupt_mode(dev, 0);
+			if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)){
+				printk(KERN_ERR "ibm-iic%d: request_irq %d failed\n",
+				       dev->idx, dev->irq);
+				/* Fallback to the polling mode */
+				dev->irq = NO_IRQ;
+			}
+		}
+	}
+
+	if (dev->irq == NO_IRQ)
+		printk(KERN_WARNING "ibm-iic%d: using polling mode\n",
+			   dev->idx);
+
+	/* Board specific settings */
+	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+		dev->fast_mode = 1;
+	else
+		dev->fast_mode = 0;
+
+	/* clckdiv is the same for *all* IIC interfaces, but I'd rather
+	 * make a copy than introduce another global. --ebs
+	 */
+	freq = of_get_property(np, "clock-frequency", NULL);
+	if (freq == NULL) {
+		freq = of_get_property(np->parent, "clock-frequency", NULL);
+		if (freq == NULL) {
+			printk(KERN_ERR "ibm-iic%d: Unable to get bus frequency\n",
+			       dev->idx);
+			ret = -EBUSY;
+			goto fail;
+		}
+	}
+
+	dev->clckdiv = iic_clckdiv(*freq);
+	DBG("%d: clckdiv = %d\n", dev->idx, dev->clckdiv);
+
+	/* Initialize IIC interface */
+	iic_dev_init(dev);
+
+	/* Register it with i2c layer */
+	adap = &dev->adap;
+	adap->dev.parent = &ofdev->dev;
+	strcpy(adap->name, "IBM IIC");
+	i2c_set_adapdata(adap, dev);
+	adap->id = I2C_HW_OCP;
+	adap->class = I2C_CLASS_HWMON;
+	adap->algo = &iic_algo;
+	adap->client_register = NULL;
+	adap->client_unregister = NULL;
+	adap->timeout = 1;
+	adap->retries = 1;
+	adap->nr = dev->idx;
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret  < 0) {
+		printk(KERN_ERR "ibm-iic%d: failed to register i2c adapter\n",
+			dev->idx);
+		goto fail;
+	}
+
+	printk(KERN_INFO "ibm-iic%d: using %s mode\n", dev->idx,
+		dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+	return 0;
+
+fail:
+	if (dev->irq != NO_IRQ){
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	iounmap(dev->vaddr);
+fail1:
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
+	return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private* dev = dev_get_drvdata(&ofdev->dev);
+
+	BUG_ON(dev == NULL);
+	if (i2c_del_adapter(&dev->adap)){
+		printk(KERN_ERR "ibm-iic%d: failed to delete i2c adapter :(\n",
+			dev->idx);
+		/* That's *very* bad, just shutdown IRQ ... */
+		if (dev->irq != NO_IRQ){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		    dev->irq = NO_IRQ;
+		}
+	} else {
+		if (dev->irq != NO_IRQ){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		}
+		iounmap(dev->vaddr);
+		kfree(dev);
+	}
+
+	return 0;
+}
+
+
+static const struct of_device_id ibm_iic_match[] =
+{
+	{ .type = "i2c", .compatible = "ibm,iic-405ex", },
+	{ .type = "i2c", .compatible = "ibm,iic-405gp", },
+	{ .type = "i2c", .compatible = "ibm,iic-440gp", },
+	{ .type = "i2c", .compatible = "ibm,iic-440gpx", },
+	{ .type = "i2c", .compatible = "ibm,iic-440grx", },
+	{}
+};
+
+static struct of_platform_driver ibm_iic_driver =
+{
+	.name   = "ibm-iic",
+	.match_table = ibm_iic_match,
+	.probe  = iic_probe,
+	.remove = iic_remove,
+};
+
+static int __init ibm_iic_init(void)
+{
+	printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
+	return of_register_platform_driver(&ibm_iic_driver);
+}
+module_init(ibm_iic_init);
+
+static void __exit ibm_iic_exit(void)
+{
+	of_unregister_platform_driver(&ibm_iic_driver);
+}
+module_exit(ibm_iic_exit);
+#endif

Re: [PATCH 1/2] i2c-ibm_iic driver

[Sean MacLennan]

Jean Delvare wrote:
> Please split your patch into logical parts:
> * Whitespace and coding-style cleanups
> * Other cleanups (e.g. changing the log levels)
> * Add OF support
>   
Here is the first patch with everything except the OF support. Really 
all I did was change the log levels based on feedback from linxppc-dev.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
---
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 7c7eb0c..a981a17 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -650,7 +650,7 @@ static inline u8 iic_clckdiv(unsigned int opb)
 	opb /= 1000000;
 
 	if (opb < 20 || opb > 150){
-		printk(KERN_CRIT "ibm-iic: invalid OPB clock frequency %u MHz\n",
+		printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u MHz\n",
 			opb);
 		opb = opb < 20 ? 20 : 150;
 	}
@@ -672,7 +672,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 			ocp->def->index);
 
 	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
-		printk(KERN_CRIT "ibm-iic%d: failed to allocate device data\n",
+		printk(KERN_ERR "ibm-iic%d: failed to allocate device data\n",
 			ocp->def->index);
 		return -ENOMEM;
 	}
@@ -687,7 +687,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 	}
 
 	if (!(dev->vaddr = ioremap(ocp->def->paddr, sizeof(struct iic_regs)))){
-		printk(KERN_CRIT "ibm-iic%d: failed to ioremap device registers\n",
+		printk(KERN_ERR "ibm-iic%d: failed to ioremap device registers\n",
 			dev->idx);
 		ret = -ENXIO;
 		goto fail2;
@@ -745,7 +745,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 	adap->nr = dev->idx >= 0 ? dev->idx : 0;
 
 	if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
-		printk(KERN_CRIT "ibm-iic%d: failed to register i2c adapter\n",
+		printk(KERN_ERR "ibm-iic%d: failed to register i2c adapter\n",
 			dev->idx);
 		goto fail;
 	}
@@ -778,7 +778,7 @@ static void __devexit iic_remove(struct ocp_device *ocp)
 	struct ibm_iic_private* dev = (struct ibm_iic_private*)ocp_get_drvdata(ocp);
 	BUG_ON(dev == NULL);
 	if (i2c_del_adapter(&dev->adap)){
-		printk(KERN_CRIT "ibm-iic%d: failed to delete i2c adapter :(\n",
+		printk(KERN_ERR "ibm-iic%d: failed to delete i2c adapter :(\n",
 			dev->idx);
 		/* That's *very* bad, just shutdown IRQ ... */
 		if (dev->irq >= 0){

Re: [PATCH 1/2] i2c-ibm_iic driver

[Jean Delvare]

On Fri, 15 Feb 2008 23:07:21 -0500, Sean MacLennan wrote:
> Jean Delvare wrote:
> > Please split your patch into logical parts:
> > * Whitespace and coding-style cleanups
> > * Other cleanups (e.g. changing the log levels)
> > * Add OF support
> >   
> Here is the first patch with everything except the OF support. Really 
> all I did was change the log levels based on feedback from linxppc-dev.
> 
> Cheers,
>    Sean
> 
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>

Applied, thanks.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c-ibm_iic driver

[Sean MacLennan]

Here is the of platform patch. I removed the retries and removed the spaces used for spacing.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
---
--- old-i2c-ibm_iic.c	2008-02-15 23:01:58.000000000 -0500
+++ i2c-ibm_iic.c	2008-02-15 23:00:44.000000000 -0500
@@ -6,6 +6,9 @@
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
  * Based on original work by
  * 	Ian DaSilva  <idasilva@mvista.com>
  *      Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
 #include <asm/ocp.h>
 #include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
 
 #include "i2c-ibm_iic.h"
 
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
 
 MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
 MODULE_LICENSE("GPL");
@@ -657,6 +665,7 @@
 	return (u8)((opb + 9) / 10 - 1);
 }
 
+#ifdef CONFIG_IBM_OCP
 /*
  * Register single IIC interface
  */
@@ -830,3 +839,188 @@
 
 module_init(iic_init);
 module_exit(iic_exit);
+#else
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+			       const struct of_device_id *match)
+{
+	static int index = 0;
+	struct device_node *np = ofdev->node;
+	struct ibm_iic_private* dev;
+	struct i2c_adapter* adap;
+	const u32 *indexp, *freq;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		printk(KERN_ERR "ibm-iic: failed to allocate device data\n");
+		return -ENOMEM;
+	}
+
+	/* This assumes we don't mix index and non-index entries. */
+	indexp = of_get_property(np, "index", NULL);
+	dev->idx = indexp ? *indexp : index++;
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	dev->vaddr = of_iomap(np, 0);
+	if (dev->vaddr == NULL) {
+		printk(KERN_ERR "ibm-iic%d: failed to ioremap device registers\n",
+			dev->idx);
+		ret = -ENXIO;
+		goto fail1;
+	}
+
+	init_waitqueue_head(&dev->wq);
+
+	if (iic_force_poll)
+		dev->irq = NO_IRQ;
+	else {
+		dev->irq = irq_of_parse_and_map(np, 0);
+		if (dev->irq == NO_IRQ)
+			printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
+		else {
+			/* Disable interrupts until we finish initialization,
+			   assumes level-sensitive IRQ setup...
+			*/
+			iic_interrupt_mode(dev, 0);
+			if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)){
+				printk(KERN_ERR "ibm-iic%d: request_irq %d failed\n",
+				       dev->idx, dev->irq);
+				/* Fallback to the polling mode */
+				dev->irq = NO_IRQ;
+			}
+		}
+	}
+
+	if (dev->irq == NO_IRQ)
+		printk(KERN_WARNING "ibm-iic%d: using polling mode\n",
+			   dev->idx);
+
+	/* Board specific settings */
+	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+		dev->fast_mode = 1;
+	else
+		dev->fast_mode = 0;
+
+	/* clckdiv is the same for *all* IIC interfaces, but I'd rather
+	 * make a copy than introduce another global. --ebs
+	 */
+	freq = of_get_property(np, "clock-frequency", NULL);
+	if (freq == NULL) {
+		freq = of_get_property(np->parent, "clock-frequency", NULL);
+		if (freq == NULL) {
+			printk(KERN_ERR "ibm-iic%d: Unable to get bus frequency\n",
+			       dev->idx);
+			ret = -EBUSY;
+			goto fail;
+		}
+	}
+
+	dev->clckdiv = iic_clckdiv(*freq);
+	DBG("%d: clckdiv = %d\n", dev->idx, dev->clckdiv);
+
+	/* Initialize IIC interface */
+	iic_dev_init(dev);
+
+	/* Register it with i2c layer */
+	adap = &dev->adap;
+	adap->dev.parent = &ofdev->dev;
+	strcpy(adap->name, "IBM IIC");
+	i2c_set_adapdata(adap, dev);
+	adap->id = I2C_HW_OCP;
+	adap->class = I2C_CLASS_HWMON;
+	adap->algo = &iic_algo;
+	adap->client_register = NULL;
+	adap->client_unregister = NULL;
+	adap->timeout = 1;
+	adap->nr = dev->idx;
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret  < 0) {
+		printk(KERN_ERR "ibm-iic%d: failed to register i2c adapter\n",
+			dev->idx);
+		goto fail;
+	}
+
+	printk(KERN_INFO "ibm-iic%d: using %s mode\n", dev->idx,
+		dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+	return 0;
+
+fail:
+	if (dev->irq != NO_IRQ){
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	iounmap(dev->vaddr);
+fail1:
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
+	return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private* dev = dev_get_drvdata(&ofdev->dev);
+
+	BUG_ON(dev == NULL);
+	if (i2c_del_adapter(&dev->adap)){
+		printk(KERN_ERR "ibm-iic%d: failed to delete i2c adapter :(\n",
+			dev->idx);
+		/* That's *very* bad, just shutdown IRQ ... */
+		if (dev->irq != NO_IRQ){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		    dev->irq = NO_IRQ;
+		}
+	} else {
+		if (dev->irq != NO_IRQ){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		}
+		iounmap(dev->vaddr);
+		kfree(dev);
+	}
+
+	return 0;
+}
+
+
+static const struct of_device_id ibm_iic_match[] =
+{
+	{ .compatible = "ibm,iic-405ex", },
+	{ .compatible = "ibm,iic-405gp", },
+	{ .compatible = "ibm,iic-440gp", },
+	{ .compatible = "ibm,iic-440gpx", },
+	{ .compatible = "ibm,iic-440grx", },
+	{}
+};
+
+static struct of_platform_driver ibm_iic_driver =
+{
+	.name	= "ibm-iic",
+	.match_table = ibm_iic_match,
+	.probe	= iic_probe,
+	.remove	= iic_remove,
+};
+
+static int __init ibm_iic_init(void)
+{
+	printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
+	return of_register_platform_driver(&ibm_iic_driver);
+}
+module_init(ibm_iic_init);
+
+static void __exit ibm_iic_exit(void)
+{
+	of_unregister_platform_driver(&ibm_iic_driver);
+}
+module_exit(ibm_iic_exit);
+#endif

Re: [PATCH 2/2] i2c-ibm_iic driver

[Jean Delvare]

Hi Sean,

On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
> Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
> 
> Cheers,
>    Sean
> 
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>

First of all: please run scripts/checkpatch.pl on your patch and fix
the reported errors. It tells me:
  total: 10 errors, 5 warnings, 222 lines checked
which is definitely too much.

Review:

> ---
> --- old-i2c-ibm_iic.c	2008-02-15 23:01:58.000000000 -0500
> +++ i2c-ibm_iic.c	2008-02-15 23:00:44.000000000 -0500

Please send a proper -p1 patch next time.

> @@ -6,6 +6,9 @@
>   * Copyright (c) 2003, 2004 Zultys Technologies.
>   * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
>   *
> + * Copyright (c) 2008 PIKA Technologies
> + * Sean MacLennan <smaclennan@pikatech.com>
> + *
>   * Based on original work by
>   * 	Ian DaSilva  <idasilva@mvista.com>
>   *      Armin Kuster <akuster@mvista.com>
> @@ -39,12 +42,17 @@
>  #include <asm/io.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-id.h>
> +
> +#ifdef CONFIG_IBM_OCP

Your patch seems to be incomplete. There is still

config I2C_IBM_IIC
	tristate "IBM PPC 4xx on-chip I2C interface"
	depends on IBM_OCP

in drivers/i2c/busses/Kconfig, so the new code can never be active.

>  #include <asm/ocp.h>
>  #include <asm/ibm4xx.h>
> +#else
> +#include <linux/of_platform.h>
> +#endif
>  
>  #include "i2c-ibm_iic.h"
>  
> -#define DRIVER_VERSION "2.1"
> +#define DRIVER_VERSION "2.2"
>  
>  MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
>  MODULE_LICENSE("GPL");
> @@ -657,6 +665,7 @@
>  	return (u8)((opb + 9) / 10 - 1);
>  }
>  
> +#ifdef CONFIG_IBM_OCP
>  /*
>   * Register single IIC interface
>   */
> @@ -830,3 +839,188 @@
>  
>  module_init(iic_init);
>  module_exit(iic_exit);
> +#else

Please add a comment saying what this #else corresponds to.

> +/*
> + * Register single IIC interface
> + */
> +static int __devinit iic_probe(struct of_device *ofdev,
> +			       const struct of_device_id *match)
> +{
> +	static int index = 0;
> +	struct device_node *np = ofdev->node;
> +	struct ibm_iic_private* dev;

Confusing variable name.

> +	struct i2c_adapter* adap;
> +	const u32 *indexp, *freq;
> +	int ret;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev) {
> +		printk(KERN_ERR "ibm-iic: failed to allocate device data\n");

Please use dev_err. Same for all other messages below.

> +		return -ENOMEM;
> +	}
> +
> +	/* This assumes we don't mix index and non-index entries. */
> +	indexp = of_get_property(np, "index", NULL);
> +	dev->idx = indexp ? *indexp : index++;

I don't like this static index thing much. Can't you just make the
"index" OF property mandatory? Mixing ways to number things can become
very confusing. In particular as you are using dev->idx later to call
i2c_add_numbered_adapter(), the caller is really supposed to know what
they are doing with the bus numbers.

> +
> +	dev_set_drvdata(&ofdev->dev, dev);
> +
> +	dev->vaddr = of_iomap(np, 0);
> +	if (dev->vaddr == NULL) {
> +		printk(KERN_ERR "ibm-iic%d: failed to ioremap device registers\n",
> +			dev->idx);
> +		ret = -ENXIO;
> +		goto fail1;
> +	}
> +
> +	init_waitqueue_head(&dev->wq);
> +
> +	if (iic_force_poll)
> +		dev->irq = NO_IRQ;
> +	else {
> +		dev->irq = irq_of_parse_and_map(np, 0);
> +		if (dev->irq == NO_IRQ)
> +			printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
> +		else {
> +			/* Disable interrupts until we finish initialization,
> +			   assumes level-sensitive IRQ setup...
> +			*/
> +			iic_interrupt_mode(dev, 0);
> +			if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)){
> +				printk(KERN_ERR "ibm-iic%d: request_irq %d failed\n",
> +				       dev->idx, dev->irq);
> +				/* Fallback to the polling mode */
> +				dev->irq = NO_IRQ;
> +			}
> +		}
> +	}
> +
> +	if (dev->irq == NO_IRQ)
> +		printk(KERN_WARNING "ibm-iic%d: using polling mode\n",
> +			   dev->idx);
> +
> +	/* Board specific settings */
> +	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
> +		dev->fast_mode = 1;
> +	else
> +		dev->fast_mode = 0;

The second part is not needed, 0 is the default thanks to kzalloc.

> +
> +	/* clckdiv is the same for *all* IIC interfaces, but I'd rather
> +	 * make a copy than introduce another global. --ebs
> +	 */
> +	freq = of_get_property(np, "clock-frequency", NULL);
> +	if (freq == NULL) {
> +		freq = of_get_property(np->parent, "clock-frequency", NULL);
> +		if (freq == NULL) {
> +			printk(KERN_ERR "ibm-iic%d: Unable to get bus frequency\n",
> +			       dev->idx);
> +			ret = -EBUSY;
> +			goto fail;
> +		}
> +	}
> +
> +	dev->clckdiv = iic_clckdiv(*freq);
> +	DBG("%d: clckdiv = %d\n", dev->idx, dev->clckdiv);
> +
> +	/* Initialize IIC interface */
> +	iic_dev_init(dev);
> +
> +	/* Register it with i2c layer */
> +	adap = &dev->adap;
> +	adap->dev.parent = &ofdev->dev;
> +	strcpy(adap->name, "IBM IIC");

strlcpy please.

> +	i2c_set_adapdata(adap, dev);
> +	adap->id = I2C_HW_OCP;
> +	adap->class = I2C_CLASS_HWMON;
> +	adap->algo = &iic_algo;
> +	adap->client_register = NULL;
> +	adap->client_unregister = NULL;

The last two statements are not needed, again kzalloc did it for you.

> +	adap->timeout = 1;
> +	adap->nr = dev->idx;

Looks to me like the block above has much in common with the original
probe function, maybe it would be worth sharing the code to make future
maintenance easier?

> +
> +	ret = i2c_add_numbered_adapter(adap);
> +	if (ret  < 0) {
> +		printk(KERN_ERR "ibm-iic%d: failed to register i2c adapter\n",
> +			dev->idx);
> +		goto fail;
> +	}
> +
> +	printk(KERN_INFO "ibm-iic%d: using %s mode\n", dev->idx,
> +		dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
> +
> +	return 0;
> +
> +fail:
> +	if (dev->irq != NO_IRQ){
> +		iic_interrupt_mode(dev, 0);
> +		free_irq(dev->irq, dev);
> +	}
> +
> +	iounmap(dev->vaddr);
> +fail1:

I suggest giving explicit names to your labels based on the next
action, e.g. err_free_irq and err_kfree.

> +	dev_set_drvdata(&ofdev->dev, NULL);
> +	kfree(dev);
> +	return ret;
> +}
> +
> +/*
> + * Cleanup initialized IIC interface
> + */
> +static int __devexit iic_remove(struct of_device *ofdev)
> +{
> +	struct ibm_iic_private* dev = dev_get_drvdata(&ofdev->dev);
> +
> +	BUG_ON(dev == NULL);

How could this happen at all?

> +	if (i2c_del_adapter(&dev->adap)){

i2c_del_adapter can't really fail and almost all other drivers don't
even bother checking the error code. But if you do, you should really
return the error code.

> +		printk(KERN_ERR "ibm-iic%d: failed to delete i2c adapter :(\n",
> +			dev->idx);
> +		/* That's *very* bad, just shutdown IRQ ... */
> +		if (dev->irq != NO_IRQ){
> +		    iic_interrupt_mode(dev, 0);
> +		    free_irq(dev->irq, dev);
> +		    dev->irq = NO_IRQ;

Bad indentation.

> +		}
> +	} else {
> +		if (dev->irq != NO_IRQ){
> +		    iic_interrupt_mode(dev, 0);
> +		    free_irq(dev->irq, dev);

Bad indentation.

> +		}
> +		iounmap(dev->vaddr);

May I suggest adding:

		dev_set_drvdata(&ofdev->dev, NULL);

> +		kfree(dev);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static const struct of_device_id ibm_iic_match[] =
> +{
> +	{ .compatible = "ibm,iic-405ex", },
> +	{ .compatible = "ibm,iic-405gp", },
> +	{ .compatible = "ibm,iic-440gp", },
> +	{ .compatible = "ibm,iic-440gpx", },
> +	{ .compatible = "ibm,iic-440grx", },
> +	{}
> +};
> +
> +static struct of_platform_driver ibm_iic_driver =
> +{
> +	.name	= "ibm-iic",
> +	.match_table = ibm_iic_match,
> +	.probe	= iic_probe,
> +	.remove	= iic_remove,

Missing __devexit_p.

> +};
> +
> +static int __init ibm_iic_init(void)
> +{
> +	printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
> +	return of_register_platform_driver(&ibm_iic_driver);
> +}
> +module_init(ibm_iic_init);
> +
> +static void __exit ibm_iic_exit(void)
> +{
> +	of_unregister_platform_driver(&ibm_iic_driver);
> +}
> +module_exit(ibm_iic_exit);

If you would name these functions iic_init and iic_exit as the original
code does, you wouldn't have to duplicate the module_init and
module_exit statements.

> +#endif

Please add a comment saying what this #endif corresponds to.

Note that I cannot test the code so I am relying on the linuxppc-dev
folks to test it.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c-ibm_iic driver

[Sean MacLennan]

Jean Delvare wrote:
> Hi Sean,
>
> On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
>   
>> Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
>>
>> Cheers,
>>    Sean
>>
>> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
>>     
>
> First of all: please run scripts/checkpatch.pl on your patch and fix
> the reported errors. It tells me:
>   total: 10 errors, 5 warnings, 222 lines checked
> which is definitely too much.
>   
Many of the errors/warnings are from the cut and paste of the original 
code. I realize this doesn't justify my new code, but does beg the 
question; should I also cleanup the original code? And how much? I am 
tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.

A few comments, everything else I agree with.

>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* This assumes we don't mix index and non-index entries. */
>> +	indexp = of_get_property(np, "index", NULL);
>> +	dev->idx = indexp ? *indexp : index++;
>>     
>
> I don't like this static index thing much. Can't you just make the
> "index" OF property mandatory? Mixing ways to number things can become
> very confusing. In particular as you are using dev->idx later to call
> i2c_add_numbered_adapter(), the caller is really supposed to know what
> they are doing with the bus numbers.
>   
I also don't really like mixing the two methods, hence the comment. I 
did this so existing dts files, which don't currently have an index, 
would work as is. Maybe it would be better to add the index?

Without my patch, or one like it, you cannot currently use the IBM IIC 
in the arch/powerpc branch, so maybe now *is* the time to enforce an index?

So if nobody responds with a good reason not to, I will change the code 
to require the index to be set. Less testing for me ;)
>> +	adap->timeout = 1;
>> +	adap->nr = dev->idx;
>>     
>
> Looks to me like the block above has much in common with the original
> probe function, maybe it would be worth sharing the code to make future
> maintenance easier?
>   
It is my understanding the the arch/ppc branch is going away in the 
middle of the year. So I kept the of platform code separate and clean 
expecting the CONFIG_IBM_OCP block to be removed in the near future. So 
future maintenance should not be an issue.
> Note that I cannot test the code so I am relying on the linuxppc-dev
> folks to test it
I can test the of platform code and the common code. I am not setup to 
test the OCP code, which is why I am loath to change it. I use the IBM 
IIC for access to an  ad7414 thermal chip (currently not in the kernel, 
I am working on that too) and for reading an eeprom.

Cheers,
   Sean

Re: [PATCH 2/2] i2c-ibm_iic driver

[Jean Delvare]

Hi Sean,

On Sat, 16 Feb 2008 15:54:14 -0500, Sean MacLennan wrote:
> Jean Delvare wrote:
> > Hi Sean,
> >
> > On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
> >   
> >> Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
> >>
> >> Cheers,
> >>    Sean
> >>
> >> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> >>     
> >
> > First of all: please run scripts/checkpatch.pl on your patch and fix
> > the reported errors. It tells me:
> >   total: 10 errors, 5 warnings, 222 lines checked
> > which is definitely too much.
> >   
> Many of the errors/warnings are from the cut and paste of the original 
> code. I realize this doesn't justify my new code, but does beg the 
> question; should I also cleanup the original code? And how much? I am 
> tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.

Just take care of the code you are adding. The old code will go away
soon anyway as I understand it, so don't bother cleaning it up.

> 
> A few comments, everything else I agree with.
> 
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	/* This assumes we don't mix index and non-index entries. */
> >> +	indexp = of_get_property(np, "index", NULL);
> >> +	dev->idx = indexp ? *indexp : index++;
> >>     
> >
> > I don't like this static index thing much. Can't you just make the
> > "index" OF property mandatory? Mixing ways to number things can become
> > very confusing. In particular as you are using dev->idx later to call
> > i2c_add_numbered_adapter(), the caller is really supposed to know what
> > they are doing with the bus numbers.
> >   
> I also don't really like mixing the two methods, hence the comment. I 
> did this so existing dts files, which don't currently have an index, 
> would work as is. Maybe it would be better to add the index?
> 
> Without my patch, or one like it, you cannot currently use the IBM IIC 
> in the arch/powerpc branch, so maybe now *is* the time to enforce an index?
> 
> So if nobody responds with a good reason not to, I will change the code 
> to require the index to be set. Less testing for me ;)

Yes, I think this is the right time to enforce the index.

> >> +	adap->timeout = 1;
> >> +	adap->nr = dev->idx;
> >>     
> >
> > Looks to me like the block above has much in common with the original
> > probe function, maybe it would be worth sharing the code to make future
> > maintenance easier?
> >   
> It is my understanding the the arch/ppc branch is going away in the 
> middle of the year. So I kept the of platform code separate and clean 
> expecting the CONFIG_IBM_OCP block to be removed in the near future. So 
> future maintenance should not be an issue.

OK, fine with me then.

> > Note that I cannot test the code so I am relying on the linuxppc-dev
> > folks to test it
> I can test the of platform code and the common code. I am not setup to 
> test the OCP code, which is why I am loath to change it. I use the IBM 
> IIC for access to an  ad7414 thermal chip (currently not in the kernel, 
> I am working on that too) and for reading an eeprom.

Note that Frank Edelhaeuser has submitted a driver for this chip
recently:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-February/022478.html
I didn't have time to review this patch yet, maybe you will want to do
it yourself.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c-ibm_iic driver

[Sean MacLennan]

An updated version of the patch. This one should answer all of Jean's 
concerns.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
---
--- for-2.6.25/drivers/i2c/busses/orig-i2c-ibm_iic.c	2008-02-18 16:36:30.000000000 -0500
+++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c	2008-02-18 17:39:53.000000000 -0500
@@ -6,6 +6,9 @@
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
  * Based on original work by
  * 	Ian DaSilva  <idasilva@mvista.com>
  *      Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
 #include <asm/ocp.h>
 #include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
 
 #include "i2c-ibm_iic.h"
 
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
 
 MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
 MODULE_LICENSE("GPL");
@@ -657,6 +665,7 @@
 	return (u8)((opb + 9) / 10 - 1);
 }
 
+#ifdef CONFIG_IBM_OCP
 /*
  * Register single IIC interface
  */
@@ -828,5 +837,182 @@
 	ocp_unregister_driver(&ibm_iic_driver);
 }
 
+#else  /* !CONFIG_IBM_OCP */
+
+static int __devinit iic_request_irq(struct of_device *ofdev,
+				     struct ibm_iic_private *dev)
+{
+	struct device_node *np = ofdev->node;
+	int irq;
+
+	if (iic_force_poll)
+		return NO_IRQ;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq == NO_IRQ) {
+		dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
+		return NO_IRQ;
+	}
+
+	/* Disable interrupts until we finish initialization, assumes
+	 *  level-sensitive IRQ setup...
+	 */
+	iic_interrupt_mode(dev, 0);
+	if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
+		dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
+		/* Fallback to the polling mode */
+		return NO_IRQ;
+	}
+
+	return irq;
+}
+
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+			       const struct of_device_id *match)
+{
+	struct device_node *np = ofdev->node;
+	struct ibm_iic_private *dev;
+	struct i2c_adapter *adap;
+	const u32 *indexp, *freq;
+	int ret;
+
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		dev_err(&ofdev->dev, "failed to allocate device data\n");
+		return -ENOMEM;
+	}
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	indexp = of_get_property(np, "index", NULL);
+	if (!indexp) {
+		dev_err(&ofdev->dev, "no index specified.\n");
+		ret = -EINVAL;
+		goto error_cleanup;
+	}
+	dev->idx = *indexp;
+
+	dev->vaddr = of_iomap(np, 0);
+	if (dev->vaddr == NULL) {
+		dev_err(&ofdev->dev, "failed to iomap device\n");
+		ret = -ENXIO;
+		goto error_cleanup;
+	}
+
+	init_waitqueue_head(&dev->wq);
+
+	dev->irq = iic_request_irq(ofdev, dev);
+	if (dev->irq == NO_IRQ)
+		dev_warn(&ofdev->dev, "using polling mode\n");
+
+	/* Board specific settings */
+	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+		dev->fast_mode = 1;
+
+	freq = of_get_property(np, "clock-frequency", NULL);
+	if (freq == NULL) {
+		freq = of_get_property(np->parent, "clock-frequency", NULL);
+		if (freq == NULL) {
+			dev_err(&ofdev->dev, "Unable to get bus frequency\n");
+			ret = -EBUSY;
+			goto error_cleanup;
+		}
+	}
+
+	dev->clckdiv = iic_clckdiv(*freq);
+	dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv);
+
+	/* Initialize IIC interface */
+	iic_dev_init(dev);
+
+	/* Register it with i2c layer */
+	adap = &dev->adap;
+	adap->dev.parent = &ofdev->dev;
+	strlcpy(adap->name, "IBM IIC", sizeof(adap->name));
+	i2c_set_adapdata(adap, dev);
+	adap->id = I2C_HW_OCP;
+	adap->class = I2C_CLASS_HWMON;
+	adap->algo = &iic_algo;
+	adap->timeout = 1;
+	adap->nr = dev->idx;
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret  < 0) {
+		dev_err(&ofdev->dev, "failed to register i2c adapter\n");
+		goto error_cleanup;
+	}
+
+	dev_dbg(&ofdev->dev, "using %s mode\n",
+		dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+	return 0;
+
+error_cleanup:
+	if (dev->irq != NO_IRQ) {
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	if (dev->vaddr)
+		iounmap(dev->vaddr);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
+	return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private *dev = dev_get_drvdata(&ofdev->dev);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+
+	i2c_del_adapter(&dev->adap);
+
+	if (dev->irq != NO_IRQ) {
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	iounmap(dev->vaddr);
+	kfree(dev);
+
+	return 0;
+}
+
+static const struct of_device_id ibm_iic_match[] = {
+	{ .compatible = "ibm,iic-405ex", },
+	{ .compatible = "ibm,iic-405gp", },
+	{ .compatible = "ibm,iic-440gp", },
+	{ .compatible = "ibm,iic-440gpx", },
+	{ .compatible = "ibm,iic-440grx", },
+	{}
+};
+
+static struct of_platform_driver ibm_iic_driver = {
+	.name	= "ibm-iic",
+	.match_table = ibm_iic_match,
+	.probe	= iic_probe,
+	.remove	= __devexit_p(iic_remove),
+};
+
+static int __init iic_init(void)
+{
+	return of_register_platform_driver(&ibm_iic_driver);
+}
+
+static void __exit iic_exit(void)
+{
+	of_unregister_platform_driver(&ibm_iic_driver);
+}
+#endif /* CONFIG_IBM_OCP */
+
 module_init(iic_init);
 module_exit(iic_exit);
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b61f56b..44c0984 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -244,7 +244,6 @@ config I2C_PIIX4
 
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
-	depends on IBM_OCP
 	help
 	  Say Y here if you want to use IIC peripheral found on 
 	  embedded IBM PPC 4xx based systems. 

Re: [PATCH 2/2] i2c-ibm_iic driver

[Jean Delvare]

Hi Sean,

On Mon, 18 Feb 2008 20:42:46 -0500, Sean MacLennan wrote:
> An updated version of the patch. This one should answer all of Jean's 
> concerns.
> 
> Cheers,
>    Sean
> 
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> ---
> --- for-2.6.25/drivers/i2c/busses/orig-i2c-ibm_iic.c	2008-02-18 16:36:30.000000000 -0500
> +++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c	2008-02-18 17:39:53.000000000 -0500
> @@ -6,6 +6,9 @@
>   * Copyright (c) 2003, 2004 Zultys Technologies.
>   * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
>   *
> + * Copyright (c) 2008 PIKA Technologies
> + * Sean MacLennan <smaclennan@pikatech.com>
> + *
>   * Based on original work by
>   * 	Ian DaSilva  <idasilva@mvista.com>
>   *      Armin Kuster <akuster@mvista.com>
> @@ -39,12 +42,17 @@
>  #include <asm/io.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-id.h>
> +
> +#ifdef CONFIG_IBM_OCP
>  #include <asm/ocp.h>
>  #include <asm/ibm4xx.h>
> +#else
> +#include <linux/of_platform.h>
> +#endif
>  
>  #include "i2c-ibm_iic.h"
>  
> -#define DRIVER_VERSION "2.1"
> +#define DRIVER_VERSION "2.2"
>  
>  MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
>  MODULE_LICENSE("GPL");
> @@ -657,6 +665,7 @@
>  	return (u8)((opb + 9) / 10 - 1);
>  }
>  
> +#ifdef CONFIG_IBM_OCP
>  /*
>   * Register single IIC interface
>   */
> @@ -828,5 +837,182 @@
>  	ocp_unregister_driver(&ibm_iic_driver);
>  }
>  
> +#else  /* !CONFIG_IBM_OCP */
> +
> +static int __devinit iic_request_irq(struct of_device *ofdev,
> +				     struct ibm_iic_private *dev)
> +{
> +	struct device_node *np = ofdev->node;
> +	int irq;
> +
> +	if (iic_force_poll)
> +		return NO_IRQ;
> +
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (irq == NO_IRQ) {
> +		dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
> +		return NO_IRQ;
> +	}
> +
> +	/* Disable interrupts until we finish initialization, assumes
> +	 *  level-sensitive IRQ setup...
> +	 */
> +	iic_interrupt_mode(dev, 0);
> +	if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
> +		dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
> +		/* Fallback to the polling mode */
> +		return NO_IRQ;
> +	}
> +
> +	return irq;
> +}
> +
> +/*
> + * Register single IIC interface
> + */
> +static int __devinit iic_probe(struct of_device *ofdev,
> +			       const struct of_device_id *match)
> +{
> +	struct device_node *np = ofdev->node;
> +	struct ibm_iic_private *dev;
> +	struct i2c_adapter *adap;
> +	const u32 *indexp, *freq;
> +	int ret;
> +
> +

Double blank line is prohibited inside a function.

> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev) {
> +		dev_err(&ofdev->dev, "failed to allocate device data\n");
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(&ofdev->dev, dev);
> +
> +	indexp = of_get_property(np, "index", NULL);
> +	if (!indexp) {
> +		dev_err(&ofdev->dev, "no index specified.\n");

This is the only error message that has a trailing dot. Remove it?

> +		ret = -EINVAL;

Isn't it a bit inconsistent to return -EINVAL here...

> +		goto error_cleanup;
> +	}
> +	dev->idx = *indexp;
> +
> +	dev->vaddr = of_iomap(np, 0);
> +	if (dev->vaddr == NULL) {
> +		dev_err(&ofdev->dev, "failed to iomap device\n");
> +		ret = -ENXIO;
> +		goto error_cleanup;
> +	}
> +
> +	init_waitqueue_head(&dev->wq);
> +
> +	dev->irq = iic_request_irq(ofdev, dev);
> +	if (dev->irq == NO_IRQ)
> +		dev_warn(&ofdev->dev, "using polling mode\n");
> +
> +	/* Board specific settings */
> +	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
> +		dev->fast_mode = 1;
> +
> +	freq = of_get_property(np, "clock-frequency", NULL);
> +	if (freq == NULL) {
> +		freq = of_get_property(np->parent, "clock-frequency", NULL);
> +		if (freq == NULL) {
> +			dev_err(&ofdev->dev, "Unable to get bus frequency\n");
> +			ret = -EBUSY;

... but -EBUSY there?

> +			goto error_cleanup;
> +		}
> +	}
> +
> +	dev->clckdiv = iic_clckdiv(*freq);
> +	dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv);
> +
> +	/* Initialize IIC interface */
> +	iic_dev_init(dev);
> +
> +	/* Register it with i2c layer */
> +	adap = &dev->adap;
> +	adap->dev.parent = &ofdev->dev;
> +	strlcpy(adap->name, "IBM IIC", sizeof(adap->name));
> +	i2c_set_adapdata(adap, dev);
> +	adap->id = I2C_HW_OCP;
> +	adap->class = I2C_CLASS_HWMON;
> +	adap->algo = &iic_algo;
> +	adap->timeout = 1;
> +	adap->nr = dev->idx;
> +
> +	ret = i2c_add_numbered_adapter(adap);
> +	if (ret  < 0) {
> +		dev_err(&ofdev->dev, "failed to register i2c adapter\n");
> +		goto error_cleanup;
> +	}
> +
> +	dev_dbg(&ofdev->dev, "using %s mode\n",

dev_info?

> +		dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
> +
> +	return 0;
> +
> +error_cleanup:
> +	if (dev->irq != NO_IRQ) {
> +		iic_interrupt_mode(dev, 0);
> +		free_irq(dev->irq, dev);
> +	}
> +
> +	if (dev->vaddr)
> +		iounmap(dev->vaddr);
> +
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +	kfree(dev);
> +	return ret;
> +}
> +
> +/*
> + * Cleanup initialized IIC interface
> + */
> +static int __devexit iic_remove(struct of_device *ofdev)
> +{
> +	struct ibm_iic_private *dev = dev_get_drvdata(&ofdev->dev);
> +
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +
> +	i2c_del_adapter(&dev->adap);
> +
> +	if (dev->irq != NO_IRQ) {
> +		iic_interrupt_mode(dev, 0);
> +		free_irq(dev->irq, dev);
> +	}
> +
> +	iounmap(dev->vaddr);
> +	kfree(dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ibm_iic_match[] = {
> +	{ .compatible = "ibm,iic-405ex", },
> +	{ .compatible = "ibm,iic-405gp", },
> +	{ .compatible = "ibm,iic-440gp", },
> +	{ .compatible = "ibm,iic-440gpx", },
> +	{ .compatible = "ibm,iic-440grx", },
> +	{}
> +};
> +
> +static struct of_platform_driver ibm_iic_driver = {
> +	.name	= "ibm-iic",
> +	.match_table = ibm_iic_match,
> +	.probe	= iic_probe,
> +	.remove	= __devexit_p(iic_remove),
> +};
> +
> +static int __init iic_init(void)
> +{
> +	return of_register_platform_driver(&ibm_iic_driver);
> +}
> +
> +static void __exit iic_exit(void)
> +{
> +	of_unregister_platform_driver(&ibm_iic_driver);
> +}
> +#endif /* CONFIG_IBM_OCP */
> +
>  module_init(iic_init);
>  module_exit(iic_exit);
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index b61f56b..44c0984 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -244,7 +244,6 @@ config I2C_PIIX4
>  
>  config I2C_IBM_IIC
>  	tristate "IBM PPC 4xx on-chip I2C interface"
> -	depends on IBM_OCP
>  	help
>  	  Say Y here if you want to use IIC peripheral found on 
>  	  embedded IBM PPC 4xx based systems. 

With this Kconfig change, "make menuconfig" lets me select the
i2c-ibm_iic driver on x86_64, but it fails to build horribly. I think
that you want to restrict the build to PPC machines somehow, or at
least make sure that either IBM_OCP or OF support is present.

The rest looks fine to me. If you can address my last few comments, I
can push your 2 i2c-ibm_iic patches to -mm.

Thanks,
-- 
Jean Delvare

Re: [PATCH 2/2] i2c-ibm_iic driver

[Stefan Roese]

On Tuesday 19 February 2008, Jean Delvare wrote:
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index b61f56b..44c0984 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -244,7 +244,6 @@ config I2C_PIIX4
> >
> >  config I2C_IBM_IIC
> >  	tristate "IBM PPC 4xx on-chip I2C interface"
> > -	depends on IBM_OCP
> >  	help
> >  	  Say Y here if you want to use IIC peripheral found on
> >  	  embedded IBM PPC 4xx based systems.
>
> With this Kconfig change, "make menuconfig" lets me select the
> i2c-ibm_iic driver on x86_64, but it fails to build horribly. I think
> that you want to restrict the build to PPC machines somehow, or at
> least make sure that either IBM_OCP or OF support is present.

How about this:

-	depends on IBM_OCP
+	depends on 4xx

Best regards,
Stefan

Re: [PATCH 2/2] i2c-ibm_iic driver

[Sean MacLennan]

Stefan Roese wrote:
> How about this:
>
> -	depends on IBM_OCP
> +	depends on 4xx
>   
That's what I did, great minds think alike ;) But the email does not 
seem to have come through, so I will repost the patch. If you already 
got an email with the above patch, you can ignore this one ;)

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>

--- for-2.6.25/drivers/i2c/busses/orig-i2c-ibm_iic.c	2008-02-18 16:36:30.000000000 -0500
+++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c	2008-02-19 16:46:35.000000000 -0500
@@ -6,6 +6,9 @@
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
  * Based on original work by
  * 	Ian DaSilva  <idasilva@mvista.com>
  *      Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
 #include <asm/ocp.h>
 #include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
 
 #include "i2c-ibm_iic.h"
 
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
 
 MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
 MODULE_LICENSE("GPL");
@@ -657,6 +665,7 @@
 	return (u8)((opb + 9) / 10 - 1);
 }
 
+#ifdef CONFIG_IBM_OCP
 /*
  * Register single IIC interface
  */
@@ -828,5 +837,181 @@
 	ocp_unregister_driver(&ibm_iic_driver);
 }
 
+#else  /* !CONFIG_IBM_OCP */
+
+static int __devinit iic_request_irq(struct of_device *ofdev,
+				     struct ibm_iic_private *dev)
+{
+	struct device_node *np = ofdev->node;
+	int irq;
+
+	if (iic_force_poll)
+		return NO_IRQ;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq == NO_IRQ) {
+		dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
+		return NO_IRQ;
+	}
+
+	/* Disable interrupts until we finish initialization, assumes
+	 *  level-sensitive IRQ setup...
+	 */
+	iic_interrupt_mode(dev, 0);
+	if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
+		dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
+		/* Fallback to the polling mode */
+		return NO_IRQ;
+	}
+
+	return irq;
+}
+
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+			       const struct of_device_id *match)
+{
+	struct device_node *np = ofdev->node;
+	struct ibm_iic_private *dev;
+	struct i2c_adapter *adap;
+	const u32 *indexp, *freq;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		dev_err(&ofdev->dev, "failed to allocate device data\n");
+		return -ENOMEM;
+	}
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	indexp = of_get_property(np, "index", NULL);
+	if (!indexp) {
+		dev_err(&ofdev->dev, "no index specified\n");
+		ret = -EINVAL;
+		goto error_cleanup;
+	}
+	dev->idx = *indexp;
+
+	dev->vaddr = of_iomap(np, 0);
+	if (dev->vaddr == NULL) {
+		dev_err(&ofdev->dev, "failed to iomap device\n");
+		ret = -ENXIO;
+		goto error_cleanup;
+	}
+
+	init_waitqueue_head(&dev->wq);
+
+	dev->irq = iic_request_irq(ofdev, dev);
+	if (dev->irq == NO_IRQ)
+		dev_warn(&ofdev->dev, "using polling mode\n");
+
+	/* Board specific settings */
+	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+		dev->fast_mode = 1;
+
+	freq = of_get_property(np, "clock-frequency", NULL);
+	if (freq == NULL) {
+		freq = of_get_property(np->parent, "clock-frequency", NULL);
+		if (freq == NULL) {
+			dev_err(&ofdev->dev, "Unable to get bus frequency\n");
+			ret = -EINVAL;
+			goto error_cleanup;
+		}
+	}
+
+	dev->clckdiv = iic_clckdiv(*freq);
+	dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv);
+
+	/* Initialize IIC interface */
+	iic_dev_init(dev);
+
+	/* Register it with i2c layer */
+	adap = &dev->adap;
+	adap->dev.parent = &ofdev->dev;
+	strlcpy(adap->name, "IBM IIC", sizeof(adap->name));
+	i2c_set_adapdata(adap, dev);
+	adap->id = I2C_HW_OCP;
+	adap->class = I2C_CLASS_HWMON;
+	adap->algo = &iic_algo;
+	adap->timeout = 1;
+	adap->nr = dev->idx;
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret  < 0) {
+		dev_err(&ofdev->dev, "failed to register i2c adapter\n");
+		goto error_cleanup;
+	}
+
+	dev_info(&ofdev->dev, "using %s mode\n",
+		 dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+	return 0;
+
+error_cleanup:
+	if (dev->irq != NO_IRQ) {
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	if (dev->vaddr)
+		iounmap(dev->vaddr);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
+	return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private *dev = dev_get_drvdata(&ofdev->dev);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+
+	i2c_del_adapter(&dev->adap);
+
+	if (dev->irq != NO_IRQ) {
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	iounmap(dev->vaddr);
+	kfree(dev);
+
+	return 0;
+}
+
+static const struct of_device_id ibm_iic_match[] = {
+	{ .compatible = "ibm,iic-405ex", },
+	{ .compatible = "ibm,iic-405gp", },
+	{ .compatible = "ibm,iic-440gp", },
+	{ .compatible = "ibm,iic-440gpx", },
+	{ .compatible = "ibm,iic-440grx", },
+	{}
+};
+
+static struct of_platform_driver ibm_iic_driver = {
+	.name	= "ibm-iic",
+	.match_table = ibm_iic_match,
+	.probe	= iic_probe,
+	.remove	= __devexit_p(iic_remove),
+};
+
+static int __init iic_init(void)
+{
+	return of_register_platform_driver(&ibm_iic_driver);
+}
+
+static void __exit iic_exit(void)
+{
+	of_unregister_platform_driver(&ibm_iic_driver);
+}
+#endif /* CONFIG_IBM_OCP */
+
 module_init(iic_init);
 module_exit(iic_exit);
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b61f56b..bda87a0 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -244,7 +244,7 @@ config I2C_PIIX4
 
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
-	depends on IBM_OCP
+	depends on 4xx
 	help
 	  Say Y here if you want to use IIC peripheral found on 
 	  embedded IBM PPC 4xx based systems. 

Re: [PATCH 2/2] i2c-ibm_iic driver

[Arnd Bergmann]

On Tuesday 19 February 2008, Stefan Roese wrote:
> On Tuesday 19 February 2008, Jean Delvare wrote:
> >
> > With this Kconfig change, "make menuconfig" lets me select the
> > i2c-ibm_iic driver on x86_64, but it fails to build horribly. I think
> > that you want to restrict the build to PPC machines somehow, or at
> > least make sure that either IBM_OCP or OF support is present.
>=20
> How about this:
>=20
> -=A0=A0=A0=A0=A0=A0=A0depends on IBM_OCP
> +=A0=A0=A0=A0=A0=A0=A0depends on 4xx

I think we should allow it to be built on other platforms as well,
as long as they have of_platform_device support.

The Axon south bridge used on IBMs QS21 blade probably has an ibm_iic,
even though it's managed by the firmware and we probably don't want
to use it at this time, someone could use the same chip in a new
design and actually do that.

In general, I also like to make it possible to enable drivers just
for the benefit of compile testing, even for stuff that you can't
find in any existing HW configuration, so as long as it builds on
a platform, I think we shouldn't forbid it:

=2D       depends on IBM_OCP
+       depends on IBM_OCP || PPC_MERGE

	Arnd <><

Re: [PATCH 2/2] i2c-ibm_iic driver

[Sean MacLennan]

Arnd Bergmann wrote:
> On Tuesday 19 February 2008, Stefan Roese wrote:
>   
>> On Tuesday 19 February 2008, Jean Delvare wrote:
>>     
>>> With this Kconfig change, "make menuconfig" lets me select the
>>> i2c-ibm_iic driver on x86_64, but it fails to build horribly. I think
>>> that you want to restrict the build to PPC machines somehow, or at
>>> least make sure that either IBM_OCP or OF support is present.
>>>       
>> How about this:
>>
>> -       depends on IBM_OCP
>> +       depends on 4xx
>>     
>
> I think we should allow it to be built on other platforms as well,
> as long as they have of_platform_device support.
>
> The Axon south bridge used on IBMs QS21 blade probably has an ibm_iic,
> even though it's managed by the firmware and we probably don't want
> to use it at this time, someone could use the same chip in a new
> design and actually do that.
>
> In general, I also like to make it possible to enable drivers just
> for the benefit of compile testing, even for stuff that you can't
> find in any existing HW configuration, so as long as it builds on
> a platform, I think we shouldn't forbid it:
>
> -       depends on IBM_OCP
> +       depends on IBM_OCP || PPC_MERGE
>
>   
I have no problem with this change. If everybody agrees, I can respin 
the patch.

Cheers,
   Sean

Re: [PATCH 2/2] i2c-ibm_iic driver

[Stephen Rothwell]

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

> > -       depends on IBM_OCP
> > +       depends on IBM_OCP || PPC_MERGE

not PPC_OF?  or even OF (give the sparc guys the opportunity to build it
for us :-))?

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

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

Re: [PATCH 2/2] i2c-ibm_iic driver

[Arnd Bergmann]

On Wednesday 20 February 2008, Stephen Rothwell wrote:
> > > - =A0 =A0 =A0 depends on IBM_OCP
> > > + =A0 =A0 =A0 depends on IBM_OCP || PPC_MERGE
>=20
> not PPC_OF? =A0or even OF (give the sparc guys the opportunity to build it
> for us :-))?
>=20

Right, I was looking for that option but couldn't find it. I would
guess sparc doesn't work because it's lacking the necessary I/O functions,
in_8 and out_8 that are powerpc specific.

so maybe:
	depends on PPC
	depends on IBM_OCP || OF

	Arnd <><

Re: [PATCH 2/2] i2c-ibm_iic driver

[Jean Delvare]

Hi Arnd,

On Tue, 19 Feb 2008 23:55:16 +0100, Arnd Bergmann wrote:
> On Tuesday 19 February 2008, Stefan Roese wrote:
> > On Tuesday 19 February 2008, Jean Delvare wrote:
> > >
> > > With this Kconfig change, "make menuconfig" lets me select the
> > > i2c-ibm_iic driver on x86_64, but it fails to build horribly. I think
> > > that you want to restrict the build to PPC machines somehow, or at
> > > least make sure that either IBM_OCP or OF support is present.
> >=20
> > How about this:
> >=20
> > -=A0=A0=A0=A0=A0=A0=A0depends on IBM_OCP
> > +=A0=A0=A0=A0=A0=A0=A0depends on 4xx
>=20
> I think we should allow it to be built on other platforms as well,
> as long as they have of_platform_device support.
>=20
> The Axon south bridge used on IBMs QS21 blade probably has an ibm_iic,
> even though it's managed by the firmware and we probably don't want
> to use it at this time, someone could use the same chip in a new
> design and actually do that.
>=20
> In general, I also like to make it possible to enable drivers just
> for the benefit of compile testing, even for stuff that you can't
> find in any existing HW configuration, so as long as it builds on
> a platform, I think we shouldn't forbid it:

Fine with me as long as the default is set appropriately (i.e. default
to not building the driver on archs/platforms where it builds but is
known to be useless.)

>=20
> -       depends on IBM_OCP
> +       depends on IBM_OCP || PPC_MERGE

--=20
Jean Delvare

Re: [PATCH 2/2] i2c-ibm_iic driver

[Sean MacLennan]

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>

--- for-2.6.25/drivers/i2c/busses/orig-i2c-ibm_iic.c	2008-02-18 16:36:30.000000000 -0500
+++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c	2008-02-19 16:46:35.000000000 -0500
@@ -6,6 +6,9 @@
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
  * Based on original work by
  * 	Ian DaSilva  <idasilva@mvista.com>
  *      Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
 #include <asm/ocp.h>
 #include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
 
 #include "i2c-ibm_iic.h"
 
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
 
 MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
 MODULE_LICENSE("GPL");
@@ -657,6 +665,7 @@
 	return (u8)((opb + 9) / 10 - 1);
 }
 
+#ifdef CONFIG_IBM_OCP
 /*
  * Register single IIC interface
  */
@@ -828,5 +837,181 @@
 	ocp_unregister_driver(&ibm_iic_driver);
 }
 
+#else  /* !CONFIG_IBM_OCP */
+
+static int __devinit iic_request_irq(struct of_device *ofdev,
+				     struct ibm_iic_private *dev)
+{
+	struct device_node *np = ofdev->node;
+	int irq;
+
+	if (iic_force_poll)
+		return NO_IRQ;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq == NO_IRQ) {
+		dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
+		return NO_IRQ;
+	}
+
+	/* Disable interrupts until we finish initialization, assumes
+	 *  level-sensitive IRQ setup...
+	 */
+	iic_interrupt_mode(dev, 0);
+	if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
+		dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
+		/* Fallback to the polling mode */
+		return NO_IRQ;
+	}
+
+	return irq;
+}
+
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+			       const struct of_device_id *match)
+{
+	struct device_node *np = ofdev->node;
+	struct ibm_iic_private *dev;
+	struct i2c_adapter *adap;
+	const u32 *indexp, *freq;
+	int ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		dev_err(&ofdev->dev, "failed to allocate device data\n");
+		return -ENOMEM;
+	}
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	indexp = of_get_property(np, "index", NULL);
+	if (!indexp) {
+		dev_err(&ofdev->dev, "no index specified\n");
+		ret = -EINVAL;
+		goto error_cleanup;
+	}
+	dev->idx = *indexp;
+
+	dev->vaddr = of_iomap(np, 0);
+	if (dev->vaddr == NULL) {
+		dev_err(&ofdev->dev, "failed to iomap device\n");
+		ret = -ENXIO;
+		goto error_cleanup;
+	}
+
+	init_waitqueue_head(&dev->wq);
+
+	dev->irq = iic_request_irq(ofdev, dev);
+	if (dev->irq == NO_IRQ)
+		dev_warn(&ofdev->dev, "using polling mode\n");
+
+	/* Board specific settings */
+	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+		dev->fast_mode = 1;
+
+	freq = of_get_property(np, "clock-frequency", NULL);
+	if (freq == NULL) {
+		freq = of_get_property(np->parent, "clock-frequency", NULL);
+		if (freq == NULL) {
+			dev_err(&ofdev->dev, "Unable to get bus frequency\n");
+			ret = -EINVAL;
+			goto error_cleanup;
+		}
+	}
+
+	dev->clckdiv = iic_clckdiv(*freq);
+	dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv);
+
+	/* Initialize IIC interface */
+	iic_dev_init(dev);
+
+	/* Register it with i2c layer */
+	adap = &dev->adap;
+	adap->dev.parent = &ofdev->dev;
+	strlcpy(adap->name, "IBM IIC", sizeof(adap->name));
+	i2c_set_adapdata(adap, dev);
+	adap->id = I2C_HW_OCP;
+	adap->class = I2C_CLASS_HWMON;
+	adap->algo = &iic_algo;
+	adap->timeout = 1;
+	adap->nr = dev->idx;
+
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret  < 0) {
+		dev_err(&ofdev->dev, "failed to register i2c adapter\n");
+		goto error_cleanup;
+	}
+
+	dev_info(&ofdev->dev, "using %s mode\n",
+		 dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+	return 0;
+
+error_cleanup:
+	if (dev->irq != NO_IRQ) {
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	if (dev->vaddr)
+		iounmap(dev->vaddr);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
+	return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private *dev = dev_get_drvdata(&ofdev->dev);
+
+	dev_set_drvdata(&ofdev->dev, NULL);
+
+	i2c_del_adapter(&dev->adap);
+
+	if (dev->irq != NO_IRQ) {
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	iounmap(dev->vaddr);
+	kfree(dev);
+
+	return 0;
+}
+
+static const struct of_device_id ibm_iic_match[] = {
+	{ .compatible = "ibm,iic-405ex", },
+	{ .compatible = "ibm,iic-405gp", },
+	{ .compatible = "ibm,iic-440gp", },
+	{ .compatible = "ibm,iic-440gpx", },
+	{ .compatible = "ibm,iic-440grx", },
+	{}
+};
+
+static struct of_platform_driver ibm_iic_driver = {
+	.name	= "ibm-iic",
+	.match_table = ibm_iic_match,
+	.probe	= iic_probe,
+	.remove	= __devexit_p(iic_remove),
+};
+
+static int __init iic_init(void)
+{
+	return of_register_platform_driver(&ibm_iic_driver);
+}
+
+static void __exit iic_exit(void)
+{
+	of_unregister_platform_driver(&ibm_iic_driver);
+}
+#endif /* CONFIG_IBM_OCP */
+
 module_init(iic_init);
 module_exit(iic_exit);
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b61f56b..bda87a0 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -244,7 +244,7 @@ config I2C_PIIX4
 
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
-	depends on IBM_OCP
+	depends on 4xx
 	help
 	  Say Y here if you want to use IIC peripheral found on 
 	  embedded IBM PPC 4xx based systems. 

Re: [PATCH 2/2] i2c-ibm_iic driver

[Jean Delvare]

Hi Sean,

On Tue, 19 Feb 2008 16:58:01 -0500, Sean MacLennan wrote:
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> 
> --- for-2.6.25/drivers/i2c/busses/orig-i2c-ibm_iic.c	2008-02-18 16:36:30.000000000 -0500
> +++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c	2008-02-19 16:46:35.000000000 -0500
> @@ -6,6 +6,9 @@
>   * Copyright (c) 2003, 2004 Zultys Technologies.
>   * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
>   *
> + * Copyright (c) 2008 PIKA Technologies
> + * Sean MacLennan <smaclennan@pikatech.com>
> + *
>   * Based on original work by
>   * 	Ian DaSilva  <idasilva@mvista.com>
>   *      Armin Kuster <akuster@mvista.com>
> (...)
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -244,7 +244,7 @@ config I2C_PIIX4
>  
>  config I2C_IBM_IIC
>  	tristate "IBM PPC 4xx on-chip I2C interface"
> -	depends on IBM_OCP
> +	depends on 4xx
>  	help
>  	  Say Y here if you want to use IIC peripheral found on 
>  	  embedded IBM PPC 4xx based systems. 

I've applied this version. I see that there are discussions going on
about the best "depends on" statement in Kconfig, feel free to submit
an updated (or incremental) patch as soon as a decision will have been
taken.

Thanks,
-- 
Jean Delvare

[PATCH] i2c-ibm_iic driver

[Sean MacLennan]

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

I converted the i2c-ibm_iic driver from an ocp driver to an of_platform 
driver. Since this driver is in the kernel.org kernel, should I rename 
it and keep the old one around? I notice this was done with the emac 
network driver.

This driver is required for the taco platform.

Cheers,
    Sean


[-- Attachment #2: i2c-patch --]
[-- Type: text/plain, Size: 7545 bytes --]

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c466c6c..e9e1493 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -241,7 +241,6 @@ config I2C_PIIX4
 
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
-	depends on IBM_OCP
 	help
 	  Say Y here if you want to use IIC peripheral found on 
 	  embedded IBM PPC 4xx based systems. 
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 9b43ff7..838006f 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -3,6 +3,10 @@
  *
  * Support for the IIC peripheral on IBM PPC 4xx
  *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *  Converted to an of_platform_driver.
+ *
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
@@ -39,12 +43,11 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
-#include <asm/ocp.h>
-#include <asm/ibm4xx.h>
+#include <linux/of_platform.h>
 
 #include "i2c-ibm_iic.h"
 
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
 
 MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
 MODULE_LICENSE("GPL");
@@ -660,50 +663,58 @@ static inline u8 iic_clckdiv(unsigned int opb)
 /*
  * Register single IIC interface
  */
-static int __devinit iic_probe(struct ocp_device *ocp){
-
+static int __devinit iic_probe(struct of_device *ofdev,
+							   const struct of_device_id *match)
+{
+	static int index = 0;
+	struct device_node *np = ofdev->node;
 	struct ibm_iic_private* dev;
 	struct i2c_adapter* adap;
-	struct ocp_func_iic_data* iic_data = ocp->def->additions;
+	const u32 *addrp, *freq;
+	u64 addr;
 	int ret;
-	
-	if (!iic_data)
-		printk(KERN_WARNING"ibm-iic%d: missing additional data!\n",
-			ocp->def->index);
 
 	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
-		printk(KERN_CRIT "ibm-iic%d: failed to allocate device data\n",
-			ocp->def->index);
+		printk(KERN_CRIT "ibm-iic: failed to allocate device data\n");
 		return -ENOMEM;
 	}
 
-	dev->idx = ocp->def->index;
-	ocp_set_drvdata(ocp, dev);
-	
-	if (!request_mem_region(ocp->def->paddr, sizeof(struct iic_regs),
-				"ibm_iic")) {
+	dev->idx = index++;
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	if((addrp = of_get_address(np, 0, NULL, NULL)) == NULL ||
+	   (addr = of_translate_address(np, addrp)) == OF_BAD_ADDR) {
+		printk(KERN_CRIT "ibm-iic%d: Unable to get iic address\n",
+			   dev->idx);
 		ret = -EBUSY;
 		goto fail1;
 	}
 
-	if (!(dev->vaddr = ioremap(ocp->def->paddr, sizeof(struct iic_regs)))){
+	if (!(dev->vaddr = ioremap(addr, sizeof(struct iic_regs)))){
 		printk(KERN_CRIT "ibm-iic%d: failed to ioremap device registers\n",
 			dev->idx);
 		ret = -ENXIO;
-		goto fail2;
+		goto fail1;
 	}
 	
 	init_waitqueue_head(&dev->wq);
 
-	dev->irq = iic_force_poll ? -1 : ocp->def->irq;
-	if (dev->irq >= 0){
+	if(iic_force_poll)
+		dev->irq = -1;
+	else if((dev->irq = irq_of_parse_and_map(np, 0)) == NO_IRQ) {
+		printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
+		dev->irq = -1;
+	}
+
+	if (dev->irq >= 0) {
 		/* Disable interrupts until we finish initialization,
 		   assumes level-sensitive IRQ setup...
 		 */
 		iic_interrupt_mode(dev, 0);
-		if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)){
+		if(request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)){
 			printk(KERN_ERR "ibm-iic%d: request_irq %d failed\n", 
-				dev->idx, dev->irq);
+				   dev->idx, dev->irq);
 			/* Fallback to the polling mode */	
 			dev->irq = -1;
 		}
@@ -711,23 +722,30 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 	
 	if (dev->irq < 0)
 		printk(KERN_WARNING "ibm-iic%d: using polling mode\n", 
-			dev->idx);
+			   dev->idx);
 		
 	/* Board specific settings */
-	dev->fast_mode = iic_force_fast ? 1 : (iic_data ? iic_data->fast_mode : 0);
+	dev->fast_mode = iic_force_fast ? 1 : 0;
 	
-	/* clckdiv is the same for *all* IIC interfaces, 
-	 * but I'd rather make a copy than introduce another global. --ebs
+	/* clckdiv is the same for *all* IIC interfaces, but I'd rather
+	 * make a copy than introduce another global. --ebs
 	 */
-	dev->clckdiv = iic_clckdiv(ocp_sys_info.opb_bus_freq);
+	if((freq = of_get_property(np, "clock-frequency", NULL)) == NULL &&
+	   (freq = of_get_property(np->parent, "clock-frequency", NULL)) == NULL) {
+		printk(KERN_CRIT "ibm-iic%d: Unable to get bus frequency\n", dev->idx);
+		ret = -EBUSY;
+		goto fail;
+	}
+
+	dev->clckdiv = iic_clckdiv(*freq);
 	DBG("%d: clckdiv = %d\n", dev->idx, dev->clckdiv);
-	
+
 	/* Initialize IIC interface */
 	iic_dev_init(dev);
 	
 	/* Register it with i2c layer */
 	adap = &dev->adap;
-	adap->dev.parent = &ocp->dev;
+	adap->dev.parent = &ofdev->dev;
 	strcpy(adap->name, "IBM IIC");
 	i2c_set_adapdata(adap, dev);
 	adap->id = I2C_HW_OCP;
@@ -737,13 +755,7 @@ static int __devinit iic_probe(struct ocp_device *ocp){
 	adap->client_unregister = NULL;
 	adap->timeout = 1;
 	adap->retries = 1;
-
-	/*
-	 * If "dev->idx" is negative we consider it as zero.
-	 * The reason to do so is to avoid sysfs names that only make
-	 * sense when there are multiple adapters.
-	 */
-	adap->nr = dev->idx >= 0 ? dev->idx : 0;
+	adap->nr = dev->idx;
 
 	if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
 		printk(KERN_CRIT "ibm-iic%d: failed to register i2c adapter\n",
@@ -763,20 +775,19 @@ fail:
 	}	
 
 	iounmap(dev->vaddr);
-fail2:	
-	release_mem_region(ocp->def->paddr, sizeof(struct iic_regs));
 fail1:
-	ocp_set_drvdata(ocp, NULL);
-	kfree(dev);	
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
 	return ret;
 }
 
 /*
  * Cleanup initialized IIC interface
  */
-static void __devexit iic_remove(struct ocp_device *ocp)
+static int __devexit iic_remove(struct of_device *ofdev)
 {
-	struct ibm_iic_private* dev = (struct ibm_iic_private*)ocp_get_drvdata(ocp);
+	struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_get_drvdata(&ofdev->dev);
+
 	BUG_ON(dev == NULL);
 	if (i2c_del_adapter(&dev->adap)){
 		printk(KERN_CRIT "ibm-iic%d: failed to delete i2c adapter :(\n",
@@ -793,41 +804,37 @@ static void __devexit iic_remove(struct ocp_device *ocp)
 		    free_irq(dev->irq, dev);
 		}
 		iounmap(dev->vaddr);
-		release_mem_region(ocp->def->paddr, sizeof(struct iic_regs));
 		kfree(dev);
 	}
+
+	return 0;
 }
 
-static struct ocp_device_id ibm_iic_ids[] __devinitdata = 
+
+static struct of_device_id ibm_iic_match[] =
 {
-	{ .vendor = OCP_VENDOR_IBM, .function = OCP_FUNC_IIC },
-	{ .vendor = OCP_VENDOR_INVALID }
+	{ .type = "i2c", .compatible = "ibm,iic", },
+	{}
 };
 
-MODULE_DEVICE_TABLE(ocp, ibm_iic_ids);
-
-static struct ocp_driver ibm_iic_driver =
+static struct of_platform_driver ibm_iic_driver =
 {
-	.name 		= "iic",
-	.id_table	= ibm_iic_ids,
+	.name  = "ibm-iic",
+	.match_table = ibm_iic_match,
+
 	.probe		= iic_probe,
-	.remove		= __devexit_p(iic_remove),
-#if defined(CONFIG_PM)
-	.suspend	= NULL,
-	.resume		= NULL,
-#endif
+	.remove		= iic_remove,
 };
 
-static int __init iic_init(void)
+static int __init ibm_iic_init(void)
 {
 	printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
-	return ocp_register_driver(&ibm_iic_driver);
+	return of_register_platform_driver(&ibm_iic_driver);
 }
+module_init(ibm_iic_init);
 
-static void __exit iic_exit(void)
+static void __exit ibm_iic_exit(void)
 {
-	ocp_unregister_driver(&ibm_iic_driver);
+	of_unregister_platform_driver(&ibm_iic_driver);
 }
-
-module_init(iic_init);
-module_exit(iic_exit);
+module_exit(ibm_iic_exit);

Re: [PATCH] i2c-ibm_iic driver

[Arnd Bergmann]

On Saturday 05 January 2008, Sean MacLennan wrote:
> I converted the i2c-ibm_iic driver from an ocp driver to an of_platform=20
> driver. Since this driver is in the kernel.org kernel, should I rename=20
> it and keep the old one around? I notice this was done with the emac=20
> network driver.

The interesting question is whether there are any functional users in
arch/ppc left for the original driver. If all platforms that used
to use i2c-ibm_iic are broken already, you can can go ahead with
the conversion. Otherwise, there are two options:

1. duplicate the driver like you suggested
2. make the same driver both a ocp and of_platform, depending on
the configuration options.

Since most of the driver is untouched by your patch, I'd lean to
the second option, but of course, that is more work for you.

Your patch otherwise looks good, but I have a few comments on
details:

> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -241,7 +241,6 @@ config I2C_PIIX4
> =A0
> =A0config I2C_IBM_IIC
> =A0=A0=A0=A0=A0=A0=A0=A0tristate "IBM PPC 4xx on-chip I2C interface"
> -=A0=A0=A0=A0=A0=A0=A0depends on IBM_OCP
> =A0=A0=A0=A0=A0=A0=A0=A0help
> =A0=A0=A0=A0=A0=A0=A0=A0 =A0Say Y here if you want to use IIC peripheral =
found on=20
> =A0=A0=A0=A0=A0=A0=A0=A0 =A0embedded IBM PPC 4xx based systems.=20

In any way, this one now needs to depend on PPC_MERGE now, you
could even make it depend on PPC_4xx, but it's often better to
allow building the driver when you can, even if it doesn't make
sense on your hardware. This gives a better compile coverage.

> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ib=
m_iic.c
> index 9b43ff7..838006f 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -3,6 +3,10 @@
> =A0 *
> =A0 * Support for the IIC peripheral on IBM PPC 4xx
> =A0 *
> + * Copyright (c) 2008 PIKA Technologies
> + * Sean MacLennan <smaclennan@pikatech.com>
> + * =A0Converted to an of_platform_driver.
> + *

Changelogs in the file itself are discouraged. In this case, it's just
one line, so it's not really harmful.

I think the convention is for copyrights to be in chronological order,
so you should put the copyright below Eugene's.

> =A0 * Copyright (c) 2003, 2004 Zultys Technologies.
> =A0 * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
> =A0 *


> +=A0=A0=A0=A0=A0=A0=A0dev->idx =3D index++;
> +
> +=A0=A0=A0=A0=A0=A0=A0dev_set_drvdata(&ofdev->dev, dev);
> +
> +=A0=A0=A0=A0=A0=A0=A0if((addrp =3D of_get_address(np, 0, NULL, NULL)) =
=3D=3D NULL ||
> +=A0=A0=A0=A0=A0=A0=A0 =A0 (addr =3D of_translate_address(np, addrp)) =3D=
=3D OF_BAD_ADDR) {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_CRIT "ibm-iic%d=
: Unable to get iic address\n",
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =
=A0 dev->idx);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ret =3D -EBUSY;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto fail1;
> =A0=A0=A0=A0=A0=A0=A0=A0}
> =A0
> -=A0=A0=A0=A0=A0=A0=A0if (!(dev->vaddr =3D ioremap(ocp->def->paddr, sizeo=
f(struct iic_regs)))){
> +=A0=A0=A0=A0=A0=A0=A0if (!(dev->vaddr =3D ioremap(addr, sizeof(struct ii=
c_regs)))){
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_CRIT "ibm-iic=
%d: failed to ioremap device registers\n",
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0d=
ev->idx);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ret =3D -ENXIO;
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto fail2;
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto fail1;
> =A0=A0=A0=A0=A0=A0=A0=A0}

Use of_iomap here to save a few lines.

> =A0=A0=A0=A0=A0=A0=A0=A0init_waitqueue_head(&dev->wq);
> =A0
> -=A0=A0=A0=A0=A0=A0=A0dev->irq =3D iic_force_poll ? -1 : ocp->def->irq;
> -=A0=A0=A0=A0=A0=A0=A0if (dev->irq >=3D 0){
> +=A0=A0=A0=A0=A0=A0=A0if(iic_force_poll)
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0dev->irq =3D -1;
> +=A0=A0=A0=A0=A0=A0=A0else if((dev->irq =3D irq_of_parse_and_map(np, 0)) =
=3D=3D NO_IRQ) {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_ERR __FILE__ ":=
 irq_of_parse_and_map failed\n");
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0dev->irq =3D -1;
> +=A0=A0=A0=A0=A0=A0=A0}
> +
> +=A0=A0=A0=A0=A0=A0=A0if (dev->irq >=3D 0) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* Disable interrupts unt=
il we finish initialization,
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 assumes level-sensit=
ive IRQ setup...
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 */

This was in the original driver, but I think it's wrong anyway:
irq =3D=3D 0 could be valid, so you shouldn't compare against that
in general. Use NO_IRQ as a symbolic way to express an invalid
IRQ (yes, I'm aware that NO_IRQ is currently defined to 0).

> @@ -711,23 +722,30 @@ static int __devinit iic_probe(struct ocp_device *o=
cp){
> =A0=A0=A0=A0=A0=A0=A0=A0
> =A0=A0=A0=A0=A0=A0=A0=A0if (dev->irq < 0)
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_WARNING "ibm-=
iic%d: using polling mode\n",=20
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0dev=
=2D>idx);
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =
=A0 dev->idx);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0
> =A0=A0=A0=A0=A0=A0=A0=A0/* Board specific settings */
> -=A0=A0=A0=A0=A0=A0=A0dev->fast_mode =3D iic_force_fast ? 1 : (iic_data ?=
 iic_data->fast_mode : 0);
> +=A0=A0=A0=A0=A0=A0=A0dev->fast_mode =3D iic_force_fast ? 1 : 0;

If there is a good reason to specify fast or slow mode per board, you may w=
ant
to make that a property in the device node.

> +
> +static struct of_device_id ibm_iic_match[] =3D
> =A0{
> -=A0=A0=A0=A0=A0=A0=A0{ .vendor =3D OCP_VENDOR_IBM, .function =3D OCP_FUN=
C_IIC },
> -=A0=A0=A0=A0=A0=A0=A0{ .vendor =3D OCP_VENDOR_INVALID }
> +=A0=A0=A0=A0=A0=A0=A0{ .type =3D "i2c", .compatible =3D "ibm,iic", },
> +=A0=A0=A0=A0=A0=A0=A0{}
> =A0};

This is probably not specific enough. I'm rather sure that someone at IBM
has implemented an i2c chip that this driver doesn't support. Maybe

	.compatible =3D "ibm,405-iic"

or similar would be a better thing to check for.

	Arnd <><

Re: [PATCH] i2c-ibm_iic driver

[Stefan Roese]

On Saturday 05 January 2008, Arnd Bergmann wrote:
> On Saturday 05 January 2008, Sean MacLennan wrote:
> > I converted the i2c-ibm_iic driver from an ocp driver to an of_platform
> > driver. Since this driver is in the kernel.org kernel, should I rename
> > it and keep the old one around? I notice this was done with the emac
> > network driver.
>
> The interesting question is whether there are any functional users in
> arch/ppc left for the original driver. If all platforms that used
> to use i2c-ibm_iic are broken already, you can can go ahead with
> the conversion.

No, they are not all broken. We still have to support arch/ppc till mid of=
=20
this year.

> Otherwise, there are two options:=20
>
> 1. duplicate the driver like you suggested
> 2. make the same driver both a ocp and of_platform, depending on
> the configuration options.
>
> Since most of the driver is untouched by your patch, I'd lean to
> the second option, but of course, that is more work for you.

I did send a patch for such a combined driver a few months ago:

http://patchwork.ozlabs.org/linuxppc/patch?person=3D305&id=3D14181

There were still same open issues and I didn't find the time till now to=20
address them. It would be great if you could take care of these issues and=
=20
submit a reworked version.

> Your patch otherwise looks good, but I have a few comments on
>
> details:
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -241,7 +241,6 @@ config I2C_PIIX4
> > =A0
> > =A0config I2C_IBM_IIC
> > =A0=A0=A0=A0=A0=A0=A0=A0tristate "IBM PPC 4xx on-chip I2C interface"
> > -=A0=A0=A0=A0=A0=A0=A0depends on IBM_OCP
> > =A0=A0=A0=A0=A0=A0=A0=A0help
> > =A0=A0=A0=A0=A0=A0=A0=A0 =A0Say Y here if you want to use IIC periphera=
l found on
> > =A0=A0=A0=A0=A0=A0=A0=A0 =A0embedded IBM PPC 4xx based systems.
>
> In any way, this one now needs to depend on PPC_MERGE now, you
> could even make it depend on PPC_4xx, but it's often better to
> allow building the driver when you can, even if it doesn't make
> sense on your hardware. This gives a better compile coverage.
>
> > diff --git a/drivers/i2c/busses/i2c-ibm_iic.c
> > b/drivers/i2c/busses/i2c-ibm_iic.c index 9b43ff7..838006f 100644
> > --- a/drivers/i2c/busses/i2c-ibm_iic.c
> > +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> > @@ -3,6 +3,10 @@
> > =A0 *
> > =A0 * Support for the IIC peripheral on IBM PPC 4xx
> > =A0 *
> > + * Copyright (c) 2008 PIKA Technologies
> > + * Sean MacLennan <smaclennan@pikatech.com>
> > + * =A0Converted to an of_platform_driver.
> > + *
>
> Changelogs in the file itself are discouraged. In this case, it's just
> one line, so it's not really harmful.
>
> I think the convention is for copyrights to be in chronological order,
> so you should put the copyright below Eugene's.
>
> > =A0 * Copyright (c) 2003, 2004 Zultys Technologies.
> > =A0 * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.ne=
t>
> > =A0 *
> >
> >
> > +=A0=A0=A0=A0=A0=A0=A0dev->idx =3D index++;
> > +
> > +=A0=A0=A0=A0=A0=A0=A0dev_set_drvdata(&ofdev->dev, dev);
> > +
> > +=A0=A0=A0=A0=A0=A0=A0if((addrp =3D of_get_address(np, 0, NULL, NULL)) =
=3D=3D NULL ||
> > +=A0=A0=A0=A0=A0=A0=A0 =A0 (addr =3D of_translate_address(np, addrp)) =
=3D=3D OF_BAD_ADDR) {
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_CRIT "ibm-iic=
%d: Unable to get iic
> > address\n", +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0 =A0 dev->idx);
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ret =3D -EBUSY;
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto fail1;
> > =A0=A0=A0=A0=A0=A0=A0=A0}
> > =A0
> > -=A0=A0=A0=A0=A0=A0=A0if (!(dev->vaddr =3D ioremap(ocp->def->paddr, siz=
eof(struct
> > iic_regs)))){ +=A0=A0=A0=A0=A0=A0=A0if (!(dev->vaddr =3D ioremap(addr, =
sizeof(struct
> > iic_regs)))){ printk(KERN_CRIT "ibm-iic%d: failed to ioremap device
> > registers\n", dev->idx);
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ret =3D -ENXIO;
> > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto fail2;
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto fail1;
> > =A0=A0=A0=A0=A0=A0=A0=A0}
>
> Use of_iomap here to save a few lines.
>
> > =A0=A0=A0=A0=A0=A0=A0=A0init_waitqueue_head(&dev->wq);
> > =A0
> > -=A0=A0=A0=A0=A0=A0=A0dev->irq =3D iic_force_poll ? -1 : ocp->def->irq;
> > -=A0=A0=A0=A0=A0=A0=A0if (dev->irq >=3D 0){
> > +=A0=A0=A0=A0=A0=A0=A0if(iic_force_poll)
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0dev->irq =3D -1;
> > +=A0=A0=A0=A0=A0=A0=A0else if((dev->irq =3D irq_of_parse_and_map(np, 0)=
) =3D=3D NO_IRQ) {
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_ERR __FILE__ =
": irq_of_parse_and_map
> > failed\n"); +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0dev->irq =3D =
=2D1;
> > +=A0=A0=A0=A0=A0=A0=A0}
> > +
> > +=A0=A0=A0=A0=A0=A0=A0if (dev->irq >=3D 0) {
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* Disable interrupts u=
ntil we finish initialization,
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 assumes level-sens=
itive IRQ setup...
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 */
>
> This was in the original driver, but I think it's wrong anyway:
> irq =3D=3D 0 could be valid, so you shouldn't compare against that
> in general. Use NO_IRQ as a symbolic way to express an invalid
> IRQ (yes, I'm aware that NO_IRQ is currently defined to 0).
>
> > @@ -711,23 +722,30 @@ static int __devinit iic_probe(struct ocp_device
> > *ocp){=20
> > =A0=A0=A0=A0=A0=A0=A0=A0if (dev->irq < 0)
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0printk(KERN_WARNING "ib=
m-iic%d: using polling mode\n",
> > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0d=
ev->idx);
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =
=A0 dev->idx);
> > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0
> > =A0=A0=A0=A0=A0=A0=A0=A0/* Board specific settings */
> > -=A0=A0=A0=A0=A0=A0=A0dev->fast_mode =3D iic_force_fast ? 1 : (iic_data=
 ?
> > iic_data->fast_mode : 0); +=A0=A0=A0=A0=A0=A0=A0dev->fast_mode =3D iic_=
force_fast ? 1 :
> > 0;
>
> If there is a good reason to specify fast or slow mode per board, you may
> want to make that a property in the device node.
>
> > +
> > +static struct of_device_id ibm_iic_match[] =3D
> > =A0{
> > -=A0=A0=A0=A0=A0=A0=A0{ .vendor =3D OCP_VENDOR_IBM, .function =3D OCP_F=
UNC_IIC },
> > -=A0=A0=A0=A0=A0=A0=A0{ .vendor =3D OCP_VENDOR_INVALID }
> > +=A0=A0=A0=A0=A0=A0=A0{ .type =3D "i2c", .compatible =3D "ibm,iic", },
> > +=A0=A0=A0=A0=A0=A0=A0{}
> > =A0};
>
> This is probably not specific enough. I'm rather sure that someone at IBM
> has implemented an i2c chip that this driver doesn't support. Maybe
>
> 	.compatible =3D "ibm,405-iic"
>
> or similar would be a better thing to check for.

	.compatible =3D "ibm,4xx-iic"

please, since 405 and 440 have the same I2C controller.

Best regards,
Stefan

Re: [PATCH] i2c-ibm_iic driver

[Arnd Bergmann]

On Saturday 05 January 2008, Stefan Roese wrote:
> >
> > This is probably not specific enough. I'm rather sure that someone at I=
BM
> > has implemented an i2c chip that this driver doesn't support. Maybe
> >
> > =A0=A0=A0=A0=A0=A0.compatible =3D "ibm,405-iic"
> >
> > or similar would be a better thing to check for.
>=20
> =A0=A0=A0=A0=A0=A0=A0=A0.compatible =3D "ibm,4xx-iic"
>=20
> please, since 405 and 440 have the same I2C controller.
>=20

But that's not how compatible properties work -- they should not
contain wildcards. If you have different devices that are
backwards compatible, you should list the older one in all
newer devices, e.g. the 440 can list that it is compatible
with both ibm,405-iic and ibm,440-iic. If there was an earlier
401 that had iic as well, you may even want to include that
in the device tree.

	Arnd <><

Re: [PATCH] i2c-ibm_iic driver

[Stefan Roese]

On Saturday 05 January 2008, Arnd Bergmann wrote:
> > > This is probably not specific enough. I'm rather sure that someone at
> > > IBM has implemented an i2c chip that this driver doesn't support. May=
be
> > >
> > > =A0=A0=A0=A0=A0=A0.compatible =3D "ibm,405-iic"
> > >
> > > or similar would be a better thing to check for.
> >
> > =A0=A0=A0=A0=A0=A0=A0=A0.compatible =3D "ibm,4xx-iic"
> >
> > please, since 405 and 440 have the same I2C controller.
>
> But that's not how compatible properties work -- they should not
> contain wildcards. If you have different devices that are
> backwards compatible, you should list the older one in all
> newer devices, e.g. the 440 can list that it is compatible
> with both ibm,405-iic and ibm,440-iic. If there was an earlier
> 401 that had iic as well, you may even want to include that
> in the device tree.

OK. Thanks for clarifying.

Best regards,
Stefan

Re: [PATCH] i2c-ibm_iic driver

[Sean MacLennan]

Arnd Bergmann wrote:
> On Saturday 05 January 2008, Stefan Roese wrote:
>   
>>> This is probably not specific enough. I'm rather sure that someone at IBM
>>> has implemented an i2c chip that this driver doesn't support. Maybe
>>>
>>>       .compatible = "ibm,405-iic"
>>>
>>> or similar would be a better thing to check for.
>>>       
>>         .compatible = "ibm,4xx-iic"
>>
>> please, since 405 and 440 have the same I2C controller.
>>
>>     
>
> But that's not how compatible properties work -- they should not
> contain wildcards. If you have different devices that are
> backwards compatible, you should list the older one in all
> newer devices, e.g. the 440 can list that it is compatible
> with both ibm,405-iic and ibm,440-iic. If there was an earlier
> 401 that had iic as well, you may even want to include that
> in the device tree.
>
> 	Arnd <><
>   
Ok. The 44x based .dts files do not list 405-iic, so would I think I 
will add two compatibility matches, one for 405 and one for 440EP. That 
way I do not break all the current .dts files. Everybody ok with that?

Cheers,
    Sean

Re: [PATCH] i2c-ibm_iic driver

[Arnd Bergmann]

On Saturday 05 January 2008, Sean MacLennan wrote:
> 
> Ok. The 44x based .dts files do not list 405-iic, so would I think I 
> will add two compatibility matches, one for 405 and one for 440EP. That 
> way I do not break all the current .dts files. Everybody ok with that?
> 

Sounds good. There are obviously no other drivers that know only
about 405 but not about 440, so there is no backwards compatibility
problem. If we ever get a 450/460/470/... that we want to support
with this driver, it can simply claim to be compatible with 440 or 405
if not both.

	Arnd <><

Re: [PATCH] i2c-ibm_iic driver

[David Gibson]

On Sat, Jan 05, 2008 at 08:18:22PM +0100, Arnd Bergmann wrote:
> On Saturday 05 January 2008, Sean MacLennan wrote:
> > 
> > Ok. The 44x based .dts files do not list 405-iic, so would I think I 
> > will add two compatibility matches, one for 405 and one for 440EP. That 
> > way I do not break all the current .dts files. Everybody ok with that?
> > 
> 
> Sounds good. There are obviously no other drivers that know only
> about 405 but not about 440, so there is no backwards compatibility
> problem. If we ever get a 450/460/470/... that we want to support
> with this driver, it can simply claim to be compatible with 440 or 405
> if not both.

Actually, I think checking for "ibm,iic" is ok.  I'm sure there are
other IBM produced i2c chips, but "IIC" is also the name of the ASIC
block that implements this controller.  "ibm,iic" in the compatible is
supposed to refer to this family of i2c bridges.  Not a great choice
on my part, perhaps, but not so awful as to go changing the existing
device trees I think.

-- 
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: [PATCH] i2c-ibm_iic driver

[Sean MacLennan]

Stefan Roese wrote:
>
>> Otherwise, there are two options: 
>>
>> 1. duplicate the driver like you suggested
>> 2. make the same driver both a ocp and of_platform, depending on
>> the configuration options.
>>
>> Since most of the driver is untouched by your patch, I'd lean to
>> the second option, but of course, that is more work for you.
>>     
>
> I did send a patch for such a combined driver a few months ago:
>
> http://patchwork.ozlabs.org/linuxppc/patch?person=305&id=14181
>
> There were still same open issues and I didn't find the time till now to 
> address them. It would be great if you could take care of these issues and 
> submit a reworked version.
>
>   
I can look into this. Thanks!

Cheers,
   Sean

Re: [PATCH] i2c-ibm_iic driver

[Sean MacLennan]

Arnd Bergmann wrote:
> On Saturday 05 January 2008, Sean MacLennan wrote:
>   
>> I converted the i2c-ibm_iic driver from an ocp driver to an of_platform 
>> driver. Since this driver is in the kernel.org kernel, should I rename 
>> it and keep the old one around? I notice this was done with the emac 
>> network driver.
>>     
>
> The interesting question is whether there are any functional users in
> arch/ppc left for the original driver. If all platforms that used
> to use i2c-ibm_iic are broken already, you can can go ahead with
> the conversion. Otherwise, there are two options:
>
> 1. duplicate the driver like you suggested
> 2. make the same driver both a ocp and of_platform, depending on
> the configuration options.
>
> Since most of the driver is untouched by your patch, I'd lean to
> the second option, but of course, that is more work for you.
>   
Given Stefan subsequent post, I'll go with the second option.
> Your patch otherwise looks good, but I have a few comments on
> details:
>
>   
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -241,7 +241,6 @@ config I2C_PIIX4
>>  
>>  config I2C_IBM_IIC
>>         tristate "IBM PPC 4xx on-chip I2C interface"
>> -       depends on IBM_OCP
>>         help
>>           Say Y here if you want to use IIC peripheral found on 
>>           embedded IBM PPC 4xx based systems. 
>>     
>
> In any way, this one now needs to depend on PPC_MERGE now, you
> could even make it depend on PPC_4xx, but it's often better to
> allow building the driver when you can, even if it doesn't make
> sense on your hardware. This gives a better compile coverage.
>
>   
>> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
>> index 9b43ff7..838006f 100644
>> --- a/drivers/i2c/busses/i2c-ibm_iic.c
>> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
>> @@ -3,6 +3,10 @@
>>   *
>>   * Support for the IIC peripheral on IBM PPC 4xx
>>   *
>> + * Copyright (c) 2008 PIKA Technologies
>> + * Sean MacLennan <smaclennan@pikatech.com>
>> + *  Converted to an of_platform_driver.
>> + *
>>     
>
> Changelogs in the file itself are discouraged. In this case, it's just
> one line, so it's not really harmful.
>
> I think the convention is for copyrights to be in chronological order,
> so you should put the copyright below Eugene's.
>   
Ok, I will change it. To be honest, I didn't think it was enough of a 
change to actually be worth a copyright, but PIKA is a little touchy 
about copyrights right now and I wanted to point out I *only* did the 
port. I will remove the changelog and move the copyright notice.
>   
>>   * Copyright (c) 2003, 2004 Zultys Technologies.
>>   * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
>>   *
>>     
>
>
>   
>> +       dev->idx = index++;
>> +
>> +       dev_set_drvdata(&ofdev->dev, dev);
>> +
>> +       if((addrp = of_get_address(np, 0, NULL, NULL)) == NULL ||
>> +          (addr = of_translate_address(np, addrp)) == OF_BAD_ADDR) {
>> +               printk(KERN_CRIT "ibm-iic%d: Unable to get iic address\n",
>> +                          dev->idx);
>>                 ret = -EBUSY;
>>                 goto fail1;
>>         }
>>  
>> -       if (!(dev->vaddr = ioremap(ocp->def->paddr, sizeof(struct iic_regs)))){
>> +       if (!(dev->vaddr = ioremap(addr, sizeof(struct iic_regs)))){
>>                 printk(KERN_CRIT "ibm-iic%d: failed to ioremap device registers\n",
>>                         dev->idx);
>>                 ret = -ENXIO;
>> -               goto fail2;
>> +               goto fail1;
>>         }
>>     
>
> Use of_iomap here to save a few lines.
>
>   
Cool, I didn't notice that function.
>>         init_waitqueue_head(&dev->wq);
>>  
>> -       dev->irq = iic_force_poll ? -1 : ocp->def->irq;
>> -       if (dev->irq >= 0){
>> +       if(iic_force_poll)
>> +               dev->irq = -1;
>> +       else if((dev->irq = irq_of_parse_and_map(np, 0)) == NO_IRQ) {
>> +               printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
>> +               dev->irq = -1;
>> +       }
>> +
>> +       if (dev->irq >= 0) {
>>                 /* Disable interrupts until we finish initialization,
>>                    assumes level-sensitive IRQ setup...
>>                  */
>>     
>
> This was in the original driver, but I think it's wrong anyway:
> irq == 0 could be valid, so you shouldn't compare against that
> in general. Use NO_IRQ as a symbolic way to express an invalid
> IRQ (yes, I'm aware that NO_IRQ is currently defined to 0).
>   
Ok.
>   
>> @@ -711,23 +722,30 @@ static int __devinit iic_probe(struct ocp_device *ocp){
>>         
>>         if (dev->irq < 0)
>>                 printk(KERN_WARNING "ibm-iic%d: using polling mode\n", 
>> -                       dev->idx);
>> +                          dev->idx);
>>                 
>>         /* Board specific settings */
>> -       dev->fast_mode = iic_force_fast ? 1 : (iic_data ? iic_data->fast_mode : 0);
>> +       dev->fast_mode = iic_force_fast ? 1 : 0;
>>     
>
> If there is a good reason to specify fast or slow mode per board, you may want
> to make that a property in the device node.
>
>   
Ok. I really don't know, none of the board ports I looked at used fast mode.
>> +
>> +static struct of_device_id ibm_iic_match[] =
>>  {
>> -       { .vendor = OCP_VENDOR_IBM, .function = OCP_FUNC_IIC },
>> -       { .vendor = OCP_VENDOR_INVALID }
>> +       { .type = "i2c", .compatible = "ibm,iic", },
>> +       {}
>>  };
>>     
>
> This is probably not specific enough. I'm rather sure that someone at IBM
> has implemented an i2c chip that this driver doesn't support. Maybe
>
> 	.compatible = "ibm,405-iic"
>
> or similar would be a better thing to check for.
>
> 	Arnd <><
>   

Ok, I will look into this.

Cheers,
    Sean

Re: [PATCH] i2c-ibm_iic driver

[Sean MacLennan]

Arnd Bergmann wrote:
> If there is a good reason to specify fast or slow mode per board, you may want
> to make that a property in the device node.
>   
I tried to add fast_mode to the .dts file and failed.

            IIC1: i2c@ef600800 {
                device_type = "i2c";
                compatible = "ibm,iic-440ep", "ibm,iic-440gp", "ibm,iic";
                reg = <ef600800 14>;
                interrupt-parent = <&UIC0>;
                interrupts = <7 4>;
                fast-mode = <0>;
            };

As soon as a I add the fast-mode line I get the following error on boot:

fdt_wrapper_setprop():105  FDT_ERR_NOSPACE

Remove the line and I boot. Any ideas?

Cheers,
   Sean