> Date: Fri, 03 Jan 2025 01:37:06 +0100
> From: Kirill A. Korinsky <kirill@korins.ky>
> 
> 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?

Apart from the style issues pointed out by mglocker@, there are a few
more.

> 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 */

The /* and */ for a multi-line comment need to be on a line of their
own (you got that right on the earlyier 7.2.1 comment).

> +	ihidev_sleep(sc, 1000);

So this changes the delay from 100ms to a full second.  That is quite
long and will make a machine boot noticably slower.  While the spec
does indead say that the device must transition to the specified power
state, it also suggests that I2C clock stretching should be used to
make to delay the I2C command completion until the device is powered
on.  Linux has the following comment:

       /*
        * The HID over I2C specification states that if a DEVICE needs time
        * after the PWR_ON request, it should utilise CLOCK stretching.
        * However, it has been observered that the Windows driver provides a
        * 1ms sleep between the PWR_ON and RESET requests.
        * According to Goodix Windows even waits 60 ms after (other?)
        * PWR_ON requests. Testing has confirmed that several devices
        * will not work properly without a delay after a PWR_ON request.
        */

Does your device still work if you leave this at 100ms?  If so, I
think we should stick with that.

>  
>  	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
> 
>