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

On 2/19/08, Jean Delvare [off-list ref] 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