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 <marcus@nazgul.ch>
> > 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.


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 <mglocker@openbsd.org>
+ *
+ * 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 <sys/param.h>
+#include <sys/systm.h>
+#include <sys/device.h>
+#include <sys/timeout.h>
+
+#include <dev/i2c/i2cvar.h>
+#include <dev/wscons/wsconsio.h>
+
+/* 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 <mglocker@openbsd.org>
+.\"
+.\" 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.