[PATCH] i2c-ibm_iic driver

[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

[PATCH] i2c-ibm_iic driver - new patch

[Sean MacLennan]

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

Second attempt. I think I have covered all the comments. It now should 
work with both ppc and powerpc architectures.

You can now specify fast-mode in the .dts file.

You can now specify an index for each entry.  If you don't I try to 
chose reasonable defaults. i.e. I keep a static int that is incremented. 
If you mix index entries with non-index entries, you are on your own.

Below is the example for the taco, I have shown fast-mode set in IIC1 as 
an example:

            IIC0: i2c@ef600700 {
                device_type = "i2c";
                compatible = "ibm,iic-440ep", "ibm,iic-440gp", "ibm,iic";
                reg = <ef600700 14>;
                interrupt-parent = <&UIC0>;
                interrupts = <2 4>;
                index = <0>;
            };

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

Cheers,
   Sean


[-- Attachment #2: i2c-patch --]
[-- Type: text/plain, Size: 6512 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..0b10bb1 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -6,7 +6,10 @@
  * Copyright (c) 2003, 2004 Zultys Technologies.
  * Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
  *
- * Based on original work by 
+ * 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>
  * 	Matt Porter  <mporter@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 @@ static inline u8 iic_clckdiv(unsigned int opb)
 	return (u8)((opb + 9) / 10 - 1);
 }
 
+#ifdef CONFIG_IBM_OCP
 /*
  * Register single IIC interface
  */
@@ -831,3 +840,192 @@ 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 *addrp, *freq;
+	u64 addr;
+	int ret;
+
+	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
+		printk(KERN_CRIT "ibm-iic: failed to allocate device data\n");
+		return -ENOMEM;
+	}
+
+	/* This assumes we don't mix index and non-index entries. */
+	if((addrp = of_get_property(np, "index", NULL)))
+		dev->idx = *addrp;
+	else
+		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(addr, sizeof(struct iic_regs)))){
+		printk(KERN_CRIT "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 if((dev->irq = irq_of_parse_and_map(np, 0)) == NO_IRQ)
+		printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
+
+	if (dev->irq != NO_IRQ) {
+		/* 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
+	 */
+	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 = &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;
+
+	if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
+		printk(KERN_CRIT "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 = (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",
+			dev->idx);
+		/* That's *very* bad, just shutdown IRQ ... */
+		if (dev->irq >= 0){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		    dev->irq = -1;
+		}
+	} else {
+		if (dev->irq != NO_IRQ){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		}
+		iounmap(dev->vaddr);
+		kfree(dev);
+	}
+
+	return 0;
+}
+
+
+static 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] i2c-ibm_iic driver - new patch

[Stephen Rothwell]

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

Hi Sean,

On Mon, 07 Jan 2008 21:03:12 -0500 Sean MacLennan <smaclennan@pikatech.com> wrote:
>

Please don't post patches as attachments.

> +static int __devinit iic_probe(struct of_device *ofdev,
> +							   const struct of_device_id *match)

Indenting could be better.

