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: Tue, 07 Jan 2025 13:31:09 +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.
> 
> I couldn't find any other driver which uses the I2C_HID_REPORT_TYPE_OUTPUT
> set report command yet.  Probably the reason why this kept undiscovered
> until now.
> 
> Feedback, OKs?

I looked a bit further into this and I think I understand what is
going on here.

The LEDs are set using an Output report.  The USB HID class spec has
the following to say about those:

  The Interrupt Out pipe is optional. If a device declares an
  Interrupt Out endpoint then Output reports are transmitted by the
  host to the device through the Interrupt Out endpoint. If no
  Interrupt Out endpoint is declared then Output reports are
  transmitted to a device through the Control endpoint, using
  Set_Report(Output) requests.

Our ukbd(4) driver always uses Set_Report(Output) requests.  Not sure
if that is a bug or not, but there is a footnote in Appendix G that
suggests it isn't:

   If a device declares an Output report then support for
   SetReport(Output) request is required. If an Ouput report is
   defined, declaration of an Interrupt Out endpoint is optional,
   however operating systems that do not support HID Interrupt Out
   endpoints will route all Output reports through the control
   endpoint using a SetReport(Output) request.

Anyway, for HID-over-I2C this SetReport(Output) alternative isn't
implemented it seems.  So instead we're supposed to just write the
Output reports to the register number defined by wOutputRegister in
the HID descriptor.

So I think we should implement a separate function
(ihidev_send_report()?) that does this and remove the WOutputRegister
special handling from I2C_HID_CMD_SET_REPORT.

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