From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/ihidev: replace HONOR's touchpad quirk
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Tue, 26 May 2026 21:21:52 +0200

On Sat, May 23, 2026 at 07:19:46PM +0200, Kirill A. Korinsky wrote:

> On Sat, 23 May 2026 16:23:45 +0200,
> Kirill A. Korinsky <kirill@korins.ky> wrote:
> > 
> > tech@,
> > 
> > Old quirk made touchpad with multitouch wroks, but it was unstable and
> > doesn't work each boot.
> > 
> > Here better quirck which simple re-reads reports multiple times and
> > usually 2-3 repeated read are enough.
> > 
> > With new quirck multitouch works each boot, I've tested 10 reboot and 10
> > cold boot.
> > 
> > Open questions: shall it be kept as device quirck or generic policy? 

I think this should be kept as a quirk.

> > Ok to commit as a quirk?
> > 
> 
> I had walk into that with instruments till the HWRITE4() and it seems that
> this device always return 0xff on the first GET_REPORT.
> 
> Both HWRITE4() write the same command, but the first response is 0xff... and
> the second one is meaningful report.
> 
> Adding timeout before command do not helps.
> 
> I have no better idea that add such quirk.
> 
> Which, actually, makes it quite stable.
> 
> Thoughs? Suggestions? Ok?

I general, I think the quirk to re-read the report descriptor is
better than the previous delay quirk.  Though, some comments in-line.

> Index: sys/dev/i2c/ihidev.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/i2c/ihidev.c,v
> diff -u -p -r1.43 ihidev.c
> --- sys/dev/i2c/ihidev.c	23 May 2026 11:10:57 -0000	1.43
> +++ sys/dev/i2c/ihidev.c	23 May 2026 17:14:57 -0000
> @@ -76,7 +76,7 @@ int	ihidev_maxrepid(void *buf, int len);
>  int	ihidev_print(void *aux, const char *pnp);
>  int	ihidev_submatch(struct device *parent, void *cf, void *aux);
>  
> -#define IHIDEV_QUIRK_RE_POWER_ON	0x1
> +#define IHIDEV_QUIRK_RETRY_GET_REPORT	0x1
>  
>  const struct ihidev_quirks {
>  	uint16_t		ihq_vid;
> @@ -84,7 +84,7 @@ const struct ihidev_quirks {
>  	int			ihq_quirks;
>  } ihidev_devs[] = {
>  	/* HONOR MagicBook Art 14 Touchpad (QTEC0002) */
> -	{ 0x35cc, 0x0104, IHIDEV_QUIRK_RE_POWER_ON },
> +	{ 0x35cc, 0x0104, IHIDEV_QUIRK_RETRY_GET_REPORT },
>  };
>  
>  const struct cfattach ihidev_ca = {
> @@ -406,6 +406,17 @@ ihidev_hid_command(struct ihidev_softc *
>  		res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr,
>  		    &cmd, cmdlen, tmprep, report_len, 0);
>  
> +		if (sc->sc_quirks & IHIDEV_QUIRK_RETRY_GET_REPORT) {
> +			/*
> +			 * HONOR MagicBook Art 14 Touchpad (QTEC0002) returns
> +			 * all 0xff bytes for the first GET_REPORT, but
> +			 * meaningful report on the second attemtp.

Typo -> attempt.

> +			 */
> +			memset(tmprep, 0, report_len);
> +			res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP,
> +			    sc->sc_addr, &cmd, cmdlen, tmprep, report_len, 0);
> +		}
> +

In your initial e-mail you are saying 2-3 repeated reads are OK.
Now you only implement another additional single read.  Are you sure
this is enough?  Wouldn't a capped retry loop be better?  Like 3-5
read attempts before finally failing?

>  		d = tmprep[0] | tmprep[1] << 8;
>  		if (d != report_len)
>  			DPRINTF(("%s: response size %d != expected length %d\n",
> @@ -653,23 +664,6 @@ ihidev_hid_desc_parse(struct ihidev_soft
>  		printf("%s: failed fetching HID report\n",
>  		    sc->sc_dev.dv_xname);
>  		return (1);
> -	}
> -
> -	if (sc->sc_quirks & IHIDEV_QUIRK_RE_POWER_ON) {
> -		if (ihidev_poweron(sc))
> -			return (1);
> -
> -		/*
> -		 * 7.2.8 states that a device shall not respond back
> -		 * after receiving the power on command, and must ensure
> -		 * that it transitions to power on state in less than 1
> -		 * second. The ihidev_poweron function uses a shorter
> -		 * sleep, sufficient for the ON-RESET sequence. Here,
> -		 * however, it sleeps for the full second to accommodate
> -		 * cold boot scenarios on affected devices.
> -		 */
> -
> -		ihidev_sleep(sc, 1000);
>  	}
>  
>  	return (0);
> 
> 
> -- 
> wbr, Kirill
>