On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> <[email protected]> wrote:
> > +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct adnp *gpio;
> > + u32 num_gpios;
> > + int err;
> > +
> > + err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> > + &num_gpios);
> > + if (err < 0)
> > + return err;
> > +
> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> > + if (client->irq == NO_IRQ)
>
> Just if (!client->irq) since NO_IRQ is 0 nowadays.
At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
However, irq_of_parse_and_map() returns 0 if the interrupt cannot be
parsed or mapped, so checking for !client->irq is, as you say, correct.
Thierry
On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding
<[email protected]> wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
>> > + if (client->irq == NO_IRQ)
>>
>> Just if (!client->irq) since NO_IRQ is 0 nowadays.
>
> At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
No. A year back, yes, but not anymore. We went to great lengths in the
ARM architecture to ensure NO_IRQ is *always 0. Russell spent
a lot of time on this.
Consult the following article on LWN:
http://lwn.net/Articles/470820/
Then grep your gitlog and you'll see we got rid of it from ARM.
If this driver is for some other arch like openrisc I might accept
it but please reconsider.
Yours,
Linus Walleij
On Fri, Aug 10, 2012 at 10:19:02AM +0200, Linus Walleij wrote:
> On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding
> <[email protected]> wrote:
> > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> >> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> >> > + if (client->irq == NO_IRQ)
> >>
> >> Just if (!client->irq) since NO_IRQ is 0 nowadays.
> >
> > At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
>
> No. A year back, yes, but not anymore. We went to great lengths in the
> ARM architecture to ensure NO_IRQ is *always 0. Russell spent
> a lot of time on this.
>
> Consult the following article on LWN:
> http://lwn.net/Articles/470820/
>
> Then grep your gitlog and you'll see we got rid of it from ARM.
Then why is there still the following in arch/arm/include/asm/irq.h?
/*
* Use this value to indicate lack of interrupt
* capability
*/
#ifndef NO_IRQ
#define NO_IRQ ((unsigned int)(-1))
#endif
> If this driver is for some other arch like openrisc I might accept
> it but please reconsider.
There's nothing in the driver that makes it ARM specific, so it could be
used on other platforms just as well. But as I also said in my previous
mail, in this particular case the value for the interrupt comes from the
call to irq_of_parse_and_map(), which will return 0 on failure,
regardless of the architecture, so there is actually no problem.
Thierry
On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
<[email protected]> wrote:
>> Consult the following article on LWN:
>> http://lwn.net/Articles/470820/
>>
>> Then grep your gitlog and you'll see we got rid of it from ARM.
>
> Then why is there still the following in arch/arm/include/asm/irq.h?
>
> /*
> * Use this value to indicate lack of interrupt
> * capability
> */
> #ifndef NO_IRQ
> #define NO_IRQ ((unsigned int)(-1))
> #endif
That's a question for Russell but I think it's basically there for
old platforms, on a "don't use it"-basis. (Maybe a comment could
be good.)
As you see non-sparse platforms can redefine NO_IRQS in their
<mach/irqs.h> file, but in practice things like the VIC and GIC
drivers have been switched over to using irqdomain which
in turn does *not* allow IRQ 0 to be used, so most platforms
are indirectly disallowed to use IRQ 0 anyway. In fact I think
some of them are just broken now.
>> If this driver is for some other arch like openrisc I might accept
>> it but please reconsider.
>
> There's nothing in the driver that makes it ARM specific, so it could be
> used on other platforms just as well.
The linked article makes it clear that the direction for the entire
kernel is to get rid of NO_IRQ and !irq is used all over the place.
> But as I also said in my previous
> mail, in this particular case the value for the interrupt comes from the
> call to irq_of_parse_and_map(), which will return 0 on failure,
> regardless of the architecture, so there is actually no problem.
OK cool.
Yours,
Linus Walleij
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote:
> On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
> <[email protected]> wrote:
> >> Consult the following article on LWN:
> >> http://lwn.net/Articles/470820/
> >>
> >> Then grep your gitlog and you'll see we got rid of it from ARM.
> >
> > Then why is there still the following in arch/arm/include/asm/irq.h?
> >
> > /*
> > * Use this value to indicate lack of interrupt
> > * capability
> > */
> > #ifndef NO_IRQ
> > #define NO_IRQ ((unsigned int)(-1))
> > #endif
>
> That's a question for Russell but I think it's basically there for
> old platforms, on a "don't use it"-basis. (Maybe a comment could
> be good.)
>
> As you see non-sparse platforms can redefine NO_IRQS in their
> <mach/irqs.h> file, but in practice things like the VIC and GIC
> drivers have been switched over to using irqdomain which
> in turn does *not* allow IRQ 0 to be used, so most platforms
> are indirectly disallowed to use IRQ 0 anyway. In fact I think
> some of them are just broken now.
In that case it might be better to just drop it altogether and wait
until people with the broken platforms start complaining.
Thierry
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote:
> On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
> <[email protected]> wrote:
> >> Consult the following article on LWN:
> >> http://lwn.net/Articles/470820/
> >>
> >> Then grep your gitlog and you'll see we got rid of it from ARM.
> >
> > Then why is there still the following in arch/arm/include/asm/irq.h?
> >
> > /*
> > * Use this value to indicate lack of interrupt
> > * capability
> > */
> > #ifndef NO_IRQ
> > #define NO_IRQ ((unsigned int)(-1))
> > #endif
>
> That's a question for Russell but I think it's basically there for
> old platforms, on a "don't use it"-basis. (Maybe a comment could
> be good.)
Just don't use it. It's there for old stuff which still needs fixing.
New code should not use it, and should test for one of:
irq <= 0
irq == 0
And new code should set irq = 0 to indicate a lack of interrupt.