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? > >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(data->regmap, uniwill_led_channel_to_bat_reg[i], value); >>> + if (ret < 0) >>> + return ret; >>> + } >>> + >>> + if (brightness) >>> + value = 0; >>> + else >>> + value = LIGHTBAR_S0_OFF; >>> + >>> + ret = regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_AC_CTRL, LIGHTBAR_S0_OFF, value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_BAT_CTRL, LIGHTBAR_S0_OFF, value); >>> +} >>> + >>> +#define LIGHTBAR_MASK (LIGHTBAR_APP_EXISTS | LIGHTBAR_S0_OFF | LIGHTBAR_S3_OFF | LIGHTBAR_WELCOME) >>> + >>> +static int uniwill_led_init(struct uniwill_data *data) >>> +{ >>> + struct led_init_data init_data = { >>> + .devicename = DRIVER_NAME, >>> + .default_label = "multicolor:" LED_FUNCTION_STATUS, >>> + .devname_mandatory = true, >>> + }; >>> + unsigned int color_indices[3] = { >>> + LED_COLOR_ID_RED, >>> + LED_COLOR_ID_GREEN, >>> + LED_COLOR_ID_BLUE, >>> + }; >>> + unsigned int value; >>> + int ret; >>> + >>> + if (!(supported_features & UNIWILL_FEATURE_LIGHTBAR)) >>> + return 0; >>> + >>> + /* >>> + * The EC has separate lightbar settings for AC and battery mode, >>> + * so we have to ensure that both settings are the same. >>> + */ >>> + ret = regmap_read(data->regmap, EC_ADDR_LIGHTBAR_AC_CTRL, &value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + value |= LIGHTBAR_APP_EXISTS; >>> + ret = regmap_write(data->regmap, EC_ADDR_LIGHTBAR_AC_CTRL, value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + /* >>> + * The breathing animation during suspend is not supported when >>> + * running on battery power. >>> + */ >>> + value |= LIGHTBAR_S3_OFF; >>> + ret = regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_BAT_CTRL, LIGHTBAR_MASK, value); >>> + if (ret < 0) >>> + 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.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; >>> + >>> + for (int i = 0; i < LED_CHANNELS; i++) { >>> + data->led_mc_subled_info[i].color_index = color_indices[i]; >>> + >>> + ret = regmap_read(data->regmap, uniwill_led_channel_to_ac_reg[i], &value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + /* >>> + * Make sure that the initial intensity value is not greater than >>> + * the maximum brightness. >>> + */ >>> + value = min(LED_MAX_BRIGHTNESS, value); >>> + ret = regmap_write(data->regmap, uniwill_led_channel_to_ac_reg[i], value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(data->regmap, uniwill_led_channel_to_bat_reg[i], value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + data->led_mc_subled_info[i].intensity = value; >>> + data->led_mc_subled_info[i].channel = i; >>> + } >>> + >>> + data->led_mc_cdev.subled_info = data->led_mc_subled_info; >>> + data->led_mc_cdev.num_colors = LED_CHANNELS; >>> + >>> + return devm_led_classdev_multicolor_register_ext(&data->wdev->dev, &data->led_mc_cdev, >>> + &init_data); >>> +} >>> [...] >>> +static const enum power_supply_property uniwill_properties[] = { >>> + POWER_SUPPLY_PROP_HEALTH, >>> + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, >>> +}; >>> + >>> +static const struct power_supply_ext uniwill_extension = { >>> + .name = DRIVER_NAME, >>> + .properties = uniwill_properties, >>> + .num_properties = ARRAY_SIZE(uniwill_properties), >>> + .get_property = uniwill_get_property, >>> + .set_property = uniwill_set_property, >>> + .property_is_writeable = uniwill_property_is_writeable, >>> +}; >>> + >>> +static int uniwill_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook) >> What is the motivation for supporting multiple batteries? >> There is still just a single parameter in the EC/etc. > > I simply assume that devices using this EC interface will only ever support a single battery anyway, > as the EC will be unable to handle more than that. I see, I was just wondering if all this code is needed. But I suppose you can't remove much more. > >>> +{ >>> + struct uniwill_data *data = container_of(hook, struct uniwill_data, hook); >>> + struct uniwill_battery_entry *entry; >>> + int ret; >>> + >>> + entry = kzalloc(sizeof(*entry), GFP_KERNEL); >>> + if (!entry) >>> + return -ENOMEM; >>> + >>> + ret = power_supply_register_extension(battery, &uniwill_extension, &data->wdev->dev, data); >>> + if (ret < 0) { >>> + kfree(entry); >>> + return ret; >>> + } >>> + >>> + scoped_guard(mutex, &data->battery_lock) { >>> + entry->battery = battery; >>> + list_add(&entry->head, &data->batteries); >>> + } >>> + >>> + return 0; >>> +} >>> [...] >>> +static int uniwill_ec_init(struct uniwill_data *data) >>> +{ >>> + unsigned int value; >>> + int ret; >>> + >>> + ret = regmap_read(data->regmap, EC_ADDR_PROJECT_ID, &value); >>> + if (ret < 0) >>> + return ret; >>> + >>> + dev_dbg(&data->wdev->dev, "Project ID: %u\n", value); >>> + >>> + ret = regmap_set_bits(data->regmap, EC_ADDR_AP_OEM, ENABLE_MANUAL_CTRL); >> I think this might turn out to be problematic. If this is enabled, then multiple >> things are not handled automatically. For example, keyboard backlight is not handled >> and as far as I can see, no `KEY_KBDILLUM{DOWN,UP}` are sent (events 0xb1, 0xb2). The >> other thing is the "performance mode" button (event 0xb0). I don't see that these two >> things would be handled in the driver. > > On the intel NUC notebooks the keyboard backlight is controlled by a separate USB device, > so the driver only has to forward the KEY_KBDILLUMTOGGLE event to userspace (the > KEY_KBDILLUM{DOWN,UP} events are not supported on those devices). This happens inside the > uniwill-wmi driver. An USB HID device controls the keyboard backlight on my laptop as well, but the brightness up/down requests arrive via this WMI mechanism. > > Once we add support for devices where the EC also controls the keyboard backlight i will > add support for those events. I am also planning to add support for the performance mode > event later. Currently the EC does not change the performance mode itself even when in > automatic mode (at least on intel NUC notebooks). Interesting, it does on mine... > > Since this driver relies on a DMI whitelist i think that we can safely add support for those > feature later when new devices are added to those list. > >>> + if (ret < 0) >>> + return ret; >>> + >>> + return devm_add_action_or_reset(&data->wdev->dev, uniwill_disable_manual_control, data); >>> +} >>> + > [...] >>> +static int uniwill_suspend_battery(struct uniwill_data *data) >>> +{ >>> + if (!(supported_features & UNIWILL_FEATURE_BATTERY)) >>> + return 0; >>> + >>> + /* >>> + * Save the current charge limit in order to restore it during resume. >>> + * We cannot use the regmap code for that since this register needs to >>> + * be declared as volatile due to CHARGE_CTRL_REACHED. >>> + */ >> What is the motivation for this? I found that this is not needed, I found that >> the value is persistent. > > The OEM application i reverse-engineered did restore this value after a resume, so > i decided to replicate this behavior. I suppose it can't hurt. > [...] Regards, Barnabás Pőcze
