Re: API for setting colors of RGB backlit keyboard zones (was [PATCH V3] platform/x86: hp-wmi: Support omen backlight control wmi-acpi methods)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.






[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux