Re: [PATCH V3] platform/x86: hp-wmi: Support omen backlight control wmi-acpi methods

Re: [PATCH V3] platform/x86: hp-wmi: Support omen backlight control wmi-acpi methods

[Hans de Goede]

Hi,

On 2/1/23 14:59, Rishit Bansal wrote:
> Hi Hans,
> 
> On 01/02/23 13:47, Hans de Goede wrote:
>> Hi Rishit,
>>
>> On 2/1/23 00:50, Rishit Bansal wrote:
>>> The HP Omen Command Studio application includes a Light Studio feature
>>> which can be used to control various features related to the keyboard
>>> backlight via the 0x20009 command.
>>>
>>> The command supports the following queries:
>>>
>>> - 0x1: Checks if keyboard lighting is supported
>>> - 0x2: Get the zone colors of each of the 4 zones on the keyboard
>>> - 0x3: Set the zone colors of each of the 4 zones on the keyboard
>>> - 0x4: Gets the state (on/off) of the backlight
>>> - 0x5: Sets the state (on/off) of the backlight
>>>
>>> This patch introduces a new sysfs led class called
>>> "hp_omen::kbd_backlight" which can be used to control the state of the
>>> backlight. It also includes a sysfs RW attribute at the following
>>> location:
>>>
>>> /sys/class/leds/hp_omen::kbd_backlight/zone_colors
>>>
>>> This file contains the color codes for each of the 4 zones of the
>>> keyboard. Each zone's color is represented by R,G and B components, each
>>> of which take a byte. Therefore, the total size of this file is always:
>>>
>>> 4 (zones) * 3 (components R,G,B) = 12 bytes
>>>
>>> An example output from this file is:
>>>
>>> $ xxd /sys/class/leds/hp_omen\:\:kbd_backlight/zone_colors
>>> 00000000: 01ff 00ff 01ff ffff 01ff 0101            ............
>>>
>>> The above output means that each zone has the following hex
>>> color codes:
>>> Zone 1: #01ff00
>>> Zone 2: #ff01ff
>>> Zone 3: #ffff01
>>> Zone 4: #ff0101
>>>
>>> Colors can be set on the backlight by writing back to this file by
>>> passing 12 bytes in the exact same format as above.
>>>
>>> Additionally this patch also maps the backlight event to the KEY_KBDILLUMTOGGLE
>>> key so it shows the correct notification on userspace.
>>>
>>> The patch has been tested on an HP Omen 15-en0037AX (AMD) laptop.
>>>
>>> Signed-off-by: Rishit Bansal <rishitbansal0@gmail.com>
>>> ---
>>> Changes since v1:
>>>   - Map backlight key to KEY_KBDILLUMTOGGLE
>>>
>>> Changes since v2:
>>>   - Changes all str operations to memcpy() to handle null bytes edge
>>>     cases
>>>   - Renamed kbd_rgb to zone_colors, and moved it to inside the
>>>     kbd_backlight directory
>>>   - Added documentation for the zone_colors file
>>>   - Removed KEY_KBDILLUMTOGGLE from the parse-map, and instead emitted
>>>     directly
>>>   - Remove logic to unregister from devm
>>>   - Moved a few constants to #define
>>>   - Updated path description with more details on zone_colors file format
>>> ---
>>>   .../ABI/testing/sysfs-platform-hp-wmi         |  33 +++++
>>>   drivers/platform/x86/hp/hp-wmi.c              | 116 ++++++++++++++++++
>>>   2 files changed, 149 insertions(+)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-platform-hp-wmi
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-platform-hp-wmi b/Documentation/ABI/testing/sysfs-platform-hp-wmi
>>> new file mode 100644
>>> index 000000000000..ccf2d29185ee
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-platform-hp-wmi
>>> @@ -0,0 +1,33 @@
>>> +What:        /sys/class/leds/hp_omen::kbd_backlight/zone_colors
>>> +Date:        Feb 2023
>>> +KernelVersion:    6.2
>>> +Contact:    Rishit Bansal <rishitbansal0@gmail.com>
>>> +Description:
>>> +        This file stores the RGB color codes for each of
>>> +        the 4 zones of the backlight on HP omen keyboards.
>>> +
>>> +        Each zone takes R,G,B values. The R,G,B values each can
>>> +        range from 0-255. This means the whole state of the colors
>>> +        can be represented in 12 bytes:
>>> +
>>> +        (4 zones * 3 color components (R,G,B) * 1 byte = 12 bytes)
>>> +
>>> +        Here is an example where we read the file:
>>> +
>>> +            xxd /sys/class/leds/hp_omen\:\:kbd_backlight/zone_colors
>>> +            00000000: 01ff 00ff 01ff ffff 01ff 0101            ............
>>> +
>>> +        The above output means that each zone has the following hex
>>> +        color codes:
>>> +        Zone 1: #01ff00
>>> +        Zone 2: #ff01ff
>>> +        Zone 3: #ffff01
>>> +        Zone 4: #ff0101
>>> +
>>> +        The colors of the each of the zones can be set by writing
>>> +        the same format to this file. For example to set all zones
>>> +        to white, we would do:
>>> +
>>> +            echo -n -e '\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff' | sudo tee /sys/class/leds/hp_omen\:\:kbd_backlight/zone_colors
>> Thank you for the new version and thank you for writing this doc, that is
>> not only helpful for users but also for the review.
>>
>> Looking at the above I think what you should do is create not 1 but 4
>> sysfs files like this:
>>
>> /sys/class/leds/hp_omen::kbd_backlight/zone1_colors
>> /sys/class/leds/hp_omen::kbd_backlight/zone2_colors
>> /sys/class/leds/hp_omen::kbd_backlight/zone3_colors
>> /sys/class/leds/hp_omen::kbd_backlight/zone4_colors
>>
>> And then a user could do e.g.:
>>
>> [hans@shalem ~]$ cat /sys/class/leds/hp_omen::kbd_backlight/zone1_colors
>> #01ff00
>> [hans@shalem ~]$
>>
>> And e.g.:
>>
>> [hans@shalem ~]$ echo #ff0000 > /sys/class/leds/hp_omen::kbd_backlight/zone1_colors
>>
>> This will make it much easier for users to use and generally
>> speaking we try to avoid putting binary files in sysfs.
>>
>> You can take a look at drivers/hid/hid-lg-g15.c and then the
>> color_store() function for an existing example of parsing
>> rgb colors in the form of #rrggbb.
>>
>> Also if you look at lg_g510_kbd_led_write() you see there that
>> that driver actually emulates a brightness range of 0-255 for
> 
> 
> I didn't think of the idea before to scale the RGB values based on the brightness range of (0-255), that's really interesting! Did a small test on my end, this does seem to function correctly, looks great. I'll include this feature in my follow up patch.
> 
> 
>> /sys/class/leds/hp_omen::kbd_backlight/brightness
>>
>> by scaling the user requested zone values by the brightness
>> value, giving a bigger brightness range in a standard
>> sysfs interface which is e.g. supported by upower and by
>> some desktop environments using upower, so that even
>> without knowing how to control the specific zones users
>> can still control at least the brightness.
>>
>> So I think that what you want to do is add:
>>
>> struct hp_omen_kbd_led {
>>     struct led_classdev cdev;
>>     u8 red[4];
>>     u8 green[4];
>>     u8 blue[4];
>>     enum led_brightness brightness;
>> };
>>
>> struct hp_omen_kbd_led omen_kbd_led;
>>
>> And then have 4 zone sysfs files which fill the red, green and blue
>> arrays (and also fill these with initial values at probe) and
>> then have an omen_kbd_led_update_zones() function which creates
>> the 12 bytes you need to send by for each zone calculating the
>> values similar to this lg_g510_kbd_led_write() code:
>>
>>          g15->transfer_buf[1] =
>>                  DIV_ROUND_CLOSEST(g15_led->red * brightness, 255);
>>          g15->transfer_buf[2] =
>>                  DIV_ROUND_CLOSEST(g15_led->green * brightness, 255);
>>          g15->transfer_buf[3] =
>>                  DIV_ROUND_CLOSEST(g15_led->blue * brightness, 255);
>>
>> And then on store of a zone, you update the red, green, blue values
>> for that zone and call omen_kbd_led_update_zones()
>>
>> and from set_omen_backlight_brightness() you then:
>>
>> 1. Store the brightness
>> 2. Do the on/off setting of the backlight as done already
>> 3. Call omen_kbd_led_update_zones() to update the zones for
>>     the brightness change
>>
>> I believe that this will give a much nicer user experience
>> then the current binary file which sets all 4 zones at once
>> approach.
>>
>> Regards,
>>
>> Hans
> 
> 
> The format with the hex color codes is definitely more user friendly. Just a small note, there is a side effect with having 4 different zone files: With the current format, it is possible to set all the colors of each zone using a single WMI method call, but with 4 different files, setting all the zones may be slightly less performant as now we'll be making 4 different WMI method calls (one for setting each zone). For userspace software which may rapidly set the colors of each zones to simulate certain effects, this would lead to an increase in the number of calls we make, and also cause possible delays. (Though from my testing, it seems the delays are negligible for most cases). Do you think it may be better to have a single zone file, with 4 hex codes instead, like the following:
> 
> $ cat /sys/class/leds/hp_omen::kbd_backlight/zone_colors
> #01ff00
> #01ff00
> #01ff00
> #01ff00
> 
> This would help us prevent the performance penalty and have it as a single WMI call. What are your thoughts on this?

I have been thinking a bit about this and I still think that having separate per-zone
files would be better. You can speedup things about 2x by only doing the call to read
the buffer once and cache the result. At least assuming the non kbd zone related bits
of the buffer never change (which should be easy enough to check).

Actually my "thinking about this" includes a new alternate proposal. Rather then
making up our own userspace API, as I did for the logitech 510 USB keyboard
new support for multi-color backlights really should use the new standardized
multi-color LED API:

https://www.kernel.org/doc/html/latest/leds/leds-class-multicolor.html
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led-multicolor
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/includ

I have been thinking about how to use this with a 4 zone keyboard and I believe
that the best way to do that is to:


1. Forget about the global on/off control, individual zones can be turned off by
setting the brightness of the zone to 0.

This does require the driver to at least turn on the global control once, or
you could:

1a) cache the global control value
1b) on zone changes check if all zones are off, if they are all off, use the
    global control to turn everything off, else turn the global control on;
    and only make the actual WMI call for this if the global control state
    changes vs the last cached value

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/

This way we are using standard existing userspace APIs rather then inventing
new userspace API which IMHO is a much better approach.

Note this is just a suggestion, if you disagree (and can motivate
why you think this is a bad idea) please do speak up about this.

And please let me know if you need any help with converting the code
to the ed-class-multicolor inetnal kernel APIs there are not that
much users yet, so I have been unable to find a good example to
point you to.

A downside of this is that it lacks e.g. support in upower. But the
kbd_backlight code in upower needs work anyways. E.g. upower does not
work with backlit USB keyboards if these are plugged in after boot,
or unplugged + re-plugged after boot. So someone really needs to
spend some time to improve the upower keyboard backlight code anyways.

Regards,

Hans






    