> +{

> +	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {

Please split the assignments from the tests.  Here and elsewhere.

> +		printk(KERN_CRIT "ibm-iic: failed to allocate device data\n");

I am not sure that these messages are necessary and, even if so, not KERN_CRIT.

> +	if(iic_force_poll)

Space after "if"

> +	if (dev->irq != NO_IRQ) {
	.
	.
> +	}
> +
> +	if (dev->irq == NO_IRQ)

	else instead?

> +		printk(KERN_WARNING "ibm-iic%d: using polling mode\n",
> +			   dev->idx);
> +static int __devexit iic_remove(struct of_device *ofdev)
> +{
> +	struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_get_drvdata(&ofdev->dev);

Unnecessary cast.

> +	if (i2c_del_adapter(&dev->adap)){
> +		printk(KERN_CRIT "ibm-iic%d: failed to delete i2c adapter :(\n",
> +			dev->idx);

This is not a KERN_CRIT situation ...

> +		/* That's *very* bad, just shutdown IRQ ... */
> +		if (dev->irq >= 0){

What is that testing? For NO_IRQ as below?

> +		    iic_interrupt_mode(dev, 0);
> +		    free_irq(dev->irq, dev);
> +		    dev->irq = -1;

NO_IRQ?

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

Should these last two be after the below brace?

> +	}
> +
> +	return 0;
> +}
> +
> +
> +static struct of_device_id ibm_iic_match[] =

This should be const.

-- 
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] i2c-ibm_iic driver - new patch

[Sean MacLennan]

Stephen Rothwell wrote:
> Hi Sean,
>
> On Mon, 07 Jan 2008 21:03:12 -0500 Sean MacLennan <smaclennan@pikatech.com> wrote:
>   
>
> Please don't post patches as attachments.
>   
Ok.
>   
>> +static int __devinit iic_probe(struct of_device *ofdev,
>> +							   const struct of_device_id *match)
>>     
>
> Indenting could be better.
>   
Sorry. Since this kernel is in a "work" directory and not in 
/usr/src/linux* my rules for deciding on the tab size didn't work :( I 
tried to correct the tabbing.
> Please split the assignments from the tests.  Here and elsewhere.
>
>   
I made the changes in my code. I am trying to leave the original code as 
much as possible.
>> +		printk(KERN_CRIT "ibm-iic: failed to allocate device data\n");
>>     
>
> I am not sure that these messages are necessary and, even if so, not KERN_CRIT.
>
>   
What would you recommend then? KERN_ERR? These are cut and paste from 
the original driver, so I left them alone. I will try KERN_ERR.
>> +		}
>> +	} else {
>> +		if (dev->irq != NO_IRQ){
>> +		    iic_interrupt_mode(dev, 0);
>> +		    free_irq(dev->irq, dev);
>> +		}
>> +		iounmap(dev->vaddr);
>> +		kfree(dev);
>>     
>
> Should these last two be after the below brace?
>
>   
I'm not really qualified to answer, but I will anyway ;) I assume the 
original author is basically saying if he cannot delete the adapter, it 
is unsafe to free the memory since the i2c code may still use it. If I 
have read that right, then I agree.

Cheers,
    Sean

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..d218a3b 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,186 @@ 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 if ((dev->irq = irq_of_parse_and_map(np, 0)) == 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] i2c-ibm_iic driver - new patch

[Stephen Rothwell]

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

On Tue, 08 Jan 2008 00:56:27 -0500 Sean MacLennan <smaclennan@pikatech.com> wrote:
>
> Stephen Rothwell wrote:
> >
> > On Mon, 07 Jan 2008 21:03:12 -0500 Sean MacLennan <smaclennan@pikatech.com> wrote:
> >   
> > Please don't post patches as attachments.
>  
> Ok.

Unfortunately, you are using thunderbird and so the patch is now wrapped.
There is a workaround, see Documentation/email-clients.txt.

> > Please split the assignments from the tests.  Here and elsewhere.
> >   
> I made the changes in my code. I am trying to leave the original code as 
> much as possible.

Thats all we ask.

> >> +	} else {
> >> +		if (dev->irq != NO_IRQ){
> >> +		    iic_interrupt_mode(dev, 0);
> >> +		    free_irq(dev->irq, dev);
> >> +		}
> >> +		iounmap(dev->vaddr);
> >> +		kfree(dev);
> >>     
> >
> > Should these last two be after the below brace?
> >
> >   
> I'm not really qualified to answer, but I will anyway ;) I assume the 
> original author is basically saying if he cannot delete the adapter, it 
> is unsafe to free the memory since the i2c code may still use it. If I 
> have read that right, then I agree.

OK, I can see that this is a "that should not happen" condition.

> +    if (iic_force_poll)
> +        dev->irq = NO_IRQ;
> +    else if ((dev->irq = irq_of_parse_and_map(np, 0)) == NO_IRQ)

You missed this one.

Overall looks better, except all your indentation is now 4 spaces. We use
a TAB character for each level of indentation and you should be able to
set your editor to *display* the TABs as 4 places if that is what you like.

-- 
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] i2c-ibm_iic driver - new patch

