From: Marcus Glocker <marcus@nazgul.ch>
Subject: uhidev fix for set report output command
To: tech@openbsd.org
Date: Mon, 6 Jan 2025 21:53:25 +0100

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?


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)