Index | Thread | Search

From:
Kirill A. Korinsky <kirill@korins.ky>
Subject:
Re: sys/ihidev: replace HONOR's touchpad quirk
To:
Mark Kettenis <mark.kettenis@xs4all.nl>, Marcus Glocker <mglocker@openbsd.org>
Cc:
OpenBSD tech <tech@openbsd.org>
Date:
Tue, 23 Jun 2026 12:26:58 +0200

Download raw body.

Thread
On Wed, 27 May 2026 12:53:46 +0200,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > Date: Wed, 27 May 2026 02:03:30 +0200
> > From: Kirill A. Korinsky <kirill@korins.ky>
> > 
> > deraadt@ suggested to send the first byte to that device.
> > 
> > It actually works and I started to dig around...
> > 
> > Seems that the touchpad ignores the first command and it is a kind wakeup
> > call. Replacing it to just touch an address works the same way.
> > 
> > It was observed that sending anything to the touchpad wakes it up but it
> > ignores the command that was sent. If it was GET_REPORT, when read reads
> > series of 0xff.
> > 
> > Sending anything to oher bus does not wake it up, but just touch it's
> > address works.
> > 
> > It smells like missed "wakeup" and I had checked linux sources in few
> > places, here and where, but I haven't found anything similar to such wakeup
> > call.
> > 
> > So, here a very narrow quirk replacment for affected device.
> > 
> > Thoughts? Ok?
> 
> This doesn't make a lot of sense to me.  What the Linux does is this
> extra POWER_ON command that we've implemented.  Why doesn't that "wake
> up" the device?  Is it in the wrong place?
> 

Sorry for long delay.

I had run a lot of expirements and checking.

So, I have more or less full picture about that touchpad.

The first things: Linux doesn't really care about GET_REPORT response. If it
doesn't read sane response (0xff 0xff ...), it uses default values.

We do not. We giveup on device.

A lot of my expirements ends with two possibel direction which makes that
touchpad stable to work.

1. A trivial hack by send the second GET_REPORT and ignore the first one.
   Small changes inside ihidev only.

2. We need to "touch" address before send that GET_REPORT somehow. I've used
   I2C_ADDR_ONLY for that and added it via cmdlen == 0 && buflen == 0.
   Ugly by implementation, but I don't see how to make it cleaner.
   But semantically it is minimal code which I needed to make it works.

I not sure which is better, but after a month of background thinking about
it I'd like to use the first one, with GET_REPORT as minimalized one.

And yes, I haven't found any trace of similar hack anywhere.

I attached both approach as two diff file.

-- 
wbr, Kirill
Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
diff -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 13:57:30 -0000
@@ -76,7 +76,7 @@ int	ihidev_maxrepid(void *buf, int len);
 int	ihidev_print(void *aux, const char *pnp);
 int	ihidev_submatch(struct device *parent, void *cf, void *aux);
 
