Goodix and the PIC methods that didn't exist

The PinePhone Pro’s touchscreen is a Goodix GT917S — a 10-finger multitouch I2C controller hanging off DTS &i2c3 at 7-bit address 0x14, with reset on GPIO3_B4 and interrupt on GPIO3_B5. FreeBSD dmesg prints that as shifted addr 0x28 on iicbus1. Linux drives it from drivers/input/touchscreen/goodix.c. We wrote a minimal port: src/sys/dev/iicbus/goodix.c, ~500 lines, evdev MT Protocol B, the standard 8-byte-per-contact data layout, the standard 0x814E status register that the driver clears after each read.

The driver attached cleanly the day it was written ( 2464a0a goodix: add Goodix GT917S multitouch touchscreen driver ). Touch events even fired — but only in the polling fallback path. The interrupt-driven path returned ENXIO from bus_setup_intr and silently routed itself to polling at 100 Hz. That’s a working stopgap, but polling at 100 Hz costs roughly 5–15 mW (i2c bus active, IRQ-thread wakeups, evdev syncs) and adds up to 10 ms of latency per touch event. We wanted real interrupts. Getting there took seven commits across four days and ended at the bottom of the GPIO driver, not the touchscreen driver.

static void
rk_pic_enable_intr(device_t dev, struct intr_irqsrc *isrc)
{
    struct rk_gpio_softc *sc = device_get_softc(dev);
    struct rk_pin_irqsrc *irqsrc = (struct rk_pin_irqsrc *)isrc;

    RK_GPIO_LOCK(sc);
    rk_gpio_write_bit(sc, RK_GPIO_INTMASK, irqsrc->irq, 0);
    rk_gpio_write_bit(sc, RK_GPIO_INTEN,   irqsrc->irq, 1);
    RK_GPIO_UNLOCK(sc);
}

static void
rk_pic_disable_intr(device_t dev, struct intr_irqsrc *isrc)
{
    struct rk_gpio_softc *sc = device_get_softc(dev);
    struct rk_pin_irqsrc *irqsrc = (struct rk_pin_irqsrc *)isrc;

    RK_GPIO_LOCK(sc);
    rk_gpio_write_bit(sc, RK_GPIO_INTEN, irqsrc->irq, 0);
    RK_GPIO_UNLOCK(sc);
}

static void
rk_pic_pre_ithread(device_t dev, struct intr_irqsrc *isrc)
{
    struct rk_gpio_softc *sc = device_get_softc(dev);
    struct rk_pin_irqsrc *irqsrc = (struct rk_pin_irqsrc *)isrc;

    RK_GPIO_LOCK(sc);
    rk_gpio_write_bit(sc, RK_GPIO_INTMASK, irqsrc->irq, 1);
    RK_GPIO_UNLOCK(sc);
}

static void
rk_pic_post_ithread(device_t dev, struct intr_irqsrc *isrc)
{
    rk_pic_enable_intr(dev, isrc);
}

static void
rk_pic_post_filter(device_t dev, struct intr_irqsrc *isrc)
{
    struct rk_gpio_softc *sc = device_get_softc(dev);
    struct rk_pin_irqsrc *irqsrc = (struct rk_pin_irqsrc *)isrc;

    RK_GPIO_LOCK(sc);
    rk_gpio_write_bit(sc, RK_GPIO_PORTA_EOI, irqsrc->irq, 1);
    RK_GPIO_UNLOCK(sc);
}

There were two more bugs around the same fix. The polling-mode codepath in goodix_poll was originally calling goodix_intr directly from a callout — which runs in softclock context, where you can’t sleep. Goodix talks to the chip over I2C, and rk_i2c_transfer sleeps on the bus. The phone panicked on the first poll cycle in 4ac5212 goodix: defer I2C work from callout to taskqueue (fix sleep-in-softclock panic) : “sleeping in softclock context.” Fixed by deferring to a taskqueue:

static void
goodix_poll_task(void *arg, int pending)
{
    struct goodix_softc *sc = arg;
    goodix_intr(sc);
}

static void
goodix_poll(void *arg)
{
    struct goodix_softc *sc = arg;
    taskqueue_enqueue(taskqueue_thread, &sc->sc_poll_task);
}

The same constraint applies to the IRQ handler when we’re driving from the rk_gpio PIC: goodix_intr can sleep, and the IRQ ithread is the right context for that, but only after pre_ithread has masked the edge. Which is one more reason pic_pre_ithread exists — it keeps the edge from re-firing while the I2C transaction is in flight, otherwise the Goodix’s STATUS register would be cleared (by us at the end of the handler) and re-asserted (by a finger-move during the i2c read) and we’d lose touch events.

The Goodix driver itself is small and boring; the interesting code lives one level down, in the GPIO PIC that any future GPIO-interrupt consumer (the FUSB302 USB-C controller, the headset detect pin, the modem RI line, the BT HOST_WAKE pin from essay 10) will rely on. With the five missing methods in place, all of those are now implementable as proper IRQ-driven drivers instead of callout-based pollers. The Goodix drop from polling to IRQ has shaved ~10 mW off idle and made finger-tracking visibly smoother under sway.

Touch is at ● working : 10-finger multitouch in X11 and Wayland, IRQ-driven, no missed events under sustained input. Goodix deep-sleep modes for power management are still untouched — that’s a power-policy refinement, not a functional gap.

Postscript: the same bug, one layer up

The PIC fix above made gpio_alloc_intr_resource work. That was sufficient for two weeks. Then the rt5640 audio bringup forced a return to first principles on iicbus, and goodix’s interrupt path stopped working again — for an entirely different reason that had been camouflaged the whole time.

goodix0: IRQ debug: parent chain dev=goodix0 parent=iicbus0 pp=rk_i2c0 ppp=simplebus0
goodix0: IRQ debug: direct BUS_ALLOC_RESOURCE(rk_i2c0, ...) -> 0
goodix0: IRQ debug: direct BUS_ALLOC_RESOURCE(simplebus0, ...) -> 0xffff…

DEVMETHOD(bus_setup_intr,        bus_generic_setup_intr),
DEVMETHOD(bus_teardown_intr,     bus_generic_teardown_intr),
DEVMETHOD(bus_alloc_resource,    bus_generic_alloc_resource),
DEVMETHOD(bus_release_resource,  bus_generic_release_resource),
DEVMETHOD(bus_activate_resource, bus_generic_activate_resource),
DEVMETHOD(bus_deactivate_resource, bus_generic_deactivate_resource),
DEVMETHOD(bus_adjust_resource,   bus_generic_adjust_resource),