[Sean MacLennan]

Let's try again.

Cheers,
    Sean

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] i2c-ibm_iic driver - new patch

[Stefan Roese]

On Tuesday 08 January 2008, Sean MacLennan wrote:
> Let's try again.

Looks good now. Time to send it to the i2c mailing list <i2c@lm-sensors.org> 
and the maintainer Jean Delvare <khali@linux-fr.org>. And please keep the 
linuxppc-dev list on CC.

Thanks.

Ciao,
Stefan

Re: [PATCH] i2c-ibm_iic driver - new patch

[Scott Wood]

On Tue, Jan 08, 2008 at 12:56:27AM -0500, Sean MacLennan wrote:
> Stephen Rothwell wrote:
> > On Mon, 07 Jan 2008 21:03:12 -0500 Sean MacLennan <smaclennan@pikatech.com> wrote:
> >> +static int __devinit iic_probe(struct of_device *ofdev,
> >> +							   const struct of_device_id *match)
> >>     
> >
> > Indenting could be better.
> >   
> Sorry. Since this kernel is in a "work" directory and not in 
> /usr/src/linux* my rules for deciding on the tab size didn't work :( I 
> tried to correct the tabbing.

Better to tab to the semantic indentation level (in this case there is
none), and then use spaces to align -- that way it looks right regardless of
what the tab size is set to.

-Scott

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

Re: [PATCH] i2c-ibm_iic driver bonus patch

[Sean MacLennan]

Here is an optional bonus patch that cleans up most of the checkpatch 
warnings in the common code. I left in the volatiles, since I don't 
understand why they where needed. The memory always seems to be access 
with in_8 and out_8, which are declared volatile. But they could be 
there to fix a very specific bug.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
---
--- for-2.6.25/drivers/i2c/busses/submitted-i2c-ibm_iic.c	2008-02-18 20:44:06.000000000 -0500
+++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c	2008-02-18 20:53:53.000000000 -0500
@@ -76,17 +76,17 @@
 #endif
 
 #if DBG_LEVEL > 0
-#  define DBG(f,x...)	printk(KERN_DEBUG "ibm-iic" f, ##x)
+#  define DBG(f, x...)	printk(KERN_DEBUG "ibm-iic" f, ##x)
 #else
-#  define DBG(f,x...)	((void)0)
+#  define DBG(f, x...)	((void)0)
 #endif
 #if DBG_LEVEL > 1
-#  define DBG2(f,x...) 	DBG(f, ##x)
+#  define DBG2(f, x...) 	DBG(f, ##x)
 #else
-#  define DBG2(f,x...) 	((void)0)
+#  define DBG2(f, x...) 	((void)0)
 #endif
 #if DBG_LEVEL > 2
-static void dump_iic_regs(const char* header, struct ibm_iic_private* dev)
+static void dump_iic_regs(const char *header, struct ibm_iic_private *dev)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 	printk(KERN_DEBUG "ibm-iic%d: %s\n", dev->idx, header);
@@ -98,9 +98,9 @@
 		in_8(&iic->extsts), in_8(&iic->clkdiv), in_8(&iic->xfrcnt),
 		in_8(&iic->xtcntlss), in_8(&iic->directcntl));
 }
-#  define DUMP_REGS(h,dev)	dump_iic_regs((h),(dev))
+#  define DUMP_REGS(h, dev)	dump_iic_regs((h), (dev))
 #else
-#  define DUMP_REGS(h,dev)	((void)0)
+#  define DUMP_REGS(h, dev)	((void)0)
 #endif
 
 /* Bus timings (in ns) for bit-banging */
