On 19/02/23 18:50, Hans de Goede wrote:
Hi,
On 2/18/23 12:48, Pavel Machek wrote:
Hi!
I do agree with you that we need to avoid kbd_backlight in the name to avoid causing existing upower code to have weird interactions with this (it supports / assumes there is only 1 kbd_backlight LED class device).
So lets go with just these 4:
/sys/class/leds/hp_omen::kbd_zoned_backlight-1/
/sys/class/leds/hp_omen::kbd_zoned_backlight-2/
/sys/class/leds/hp_omen::kbd_zoned_backlight-3/
/sys/class/leds/hp_omen::kbd_zoned_backlight-4/
Using the _zoned_ between kbd and baclight to avoid confusing the existing upower code. Then once this has landed we can look into extending upower support for this.
Note the requested documentation patch should probably also explain that the _zoned_ was done deliberately to make current upower code ignore the devices.
This makes sense, I agree that the global LED file will cause more confusion
and hacks in the code. I'll start working on the _zoned_ naming scheme with
4 files + documentation changes and make a patch for this soon!
/sys/class/leds/:rgb:kbd_zoned_backlight-4/ is better than what was
suggested above.
Ah yes using rgb for the color part of the name makes sense.
But we already use _1 suffix to deduplicate the, so
I'm not sure this is best naming.
I guess we could try to actually name the zones, something like
(no idea if this are indeed the 4 zones):
:rgb:kbd_zoned_backlight-main
:rgb:kbd_zoned_backlight-wasd
:rgb:kbd_zoned_backlight-cursor
:rgb:kbd_zoned_backlight-numpad
Rishit any comments on this or improvements to it.
Here is an image of how the 4 zones on the keyboard look like
(https://imgur.com/a/iQdRWCM). I think we can call them "left",
"middle", "right", and "wasd":
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-wasd
There are keyboards with per-key backlight. How do you suggest to
solve those?
I really think those fall into a separate category, currently AFAIK
all support for those use /dev/hidraw directly from userspace.
And any kernel API would need to likely be ioctl based, allowing
setting all the LEDs in a single syscall otherwise setting the
LEDs becomes to expensive / introduces to much latency when
doing software driven animations. So I think the best thing to
do there is to declare these out-of-scope for the classic
sysfs based LED class API.
I agree with this as well, a separate API will be required for more
complex use cases with per-key color control. For most future cases
with a limited number of zones, our current approach with multi LED
"kbd_zoned" files should work out.
Regards,
Hans