>>> +
>>> +
>>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>>> index 0a99058be813..f86cb7feaad4 100644
>>> --- a/drivers/platform/x86/hp/hp-wmi.c
>>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>>> @@ -27,6 +27,7 @@
>>>   #include <linux/rfkill.h>
>>>   #include <linux/string.h>
>>>   #include <linux/dmi.h>
>>> +#include <linux/leds.h>
>>>     MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
>>>   MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
>>> @@ -136,6 +137,7 @@ enum hp_wmi_command {
>>>       HPWMI_WRITE    = 0x02,
>>>       HPWMI_ODM    = 0x03,
>>>       HPWMI_GM    = 0x20008,
>>> +    HPWMI_KB    = 0x20009,
>>>   };
>>>     enum hp_wmi_hardware_mask {
>>> @@ -254,6 +256,9 @@ static const char * const tablet_chassis_types[] = {
>>>     #define DEVICE_MODE_TABLET    0x06
>>>   +#define OMEN_ZONE_COLOR_OFFSET 0x19
>>> +#define OMEN_ZONE_COLOR_LEN 0x0c
>>> +
>>>   /* map output size to the corresponding WMI method id */
>>>   static inline int encode_outsize_for_pvsz(int outsize)
>>>   {
>>> @@ -734,12 +739,56 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>>       return count;
>>>   }
>>>   +static ssize_t zone_colors_show(struct device *dev,
>>> +                    struct device_attribute *attr, char *buf)
>>> +{
>>> +    u8 val[128];
>>> +
>>> +    int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>> +                       zero_if_sup(val), sizeof(val));
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    memcpy(buf, &val[OMEN_ZONE_COLOR_OFFSET], OMEN_ZONE_COLOR_LEN);
>>> +
>>> +    return OMEN_ZONE_COLOR_LEN;
>>> +}
>>> +
>>> +static ssize_t zone_colors_store(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     const char *buf, size_t count)
>>> +{
>>> +    u8 val[128];
>>> +    int ret;
>>> +
>>> +    ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>> +                   zero_if_sup(val), sizeof(val));
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (count != OMEN_ZONE_COLOR_LEN)
>>> +        return -1;
>>> +
>>> +    memcpy(&val[OMEN_ZONE_COLOR_OFFSET], buf, count);
>>> +
>>> +    ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_KB, &val, sizeof(val),
>>> +                   0);
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return OMEN_ZONE_COLOR_LEN;
>>> +}
>>> +
>>>   static DEVICE_ATTR_RO(display);
>>>   static DEVICE_ATTR_RO(hddtemp);
>>>   static DEVICE_ATTR_RW(als);
>>>   static DEVICE_ATTR_RO(dock);
>>>   static DEVICE_ATTR_RO(tablet);
>>>   static DEVICE_ATTR_RW(postcode);
>>> +static DEVICE_ATTR_RW(zone_colors);
>>>     static struct attribute *hp_wmi_attrs[] = {
>>>       &dev_attr_display.attr,
>>> @@ -752,6 +801,12 @@ static struct attribute *hp_wmi_attrs[] = {
>>>   };
>>>   ATTRIBUTE_GROUPS(hp_wmi);
>>>   +static struct attribute *omen_kbd_led_attrs[] = {
>>> +    &dev_attr_zone_colors.attr,
>>> +    NULL,
>>> +};
>>> +ATTRIBUTE_GROUPS(omen_kbd_led);
>>> +
>>>   static void hp_wmi_notify(u32 value, void *context)
>>>   {
>>>       struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>> @@ -853,6 +908,10 @@ static void hp_wmi_notify(u32 value, void *context)
>>>       case HPWMI_PROXIMITY_SENSOR:
>>>           break;
>>>       case HPWMI_BACKLIT_KB_BRIGHTNESS:
>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
>>> +        input_sync(hp_wmi_input_dev);
>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
>>> +        input_sync(hp_wmi_input_dev);
>>>           break;
>>>       case HPWMI_PEAKSHIFT_PERIOD:
>>>           break;
>>> @@ -1294,6 +1353,60 @@ static int thermal_profile_setup(void)
>>>     static int hp_wmi_hwmon_init(void);
>>>   +static enum led_brightness get_omen_backlight_brightness(struct led_classdev *cdev)
>>> +{
>>> +    u8 val;
>>> +
>>> +    int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>> +
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return (val & 0x80) ? LED_ON : LED_OFF;
>>> +}
>>> +
>>> +static void set_omen_backlight_brightness(struct led_classdev *cdev, enum led_brightness value)
>>> +{
>>> +    char buffer[4] = { (value == LED_OFF) ? 0x64 : 0xe4, 0, 0, 0 };
>>> +
>>> +    hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_KB, &buffer,
>>> +                       sizeof(buffer), 0);
>>> +}
>>> +
>>> +static struct led_classdev omen_kbd_led = {
>>> +    .name = "hp_omen::kbd_backlight",
>>> +    .brightness_set = set_omen_backlight_brightness,
>>> +    .brightness_get = get_omen_backlight_brightness,
>>> +    .max_brightness = 1,
>>> +    .groups = omen_kbd_led_groups,
>>> +};
>>> +
>>> +static bool is_omen_lighting_supported(void)
>>> +{
>>> +    u8 val;
>>> +
>>> +    int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>> +
>>> +    if (ret)
>>> +        return false;
>>> +
>>> +    return (val & 1) == 1;
>>> +}
>>> +
>>> +static int omen_backlight_init(struct device *dev)
>>> +{
>>> +    int ret;
>>> +
>>> +    input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE);
>>> +
>>> +    ret = devm_led_classdev_register(dev, &omen_kbd_led);
>>> +
>>> +    if (ret < 0)
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>   {
>>>       int err;
>>> @@ -1321,6 +1434,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>         thermal_profile_setup();
>>>   +    if (is_omen_lighting_supported())
>>> +        omen_backlight_init(&device->dev);
>>> +
>>>       return 0;
>>>   }
>>>   
> 

Re: [PATCH V3] platform/x86: hp-wmi: Support omen backlight control wmi-acpi methods

[Rishit Bansal]

Hi,

On 02/02/23 18:13, Hans de Goede wrote:
> Hi,
>
> On 2/1/23 14:59, Rishit Bansal wrote:
>> Hi Hans,
>>
>> On 01/02/23 13:47, Hans de Goede wrote:
>>> Hi Rishit,
>>>
>>> On 2/1/23 00:50, Rishit Bansal wrote:
>>>> The HP Omen Command Studio application includes a Light Studio feature
>>>> which can be used to control various features related to the keyboard
>>>> backlight via the 0x20009 command.
>>>>
>>>> The command supports the following queries:
>>>>
>>>> - 0x1: Checks if keyboard lighting is supported
>>>> - 0x2: Get the zone colors of each of the 4 zones on the keyboard
>>>> - 0x3: Set the zone colors of each of the 4 zones on the keyboard
>>>> - 0x4: Gets the state (on/off) of the backlight
>>>> - 0x5: Sets the state (on/off) of the backlight
>>>>
>>>> This patch introduces a new sysfs led class called
>>>> "hp_omen::kbd_backlight" which can be used to control the state of the
>>>> backlight. It also includes a sysfs RW attribute at the following
>>>> location:
>>>>
>>>> /sys/class/leds/hp_omen::kbd_backlight/zone_colors
>>>>
>>>> This file contains the color codes for each of the 4 zones of the
>>>> keyboard. Each zone's color is represented by R,G and B components, each
>>>> of which take a byte. Therefore, the total size of this file is always:
>>>>
>>>> 4 (zones) * 3 (components R,G,B) = 12 bytes
>>>>
>>>> An example output from this file is:
>>>>
>>>> $ xxd /sys/class/leds/hp_omen\:\:kbd_backlight/zone_colors
>>>> 00000000: 01ff 00ff 01ff ffff 01ff 0101            ............
>>>>
>>>> The above output means that each zone has the following hex
>>>> color codes:
>>>> Zone 1: #01ff00
>>>> Zone 2: #ff01ff
>>>> Zone 3: #ffff01
>>>> Zone 4: #ff0101
>>>>
>>>> Colors can be set on the backlight by writing back to this file by
>>>> passing 12 bytes in the exact same format as above.
>>>>
>>>> Additionally this patch also maps the backlight event to the KEY_KBDILLUMTOGGLE
>>>> key so it shows the correct notification on userspace.
>>>>
>>>> The patch has been tested on an HP Omen 15-en0037AX (AMD) laptop.
>>>>
>>>> Signed-off-by: Rishit Bansal <rishitbansal0@gmail.com>
>>>> ---
>>>> Changes since v1:
>>>>    - Map backlight key to KEY_KBDILLUMTOGGLE
>>>>
>>>> Changes since v2:
>>>>    - Changes all str operations to memcpy() to handle null bytes edge
>>>>      cases
>>>>    - Renamed kbd_rgb to zone_colors, and moved it to inside the
>>>>      kbd_backlight directory
>>>>    - Added documentation for the zone_colors file
>>>>    - Removed KEY_KBDILLUMTOGGLE from the parse-map, and instead emitted
>>>>      directly
>>>>    - Remove logic to unregister from devm
>>>>    - Moved a few constants to #define
>>>>    - Updated path description with more details on zone_colors file format
>>>> ---
>>>>    .../ABI/testing/sysfs-platform-hp-wmi         |  33 +++++
>>>>    drivers/platform/x86/hp/hp-wmi.c              | 116 ++++++++++++++++++
>>>>    2 files changed, 149 insertions(+)
>>>>    create mode 100644 Documentation/ABI/testing/sysfs-platform-hp-wmi
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-platform-hp-wmi b/Documentation/ABI/testing/sysfs-platform-hp-wmi
>>>> new file mode 100644
>>>> index 000000000000..ccf2d29185ee
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-platform-hp-wmi
>>>> @@ -0,0 +1,33 @@
>>>> +What:        /sys/class/leds/hp_omen::kbd_backlight/zone_colors
>>>> +Date:        Feb 2023
>>>> +KernelVersion:    6.2
>>>> +Contact:    Rishit Bansal <rishitbansal0@gmail.com>
>>>> +Description:
>>>> +        This file stores the RGB color codes for each of
>>>> +        the 4 zones of the backlight on HP omen keyboards.
>>>> +
>>>> +        Each zone takes R,G,B values. The R,G,B values each can
>>>> +        range from 0-255. This means the whole state of the colors
>>>> +        can be represented in 12 bytes:
>>>> +
>>>> +        (4 zones * 3 color components (R,G,B) * 1 byte = 12 bytes)
>>>> +
>>>> +        Here is an example where we read the file:
>>>> +
>>>> +            xxd /sys/class/leds/hp_omen\:\:kbd_backlight/zone_colors
>>>> +            00000000: 01ff 00ff 01ff ffff 01ff 0101            ............
>>>> +
>>>> +        The above output means that each zone has the following hex
>>>> +        color codes:
>>>> +        Zone 1: #01ff00
>>>> +        Zone 2: #ff01ff
>>>> +        Zone 3: #ffff01
>>>> +        Zone 4: #ff0101
>>>> +
>>>> +        The colors of the each of the zones can be set by writing
>>>> +        the same format to this file. For example to set all zones
>>>> +        to white, we would do:
>>>> +
>>>> +            echo -n -e '\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff' | sudo tee /sys/class/leds/hp_omen\:\:kbd_backlight/zone_colors
>>> Thank you for the new version and thank you for writing this doc, that is
>>> not only helpful for users but also for the review.
>>>
>>> Looking at the above I think what you should do is create not 1 but 4
>>> sysfs files like this:
>>>
>>> /sys/class/leds/hp_omen::kbd_backlight/zone1_colors
>>> /sys/class/leds/hp_omen::kbd_backlight/zone2_colors
>>> /sys/class/leds/hp_omen::kbd_backlight/zone3_colors
>>> /sys/class/leds/hp_omen::kbd_backlight/zone4_colors
>>>
>>> And then a user could do e.g.:
>>>
>>> [hans@shalem ~]$ cat /sys/class/leds/hp_omen::kbd_backlight/zone1_colors
>>> #01ff00
>>> [hans@shalem ~]$
>>>
>>> And e.g.:
>>>
>>> [hans@shalem ~]$ echo #ff0000 > /sys/class/leds/hp_omen::kbd_backlight/zone1_colors
>>>
>>> This will make it much easier for users to use and generally
>>> speaking we try to avoid putting binary files in sysfs.
>>>
>>> You can take a look at drivers/hid/hid-lg-g15.c and then the
>>> color_store() function for an existing example of parsing
>>> rgb colors in the form of #rrggbb.
>>>
>>> Also if you look at lg_g510_kbd_led_write() you see there that
>>> that driver actually emulates a brightness range of 0-255 for
>>
>> I didn't think of the idea before to scale the RGB values based on the brightness range of (0-255), that's really interesting! Did a small test on my end, this does seem to function correctly, looks great. I'll include this feature in my follow up patch.
>>
>>
>>> /sys/class/leds/hp_omen::kbd_backlight/brightness
>>>
>>> by scaling the user requested zone values by the brightness
>>> value, giving a bigger brightness range in a standard
>>> sysfs interface which is e.g. supported by upower and by
>>> some desktop environments using upower, so that even
>>> without knowing how to control the specific zones users
>>> can still control at least the brightness.
>>>
>>> So I think that what you want to do is add:
>>>
>>> struct hp_omen_kbd_led {
>>>      struct led_classdev cdev;
>>>      u8 red[4];
>>>      u8 green[4];
>>>      u8 blue[4];
>>>      enum led_brightness brightness;
>>> };
>>>
>>> struct hp_omen_kbd_led omen_kbd_led;
>>>
>>> And then have 4 zone sysfs files which fill the red, green and blue
>>> arrays (and also fill these with initial values at probe) and
>>> then have an omen_kbd_led_update_zones() function which creates
>>> the 12 bytes you need to send by for each zone calculating the
>>> values similar to this lg_g510_kbd_led_write() code:
>>>
>>>           g15->transfer_buf[1] =
>>>                   DIV_ROUND_CLOSEST(g15_led->red * brightness, 255);
>>>           g15->transfer_buf[2] =
>>>                   DIV_ROUND_CLOSEST(g15_led->green * brightness, 255);
>>>           g15->transfer_buf[3] =
>>>                   DIV_ROUND_CLOSEST(g15_led->blue * brightness, 255);
>>>
>>> And then on store of a zone, you update the red, green, blue values
>>> for that zone and call omen_kbd_led_update_zones()
>>>
>>> and from set_omen_backlight_brightness() you then:
>>>
>>> 1. Store the brightness
>>> 2. Do the on/off setting of the backlight as done already
>>> 3. Call omen_kbd_led_update_zones() to update the zones for
>>>      the brightness change
>>>
>>> I believe that this will give a much nicer user experience
>>> then the current binary file which sets all 4 zones at once
>>> approach.
>>>
>>> Regards,
>>>
>>> Hans
>>
>> The format with the hex color codes is definitely more user friendly. Just a small note, there is a side effect with having 4 different zone files: With the current format, it is possible to set all the colors of each zone using a single WMI method call, but with 4 different files, setting all the zones may be slightly less performant as now we'll be making 4 different WMI method calls (one for setting each zone). For userspace software which may rapidly set the colors of each zones to simulate certain effects, this would lead to an increase in the number of calls we make, and also cause possible delays. (Though from my testing, it seems the delays are negligible for most cases). Do you think it may be better to have a single zone file, with 4 hex codes instead, like the following:
>>
>> $ cat /sys/class/leds/hp_omen::kbd_backlight/zone_colors
>> #01ff00
>> #01ff00
>> #01ff00
>> #01ff00
>>
>> This would help us prevent the performance penalty and have it as a single WMI call. What are your thoughts on this?
> I have been thinking a bit about this and I still think that having separate per-zone
> files would be better. You can speedup things about 2x by only doing the call to read
> the buffer once and cache the result. At least assuming the non kbd zone related bits
> of the buffer never change (which should be easy enough to check).
>
> Actually my "thinking about this" includes a new alternate proposal. Rather then
> making up our own userspace API, as I did for the logitech 510 USB keyboard
> new support for multi-color backlights really should use the new standardized
> multi-color LED API:
>
> https://www.kernel.org/doc/html/latest/leds/leds-class-multicolor.html
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led-multicolor
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/includ
>
> I have been thinking about how to use this with a 4 zone keyboard and I believe
> that the best way to do that is to:
>
>
> 1. Forget about the global on/off control, individual zones can be turned off by
> setting the brightness of the zone to 0.
>
> This does require the driver to at least turn on the global control once, or
> you could:
>
> 1a) cache the global control value
> 1b) on zone changes check if all zones are off, if they are all off, use the
>      global control to turn everything off, else turn the global control on;
>      and only make the actual WMI call for this if the global control state
>      changes vs the last cached value
>
> 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/
>
> This way we are using standard existing userspace APIs rather then inventing
> new userspace API which IMHO is a much better approach.
>
> Note this is just a suggestion, if you disagree (and can motivate
> why you think this is a bad idea) please do speak up about this.
>
> And please let me know if you need any help with converting the code
> to the ed-class-multicolor inetnal kernel APIs there are not that
> much users yet, so I have been unable to find a good example to
> point you to.
>
> A downside of this is that it lacks e.g. support in upower. But the
> kbd_backlight code in upower needs work anyways. E.g. upower does not
> work with backlit USB keyboards if these are plugged in after boot,
> or unplugged + re-plugged after boot. So someone really needs to
> spend some time to improve the upower keyboard backlight code anyways.
>
> Regards,
>
> Hans
>

