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

[Werner Sembach]

Am 08.10.24 um 11:53 schrieb 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).

I'm confused about the split between get and send feature reports 
https://www.kernel.org/doc/html/latest/hid/hidraw.html

I guess then a get feature report can also carry input data and the difference 
is that a send feature report doesn't wait for a reply? but then what is it's 
reason of existence in contrast to an output report?

> 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.
Thanks for reminding me that output reports exist xD.
> > 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.

I will look into it.

Since the interface is fixed I don't need to flesh out the whole descriptor 
(which i thought i must do) and usage page (0xff42, because NB04 and the wmi has 
2 other ec controlling wmi interfaces besides the AB one), report usage 
(matching the wmi comand id's) and report size should be enough.

Regards,

Werner

> Cheers,
> Benjamin