Message-ID: <9e4733910801200718q7304bc29q38e67580613189e4@mail.gmail.com>
Date:	Sun, 20 Jan 2008 10:18:59 -0500
From:	"Jon Smirl" <jonsmirl@...il.com>
To:	"Jean Delvare" <khali@...ux-fr.org>,
	"Benjamin Herrenschmidt" <benh@...nel.crashing.org>
Cc:	i2c@...sensors.org, linuxppc-dev@...abs.org,
	linux-kernel@...r.kernel.org
Subject: Re: [i2c] [PATCH 19 3/5] Clean up error returns

On 1/20/08, Jean Delvare <khali@...ux-fr.org> wrote:
> > @@ -381,7 +385,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);
> >
> >
>
> Is this last chunk a cleanup or a bugfix? It seems that NO_IRQ can have
> value 0 or -1 depending on the architecture, so your change is real on
> some architectures.

I was confused by this too. Search the ppc list archives and there is
a thread about it where BenH tries to explain the correct way of
fixing it to me.

This is part of the ppc to powerpc conversion that has not been
completely cleaned up in this driver. NO_IRQ = -1 is ppc and NO_IRQ =
0 is powerpc. Since this driver didn't originally use the NO_IRQ
define it didn't get automatically converted. We need to identify the
right places where NO_IRQ should have been used.

>
> I have to admit that I'm a bit confused by the way IRQs are handled by
> this driver. On the one hand, there is code to handle polled-mode:
>
> static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> {
>         (...)
>         if (i2c->irq == 0)

I missed this one, it should have been NO_IRQ.

>         {
>                 (...)
>         } else {
>                 /* Interrupt mode */
>
> But on the other hand the initialization code bails out if the platform
> device doesn't provide an IRQ:

>
> static int fsl_i2c_probe(struct platform_device *pdev)
> {
>         (...)
>         i2c->irq = platform_get_irq(pdev, 0);
>         if (i2c->irq < 0) {
>                 result = -ENXIO;
>                 goto fail_get_irq;

It is only bailing out on an error, not the NO_IRQ case. But maybe the
comparison needs to be with NO_IRQ instead of zero.



>         }
>
> So it seems to me like the polling mode code is never actually used?
> Unless some platforms include an "empty" IRQ in their device
> definition. Which indeed seems to be the case... but then they set the
> IRQ to 0, NOT to NO_IRQ, so I'm wondering if the change you propose is
> really correct.

All of this is very confusing to me, There are physical IRQs and
virtual IRQs. Apparently zero is a legal physical IRQ but it is not a
legal virtual IRQ. We only get virtual IRQs in this code. We need to
get BenH to give us the right answer on these two cases.


>
> Either way, there are more places in the driver where the IRQ is
> compared to 0, so if your change is correct, it should be applied
> consistently. Thus I will revert this part for the time being, if any
> change is really needed with regards to interrupts in this driver,
> please send a separate patch. But please double-check first, as I said
> above it's trickier than it looks.
>
> --
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@...il.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists