Index | Thread | Search

From:
Kirill A. Korinsky <kirill@korins.ky>
Subject:
Re: ims(4) touchscreen support for the SGB4E
To:
Marcus Glocker <marcus@nazgul.ch>
Cc:
tech@openbsd.org, joshua stein <jcs@jcs.org>
Date:
Sat, 23 May 2026 21:58:45 +0200

Download raw body.

Thread
On Sat, 23 May 2026 17:01:50 +0200,
Marcus Glocker <marcus@nazgul.ch> wrote:
> 
> This supersedes the earlier imt(4) touchscreen diff, which regressed
> kirill's Honor panel.
> 
> The difference is that we enable the Samsung Galaxy Book4 Edge
> touchscreen support with ims(4), not imt(4), which has less impact,
> and introduces less risk for regression:
> 
> - The ihidev(4) change resolves the in-tree polled finger-up TODO
>   and is child-agnostic.  This touchscreen remains polled, until we
>   can find out which is the correct GPIO for enabling interrupts on
>   this device.
> 
> - The hidms gate is edge-not-level so it spares pens and never touches
>   relative mice.
> 
> Together with the DTS diff (not changed from last time) to power-on
> the touchpanel device, this makes the touchscreen work on the
> Samsung Galaxy Book4 Edge.
> 
> I would appreciate some regression testing from people who are using
> ims(4) devices.
>

Tested on

1. Huawei Matebook X 2020 where I have 3 ims devices

ims0 at ihidev0 reportid 1: 2 buttons
wsmouse1 at ims0 mux 0
ims1 at ihidev0 reportid 14: 0 buttons
wsmouse2 at ims1 mux 0
ims2 at ihidev1 reportid 1: 1 button, tip
wsmouse3 at ims2 mux 0

2. Honor Magicbook 14 Snapdragon with 

ims0 at ihidev0 reportid 1: 1 button, tip
wsmouse0 at ims0 mux 0
ims1 at ihidev2 reportid 1: 3 buttons, Z dir
wsmouse2 at ims1 mux 0

No regression: touchscreen, touchpad with multitouch works on both laptops.

