Message-ID: <645bc973-bc0e-4e2f-9df1-6670892e7f32@gmx.de>
Date: Mon, 30 Jun 2025 15:21:26 +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:55 schrieb Werner Sembach:

>
> Am 30.06.25 um 14:40 schrieb Armin Wolf:
>> 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?
>
> Not out of my head, would have to dig for it again or even re reverse 
> engineer it again as I'm not sure if I wrote it down ...
>
> So if it is not required I would like to leave it at that and only do 
> the work if it turns out to be needed.
>
Fine with me.

>>
>> Thanks,
>> Armin Wolf
>>
>

Powered by blists - more mailing lists