@@ -111,25 +111,26 @@
 	unsigned int high;
 	unsigned int buf;
 } timings [] = {
-/* Standard mode (100 KHz) */
-{
-	.hd_sta	= 4000,
-	.su_sto	= 4000,
-	.low	= 4700,
-	.high	= 4000,
-	.buf	= 4700,
-},
-/* Fast mode (400 KHz) */
-{
-	.hd_sta = 600,
-	.su_sto	= 600,
-	.low 	= 1300,
-	.high 	= 600,
-	.buf	= 1300,
-}};
+	/* Standard mode (100 KHz) */
+	{
+		.hd_sta	= 4000,
+		.su_sto	= 4000,
+		.low	= 4700,
+		.high	= 4000,
+		.buf	= 4700,
+	},
+	/* Fast mode (400 KHz) */
+	{
+		.hd_sta = 600,
+		.su_sto	= 600,
+		.low 	= 1300,
+		.high 	= 600,
+		.buf	= 1300,
+	}
+};
 
 /* Enable/disable interrupt generation */
-static inline void iic_interrupt_mode(struct ibm_iic_private* dev, int enable)
+static inline void iic_interrupt_mode(struct ibm_iic_private *dev, int enable)
 {
 	out_8(&dev->vaddr->intmsk, enable ? INTRMSK_EIMTC : 0);
 }
@@ -137,7 +138,7 @@
 /*
  * Initialize IIC interface.
  */
-static void iic_dev_init(struct ibm_iic_private* dev)
+static void iic_dev_init(struct ibm_iic_private *dev)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 
@@ -182,7 +183,7 @@
 /*
  * Reset IIC interface
  */
-static void iic_dev_reset(struct ibm_iic_private* dev)
+static void iic_dev_reset(struct ibm_iic_private *dev)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 	int i;
@@ -191,19 +192,19 @@
 	DBG("%d: soft reset\n", dev->idx);
 	DUMP_REGS("reset", dev);
 
-    	/* Place chip in the reset state */
+	/* Place chip in the reset state */
 	out_8(&iic->xtcntlss, XTCNTLSS_SRST);
 
 	/* Check if bus is free */
 	dc = in_8(&iic->directcntl);
