Re: Adding an interrupt locator to the GPIO, I2C and SPI buses

[Marc Balmer <marc%msys.ch@localhost>]

Am 18.04.13 07:59, schrieb Marc Balmer:
> Khorben,
> 
>> in the process of writing more drivers for the Nokia N900 (OMAP3) I
>> realize that quite a few devices and sensors interrupt on GPIO pins. I
>> finally understood recently that the GPIO pins are actually usable
>> directly as interrupt identifiers, as referenced in their respective
>> "intrbase" locators. An example to illustrate this:
>>
>> omapgpio0 at obio1 addr 0x48310000 size 0x0400 intrbase 96 intr 29
>>
>> Here, a bunch of pins belonging to [what really is] gpio0 interrupt
>> internally as 29, but in turn each pin is individually usable as an
>> interrupt identifier as 96 + the pin offset:
>> - pin 1 is mapped as interrupt 96
>> - pin 2 is at 97
>> - ...and so on for as many pins as found on gpio0 (here 32)
>>
>> For the devices or sensors found on either of the GPIO, I2C or SPI buses
>> and interrupting on a GPIO pin, I think it would totally make sense to
>> be able to specify the proper pin directly within the kernel
>> configuration file. In my particular case, the configuration file would
>> then look like this:
>>
>> n900cambtn0 at gpio2 offset 4 mask 0x3 intr 164 #165 too actually
>> n900acad0 at gpio0 offset 7 mask 0x1 intr 103
>> n900kbdslide0 at gpio2 offset 7 mask 0x1 intr 167
>> n900lckbtn0 at gpio3 offset 17 mask 0x1 intr 209
>> stmemsis0 at iic2 addr 0x1d intr 277
>> tsc2005ts0 at spi0 slave 0 intr 196
>> wilink0 at spi3 slave 0 intr 138
>>
>> This makes sense on the GPIO bus too, because a given device may use
>> multiple pins (eg as input/output) while having a separate one for
>> interrupts.
> 
> Not all interrupt capable GPIO devices work like the ones you find on
> the N900.  Basically, you are a adressing the issue at the wrong level.
> 
> Here is how this should really work:
> 
> The low-level gpio driver, let's call it n900gpio, should report to the
> gpio(4) framework which of it's pins are interrupt capable (that needs a
> new pin capability).  And when a a driver like n900cambtn(4) attaches at
> a certain GPIO pin, it checks whether this pin is interrupt capable,
> bails out if it is not, enables the interrupt otherwise.  This needs an
> extension of the current GPIO framework, something that is needed
> anyways (see the BUGS section of gpio(4)...).
> 
> The kernel configuration could then look like this:
> 
> n900gpio0 at whateverbus0
> gpio0 at n900gpio0
> n900cambtn0 at gpio0 offset 4 mask 3
> 

Actually a driver n900cambtn0(4) that attaches at a gpio(4) pin should
not exist at all, it is wrong by design:

Drivers that attach at gpio(4) pins are hardware agnostic, they access
the underlying hardware through gpio(4), which forms a layer of
abstraction.  A driver that attaches to a gpio(4) pin can work on any
gpio(4) pin that provides the right capabilities.  Such drivers merely
implement kind of concept, like "pulse-width modulate a pin, turn on/off
a led" etc.  So instead of n900cambtn(4), there should be a generic
gpiocambtn(4) driver that can operate on any gpio(4) device.

This is just one more reason not to add a interrupt locator at the
gpio(4) level, it is wrong by design.

The main problem, however, is of course that gpio(4) has no interrupt
capabilities, yet.  Being the maintainer of gpio(4) I feel a bit guilty
of this, so I will start to change it ;)  You can then implement your
powerbutton, camerabuttons, kbd slider drivers and what not on top of
that in a proper way (and gain reusability at the same time).  I am sure
stuff like lock buttons can be found on other machines as well.

>> I am attaching a patch that implements this extra locator for the GPIO,
>> I2C and SPI buses. I have tested it successfully on GPIO and I2C; I
>> would like to have positive feedback before I commit it.
> 
> At least in the GPIO case the diff is not ok, please do not commit it.
> I can offer to work on general interrupting capabilities of the GPIO
> framework, I even have hardware I can test that with.  We could work
> together towards a solution that is generally usable and that avoids
> such hacks.
> 
> Also your patch completely ignores the gpioctl(8) command, which is used
> among other things to attach drivers to individual GPIO pins at runtime.
>  If any new locator would be added to gpio(4), gpioctl(8) would have to
> reflect that.
> 
>>
>> The second patch illustrates how it lets me get rid of the locators I
>> had to hard-code for the first two N900 drivers that I have committed so
>> far.
>>
>> HTH,
>>
>