On Sun, Dec 22, 2024 at 10:23:30PM GMT, Kirill A. Korinsky wrote:

> After mglocker@ commit to qcgpio.c(v 1.12) with updated mapping, touchpad
> started worked.
> 
> The only quirks that I need to boot it is skip reset before get HID report.

If skipping the reset entirely fixes your touch-pad, I'm fine to
introduce the quirk.  Though, a few re-marks inline ...
 
> Index: sys/dev/i2c/ihidev.c
> ===================================================================
> RCS file: /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	22 Dec 2024 21:13:48 -0000
> @@ -61,6 +61,7 @@ static int I2C_HID_POWER_ON	= 0x0;
>  static int I2C_HID_POWER_OFF	= 0x1;
>  
>  int	ihidev_match(struct device *, void *, void *);
> +int	ihidev_quirks(struct ihidev_softc *);
>  void	ihidev_attach(struct device *, struct device *, void *);
>  int	ihidev_detach(struct device *, int);
>  int	ihidev_activate(struct device *, int);
> @@ -75,6 +76,17 @@ 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_SKIP_RESET	0x1
> +
> +const struct ihidev_quirks {
> +	uint16_t		ihq_vid;
> +	uint16_t		ihq_pid;
> +	int			ihq_quirks;
> +} ihidev_devs[] = {
> +	/* HONOR MagicBook Art 14 Touchpad (QTEC0002) */
> +	{ 0x35cc, 0x0104, IHIDEV_QUIRK_SKIP_RESET },
> +};
> +
>  const struct cfattach ihidev_ca = {
>  	sizeof(struct ihidev_softc),
>  	ihidev_match,
> @@ -98,6 +110,19 @@ ihidev_match(struct device *parent, void
>  	return (0);
>  }
>  
> +int
> +ihidev_quirks(struct ihidev_softc *sc)
> +{
> +	const struct ihidev_quirks *q;
> +	int i, nent = nitems(ihidev_devs);
> +
> +	for (i = 0, q = ihidev_devs; i < nent; i++, q++)
> +		if (q->ihq_vid == sc->sc_vid && q->ihq_pid == sc->sc_pid)

I think there is no need to introduce two new variables for the vendor
and product id since we already have that information available in
sc->hid_desc.wProductID and sc->hid_desc.wVendorID.

> +			return (q->ihq_quirks);
> +
> +	return (0);
> +}
> +
>  void
>  ihidev_attach(struct device *parent, struct device *self, void *aux)
>  {
> @@ -597,6 +622,13 @@ ihidev_hid_desc_parse(struct ihidev_soft
>  		return (1);
>  	}
>  
> +	sc->sc_vid = letoh16(sc->hid_desc.wVendorID);
> +	sc->sc_pid = letoh16(sc->hid_desc.wProductID);

Wouldn't be required as stated above.

> +	sc->sc_quirks = ihidev_quirks(sc);

Could we move this to the very top of the ihidev_hid_desc_parse()
function?  Since the product and vendor ids would be already available
at that point, and it would make the quirk assignment look more generic
in case we need to add more devices in the future, instead of just
placing it right before we need it for this device.

> +
> +	if (sc->sc_quirks & IHIDEV_QUIRK_SKIP_RESET)
> +		goto skip_rest;

I think "skip_reset" would be more clear.

> +
>  	while (retries-- > 0) {
>  		if (ihidev_reset(sc)) {
>  			if (retries == 0)
> @@ -608,6 +640,7 @@ ihidev_hid_desc_parse(struct ihidev_soft
>  			break;
>  	}
>  
> +skip_rest:
>  	sc->sc_reportlen = letoh16(sc->hid_desc.wReportDescLength);
>  	sc->sc_report = malloc(sc->sc_reportlen, M_DEVBUF, M_WAITOK | M_ZERO);
>  
> Index: sys/dev/i2c/ihidev.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/i2c/ihidev.h,v
> diff -u -p -r1.9 ihidev.h
> --- sys/dev/i2c/ihidev.h	3 Sep 2022 15:48:16 -0000	1.9
> +++ sys/dev/i2c/ihidev.h	22 Dec 2024 21:13:48 -0000
> @@ -93,6 +93,10 @@ struct ihidev_softc {
>  	int		sc_fastpoll;
>  	struct timeout	sc_timer;
>  	int		sc_dying;
> +
> +	uint16_t	sc_vid;
> +	uint16_t	sc_pid;

Wouldn't be required as stated above.

> +	int		sc_quirks;
>  };
>  
>  struct ihidev {
> 
> 
> -- 
> wbr, Kirill
>