Re: gpio interrupts

[Jared McNeill <jmcneill%invisible.ca@localhost>]

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.

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

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

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

Not MPSAFE?

> +awin_gpio_set_pin_eint(int pin, uint32_t m)
> ...
> +	KASSERT(g <= 3);
> +	KASSERT(s <= 28);

What are these magic numbers?

> +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
  }


Cheers,
Jared