I agree the multi-color class is the correct thing to use here, but I am 
not completely sure if we should have multiple files in /sys/class/leds 
with the string "kbd_backlight" in them. UPower seems to take the first 
occurence of kbd_backlight and assume that's the keyboard backlight 
(https://github.com/freedesktop/upower/blob/0e256ece04a98d3d202ed9640d33c56aaaeda137/src/up-kbd-backlight.c#L263-L269). 
I completely agree that this implementation needs more work on it, but 
it may have unintended consequences with software that uses UPower's 
kbd_backlight to control the keyboard.

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?


One alternative I can think of to have the "best of both worlds" 
(maintain support with Upower, and conform with the muti-color led 
specification), is to use the multi-color led class, and put all the 
indexes/brightness under one file. (Please correct me if the multi led 
specification does not allow this, but I don't see any limitation for 
having indexes other then just "red", "green" and "blue"):


$ cat /sys/class/leds/hp_omen::kbd_backlight/multi_index

zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue 
zone_3_red zone_3_green zone_3_blue zone_4_red zone_4_green zone_4_blue


And we can set it accordingly by doing:

$ echo 255 255 255 255 255 255 255 255 255 255 255 255 > 
/sys/class/leds/hp_omen::kbd_backlight/multi_intensity


And then I can use "led_mc_calc_color_components" when the brightness is 
changed to directly compute the brightness of each index value and pass 
it to the keyboard through the WMI method.


I know this suggestion goes back to us putting the all zones under a 
single file (sort of, we are atleast a bit closer to atleast following a 
spec now), but what are your thoughts on doing it this way with 
multi_index instead?



>
>
>
>
>      
>
>>>> +
>>>> +
>>>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>>>> index 0a99058be813..f86cb7feaad4 100644
>>>> --- a/drivers/platform/x86/hp/hp-wmi.c
>>>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>>>> @@ -27,6 +27,7 @@
>>>>    #include <linux/rfkill.h>
>>>>    #include <linux/string.h>
>>>>    #include <linux/dmi.h>
>>>> +#include <linux/leds.h>
>>>>      MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
>>>>    MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
>>>> @@ -136,6 +137,7 @@ enum hp_wmi_command {
>>>>        HPWMI_WRITE    = 0x02,
>>>>        HPWMI_ODM    = 0x03,
>>>>        HPWMI_GM    = 0x20008,
>>>> +    HPWMI_KB    = 0x20009,
>>>>    };
>>>>      enum hp_wmi_hardware_mask {
>>>> @@ -254,6 +256,9 @@ static const char * const tablet_chassis_types[] = {
>>>>      #define DEVICE_MODE_TABLET    0x06
>>>>    +#define OMEN_ZONE_COLOR_OFFSET 0x19
>>>> +#define OMEN_ZONE_COLOR_LEN 0x0c
>>>> +
>>>>    /* map output size to the corresponding WMI method id */
>>>>    static inline int encode_outsize_for_pvsz(int outsize)
>>>>    {
>>>> @@ -734,12 +739,56 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>>>        return count;
>>>>    }
>>>>    +static ssize_t zone_colors_show(struct device *dev,
>>>> +                    struct device_attribute *attr, char *buf)
>>>> +{
>>>> +    u8 val[128];
>>>> +
>>>> +    int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>>> +                       zero_if_sup(val), sizeof(val));
>>>> +
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    memcpy(buf, &val[OMEN_ZONE_COLOR_OFFSET], OMEN_ZONE_COLOR_LEN);
>>>> +
>>>> +    return OMEN_ZONE_COLOR_LEN;
>>>> +}
>>>> +
>>>> +static ssize_t zone_colors_store(struct device *dev,
>>>> +                     struct device_attribute *attr,
>>>> +                     const char *buf, size_t count)
>>>> +{
>>>> +    u8 val[128];
>>>> +    int ret;
>>>> +
>>>> +    ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>>> +                   zero_if_sup(val), sizeof(val));
>>>> +
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    if (count != OMEN_ZONE_COLOR_LEN)
>>>> +        return -1;
>>>> +
>>>> +    memcpy(&val[OMEN_ZONE_COLOR_OFFSET], buf, count);
>>>> +
>>>> +    ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_KB, &val, sizeof(val),
>>>> +                   0);
>>>> +
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    return OMEN_ZONE_COLOR_LEN;
>>>> +}
>>>> +
>>>>    static DEVICE_ATTR_RO(display);
>>>>    static DEVICE_ATTR_RO(hddtemp);
>>>>    static DEVICE_ATTR_RW(als);
>>>>    static DEVICE_ATTR_RO(dock);
>>>>    static DEVICE_ATTR_RO(tablet);
>>>>    static DEVICE_ATTR_RW(postcode);
>>>> +static DEVICE_ATTR_RW(zone_colors);
>>>>      static struct attribute *hp_wmi_attrs[] = {
>>>>        &dev_attr_display.attr,
>>>> @@ -752,6 +801,12 @@ static struct attribute *hp_wmi_attrs[] = {
>>>>    };
>>>>    ATTRIBUTE_GROUPS(hp_wmi);
>>>>    +static struct attribute *omen_kbd_led_attrs[] = {
>>>> +    &dev_attr_zone_colors.attr,
>>>> +    NULL,
>>>> +};
>>>> +ATTRIBUTE_GROUPS(omen_kbd_led);
>>>> +
>>>>    static void hp_wmi_notify(u32 value, void *context)
>>>>    {
>>>>        struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>>> @@ -853,6 +908,10 @@ static void hp_wmi_notify(u32 value, void *context)
>>>>        case HPWMI_PROXIMITY_SENSOR:
>>>>            break;
>>>>        case HPWMI_BACKLIT_KB_BRIGHTNESS:
>>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
>>>> +        input_sync(hp_wmi_input_dev);
>>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
>>>> +        input_sync(hp_wmi_input_dev);
>>>>            break;
>>>>        case HPWMI_PEAKSHIFT_PERIOD:
>>>>            break;
>>>> @@ -1294,6 +1353,60 @@ static int thermal_profile_setup(void)
>>>>      static int hp_wmi_hwmon_init(void);
>>>>    +static enum led_brightness get_omen_backlight_brightness(struct led_classdev *cdev)
>>>> +{
>>>> +    u8 val;
>>>> +
>>>> +    int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>>> +
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    return (val & 0x80) ? LED_ON : LED_OFF;
>>>> +}
>>>> +
>>>> +static void set_omen_backlight_brightness(struct led_classdev *cdev, enum led_brightness value)
>>>> +{
>>>> +    char buffer[4] = { (value == LED_OFF) ? 0x64 : 0xe4, 0, 0, 0 };
>>>> +
>>>> +    hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_KB, &buffer,
>>>> +                       sizeof(buffer), 0);
>>>> +}
>>>> +
>>>> +static struct led_classdev omen_kbd_led = {
>>>> +    .name = "hp_omen::kbd_backlight",
>>>> +    .brightness_set = set_omen_backlight_brightness,
>>>> +    .brightness_get = get_omen_backlight_brightness,
>>>> +    .max_brightness = 1,
>>>> +    .groups = omen_kbd_led_groups,
>>>> +};
>>>> +
>>>> +static bool is_omen_lighting_supported(void)
>>>> +{
>>>> +    u8 val;
>>>> +
>>>> +    int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>>> +
>>>> +    if (ret)
>>>> +        return false;
>>>> +
>>>> +    return (val & 1) == 1;
>>>> +}
>>>> +
>>>> +static int omen_backlight_init(struct device *dev)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE);
>>>> +
>>>> +    ret = devm_led_classdev_register(dev, &omen_kbd_led);
>>>> +
>>>> +    if (ret < 0)
>>>> +        return -1;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>    {
>>>>        int err;
>>>> @@ -1321,6 +1434,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>          thermal_profile_setup();
>>>>    +    if (is_omen_lighting_supported())
>>>> +        omen_backlight_init(&device->dev);
>>>> +
>>>>        return 0;
>>>>    }
>>>>    

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

