[0/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04

Message

Werner Sembach Sept. 26, 2024, 5:44 p.m. UTC

Hi,
took some time but now a first working draft of the suggested new way of
handling per-key RGB keyboard backlights is finished. See:
https://lore.kernel.org/all/[e-mail]/
First time for me sending a whole new driver to the LKML, so please excuse
mistakes I might have made.

Known bugs:
- The device has a lightbar which is currently not implemented and
  therefore stuck to blue once the first backlight control command is send.

What is still missing:
- The leds fallback
- Lightbar control

Some general noob questions:

Initially I though it would be nice to have 2 modules, one jsut being the
wmi initialization and utility stuff and one just being the backlight logic
stuff, being loaded automatically via module_alias, but that would still
require me to create the virtual hid device during the wmi_ab probe, and
that already needs the ll_driver, so i guess I have to do it statically
like i did now?
Or in other words: I would have liked to have a module dependency graph
like this:
    tuxedo_nb04_lamp_array depends on tuxedo_nb04_platform (combining *_wmi_init and *_wmi_utility)
but if i currently split it into modules i would get this:
    tuxedo_nb04_wmi_ab_init dpends on tuxedo_nb04_wmi_ab_lamp_array depends on tuxedo_nb04_wmi_utility

Currently after creating the virtual hdev in the wmi init probe function I
have to keep track of it and manually destroy it during the wmi init
remove. Can this be automated devm_kzalloc-style?

Kind regards,
Werner Sembach

Comments

Benjamin Tissoires Sept. 27, 2024, 4:08 p.m. UTC | #1

On Sep 26 2024, Werner Sembach wrote:
> Hi,
> took some time but now a first working draft of the suggested new way of
> handling per-key RGB keyboard backlights is finished. See:
> https://lore.kernel.org/all/[e-mail]/
> First time for me sending a whole new driver to the LKML, so please excuse
> mistakes I might have made.
> 
> Known bugs:
> - The device has a lightbar which is currently not implemented and
>   therefore stuck to blue once the first backlight control command is send.
> 
> What is still missing:
> - The leds fallback
> - Lightbar control
> 
> Some general noob questions:
> 
> Initially I though it would be nice to have 2 modules, one jsut being the
> wmi initialization and utility stuff and one just being the backlight logic
> stuff, being loaded automatically via module_alias, but that would still
> require me to create the virtual hid device during the wmi_ab probe, and
> that already needs the ll_driver, so i guess I have to do it statically
> like i did now?
> Or in other words: I would have liked to have a module dependency graph
> like this:
>     tuxedo_nb04_lamp_array depends on tuxedo_nb04_platform (combining *_wmi_init and *_wmi_utility)
> but if i currently split it into modules i would get this:
>     tuxedo_nb04_wmi_ab_init dpends on tuxedo_nb04_wmi_ab_lamp_array depends on tuxedo_nb04_wmi_utility

On more general question to you: how much confident are you about your
LampArray implementation?

If you still need to add/fix stuff in it, I would advise you to have a
simple HID device, with bare minimum functionality, and then add the
LampArray functionality on top through HID-BPF. This way you can fix
LampArray out of band with the kernel, while having a more stable kernel
module. This should be possible with v6.11+.

Another solution is to still have your wmi-to-hid module, and then a
HID kernel module in drivers/hid that supports LampArray.

But I would strongly suggest while you are figuring out the userspace
part to stick to HID-BPF, and then once you are happy we can move to a
full kernel module.

Cheers,
Benjamin

> 
> Currently after creating the virtual hdev in the wmi init probe function I
> have to keep track of it and manually destroy it during the wmi init
> remove. Can this be automated devm_kzalloc-style?
> 
> Kind regards,
> Werner Sembach
> 
>

Pavel Machek Sept. 27, 2024, 9:03 p.m. UTC | #2

On Fri 2024-09-27 18:08:52, Benjamin Tissoires wrote:
> On Sep 26 2024, Werner Sembach wrote:
> > Hi,
> > took some time but now a first working draft of the suggested new way of
> > handling per-key RGB keyboard backlights is finished. See:
> > https://lore.kernel.org/all/[e-mail]/
> > First time for me sending a whole new driver to the LKML, so please excuse
> > mistakes I might have made.
> > 
> > Known bugs:
> > - The device has a lightbar which is currently not implemented and
> >   therefore stuck to blue once the first backlight control command is send.
> > 
> > What is still missing:
> > - The leds fallback
> > - Lightbar control
> > 
> > Some general noob questions:
> > 
> > Initially I though it would be nice to have 2 modules, one jsut being the
> > wmi initialization and utility stuff and one just being the backlight logic
> > stuff, being loaded automatically via module_alias, but that would still
> > require me to create the virtual hid device during the wmi_ab probe, and
> > that already needs the ll_driver, so i guess I have to do it statically
> > like i did now?
> > Or in other words: I would have liked to have a module dependency graph
> > like this:
> >     tuxedo_nb04_lamp_array depends on tuxedo_nb04_platform (combining *_wmi_init and *_wmi_utility)
> > but if i currently split it into modules i would get this:
> >     tuxedo_nb04_wmi_ab_init dpends on tuxedo_nb04_wmi_ab_lamp_array depends on tuxedo_nb04_wmi_utility
> 
> On more general question to you: how much confident are you about your
> LampArray implementation?
> 
> If you still need to add/fix stuff in it, I would advise you to have a
> simple HID device, with bare minimum functionality, and then add the
> LampArray functionality on top through HID-BPF. This way you can fix
> LampArray out of band with the kernel, while having a more stable kernel
> module. This should be possible with v6.11+.
> 
> Another solution is to still have your wmi-to-hid module, and then a
> HID kernel module in drivers/hid that supports LampArray.
> 
> But I would strongly suggest while you are figuring out the userspace
> part to stick to HID-BPF, and then once you are happy we can move to a
> full kernel module.

What about creating real kernel driver with real interface to
userland, instead? My preference would be treating this as a display,
but nearly anything is better than _this_.

Best regards,
								Pavel

Werner Sembach Sept. 28, 2024, 7:31 a.m. UTC | #3

Hi Benjamin,

Am 27.09.24 um 18:08 schrieb Benjamin Tissoires:
> On Sep 26 2024, Werner Sembach wrote:
>> Hi,
>> took some time but now a first working draft of the suggested new way of
>> handling per-key RGB keyboard backlights is finished. See:
>> https://lore.kernel.org/all/[e-mail]/
>> First time for me sending a whole new driver to the LKML, so please excuse
>> mistakes I might have made.
>>
>> Known bugs:
>> - The device has a lightbar which is currently not implemented and
>>    therefore stuck to blue once the first backlight control command is send.
>>
>> What is still missing:
>> - The leds fallback
>> - Lightbar control
>>
>> Some general noob questions:
>>
>> Initially I though it would be nice to have 2 modules, one jsut being the
>> wmi initialization and utility stuff and one just being the backlight logic
>> stuff, being loaded automatically via module_alias, but that would still
>> require me to create the virtual hid device during the wmi_ab probe, and
>> that already needs the ll_driver, so i guess I have to do it statically
>> like i did now?
>> Or in other words: I would have liked to have a module dependency graph
>> like this:
>>      tuxedo_nb04_lamp_array depends on tuxedo_nb04_platform (combining *_wmi_init and *_wmi_utility)
>> but if i currently split it into modules i would get this:
>>      tuxedo_nb04_wmi_ab_init dpends on tuxedo_nb04_wmi_ab_lamp_array depends on tuxedo_nb04_wmi_utility
> On more general question to you: how much confident are you about your
> LampArray implementation?
>
> If you still need to add/fix stuff in it, I would advise you to have a
> simple HID device, with bare minimum functionality, and then add the
> LampArray functionality on top through HID-BPF. This way you can fix
> LampArray out of band with the kernel, while having a more stable kernel
> module. This should be possible with v6.11+.
>
> Another solution is to still have your wmi-to-hid module, and then a
> HID kernel module in drivers/hid that supports LampArray.
>
> But I would strongly suggest while you are figuring out the userspace
> part to stick to HID-BPF, and then once you are happy we can move to a
> full kernel module.

I don't expect this patch to get merged right away, but like i wrote, 
wanted to collect some feedback on it to already start refining it.

With this driver now functional I have something to build and test 
userspace against while waiting on the feedback and the undoubtly 
following discussion of details to get it right ^^.

Until now I only tested with a very simple, self built command line 
binary, looping some patterns. My next step is to try the work in 
progress implementetion for LampArray in OpenRGB: 
https://gitlab.com/CalcProgrammer1/OpenRGB/-/merge_requests/2348

Regards,

Werner

>
> Cheers,
> Benjamin
>
>> Currently after creating the virtual hdev in the wmi init probe function I
>> have to keep track of it and manually destroy it during the wmi init
>> remove. Can this be automated devm_kzalloc-style?
>>
>> Kind regards,
>> Werner Sembach
>>
>>