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

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

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

Cheers,

Mark

> 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 19:03:21 -0000
> @@ -0,0 +1,286 @@
> +/*	$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;
> +
> +	struct rwlock	sc_lock;
> +	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;
> +
> +	rw_init(&sc->sc_lock, "samsabi");
> +
> +	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;
> +
> +	rw_enter_write(&sc->sc_lock);
> +	iic_acquire_bus(sc->sc_tag, I2C_F_POLL);
> +
> +	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, I2C_F_POLL);
> +	} else {
> +		/* write command */
> +		error = iic_exec(sc->sc_tag, I2C_OP_WRITE_WITH_STOP,
> +		    sc->sc_addr, cmd, cmdlen, NULL, 0, I2C_F_POLL);
> +	}
> +
> +	iic_release_bus(sc->sc_tag, I2C_F_POLL);
> +	rw_exit_write(&sc->sc_lock);
> +
> +	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 19:03:21 -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 19:03:21 -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 19:03:21 -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.
> 
>