-	if (!DIRCTNL_FREE(dc)){
+	if (!DIRCTNL_FREE(dc)) {
 		DBG("%d: trying to regain bus control\n", dev->idx);
 
 		/* Try to set bus free state */
 		out_8(&iic->directcntl, DIRCNTL_SDAC | DIRCNTL_SCC);
 
 		/* Wait until we regain bus control */
-		for (i = 0; i < 100; ++i){
+		for (i = 0; i < 100; ++i) {
 			dc = in_8(&iic->directcntl);
 			if (DIRCTNL_FREE(dc))
 				break;
@@ -235,7 +236,7 @@
 static int iic_dc_wait(volatile struct iic_regs __iomem *iic, u8 mask)
 {
 	unsigned long x = jiffies + HZ / 28 + 2;
-	while ((in_8(&iic->directcntl) & mask) != mask){
+	while ((in_8(&iic->directcntl) & mask) != mask) {
 		if (unlikely(time_after(jiffies, x)))
 			return -1;
 		cond_resched();
@@ -243,15 +244,15 @@
 	return 0;
 }
 
-static int iic_smbus_quick(struct ibm_iic_private* dev, const struct i2c_msg* p)
+static int iic_smbus_quick(struct ibm_iic_private *dev, const struct i2c_msg *p)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
-	const struct i2c_timings* t = &timings[dev->fast_mode ? 1 : 0];
+	const struct i2c_timings *t = &timings[dev->fast_mode ? 1 : 0];
 	u8 mask, v, sda;
 	int i, res;
 
 	/* Only 7-bit addresses are supported */
-	if (unlikely(p->flags & I2C_M_TEN)){
+	if (unlikely(p->flags & I2C_M_TEN)) {
 		DBG("%d: smbus_quick - 10 bit addresses are not supported\n",
 			dev->idx);
 		return -EINVAL;
@@ -275,7 +276,7 @@
 
 	/* Send address */
 	v = (u8)((p->addr << 1) | ((p->flags & I2C_M_RD) ? 1 : 0));
-	for (i = 0, mask = 0x80; i < 8; ++i, mask >>= 1){
+	for (i = 0, mask = 0x80; i < 8; ++i, mask >>= 1) {
 		out_8(&iic->directcntl, sda);
 		ndelay(t->low / 2);
 		sda = (v & mask) ? DIRCNTL_SDAC : 0;
@@ -330,7 +331,7 @@
  */
 static irqreturn_t iic_handler(int irq, void *dev_id)
 {
-	struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id;
+	struct ibm_iic_private *dev = (struct ibm_iic_private *)dev_id;
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 
 	DBG2("%d: irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
@@ -347,11 +348,11 @@
  * Get master transfer result and clear errors if any.
  * Returns the number of actually transferred bytes or error (<0)
  */
-static int iic_xfer_result(struct ibm_iic_private* dev)
+static int iic_xfer_result(struct ibm_iic_private *dev)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 
-	if (unlikely(in_8(&iic->sts) & STS_ERR)){
+	if (unlikely(in_8(&iic->sts) & STS_ERR)) {
 		DBG("%d: xfer error, EXTSTS = 0x%02x\n", dev->idx,
 			in_8(&iic->extsts));
 
@@ -367,20 +368,19 @@
 		 * IIC interface is usually stuck in some strange
 		 * state, the only way out - soft reset.
 		 */
-		if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
+		if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE) {
 			DBG("%d: bus is stuck, resetting\n", dev->idx);
 			iic_dev_reset(dev);
 		}
 		return -EREMOTEIO;
-	}
-	else
+	} else
 		return in_8(&iic->xfrcnt) & XFRCNT_MTC_MASK;
 }
 
 /*
  * Try to abort active transfer.
  */
-static void iic_abort_xfer(struct ibm_iic_private* dev)
+static void iic_abort_xfer(struct ibm_iic_private *dev)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 	unsigned long x;
@@ -394,8 +394,8 @@
 	 * It's not worth to be optimized, just poll (timeout >= 1 tick)
 	 */
 	x = jiffies + 2;
-	while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
-		if (time_after(jiffies, x)){
+	while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE) {
+		if (time_after(jiffies, x)) {
 			DBG("%d: abort timeout, resetting...\n", dev->idx);
 			iic_dev_reset(dev);
 			return;
@@ -412,19 +412,19 @@
  * It puts current process to sleep until we get interrupt or timeout expires.
  * Returns the number of transferred bytes or error (<0)
  */
-static int iic_wait_for_tc(struct ibm_iic_private* dev){
-
+static int iic_wait_for_tc(struct ibm_iic_private *dev)
+{
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 	int ret = 0;
 
-	if (dev->irq >= 0){
+	if (dev->irq >= 0) {
 		/* Interrupt mode */
 		ret = wait_event_interruptible_timeout(dev->wq,
 			!(in_8(&iic->sts) & STS_PT), dev->adap.timeout * HZ);
 
 		if (unlikely(ret < 0))
 			DBG("%d: wait interrupted\n", dev->idx);
-		else if (unlikely(in_8(&iic->sts) & STS_PT)){
+		else if (unlikely(in_8(&iic->sts) & STS_PT)) {
 			DBG("%d: wait timeout\n", dev->idx);
 			ret = -ETIMEDOUT;
 		}
@@ -433,14 +433,14 @@
 		/* Polling mode */
 		unsigned long x = jiffies + dev->adap.timeout * HZ;
 
-		while (in_8(&iic->sts) & STS_PT){
-			if (unlikely(time_after(jiffies, x))){
+		while (in_8(&iic->sts) & STS_PT) {
+			if (unlikely(time_after(jiffies, x))) {
 				DBG("%d: poll timeout\n", dev->idx);
 				ret = -ETIMEDOUT;
 				break;
 			}
 
-			if (unlikely(signal_pending(current))){
+			if (unlikely(signal_pending(current))) {
 				DBG("%d: poll interrupted\n", dev->idx);
 				ret = -ERESTARTSYS;
 				break;
@@ -462,11 +462,11 @@
 /*
  * Low level master transfer routine
  */
-static int iic_xfer_bytes(struct ibm_iic_private* dev, struct i2c_msg* pm,
+static int iic_xfer_bytes(struct ibm_iic_private *dev, struct i2c_msg *pm,
 			  int combined_xfer)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
-	char* buf = pm->buf;
+	char *buf = pm->buf;
 	int i, j, loops, ret = 0;
 	int len = pm->len;
 
@@ -475,7 +475,7 @@
 		cntl |= CNTL_RW;
 
 	loops = (len + 3) / 4;
-	for (i = 0; i < loops; ++i, len -= 4){
+	for (i = 0; i < loops; ++i, len -= 4) {
 		int count = len > 4 ? 4 : len;
 		u8 cmd = cntl | ((count - 1) << CNTL_TCT_SHIFT);
 
@@ -498,7 +498,7 @@
 
 		if (unlikely(ret < 0))
 			break;
-		else if (unlikely(ret != count)){
+		else if (unlikely(ret != count)) {
 			DBG("%d: xfer_bytes, requested %d, transfered %d\n",
 				dev->idx, count, ret);
 
@@ -521,7 +521,7 @@
 /*
  * Set target slave address for master transfer
  */
-static inline void iic_address(struct ibm_iic_private* dev, struct i2c_msg* msg)
+static inline void iic_address(struct ibm_iic_private *dev, struct i2c_msg *msg)
 {
 	volatile struct iic_regs __iomem *iic = dev->vaddr;
 	u16 addr = msg->addr;
@@ -529,24 +529,24 @@
 	DBG2("%d: iic_address, 0x%03x (%d-bit)\n", dev->idx,
 		addr, msg->flags & I2C_M_TEN ? 10 : 7);
 
-	if (msg->flags & I2C_M_TEN){
+	if (msg->flags & I2C_M_TEN) {
 	    out_8(&iic->cntl, CNTL_AMD);
 	    out_8(&iic->lmadr, addr);
 	    out_8(&iic->hmadr, 0xf0 | ((addr >> 7) & 0x06));
-	}
-	else {
+	} else {
 	    out_8(&iic->cntl, 0);
 	    out_8(&iic->lmadr, addr << 1);
 	}
 }
 
-static inline int iic_invalid_address(const struct i2c_msg* p)
+static inline int iic_invalid_address(const struct i2c_msg *p)
 {
-	return (p->addr > 0x3ff) || (!(p->flags & I2C_M_TEN) && (p->addr > 0x7f));
+	return (p->addr > 0x3ff) ||
+		(!(p->flags & I2C_M_TEN) && (p->addr > 0x7f));
 }
 
-static inline int iic_address_neq(const struct i2c_msg* p1,
-				  const struct i2c_msg* p2)
+static inline int iic_address_neq(const struct i2c_msg *p1,
+				  const struct i2c_msg *p2)
 {
 	return (p1->addr != p2->addr)
 		|| ((p1->flags & I2C_M_TEN) != (p2->flags & I2C_M_TEN));
@@ -558,9 +558,13 @@
  */
 static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
-    	struct ibm_iic_private* dev = (struct ibm_iic_private*)(i2c_get_adapdata(adap));
-	volatile struct iic_regs __iomem *iic = dev->vaddr;
+	struct ibm_iic_private *dev;
+	volatile struct iic_regs __iomem *iic;
 	int i, ret = 0;
+	u8 extsts;
+
+	dev = (struct ibm_iic_private *)(i2c_get_adapdata(adap));
+	iic = dev->vaddr;
 
 	DBG2("%d: iic_xfer, %d msg(s)\n", dev->idx, num);
 
@@ -570,14 +574,14 @@
 	/* Check the sanity of the passed messages.
 	 * Uhh, generic i2c layer is more suitable place for such code...
 	 */
-	if (unlikely(iic_invalid_address(&msgs[0]))){
+	if (unlikely(iic_invalid_address(&msgs[0]))) {
 		DBG("%d: invalid address 0x%03x (%d-bit)\n", dev->idx,
 			msgs[0].addr, msgs[0].flags & I2C_M_TEN ? 10 : 7);
 		return -EINVAL;
 	}
-	for (i = 0; i < num; ++i){
-		if (unlikely(msgs[i].len <= 0)){
-			if (num == 1 && !msgs[0].len){
+	for (i = 0; i < num; ++i) {
+		if (unlikely(msgs[i].len <= 0)) {
+			if (num == 1 && !msgs[0].len) {
 				/* Special case for I2C_SMBUS_QUICK emulation.
 				 * IBM IIC doesn't support 0-length transactions
 				 * so we have to emulate them using bit-banging.
@@ -588,14 +592,15 @@
 				msgs[i].len, i);
 			return -EINVAL;
 		}
-		if (unlikely(iic_address_neq(&msgs[0], &msgs[i]))){
+		if (unlikely(iic_address_neq(&msgs[0], &msgs[i]))) {
 			DBG("%d: invalid addr in msg[%d]\n", dev->idx, i);
 			return -EINVAL;
 		}
 	}
 
 	/* Check bus state */
-	if (unlikely((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE)){
+	extsts = in_8(&iic->extsts);
+	if (unlikely((extsts & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE)) {
 		DBG("%d: iic_xfer, bus is not free\n", dev->idx);
 
 		/* Usually it means something serious has happend.
@@ -608,12 +613,11 @@
 		 */
 		iic_dev_reset(dev);
 
-		if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
+		if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE) {
 			DBG("%d: iic_xfer, bus is still not free\n", dev->idx);
 			return -EREMOTEIO;
 		}
-	}
-	else {
+	} else {
 		/* Flush master data buffer (just in case) */
 		out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB);
 	}
@@ -622,7 +626,7 @@
 	iic_address(dev, &msgs[0]);
 
 	/* Do real transfer */
-    	for (i = 0; i < num && !ret; ++i)
+	for (i = 0; i < num && !ret; ++i)
 		ret = iic_xfer_bytes(dev, &msgs[i], i < num - 1);
 
 	return ret < 0 ? ret : num;
@@ -648,7 +652,7 @@
 	 * Previous driver version used hardcoded divider value 4,
 	 * it corresponds to OPB frequency from the range (40, 50] MHz
 	 */
-	if (!opb){
+	if (!opb) {
 		printk(KERN_WARNING "ibm-iic: using compatibility value for OPB freq,"
 			" fix your board specific setup\n");
 		opb = 50000000;
@@ -657,9 +661,9 @@
 	/* Convert to MHz */
 	opb /= 1000000;
 
-	if (opb < 20 || opb > 150){
-		printk(KERN_WARNING "ibm-iic: invalid OPB clock frequency %u MHz\n",
-			opb);
+	if (opb < 20 || opb > 150) {
+		printk(KERN_WARNING "ibm-iic: "
+		       "invalid OPB clock frequency %u MHz\n", opb);
 		opb = opb < 20 ? 20 : 150;
 	}
 	return (u8)((opb + 9) / 10 - 1);

Re: [PATCH] i2c-ibm_iic driver bonus patch

[Arnd Bergmann]

On Tuesday 19 February 2008, Sean MacLennan wrote:
>  I left in the volatiles, since I don't 
> understand why they where needed. The memory always seems to be access 
> with in_8 and out_8, which are declared volatile. But they could be 
> there to fix a very specific bug.

It's very unlikely that they were really needed, and you certainly
shouldn't mark data as volatile in new code. It's very common to
mark I/O data structures as volatile when they should be __iomem,
because that's what people learn at university, but that is never
the right solution, even if it can hide other bugs in your code.

Of course, unlike the other changes in your patch, it does impact
code generation, so if you want to change it, that should be a
separate patch.

	Arnd <><