From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/ihidev: respect RESET_RESPONSE and adjsut POWER ON timeout
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: tech@openbsd.org, marcus@nazgul.ch, mvs@openbsd.org
Date: Fri, 03 Jan 2025 18:21:02 +0100

On Fri, 03 Jan 2025 16:00:52 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> Does your device still work if you leave this at 100ms?  If so, I
> think we should stick with that.
>

Thanks to challenge it. Indeed, add a delay for all machines is wrong move.

The longer sleep is required for the second patch only where I introduce a
quirk to re-power the device. Without waitng quite enough, almost the
seconds, it reads only as:

        imt0 at ihidev1ihidev1: failed fetching report
        ims0 at ihidev1 reportid 1: 3 buttons, Z dir

but seems that only on the cold boot. Otherwise with 1 seconds sleep or on
warm boot, it reads as:

        imt0 at ihidev1: touchpad, 5 contacts
        ims0 at ihidev1 reportid 1: 3 buttons, Z dir

Here I drop sleep changes, and moved them to the second patch which is
specifed for only affected devices.

Ok?

Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
diff -u -p -u -p -r1.34 ihidev.c
--- sys/dev/i2c/ihidev.c	2 Jan 2025 23:26:50 -0000	1.34
+++ sys/dev/i2c/ihidev.c	3 Jan 2025 16:28:19 -0000
@@ -55,6 +55,7 @@ enum {
 
 	/* pseudo commands */
 	I2C_HID_REPORT_DESCR	= 0x100,
+	I2C_HID_RESET_RESPONSE	= 0x101,
 };
 
 static int I2C_HID_POWER_ON	= 0x0;
@@ -516,6 +517,32 @@ ihidev_hid_command(struct ihidev_softc *
 
 		break;
 	}
+	case I2C_HID_RESET_RESPONSE: {
+		int i;
+		uint8_t buf[2] = { 0xff, 0xff };
+
+		DPRINTF(("%s: HID command I2C_HID_RESET_RESPONSE\n",
+		    sc->sc_dev.dv_xname));
+
+		/*
+		 * 7.2.1 states that a device should response for RESET
+		 * in less than 5 seconds. It uses poll instead of
+		 * tsleep because interrupts are blocked during autoconf.
+		 */
+		for (i = 0; i < 50; i++) {
+			ihidev_sleep(sc, 100);
+			res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
+			    NULL, 0, buf, sizeof(buf), 0);
+			DPRINTF(("%s: read attempt %d: 0x%x, 0x%x, res: %d\n",
+			    sc->sc_dev.dv_xname, i, buf[0], buf[1], res));
+			if (!res)
+				res = (buf[0] != 0x00 || buf[1] != 0x00);
+			if (!res)
+				break;
+		}
+
+		break;
+	}
 	default:
 		printf("%s: unknown command %d\n", sc->sc_dev.dv_xname,
 		    hidcmd);
@@ -557,7 +584,11 @@ ihidev_reset(struct ihidev_softc *sc)
 		return (1);
 	}
 
-	ihidev_sleep(sc, 100);
+	if (ihidev_hid_command(sc, I2C_HID_RESET_RESPONSE, 0)) {
+		printf("%s: unexpected reset response\n",
+		    sc->sc_dev.dv_xname);
+		return (1);
+	}
 
 	return (0);
 }
@@ -571,8 +602,6 @@ ihidev_reset(struct ihidev_softc *sc)
 int
 ihidev_hid_desc_parse(struct ihidev_softc *sc)
 {
-	int retries = 3;
-
 	/* must be v01.00 */
 	if (letoh16(sc->hid_desc.bcdVersion) != 0x0100) {
 		printf("%s: bad HID descriptor bcdVersion (0x%x)\n",
@@ -598,16 +627,8 @@ ihidev_hid_desc_parse(struct ihidev_soft
 		return (1);
 	}
 
-	while (retries-- > 0) {
-		if (ihidev_reset(sc)) {
-			if (retries == 0)
-				return(1);
-
-			ihidev_sleep(sc, 10);
-		}
-		else
-			break;
-	}
+	if (ihidev_reset(sc))
+		return (1);
 
 	sc->sc_reportlen = letoh16(sc->hid_desc.wReportDescLength);
 	sc->sc_report = malloc(sc->sc_reportlen, M_DEVBUF, M_WAITOK | M_ZERO);


-- 
wbr, Kirill