On Fri, Jan 03, 2025 at 06:21:02PM GMT, Kirill A. Korinsky wrote:

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

Even better if we can plug the 1000ms delay in to a quirk only for the
devices which require it!

Two small comments inline.
 
> 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);

You could have fixed this line break if you anyway had to create a new
patch ;-)

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

Have you intentionally removed the DPRINTF "resetting" from your last
patch?  I think it would be better placed in the reset function itself,
as in your last patch.

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