Message-ID: <e167ed33-010d-4cdb-ae53-4afeb3efdea7@gmx.de>
Date: Mon, 30 Jun 2025 14:40:35 +0200
From: Armin Wolf <W_Armin@....de>
To: Werner Sembach <wse@...edocomputers.com>,
 Pőcze Barnabás <pobrn@...tonmail.com>,
 ilpo.jarvinen@...ux.intel.com, hdegoede@...hat.com, chumuzero@...il.com,
 corbet@....net, cs@...edo.de, ggo@...edocomputers.com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 platform-driver-x86@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] platform/x86: Add Uniwill laptop driver

Am 30.06.25 um 14:32 schrieb Werner Sembach:

> Hi,
>
> Am 28.06.25 um 01:09 schrieb Armin Wolf:
>> Am 25.06.25 um 17:59 schrieb Pőcze Barnabás:
>>
>>> Hi
>>>
>>> 2025. 06. 23. 0:36 keltezéssel, Armin Wolf írta:
>>>> Am 22.06.25 um 23:37 schrieb Pőcze Barnabás:
>>>>
>>>>> Hi
>>>>>
>>>>>
>>>>> 2025. 06. 15. 19:59 keltezéssel, Armin Wolf írta:
>>>>>> Add a new driver for Uniwill laptops. The driver uses a ACPI WMI
>>>>>> interface to talk with the embedded controller, but relies on a
>>>>>> DMI whitelist for autoloading since Uniwill just copied the WMI
>>>>>> GUID from the Windows driver example.
>>>>>>
>>>>>> The driver is reverse-engineered based on the following information:
>>>>>> - OEM software from intel
>>>>>> - https://github.com/pobrn/qc71_laptop
>>>>> Oh... I suppose an end of an era for me...
>>>> I now remember that we interacted on the mailing lists before, 
>>>> sorry for not CCing
>>>> you on this patch series.
>>>>
>>>> Do you want a Co-developed-by tag on those patches?
>>> I'll leave it up to you.
>>>
>>>
>>>>>> - https://github.com/tuxedocomputers/tuxedo-drivers
>>>>>> - https://github.com/tuxedocomputers/tuxedo-control-center
>>>>>>
>>>>>> The underlying EC supports various features, including hwmon 
>>>>>> sensors,
>>>>>> battery charge limiting, a RGB lightbar and keyboard-related 
>>>>>> controls.
>>>>>>
>>>>>> Reported-by: cyear <chumuzero@...il.com>
>>>>>> Closes: https://github.com/lm-sensors/lm-sensors/issues/508
>>>>>> Closes: https://github.com/Wer-Wolf/uniwill-laptop/issues/3
>>>>>> Signed-off-by: Armin Wolf <W_Armin@....de>
>>>>>> ---
>>>>>>      .../ABI/testing/sysfs-driver-uniwill-laptop   | 53 +
>>>>>>      Documentation/wmi/devices/uniwill-laptop.rst  | 109 ++
>>>>>>      MAINTAINERS                                   | 8 +
>>>>>>      drivers/platform/x86/uniwill/Kconfig          | 17 +
>>>>>>      drivers/platform/x86/uniwill/Makefile         | 1 +
>>>>>>      drivers/platform/x86/uniwill/uniwill-laptop.c | 1477 
>>>>>> +++++++++++++++++
>>>>>>      drivers/platform/x86/uniwill/uniwill-wmi.c    | 3 +-
>>>>>>      7 files changed, 1667 insertions(+), 1 deletion(-)
>>>>>>      create mode 100644 
>>>>>> Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>>>>>>      create mode 100644 Documentation/wmi/devices/uniwill-laptop.rst
>>>>>>      create mode 100644 
>>>>>> drivers/platform/x86/uniwill/uniwill-laptop.c
>>>>>>
>>>> [...]
>>>>>> +
>>>>>> +static const unsigned int 
>>>>>> uniwill_led_channel_to_bat_reg[LED_CHANNELS] = {
>>>>>> +    EC_ADDR_LIGHTBAR_BAT_RED,
>>>>>> +    EC_ADDR_LIGHTBAR_BAT_GREEN,
>>>>>> +    EC_ADDR_LIGHTBAR_BAT_BLUE,
>>>>>> +};
>>>>>> +
>>>>>> +static const unsigned int 
>>>>>> uniwill_led_channel_to_ac_reg[LED_CHANNELS] = {
>>>>>> +    EC_ADDR_LIGHTBAR_AC_RED,
>>>>>> +    EC_ADDR_LIGHTBAR_AC_GREEN,
>>>>>> +    EC_ADDR_LIGHTBAR_AC_BLUE,
>>>>>> +};
>>>>>> +
>>>>>> +static int uniwill_led_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, led_mc_cdev);
>>>>>> +    unsigned int value;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = led_mc_calc_color_components(led_mc_cdev, brightness);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    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);
>>>>>> +        ret = regmap_write(data->regmap, 
>>>>>> uniwill_led_channel_to_ac_reg[i], value);
>>>>> This is interesting. I am not sure which "control center" 
>>>>> application you have looked at,
>>>>> but I found many lookup tables based on the exact model, etc. For 
>>>>> example, on my laptop
>>>>> any value larger than 36 will simply turn that color component 
>>>>> off. Have you seen
>>>>> anything like that?
>>>> I was using the Intel NUC studio software application during 
>>>> reverse-engineering and had a user
>>>> test the resulting code on a Intel NUC notebook. AFAIK the OEM 
>>>> software did not use a lookup table.
>>>>
>>>> If we extend this driver in the future then we might indeed use the 
>>>> quirk system to change the max.
>>>> LED brightness depending on the model.
>>> I see. So everything up to 200 works. And after that do you know if 
>>> it turns off or what happens?
>>
>> The user who tested the driver reported that "the brightest lightbar 
>> setting is 200", so i assume
>> that the lightbar simply clamps the values. However i would not trust 
>> the EC firmware in the slightest,
>> i can definitely imagine that other models react differently.
>
> Iirc at least for keyboard backlight on tf devices there was a value 
> that could be overwritten to make the values 0-255 instead of 0-200, 
> maybe this is also true for the lightbar, but i don't know if this 
> affects the livespan of the leds.
>
> Best regards,
>
> Werner
>
Interesting, do you know the register offset of this value?

Thanks,
Armin Wolf

Powered by blists - more mailing lists