> 
> Index: sys/dev/hid/hidms.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/hid/hidms.c,v
> diff -u -p -u -p -r1.11 hidms.c
> --- sys/dev/hid/hidms.c	10 May 2024 10:49:10 -0000	1.11
> +++ sys/dev/hid/hidms.c	23 May 2026 14:30:24 -0000
> @@ -533,7 +533,7 @@ hidms_input(struct hidms *ms, uint8_t *d
>  {
>  	int dx, dy, dz, dw;
>  	u_int32_t buttons = 0;
> -	int i, s;
> +	int i, s, released;
>  
>  	DPRINTFN(5,("hidms_input: len=%d\n", len));
>  
> @@ -595,17 +595,26 @@ hidms_input(struct hidms *ms, uint8_t *d
>  	    buttons != ms->sc_buttons) {
>  		DPRINTFN(10, ("hidms_input: x:%d y:%d z:%d w:%d buttons:0x%x\n",
>  			dx, dy, dz, dw, buttons));
> +		released = (ms->sc_buttons != 0 && buttons == 0);
>  		ms->sc_buttons = buttons;
>  		if (ms->sc_wsmousedev != NULL) {
>  			s = spltty();
> +			/*
> +			 * A polled touchscreen reports finger-up as a zeroed
> +			 * packet; emitting its (0,0) as an absolute position
> +			 * would snap the pointer to the corner.  On a button
> +			 * release edge, hold the last position instead.
> +			 */
>  			if (ms->sc_flags & HIDMS_ABSX) {
> -				wsmouse_set(ms->sc_wsmousedev,
> -				    WSMOUSE_ABS_X, dx, 0);
> +				if (!released)
> +					wsmouse_set(ms->sc_wsmousedev,
> +					    WSMOUSE_ABS_X, dx, 0);
>  				dx = 0;
>  			}
>  			if (ms->sc_flags & HIDMS_ABSY) {
> -				wsmouse_set(ms->sc_wsmousedev,
> -				    WSMOUSE_ABS_Y, dy, 0);
> +				if (!released)
> +					wsmouse_set(ms->sc_wsmousedev,
> +					    WSMOUSE_ABS_Y, dy, 0);
>  				dy = 0;
>  			}
>  			WSMOUSE_INPUT(ms->sc_wsmousedev,
> Index: sys/dev/i2c/ihidev.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
> diff -u -p -u -p -r1.43 ihidev.c
> --- sys/dev/i2c/ihidev.c	23 May 2026 11:10:57 -0000	1.43
> +++ sys/dev/i2c/ihidev.c	23 May 2026 14:30:24 -0000
> @@ -142,6 +142,7 @@ ihidev_attach(struct device *parent, str
>  	sc->sc_tag = ia->ia_tag;
>  	sc->sc_addr = ia->ia_addr;
>  	sc->sc_hid_desc_addr = ia->ia_size;
> +	sc->sc_lastrepid = -1;
>  
>  	if (ihidev_hid_command(sc, I2C_HID_CMD_DESCR, NULL) ||
>  	    ihidev_hid_desc_parse(sc)) {
> @@ -159,10 +160,13 @@ ihidev_attach(struct device *parent, str
>  
>  	/* find largest report size and allocate memory for input buffer */
>  	sc->sc_isize = letoh16(sc->hid_desc.wMaxInputLength);
> +	sc->sc_repsizes = mallocarray(sc->sc_nrepid, sizeof(int),
> +	    M_DEVBUF, M_WAITOK | M_ZERO);
>  	for (repid = 0; repid < sc->sc_nrepid; repid++) {
>  		repsz = hid_report_size(sc->sc_report, sc->sc_reportlen,
>  		    hid_input, repid);
>  		repsizes[repid] = repsz;
> +		sc->sc_repsizes[repid] = repsz;
>  		if (repsz > sc->sc_isize)
>  			sc->sc_isize = repsz;
>  		if (repsz != 0)
> @@ -255,6 +259,9 @@ ihidev_detach(struct device *self, int f
>  	if (sc->sc_report != NULL)
>  		free(sc->sc_report, M_DEVBUF, sc->sc_reportlen);
>  
> +	if (sc->sc_repsizes != NULL)
> +		free(sc->sc_repsizes, M_DEVBUF, sc->sc_nrepid * sizeof(int));
> +
>  	return (0);
>  }
>  
> @@ -718,10 +725,21 @@ ihidev_intr(void *arg)
>  	psize = sc->sc_ibuf[0] | sc->sc_ibuf[1] << 8;
>  	if (psize <= 2 || psize > sc->sc_isize) {
>  		if (sc->sc_poll) {
> -			/*
> -			 * TODO: all fingers are up, should we pass to hid
> -			 * layer?
> -			 */
> +			/* empty packet: hand the last subdev a zeroed report
> +			 * once so it releases its contacts (polled finger-up) */
> +			int lrep = sc->sc_lastrepid;
> +			int rsz;
> +
> +			if (lrep >= 0 && lrep < sc->sc_nrepid &&
> +			    (scd = sc->sc_subdevs[lrep]) != NULL &&
> +			    (scd->sc_state & IHIDEV_OPEN) && !sc->sc_dying) {
> +				rsz = sc->sc_repsizes[lrep];
> +				if (rsz > 0 && rsz <= sc->sc_isize) {
> +					memset(sc->sc_ibuf, 0, rsz);
> +					scd->sc_intr(scd, sc->sc_ibuf, rsz);
> +				}
> +				sc->sc_lastrepid = -1;
> +			}
>  			sc->sc_fastpoll = 0;
>  			goto more_polling;
>  		} else
> @@ -770,8 +788,10 @@ ihidev_intr(void *arg)
>  		return (1);
>  	}
>  
> -	if (!sc->sc_dying)
> +	if (!sc->sc_dying) {
> +		sc->sc_lastrepid = rep;
>  		scd->sc_intr(scd, p, psize);
> +	}
>  
>  	if (sc->sc_poll && (fast != sc->sc_fastpoll)) {
>  		DPRINTF(("%s: %s->%s polling\n", sc->sc_dev.dv_xname,
> Index: sys/dev/i2c/ihidev.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/i2c/ihidev.h,v
> diff -u -p -u -p -r1.11 ihidev.h
> --- sys/dev/i2c/ihidev.h	7 Jan 2025 19:26:14 -0000	1.11
> +++ sys/dev/i2c/ihidev.h	23 May 2026 14:30:24 -0000
> @@ -85,6 +85,8 @@ struct ihidev_softc {
>  
>  	u_int		sc_isize;
>  	u_char		*sc_ibuf;
> +	int		sc_lastrepid;	/* report id of last non-empty input */
> +	int		*sc_repsizes;	/* per-report input size, for poll path */
>  
>  	int		sc_refcnt;
>  
> Index: sysutils/firmware/arm64-qcom-dtb/Makefile
> ===================================================================
> RCS file: /cvs/ports/sysutils/firmware/arm64-qcom-dtb/Makefile,v
> diff -u -p -u -p -r1.29 Makefile
> --- sysutils/firmware/arm64-qcom-dtb/Makefile	19 May 2026 04:47:35 -0000	1.29
> +++ sysutils/firmware/arm64-qcom-dtb/Makefile	21 May 2026 19:32:13 -0000
> @@ -1,6 +1,6 @@
>  FW_DRIVER=	arm64-qcom-dtb
>  FW_VER=		2.7
> -REVISION=	1
> +REVISION=	2
>  
>  DISTNAME=	devicetree-rebasing-6.17-dts
>  
> Index: sysutils/firmware/arm64-qcom-dtb/patches/patch-src_arm64_qcom_x1e80100-samsung-galaxy-book4-edge_dts
> ===================================================================
> RCS file: /cvs/ports/sysutils/firmware/arm64-qcom-dtb/patches/patch-src_arm64_qcom_x1e80100-samsung-galaxy-book4-edge_dts,v
> diff -u -p -u -p -r1.3 patch-src_arm64_qcom_x1e80100-samsung-galaxy-book4-edge_dts
> --- sysutils/firmware/arm64-qcom-dtb/patches/patch-src_arm64_qcom_x1e80100-samsung-galaxy-book4-edge_dts	19 May 2026 04:47:35 -0000	1.3
> +++ sysutils/firmware/arm64-qcom-dtb/patches/patch-src_arm64_qcom_x1e80100-samsung-galaxy-book4-edge_dts	21 May 2026 19:32:13 -0000
> @@ -1,7 +1,7 @@
>  Index: src/arm64/qcom/x1e80100-samsung-galaxy-book4-edge.dts
>  --- src/arm64/qcom/x1e80100-samsung-galaxy-book4-edge.dts.orig
>  +++ src/arm64/qcom/x1e80100-samsung-galaxy-book4-edge.dts
> -@@ -0,0 +1,977 @@
> +@@ -0,0 +1,988 @@
>  +// SPDX-License-Identifier: BSD-3-Clause
>  +/*
>  + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> @@ -10,6 +10,7 @@ Index: src/arm64/qcom/x1e80100-samsung-g
>  +/dts-v1/;
>  +
>  +#include <dt-bindings/gpio/gpio.h>
> ++#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>  +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>  +
>  +#include "x1e80100.dtsi"
> @@ -181,6 +182,24 @@ Index: src/arm64/qcom/x1e80100-samsung-g
>  +		regulator-always-on;
>  +		regulator-boot-on;
>  +	};
> ++
> ++	/* 3V3 rail powering the touchscreen, gated by pm8550ve_8 gpio6 */
> ++	vreg_misc_3p3: regulator-misc-3p3 {
> ++		compatible = "regulator-fixed";
> ++
> ++		regulator-name = "VREG_MISC_3P3";
> ++		regulator-min-microvolt = <3300000>;
> ++		regulator-max-microvolt = <3300000>;
> ++
> ++		gpio = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>;
> ++		enable-active-high;
> ++
> ++		pinctrl-0 = <&misc_3p3_reg_en>;
> ++		pinctrl-names = "default";
> ++
> ++		regulator-boot-on;
> ++		regulator-always-on;
> ++	};
>  +};
>  +
>  +&apps_rsc {
> @@ -572,26 +591,33 @@ Index: src/arm64/qcom/x1e80100-samsung-g
>  +	};
>  +};
>  +
> ++/* GXTP7936 touchscreen, powered by vreg_misc_3p3.  No usable IRQ pin
> ++ * known, so ihidev(4) polls. */
>  +&i2c8 {
>  +	clock-frequency = <400000>;
>  +
> -+	status = "disabled";
> ++	status = "okay";
>  +
>  +	touchscreen@5d {
>  +		compatible = "hid-over-i2c";
>  +		reg = <0x5d>;
>  +
>  +		hid-descr-addr = <0x1>;
> -+		/*
> -+		 * Pin 51 is creating an interrupt storm.  Hence, choosing
> -+		 * some other pin which just does nothing.  But obviously,
> -+		 * the touchscreen won't work for now.
> -+		 */
> -+		 //interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
> -+		interrupts-extended = <&tlmm 33 IRQ_TYPE_LEVEL_LOW>;
>  +
> -+		pinctrl-0 = <&ts0_default>;
> -+		pinctrl-names = "default";
> ++		vdd-supply = <&vreg_misc_3p3>;
> ++	};
> ++};
> ++
> ++&pm8550ve_8_gpios {
> ++	misc_3p3_reg_en: misc-3p3-reg-en-state {
> ++		pins = "gpio6";
> ++		function = "normal";
> ++		bias-disable;
> ++		input-disable;
> ++		output-enable;
> ++		drive-push-pull;
> ++		power-source = <1>;	/* 1.8 V */
> ++		qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
>  +	};
>  +};
>  +
> @@ -863,21 +889,6 @@ Index: src/arm64/qcom/x1e80100-samsung-g
>  +		pins = "gpio3";
>  +		function = "gpio";
>  +		bias-disable;
> -+	};
> -+
> -+	ts0_default: ts0-default-state {
> -+		int-n-pins {
> -+			pins = "gpio51";
> -+			function = "gpio";
> -+			bias-disable;
> -+		};
> -+
> -+		reset-n-pins {
> -+			pins = "gpio48";
> -+			function = "gpio";
> -+			output-high;
> -+			drive-strength = <16>;
> -+		};
>  +	};
>  +
>  +	wcd_default: wcd-reset-n-active-state {

-- 
wbr, Kirill