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

[Armin Wolf <W_Armin@gmx.de>]

Am 09.07.26 um 15:23 schrieb Werner Sembach:

> [...]
>>>>>> +
>>>>>> +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?
>
> I will not stop you if you have time to do it ^^
>
> you are probably quicker in working on your own code
>
> i would do a
>
> UNIWILL_FEATURE_KBL_WHITE_ONLY and a
>
> UNIWILL_FEATURE_KBL_ONE_ZONE_RGB (there are also Uniwill devices with 
> per-key RGB, but that is controlled via a usb device)
>
> also required is a way to set max intensity via the device descriptor, 
> most tuxedo devices have 200 as max value there and not 50
>
> best regards,
>
> Werner
>
Alright, i will send the necessary patches soon.

Thanks,
Armin Wolf

>>
>> Thanks,
>> Armin Wolf 
> [...]
>