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
> 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.
> >> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> >> > + if (!gpio->irq_mask)
> >> > + return -ENOMEM;
> >> > +
> >> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> >> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
> >> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> >> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> >> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
> >> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
> >>
> >> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you
> >> doing
> >> here? Explain with some comment atleast.
> >
> > Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
> > irq_low to keep track of the current level and trigger modes for each
> > interrupt. Instead of allocating small chunks for each of these I
> > allocate one chunk and just make the others point into that.
>
> Maybe you said this would go away in this case not comments
> of course.
irq_mask and irq_mask_cur can go away I think.
> But I wasn't getting the multiplication part. I understand the
> allocation, 7 registers time the number of registers (hm, there
> is something about the naming too....)
You're right, regs should probably be called num_regs.
> You're storing the things in such an awkward way: all
> current masks for all registers sets, then all levels for
> all register etc.
>
> Instead could you store all the flags for *one* instance
> then the next set of registers etc.
I was following the register layout of the controller to keep things
consistent.
Thierry
pgpooaBaHKGUC.pgp
Description: PGP signature
