Hi,
On 07/02/23 17:23, Pavel Machek wrote:
Hi!
2. Create 4 separate multi-color LED sysfs devices for each zone:
/sys/class/leds/hp_omen::kbd_backlight-zone1/
/sys/class/leds/hp_omen::kbd_backlight-zone2/
/sys/class/leds/hp_omen::kbd_backlight-zone3/
/sys/class/leds/hp_omen::kbd_backlight-zone4/
4 separate devices, please. And the naming should be consistent with
the rest, so
:rbg:kbd_backlight-zone1
As covered above previously, we cannot have kbd_backlight in the name as
Upower and several other userspace software which depend on it assume
that /sys/class/leds has just a single file name with the string
"kbd_backlight" in it:
For example, Ubuntu (and most gnome based distros) by default ships with gnome-settings-daemon, which by default attempts to dim the keyboard backlight after a short duration when on the "Low Power" ACPI platform profile. (https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/power/gsd-power-manager.c#L1671). This was currently working as intended with the v2 patch, but if we introduce 4 different files for each zone, this may start dimming only one keyboard zone on low power instead of all 4 of them, which is certainly not intended. There are also multiple projects (mostly gnome extensions) that interact with UPower which might also function incorrectly in other ways. I don't think we should release a feature in the driver which caused unintended consequences like the ones mentioned, especially if the software is popular. What is your opinion on this?
However, as Hans mentioned above, its possible to keep 4 seperate files
and use a name other than kbd_backlight, so that we don't break existing
stuff until the issue is fixed on upower:
/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/
would be closer to something consistent. Should be documented in
Documentation/leds/well-known-leds.txt
. And if you take a look there, you'll notice we already have N900
that has 6 zones with white backlight.
This is interesting as well, it appears the N900 also doesn't have
"kbd_backlight" in the name at all. It instead uses a format like the
following:
/sys/class/leds/lp5523:kb1/
/sys/class/leds/lp5523:kb2/
...
I'm not sure if this is because the N900 driver was made long before we
had the concept of "kbd_backlight" in the name, or because of some other
reason. There are about 9-10 drivers on the kernel which are sticking
with using the "kbd_backlight" convention, so N900 seems to be an
outlier here.
But I'd really like to see plan to go forward. AFAICT there are
keyboards with per-key backlight, and those start to look less like a
set of LEDs and more like a display..
Best regards,
Pavel
Something else I would like to add. I had a look at
include/dt-bindings/leds/common.h, and it defines the following:
/* Standard LED colors */
#define LED_COLOR_ID_WHITE 0
#define LED_COLOR_ID_RED 1
#define LED_COLOR_ID_GREEN 2
#define LED_COLOR_ID_BLUE 3
#define LED_COLOR_ID_AMBER 4
#define LED_COLOR_ID_VIOLET 5
#define LED_COLOR_ID_YELLOW 6
#define LED_COLOR_ID_IR 7
#define LED_COLOR_ID_MULTI 8 /* For multicolor LEDs */
#define LED_COLOR_ID_RGB 9 /* For multicolor LEDs that can do arbitrary
color,
so this would include RGBW and similar */
#define LED_COLOR_ID_PURPLE 10
#define LED_COLOR_ID_ORANGE 11
#define LED_COLOR_ID_PINK 12
#define LED_COLOR_ID_CYAN 13
#define LED_COLOR_ID_LIME 14
#define LED_COLOR_ID_MAX 15
This means that the proposal I had made for supporting intensities such
as zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green
zone_2_blue ... would be invalid as well, and inconsistent with these
definitions. The limit of "15" would also prohibit us from supporting
keyboards in the future which support lighting for every single key, as
we would need way more than 15 indexes to accommodate all of these.
So we are at sort of a conflicted state where none of the standards seem
to correctly "completely" accomodate every single case/scenario of
keyboard backlighting and zones.
Here is yet another approach to handle this, which I feel we should
consider:
We can keep the kbd_backlight file, and additionally have the 4 zones as
separate files, (a total of 5 files) like the following:
1. /sys/class/leds/hp_omen::kbd_backlight
This file controls the global backlight brightness for all 4 zones. It
will have no control for RGB control at this level, this is just sort of
a global switch for the entire backlight. Setting the brightness on this
level will update the brightness for every zone. This file will also
help us maintain support with Upower.
2.
/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/
These will be multi intensity RGBs, each supporting "red green blue"
intensities, and can be used to individually control the brightness of
each zone. Note that these files don't have "kbd_backlight" in the name
for us to not mess with Upower's logic of only having a single keyboard
backlight. This can be documented in
Documentation/leds/well-known-leds.txt for future drivers which plan to
support something similar.
Please let me know if you have any suggestions/comments around this new
way of handling this.
