From: Mark Kettenis Subject: Re: samsabi(4): Keyboard backlight support for the SGB4E To: Marcus Glocker Cc: tech@openbsd.org Date: Mon, 25 May 2026 22:58:10 +0200 > Date: Mon, 25 May 2026 22:07:02 +0200 > From: Marcus Glocker > > On Mon, May 25, 2026 at 09:39:09PM +0200, Mark Kettenis wrote: > > > > Date: Mon, 25 May 2026 21:10:36 +0200 > > > From: Marcus Glocker > > > Content-Type: text/plain; charset=us-ascii > > > Content-Disposition: inline > > > > > > This diff adds initial keyboard backlight support for the Samsung Galaxy > > > Book4 Edge. From the man page: > > > > > > "The samsabi driver provides support for the Samsung Advanced BIOS > > > Interface (SABI), a vendor command interface of the embedded controller > > > found in Samsung laptops based on the Qualcomm Snapdragon X1E80100, such > > > as the Galaxy Book4 Edge. The controller, identified as "SAM060B" in > > > ACPI, is an ENE KB9058 micro controller running Samsung firmware. It > > > exposes the SABI command protocol on the I2C bus at a separate address > > > from the "Mbox" channel used by sambat(4) for battery status." > > > > > > Further devices can be controlled through this interface in the future, > > > for example fan control. > > > > > > Ok? > > > > A few remarks: > > > > * You "invented" the devicetree bindings for this thing isn't it? > > That's fine, but if this ever ends up as an "official" binding and > > changes get made, we should follow. Not a big issue since this > > isn't essential hardware. > > That's right. And yes, fully agree, if this DTS should ever land in > Linux, we would need to make the adaptions in our drivers. But I > think that should be feasible. > > > * The locking looks very suspicious. Why do you think you need this > > lock? As far as I can tell, all this code runs with the kernel > > lock. > > I was not sure about the kernel lock, but you're right. I removed > the lock. > > > * Why do you use I2C_F_POLL? As far as I can tell all this code runs > > in process context, so there is no reason we cannot sleep. > > You're also right. I moved I2C_F_POLL to 0. > > Attached the updated diff. ok kettenis@ > Index: sys/dev/i2c/samsabi.c > =================================================================== > RCS file: sys/dev/i2c/samsabi.c > diff -N sys/dev/i2c/samsabi.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ sys/dev/i2c/samsabi.c 25 May 2026 20:03:18 -0000 > @@ -0,0 +1,281 @@ > +/* $OpenBSD$ */ > + > +/* > + * Copyright (c) 2026 Marcus Glocker > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > +/* > + * A driver to control the Samsung Advanced BIOS Interface (SABI), a Samsung > + * proprietary firmware/EC command interface on address 0x62. > + * > + * For now the driver supports: > + * > + * - Keyboard backlight control > + */ > + > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +/* Debugging */ > +/* #define SAMSABI_DEBUG */ > +#ifdef SAMSABI_DEBUG > +#define DPRINTF(x) printf x > +#else > +#define DPRINTF(x) > +#endif > + > +/* Keyboard control */ > +#define SAMSABI_KBD_CMD_SET 0x10 > +#define SAMSABI_KBD_CMD_GET 0x11 > +#define SAMSABI_KBD_MIN_BACKLIGHT 0 > +#define SAMSABI_KBD_MAX_BACKLIGHT 3 > + > +struct samsabi_softc { > + struct device sc_dev; > + i2c_tag_t sc_tag; > + int sc_addr; > + > + unsigned int sc_backlight; > + struct timeout sc_to_keepalive; > +}; > + > +int samsabi_match(struct device *, void *, void *); > +void samsabi_attach(struct device *, struct device *, void *); > +int samsabi_activate(struct device *, int); > + > +int samsabi_cmd(struct samsabi_softc *, const uint8_t *, size_t, > + uint8_t *, size_t); > +int samsabi_kbd_get(struct samsabi_softc *, uint8_t *); > +int samsabi_kbd_set(struct samsabi_softc *, uint8_t); > +int samsabi_kbd_probe(struct samsabi_softc *); > +void samsabi_kbd_keepalive(void *); > +int samsabi_get_backlight(struct wskbd_backlight *); > +int samsabi_set_backlight(struct wskbd_backlight *); > +extern int (*wskbd_get_backlight)(struct wskbd_backlight *); > +extern int (*wskbd_set_backlight)(struct wskbd_backlight *); > + > +const struct cfattach samsabi_ca = { > + sizeof(struct samsabi_softc), samsabi_match, samsabi_attach, NULL, > + samsabi_activate > +}; > + > +struct cfdriver samsabi_cd = { > + NULL, "samsabi", DV_DULL > +}; > + > +int > +samsabi_match(struct device *parent, void *match, void *aux) > +{ > + struct i2c_attach_args *ia = aux; > + > + if (strcmp(ia->ia_name, "samsung,galaxybook-sabi") == 0) > + return 1; > + > + return 0; > +} > + > +void > +samsabi_attach(struct device *parent, struct device *self, void *aux) > +{ > + struct samsabi_softc *sc = (struct samsabi_softc *)self; > + struct i2c_attach_args *ia = aux; > + int error; > + > + sc->sc_tag = ia->ia_tag; > + sc->sc_addr = ia->ia_addr; > + > + error = samsabi_kbd_probe(sc); > + if (error) { > + printf(": keyboard backlight probe error=%d\n", error); > + return; > + } > + > + wskbd_get_backlight = samsabi_get_backlight; > + wskbd_set_backlight = samsabi_set_backlight; > + > + timeout_set_proc(&sc->sc_to_keepalive, samsabi_kbd_keepalive, sc); > + > + printf(": %s\n", ia->ia_name); > +} > + > +int > +samsabi_activate(struct device *self, int act) > +{ > + struct samsabi_softc *sc = (struct samsabi_softc *)self; > + > + switch (act) { > + case DVACT_QUIESCE: > + timeout_del(&sc->sc_to_keepalive); > + samsabi_kbd_set(sc, 0); > + break; > + case DVACT_WAKEUP: > + samsabi_kbd_set(sc, sc->sc_backlight); > + if (sc->sc_backlight != 0) > + timeout_add_sec(&sc->sc_to_keepalive, 1); > + break; > + } > + > + return 0; > +} > + > +int > +samsabi_cmd(struct samsabi_softc *sc, const uint8_t *cmd, size_t cmdlen, > + uint8_t *rsp, size_t rsplen) > +{ > + int error; > + > + iic_acquire_bus(sc->sc_tag, 0); > + > + if (rsplen > 0) { > + /* write command and read response */ > + error = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, > + sc->sc_addr, cmd, cmdlen, rsp, rsplen, 0); > + } else { > + /* write command */ > + error = iic_exec(sc->sc_tag, I2C_OP_WRITE_WITH_STOP, > + sc->sc_addr, cmd, cmdlen, NULL, 0, 0); > + } > + > + iic_release_bus(sc->sc_tag, 0); > + > + return error; > +} > + > +int > +samsabi_kbd_get(struct samsabi_softc *sc, uint8_t *level) > +{ > + uint8_t cmd = SAMSABI_KBD_CMD_GET; > + /* > + * GET command response: > + * rsp[3] = Backlight brightness level (0-3) > + */ > + uint8_t rsp[8]; > + int error; > + > + memset(rsp, 0, sizeof(rsp)); > + > + error = samsabi_cmd(sc, &cmd, 1, rsp, sizeof(rsp)); > + if (error) > + return error; > + > + DPRINTF(("%s: rsp=%02x %02x %02x %02x %02x %02x %02x %02x\n", > + __func__, rsp[0], rsp[1], rsp[2], rsp[3], rsp[4], rsp[5], rsp[6], > + rsp[7])); > + > + *level = rsp[3]; > + > + return 0; > +} > + > +int > +samsabi_kbd_set(struct samsabi_softc *sc, uint8_t level) > +{ > + /* > + * SET command: > + * cmd[0] = 0x10 > + * cmd[2] = Backlight brightness level: > + * 0 = off, 1 = low, 2 = mid, 3 = high > + */ > + uint8_t cmd[3] = { SAMSABI_KBD_CMD_SET, 0, level }; > + int error; > + > + error = samsabi_cmd(sc, cmd, sizeof(cmd), NULL, 0); > + if (error) > + return error; > + > + return 0; > +} > + > +int > +samsabi_kbd_probe(struct samsabi_softc *sc) > +{ > + int error; > + uint8_t level; > + > + /* Check if we can get the level value */ > + error = samsabi_kbd_get(sc, &level); > + if (error) > + return error; > + > + /* Make the keyboard light up once */ > + error = samsabi_kbd_set(sc, SAMSABI_KBD_MAX_BACKLIGHT); > + if (error) > + return error; > + > + return 0; > +} > + > +void > +samsabi_kbd_keepalive(void *arg) > +{ > + struct samsabi_softc *sc = arg; > + > + DPRINTF(("%s: level=%d\n", __func__, sc->sc_backlight)); > + > + (void)samsabi_kbd_set(sc, sc->sc_backlight); > + > + timeout_add_sec(&sc->sc_to_keepalive, 1); > +} > + > +int > +samsabi_get_backlight(struct wskbd_backlight *kbl) > +{ > + struct samsabi_softc *sc = samsabi_cd.cd_devs[0]; > + > + KASSERT(sc != NULL); > + > + kbl->min = SAMSABI_KBD_MIN_BACKLIGHT; > + kbl->max = SAMSABI_KBD_MAX_BACKLIGHT; > + kbl->curval = sc->sc_backlight; > + > + return 0; > +} > + > +int > +samsabi_set_backlight(struct wskbd_backlight *kbl) > +{ > + struct samsabi_softc *sc = samsabi_cd.cd_devs[0]; > + int error; > + uint8_t level; > + > + KASSERT(sc != NULL); > + > + DPRINTF(("%s: curval=%d\n", __func__, kbl->curval)); > + > + /* > + * XXX Only full brightness can be held lit: the EC fades lower > + * levels faster than the keepalive timeout can refresh them, so > + * treat any non-zero request as full brightness for now. > + */ > + level = kbl->curval ? SAMSABI_KBD_MAX_BACKLIGHT : 0; > + > + error = samsabi_kbd_set(sc, level); > + if (error) > + return error; > + > + sc->sc_backlight = level; > + > + if (sc->sc_backlight == 0) > + timeout_del(&sc->sc_to_keepalive); > + else > + timeout_add_sec(&sc->sc_to_keepalive, 1); > + > + return 0; > +} > Index: sys/dev/i2c/files.i2c > =================================================================== > RCS file: /cvs/src/sys/dev/i2c/files.i2c,v > diff -u -p -u -p -r1.75 files.i2c > --- sys/dev/i2c/files.i2c 19 May 2026 04:40:45 -0000 1.75 > +++ sys/dev/i2c/files.i2c 25 May 2026 20:03:18 -0000 > @@ -279,6 +279,11 @@ device sambat > attach sambat at i2c > file dev/i2c/sambat.c sambat > > +# Samsung Advanced BIOS Interface (SABI) > +device samsabi > +attach samsabi at i2c > +file dev/i2c/samsabi.c samsabi > + > # Consumer Control Keyboards > device icc: hid, hidcc, wskbddev > attach icc at ihidbus > Index: sys/arch/arm64/conf/GENERIC > =================================================================== > RCS file: /cvs/src/sys/arch/arm64/conf/GENERIC,v > diff -u -p -u -p -r1.316 GENERIC > --- sys/arch/arm64/conf/GENERIC 19 May 2026 04:49:33 -0000 1.316 > +++ sys/arch/arm64/conf/GENERIC 25 May 2026 20:03:19 -0000 > @@ -628,6 +628,7 @@ tcpci* at iic? # USB Type-C controlle > tipd* at iic? # TPS6598x Type-C controller > pijuice* at iic? # PiJuice HAT > sambat* at iic? # Samsung SAM060B battery monitor > +samsabi* at iic? # Samsung Advance BIOS Interface > > # GPIO "pin bus" drivers > gpioiic* at gpio? # I2C bus bit-banging > Index: share/man/man4/samsabi.4 > =================================================================== > RCS file: share/man/man4/samsabi.4 > diff -N share/man/man4/samsabi.4 > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ share/man/man4/samsabi.4 25 May 2026 20:03:19 -0000 > @@ -0,0 +1,72 @@ > +.\" $OpenBSD$ > +.\" > +.\" Copyright (c) 2026 Marcus Glocker > +.\" > +.\" Permission to use, copy, modify, and distribute this software for any > +.\" purpose with or without fee is hereby granted, provided that the above > +.\" copyright notice and this permission notice appear in all copies. > +.\" > +.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > +.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > +.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > +.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > +.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > +.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > +.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > +.\" > +.Dd $Mdocdate: May 25 2026 $ > +.Dt SAMSABI 4 > +.Os > +.Sh NAME > +.Nm samsabi > +.Nd Samsung laptop keyboard backlight > +.Sh SYNOPSIS > +.Cd "samsabi* at iic?" > +.Sh DESCRIPTION > +The > +.Nm > +driver provides support for the Samsung Advanced BIOS Interface > +.Pq SABI , > +a vendor command interface of the embedded controller found in Samsung > +laptops based on the Qualcomm Snapdragon X1E80100, such as the Galaxy > +Book4 Edge. > +The controller, identified as > +.Dq SAM060B > +in ACPI, is an ENE KB9058 micro controller running Samsung firmware. > +It exposes the SABI command protocol on the I2C bus at a separate > +address from the > +.Dq Mbox > +channel used by > +.Xr sambat 4 > +for battery status. > +.Pp > +The > +.Nm > +driver currently uses this interface to control the keyboard backlight. > +The backlight level can be retrieved and modified with the > +.Xr wsconsctl 8 > +variable > +.Va keyboard.backlight . > +.Sh SEE ALSO > +.Xr iic 4 , > +.Xr intro 4 , > +.Xr sambat 4 , > +.Xr wskbd 4 , > +.Xr wsconsctl 8 > +.Sh HISTORY > +The > +.Nm > +driver first appeared in > +.Ox 8.0 . > +.Sh AUTHORS > +.An -nosplit > +The > +.Nm > +driver was written by > +.An Marcus Glocker Aq Mt mglocker@openbsd.org . > +.Sh BUGS > +The embedded controller does not hold intermediate keyboard backlight > +levels: it fades the backlight off shortly after a level is set, and > +only full brightness can be refreshed often enough to be kept lit. > +As a result the driver treats any non-zero brightness request as full > +brightness. >