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

[Pierre Pronchery <khorben%defora.org@localhost>]

                        Hi,

[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.

Does it involve compiling the drivers as modules?

> 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?

(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.

> 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.

> 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 live with the crude hard-coded locator for the interrupt pin in
the meantime.

Thank you,
-- 
khorben