From: Bryan Steele Subject: Re: aplhidev: enable on amd64, add keyboard backlight support To: tech@openbsd.org Date: Thu, 13 Nov 2025 22:09:09 -0500 On Thu, Nov 13, 2025 at 08:15:16PM -0600, joshua stein wrote: > This moves aplhidev out of arch/arm64 and into dev/spi, adds > attachment via ACPI through ispi, enables it on amd64, and adds > support for adjusting keyboard backlight through 'wsconsctl > keyboard.backlight'. > > Tested on a 2017 MacBook 12". Hopefully it didn't break anything on > Apple M* arm64 laptops and I'm curious if the keyboard backlight > support also works there. I haven't tested your diff, but at least on Apple M1 there is are seperate pwm(4)/pwmled(4) drivers that handle the keyboard backlight which this may conflict with. ./sys/dev/fdt/pwmleds.c > > Individual commits are here but this is just one consolidated diff: > > https://github.com/jcs/openbsd-src/commit/5cc8811f104532139877139b6852ae627963a602 > https://github.com/jcs/openbsd-src/commit/3a57a3b8440efc45d4e75a36e843328f8e5ff015 > https://github.com/jcs/openbsd-src/commit/852de258b6625c33641bdf36303699ba157f3d55 > > I also tested it in RAMDISK_CD so the keyboard works during > installation but that's not included here. > > > diff --git sys/arch/amd64/conf/GENERIC sys/arch/amd64/conf/GENERIC > index 37f1ac6d7c0..4046a147d47 100644 > --- sys/arch/amd64/conf/GENERIC > +++ sys/arch/amd64/conf/GENERIC > @@ -208,6 +208,11 @@ gpio* at skgpio? > > ispi* at acpi? # Intel LPSS SPI controller > ispi* at pci? > +aplhidev* at spi? # Apple SPI HID > +aplkbd* at aplhidev? > +wskbd* at aplkbd? mux 1 > +aplms* at aplhidev? > +wsmouse* at aplms? mux 0 > > #option PCMCIAVERBOSE > > diff --git sys/arch/amd64/conf/files.amd64 sys/arch/amd64/conf/files.amd64 > index 0c1e224f5c2..25476378a97 100644 > --- sys/arch/amd64/conf/files.amd64 > +++ sys/arch/amd64/conf/files.amd64 > @@ -280,6 +280,17 @@ include "dev/onewire/files.onewire" > # > attach ipmi at mainbus > > +# > +# SPI > +# > +device aplhidev {} > +attach aplhidev at spi > +device aplkbd: hid, hidkbd, wskbddev > +attach aplkbd at aplhidev > +device aplms: hid, hidms, wsmousedev > +attach aplms at aplhidev > +file dev/spi/aplhidev.c aplhidev | aplkbd | aplms needs-flag > + > # > # device major numbers > # > diff --git sys/arch/arm64/conf/files.arm64 sys/arch/arm64/conf/files.arm64 > index 34d9f5d35f5..5cf73280050 100644 > --- sys/arch/arm64/conf/files.arm64 > +++ sys/arch/arm64/conf/files.arm64 > @@ -186,7 +186,7 @@ device aplkbd: hid, hidkbd, wskbddev > attach aplkbd at aplhidev > device aplms: hid, hidms, wsmousedev > attach aplms at aplhidev > -file arch/arm64/dev/aplhidev.c aplhidev | aplkbd | aplms needs-flag > +file dev/spi/aplhidev.c aplhidev | aplkbd | aplms needs-flag > > device aplmbox > attach aplmbox at fdt > diff --git sys/dev/acpi/ispi_acpi.c sys/dev/acpi/ispi_acpi.c > index 6a226a30482..8e0b820be58 100644 > --- sys/dev/acpi/ispi_acpi.c > +++ sys/dev/acpi/ispi_acpi.c > @@ -92,6 +92,7 @@ int > ispi_acpi_found_hid(struct aml_node *node, void *arg) > { > struct ispi_softc *sc = (struct ispi_softc *)arg; > + struct spi_attach_args sa; > int64_t sta; > char cdev[32], dev[32]; > > @@ -110,7 +111,17 @@ ispi_acpi_found_hid(struct aml_node *node, void *arg) > > acpi_attach_deps(acpi_softc, node->parent); > > - /* TODO */ > + if (strcmp(cdev, "apple-spi-topcase") == 0) { > + memset(&sa, 0, sizeof(sa)); > + sa.sa_tag = &sc->sc_spi_tag; > + sa.sa_name = cdev; > + sa.sa_cookie = node->parent; > + > + if (config_found(&sc->sc_dev, &sa, ispi_spi_print)) { > + node->attached = 1; > + return 1; > + } > + } > > return 0; > } > diff --git sys/arch/arm64/dev/aplhidev.c sys/dev/spi/aplhidev.c > similarity index 74% > rename from sys/arch/arm64/dev/aplhidev.c > rename to sys/dev/spi/aplhidev.c > index 97d00a8878a..f8f0f35d88b 100644 > --- sys/arch/arm64/dev/aplhidev.c > +++ sys/dev/spi/aplhidev.c > @@ -25,13 +25,23 @@ > > #include > > -#include > - > #include > > +#ifdef __HAVE_FDT > +#include > #include > #include > #include > +#endif > + > +#include "acpi.h" > +#if NACPI > 0 > +#include > +#include > +#include > +#include > +#include > +#endif > > #include > > @@ -46,6 +56,14 @@ > > #include "aplhidev.h" > > +/* #define APLHIDEV_DEBUG 1 */ > + > +#ifdef APLHIDEV_DEBUG > +#define DPRINTF(x) printf x > +#else > +#define DPRINTF(x) > +#endif > + > #define APLHIDEV_READ_PACKET 0x20 > #define APLHIDEV_WRITE_PACKET 0x40 > > @@ -63,6 +81,12 @@ > #define APLHIDEV_MODE_HID 0x00 > #define APLHIDEV_MODE_RAW 0x01 > #define APLHIDEV_GET_DIMENSIONS 0xd932 > +#define APLHIDEV_SET_BACKLIGHT 0xb051 > +#define APLHIDEV_BACKLIGHT_MAGIC 0x01b0 > +#define APLHIDEV_BACKLIGHT_MIN 32 > +#define APLHIDEV_BACKLIGHT_MAX 255 > +#define APLHIDEV_BACKLIGHT_ON 0x01f4 > +#define APLHIDEV_BACKLIGHT_OFF 0x0001 > > struct aplhidev_dim { > uint32_t width; > @@ -131,9 +155,19 @@ struct aplhidev_set_mode { > uint16_t crc; > }; > > +struct aplhidev_set_backlight { > + struct aplhidev_msghdr hdr; > + uint16_t magic; > + uint16_t level; > + uint16_t on_off; > + uint16_t crc; > +}; > + > struct aplhidev_softc { > struct device sc_dev; > - int sc_node; > +#if NACPI > 0 > + struct aml_node *sc_dev_node; > +#endif > > spi_tag_t sc_spi_tag; > struct spi_config sc_spi_conf; > @@ -166,6 +200,13 @@ struct aplhidev_softc { > > int aplhidev_match(struct device *, void *, void *); > void aplhidev_attach(struct device *, struct device *, void *); > +#ifdef __HAVE_FDT > +void aplhidev_fdt_init(struct aplhidev_softc *, void *); > +#elif NACPI > 0 > +int aplhidev_acpi_init(struct aplhidev_softc *); > +int aplhidev_enable_spi(struct aplhidev_softc *); > +int aplhidev_gpe_intr(struct aml_node *, int, void *); > +#endif > > const struct cfattach aplhidev_ca = { > sizeof(struct aplhidev_softc), aplhidev_match, aplhidev_attach > @@ -178,9 +219,16 @@ struct cfdriver aplhidev_cd = { > void aplhidev_get_info(struct aplhidev_softc *); > void aplhidev_get_descriptor(struct aplhidev_softc *, uint8_t); > void aplhidev_set_leds(struct aplhidev_softc *, uint8_t); > +void aplhidev_set_backlight(struct aplhidev_softc *, uint8_t); > void aplhidev_set_mode(struct aplhidev_softc *, uint8_t); > void aplhidev_get_dimensions(struct aplhidev_softc *); > > +void aplkbd_set_backlight(void *); > +extern int (*wskbd_get_backlight)(struct wskbd_backlight *); > +extern int (*wskbd_set_backlight)(struct wskbd_backlight *); > +int aplkbd_wskbd_get_backlight(struct wskbd_backlight *); > +int aplkbd_wskbd_set_backlight(struct wskbd_backlight *); > + > int aplhidev_intr(void *); > void aplkbd_intr(struct device *, uint8_t *, size_t); > void aplms_intr(struct device *, uint8_t *, size_t); > @@ -189,10 +237,26 @@ int > aplhidev_match(struct device *parent, void *match, void *aux) > { > struct spi_attach_args *sa = aux; > +#if NACPI > 0 > + struct aml_node *node = (struct aml_node *)sa->sa_cookie; > + uint64_t val; > +#endif > > +#if defined(__HAVE_FDT) > if (strcmp(sa->sa_name, "apple,spi-hid-transport") == 0) > return 1; > > +#elif NACPI > 0 > + if (strcmp(sa->sa_name, "apple-spi-topcase") == 0) { > + /* don't attach if USB interface is present */ > + if (aml_evalinteger(acpi_softc, node, "UIST", 0, NULL, > + &val) == 0 && val) > + return 0; > + > + return 1; > + } > +#endif > + > return 0; > } > > @@ -206,34 +270,15 @@ aplhidev_attach(struct device *parent, struct device *self, void *aux) > int retry; > > sc->sc_spi_tag = sa->sa_tag; > - sc->sc_node = *(int *)sa->sa_cookie; > > - sc->sc_gpiolen = OF_getproplen(sc->sc_node, "spien-gpios"); > - if (sc->sc_gpiolen > 0) { > - sc->sc_gpio = malloc(sc->sc_gpiolen, M_TEMP, M_WAITOK); > - OF_getpropintarray(sc->sc_node, "spien-gpios", > - sc->sc_gpio, sc->sc_gpiolen); > - gpio_controller_config_pin(sc->sc_gpio, GPIO_CONFIG_OUTPUT); > +#ifdef __HAVE_FDT > + aplhidev_fdt_init(sc, sa->sa_cookie); > > - /* Reset */ > - gpio_controller_set_pin(sc->sc_gpio, 1); > - delay(5000); > - gpio_controller_set_pin(sc->sc_gpio, 0); > - delay(5000); > - > - /* Enable. */ > - gpio_controller_set_pin(sc->sc_gpio, 1); > - delay(50000); > - } > - > - sc->sc_spi_conf.sc_bpw = 8; > - sc->sc_spi_conf.sc_freq = OF_getpropint(sc->sc_node, > - "spi-max-frequency", 0); > - sc->sc_spi_conf.sc_cs = OF_getpropint(sc->sc_node, "reg", 0); > - sc->sc_spi_conf.sc_cs_delay = 100; > - > - fdt_intr_establish(sc->sc_node, IPL_TTY, > - aplhidev_intr, sc, sc->sc_dev.dv_xname); > +#elif NACPI > 0 > + sc->sc_dev_node = (struct aml_node *)sa->sa_cookie; > + if (aplhidev_acpi_init(sc) != 0) > + return; > +#endif > > aplhidev_get_info(sc); > for (retry = 10; retry > 0; retry--) { > @@ -303,6 +348,179 @@ aplhidev_attach(struct device *parent, struct device *self, void *aux) > } > } > > +#ifdef __HAVE_FDT > +void > +aplhidev_fdt_init(struct aplhidev_softc *sc, void *cookie) > +{ > + int node = *(int *)cookie; > + > + sc->sc_gpiolen = OF_getproplen(inode, "spien-gpios"); > + if (sc->sc_gpiolen > 0) { > + sc->sc_gpio = malloc(sc->sc_gpiolen, M_TEMP, M_WAITOK); > + OF_getpropintarray(node, "spien-gpios", > + sc->sc_gpio, sc->sc_gpiolen); > + gpio_controller_config_pin(sc->sc_gpio, GPIO_CONFIG_OUTPUT); > + > + /* Reset */ > + gpio_controller_set_pin(sc->sc_gpio, 1); > + delay(5000); > + gpio_controller_set_pin(sc->sc_gpio, 0); > + delay(5000); > + > + /* Enable. */ > + gpio_controller_set_pin(sc->sc_gpio, 1); > + delay(50000); > + } > + > + sc->sc_spi_conf.sc_bpw = 8; > + sc->sc_spi_conf.sc_freq = OF_getpropint(node, "spi-max-frequency", 0); > + sc->sc_spi_conf.sc_cs = OF_getpropint(node, "reg", 0); > + sc->sc_spi_conf.sc_cs_delay = 100; > + > + fdt_intr_establish(sc->sc_node, IPL_TTY, > + aplhidev_intr, sc, sc->sc_dev.dv_xname); > +} > +#endif > + > +#if NACPI > 0 > +int > +aplhidev_acpi_init(struct aplhidev_softc *sc) > +{ > + /* a0b5b7c6-1318-441c-b0c9-fe695eaf949b */ > + static uint8_t guid[] = { > + 0xC6, 0xB7, 0xB5, 0xA0, 0x18, 0x13, 0x1C, 0x44, > + 0xB0, 0xC9, 0xFE, 0x69, 0x5E, 0xAF, 0x94, 0x9B, > + }; > + struct aml_value cmd[4], res; > + struct aml_node *node; > + uint64_t val; > + int i; > + > + /* > + * On newer Apple hardware where we claim an OSI of Darwin, _CRS > + * doesn't return a useful SpiSerialBusV2 object but instead returns > + * parameters from a _DSM method when called with a particular UUID > + * which macOS does. > + */ > + if (!aml_searchname(sc->sc_dev_node, "_DSM")) { > + printf("%s: couldn't find _DSM at %s\n", sc->sc_dev.dv_xname, > + aml_nodename(sc->sc_dev_node)); > + return 1; > + } > + > + bzero(&cmd, sizeof(cmd)); > + cmd[0].type = AML_OBJTYPE_BUFFER; > + cmd[0].v_buffer = (uint8_t *)&guid; > + cmd[0].length = sizeof(guid); > + cmd[1].type = AML_OBJTYPE_INTEGER; > + cmd[1].v_integer = 1; > + cmd[1].length = 1; > + cmd[2].type = AML_OBJTYPE_INTEGER; > + cmd[2].v_integer = 1; > + cmd[2].length = 1; > + cmd[3].type = AML_OBJTYPE_BUFFER; > + cmd[3].length = 0; > + > + if (aml_evalname(acpi_softc, sc->sc_dev_node, "_DSM", 4, cmd, &res)) { > + printf("%s: eval of _DSM at %s failed\n", > + sc->sc_dev.dv_xname, aml_nodename(sc->sc_dev_node)); > + return 1; > + } > + > + if (res.type != AML_OBJTYPE_PACKAGE) { > + printf("%s: bad _DSM result at %s: %d\n", > + sc->sc_dev.dv_xname, aml_nodename(sc->sc_dev_node), > + res.type); > + aml_freevalue(&res); > + return 1; > + } > + > + if (res.length % 2 != 0) { > + printf("%s: _DSM length %d not even\n", sc->sc_dev.dv_xname, > + res.length); > + aml_freevalue(&res); > + return 1; > + } > + > + for (i = 0; i < res.length; i += 2) { > + char *k; > + > + if (res.v_package[i]->type != AML_OBJTYPE_STRING || > + res.v_package[i + 1]->type != AML_OBJTYPE_BUFFER) { > + printf("%s: expected string+buffer, got %d+%d\n", > + sc->sc_dev.dv_xname, res.v_package[i]->type, > + res.v_package[i + 1]->type); > + aml_freevalue(&res); > + return 1; > + } > + > + k = res.v_package[i]->v_string; > + val = aml_val2int(res.v_package[i + 1]); > + > + DPRINTF(("%s: %s = %lld\n", sc->sc_dev.dv_xname, k, val)); > + > + if (strcmp(k, "spiSclkPeriod") == 0) { > + sc->sc_spi_conf.sc_freq = 1000000000 / val; > + } else if (strcmp(k, "spiWordSize") == 0) { > + sc->sc_spi_conf.sc_bpw = val; > + } else if (strcmp(k, "spiSPO") == 0) { > + if (val) > + sc->sc_spi_conf.sc_flags |= SPI_CONFIG_CPOL; > + } else if (strcmp(k, "spiSPH") == 0) { > + if (val) > + sc->sc_spi_conf.sc_flags |= SPI_CONFIG_CPHA; > + } else if (strcmp(k, "spiCSDelay") == 0) { > + sc->sc_spi_conf.sc_cs_delay = val; > + } else { > + DPRINTF(("%s: unused _DSM key %s\n", > + sc->sc_dev.dv_xname, k)); > + } > + } > + aml_freevalue(&res); > + > + if (aplhidev_enable_spi(sc) != 0) > + return 1; > + > + node = aml_searchname(sc->sc_dev_node, "_GPE"); > + if (node) > + aml_register_notify(sc->sc_dev_node, NULL, aplhidev_gpe_intr, > + sc, ACPIDEV_NOPOLL); > + > + return 0; > +} > + > +int > +aplhidev_enable_spi(struct aplhidev_softc *sc) > +{ > + struct aml_value arg; > + uint64_t val; > + > + /* if SPI is not enabled, enable it */ > + if (aml_evalinteger(acpi_softc, sc->sc_dev_node, "SIST", 0, NULL, > + &val) == 0 && !val) { > + DPRINTF(("%s: SIST is %lld\n", sc->sc_dev.dv_xname, val)); > + > + bzero(&arg, sizeof(arg)); > + arg.type = AML_OBJTYPE_INTEGER; > + arg.v_integer = 1; > + arg.length = 1; > + > + if (aml_evalname(acpi_softc, sc->sc_dev_node, "SIEN", 1, &arg, > + NULL) != 0) { > + DPRINTF(("%s: couldn't enable SPI mode\n", __func__)); > + return 1; > + } > + > + DELAY(500); > + } else { > + DPRINTF(("%s: SIST is already %lld\n", sc->sc_dev.dv_xname, > + val)); > + } > + > + return 0; > +} > +#endif > + > void > aplhidev_get_info(struct aplhidev_softc *sc) > { > @@ -409,6 +627,45 @@ aplhidev_set_leds(struct aplhidev_softc *sc, uint8_t leds) > spi_release_bus(sc->sc_spi_tag, 0); > } > > +void > +aplhidev_set_backlight(struct aplhidev_softc *sc, uint8_t level) > +{ > + struct aplhidev_spi_packet packet; > + struct aplhidev_set_backlight *msg; > + struct aplhidev_spi_status status; > + > + memset(&packet, 0, sizeof(packet)); > + packet.flags = APLHIDEV_WRITE_PACKET; > + packet.device = APLHIDEV_KBD_DEVICE; > + packet.len = sizeof(*msg); > + > + msg = (void *)&packet.data[0]; > + msg->hdr.type = APLHIDEV_SET_BACKLIGHT; > + msg->hdr.device = APLHIDEV_KBD_DEVICE; > + msg->hdr.msgid = sc->sc_msgid++; > + msg->hdr.cmdlen = sizeof(*msg) - sizeof(struct aplhidev_msghdr) - 2; > + msg->hdr.rsplen = msg->hdr.cmdlen; > + msg->magic = htole16(APLHIDEV_BACKLIGHT_MAGIC); > + if (level <= APLHIDEV_BACKLIGHT_MIN) { > + msg->level = 0; > + msg->on_off = htole16(APLHIDEV_BACKLIGHT_OFF); > + } else { > + msg->level = htole16(level); > + msg->on_off = htole16(APLHIDEV_BACKLIGHT_ON); > + } > + msg->crc = crc16(0, (void *)msg, sizeof(*msg) - 2); > + > + packet.crc = crc16(0, (void *)&packet, sizeof(packet) - 2); > + > + spi_acquire_bus(sc->sc_spi_tag, 0); > + spi_config(sc->sc_spi_tag, &sc->sc_spi_conf); > + spi_transfer(sc->sc_spi_tag, (char *)&packet, NULL, sizeof(packet), > + SPI_KEEP_CS); > + delay(100); > + spi_read(sc->sc_spi_tag, (char *)&status, sizeof(status)); > + spi_release_bus(sc->sc_spi_tag, 0); > +} > + > void > aplhidev_set_mode(struct aplhidev_softc *sc, uint8_t mode) > { > @@ -477,6 +734,16 @@ aplhidev_get_dimensions(struct aplhidev_softc *sc) > delay(1000); > } > > +#if NACPI > 0 > +int > +aplhidev_gpe_intr(struct aml_node *node, int gpe, void *arg) > +{ > + struct aplhidev_softc *sc = arg; > + > + return aplhidev_intr(sc); > +} > +#endif > + > int > aplhidev_intr(void *arg) > { > @@ -566,6 +833,7 @@ struct aplkbd_softc { > struct aplhidev_softc *sc_hidev; > struct hidkbd sc_kbd; > int sc_spl; > + int sc_backlight; > }; > > void aplkbd_cngetc(void *, u_int *, int *); > @@ -641,7 +909,12 @@ aplkbd_attach(struct device *parent, struct device *self, void *aux) > aplkbd_enable(sc, 1); > } > > + > hidkbd_attach_wskbd(kbd, KB_US | KB_DEFAULT, &aplkbd_accessops); > + > + sc->sc_backlight = APLHIDEV_BACKLIGHT_MIN; > + wskbd_get_backlight = aplkbd_wskbd_get_backlight; > + wskbd_set_backlight = aplkbd_wskbd_set_backlight; > } > > void > @@ -693,6 +966,43 @@ aplkbd_set_leds(void *v, int leds) > aplhidev_set_leds(sc->sc_hidev, res); > } > > +int > +aplkbd_wskbd_get_backlight(struct wskbd_backlight *kbl) > +{ > + struct aplkbd_softc *sc = aplkbd_cd.cd_devs[0]; > + > + if (sc == NULL) > + return 0; > + > + kbl->min = APLHIDEV_BACKLIGHT_MIN; > + kbl->max = APLHIDEV_BACKLIGHT_MAX; > + kbl->curval = sc->sc_backlight; > + > + return 0; > +} > + > +int > +aplkbd_wskbd_set_backlight(struct wskbd_backlight *kbl) > +{ > + struct aplkbd_softc *sc = aplkbd_cd.cd_devs[0]; > + int val; > + > + if (sc == NULL) > + return -1; > + > + val = kbl->curval; > + if (val < APLHIDEV_BACKLIGHT_MIN) > + val = APLHIDEV_BACKLIGHT_MIN; > + if (val > APLHIDEV_BACKLIGHT_MAX) > + val = APLHIDEV_BACKLIGHT_MAX; > + > + sc->sc_backlight = val; > + aplhidev_set_backlight((struct aplhidev_softc *)sc->sc_dev.dv_parent, > + sc->sc_backlight); > + > + return 0; > +} > + > /* Console interface. */ > void > aplkbd_cngetc(void *v, u_int *type, int *data) > >