From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: uhidev fix for set report output command
To: Marcus Glocker <marcus@nazgul.ch>
Cc: tech@openbsd.org
Date: Mon, 06 Jan 2025 22:37:59 +0100

> Date: Mon, 6 Jan 2025 21:53:25 +0100
> From: Marcus Glocker <marcus@nazgul.ch>
> 
> When I was trying to add LED support for ikbd(4), I noticed that the set
> report output command in ihidev isn't working as expected, so my caps
> lock LED never got lit.  While checking with kettenis@ on that topic,
> he reported that he faced the same issue back then when trying to add
> LED support to ikbd(4), and he is suspecting an issue in the ihidev set
> report command.
> 
> After investigating further, it turned out that when we use the
> wOutputRegister for the set report output command, the wCommandRegister
> block needs to be omitted.  There is a clear statement in the Linux
> driver about it:
> 
> https://github.com/torvalds/linux/blob/master/drivers/hid/i2c-hid/i2c-hid-core.c#L372
> 
> With the following patch, and the ikbd LED extension (which I would
> share once this got fixed), I can control the caps lock LED on my Samsung
> laptop.  I allowed myself to add some comment, since this isn't so
> obvious to grasp, and not very well documented in the hid-over-i2c specs
> IMO.

Can you share it right now such that I can test this on the x13s?

One diff that includes both changes is fine for that.

> Index: sys/dev/i2c/ihidev.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
> diff -u -p -u -p -r1.35 ihidev.c
> --- sys/dev/i2c/ihidev.c	6 Jan 2025 02:13:55 -0000	1.35
> +++ sys/dev/i2c/ihidev.c	6 Jan 2025 20:40:39 -0000
> @@ -453,6 +453,22 @@ ihidev_hid_command(struct ihidev_softc *
>  			cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister)
>  			    >> 8;
>  		} else {
> +			/*
> +			 * When using the wOutputRegister for a set report
> +			 * output command, the wCommandRegister command needs
> +			 * to be omitted, and we write the report data straight
> +			 * in to the wOutputRegister:
> +			 *
> +			 * cmd[0] = wOutputRegister (LSB)
> +			 * cmd[1] = wOutputRegister (MSB)
> +			 * cmd[2] = Length Field (LSB)
> +			 * cmd[3] = Length Field (MSB)
> +			 * cmd[4] = Report ID
> +			 * cmd[5] = DATA
> +			 * ...
> +			 */
> +			cmdlen = 5;
> +			dataoff = 0;
>  			cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister)
>  			    & 0xff;
>  			cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister)
> 
>