[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 
this year.

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

> 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 at 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.
>
> >   * Copyright (c) 2003, 2004 Zultys Technologies.
> >   * Eugene Surovegin <eugene.surovegin at zultys.com> or <ebs at 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.
>
> >         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).
>
> > @@ -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.
>
> > +
> > +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.

	.compatible = "ibm,4xx-iic"

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

Best regards,
Stefan