Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices

[Benjamin Tissoires]

On Oct 07 2024, Werner Sembach wrote:
> Hi,
> 
> Am 02.10.24 um 10:31 schrieb Benjamin Tissoires:
> > On Oct 01 2024, Werner Sembach wrote:
> > > Hi Benjamin,
> > > 
> > > Am 01.10.24 um 15:41 schrieb Benjamin Tissoires:
> > > > [...]
> > > > PPS: sorry for pushing that hard on HID-BPF, but I can see that it fits
> > > > all of the requirements here:
> > > > - need to be dynamic
> > > > - still unsure of the userspace implementation, meaning that userspace
> > > >     might do something wrong, which might require kernel changes
> > > Well the reference implementetion for the arduiono macropad from microsoft
> > > ignores the intensity (brightness) channel on rgb leds contrary to the HID
> > > spec, soo yeah you have a point here ...
> > Heh :)
> > 
> > > > - possibility to extend later the kernel API
> > > > - lots of fun :)
> > > You advertise it good ;). More work for me now but maybe less work for me
> > > later, I will look into it.
> > Again, I'm pushing this because I see the benefits and because I can
> > probably reuse the same code on my Corsair and Logitech keyboards. But
> > also, keep in mind that it's not mandatory because you can actually
> > attach the BPF code on top of your existing driver to change the way it
> > behaves. It'll be slightly more complex if you don't let a couple of
> > vendor passthrough reports that we can use to directly talk to the
> > device without any tampering, but that's doable. But if you want to keep
> > the current implementation and have a different layout, this can easily
> > be done in BPF on top.
> > 
> > Cheers,
> > Benjamin
> > 
> > 
> > [0] https://lore.kernel.org/linux-input/20241001-hid-bpf-hid-generic-v3-0-2ef1019468df@xxxxxxxxxx/T/#t
> 
> Thinking about the minimal WMI to HID today, but found a problem: a HID
> feature report is either strictly input or output afaik, but the WMI
> interface has both in some functions.

Not sure you are talking about feature reports, because they are
read/write. It's just that they are synchronous over the USB control
endpoint (on USB).

An input report is strictly directed from the device, and an output
report is from the host to the device.

But a feature report is bidirectional.

> 
> How would I map that?

Depending on the WMI interface, if you want this to be synchronous,
defining a Feature report is correct, otherwise (if you don't need
feedback from WMI), you can declare the commands to WMI as Output
reports.

> 
> If I split everything in input and output the new interface wouldn't
> actually be much smaller.

The HID report descriptor doesn't need to be smaller. The fact that by
default it exposes only one or two LEDs so we don't have the micrometers
arrays is the only purpose.

But if we also implement a not-full HID implementation of LampArray, we
should be able to strip out the parts that we don't care in the LED
class implementation, like the exact positioning, or the multiupdate.

> 
> Also what would I write for the usage for the reserved padding in the report
> descriptor. Usage: 0x00?

padding are ignored by HID. So whatever current usage you have is fine.

However, if you are talking about the custom WMI vendor access, I'd go
with a vendor collection (usage page 0xff00, usage 0x08 for the 8 bytes
long WMI command for instance, 0x10 for the 16 bytes long one).

Side note: in drivers/hid/bpf/progs/hid_report_helpers.h we have some
autogenerated macros to help writing report descriptors (see
drivers/hid/bpf/progs/Huion__Dial-2.bpf.c for an example of usage). It's
in the hid-bpf tree but I think we might be able to include this in
other drivers (or do a minimal rewrite/move into include).
I'm not asking you to use it on your code right now, but this has the
advantage of becoming less "binary blob" in your code, and prevent
mistakes where you edit the comments but not the values.

Cheers,
Benjamin