[Hans de Goede]

Hi Rishit,

On 2/2/23 20:59, Rishit Bansal wrote:
> Hi,
> 
> On 02/02/23 18:13, Hans de Goede wrote:

<snip>

>> I have been thinking a bit about this and I still think that having separate per-zone
>> files would be better. You can speedup things about 2x by only doing the call to read
>> the buffer once and cache the result. At least assuming the non kbd zone related bits
>> of the buffer never change (which should be easy enough to check).
>>
>> Actually my "thinking about this" includes a new alternate proposal. Rather then
>> making up our own userspace API, as I did for the logitech 510 USB keyboard
>> new support for multi-color backlights really should use the new standardized
>> multi-color LED API:
>>
>> https://www.kernel.org/doc/html/latest/leds/leds-class-multicolor.html
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led-multicolor
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/includ
>>
>> I have been thinking about how to use this with a 4 zone keyboard and I believe
>> that the best way to do that is to:
>>
>>
>> 1. Forget about the global on/off control, individual zones can be turned off by
>> setting the brightness of the zone to 0.
>>
>> This does require the driver to at least turn on the global control once, or
>> you could:
>>
>> 1a) cache the global control value
>> 1b) on zone changes check if all zones are off, if they are all off, use the
>>      global control to turn everything off, else turn the global control on;
>>      and only make the actual WMI call for this if the global control state
>>      changes vs the last cached value
>>
>> 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/
>>
>> This way we are using standard existing userspace APIs rather then inventing
>> new userspace API which IMHO is a much better approach.
>>
>> Note this is just a suggestion, if you disagree (and can motivate
>> why you think this is a bad idea) please do speak up about this.
>>
>> And please let me know if you need any help with converting the code
>> to the ed-class-multicolor inetnal kernel APIs there are not that
>> much users yet, so I have been unable to find a good example to
>> point you to.
>>
>> A downside of this is that it lacks e.g. support in upower. But the
>> kbd_backlight code in upower needs work anyways. E.g. upower does not
>> work with backlit USB keyboards if these are plugged in after boot,
>> or unplugged + re-plugged after boot. So someone really needs to
>> spend some time to improve the upower keyboard backlight code anyways.
>>
>> Regards,
>>
>> Hans
>>
> 
> I agree the multi-color class is the correct thing to use here, but I am not completely sure if we should have multiple files in /sys/class/leds with the string "kbd_backlight" in them. UPower seems to take the first occurence of kbd_backlight and assume that's the keyboard backlight (https://github.com/freedesktop/upower/blob/0e256ece04a98d3d202ed9640d33c56aaaeda137/src/up-kbd-backlight.c#L263-L269). I completely agree that this implementation needs more work on it, but it may have unintended consequences with software that uses UPower's kbd_backlight to control the keyboard.
> 
> 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?

I was hoping / expecting that using $foo::kbd_backlight-$postfix
would make current upower code ignore the LED class devices, but
you are right, upower does not parse the string and then checks that
the part after the last ':' is kbd_backlight it simply does a strstr
for kbd_backlight in the LED class-device's name. So it would indeed
pick one zone and use that.

