i2c-ibm_iic driver

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

index c466c6c..e9e1493 100644
@@ -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. 

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:

found on=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.

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.

=3D OF_BAD_ADDR) {

: Unable to get iic address\n",

=A0 dev->idx);

f(struct iic_regs)))){

c_regs)))){

%d: failed to ioremap device registers\n",

ev->idx);

Use of_iomap here to save a few lines.

=3D=3D NO_IRQ) {

irq_of_parse_and_map failed\n");

il we finish initialization,

ive IRQ setup...

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

iic%d: using polling mode\n",=20

=2D>idx);

=A0 dev->idx);

iic_data->fast_mode : 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.

C_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 =3D "ibm,405-iic"

or similar would be a better thing to check for.

	Arnd <><

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

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.

l found on

t>

=3D=3D NULL ||

=3D=3D OF_BAD_ADDR) {

%d: Unable to get iic

=A0=A0=A0=A0 =A0 dev->idx);

eof(struct

sizeof(struct

) =3D=3D NO_IRQ) {

": irq_of_parse_and_map

=2D1;

ntil we finish initialization,

itive IRQ setup...

m-iic%d: using polling mode\n",

ev->idx);

=A0 dev->idx);

?

force_fast ? 1 :

UNC_IIC },

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

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

Best regards,
Stefan

BM

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

be

OK. Thanks for clarifying.

Best regards,
Stefan

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

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

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

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

index c466c6c..e9e1493 100644
@@ -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. 

Hi Sean,

Please don't post patches as attachments.

	.
	.

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Ok.

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.

I made the changes in my code. I am trying to leave the original code as much as possible.

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.

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

index c466c6c..e9e1493 100644
@@ -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.

index 9b43ff7..d218a3b 100644
@@ -6,6 +6,9 @@
 * Copyright (c) 2003, 2004 Zultys Technologies.
 * Eugene Surovegin <[email protected]> or <[email protected]>
 *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <[email protected]>
+ *
 * Based on original work by
 *     Ian DaSilva  <[email protected]>
 *      Armin Kuster <[email protected]>

@@ -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

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

Thats all we ask.

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

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Let's try again.

Cheers,
Sean

index c466c6c..e9e1493 100644
@@ -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.

index 9b43ff7..98476fc 100644
@@ -6,6 +6,9 @@
 * Copyright (c) 2003, 2004 Zultys Technologies.
 * Eugene Surovegin <[email protected]> or <[email protected]>
 *
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <[email protected]>
+ *
 * Based on original work by 
 * 	Ian DaSilva  <[email protected]>
 *      Armin Kuster <[email protected]>

@@ -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

Looks good now. Time to send it to the i2c mailing list <[email protected]> and the maintainer Jean Delvare <[email protected]>. And please keep the linuxppc-dev list on CC.

Thanks.

Ciao,
Stefan

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

I can look into this. Thanks!

Cheers,
Sean

Given Stefan subsequent post, I'll go with the second option.

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.

Cool, I didn't notice that function.

Ok.

Ok. I really don't know, none of the board ports I looked at used fast mode.

Ok, I will look into this.

Cheers,
Sean

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

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

---

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

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