Re: [RFC PATCH 2/3] platform/x86: uniwill-laptop: Implement lightbar for XMG Fusion (L19)

[Armin Wolf]

Am 22.04.26 um 13:17 schrieb Armin Wolf:
> Am 21.04.26 um 22:01 schrieb Werner Sembach:
> > The XMG Fusion (L19) also has a lightbar but with a different max
> > brightness value of 36 instead of 200.
> >
> > This patch adds a new feature flag for the driver to correctly handle 
> > this
> > case and applies it to the mentioned device.
> >
> > Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/platform/x86/uniwill/uniwill-acpi.c | 19 +++++++++++++------
> >   1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/ 
> > platform/x86/uniwill/uniwill-acpi.c
> > index 7d1fdbacc6871..62d56cc67e2e8 100644
> > --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> > +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> > @@ -318,7 +318,6 @@
> >   #define FAN_TABLE_LENGTH    16
> >   #define LED_CHANNELS        3
> > -#define LED_MAX_BRIGHTNESS    200
> >   #define UNIWILL_FEATURE_FN_LOCK            BIT(0)
> >   #define UNIWILL_FEATURE_SUPER_KEY        BIT(1)
> > @@ -344,6 +343,7 @@ struct uniwill_data {
> >       acpi_handle handle;
> >       struct regmap *regmap;
> >       unsigned int features;
> > +    u8 lightbar_max_brightness;
> >       struct acpi_battery_hook hook;
> >       struct mutex battery_lock;    /* Protects the list of currently 
> > registered batteries */
> >       union {
> > @@ -376,6 +376,7 @@ struct uniwill_battery_entry {
> >   struct uniwill_device_descriptor {
> >       unsigned int features;
> > +    u8 lightbar_max_brightness;
> >       /* Executed during driver probing */
> >       int (*probe)(struct uniwill_data *data);
> >   };
> > @@ -1340,7 +1341,7 @@ static int uniwill_led_brightness_set(struct 
> > led_classdev *led_cdev, enum led_br
> >       for (int i = 0; i < LED_CHANNELS; i++) {
> >           /* Prevent the brightness values from overflowing */
> > -        value = min(LED_MAX_BRIGHTNESS, data- 
> > >led_mc_subled_info[i].brightness);
> > +        value = min(data->lightbar_max_brightness, data- 
> > >led_mc_subled_info[i].brightness);
> >           ret = regmap_write(data->regmap, 
> > uniwill_led_channel_to_ac_reg[i], value);
> >           if (ret < 0)
> >               return ret;
> > @@ -1376,12 +1377,14 @@ static int uniwill_led_init(struct 
> > uniwill_data *data)
> >           LED_COLOR_ID_GREEN,
> >           LED_COLOR_ID_BLUE,
> >       };
> > -    unsigned int value;
> > +    unsigned int value, max_brightness;
> >       int ret;
> >       if (!uniwill_device_supports(data, UNIWILL_FEATURE_LIGHTBAR))
> >           return 0;
> > +    max_brightness = data->lightbar_max_brightness;
>
> Please drop the max_brightness variable and assign 
> led_cdev.max_brightness and led_cdev.brightness directly.
>
> > +
> >       ret = devm_mutex_init(data->dev, &data->led_lock);
> >       if (ret < 0)
> >           return ret;
> > @@ -1409,14 +1412,14 @@ static int uniwill_led_init(struct 
> > uniwill_data *data)
> >           return ret;
> >       data->led_mc_cdev.led_cdev.color = LED_COLOR_ID_MULTI;
> > -    data->led_mc_cdev.led_cdev.max_brightness = LED_MAX_BRIGHTNESS;
> > +    data->led_mc_cdev.led_cdev.max_brightness = max_brightness;
> >       data->led_mc_cdev.led_cdev.flags = LED_REJECT_NAME_CONFLICT;
> >       data->led_mc_cdev.led_cdev.brightness_set_blocking = 
> > uniwill_led_brightness_set;
> >       if (value & LIGHTBAR_S0_OFF)
> >           data->led_mc_cdev.led_cdev.brightness = 0;
> >       else
> > -        data->led_mc_cdev.led_cdev.brightness = LED_MAX_BRIGHTNESS;
> > +        data->led_mc_cdev.led_cdev.brightness = max_brightness;
> >       for (int i = 0; i < LED_CHANNELS; i++) {
> >           data->led_mc_subled_info[i].color_index = color_indices[i];
> > @@ -1429,7 +1432,7 @@ static int uniwill_led_init(struct uniwill_data 
> > *data)
> >            * Make sure that the initial intensity value is not greater 
> > than
> >            * the maximum brightness.
> >            */
> > -        value = min(LED_MAX_BRIGHTNESS, value);
> > +        value = min(max_brightness, value);
> >           ret = regmap_write(data->regmap, 
> > uniwill_led_channel_to_ac_reg[i], value);
> >           if (ret < 0)
> >               return ret;
> > @@ -1882,6 +1885,7 @@ static int uniwill_probe(struct platform_device 
> > *pdev)
> >           return ret;
> >       data->features = device_descriptor.features;
> > +    data->lightbar_max_brightness = 
> > device_descriptor.lightbar_max_brightness;
> >       /*
> >        * Some devices might need to perform some device-specific 
> > initialization steps
> > @@ -2130,11 +2134,13 @@ static struct platform_driver uniwill_driver = {
> >   static struct uniwill_device_descriptor lapqc71a_lapqc71b_descriptor 
> > __initdata = {
> >       .features = UNIWILL_FEATURE_SUPER_KEY |
> > +            UNIWILL_FEATURE_LIGHTBAR |
> >               UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT |
> >               UNIWILL_FEATURE_CPU_TEMP |
> >               UNIWILL_FEATURE_GPU_TEMP |
> >               UNIWILL_FEATURE_PRIMARY_FAN |
> >               UNIWILL_FEATURE_SECONDARY_FAN,
> > +    .lightbar_max_brightness = 36,
> >   };
> >   static struct uniwill_device_descriptor lapac71h_descriptor 
> > __initdata = {
> > @@ -2158,6 +2164,7 @@ static struct uniwill_device_descriptor 
> > lapkc71f_descriptor __initdata = {
> >               UNIWILL_FEATURE_GPU_TEMP |
> >               UNIWILL_FEATURE_PRIMARY_FAN |
> >               UNIWILL_FEATURE_SECONDARY_FAN,
> > +    .lightbar_max_brightness = 200,
>
> Please also set lightbar_max_brightness to 200 during module 
> initialization when the "force" parameter is set.
>
> I suggest you rebase this series on top of the "platform/x86: uniwill- 
> laptop: Charging-related improvements" series to avoid merge conflicts.

Forget about the rebase part, the series is already based on this series xd.

Thanks,
Armin Wolf

> Thanks,
> Armin Wolf
>
> >   };
> >   /*