From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: Fix boot on HONOR MagicBook Art 14 Snapdragon
To: Marcus Glocker <marcus@nazgul.ch>
Cc: kirill@korins.ky, tech@openbsd.org
Date: Mon, 23 Dec 2024 11:03:20 +0100

> Date: Mon, 23 Dec 2024 10:42:59 +0100
> From: Marcus Glocker <marcus@nazgul.ch>
> 
> 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 ...

I can't find any evidence in the Linux kernel tree that it skips the
reset when it probes the device.  And it has a quirk for this
particular hid-over-i2c device.  That said, that quirk was added for
the Intel variant of this machine.  So I'm fine with adding this quirk
as well.

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