Re: [PATCH 1/7] platform/x86: uniwill-laptop: Add keyboard backlight support

[Armin Wolf <W_Armin@gmx.de>]

Am 08.07.26 um 22:15 schrieb Werner Sembach:

>
> Am 08.07.26 um 22:08 schrieb Armin Wolf:
>> Am 08.07.26 um 21:11 schrieb Werner Sembach:
>>
>>> Hi,
>>>
>>> Am 30.05.26 um 19:08 schrieb Armin Wolf:
>>>> Many Uniwill-based devices support either a white-only or fully
>>>> features RGB keyboard backlight. Add support for this feature
>>>> and handle the associated WMI events.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>>   .../admin-guide/laptops/uniwill-laptop.rst    |  13 +
>>>>   drivers/platform/x86/uniwill/uniwill-acpi.c   | 392 
>>>> +++++++++++++++++-
>>>>   2 files changed, 397 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/laptops/uniwill-laptop.rst 
>>>> b/Documentation/admin-guide/laptops/uniwill-laptop.rst
>>>> index 24b41dbab886..2b1e5da703a5 100644
>>>> --- a/Documentation/admin-guide/laptops/uniwill-laptop.rst
>>>> +++ b/Documentation/admin-guide/laptops/uniwill-laptop.rst
>>>> @@ -77,6 +77,19 @@ LED class device. The default name of this LED 
>>>> class device is ``uniwill:multico
>>>>   See Documentation/ABI/testing/sysfs-driver-uniwill-laptop for 
>>>> details on how to control the various
>>>>   animation modes of the lightbar.
>>>>   +Keyboard Backlight
>>>> +------------------
>>>> +
>>>> +The ``uniwill-laptop`` driver supports controlling the keyboard 
>>>> backlight using the standard
>>>> +LED class interface. The default name of this LED class device is 
>>>> ``uniwill:white:kbd_backlight``
>>>> +when the keyboard backlight supports only a single color, or 
>>>> ``uniwill:multicolor:kbd_backlight``
>>>> +when the keyboard backlight supports RGB colors. The maximum 
>>>> intensity for each color channel
>>>> +in RGB mode is 50.
>>>> +
>>>> +Keep in mind that due to hardware design choices, the driver does 
>>>> not support the RGB value
>>>> +``0x000000`` (black), instead it will fall back to ``0x010101`` 
>>>> (faint white). In order to
>>>> +disable the keyboard backlight, the standard LED brightness 
>>>> setting has to be used instead.
>>>> +
>>>>   Configurable TGP
>>>>   ----------------
>>>>   diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>>>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>>> index ab063ead45b9..fd040197b189 100644
>>>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>>>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>>> @@ -110,8 +110,31 @@
>>>>   #define EC_ADDR_BAT_CYCLE_COUNT_2    0x04A7
>>>>     #define EC_ADDR_PROJECT_ID        0x0740
>>>> +#define PROJECT_ID_NONE            0x00
>>>> +#define PROJECT_ID_GI            0x01
>>>> +#define PROJECT_ID_GJ            0x02
>>>> +#define PROJECT_ID_GK            0x03
>>>> +#define PROJECT_ID_GICN            0x04
>>>> +#define PROJECT_ID_GJCN            0x05
>>>> +#define PROJECT_ID_GK5CN_X        0x06
>>>> +#define PROJECT_ID_GK7CN_S        0x07
>>>> +#define PROJECT_ID_GK7CPCS_GK5CQ7Z    0x08
>>>> +#define PROJECT_ID_PF            0x09
>>>> +#define PROJECT_ID_GK5CP_4X_5X_6X    0x0A
>>>> +#define PROJECT_ID_IDP            0x0B
>>>> +#define PROJECT_ID_IDY_6Y        0x0C
>>>> +#define PROJECT_ID_IDY_7Y        0x0D
>>>> +#define PROJECT_ID_PF4MU_PF4MN_PF5MU    0x0E
>>>> +#define PROJECT_ID_CML_GAMING        0x0F
>>>> +#define PROJECT_ID_GK7NXXR        0x10
>>>> +#define PROJECT_ID_GM5MU1Y        0x11
>>>>   #define PROJECT_ID_PH4TRX1        0x12
>>>> +#define PROJECT_ID_PH4TUX1        0x13
>>>> +#define PROJECT_ID_PH4TQX1        0x14
>>>>   #define PROJECT_ID_PH6TRX1        0x15
>>>> +#define PROJECT_ID_PH6TQXX        0x16
>>>> +#define PROJECT_ID_PHXAXXX        0x17
>>>> +#define PROJECT_ID_PHXPXXX        0x18
>>>>     #define EC_ADDR_AP_OEM            0x0741
>>>>   #define    ENABLE_MANUAL_CTRL        BIT(0)
>>>> @@ -214,6 +237,7 @@
>>>>   #define FAN_TABLE_OFFICE_MODE        BIT(2)
>>>>   #define FAN_V3                BIT(3)
>>>>   #define DEFAULT_MODE            BIT(4)
>>>> +#define ENABLE_CHINA_MODE        BIT(6)
>>>>     #define EC_ADDR_PL1_SETTING        0x0783
>>>>   @@ -225,11 +249,11 @@
>>>>   #define FAN_CURVE_LENGTH        5
>>>>     #define EC_ADDR_KBD_STATUS        0x078C
>>>> -#define KBD_WHITE_ONLY            BIT(0)    // ~single color
>>>> -#define KBD_SINGLE_COLOR_OFF        BIT(1)
>>>> +#define KBD_WHITE_ONLY            BIT(0)
>>>> +#define KBD_POWER_OFF            BIT(1)
>>>>   #define KBD_TURBO_LEVEL_MASK        GENMASK(3, 2)
>>>>   #define KBD_APPLY            BIT(4)
>>>> -#define KBD_BRIGHTNESS            GENMASK(7, 5)
>>>> +#define KBD_BRIGHTNESS_MASK        GENMASK(7, 5)
>>>>     #define EC_ADDR_FAN_CTRL        0x078E
>>>>   #define FAN3P5                BIT(1)
>>>> @@ -320,6 +344,9 @@
>>>>   #define LED_CHANNELS        3
>>>>   #define LED_MAX_BRIGHTNESS    200
>>>>   +#define KBD_LED_CHANNELS    3
>>>> +#define KBD_LED_MAX_INTENSITY    50
>>>> +
>>>>   #define UNIWILL_FEATURE_FN_LOCK            BIT(0)
>>>>   #define UNIWILL_FEATURE_SUPER_KEY        BIT(1)
>>>>   #define UNIWILL_FEATURE_TOUCHPAD_TOGGLE        BIT(2)
>>>> @@ -333,6 +360,7 @@
>>>>   #define UNIWILL_FEATURE_SECONDARY_FAN        BIT(9)
>>>>   #define UNIWILL_FEATURE_NVIDIA_CTGP_CONTROL    BIT(10)
>>>>   #define UNIWILL_FEATURE_USB_C_POWER_PRIORITY    BIT(11)
>>>> +#define UNIWILL_FEATURE_KEYBOARD_BACKLIGHT    BIT(12)
>>>>     enum usb_c_power_priority_options {
>>>>       USB_C_POWER_PRIORITY_CHARGING = 0,
>>>> @@ -344,6 +372,7 @@ struct uniwill_data {
>>>>       acpi_handle handle;
>>>>       struct regmap *regmap;
>>>>       unsigned int features;
>>>> +    u8 project_id;
>>>>       struct acpi_battery_hook hook;
>>>>       struct mutex battery_lock;    /* Protects the list of 
>>>> currently registered batteries */
>>>>       union {
>>>> @@ -362,6 +391,18 @@ struct uniwill_data {
>>>>       struct mutex led_lock;        /* Protects writes to the 
>>>> lightbar registers */
>>>>       struct led_classdev_mc led_mc_cdev;
>>>>       struct mc_subled led_mc_subled_info[LED_CHANNELS];
>>>> +    bool single_color_kbd;
>>>> +    u8 kbd_led_max_brightness;
>>>> +    unsigned int last_kbd_status;
>>>> +    union {
>>>> +        struct {
>>>> +            /* Protects writes to the RGB keyboard backlight 
>>>> registers */
>>>> +            struct mutex kbd_rgb_led_lock;
>>>> +            struct led_classdev_mc kbd_led_mc_cdev;
>>>> +            struct mc_subled 
>>>> kbd_led_mc_subled_info[KBD_LED_CHANNELS];
>>>> +        };
>>>> +        struct led_classdev kbd_led_cdev;
>>>> +    };
>>>>       struct mutex input_lock;    /* Protects input sequence during 
>>>> notify */
>>>>       struct input_dev *input_device;
>>>>       struct notifier_block nb;
>>>> @@ -376,6 +417,7 @@ struct uniwill_battery_entry {
>>>>     struct uniwill_device_descriptor {
>>>>       unsigned int features;
>>>> +    u8 kbd_led_max_brightness;
>>>>       /* Executed during driver probing */
>>>>       int (*probe)(struct uniwill_data *data);
>>>>   };
>>>> @@ -427,6 +469,9 @@ static const struct key_entry uniwill_keymap[] = {
>>>>       { KE_KEY,       UNIWILL_OSD_KBDILLUMDOWN, { KEY_KBDILLUMDOWN }},
>>>>       { KE_KEY,       UNIWILL_OSD_KBDILLUMUP, { KEY_KBDILLUMUP }},
>>>>   +    /* Reported when the EC changed the keyboard backlight 
>>>> brightness */
>>>> +    { KE_IGNORE,    UNIWILL_OSD_BACKLIGHT_LEVEL_CHANGE, { 
>>>> KEY_UNKNOWN }},
>>>> +
>>>>       /* Reported when the user wants to toggle the microphone mute 
>>>> status */
>>>>       { KE_KEY,       UNIWILL_OSD_MIC_MUTE, { KEY_MICMUTE }},
>>>>   @@ -435,11 +480,6 @@ static const struct key_entry 
>>>> uniwill_keymap[] = {
>>>>         /* Reported when the user wants to toggle the brightness of 
>>>> the keyboard */
>>>>       { KE_KEY,       UNIWILL_OSD_KBDILLUMTOGGLE, { 
>>>> KEY_KBDILLUMTOGGLE }},
>>>> -    { KE_KEY,       UNIWILL_OSD_KB_LED_LEVEL0, { 
>>>> KEY_KBDILLUMTOGGLE }},
>>>> -    { KE_KEY,       UNIWILL_OSD_KB_LED_LEVEL1, { 
>>>> KEY_KBDILLUMTOGGLE }},
>>>> -    { KE_KEY,       UNIWILL_OSD_KB_LED_LEVEL2, { 
>>>> KEY_KBDILLUMTOGGLE }},
>>>> -    { KE_KEY,       UNIWILL_OSD_KB_LED_LEVEL3, { 
>>>> KEY_KBDILLUMTOGGLE }},
>>>> -    { KE_KEY,       UNIWILL_OSD_KB_LED_LEVEL4, { 
>>>> KEY_KBDILLUMTOGGLE }},
>>>>         /* FIXME: find out the exact meaning of those events */
>>>>       { KE_IGNORE,    UNIWILL_OSD_BAT_CHARGE_FULL_24_H, { 
>>>> KEY_UNKNOWN }},
>>>> @@ -547,6 +587,11 @@ static bool uniwill_writeable_reg(struct 
>>>> device *dev, unsigned int reg)
>>>>       case EC_ADDR_LIGHTBAR_AC_BLUE:
>>>>       case EC_ADDR_BIOS_OEM:
>>>>       case EC_ADDR_TRIGGER:
>>>> +    case EC_ADDR_RGB_RED:
>>>> +    case EC_ADDR_RGB_GREEN:
>>>> +    case EC_ADDR_RGB_BLUE:
>>>> +    case EC_ADDR_BIOS_OEM_2:
>>>> +    case EC_ADDR_KBD_STATUS:
>>>>       case EC_ADDR_OEM_4:
>>>>       case EC_ADDR_CHARGE_CTRL:
>>>>       case EC_ADDR_LIGHTBAR_BAT_CTRL:
>>>> @@ -583,8 +628,14 @@ static bool uniwill_readable_reg(struct device 
>>>> *dev, unsigned int reg)
>>>>       case EC_ADDR_BIOS_OEM:
>>>>       case EC_ADDR_PWM_1:
>>>>       case EC_ADDR_PWM_2:
>>>> +    case EC_ADDR_SUPPORT_2:
>>>>       case EC_ADDR_TRIGGER:
>>>>       case EC_ADDR_SWITCH_STATUS:
>>>> +    case EC_ADDR_RGB_RED:
>>>> +    case EC_ADDR_RGB_GREEN:
>>>> +    case EC_ADDR_RGB_BLUE:
>>>> +    case EC_ADDR_BIOS_OEM_2:
>>>> +    case EC_ADDR_KBD_STATUS:
>>>>       case EC_ADDR_OEM_4:
>>>>       case EC_ADDR_CHARGE_CTRL:
>>>>       case EC_ADDR_LIGHTBAR_BAT_CTRL:
>>>> @@ -616,8 +667,10 @@ static bool uniwill_volatile_reg(struct device 
>>>> *dev, unsigned int reg)
>>>>       case EC_ADDR_BIOS_OEM:
>>>>       case EC_ADDR_PWM_1:
>>>>       case EC_ADDR_PWM_2:
>>>> +    case EC_ADDR_SUPPORT_2:
>>>>       case EC_ADDR_TRIGGER:
>>>>       case EC_ADDR_SWITCH_STATUS:
>>>> +    case EC_ADDR_KBD_STATUS:
>>>>       case EC_ADDR_OEM_4:
>>>>       case EC_ADDR_CHARGE_CTRL:
>>>>       case EC_ADDR_USB_C_POWER_PRIORITY:
>>>> @@ -1441,6 +1494,246 @@ static int uniwill_led_init(struct 
>>>> uniwill_data *data)
>>>>                                &init_data);
>>>>   }
>>>>   +static int uniwill_notify_kbd_led(struct uniwill_data *data, int 
>>>> brightness)
>>>> +{
>>>> +    struct led_classdev *led_cdev;
>>>> +    int ret;
>>>> +
>>>> +    if (data->single_color_kbd)
>>>> +        led_cdev = &data->kbd_led_cdev;
>>>> +    else
>>>> +        led_cdev = &data->kbd_led_mc_cdev.led_cdev;
>>>> +
>>>> +    guard(mutex)(&led_cdev->led_access);
>>>> +
>>>> +    /* Sync the LED brightness with the actual hardware state */
>>>> +    ret = led_update_brightness(led_cdev);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    led_classdev_notify_brightness_hw_changed(led_cdev, brightness);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +#define KBD_LED_MASK    (KBD_BRIGHTNESS_MASK | KBD_APPLY | 
>>>> KBD_POWER_OFF)
>>>> +
>>>> +static int uniwill_kbd_led_write_brightness(struct uniwill_data 
>>>> *data, int brightness)
>>>> +{
>>>> +    /* KBD_POWER_OFF is always implicitly cleared */
>>>> +    unsigned int regval = FIELD_PREP(KBD_BRIGHTNESS_MASK, 
>>>> brightness) | KBD_APPLY;
>>>> +
>>>> +    /* We must ensure that the "apply" bit is always written */
>>>> +    return regmap_write_bits(data->regmap, EC_ADDR_KBD_STATUS, 
>>>> KBD_LED_MASK, regval);
>>>> +}
>>>> +
>>>> +static int uniwill_kbd_led_read_brightness(struct uniwill_data *data)
>>>> +{
>>>> +    unsigned int regval;
>>>> +    int ret;
>>>> +
>>>> +    ret = regmap_read(data->regmap, EC_ADDR_KBD_STATUS, &regval);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    return min(FIELD_GET(KBD_BRIGHTNESS_MASK, regval), 
>>>> data->kbd_led_max_brightness);
>>>> +}
>>>> +
>>>> +static int uniwill_kbd_led_brightness_set(struct led_classdev 
>>>> *led_cdev,
>>>> +                      enum led_brightness brightness)
>>>> +{
>>>> +    struct uniwill_data *data = container_of(led_cdev, struct 
>>>> uniwill_data, kbd_led_cdev);
>>>> +
>>>> +    return uniwill_kbd_led_write_brightness(data, brightness);
>>>> +}
>>>> +
>>>> +static enum led_brightness uniwill_kbd_led_brightness_get(struct 
>>>> led_classdev *led_cdev)
>>>> +{
>>>> +    struct uniwill_data *data = container_of(led_cdev, struct 
>>>> uniwill_data, kbd_led_cdev);
>>>> +
>>>> +    return uniwill_kbd_led_read_brightness(data);
>>>> +}
>>>> +
>>>> +static const unsigned int 
>>>> uniwill_kbd_led_channel_to_reg[KBD_LED_CHANNELS] = {
>>>> +    EC_ADDR_RGB_RED,
>>>> +    EC_ADDR_RGB_GREEN,
>>>> +    EC_ADDR_RGB_BLUE,
>>>> +};
>>>> +
>>>> +static int uniwill_kbd_led_mc_brightness_set(struct led_classdev 
>>>> *led_cdev,
>>>> +                         enum led_brightness brightness)
>>>> +{
>>>> +    struct led_classdev_mc *led_mc_cdev = lcdev_to_mccdev(led_cdev);
>>>> +    struct uniwill_data *data = container_of(led_mc_cdev, struct 
>>>> uniwill_data, kbd_led_mc_cdev);
>>>> +    unsigned int min_intensity = 0;
>>>> +    unsigned int regval;
>>>> +    int ret;
>>>> +
>>>> +    guard(mutex)(&data->kbd_rgb_led_lock);
>>>> +
>>>> +    /*
>>>> +     * The EC interprets a RGB value of 0x000000 as a command to 
>>>> restore
>>>> +     * the device-specfic default RGB value. Work around this by 
>>>> writing
>>>> +     * a RGB value of 0x010101 (faint white) instead.
>>>> +     */
>>>> +    if (data->kbd_led_mc_subled_info[0].intensity == 0 &&
>>>> +        data->kbd_led_mc_subled_info[1].intensity == 0 &&
>>>> +        data->kbd_led_mc_subled_info[2].intensity == 0)
>>>> +        min_intensity = 1;
>>>> +
>>>> +    for (int i = 0; i < KBD_LED_CHANNELS; i++) {
>>>> +        regval = max(data->kbd_led_mc_subled_info[i].intensity, 
>>>> min_intensity);
>>>> +        ret = regmap_write(data->regmap, 
>>>> uniwill_kbd_led_channel_to_reg[i], regval);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    ret = regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
>>>> RGB_APPLY_COLOR, RGB_APPLY_COLOR);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    return uniwill_kbd_led_write_brightness(data, brightness);
>>>> +}
>>>> +
>>>> +static enum led_brightness 
>>>> uniwill_kbd_led_mc_brightness_get(struct led_classdev *led_cdev)
>>>> +{
>>>> +    struct led_classdev_mc *led_mc_cdev = lcdev_to_mccdev(led_cdev);
>>>> +    struct uniwill_data *data = container_of(led_mc_cdev, struct 
>>>> uniwill_data, kbd_led_mc_cdev);
>>>> +
>>>> +    return uniwill_kbd_led_read_brightness(data);
>>>> +}
>>>> +
>>>> +static int uniwill_kbd_led_init(struct uniwill_data *data)
>>>> +{
>>>> +    unsigned int color_indices[KBD_LED_CHANNELS] = {
>>>> +        LED_COLOR_ID_RED,
>>>> +        LED_COLOR_ID_GREEN,
>>>> +        LED_COLOR_ID_BLUE,
>>>> +    };
>>>> +    struct led_init_data init_data = {
>>>> +        .devicename = DRIVER_NAME,
>>>> +        .devname_mandatory = true,
>>>> +    };
>>>> +    bool intensity_all_zeros = true;
>>>> +    bool needs_trigger = false;
>>>> +    unsigned int regval;
>>>> +    int ret;
>>>> +
>>>> +    if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +        return 0;
>>>> +
>>>> +    ret = regmap_read(data->regmap, EC_ADDR_SUPPORT_2, &regval);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    if (!(regval & CHINA_MODE)) {
>>>> +        ret = regmap_set_bits(data->regmap, EC_ADDR_BIOS_OEM_2, 
>>>> ENABLE_CHINA_MODE);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    ret = regmap_read(data->regmap, EC_ADDR_KBD_STATUS, &regval);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    regval |= KBD_APPLY;
>>>> +    regval &= ~KBD_POWER_OFF;
>>>> +    ret = regmap_write(data->regmap, EC_ADDR_KBD_STATUS, regval);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    switch (data->project_id) {
>>>> +    case PROJECT_ID_PF:
>>>> +    case PROJECT_ID_PF4MU_PF4MN_PF5MU:
>>>> +    case PROJECT_ID_PH4TRX1:
>>>> +    case PROJECT_ID_PH4TUX1:
>>>> +    case PROJECT_ID_PH4TQX1:
>>>> +    case PROJECT_ID_PH6TRX1:
>>>> +    case PROJECT_ID_PH6TQXX:
>>>> +    case PROJECT_ID_PHXAXXX:
>>>> +    case PROJECT_ID_PHXPXXX:
>>>> +        data->single_color_kbd = true;
>>>> +        break;
>>>> +    default:
>>>> +        data->single_color_kbd = regval & KBD_WHITE_ONLY;
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    if (data->single_color_kbd) {
>>>> +        init_data.default_label = "white:" 
>>>> LED_FUNCTION_KBD_BACKLIGHT;
>>>> +        data->kbd_led_cdev.max_brightness = 
>>>> data->kbd_led_max_brightness;
>>>> +        data->kbd_led_cdev.color = LED_COLOR_ID_WHITE;
>>>> +        data->kbd_led_cdev.flags = LED_BRIGHT_HW_CHANGED | 
>>>> LED_REJECT_NAME_CONFLICT;
>>>> +        data->kbd_led_cdev.brightness_set_blocking = 
>>>> uniwill_kbd_led_brightness_set;
>>>> +        data->kbd_led_cdev.brightness_get = 
>>>> uniwill_kbd_led_brightness_get;
>>>> +
>>>> +        return devm_led_classdev_register_ext(data->dev, 
>>>> &data->kbd_led_cdev, &init_data);
>>>> +    }
>>>> +
>>>> +    for (int i = 0; i < KBD_LED_CHANNELS; i++) {
>>>> +        data->kbd_led_mc_subled_info[i].color_index = 
>>>> color_indices[i];
>>>> +
>>>> +        ret = regmap_read(data->regmap, 
>>>> uniwill_kbd_led_channel_to_reg[i], &regval);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +
>>>> +        /*
>>>> +         * Make sure that the initial intensity value is not 
>>>> greater than
>>>> +         * the maximum intensity.
>>>> +         */
>>>> +        if (regval > KBD_LED_MAX_INTENSITY) {
>>>> +            regval = KBD_LED_MAX_INTENSITY;
>>>> +            ret = regmap_write(data->regmap, 
>>>> uniwill_kbd_led_channel_to_reg[i], regval);
>>>> +            if (ret < 0)
>>>> +                return ret;
>>>> +
>>>> +            needs_trigger = true;
>>>> +        }
>>>> +
>>>> +        if (regval)
>>>> +            intensity_all_zeros = false;
>>>> +
>>>> +        data->kbd_led_mc_subled_info[i].intensity = regval;
>>>> +        data->kbd_led_mc_subled_info[i].max_intensity = 
>>>> KBD_LED_MAX_INTENSITY;
>>>> +        data->kbd_led_mc_subled_info[i].channel = i;
>>>> +    }
>>>> +
>>>> +    /* See uniwill_kbd_led_mc_brightness_set() for an 
>>>> explaination. */
>>>> +    if (intensity_all_zeros) {
>>>> +        for (int i = 0; i < KBD_LED_CHANNELS; i++) {
>>>> +            data->kbd_led_mc_subled_info[i].intensity = 1;
>>>> +            ret = regmap_write(data->regmap, 
>>>> uniwill_kbd_led_channel_to_reg[i], 1);
>>>> +            if (ret < 0)
>>>> +                return ret;
>>>> +        }
>>>> +
>>>> +        needs_trigger = true;
>>>> +    }
>>>> +
>>>> +    if (needs_trigger) {
>>>> +        ret = regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
>>>> RGB_APPLY_COLOR,
>>>> +                    RGB_APPLY_COLOR);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    ret = devm_mutex_init(data->dev, &data->kbd_rgb_led_lock);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    init_data.default_label = "multicolor:" 
>>>> LED_FUNCTION_KBD_BACKLIGHT;
>>>> +    data->kbd_led_mc_cdev.led_cdev.max_brightness = 
>>>> data->kbd_led_max_brightness;
>>>> +    data->kbd_led_mc_cdev.led_cdev.color = LED_COLOR_ID_MULTI;
>>>> +    data->kbd_led_mc_cdev.led_cdev.flags = LED_BRIGHT_HW_CHANGED | 
>>>> LED_REJECT_NAME_CONFLICT;
>>>> + data->kbd_led_mc_cdev.led_cdev.brightness_set_blocking = 
>>>> uniwill_kbd_led_mc_brightness_set;
>>>> +    data->kbd_led_mc_cdev.led_cdev.brightness_get = 
>>>> uniwill_kbd_led_mc_brightness_get;
>>>> +    data->kbd_led_mc_cdev.subled_info = data->kbd_led_mc_subled_info;
>>>> +    data->kbd_led_mc_cdev.num_colors = KBD_LED_CHANNELS;
>>>> +
>>>> +    return devm_led_classdev_multicolor_register_ext(data->dev, 
>>>> &data->kbd_led_mc_cdev,
>>>> +                             &init_data);
>>>> +}
>>>> +
>>>
>>> Sorry I know I'm way to late with my comment, but I'm not at all a 
>>> fan of how complex this whole function is:
>>>
>>> - When we have to set max brightness on a per device basis anyway, 
>>> we can also set rgb or white only there and can skip this whole 
>>> detection mechanism with the supported bits that are unreliable anyway
>>>
>>> - we have a 2nd "supported devices/features list" inside this 
>>> function which is abstracting away from the actual supported feature 
>>> list
>>>
>>> Should i try to spin up something much simpler, but that requires 
>>> explicit setting in the device descriptor?
>>>
>>> Best regards
>>>
>>> Werner
>>>
>> Good point, but in this case i think having this small device list is 
>> OK, because the original control center
>> does something similar.
> yeah the original cc and tuxedo-drivers has these small lists all over 
> the place ... also don't like it there
>> If we encounter a device that does not work with this strategy then 
>> we can indeed
>> move the code into the device descriptor.
>
> later we might not be able to get the information anymore which 
> devices are rgb and which are white only when the supported devices 
> lists grows, then we are stuck to reading the feature bits.
>
> best regards,
>
> Werner
>
Good point, in this case using the device descriptor for this feature detection
might indeed be better in the long term.

Do you want to send the patches or should i do it?

Thanks,
Armin Wolf

>>
>> Thanks,
>> Armin Wolf
>>
>>>>   static unsigned int uniwill_sanitize_battery_threshold(unsigned 
>>>> int value)
>>>>   {
>>>>       /* 0 means "charging threshold not active" */
>>>> @@ -1789,6 +2082,31 @@ static int uniwill_notifier_call(struct 
>>>> notifier_block *nb, unsigned long action
>>>>           sysfs_notify(&data->dev->kobj, NULL, "fn_lock");
>>>>             return NOTIFY_OK;
>>>> +    case UNIWILL_OSD_KB_LED_LEVEL0:
>>>> +        if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +            return NOTIFY_DONE;
>>>> +
>>>> +        return notifier_from_errno(uniwill_notify_kbd_led(data, 0));
>>>> +    case UNIWILL_OSD_KB_LED_LEVEL1:
>>>> +        if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +            return NOTIFY_DONE;
>>>> +
>>>> +        return notifier_from_errno(uniwill_notify_kbd_led(data, 1));
>>>> +    case UNIWILL_OSD_KB_LED_LEVEL2:
>>>> +        if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +            return NOTIFY_DONE;
>>>> +
>>>> +        return notifier_from_errno(uniwill_notify_kbd_led(data, 2));
>>>> +    case UNIWILL_OSD_KB_LED_LEVEL3:
>>>> +        if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +            return NOTIFY_DONE;
>>>> +
>>>> +        return notifier_from_errno(uniwill_notify_kbd_led(data, 3));
>>>> +    case UNIWILL_OSD_KB_LED_LEVEL4:
>>>> +        if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +            return NOTIFY_DONE;
>>>> +
>>>> +        return notifier_from_errno(uniwill_notify_kbd_led(data, 4));
>>>>       default:
>>>>           mutex_lock(&data->input_lock);
>>>>           sparse_keymap_report_event(data->input_device, action, 1, 
>>>> true);
>>>> @@ -1842,6 +2160,7 @@ static int uniwill_ec_init(struct 
>>>> uniwill_data *data)
>>>>       if (ret < 0)
>>>>           return ret;
>>>>   +    data->project_id = value;
>>>>       dev_dbg(data->dev, "Project ID: %u\n", value);
>>>>         ret = regmap_set_bits(data->regmap, EC_ADDR_AP_OEM, 
>>>> ENABLE_MANUAL_CTRL);
>>>> @@ -1885,6 +2204,7 @@ static int uniwill_probe(struct 
>>>> platform_device *pdev)
>>>>           return ret;
>>>>         data->features = device_descriptor.features;
>>>> +    data->kbd_led_max_brightness = 
>>>> device_descriptor.kbd_led_max_brightness;
>>>>         /*
>>>>        * Some devices might need to perform some device-specific 
>>>> initialization steps
>>>> @@ -1905,6 +2225,10 @@ static int uniwill_probe(struct 
>>>> platform_device *pdev)
>>>>       if (ret < 0)
>>>>           return ret;
>>>>   +    ret = uniwill_kbd_led_init(data);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>>       ret = uniwill_hwmon_init(data);
>>>>       if (ret < 0)
>>>>           return ret;
>>>> @@ -1976,6 +2300,31 @@ static int uniwill_suspend_battery(struct 
>>>> uniwill_data *data)
>>>>       return regmap_read(data->regmap, EC_ADDR_CHARGE_CTRL, 
>>>> &data->last_charge_ctrl);
>>>>   }
>>>>   +static int uniwill_suspend_kbd_led(struct uniwill_data *data)
>>>> +{
>>>> +    unsigned int regval;
>>>> +    int ret;
>>>> +
>>>> +    if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +        return 0;
>>>> +
>>>> +    ret = regmap_read(data->regmap, EC_ADDR_KBD_STATUS, &regval);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    /*
>>>> +     * Save the current keyboard backlight settings in order to 
>>>> restore them
>>>> +     * during resume. We cannot use the regmap code for that since 
>>>> this register
>>>> +     * needs to be declared as volatile because the brightness can 
>>>> be changed
>>>> +     * by the EC.
>>>> +     */
>>>> +    data->last_kbd_status = regval;
>>>> +    FIELD_MODIFY(KBD_BRIGHTNESS_MASK, &regval, 0);
>>>> +    regval |= KBD_APPLY | KBD_POWER_OFF;
>>>> +
>>>> +    return regmap_write(data->regmap, EC_ADDR_KBD_STATUS, regval);
>>>> +}
>>>> +
>>>>   static int uniwill_suspend_nvidia_ctgp(struct uniwill_data *data)
>>>>   {
>>>>       if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_NVIDIA_CTGP_CONTROL))
>>>> @@ -2006,6 +2355,10 @@ static int uniwill_suspend(struct device *dev)
>>>>       if (ret < 0)
>>>>           return ret;
>>>>   +    ret = uniwill_suspend_kbd_led(data);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>>       ret = uniwill_suspend_nvidia_ctgp(data);
>>>>       if (ret < 0)
>>>>           return ret;
>>>> @@ -2052,6 +2405,23 @@ static int uniwill_resume_battery(struct 
>>>> uniwill_data *data)
>>>>       return 0;
>>>>   }
>>>>   +static int uniwill_resume_kbd_led(struct uniwill_data *data)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
>>>> +        return 0;
>>>> +
>>>> +    ret = regmap_write(data->regmap, EC_ADDR_KBD_STATUS, 
>>>> data->last_kbd_status | KBD_APPLY);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    if (data->single_color_kbd)
>>>> +        return 0;
>>>> +
>>>> +    return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
>>>> RGB_APPLY_COLOR, RGB_APPLY_COLOR);
>>>> +}
>>>> +
>>>>   static int uniwill_resume_nvidia_ctgp(struct uniwill_data *data)
>>>>   {
>>>>       if (!uniwill_device_supports(data, 
>>>> UNIWILL_FEATURE_NVIDIA_CTGP_CONTROL))
>>>> @@ -2096,6 +2466,10 @@ static int uniwill_resume(struct device *dev)
>>>>       if (ret < 0)
>>>>           return ret;
>>>>   +    ret = uniwill_resume_kbd_led(data);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>>       ret = uniwill_resume_nvidia_ctgp(data);
>>>>       if (ret < 0)
>>>>           return ret;
>>>> @@ -2745,6 +3119,8 @@ static int __init uniwill_init(void)
>>>>       if (force) {
>>>>           /* Assume that the device supports all features except 
>>>> the charge limit */
>>>>           device_descriptor.features = UINT_MAX & 
>>>> ~UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT;
>>>> +        /* Some models only support 3 brightness levels */
>>>> +        device_descriptor.kbd_led_max_brightness = 4;
>>>>           pr_warn("Enabling potentially unsupported features\n");
>>>>       }
>>>
>