Re: [PATCH v2 2/3] platform/x86: uniwill-laptop: Remove single color keyboard detection

[Armin Wolf]

Am 15.07.26 um 17:16 schrieb Werner Sembach:

> Am 11.07.26 um 00:35 schrieb Armin Wolf:
> > It turns out that the code for detecting single color keyboards
> > might not work correctly on future models. Remove it and use the
> > device descriptor instead.
> Technically: It already doesn't work reliable on old devices, that's 
> why the device list inside the init function was required. This patch 
> is basically to move this to the device_descriptor struct that is 
> already present.

Good catch, i will reword this.

> > Suggested-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > ---
> >   drivers/platform/x86/uniwill/uniwill-acpi.c | 32 +++++++--------------
> >   1 file changed, 11 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
> > b/drivers/platform/x86/uniwill/uniwill-acpi.c
> > index d27f316800f6..4591ee299a90 100644
> > --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> > +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> > @@ -255,6 +255,7 @@
> >   #define FAN_CURVE_LENGTH        5
> >     #define EC_ADDR_KBD_STATUS        0x078C
> > +/* Unreliable */
> >   #define KBD_WHITE_ONLY            BIT(0)
> >   #define KBD_POWER_OFF            BIT(1)
> >   #define KBD_TURBO_LEVEL_MASK        GENMASK(3, 2)
> > @@ -400,7 +401,7 @@ struct uniwill_data {
> >       u8 lightbar_max_brightness;
> >       struct led_classdev_mc led_mc_cdev;
> >       struct mc_subled led_mc_subled_info[LED_CHANNELS];
> > -    bool single_color_kbd;
> > +    bool kbd_led_single_color;
> >       u8 kbd_led_max_brightness;
> >       unsigned int last_kbd_status;
> >       union {
> > @@ -426,6 +427,7 @@ struct uniwill_battery_entry {
> >     struct uniwill_device_descriptor {
> >       unsigned int features;
> > +    bool kbd_led_single_color;
>
> Why do this as a bool instead of a UNIWILL_FEATURE_* define? We 
> already have mutually exclusive UNIWILL_FEATURE_* defines for the 
> charging modes.
>
> Best regards
>
> Werner

I decided against that because both feature defines would effectively control the same hardware registers.
Instead i decided to have a common code path for the whole keyboard backlight feature and use a flag for
the RGB support instead.

This also integrates better with the existing code IMHO.

Thanks,
Armin Wolf

> >       u8 kbd_led_max_brightness;
> >       u8 lightbar_max_brightness;
> >       /* Executed during driver probing */
> > @@ -1629,7 +1631,7 @@ static int uniwill_notify_kbd_led(struct 
> > uniwill_data *data, int brightness)
> >       struct led_classdev *led_cdev;
> >       int ret;
> >   -    if (data->single_color_kbd)
> > +    if (data->kbd_led_single_color)
> >           led_cdev = &data->kbd_led_cdev;
> >       else
> >           led_cdev = &data->kbd_led_mc_cdev.led_cdev;
> > @@ -1858,24 +1860,7 @@ static int uniwill_kbd_led_init(struct 
> > uniwill_data *data)
> >       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)
> > +    if (data->kbd_led_single_color)
> >           return uniwill_white_kbd_led_init(data);
> >         return uniwill_rgb_kbd_led_init(data);
> > @@ -2351,6 +2336,7 @@ static int uniwill_probe(struct platform_device 
> > *pdev)
> >           return ret;
> >         data->features = device_descriptor.features;
> > +    data->kbd_led_single_color = 
> > device_descriptor.kbd_led_single_color;
> >       data->kbd_led_max_brightness = 
> > device_descriptor.kbd_led_max_brightness;
> >       data->lightbar_max_brightness = 
> > device_descriptor.lightbar_max_brightness;
> >   @@ -2580,7 +2566,7 @@ static int uniwill_resume_kbd_led(struct 
> > uniwill_data *data)
> >       if (ret < 0)
> >           return ret;
> >   -    if (data->single_color_kbd)
> > +    if (data->kbd_led_single_color)
> >           return 0;
> >         return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
> > RGB_APPLY_COLOR, RGB_APPLY_COLOR);
> > @@ -2687,6 +2673,7 @@ static struct uniwill_device_descriptor 
> > machenike_l16p_descriptor __initdata = {
> >               UNIWILL_FEATURE_KEYBOARD_BACKLIGHT |
> >               UNIWILL_FEATURE_AC_AUTO_BOOT |
> >               UNIWILL_FEATURE_USB_POWERSHARE,
> > +    .kbd_led_single_color = false,
> >       .kbd_led_max_brightness = 4,
> >   };
> >   @@ -2869,6 +2856,7 @@ static struct uniwill_device_descriptor 
> > x4sp4nal_descriptor __initdata = {
> >               UNIWILL_FEATURE_KEYBOARD_BACKLIGHT |
> >               UNIWILL_FEATURE_AC_AUTO_BOOT |
> >               UNIWILL_FEATURE_USB_POWERSHARE,
> > +    .kbd_led_single_color = true,
> >       .kbd_led_max_brightness = 2,
> >   };
> >   @@ -3363,6 +3351,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 have a (white) single color keyboard 
> > backlight */
> > +        device_descriptor.kbd_led_single_color = false;
> >           /* Some models only support 3 brightness levels */
> >           device_descriptor.kbd_led_max_brightness = 4;
> >           /* Some models only support 36 brightness levels per color 
> > component */