-#define IHIDEV_QUIRK_RE_POWER_ON	0x1
+#define IHIDEV_QUIRK_RETRY_GET_REPORT	0x1
 
 const struct ihidev_quirks {
 	uint16_t		ihq_vid;
@@ -84,7 +84,7 @@ const struct ihidev_quirks {
 	int			ihq_quirks;
 } ihidev_devs[] = {
 	/* HONOR MagicBook Art 14 Touchpad (QTEC0002) */
-	{ 0x35cc, 0x0104, IHIDEV_QUIRK_RE_POWER_ON },
+	{ 0x35cc, 0x0104, IHIDEV_QUIRK_RETRY_GET_REPORT },
 };
 
 const struct cfattach ihidev_ca = {
@@ -368,6 +368,7 @@ ihidev_hid_command(struct ihidev_softc *
 		int dataoff = 4;
 		int report_id = rreq->id;
 		int report_len = rreq->len + 2 + 1;
+		int attempt, ntries;
 		int d;
 		uint8_t *tmprep;
 
@@ -403,8 +404,22 @@ ihidev_hid_command(struct ihidev_softc *
 		tmprep = malloc(report_len, M_DEVBUF, M_WAITOK | M_ZERO);
 
 		/* type 3 id 8: 22 00 38 02 23 00 */
-		res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
-		    &cmd, cmdlen, tmprep, report_len, 0);
+		ntries = (sc->sc_quirks & IHIDEV_QUIRK_RETRY_GET_REPORT) ?
+		    5 : 1;
+		for (attempt = 0; attempt < ntries; attempt++) {
+			memset(tmprep, 0, report_len);
+			res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP,
+			    sc->sc_addr, &cmd, cmdlen, tmprep, report_len, 0);
+
+			d = tmprep[0] | tmprep[1] << 8;
+			if (res == 0 &&
+			    d == report_len &&
+			    tmprep[2] == rreq->id)
+				break;
+
+			if (attempt + 1 < ntries)
+				ihidev_sleep(sc, 100);
+		}
 
 		d = tmprep[0] | tmprep[1] << 8;
 		if (d != report_len)
@@ -653,23 +668,6 @@ ihidev_hid_desc_parse(struct ihidev_soft
 		printf("%s: failed fetching HID report\n",
 		    sc->sc_dev.dv_xname);
 		return (1);
-	}
-
-	if (sc->sc_quirks & IHIDEV_QUIRK_RE_POWER_ON) {
-		if (ihidev_poweron(sc))
-			return (1);
-
-		/*
-		 * 7.2.8 states that a device shall not respond back
-		 * after receiving the power on command, and must ensure
-		 * that it transitions to power on state in less than 1
-		 * second. The ihidev_poweron function uses a shorter
-		 * sleep, sufficient for the ON-RESET sequence. Here,
-		 * however, it sleeps for the full second to accommodate
-		 * cold boot scenarios on affected devices.
-		 */
-
-		ihidev_sleep(sc, 1000);
 	}
 
 	return (0);
Index: sys/dev/fdt/qciic_fdt.c
===================================================================
RCS file: /home/cvs/src/sys/dev/fdt/qciic_fdt.c,v
diff -u -p -r1.4 qciic_fdt.c
--- sys/dev/fdt/qciic_fdt.c	24 May 2026 10:36:01 -0000	1.4
+++ sys/dev/fdt/qciic_fdt.c	26 May 2026 23:30:50 -0000
@@ -38,6 +38,7 @@
 #define GENI_M_CMD0			0x600
 #define  GENI_M_CMD0_OPCODE_I2C_WRITE	(0x1 << 27)
 #define  GENI_M_CMD0_OPCODE_I2C_READ	(0x2 << 27)
+#define  GENI_M_CMD0_OPCODE_I2C_ADDR_ONLY	(0x4 << 27)
 #define  GENI_M_CMD0_SLV_ADDR_SHIFT	9
 #define  GENI_M_CMD0_STOP_STRETCH	(1 << 2)
 #define GENI_M_IRQ_STATUS		0x610
@@ -222,6 +223,15 @@ qciic_fdt_exec(void *cookie, i2c_op_t op
 
 	if (buflen == 0 && I2C_OP_STOP_P(op))
 		m_param &= ~GENI_M_CMD0_STOP_STRETCH;
+
+	if (cmdlen == 0 && buflen == 0 && I2C_OP_WRITE_P(op)) {
+		stat = HREAD4(sc, GENI_M_IRQ_STATUS);
+		HWRITE4(sc, GENI_M_IRQ_CLEAR, stat);
+		m_cmd = GENI_M_CMD0_OPCODE_I2C_ADDR_ONLY | m_param;
+		HWRITE4(sc, GENI_M_CMD0, m_cmd);
+
+		return qciic_fdt_wait(sc, GENI_M_IRQ_CMD_DONE);
+	}
 
 	if (cmdlen > 0) {
 		stat = HREAD4(sc, GENI_M_IRQ_STATUS);
Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
diff -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	26 May 2026 23:36:08 -0000
@@ -76,7 +76,7 @@ int	ihidev_maxrepid(void *buf, int len);
 int	ihidev_print(void *aux, const char *pnp);
 int	ihidev_submatch(struct device *parent, void *cf, void *aux);
 
-#define IHIDEV_QUIRK_RE_POWER_ON	0x1
+#define IHIDEV_QUIRK_WAKEUP	0x1
 
 const struct ihidev_quirks {
 	uint16_t		ihq_vid;
@@ -84,7 +84,7 @@ const struct ihidev_quirks {
 	int			ihq_quirks;
 } ihidev_devs[] = {
 	/* HONOR MagicBook Art 14 Touchpad (QTEC0002) */
-	{ 0x35cc, 0x0104, IHIDEV_QUIRK_RE_POWER_ON },
+	{ 0x35cc, 0x0104, IHIDEV_QUIRK_WAKEUP },
 };
 
 const struct cfattach ihidev_ca = {
@@ -402,6 +402,16 @@ ihidev_hid_command(struct ihidev_softc *
 		 */
 		tmprep = malloc(report_len, M_DEVBUF, M_WAITOK | M_ZERO);
 
+		if (sc->sc_quirks & IHIDEV_QUIRK_WAKEUP) {
+			/*
+			 * Some devices, for example HONOR's Touchpad (QTEC0002)
+			 * ignores the first command and returns all 0xff bytes
+			 * unless it was "woke up" by an addressed transaction.
+			 */
+			iic_exec(sc->sc_tag, I2C_OP_WRITE_WITH_STOP, sc->sc_addr,
+			    NULL, 0, NULL, 0, 0);
+		}
+
 		/* type 3 id 8: 22 00 38 02 23 00 */
 		res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
 		    &cmd, cmdlen, tmprep, report_len, 0);
@@ -653,23 +663,6 @@ ihidev_hid_desc_parse(struct ihidev_soft
 		printf("%s: failed fetching HID report\n",
 		    sc->sc_dev.dv_xname);
 		return (1);
-	}
-
-	if (sc->sc_quirks & IHIDEV_QUIRK_RE_POWER_ON) {
-		if (ihidev_poweron(sc))
-			return (1);
-
-		/*
-		 * 7.2.8 states that a device shall not respond back
-		 * after receiving the power on command, and must ensure
-		 * that it transitions to power on state in less than 1
-		 * second. The ihidev_poweron function uses a shorter
-		 * sleep, sufficient for the ON-RESET sequence. Here,
-		 * however, it sleeps for the full second to accommodate
-		 * cold boot scenarios on affected devices.
-		 */
-
-		ihidev_sleep(sc, 1000);
 	}
 
 	return (0);