From: Mike Larkin <mlarkin@nested.page>
Subject: Re: ihidev: be less aggressive with reset, fix T14s
To: Tobias Heider <tobias.heider@stusta.de>
Cc: tech@openbsd.org
Date: Sun, 13 Oct 2024 19:04:50 -0700

On Sat, Oct 12, 2024 at 02:53:08PM +0200, Tobias Heider wrote:
> This diff fixes the ihidev keyboard on the Lenovo T14s Gen6.
> It turns out that this device doesn't like to receive multiple reset
> commands at all, after looking at other drivers it seems like we are
> the only ones that reset so aggressively.
>
> The diff replaces all but one occurrences of ihidev_reset() with the
> new ihidev_poweron() which only sends a power on command instead of
> always doing both.
>
> It would probably make sense to test this extensively on a range of hardware,
> Linux has a quirk to add explicit resets but that seems to only be needed for
> very few devices (ALPS 0x44e, possibly more).
> If break with this we might want to add a quirk to still reset them.
>

Thank you for finding this. Works on my t14s gen6

> diff 502b52cbdf550ec88ed0fcdac323f77049e93d3d 56627129dafdc9ab5465cdd1d777aa09e63460c1
> commit - 502b52cbdf550ec88ed0fcdac323f77049e93d3d
> commit + 56627129dafdc9ab5465cdd1d777aa09e63460c1
> blob - 4e65a4bf350b37ad5567a0c8d6535bd4e3c9cdbe
> blob + f6b8cc7b3436dc6c9bff99c817d4e5e2a45c3f44
> --- sys/dev/i2c/ihidev.c
> +++ sys/dev/i2c/ihidev.c
> @@ -67,6 +67,7 @@ int	ihidev_activate(struct device *, int);
>
>  int	ihidev_hid_command(struct ihidev_softc *, int, void *);
>  int	ihidev_intr(void *);
> +int	ihidev_poweron(struct ihidev_softc *);
>  int	ihidev_reset(struct ihidev_softc *);
>  int	ihidev_hid_desc_parse(struct ihidev_softc *);
>
> @@ -248,7 +249,7 @@ ihidev_activate(struct device *self, int act)
>  			    sc->sc_dev.dv_xname);
>  		break;
>  	case DVACT_WAKEUP:
> -		ihidev_reset(sc);
> +		ihidev_poweron(sc);
>  		sc->sc_dying = 0;
>  		if (sc->sc_poll && timeout_initialized(&sc->sc_timer))
>  			timeout_add(&sc->sc_timer, 2000);
> @@ -525,7 +526,7 @@ ihidev_hid_command(struct ihidev_softc *sc, int hidcmd
>  }
>
>  int
> -ihidev_reset(struct ihidev_softc *sc)
> +ihidev_poweron(struct ihidev_softc *sc)
>  {
>  	DPRINTF(("%s: resetting\n", sc->sc_dev.dv_xname));
>
> @@ -536,6 +537,16 @@ ihidev_reset(struct ihidev_softc *sc)
>
>  	ihidev_sleep(sc, 100);
>
> +	return 0;
> +}
> +
> +
> +int
> +ihidev_reset(struct ihidev_softc *sc)
> +{
> +	if (ihidev_poweron(sc))
> +		return (1);
> +
>  	if (ihidev_hid_command(sc, I2C_HID_CMD_RESET, 0)) {
>  		printf("%s: failed to reset hardware\n", sc->sc_dev.dv_xname);
>
> @@ -784,7 +795,7 @@ ihidev_open(struct ihidev *scd)
>  		return (0);
>
>  	/* power on */
> -	ihidev_reset(sc);
> +	ihidev_poweron(sc);
>
>  	if (sc->sc_poll) {
>  		if (!timeout_initialized(&sc->sc_timer))
>