Re: gpio interrupts

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Thu, Apr 07, 2016 at 05:34:00PM -0300, Jared McNeill wrote:
> Hi Manuel --
> 
> A few comments:
> 
> >The attached patch adds two functions to gpio.c:
> >gpio_pin_wait() is called by a thread that wants to wait on an interrupt.
> >A thread can wait only on a single pin.
> 
> Any particular reason you went with this approach instead of say, something
> that follows the typical foo_intr_establish pattern? I think "register a
> callback" would be a much more typical use-case for a device driver here.

In a SMP world you want to do less in interrupt handlers and more in
kernel threads, basically. Also, in this kind of setup, the
device raising an interrupt is most likely behind a bus that itself needs
interrupts to work (i2c, spi, uart, ...). So the action triggered by
the interrupt can't be part of the interrupt handler, it has to execute
in a thread.
Also, if we ever makes this usable up to userland, condvar will be needed.

I can think of one kind of setup that would need to run from interrupt
handler: if this pin is connected to a PPS signal.

I could add a callback scheme if there is a need for that, but this
raises a question: how to handle IPL properly ?
AFAIK the only MI interface is splfoo()/splx(), splraise/spllower are
not mandatory.

> 
> >+kmutex_t awin_gpio_intr_lock;
> >+uint32_t awin_gpio_ecfg[3];
> >+uint32_t awin_gpio_eintr_configured;
> >+bus_space_handle_t awin_gpio_eint_bsh;
> 
> These need to be in the softc. There is one port controller on sun4i/sun7i,
> but sun6i/sun8i/sun9i have two (PIO vs R_PIO).

OK, I'll change that.

> 
> >@@ -345,13 +362,23 @@ awin_gpio_config_pins(device_t self)
> 
> Instead of having the pin group / index / SoC defines hidden in this
> function, it's better to put the pin capabilities in the awin_gpio_pin_group
> structure. Having SoC-specific code mixed through the driver is going to
> make this really difficult to maintain as you add support for more SoCs. The
> port controllers on various Allwinner SoCs are basically the same, only with
> different pin counts / function assignments.

OK, I'll change that too. I agree it's better, but I don't want to change
too much code here before the MI part stabilizes.


> 
> >+	sc->sc_ih = intr_establish(loc->loc_intr, IPL_VM, IST_LEVEL,
> >+	    awin_gpio_intr, &awin_gpio_sc);
> 
> Not MPSAFE?

Shoukd be. I just didn't remember about it.

> 
> >+awin_gpio_set_pin_eint(int pin, uint32_t m)
> >...
> >+	KASSERT(g <= 3);
> >+	KASSERT(s <= 28);
> 
> What are these magic numbers?

this is what you get for the A20. This needs to be cleaned up.

> 
> >+gpio_intr(device_t self, uint32_t evts)
> >...
> >+	for (int i = 0; i < sc->sc_npins; i++) {
> >+		if (evts & (1 << i)) {
> 
> You don't need to test every bit here here, only enabled ones, i.e.:
> 
>   while ((bit = ffs(evts)) != 0) {
>           mask = (1U << (bit - 1));
>           evts &= ~mask;
>           // do stuff
>   }

thanks, indeed it's better.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--