On Wed, 25 Dec 2024 12:40:32 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > From: Vitaliy Makkoveev <otto@bsdbox.dev>
> > Date: Wed, 25 Dec 2024 01:33:40 +0300
> > 
> > > On 25 Dec 2024, at 01:26, Vitaliy Makkoveev <otto@bsdbox.dev> wrote:
> > > 
> > > delay(9) is arch specific, IIRC it doesn’t interrupts on x86.
> > 
> > Should be "it doesn’t block interrupts on x86."
> 
> It doesn't block interrupts on any architecture.
> 
> However, interrupts are blocked during "autoconf".  So our drivers
> can't rely on interrupts being delivered in their attach functions and
> need to poll instead.

This means that original design of a driver is the right one.

Here a diff which respects it. I also double check the timeout in the
specification and adjusted it with refference to a section.

Tested on two machine:
 - Huawe Matebook X 2020;
 - Honor Magicbook 14 Art Snapdragon.

Ok?

Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
diff -u -p -r1.33 ihidev.c
--- sys/dev/i2c/ihidev.c	18 Oct 2024 12:53:49 -0000	1.33
+++ sys/dev/i2c/ihidev.c	25 Dec 2024 12:22:59 -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;
@@ -515,6 +516,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);
@@ -528,14 +555,15 @@ ihidev_hid_command(struct ihidev_softc *
 int
 ihidev_poweron(struct ihidev_softc *sc)
 {
-	DPRINTF(("%s: resetting\n", sc->sc_dev.dv_xname));
-
 	if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, &I2C_HID_POWER_ON)) {
 		printf("%s: failed to power on\n", sc->sc_dev.dv_xname);
 		return (1);
 	}
 
-	ihidev_sleep(sc, 100);
+	/* 7.2.8 states that a device shall not respond back after
+	 * receiving the command, and must ensure that it transitions to
+	 * specified power state in less than 1 second */
+	ihidev_sleep(sc, 1000);
 
 	return 0;
 }
@@ -547,6 +575,8 @@ ihidev_reset(struct ihidev_softc *sc)
 	if (ihidev_poweron(sc))
 		return (1);
 
+	DPRINTF(("%s: resetting\n", sc->sc_dev.dv_xname));
+
 	if (ihidev_hid_command(sc, I2C_HID_CMD_RESET, 0)) {
 		printf("%s: failed to reset hardware\n", sc->sc_dev.dv_xname);
 
@@ -556,7 +586,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);
 }
@@ -570,8 +604,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",
@@ -597,16 +629,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