Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

["Jon Smirl" <jonsmirl@gmail.com>]

On 2/19/08, Jean Delvare <khali@linux-fr.org> wrote:
>  >       i2c->irq = platform_get_irq(pdev, 0);
>  > -     if (i2c->irq < 0) {
>  > +     if (i2c->irq < NO_IRQ) {
>
>
> I am skeptical about this one. Can platform_get_irq() really return
>  NO_IRQ? I thought that the IRQ resource would be plain missing if the
>  device has no IRQ, so I would expect:
>
>
>         i2c->irq = platform_get_irq(pdev, 0);
>         if (i2c->irq < 0)
>
>                 i2c->irq = NO_IRQ; /* Use polling */
>
>  Testing against NO_IRQ suggests that devices with no IRQ would still
>  have an IRQ resource defined and explicitly set to NO_IRQ. Sounds weird
>  to me. Can you please clarify this point?

Your fix is correct. I'm not sure polling worked in the original driver.

>  For what it's worth, no other kernel driver checks for irq < NO_IRQ.
>  They all check for irq < 0 after calling platform_get_irq().
>
>
>  >               result = -ENXIO;
>  >               goto fail_get_irq;
>  >       }
>  > @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  >               goto fail_map;
>  >       }
>  >
>  > -     if (i2c->irq != 0)
>  > +     if (i2c->irq != NO_IRQ)
>  >               if ((result = request_irq(i2c->irq, mpc_i2c_isr,
>  >                                         IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
>  >                       printk(KERN_ERR
>  > @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  >       return result;
>  >
>  >        fail_add:
>  > -     if (i2c->irq != 0)
>  > +     if (i2c->irq != NO_IRQ)
>  >               free_irq(i2c->irq, i2c);
>  >        fail_irq:
>  >       iounmap(i2c->base);
>  > @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
>  >       i2c_del_adapter(&i2c->adap);
>  >       platform_set_drvdata(pdev, NULL);
>  >
>  > -     if (i2c->irq != 0)
>  > +     if (i2c->irq != NO_IRQ)
>  >               free_irq(i2c->irq, i2c);
>  >
>  >       iounmap(i2c->base);
>
>
> The rest looks good.
>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com