So one thing which we could do is change the name to e.g. :

/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/

To make upower ignore all zones (until it learns about
such setups with new code).

> One alternative I can think of to have the "best of both worlds" (maintain support with Upower, and conform with the muti-color led specification), is to use the multi-color led class, and put all the indexes/brightness under one file. (Please correct me if the multi led specification does not allow this, but I don't see any limitation for having indexes other then just "red", "green" and "blue"):
> 
> 
> $ cat /sys/class/leds/hp_omen::kbd_backlight/multi_index
> 
> zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue zone_3_red zone_3_green zone_3_blue zone_4_red zone_4_green zone_4_blue
> 
> 
> And we can set it accordingly by doing:
> 
> $ echo 255 255 255 255 255 255 255 255 255 255 255 255 > /sys/class/leds/hp_omen::kbd_backlight/multi_intensity
> 
> 
> And then I can use "led_mc_calc_color_components" when the brightness is changed to directly compute the brightness of each index value and pass it to the keyboard through the WMI method.
> 
> 
> I know this suggestion goes back to us putting the all zones under a single file (sort of, we are atleast a bit closer to atleast following a spec now), but what are your thoughts on doing it this way with multi_index instead?

That is quite an interesting proposal, it would still make the current
kbd-backlight dimming in g-s-d + upower work and it means we only
need 1 WMI call for changing all the zones, so this nicely matches
with the actual firmware-API for this.

So yes lets go this route, that seems the best way to do this to me.

Note can you also add a separate patch to document the uses of:

zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue ...

As the way to set per-zone colors for RGB backlight keyboards with zones ?

Either in:

Documentation/ABI/testing/sysfs-class-led-multicolor

or in:

Documentation/leds/leds-class-multicolor.rst

?

The idea here is to have a standard way of doing this to refer to
if / when support for more zoned rgb backlight keyboards gets
added to the kernel.

I have added Dan Murphy, who is listed as contact for this in:
Documentation/ABI/testing/sysfs-class-led-multicolor
to the Cc.

Regards,

Hans





>>>>> +
>>>>> +
>>>>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>>>>> index 0a99058be813..f86cb7feaad4 100644
>>>>> --- a/drivers/platform/x86/hp/hp-wmi.c
>>>>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>>>>> @@ -27,6 +27,7 @@
>>>>>    #include <linux/rfkill.h>
>>>>>    #include <linux/string.h>
>>>>>    #include <linux/dmi.h>
>>>>> +#include <linux/leds.h>
>>>>>      MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
>>>>>    MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
>>>>> @@ -136,6 +137,7 @@ enum hp_wmi_command {
>>>>>        HPWMI_WRITE    = 0x02,
>>>>>        HPWMI_ODM    = 0x03,
>>>>>        HPWMI_GM    = 0x20008,
>>>>> +    HPWMI_KB    = 0x20009,
>>>>>    };
>>>>>      enum hp_wmi_hardware_mask {
>>>>> @@ -254,6 +256,9 @@ static const char * const tablet_chassis_types[] = {
>>>>>      #define DEVICE_MODE_TABLET    0x06
>>>>>    +#define OMEN_ZONE_COLOR_OFFSET 0x19
>>>>> +#define OMEN_ZONE_COLOR_LEN 0x0c
>>>>> +
>>>>>    /* map output size to the corresponding WMI method id */
>>>>>    static inline int encode_outsize_for_pvsz(int outsize)
>>>>>    {
>>>>> @@ -734,12 +739,56 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>>>>        return count;
>>>>>    }
>>>>>    +static ssize_t zone_colors_show(struct device *dev,
>>>>> +                    struct device_attribute *attr, char *buf)
>>>>> +{
>>>>> +    u8 val[128];
>>>>> +
>>>>> +    int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>>>> +                       zero_if_sup(val), sizeof(val));
>>>>> +
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    memcpy(buf, &val[OMEN_ZONE_COLOR_OFFSET], OMEN_ZONE_COLOR_LEN);
>>>>> +
>>>>> +    return OMEN_ZONE_COLOR_LEN;
>>>>> +}
>>>>> +
>>>>> +static ssize_t zone_colors_store(struct device *dev,
>>>>> +                     struct device_attribute *attr,
>>>>> +                     const char *buf, size_t count)
>>>>> +{
>>>>> +    u8 val[128];
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>>>> +                   zero_if_sup(val), sizeof(val));
>>>>> +
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    if (count != OMEN_ZONE_COLOR_LEN)
>>>>> +        return -1;
>>>>> +
>>>>> +    memcpy(&val[OMEN_ZONE_COLOR_OFFSET], buf, count);
>>>>> +
>>>>> +    ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_KB, &val, sizeof(val),
>>>>> +                   0);
>>>>> +
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    return OMEN_ZONE_COLOR_LEN;
>>>>> +}
>>>>> +
>>>>>    static DEVICE_ATTR_RO(display);
>>>>>    static DEVICE_ATTR_RO(hddtemp);
>>>>>    static DEVICE_ATTR_RW(als);
>>>>>    static DEVICE_ATTR_RO(dock);
>>>>>    static DEVICE_ATTR_RO(tablet);
>>>>>    static DEVICE_ATTR_RW(postcode);
>>>>> +static DEVICE_ATTR_RW(zone_colors);
>>>>>      static struct attribute *hp_wmi_attrs[] = {
>>>>>        &dev_attr_display.attr,
>>>>> @@ -752,6 +801,12 @@ static struct attribute *hp_wmi_attrs[] = {
>>>>>    };
>>>>>    ATTRIBUTE_GROUPS(hp_wmi);
>>>>>    +static struct attribute *omen_kbd_led_attrs[] = {
>>>>> +    &dev_attr_zone_colors.attr,
>>>>> +    NULL,
>>>>> +};
>>>>> +ATTRIBUTE_GROUPS(omen_kbd_led);
>>>>> +
>>>>>    static void hp_wmi_notify(u32 value, void *context)
>>>>>    {
>>>>>        struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>>>> @@ -853,6 +908,10 @@ static void hp_wmi_notify(u32 value, void *context)
>>>>>        case HPWMI_PROXIMITY_SENSOR:
>>>>>            break;
>>>>>        case HPWMI_BACKLIT_KB_BRIGHTNESS:
>>>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
>>>>> +        input_sync(hp_wmi_input_dev);
>>>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
>>>>> +        input_sync(hp_wmi_input_dev);
>>>>>            break;
>>>>>        case HPWMI_PEAKSHIFT_PERIOD:
>>>>>            break;
>>>>> @@ -1294,6 +1353,60 @@ static int thermal_profile_setup(void)
>>>>>      static int hp_wmi_hwmon_init(void);
>>>>>    +static enum led_brightness get_omen_backlight_brightness(struct led_classdev *cdev)
>>>>> +{
>>>>> +    u8 val;
>>>>> +
>>>>> +    int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>>>> +
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    return (val & 0x80) ? LED_ON : LED_OFF;
>>>>> +}
>>>>> +
>>>>> +static void set_omen_backlight_brightness(struct led_classdev *cdev, enum led_brightness value)
>>>>> +{
>>>>> +    char buffer[4] = { (value == LED_OFF) ? 0x64 : 0xe4, 0, 0, 0 };
>>>>> +
>>>>> +    hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_KB, &buffer,
>>>>> +                       sizeof(buffer), 0);
>>>>> +}
>>>>> +
>>>>> +static struct led_classdev omen_kbd_led = {
>>>>> +    .name = "hp_omen::kbd_backlight",
>>>>> +    .brightness_set = set_omen_backlight_brightness,
>>>>> +    .brightness_get = get_omen_backlight_brightness,
>>>>> +    .max_brightness = 1,
>>>>> +    .groups = omen_kbd_led_groups,
>>>>> +};
>>>>> +
>>>>> +static bool is_omen_lighting_supported(void)
>>>>> +{
>>>>> +    u8 val;
>>>>> +
>>>>> +    int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>>>> +
>>>>> +    if (ret)
>>>>> +        return false;
>>>>> +
>>>>> +    return (val & 1) == 1;
>>>>> +}
>>>>> +
>>>>> +static int omen_backlight_init(struct device *dev)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE);
>>>>> +
>>>>> +    ret = devm_led_classdev_register(dev, &omen_kbd_led);
>>>>> +
>>>>> +    if (ret < 0)
>>>>> +        return -1;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>    static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>>    {
>>>>>        int err;
>>>>> @@ -1321,6 +1434,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>>          thermal_profile_setup();
>>>>>    +    if (is_omen_lighting_supported())
>>>>> +        omen_backlight_init(&device->dev);
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    
> 

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

[Rishit Bansal]

On 06/02/23 20:02, Hans de Goede wrote:
> Hi Rishit,
> 
> On 2/2/23 20:59, Rishit Bansal wrote:
>> Hi,
>>
>> On 02/02/23 18:13, Hans de Goede wrote:
> 
> 
>>> I have been thinking a bit about this and I still think that having separate per-zone
>>> files would be better. You can speedup things about 2x by only doing the call to read
>>> the buffer once and cache the result. At least assuming the non kbd zone related bits
>>> of the buffer never change (which should be easy enough to check).
>>>
>>> Actually my "thinking about this" includes a new alternate proposal. Rather then
>>> making up our own userspace API, as I did for the logitech 510 USB keyboard
>>> new support for multi-color backlights really should use the new standardized
>>> multi-color LED API:
>>>
>>> https://www.kernel.org/doc/html/latest/leds/leds-class-multicolor.html
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-led-multicolor
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/includ
>>>
>>> I have been thinking about how to use this with a 4 zone keyboard and I believe
>>> that the best way to do that is to:
>>>
>>>
>>> 1. Forget about the global on/off control, individual zones can be turned off by
>>> setting the brightness of the zone to 0.
>>>
>>> This does require the driver to at least turn on the global control once, or
>>> you could:
>>>
>>> 1a) cache the global control value
>>> 1b) on zone changes check if all zones are off, if they are all off, use the
>>>       global control to turn everything off, else turn the global control on;
>>>       and only make the actual WMI call for this if the global control state
>>>       changes vs the last cached value
>>>
>>> 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/
>>>
>>> This way we are using standard existing userspace APIs rather then inventing
>>> new userspace API which IMHO is a much better approach.
>>>
>>> Note this is just a suggestion, if you disagree (and can motivate
>>> why you think this is a bad idea) please do speak up about this.
>>>
>>> And please let me know if you need any help with converting the code
>>> to the ed-class-multicolor inetnal kernel APIs there are not that
>>> much users yet, so I have been unable to find a good example to
>>> point you to.
>>>
>>> A downside of this is that it lacks e.g. support in upower. But the
>>> kbd_backlight code in upower needs work anyways. E.g. upower does not
>>> work with backlit USB keyboards if these are plugged in after boot,
>>> or unplugged + re-plugged after boot. So someone really needs to
>>> spend some time to improve the upower keyboard backlight code anyways.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>
>> I agree the multi-color class is the correct thing to use here, but I am not completely sure if we should have multiple files in /sys/class/leds with the string "kbd_backlight" in them. UPower seems to take the first occurence of kbd_backlight and assume that's the keyboard backlight (https://github.com/freedesktop/upower/blob/0e256ece04a98d3d202ed9640d33c56aaaeda137/src/up-kbd-backlight.c#L263-L269). I completely agree that this implementation needs more work on it, but it may have unintended consequences with software that uses UPower's kbd_backlight to control the keyboard.
>>
>> 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?
> 
> I was hoping / expecting that using $foo::kbd_backlight-$postfix
> would make current upower code ignore the LED class devices, but
> you are right, upower does not parse the string and then checks that
> the part after the last ':' is kbd_backlight it simply does a strstr
> for kbd_backlight in the LED class-device's name. So it would indeed
> pick one zone and use that.
> 
> So one thing which we could do is change the name to e.g. :
> 
> /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/
> 
> To make upower ignore all zones (until it learns about
> such setups with new code).
> 
>> One alternative I can think of to have the "best of both worlds" (maintain support with Upower, and conform with the muti-color led specification), is to use the multi-color led class, and put all the indexes/brightness under one file. (Please correct me if the multi led specification does not allow this, but I don't see any limitation for having indexes other then just "red", "green" and "blue"):
>>
>>
>> $ cat /sys/class/leds/hp_omen::kbd_backlight/multi_index
>>
>> zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue zone_3_red zone_3_green zone_3_blue zone_4_red zone_4_green zone_4_blue
>>
>>
>> And we can set it accordingly by doing:
>>
>> $ echo 255 255 255 255 255 255 255 255 255 255 255 255 > /sys/class/leds/hp_omen::kbd_backlight/multi_intensity
>>
>>
>> And then I can use "led_mc_calc_color_components" when the brightness is changed to directly compute the brightness of each index value and pass it to the keyboard through the WMI method.
>>
>>
>> I know this suggestion goes back to us putting the all zones under a single file (sort of, we are atleast a bit closer to atleast following a spec now), but what are your thoughts on doing it this way with multi_index instead?
> 
> That is quite an interesting proposal, it would still make the current
> kbd-backlight dimming in g-s-d + upower work and it means we only
> need 1 WMI call for changing all the zones, so this nicely matches
> with the actual firmware-API for this.
> 
> So yes lets go this route, that seems the best way to do this to me.
> 
> Note can you also add a separate patch to document the uses of:
> 
> zone_1_red zone_1_green zone_1_blue zone_2_red zone_2_green zone_2_blue ...
> 
> As the way to set per-zone colors for RGB backlight keyboards with zones ?
> 
> Either in:
> 
> Documentation/ABI/testing/sysfs-class-led-multicolor
> 
> or in:
> 
> Documentation/leds/leds-class-multicolor.rst
> 
> ?

Sounds great! I'll make a v4 patch with this approach, and another patch 
for the zone documentation.


On a side note: While using this patch on my machine for a while, I also 
noticed another edge case where it would automatically turn on my 
backlight even if I explicitly turned it off using the hardware button 
(Fn+F4). I tracked this down to not supporting the 
"brightness_hw_change" 
(https://github.com/freedesktop/upower/blob/0e256ece04a98d3d202ed9640d33c56aaaeda137/src/up-kbd-backlight.c#L297) 
attribute correctly, which upower uses to detect changes in the state of 
the backlight. I will also add a fix for this in my next patch.

(Sorry if this got sent twice, it seems an HTML tag made its way into 
the previous email and it got rejected by the mailing list>

> 
> The idea here is to have a standard way of doing this to refer to
> if / when support for more zoned rgb backlight keyboards gets
> added to the kernel.
> 
> I have added Dan Murphy, who is listed as contact for this in:
> Documentation/ABI/testing/sysfs-class-led-multicolor
> to the Cc.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>>>>>> +
>>>>>> +
>>>>>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>>>>>> index 0a99058be813..f86cb7feaad4 100644
>>>>>> --- a/drivers/platform/x86/hp/hp-wmi.c
>>>>>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>>>>>> @@ -27,6 +27,7 @@
>>>>>>     #include <linux/rfkill.h>
>>>>>>     #include <linux/string.h>
>>>>>>     #include <linux/dmi.h>
>>>>>> +#include <linux/leds.h>
>>>>>>       MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
>>>>>>     MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
>>>>>> @@ -136,6 +137,7 @@ enum hp_wmi_command {
>>>>>>         HPWMI_WRITE    = 0x02,
>>>>>>         HPWMI_ODM    = 0x03,
>>>>>>         HPWMI_GM    = 0x20008,
>>>>>> +    HPWMI_KB    = 0x20009,
>>>>>>     };
>>>>>>       enum hp_wmi_hardware_mask {
>>>>>> @@ -254,6 +256,9 @@ static const char * const tablet_chassis_types[] = {
>>>>>>       #define DEVICE_MODE_TABLET    0x06
>>>>>>     +#define OMEN_ZONE_COLOR_OFFSET 0x19
>>>>>> +#define OMEN_ZONE_COLOR_LEN 0x0c
>>>>>> +
>>>>>>     /* map output size to the corresponding WMI method id */
>>>>>>     static inline int encode_outsize_for_pvsz(int outsize)
>>>>>>     {
>>>>>> @@ -734,12 +739,56 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>>>>>         return count;
>>>>>>     }
>>>>>>     +static ssize_t zone_colors_show(struct device *dev,
>>>>>> +                    struct device_attribute *attr, char *buf)
>>>>>> +{
>>>>>> +    u8 val[128];
>>>>>> +
>>>>>> +    int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>>>>> +                       zero_if_sup(val), sizeof(val));
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    memcpy(buf, &val[OMEN_ZONE_COLOR_OFFSET], OMEN_ZONE_COLOR_LEN);
>>>>>> +
>>>>>> +    return OMEN_ZONE_COLOR_LEN;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t zone_colors_store(struct device *dev,
>>>>>> +                     struct device_attribute *attr,
>>>>>> +                     const char *buf, size_t count)
>>>>>> +{
>>>>>> +    u8 val[128];
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_KB, &val,
>>>>>> +                   zero_if_sup(val), sizeof(val));
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    if (count != OMEN_ZONE_COLOR_LEN)
>>>>>> +        return -1;
>>>>>> +
>>>>>> +    memcpy(&val[OMEN_ZONE_COLOR_OFFSET], buf, count);
>>>>>> +
>>>>>> +    ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_KB, &val, sizeof(val),
>>>>>> +                   0);
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    return OMEN_ZONE_COLOR_LEN;
>>>>>> +}
>>>>>> +
>>>>>>     static DEVICE_ATTR_RO(display);
>>>>>>     static DEVICE_ATTR_RO(hddtemp);
>>>>>>     static DEVICE_ATTR_RW(als);
>>>>>>     static DEVICE_ATTR_RO(dock);
>>>>>>     static DEVICE_ATTR_RO(tablet);
>>>>>>     static DEVICE_ATTR_RW(postcode);
>>>>>> +static DEVICE_ATTR_RW(zone_colors);
>>>>>>       static struct attribute *hp_wmi_attrs[] = {
>>>>>>         &dev_attr_display.attr,
>>>>>> @@ -752,6 +801,12 @@ static struct attribute *hp_wmi_attrs[] = {
>>>>>>     };
>>>>>>     ATTRIBUTE_GROUPS(hp_wmi);
>>>>>>     +static struct attribute *omen_kbd_led_attrs[] = {
>>>>>> +    &dev_attr_zone_colors.attr,
>>>>>> +    NULL,
>>>>>> +};
>>>>>> +ATTRIBUTE_GROUPS(omen_kbd_led);
>>>>>> +
>>>>>>     static void hp_wmi_notify(u32 value, void *context)
>>>>>>     {
>>>>>>         struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>>>>> @@ -853,6 +908,10 @@ static void hp_wmi_notify(u32 value, void *context)
>>>>>>         case HPWMI_PROXIMITY_SENSOR:
>>>>>>             break;
>>>>>>         case HPWMI_BACKLIT_KB_BRIGHTNESS:
>>>>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
>>>>>> +        input_sync(hp_wmi_input_dev);
>>>>>> +        input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
>>>>>> +        input_sync(hp_wmi_input_dev);
>>>>>>             break;
>>>>>>         case HPWMI_PEAKSHIFT_PERIOD:
>>>>>>             break;
>>>>>> @@ -1294,6 +1353,60 @@ static int thermal_profile_setup(void)
>>>>>>       static int hp_wmi_hwmon_init(void);
>>>>>>     +static enum led_brightness get_omen_backlight_brightness(struct led_classdev *cdev)
>>>>>> +{
>>>>>> +    u8 val;
>>>>>> +
>>>>>> +    int ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    return (val & 0x80) ? LED_ON : LED_OFF;
>>>>>> +}
>>>>>> +
>>>>>> +static void set_omen_backlight_brightness(struct led_classdev *cdev, enum led_brightness value)
>>>>>> +{
>>>>>> +    char buffer[4] = { (value == LED_OFF) ? 0x64 : 0xe4, 0, 0, 0 };
>>>>>> +
>>>>>> +    hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_KB, &buffer,
>>>>>> +                       sizeof(buffer), 0);
>>>>>> +}
>>>>>> +
>>>>>> +static struct led_classdev omen_kbd_led = {
>>>>>> +    .name = "hp_omen::kbd_backlight",
>>>>>> +    .brightness_set = set_omen_backlight_brightness,
>>>>>> +    .brightness_get = get_omen_backlight_brightness,
>>>>>> +    .max_brightness = 1,
>>>>>> +    .groups = omen_kbd_led_groups,
>>>>>> +};
>>>>>> +
>>>>>> +static bool is_omen_lighting_supported(void)
>>>>>> +{
>>>>>> +    u8 val;
>>>>>> +
>>>>>> +    int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_KB, &val, zero_if_sup(val), sizeof(val));
>>>>>> +
>>>>>> +    if (ret)
>>>>>> +        return false;
>>>>>> +
>>>>>> +    return (val & 1) == 1;
>>>>>> +}
>>>>>> +
>>>>>> +static int omen_backlight_init(struct device *dev)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE);
>>>>>> +
>>>>>> +    ret = devm_led_classdev_register(dev, &omen_kbd_led);
>>>>>> +
>>>>>> +    if (ret < 0)
>>>>>> +        return -1;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>     static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>>>     {
>>>>>>         int err;
>>>>>> @@ -1321,6 +1434,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>>>>>>           thermal_profile_setup();
>>>>>>     +    if (is_omen_lighting_supported())
>>>>>> +        omen_backlight_init(&device->dev);
>>>>>> +
>>>>>>         return 0;
>>>>>>     }
>>>>>>     
>>
> 

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

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

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.

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
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Rishit Bansal]

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.

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

[Hans de Goede]

Hi,

On 2/7/23 14:05, Rishit Bansal wrote:
> 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.

I am not really a fan of this. When the "global" LED then is turned off (brightness=0) then all the other LED devices all of a sudden do nothing and writing values > 0 to their brightness won't turn them on which is not how the LED class API is supposed to work. We can come up with various tricks to work around this, but the fact remains that if we go this route we end up with weird hard to define interaction between 2 LED devices while from an userspace API pov they really should be independent.

note that both Pavel and I suggested using 4 multi-color LED class devices (1 per zone) for this and I still/really believe that this is the best way to deal with this.

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.

Regards,

hans

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

[Rishit Bansal]

Hi,

On 13/02/23 18:19, Hans de Goede wrote:
> Hi,
> 
> On 2/7/23 14:05, Rishit Bansal wrote:
>> 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.
> 
> I am not really a fan of this. When the "global" LED then is turned off (brightness=0) then all the other LED devices all of a sudden do nothing and writing values > 0 to their brightness won't turn them on which is not how the LED class API is supposed to work. We can come up with various tricks to work around this, but the fact remains that if we go this route we end up with weird hard to define interaction between 2 LED devices while from an userspace API pov they really should be independent.
> 
> note that both Pavel and I suggested using 4 multi-color LED class devices (1 per zone) for this and I still/really believe that this is the best way to deal with this.
> 
> 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.
> 
> Regards,
> 
> hans
> 
> 

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!

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

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. But we already use _1 suffix to deduplicate the, so
I'm not sure this is best naming.

There are keyboards with per-key backlight. How do you suggest to
solve those?

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Hans de Goede]

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.

> 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.

Regards,

Hans

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

[Rishit Bansal]

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
> 
> 
> 

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

[Hans de Goede]

Hi,

On 2/19/23 19:46, Rishit Bansal wrote:
> 
> 
> 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

Sounds good to me, lets go for this. Please add these names to
the requested documentation update.

Regards,

Hans