On Tue, Jan 07, 2025 at 01:31:09PM GMT, Mark Kettenis wrote:

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

All right.  Were you thinking about something along those lines?


Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
diff -u -p -u -p -r1.36 ihidev.c
--- sys/dev/i2c/ihidev.c	7 Jan 2025 15:25:18 -0000	1.36
+++ sys/dev/i2c/ihidev.c	7 Jan 2025 17:33:40 -0000
@@ -477,17 +477,8 @@ ihidev_hid_command(struct ihidev_softc *
 
 		cmd[2] = report_id | rreq->type << 4;
 
-		if (rreq->type == I2C_HID_REPORT_TYPE_FEATURE) {
-			cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister)
-			    & 0xff;
-			cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister)
-			    >> 8;
-		} else {
-			cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister)
-			    & 0xff;
-			cmd[dataoff++] = htole16(sc->hid_desc.wOutputRegister)
-			    >> 8;
-		}
+		cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister) & 0xff;
+		cmd[dataoff++] = htole16(sc->hid_desc.wDataRegister) >> 8;
 
 		cmd[dataoff++] = report_len & 0xff;
 		cmd[dataoff++] = report_len >> 8;
@@ -966,4 +957,35 @@ ihidev_set_report(struct device *dev, in
 	}
 
 	return 0;
+}
+
+int
+ihidev_send_report(struct device *dev, int repid, void *data, int data_len)
+{
+	struct ihidev_softc *sc = (struct ihidev_softc *)dev;
+	uint8_t *finalcmd, cmd[5];
+	int cmd_len, report_len, res;
+
+	cmd_len = sizeof(cmd);
+	report_len = 2 + 1 + data_len;
+
+	cmd[0] = htole16(sc->hid_desc.wOutputRegister) & 0xff;
+	cmd[1] = htole16(sc->hid_desc.wOutputRegister) >> 8;
+	cmd[2] = report_len & 0xff;
+	cmd[3] = report_len >> 8;
+	cmd[4] = repid;
+	
+	finalcmd = malloc(cmd_len + data_len, M_DEVBUF, M_NOWAIT | M_ZERO);
+	if (finalcmd == NULL)
+		return ENOMEM;
+
+	memcpy(finalcmd, cmd, cmd_len);
+	memcpy(finalcmd + cmd_len, data, data_len);
+
+	res = iic_exec(sc->sc_tag, I2C_OP_WRITE_WITH_STOP, sc->sc_addr,
+	    finalcmd, cmd_len + data_len, NULL, 0, 0);
+
+	free(finalcmd, M_DEVBUF, cmd_len + data_len);
+
+	return res;
 }
Index: sys/dev/i2c/ihidev.h
===================================================================
RCS file: /cvs/src/sys/dev/i2c/ihidev.h,v
diff -u -p -u -p -r1.10 ihidev.h
--- sys/dev/i2c/ihidev.h	7 Jan 2025 15:25:18 -0000	1.10
+++ sys/dev/i2c/ihidev.h	7 Jan 2025 17:33:40 -0000
@@ -137,5 +137,6 @@ int ihidev_ioctl(struct ihidev *, u_long
 int ihidev_report_type_conv(int);
 int ihidev_set_report(struct device *, int, int, void *, int);
 int ihidev_get_report(struct device *, int, int, void *, int);
+int ihidev_send_report(struct device *, int, void *, int);
 
 void ihidev_poll(void *);
Index: sys/dev/i2c/ikbd.c
===================================================================
RCS file: /cvs/src/sys/dev/i2c/ikbd.c,v
diff -u -p -u -p -r1.2 ikbd.c
--- sys/dev/i2c/ikbd.c	3 Sep 2022 15:48:16 -0000	1.2
+++ sys/dev/i2c/ikbd.c	7 Jan 2025 17:33:40 -0000
@@ -36,6 +36,7 @@
 
 struct ikbd_softc {
 	struct ihidev	sc_hdev;
+#define sc_ledsize	sc_hdev.sc_osize
 	struct hidkbd	sc_kbd;
 	int		sc_spl;
 };
@@ -167,6 +168,14 @@ ikbd_enable(void *v, int on)
 void
 ikbd_set_leds(void *v, int leds)
 {
+	struct ikbd_softc *sc = v;
+	struct hidkbd *kbd = &sc->sc_kbd;
+	uint8_t res;
+
+	if (sc->sc_ledsize && hidkbd_set_leds(kbd, leds, &res) != 0) {
+		ihidev_send_report((struct device *)sc->sc_hdev.sc_parent,
+		    sc->sc_hdev.sc_report_id, &res, 1);
+	}
 }
 
 int
@@ -180,6 +189,9 @@ ikbd_ioctl(void *v, u_long cmd, caddr_t 
 	case WSKBDIO_GTYPE:
 		/* XXX: should we set something else? */
 		*(u_int *)data = WSKBD_TYPE_USB;
+		return 0;
+	case WSKBDIO_SETLEDS:
+		ikbd_set_leds(v, *(int *)data);
 		return 0;
 	default:
 		rc = ihidev_ioctl(&sc->sc_hdev, cmd, data, flag, p);