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

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

> [trying to re-order quoting for clarity]
> 
> On 18/04/2013 07:59, Marc Balmer wrote:
>>
>> [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.
> 
> I wasn't aware of this command and it looks like I should use it indeed.
> Right now it is impractical for me though, because of the lack of
> keyboard and network drivers on this hardware.

Besides gpio(4) and gpioctl(8), there is also a gpio.conf(5)
configuration file so that you can wire-up your GPIO stuff during system
boot time (and it can even be locked-down, so that only specific GPIO
pins are accessible from userland when the system is running).

The config file is there so that you do not need to bake a custom kernel
for your device in most cases.

> 
> Does it involve compiling the drivers as modules?

All gpio(4) drivers can be build as modules, of course.

> 
>> 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.
> 
> I totally agree here; however, just like you mentioned:
> 
>> 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.
> 
> Let's take the lock button as a first example: on the N900 it is
> implemented as a press button. It just reports "toggle the locking state
> now". On other hardware it might be a switch instead, which would report
> either "lock now" or "unlock now"; and even then, GPIO_PIN_HIGH may mean
> one or the other. How can we address this in a generic manner?

The drivers have to be flexible enough to cover the situations we know
about.  That starts with interrupts being triggered differently:  edge
base, falling edge, raising edge, etc.  And then the drivers attaching
to GPIO pins, like these buttons, must be configurable enough.  Nothing
is wrong with adding specific locators to these, just they need a way to
be controllable from userland.

> 
> (that's precisely why I put everything under sys/arch/evbarm/n900 for now)
> 
> Likewise, here the camera button is quite specific, and really has two
> buttons in one: gently pressing focuses, while pressing all the way
> through captures. I am considering reporting these as keyboard events
> rather than through sysmon_pswitch(9) hotkeys eventually.
> 
> The volume button will also require some sort of sysctl(9) interface to
> make it context aware:
> - playback volume up/down,
> - call volume up/down,
> - zoom in/out,
> - scroll up/down...
> This could certainly be done like you did for the keylock support for
> instance.
> 
> Still, I wonder to which extent all of this can be implemented in a
> completely generic manner - although it would be much nicer indeed.

Of course it is not always immediately obvious how to address such a
situation, but at the very least we should try to be generic, maybe that
means inventing new mechanisms.

> 
>> 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)...).
> 
> Understood. In that case I guess I would no longer use intr_establish(),
> but rather something like gpio_pin_ctl() with an interrupt type and a
> callback - in any case, let me know how I can help on this.

Yes, you basically tell the gpio(4) framework that you want to be
"called" when a certain interrupt happens.  It is then up to the
underlying driver to program the hardware and establish the interrupt
handler.  On which condition the interrupt should occur (falling/raising
edge etc.) would be programmed from userspace, like the direction of a
pin (IN, OUT, etc).

> 
>> 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.
> 
> Would it be fine if in the meantime, I keep pushing the drivers that I
> need in sys/arch/evbarm/n900, so that it will simply be a matter of
> converting and moving them to sys/dev/gpio once gpio(4) is extended?

I can not forbid you to committ your stuff under evbarm/n900 ;) I do
think, however, that it is a bad idea to do so right now.  We are not in
a rush to get this in and I think we just found a path to work towards a
generic solution which maybe in place in a few weeks of time.

But of course, this is up to you, you may have your reasons why you want
the drivers to be in the tree.

> 
> I can live with the crude hard-coded locator for the interrupt pin in
> the meantime.

I suggest to make it a #define with a short comment, instead of putting
a magic number directly in the source code.