From: Kirill A. Korinsky Subject: sys/ihidev: respect RESET_RESPONSE and adjsut POWER ON timeout To: OpenBSD tech Cc: Marcus Glocker , mvs@openbsd.org Date: Fri, 03 Jan 2025 01:37:06 +0100 tech@, i2c states that a device should response for RESET in less than 5 seconds, and if it doesn't reponse as 0x00 0x00 it shouldn't be used. Also, a device shall not respond back after receiving POWER ON, and must ensure that it transitions power on state in less than 1 second, not 100ms. Without waiting the RESET response a HID report description from touchpad at Honor Magicbook 14 Art Snapdragon is reading as as series of 0xFF bytes every time, and with not enough waiting after power on likes 1 out of 10 reboot. Feedback and help to find the right fix instead of qurik from mvs@ Additionally tested on Huawe Matebook X 2020, no regression. Feedback? Tests? 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 00:24:12 -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); @@ -529,14 +556,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; } @@ -548,6 +576,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); @@ -557,7 +587,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 +605,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 +630,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