Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote: > On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote: > On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding > 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 > 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 pgpOCscxTQviC.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:19:02AM +0200, Linus Walleij wrote: > On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding > 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 pgpoLpPlYj3ra.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding thierry.red...@avionic-design.de 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:19:02AM +0200, Linus Walleij wrote: On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding thierry.red...@avionic-design.de 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 pgpoLpPlYj3ra.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding thierry.red...@avionic-design.de 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote: On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding thierry.red...@avionic-design.de 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 pgpOCscxTQviC.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote: On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding thierry.red...@avionic-design.de 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> 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",
> > + _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
pgpTQ1BKHEjMm.pgp
Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote: > On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding > wrote: > > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: > >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding > >> wrote: > > >> > +- interrupt-controller: Marks the device as an interrupt controller. > >> > +- nr-gpios: The number of pins supported by the controller. > >> > >> These two last things look very generic, like something every GPIO > >> driver could want to expose. > > > > As Arnd mentioned, interrupt-controller is a generic property required > > by all interrupt controller nodes. Perhaps it shouldn't be listed in the > > DT binding for this driver. > > After reading Rob's etc comments I think nr-gpios should be in this > binding, but interrupt-controller seems like it should go into > Documentation/devicetree/bindings/gpio/gpio.txt > can you take care of this? > > (Anyone agains, beat me up...) Perhaps this should go into some more generic document (perhaps something like Documentation/devicetree/bindings/interrupts.txt) to describe the common properties for interrupt controllers and refer to that document in bindings that use them. It might be good to document the #interrupt-cells property as well, with the common meaning as defined by the various irq_domain_ops.xlate() callbacks. Do you want me to replace occurrences of this in other binding documents by a reference to the new document while I'm at it? Thierry pgpOdea7mUDdh.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote: On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding thierry.red...@avionic-design.de wrote: On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding thierry.red...@avionic-design.de wrote: +- interrupt-controller: Marks the device as an interrupt controller. +- nr-gpios: The number of pins supported by the controller. These two last things look very generic, like something every GPIO driver could want to expose. As Arnd mentioned, interrupt-controller is a generic property required by all interrupt controller nodes. Perhaps it shouldn't be listed in the DT binding for this driver. After reading Rob's etc comments I think nr-gpios should be in this binding, but interrupt-controller seems like it should go into Documentation/devicetree/bindings/gpio/gpio.txt can you take care of this? (Anyone agains, beat me up...) Perhaps this should go into some more generic document (perhaps something like Documentation/devicetree/bindings/interrupts.txt) to describe the common properties for interrupt controllers and refer to that document in bindings that use them. It might be good to document the #interrupt-cells property as well, with the common meaning as defined by the various irq_domain_ops.xlate() callbacks. Do you want me to replace occurrences of this in other binding documents by a reference to the new document while I'm at it? Thierry pgpOdea7mUDdh.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
thierry.red...@avionic-design.de 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
pgpTQ1BKHEjMm.pgp
Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Mon, Aug 6, 2012 at 7:11 AM, Thierry Reding
wrote:
> On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
>> We're working on a goal of a "single zImage" (one unified ARM
>> kernel) which means your platform must be able to handle the
>> case where this is turned on anyway, so I would suggest you
>> drop the optional compile-time IRQ support, just make it
>> optional at runtime instead.
>
> I don't quite understand. Do you want me to add a module parameter to
> make it optional at runtime? Since the driver is now OF only I suppose I
> could make it optional on the interrupt-controller property as well.
No, no module parameter. Just don't register the IRQ domain if there
are not IRQ resources in the device tree, if the interrupt-controller
property is not present I guess?
>> OK but atleast find a way to move this to the probe() function,
>> what happens if the debugfs file is browsed and you run out
>> of memory? Not nice, and you were using this to debug as
>> well...
>
> Alright, I can do that. Alternatively I could probably drop the
> allocations altogether and use local variables within the second loop to
> store the variables:
>
> for (i = 0; i < num_regs; i++) {
> u8 ddr, plr, ier, isr, ptr;
>
> err = adnp_read(gpio, GPIO_DDR(gpio) + i, );
> if (err < 0)
> goto out;
>
> ...
> }
>
> With the proper locking this shouldn't be a problem. The reason why I
> used the block-wise approach in the first place was that the register
> accesses were more "atomic". Of course without locking this is non-
> sense.
Either approach works, the above seems more elegant though!
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Mon, Aug 6, 2012 at 7:11 AM, Thierry Reding
thierry.red...@avionic-design.de wrote:
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
We're working on a goal of a single zImage (one unified ARM
kernel) which means your platform must be able to handle the
case where this is turned on anyway, so I would suggest you
drop the optional compile-time IRQ support, just make it
optional at runtime instead.
I don't quite understand. Do you want me to add a module parameter to
make it optional at runtime? Since the driver is now OF only I suppose I
could make it optional on the interrupt-controller property as well.
No, no module parameter. Just don't register the IRQ domain if there
are not IRQ resources in the device tree, if the interrupt-controller
property is not present I guess?
OK but atleast find a way to move this to the probe() function,
what happens if the debugfs file is browsed and you run out
of memory? Not nice, and you were using this to debug as
well...
Alright, I can do that. Alternatively I could probably drop the
allocations altogether and use local variables within the second loop to
store the variables:
for (i = 0; i num_regs; i++) {
u8 ddr, plr, ier, isr, ptr;
err = adnp_read(gpio, GPIO_DDR(gpio) + i, ddr);
if (err 0)
goto out;
...
}
With the proper locking this shouldn't be a problem. The reason why I
used the block-wise approach in the first place was that the register
accesses were more atomic. Of course without locking this is non-
sense.
Either approach works, the above seems more elegant though!
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote: > On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding > wrote: > > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: > >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding > >> wrote: > > >> > +- interrupt-controller: Marks the device as an interrupt controller. > >> > +- nr-gpios: The number of pins supported by the controller. > >> > >> These two last things look very generic, like something every GPIO > >> driver could want to expose. > > > > As Arnd mentioned, interrupt-controller is a generic property required > > by all interrupt controller nodes. Perhaps it shouldn't be listed in the > > DT binding for this driver. > > After reading Rob's etc comments I think nr-gpios should be in this > binding, but interrupt-controller seems like it should go into > Documentation/devicetree/bindings/gpio/gpio.txt > can you take care of this? > > (Anyone agains, beat me up...) Yes, that makes sense. The interrupt-controller property is not explicitly mentioned in Documentation/devicetree at all. Perhaps the reason is that it is in fact a standard property and, I think, is already defined by IEEE 1275. I don't think it would hurt to repeat it explicitly for GPIO bindings in general, though. > >> > +config GPIO_ADNP > >> > + tristate "Avionic Design N-bit GPIO expander" > >> > + depends on I2C && OF > >> > + help > >> > + This option enables support for N GPIOs found on Avionic Design > >> > + I2C GPIO expanders. The register space will be extended by > >> > powers > >> > + of two, so the controller will need to accomodate for that. For > >> > + example: if a controller provides 48 pins, 6 registers will be > >> > + enough to represent all pins, but the driver will assume a > >> > + register layout for 64 pins (8 registers). > >> > + > >> > +config GPIO_ADNP_IRQ > >> > + tristate "Interrupt controller support" > >> > + depends on GPIO_ADNP > >> > + help > >> > + Say yes here to enable the Avionic Design N-bit GPIO expander > >> > to > >> > + be used as an interrupt controller. > >> > >> First: please describe the usecase where the Avionic driver is used > >> without making use of the IRQ, and *why* it should be possible > >> to configure this out. E.g. is there a hardware which isn't using the > >> IRQ portions? If there is no non-irq usecase just drop this > >> config option. > > > > This expander is used in a number of Tegra-based boards and handles > > things like enabling or disabling power to a given IC but on other > > boards it is also used to handle interrupts from input devices or > > card-detect for example. > > > > The controller is synthesized in a CPLD, which is one of the reasons for > > the nr-gpios DT property. There is at least one platform that currently > > doesn't use the interrupt functionality. Mainly I allowed this to be > > configured out in order to reduce the number of interrupts required for > > a platform. Another reason was that at the time I first wrote this, IRQ > > domains hadn't been available, so the driver couldn't be built as a > > module if interrupt support was required. This also no longer applies. > > > > I'm actually fine either way, but I thought it'd be most flexible by > > keeping the IRQ support optional for the above reasons. > > We're working on a goal of a "single zImage" (one unified ARM > kernel) which means your platform must be able to handle the > case where this is turned on anyway, so I would suggest you > drop the optional compile-time IRQ support, just make it > optional at runtime instead. I don't quite understand. Do you want me to add a module parameter to make it optional at runtime? Since the driver is now OF only I suppose I could make it optional on the interrupt-controller property as well. > > On that note, provided there is special additional circuitry, the GPIO > > controller is able to detect tristate on an input. I'm not aware that > > the pinctrl subsystem provides for that functionality yet, right? > > pinctrl is usually about *setting* things into tristate, but I'm all > for adding support for getting it as well. But that depends on > the generic pin configurations being adopted first (still most > controllers have their own way of doing pin config, so this > cannot be represented in a generic way). As I mentioned, the only hardware where this was ever used is already EOL and I don't expect this functionality to be required anytime soon for another project. > >> > + base = kzalloc(regs * 5, GFP_KERNEL); > >> > >> Why kzalloc()/kfree() when you can just use a > >> > >> static u8 base[N]; > >> > >> where N is the max number of registers on any HW instead? > > > > As I explained above, the number of pins is configurable, so it'd be > > quite a waste to always assume a maximum of, say, 256 pins if the > > hardware actually only uses 8. > > OK but atleast find
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding wrote: > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding >> wrote: >> > +- interrupt-controller: Marks the device as an interrupt controller. >> > +- nr-gpios: The number of pins supported by the controller. >> >> These two last things look very generic, like something every GPIO >> driver could want to expose. > > As Arnd mentioned, interrupt-controller is a generic property required > by all interrupt controller nodes. Perhaps it shouldn't be listed in the > DT binding for this driver. After reading Rob's etc comments I think nr-gpios should be in this binding, but interrupt-controller seems like it should go into Documentation/devicetree/bindings/gpio/gpio.txt can you take care of this? (Anyone agains, beat me up...) >> > +config GPIO_ADNP >> > + tristate "Avionic Design N-bit GPIO expander" >> > + depends on I2C && OF >> > + help >> > + This option enables support for N GPIOs found on Avionic Design >> > + I2C GPIO expanders. The register space will be extended by powers >> > + of two, so the controller will need to accomodate for that. For >> > + example: if a controller provides 48 pins, 6 registers will be >> > + enough to represent all pins, but the driver will assume a >> > + register layout for 64 pins (8 registers). >> > + >> > +config GPIO_ADNP_IRQ >> > + tristate "Interrupt controller support" >> > + depends on GPIO_ADNP >> > + help >> > + Say yes here to enable the Avionic Design N-bit GPIO expander to >> > + be used as an interrupt controller. >> >> First: please describe the usecase where the Avionic driver is used >> without making use of the IRQ, and *why* it should be possible >> to configure this out. E.g. is there a hardware which isn't using the >> IRQ portions? If there is no non-irq usecase just drop this >> config option. > > This expander is used in a number of Tegra-based boards and handles > things like enabling or disabling power to a given IC but on other > boards it is also used to handle interrupts from input devices or > card-detect for example. > > The controller is synthesized in a CPLD, which is one of the reasons for > the nr-gpios DT property. There is at least one platform that currently > doesn't use the interrupt functionality. Mainly I allowed this to be > configured out in order to reduce the number of interrupts required for > a platform. Another reason was that at the time I first wrote this, IRQ > domains hadn't been available, so the driver couldn't be built as a > module if interrupt support was required. This also no longer applies. > > I'm actually fine either way, but I thought it'd be most flexible by > keeping the IRQ support optional for the above reasons. We're working on a goal of a "single zImage" (one unified ARM kernel) which means your platform must be able to handle the case where this is turned on anyway, so I would suggest you drop the optional compile-time IRQ support, just make it optional at runtime instead. >> > + u8 *irq_mask; >> > + u8 *irq_mask_cur; >> > + u8 *irq_level; >> > + u8 *irq_rise; >> > + u8 *irq_fall; >> > + u8 *irq_high; >> > + u8 *irq_low; >> >> Some or all of this looks like a regmap reimplementation, see below. > > Actually none of these represent actual registers, except for irq_mask > and irq_mask_cur. They are used to emulate various IRQ trigger modes. OK. >> When I do this at several places in a driver I usually define >> a macro like >> >> #define to_adnp(foo) container_of(foo, struct adnp, gpio) >> >> Used like so: >> >> struct adnp *adnp = to_adnp(chip); > > Yes, I usually do that as well. I guess this driver has been in the > works in too many variants for too long. =) OK expect it to be changed in v3 then... >> > + unsigned int reg = offset >> gpio->reg_shift; >> > + unsigned int pos = offset & 7; >> > + u8 value; >> > + int err; >> > + >> > + mutex_lock(>i2c_lock); >> >> The point of taking this mutex here avoids me. >> adnp_read() only performs a single transaction. > > I think that's a relic from an earlier version that used to access the > PTR (Pin Tristate Register) as well. At the time I used to return 2 here > to signify a tristated input, which was dependent on the contents of the > PTR. Tristating an output is, I believe, better done using pinmux/ > pinctrl nowadays, so I took that code because the only platform where > this was ever used will probably never be mainlined. OK so will be gone in v3 I guess. > On that note, provided there is special additional circuitry, the GPIO > controller is able to detect tristate on an input. I'm not aware that > the pinctrl subsystem provides for that functionality yet, right? pinctrl is usually about *setting* things into tristate, but I'm all for adding support for
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding thierry.red...@avionic-design.de wrote: On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding thierry.red...@avionic-design.de wrote: +- interrupt-controller: Marks the device as an interrupt controller. +- nr-gpios: The number of pins supported by the controller. These two last things look very generic, like something every GPIO driver could want to expose. As Arnd mentioned, interrupt-controller is a generic property required by all interrupt controller nodes. Perhaps it shouldn't be listed in the DT binding for this driver. After reading Rob's etc comments I think nr-gpios should be in this binding, but interrupt-controller seems like it should go into Documentation/devicetree/bindings/gpio/gpio.txt can you take care of this? (Anyone agains, beat me up...) +config GPIO_ADNP + tristate Avionic Design N-bit GPIO expander + depends on I2C OF + help + This option enables support for N GPIOs found on Avionic Design + I2C GPIO expanders. The register space will be extended by powers + of two, so the controller will need to accomodate for that. For + example: if a controller provides 48 pins, 6 registers will be + enough to represent all pins, but the driver will assume a + register layout for 64 pins (8 registers). + +config GPIO_ADNP_IRQ + tristate Interrupt controller support + depends on GPIO_ADNP + help + Say yes here to enable the Avionic Design N-bit GPIO expander to + be used as an interrupt controller. First: please describe the usecase where the Avionic driver is used without making use of the IRQ, and *why* it should be possible to configure this out. E.g. is there a hardware which isn't using the IRQ portions? If there is no non-irq usecase just drop this config option. This expander is used in a number of Tegra-based boards and handles things like enabling or disabling power to a given IC but on other boards it is also used to handle interrupts from input devices or card-detect for example. The controller is synthesized in a CPLD, which is one of the reasons for the nr-gpios DT property. There is at least one platform that currently doesn't use the interrupt functionality. Mainly I allowed this to be configured out in order to reduce the number of interrupts required for a platform. Another reason was that at the time I first wrote this, IRQ domains hadn't been available, so the driver couldn't be built as a module if interrupt support was required. This also no longer applies. I'm actually fine either way, but I thought it'd be most flexible by keeping the IRQ support optional for the above reasons. We're working on a goal of a single zImage (one unified ARM kernel) which means your platform must be able to handle the case where this is turned on anyway, so I would suggest you drop the optional compile-time IRQ support, just make it optional at runtime instead. + u8 *irq_mask; + u8 *irq_mask_cur; + u8 *irq_level; + u8 *irq_rise; + u8 *irq_fall; + u8 *irq_high; + u8 *irq_low; Some or all of this looks like a regmap reimplementation, see below. Actually none of these represent actual registers, except for irq_mask and irq_mask_cur. They are used to emulate various IRQ trigger modes. OK. When I do this at several places in a driver I usually define a macro like #define to_adnp(foo) container_of(foo, struct adnp, gpio) Used like so: struct adnp *adnp = to_adnp(chip); Yes, I usually do that as well. I guess this driver has been in the works in too many variants for too long. =) OK expect it to be changed in v3 then... + unsigned int reg = offset gpio-reg_shift; + unsigned int pos = offset 7; + u8 value; + int err; + + mutex_lock(gpio-i2c_lock); The point of taking this mutex here avoids me. adnp_read() only performs a single transaction. I think that's a relic from an earlier version that used to access the PTR (Pin Tristate Register) as well. At the time I used to return 2 here to signify a tristated input, which was dependent on the contents of the PTR. Tristating an output is, I believe, better done using pinmux/ pinctrl nowadays, so I took that code because the only platform where this was ever used will probably never be mainlined. OK so will be gone in v3 I guess. On that note, provided there is special additional circuitry, the GPIO controller is able to detect tristate on an input. I'm not aware that the pinctrl subsystem provides for that functionality yet, right? pinctrl is usually about *setting* things into tristate, but I'm all for adding support for getting it as well. But that depends on the generic pin configurations being adopted first (still most controllers have their own
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote: On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding thierry.red...@avionic-design.de wrote: On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding thierry.red...@avionic-design.de wrote: +- interrupt-controller: Marks the device as an interrupt controller. +- nr-gpios: The number of pins supported by the controller. These two last things look very generic, like something every GPIO driver could want to expose. As Arnd mentioned, interrupt-controller is a generic property required by all interrupt controller nodes. Perhaps it shouldn't be listed in the DT binding for this driver. After reading Rob's etc comments I think nr-gpios should be in this binding, but interrupt-controller seems like it should go into Documentation/devicetree/bindings/gpio/gpio.txt can you take care of this? (Anyone agains, beat me up...) Yes, that makes sense. The interrupt-controller property is not explicitly mentioned in Documentation/devicetree at all. Perhaps the reason is that it is in fact a standard property and, I think, is already defined by IEEE 1275. I don't think it would hurt to repeat it explicitly for GPIO bindings in general, though. +config GPIO_ADNP + tristate Avionic Design N-bit GPIO expander + depends on I2C OF + help + This option enables support for N GPIOs found on Avionic Design + I2C GPIO expanders. The register space will be extended by powers + of two, so the controller will need to accomodate for that. For + example: if a controller provides 48 pins, 6 registers will be + enough to represent all pins, but the driver will assume a + register layout for 64 pins (8 registers). + +config GPIO_ADNP_IRQ + tristate Interrupt controller support + depends on GPIO_ADNP + help + Say yes here to enable the Avionic Design N-bit GPIO expander to + be used as an interrupt controller. First: please describe the usecase where the Avionic driver is used without making use of the IRQ, and *why* it should be possible to configure this out. E.g. is there a hardware which isn't using the IRQ portions? If there is no non-irq usecase just drop this config option. This expander is used in a number of Tegra-based boards and handles things like enabling or disabling power to a given IC but on other boards it is also used to handle interrupts from input devices or card-detect for example. The controller is synthesized in a CPLD, which is one of the reasons for the nr-gpios DT property. There is at least one platform that currently doesn't use the interrupt functionality. Mainly I allowed this to be configured out in order to reduce the number of interrupts required for a platform. Another reason was that at the time I first wrote this, IRQ domains hadn't been available, so the driver couldn't be built as a module if interrupt support was required. This also no longer applies. I'm actually fine either way, but I thought it'd be most flexible by keeping the IRQ support optional for the above reasons. We're working on a goal of a single zImage (one unified ARM kernel) which means your platform must be able to handle the case where this is turned on anyway, so I would suggest you drop the optional compile-time IRQ support, just make it optional at runtime instead. I don't quite understand. Do you want me to add a module parameter to make it optional at runtime? Since the driver is now OF only I suppose I could make it optional on the interrupt-controller property as well. On that note, provided there is special additional circuitry, the GPIO controller is able to detect tristate on an input. I'm not aware that the pinctrl subsystem provides for that functionality yet, right? pinctrl is usually about *setting* things into tristate, but I'm all for adding support for getting it as well. But that depends on the generic pin configurations being adopted first (still most controllers have their own way of doing pin config, so this cannot be represented in a generic way). As I mentioned, the only hardware where this was ever used is already EOL and I don't expect this functionality to be required anytime soon for another project. + base = kzalloc(regs * 5, GFP_KERNEL); Why kzalloc()/kfree() when you can just use a static u8 base[N]; where N is the max number of registers on any HW instead? As I explained above, the number of pins is configurable, so it'd be quite a waste to always assume a maximum of, say, 256 pins if the hardware actually only uses 8. OK but atleast find a way to move this to the probe() function, what happens if the debugfs file is browsed and you run out of memory? Not nice, and you were using this to debug as
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Tue, Jul 31, 2012 at 05:03:05PM -0500, Rob Herring wrote: > For nr-gpios, I think it is typically not needed. Generally, you will > know how many gpio lines the h/w has based on the compatible string. If > this part really is the same part but different packages with different > numbers of gpio, then this property makes sense. That's exactly what I need it for. The controller is synthesized in a CPLD, so technically the part is always the same, but the configured logic can implement any number of GPIOs. Thierry pgpJLaYwYBts2.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Tue, Jul 31, 2012 at 05:03:05PM -0500, Rob Herring wrote: For nr-gpios, I think it is typically not needed. Generally, you will know how many gpio lines the h/w has based on the compatible string. If this part really is the same part but different packages with different numbers of gpio, then this property makes sense. That's exactly what I need it for. The controller is synthesized in a CPLD, so technically the part is always the same, but the configured logic can implement any number of GPIOs. Thierry pgpJLaYwYBts2.pgp Description: PGP signature
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On 07/31/2012 04:03 PM, Rob Herring wrote: > On 07/30/2012 02:47 AM, Thierry Reding wrote: >> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: >>> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding >>> wrote: >>> This commit adds a driver for the Avionic Design N-bit GPIO expander. The expander provides a variable number of GPIO pins with interrupt support. diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt +Required properties: +- nr-gpios: The number of pins supported by the controller. > For nr-gpios, I think it is typically not needed. Generally, you will > know how many gpio lines the h/w has based on the compatible string. If > this part really is the same part but different packages with different > numbers of gpio, then this property makes sense. Otherwise, I would say > drop it. > > If your goal is how many gpios are you using, you really need a bitmap > instead if you want it to be generic. > > IIRC, Tegra also needed this in that they N instances of some number of > bits and the registers are interleaved so they can't have separate nodes. In the end, I got away without having a separate property to represent this. Instead, the code keys off the number of interrupts listed in the interrupts property, there being one interrupt per GPIO bank, and hence dynamically instantiates the number of banks based on that. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On 07/30/2012 02:47 AM, Thierry Reding wrote: > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding >> wrote: >> >>> This commit adds a driver for the Avionic Design N-bit GPIO expander. >>> The expander provides a variable number of GPIO pins with interrupt >>> support. >> (...) >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt >>> b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt >>> new file mode 100644 >>> index 000..513a18e >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt >>> @@ -0,0 +1,38 @@ >>> +Avionic Design N-bit GPIO expander bindings >>> + >>> +Required properties: >>> +- compatible: should be "ad,gpio-adnp" >>> +- reg: The I2C slave address for this device. >>> +- interrupt-parent: phandle of the parent interrupt controller. >>> +- interrupts: Interrupt specifier for the controllers interrupt. >>> +- #gpio-cells: Should be 2. The first cell is the GPIO number and the >>> + second cell is used to specify optional parameters: >>> + - bit 0: polarity (0: normal, 1: inverted) >>> +- gpio-controller: Marks the device as a GPIO controller >>> +- #interrupt-cells: Should be 2. The first cell contains the GPIO number, >>> + whereas the second cell is used to specify flags: >>> +bits[3:0] trigger type and level flags >>> + 1 = low-to-high edge triggered >>> + 2 = high-to-low edge triggered >>> + 4 = active high level-sensitive >>> + 8 = active low level-sensitive >> >> Why on earth would a bunch of flags be an "interrupt cell"? >> >> Maybe there is something about DT bindings I don't get so >> please educate me. >> >> I can see that OMAP is doing this, but is it a good idea? >> I really need Rob/Grant to comment on this. >> >>> +- interrupt-controller: Marks the device as an interrupt controller. >>> +- nr-gpios: The number of pins supported by the controller. >> >> These two last things look very generic, like something every GPIO >> driver could want to expose. > > As Arnd mentioned, interrupt-controller is a generic property required > by all interrupt controller nodes. Perhaps it shouldn't be listed in the > DT binding for this driver. > > As to the nr-gpios property, this is actually not only for informational > purposes, but it also allows the driver to be configured to handle any > number of bits (powers of two). However since this is also a description > of the hardware it may be useful to make this into a generic property > for GPIO controllers. > >> I'd really like to have Grant's word on GPIO DT bindings and how these >> should look, I had some discussion with Wolfram (the I2C maintainer) >> about bindings turning out less generic than they ought to be, so we >> need some discussion on this. >> >> Arnd recently consolidated some MMC props, maybe we need to do >> the same for GPIO drivers. For nr-gpios, I think it is typically not needed. Generally, you will know how many gpio lines the h/w has based on the compatible string. If this part really is the same part but different packages with different numbers of gpio, then this property makes sense. Otherwise, I would say drop it. If your goal is how many gpios are you using, you really need a bitmap instead if you want it to be generic. IIRC, Tegra also needed this in that they N instances of some number of bits and the registers are interleaved so they can't have separate nodes. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On 07/30/2012 02:47 AM, Thierry Reding wrote: On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding thierry.red...@avionic-design.de wrote: This commit adds a driver for the Avionic Design N-bit GPIO expander. The expander provides a variable number of GPIO pins with interrupt support. (...) diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt new file mode 100644 index 000..513a18e --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt @@ -0,0 +1,38 @@ +Avionic Design N-bit GPIO expander bindings + +Required properties: +- compatible: should be ad,gpio-adnp +- reg: The I2C slave address for this device. +- interrupt-parent: phandle of the parent interrupt controller. +- interrupts: Interrupt specifier for the controllers interrupt. +- #gpio-cells: Should be 2. The first cell is the GPIO number and the + second cell is used to specify optional parameters: + - bit 0: polarity (0: normal, 1: inverted) +- gpio-controller: Marks the device as a GPIO controller +- #interrupt-cells: Should be 2. The first cell contains the GPIO number, + whereas the second cell is used to specify flags: +bits[3:0] trigger type and level flags + 1 = low-to-high edge triggered + 2 = high-to-low edge triggered + 4 = active high level-sensitive + 8 = active low level-sensitive Why on earth would a bunch of flags be an interrupt cell? Maybe there is something about DT bindings I don't get so please educate me. I can see that OMAP is doing this, but is it a good idea? I really need Rob/Grant to comment on this. +- interrupt-controller: Marks the device as an interrupt controller. +- nr-gpios: The number of pins supported by the controller. These two last things look very generic, like something every GPIO driver could want to expose. As Arnd mentioned, interrupt-controller is a generic property required by all interrupt controller nodes. Perhaps it shouldn't be listed in the DT binding for this driver. As to the nr-gpios property, this is actually not only for informational purposes, but it also allows the driver to be configured to handle any number of bits (powers of two). However since this is also a description of the hardware it may be useful to make this into a generic property for GPIO controllers. I'd really like to have Grant's word on GPIO DT bindings and how these should look, I had some discussion with Wolfram (the I2C maintainer) about bindings turning out less generic than they ought to be, so we need some discussion on this. Arnd recently consolidated some MMC props, maybe we need to do the same for GPIO drivers. For nr-gpios, I think it is typically not needed. Generally, you will know how many gpio lines the h/w has based on the compatible string. If this part really is the same part but different packages with different numbers of gpio, then this property makes sense. Otherwise, I would say drop it. If your goal is how many gpios are you using, you really need a bitmap instead if you want it to be generic. IIRC, Tegra also needed this in that they N instances of some number of bits and the registers are interleaved so they can't have separate nodes. Rob -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On 07/31/2012 04:03 PM, Rob Herring wrote: On 07/30/2012 02:47 AM, Thierry Reding wrote: On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding thierry.red...@avionic-design.de wrote: This commit adds a driver for the Avionic Design N-bit GPIO expander. The expander provides a variable number of GPIO pins with interrupt support. diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt +Required properties: +- nr-gpios: The number of pins supported by the controller. For nr-gpios, I think it is typically not needed. Generally, you will know how many gpio lines the h/w has based on the compatible string. If this part really is the same part but different packages with different numbers of gpio, then this property makes sense. Otherwise, I would say drop it. If your goal is how many gpios are you using, you really need a bitmap instead if you want it to be generic. IIRC, Tegra also needed this in that they N instances of some number of bits and the registers are interleaved so they can't have separate nodes. In the end, I got away without having a separate property to represent this. Instead, the code keys off the number of interrupts listed in the interrupts property, there being one interrupt per GPIO bank, and hence dynamically instantiates the number of banks based on that. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: > On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding > wrote: > > > This commit adds a driver for the Avionic Design N-bit GPIO expander. > > The expander provides a variable number of GPIO pins with interrupt > > support. > (...) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt > > b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt > > new file mode 100644 > > index 000..513a18e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt > > @@ -0,0 +1,38 @@ > > +Avionic Design N-bit GPIO expander bindings > > + > > +Required properties: > > +- compatible: should be "ad,gpio-adnp" > > +- reg: The I2C slave address for this device. > > +- interrupt-parent: phandle of the parent interrupt controller. > > +- interrupts: Interrupt specifier for the controllers interrupt. > > +- #gpio-cells: Should be 2. The first cell is the GPIO number and the > > + second cell is used to specify optional parameters: > > + - bit 0: polarity (0: normal, 1: inverted) > > +- gpio-controller: Marks the device as a GPIO controller > > +- #interrupt-cells: Should be 2. The first cell contains the GPIO number, > > + whereas the second cell is used to specify flags: > > +bits[3:0] trigger type and level flags > > + 1 = low-to-high edge triggered > > + 2 = high-to-low edge triggered > > + 4 = active high level-sensitive > > + 8 = active low level-sensitive > > Why on earth would a bunch of flags be an "interrupt cell"? > > Maybe there is something about DT bindings I don't get so > please educate me. > > I can see that OMAP is doing this, but is it a good idea? > I really need Rob/Grant to comment on this. > > > +- interrupt-controller: Marks the device as an interrupt controller. > > +- nr-gpios: The number of pins supported by the controller. > > These two last things look very generic, like something every GPIO > driver could want to expose. As Arnd mentioned, interrupt-controller is a generic property required by all interrupt controller nodes. Perhaps it shouldn't be listed in the DT binding for this driver. As to the nr-gpios property, this is actually not only for informational purposes, but it also allows the driver to be configured to handle any number of bits (powers of two). However since this is also a description of the hardware it may be useful to make this into a generic property for GPIO controllers. > I'd really like to have Grant's word on GPIO DT bindings and how these > should look, I had some discussion with Wolfram (the I2C maintainer) > about bindings turning out less generic than they ought to be, so we > need some discussion on this. > > Arnd recently consolidated some MMC props, maybe we need to do > the same for GPIO drivers. > > (...) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > > index 502b5ea..d1b0f7d 100644 > > --- a/drivers/gpio/Kconfig > > +++ b/drivers/gpio/Kconfig > > @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ > > Say yes here to enable the adp5588 to be used as an interrupt > > controller. It requires the driver to be built in the kernel. > > > > +config GPIO_ADNP > > + tristate "Avionic Design N-bit GPIO expander" > > + depends on I2C && OF > > + help > > + This option enables support for N GPIOs found on Avionic Design > > + I2C GPIO expanders. The register space will be extended by powers > > + of two, so the controller will need to accomodate for that. For > > + example: if a controller provides 48 pins, 6 registers will be > > + enough to represent all pins, but the driver will assume a > > + register layout for 64 pins (8 registers). > > + > > +config GPIO_ADNP_IRQ > > + tristate "Interrupt controller support" > > + depends on GPIO_ADNP > > + help > > + Say yes here to enable the Avionic Design N-bit GPIO expander to > > + be used as an interrupt controller. > > First: please describe the usecase where the Avionic driver is used > without making use of the IRQ, and *why* it should be possible > to configure this out. E.g. is there a hardware which isn't using the > IRQ portions? If there is no non-irq usecase just drop this > config option. This expander is used in a number of Tegra-based boards and handles things like enabling or disabling power to a given IC but on other boards it is also used to handle interrupts from input devices or card-detect for example. The controller is synthesized in a CPLD, which is one of the reasons for the nr-gpios DT property. There is at least one platform that currently doesn't use the interrupt functionality. Mainly I allowed this to be configured out in order to reduce the number of interrupts required for a platform. Another reason was that at the time I first wrote this, IRQ domains hadn't been available, so the driver couldn't be built as a module if interrupt
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote: On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding thierry.red...@avionic-design.de wrote: This commit adds a driver for the Avionic Design N-bit GPIO expander. The expander provides a variable number of GPIO pins with interrupt support. (...) diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt new file mode 100644 index 000..513a18e --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt @@ -0,0 +1,38 @@ +Avionic Design N-bit GPIO expander bindings + +Required properties: +- compatible: should be ad,gpio-adnp +- reg: The I2C slave address for this device. +- interrupt-parent: phandle of the parent interrupt controller. +- interrupts: Interrupt specifier for the controllers interrupt. +- #gpio-cells: Should be 2. The first cell is the GPIO number and the + second cell is used to specify optional parameters: + - bit 0: polarity (0: normal, 1: inverted) +- gpio-controller: Marks the device as a GPIO controller +- #interrupt-cells: Should be 2. The first cell contains the GPIO number, + whereas the second cell is used to specify flags: +bits[3:0] trigger type and level flags + 1 = low-to-high edge triggered + 2 = high-to-low edge triggered + 4 = active high level-sensitive + 8 = active low level-sensitive Why on earth would a bunch of flags be an interrupt cell? Maybe there is something about DT bindings I don't get so please educate me. I can see that OMAP is doing this, but is it a good idea? I really need Rob/Grant to comment on this. +- interrupt-controller: Marks the device as an interrupt controller. +- nr-gpios: The number of pins supported by the controller. These two last things look very generic, like something every GPIO driver could want to expose. As Arnd mentioned, interrupt-controller is a generic property required by all interrupt controller nodes. Perhaps it shouldn't be listed in the DT binding for this driver. As to the nr-gpios property, this is actually not only for informational purposes, but it also allows the driver to be configured to handle any number of bits (powers of two). However since this is also a description of the hardware it may be useful to make this into a generic property for GPIO controllers. I'd really like to have Grant's word on GPIO DT bindings and how these should look, I had some discussion with Wolfram (the I2C maintainer) about bindings turning out less generic than they ought to be, so we need some discussion on this. Arnd recently consolidated some MMC props, maybe we need to do the same for GPIO drivers. (...) diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 502b5ea..d1b0f7d 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ Say yes here to enable the adp5588 to be used as an interrupt controller. It requires the driver to be built in the kernel. +config GPIO_ADNP + tristate Avionic Design N-bit GPIO expander + depends on I2C OF + help + This option enables support for N GPIOs found on Avionic Design + I2C GPIO expanders. The register space will be extended by powers + of two, so the controller will need to accomodate for that. For + example: if a controller provides 48 pins, 6 registers will be + enough to represent all pins, but the driver will assume a + register layout for 64 pins (8 registers). + +config GPIO_ADNP_IRQ + tristate Interrupt controller support + depends on GPIO_ADNP + help + Say yes here to enable the Avionic Design N-bit GPIO expander to + be used as an interrupt controller. First: please describe the usecase where the Avionic driver is used without making use of the IRQ, and *why* it should be possible to configure this out. E.g. is there a hardware which isn't using the IRQ portions? If there is no non-irq usecase just drop this config option. This expander is used in a number of Tegra-based boards and handles things like enabling or disabling power to a given IC but on other boards it is also used to handle interrupts from input devices or card-detect for example. The controller is synthesized in a CPLD, which is one of the reasons for the nr-gpios DT property. There is at least one platform that currently doesn't use the interrupt functionality. Mainly I allowed this to be configured out in order to reduce the number of interrupts required for a platform. Another reason was that at the time I first wrote this, IRQ domains hadn't been available, so the driver couldn't be built as a module if interrupt support was required. This also no longer applies. I'm actually fine either way, but I thought it'd be most flexible by
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sunday 29 July 2012, Linus Walleij wrote: > > +- #interrupt-cells: Should be 2. The first cell contains the GPIO number, > > + whereas the second cell is used to specify flags: > > +bits[3:0] trigger type and level flags > > + 1 = low-to-high edge triggered > > + 2 = high-to-low edge triggered > > + 4 = active high level-sensitive > > + 8 = active low level-sensitive > > Why on earth would a bunch of flags be an "interrupt cell"? > > Maybe there is something about DT bindings I don't get so > please educate me. > > I can see that OMAP is doing this, but is it a good idea? > I really need Rob/Grant to comment on this. It is in fact a good idea, and it's very common to have it. The point is that the interrupt controller requires this setting in order to configure that IRQ for use, so putting it into a property of the device that triggers the IRQ is the right place to have it. > > +- interrupt-controller: Marks the device as an interrupt controller. > > +- nr-gpios: The number of pins supported by the controller. > > These two last things look very generic, like something every GPIO > driver could want to expose. > > I'd really like to have Grant's word on GPIO DT bindings and how these > should look, I had some discussion with Wolfram (the I2C maintainer) > about bindings turning out less generic than they ought to be, so we > need some discussion on this. > > Arnd recently consolidated some MMC props, maybe we need to do > the same for GPIO drivers. The "interrupt-controller" property is standardized by IEEE. Not sure about what specifies the "nr-gpios" one, but it seems to be a good idea to have everyone do the same thing here. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
wrote:
> This commit adds a driver for the Avionic Design N-bit GPIO expander.
> The expander provides a variable number of GPIO pins with interrupt
> support.
(...)
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> new file mode 100644
> index 000..513a18e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> @@ -0,0 +1,38 @@
> +Avionic Design N-bit GPIO expander bindings
> +
> +Required properties:
> +- compatible: should be "ad,gpio-adnp"
> +- reg: The I2C slave address for this device.
> +- interrupt-parent: phandle of the parent interrupt controller.
> +- interrupts: Interrupt specifier for the controllers interrupt.
> +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
> + second cell is used to specify optional parameters:
> + - bit 0: polarity (0: normal, 1: inverted)
> +- gpio-controller: Marks the device as a GPIO controller
> +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> + whereas the second cell is used to specify flags:
> +bits[3:0] trigger type and level flags
> + 1 = low-to-high edge triggered
> + 2 = high-to-low edge triggered
> + 4 = active high level-sensitive
> + 8 = active low level-sensitive
Why on earth would a bunch of flags be an "interrupt cell"?
Maybe there is something about DT bindings I don't get so
please educate me.
I can see that OMAP is doing this, but is it a good idea?
I really need Rob/Grant to comment on this.
> +- interrupt-controller: Marks the device as an interrupt controller.
> +- nr-gpios: The number of pins supported by the controller.
These two last things look very generic, like something every GPIO
driver could want to expose.
I'd really like to have Grant's word on GPIO DT bindings and how these
should look, I had some discussion with Wolfram (the I2C maintainer)
about bindings turning out less generic than they ought to be, so we
need some discussion on this.
Arnd recently consolidated some MMC props, maybe we need to do
the same for GPIO drivers.
(...)
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 502b5ea..d1b0f7d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
> Say yes here to enable the adp5588 to be used as an interrupt
> controller. It requires the driver to be built in the kernel.
>
> +config GPIO_ADNP
> + tristate "Avionic Design N-bit GPIO expander"
> + depends on I2C && OF
> + help
> + This option enables support for N GPIOs found on Avionic Design
> + I2C GPIO expanders. The register space will be extended by powers
> + of two, so the controller will need to accomodate for that. For
> + example: if a controller provides 48 pins, 6 registers will be
> + enough to represent all pins, but the driver will assume a
> + register layout for 64 pins (8 registers).
> +
> +config GPIO_ADNP_IRQ
> + tristate "Interrupt controller support"
> + depends on GPIO_ADNP
> + help
> + Say yes here to enable the Avionic Design N-bit GPIO expander to
> + be used as an interrupt controller.
First: please describe the usecase where the Avionic driver is used
without making use of the IRQ, and *why* it should be possible
to configure this out. E.g. is there a hardware which isn't using the
IRQ portions? If there is no non-irq usecase just drop this
config option.
If you're keeping it:
Now this is going to appear as a single line under the ADNP driver
config entry saying "Interrupt controller support", which is too
generic.
Please make it expand to a submenu so that it gets indented below the
controller driver.
(...)
> diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
> new file mode 100644
> index 000..c122ff4
> --- /dev/null
> +++ b/drivers/gpio/gpio-adnp.c
> @@ -0,0 +1,615 @@
> +/*
> + * Copyright (C) 2011-2012 Avionic Design GmbH
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
Good!
> +#include
> +#include
> +#include
> +#include
> +
> +#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
> +#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
> +#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
> +#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
> +#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
> +
> +struct adnp {
> + struct i2c_client *client;
> + struct gpio_chip gpio;
> + unsigned int reg_shift;
> +
> + struct mutex i2c_lock;
> +
> + struct irq_domain *domain;
> + struct mutex irq_lock;
> +
> + u8 *irq_mask;
> + u8 *irq_mask_cur;
> +
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
thierry.red...@avionic-design.de wrote:
This commit adds a driver for the Avionic Design N-bit GPIO expander.
The expander provides a variable number of GPIO pins with interrupt
support.
(...)
diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
new file mode 100644
index 000..513a18e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
@@ -0,0 +1,38 @@
+Avionic Design N-bit GPIO expander bindings
+
+Required properties:
+- compatible: should be ad,gpio-adnp
+- reg: The I2C slave address for this device.
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: Interrupt specifier for the controllers interrupt.
+- #gpio-cells: Should be 2. The first cell is the GPIO number and the
+ second cell is used to specify optional parameters:
+ - bit 0: polarity (0: normal, 1: inverted)
+- gpio-controller: Marks the device as a GPIO controller
+- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
+ whereas the second cell is used to specify flags:
+bits[3:0] trigger type and level flags
+ 1 = low-to-high edge triggered
+ 2 = high-to-low edge triggered
+ 4 = active high level-sensitive
+ 8 = active low level-sensitive
Why on earth would a bunch of flags be an interrupt cell?
Maybe there is something about DT bindings I don't get so
please educate me.
I can see that OMAP is doing this, but is it a good idea?
I really need Rob/Grant to comment on this.
+- interrupt-controller: Marks the device as an interrupt controller.
+- nr-gpios: The number of pins supported by the controller.
These two last things look very generic, like something every GPIO
driver could want to expose.
I'd really like to have Grant's word on GPIO DT bindings and how these
should look, I had some discussion with Wolfram (the I2C maintainer)
about bindings turning out less generic than they ought to be, so we
need some discussion on this.
Arnd recently consolidated some MMC props, maybe we need to do
the same for GPIO drivers.
(...)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 502b5ea..d1b0f7d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
Say yes here to enable the adp5588 to be used as an interrupt
controller. It requires the driver to be built in the kernel.
+config GPIO_ADNP
+ tristate Avionic Design N-bit GPIO expander
+ depends on I2C OF
+ help
+ This option enables support for N GPIOs found on Avionic Design
+ I2C GPIO expanders. The register space will be extended by powers
+ of two, so the controller will need to accomodate for that. For
+ example: if a controller provides 48 pins, 6 registers will be
+ enough to represent all pins, but the driver will assume a
+ register layout for 64 pins (8 registers).
+
+config GPIO_ADNP_IRQ
+ tristate Interrupt controller support
+ depends on GPIO_ADNP
+ help
+ Say yes here to enable the Avionic Design N-bit GPIO expander to
+ be used as an interrupt controller.
First: please describe the usecase where the Avionic driver is used
without making use of the IRQ, and *why* it should be possible
to configure this out. E.g. is there a hardware which isn't using the
IRQ portions? If there is no non-irq usecase just drop this
config option.
If you're keeping it:
Now this is going to appear as a single line under the ADNP driver
config entry saying Interrupt controller support, which is too
generic.
Please make it expand to a submenu so that it gets indented below the
controller driver.
(...)
diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
new file mode 100644
index 000..c122ff4
--- /dev/null
+++ b/drivers/gpio/gpio-adnp.c
@@ -0,0 +1,615 @@
+/*
+ * Copyright (C) 2011-2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/gpio.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/irqdomain.h
Good!
+#include linux/module.h
+#include linux/of_irq.h
+#include linux/seq_file.h
+#include linux/slab.h
+
+#define GPIO_DDR(gpio) (0x00 (gpio)-reg_shift)
+#define GPIO_PLR(gpio) (0x01 (gpio)-reg_shift)
+#define GPIO_IER(gpio) (0x02 (gpio)-reg_shift)
+#define GPIO_ISR(gpio) (0x03 (gpio)-reg_shift)
+#define GPIO_PTR(gpio) (0x04 (gpio)-reg_shift)
+
+struct adnp {
+ struct i2c_client *client;
+ struct gpio_chip gpio;
+ unsigned int reg_shift;
+
+ struct mutex i2c_lock;
+
+ struct irq_domain *domain;
+ struct mutex irq_lock;
+
+ u8 *irq_mask;
+
Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
On Sunday 29 July 2012, Linus Walleij wrote: +- #interrupt-cells: Should be 2. The first cell contains the GPIO number, + whereas the second cell is used to specify flags: +bits[3:0] trigger type and level flags + 1 = low-to-high edge triggered + 2 = high-to-low edge triggered + 4 = active high level-sensitive + 8 = active low level-sensitive Why on earth would a bunch of flags be an interrupt cell? Maybe there is something about DT bindings I don't get so please educate me. I can see that OMAP is doing this, but is it a good idea? I really need Rob/Grant to comment on this. It is in fact a good idea, and it's very common to have it. The point is that the interrupt controller requires this setting in order to configure that IRQ for use, so putting it into a property of the device that triggers the IRQ is the right place to have it. +- interrupt-controller: Marks the device as an interrupt controller. +- nr-gpios: The number of pins supported by the controller. These two last things look very generic, like something every GPIO driver could want to expose. I'd really like to have Grant's word on GPIO DT bindings and how these should look, I had some discussion with Wolfram (the I2C maintainer) about bindings turning out less generic than they ought to be, so we need some discussion on this. Arnd recently consolidated some MMC props, maybe we need to do the same for GPIO drivers. The interrupt-controller property is standardized by IEEE. Not sure about what specifies the nr-gpios one, but it seems to be a good idea to have everyone do the same thing here. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
This commit adds a driver for the Avionic Design N-bit GPIO expander.
The expander provides a variable number of GPIO pins with interrupt
support.
Cc: Grant Likely
Cc: Rob Herring
Cc: devicetree-disc...@lists.ozlabs.org
Cc: Linus Walleij
Signed-off-by: Thierry Reding
---
Changes in v2:
- allow building the driver as a module
- assign of_node unconditionally
- use linear mapping IRQ domain
- properly cleanup IRQ domain
- add OF device table and annotate device tables
- emulate rising and falling edge triggers
- increase #gpio-cells to 2
- drop support for !OF
- use IS_ENABLED to conditionalize DEBUG_FS code
---
.../devicetree/bindings/gpio/gpio-adnp.txt | 38 ++
drivers/gpio/Kconfig | 18 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-adnp.c | 615 +
4 files changed, 672 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-adnp.txt
create mode 100644 drivers/gpio/gpio-adnp.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
new file mode 100644
index 000..513a18e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
@@ -0,0 +1,38 @@
+Avionic Design N-bit GPIO expander bindings
+
+Required properties:
+- compatible: should be "ad,gpio-adnp"
+- reg: The I2C slave address for this device.
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: Interrupt specifier for the controllers interrupt.
+- #gpio-cells: Should be 2. The first cell is the GPIO number and the
+ second cell is used to specify optional parameters:
+ - bit 0: polarity (0: normal, 1: inverted)
+- gpio-controller: Marks the device as a GPIO controller
+- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
+ whereas the second cell is used to specify flags:
+bits[3:0] trigger type and level flags
+ 1 = low-to-high edge triggered
+ 2 = high-to-low edge triggered
+ 4 = active high level-sensitive
+ 8 = active low level-sensitive
+- interrupt-controller: Marks the device as an interrupt controller.
+- nr-gpios: The number of pins supported by the controller.
+
+Example:
+
+ gpioext: gpio-adnp@41 {
+ compatible = "ad,gpio-adnp";
+ reg = <0x41>;
+
+ interrupt-parent = <>;
+ interrupts = <160 1>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ nr-gpios = <64>;
+ };
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 502b5ea..d1b0f7d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
Say yes here to enable the adp5588 to be used as an interrupt
controller. It requires the driver to be built in the kernel.
+config GPIO_ADNP
+ tristate "Avionic Design N-bit GPIO expander"
+ depends on I2C && OF
+ help
+ This option enables support for N GPIOs found on Avionic Design
+ I2C GPIO expanders. The register space will be extended by powers
+ of two, so the controller will need to accomodate for that. For
+ example: if a controller provides 48 pins, 6 registers will be
+ enough to represent all pins, but the driver will assume a
+ register layout for 64 pins (8 registers).
+
+config GPIO_ADNP_IRQ
+ tristate "Interrupt controller support"
+ depends on GPIO_ADNP
+ help
+ Say yes here to enable the Avionic Design N-bit GPIO expander to
+ be used as an interrupt controller.
+
comment "PCI GPIO expanders:"
config GPIO_CS5535
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d370481..73affce 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIO_GENERIC)+= gpio-generic.o
obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
+obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
new file mode 100644
index 000..c122ff4
--- /dev/null
+++ b/drivers/gpio/gpio-adnp.c
@@ -0,0 +1,615 @@
+/*
+ * Copyright (C) 2011-2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
+#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
+#define GPIO_IER(gpio) (0x02 <<
[PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
This commit adds a driver for the Avionic Design N-bit GPIO expander.
The expander provides a variable number of GPIO pins with interrupt
support.
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Rob Herring rob.herr...@calxeda.com
Cc: devicetree-disc...@lists.ozlabs.org
Cc: Linus Walleij linus.wall...@stericsson.com
Signed-off-by: Thierry Reding thierry.red...@avionic-design.de
---
Changes in v2:
- allow building the driver as a module
- assign of_node unconditionally
- use linear mapping IRQ domain
- properly cleanup IRQ domain
- add OF device table and annotate device tables
- emulate rising and falling edge triggers
- increase #gpio-cells to 2
- drop support for !OF
- use IS_ENABLED to conditionalize DEBUG_FS code
---
.../devicetree/bindings/gpio/gpio-adnp.txt | 38 ++
drivers/gpio/Kconfig | 18 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-adnp.c | 615 +
4 files changed, 672 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-adnp.txt
create mode 100644 drivers/gpio/gpio-adnp.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
new file mode 100644
index 000..513a18e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
@@ -0,0 +1,38 @@
+Avionic Design N-bit GPIO expander bindings
+
+Required properties:
+- compatible: should be ad,gpio-adnp
+- reg: The I2C slave address for this device.
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: Interrupt specifier for the controllers interrupt.
+- #gpio-cells: Should be 2. The first cell is the GPIO number and the
+ second cell is used to specify optional parameters:
+ - bit 0: polarity (0: normal, 1: inverted)
+- gpio-controller: Marks the device as a GPIO controller
+- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
+ whereas the second cell is used to specify flags:
+bits[3:0] trigger type and level flags
+ 1 = low-to-high edge triggered
+ 2 = high-to-low edge triggered
+ 4 = active high level-sensitive
+ 8 = active low level-sensitive
+- interrupt-controller: Marks the device as an interrupt controller.
+- nr-gpios: The number of pins supported by the controller.
+
+Example:
+
+ gpioext: gpio-adnp@41 {
+ compatible = ad,gpio-adnp;
+ reg = 0x41;
+
+ interrupt-parent = gpio;
+ interrupts = 160 1;
+
+ gpio-controller;
+ #gpio-cells = 2;
+
+ interrupt-controller;
+ #interrupt-cells = 2;
+
+ nr-gpios = 64;
+ };
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 502b5ea..d1b0f7d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
Say yes here to enable the adp5588 to be used as an interrupt
controller. It requires the driver to be built in the kernel.
+config GPIO_ADNP
+ tristate Avionic Design N-bit GPIO expander
+ depends on I2C OF
+ help
+ This option enables support for N GPIOs found on Avionic Design
+ I2C GPIO expanders. The register space will be extended by powers
+ of two, so the controller will need to accomodate for that. For
+ example: if a controller provides 48 pins, 6 registers will be
+ enough to represent all pins, but the driver will assume a
+ register layout for 64 pins (8 registers).
+
+config GPIO_ADNP_IRQ
+ tristate Interrupt controller support
+ depends on GPIO_ADNP
+ help
+ Say yes here to enable the Avionic Design N-bit GPIO expander to
+ be used as an interrupt controller.
+
comment PCI GPIO expanders:
config GPIO_CS5535
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d370481..73affce 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIO_GENERIC)+= gpio-generic.o
obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
+obj-$(CONFIG_GPIO_ADNP)+= gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
new file mode 100644
index 000..c122ff4
--- /dev/null
+++ b/drivers/gpio/gpio-adnp.c
@@ -0,0 +1,615 @@
+/*
+ * Copyright (C) 2011-2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/gpio.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/irqdomain.h
+#include linux/module.h
+#include
