Re: [RFC PATCH 2/3] platform/x86: Add Uniwill laptop driver

[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@xxxxxxxxx>
> > > > > > > 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@xxxxxx>
> > > > > > > ---
> > > > > > >      .../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.

> Thanks,
> Armin Wolf