Message-ID: <7b0243fd-15c6-42da-8570-9ad9cd5163af@gmx.de>
Date: Mon, 23 Jun 2025 00:36:59 +0200
From: Armin Wolf <W_Armin@....de>
To: Pőcze Barnabás <pobrn@...tonmail.com>,
 ilpo.jarvinen@...ux.intel.com, hdegoede@...hat.com, chumuzero@...il.com,
 corbet@....net, cs@...edo.de, wse@...edocomputers.com,
 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 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?

>> - 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
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-uniwill-laptop b/Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>> new file mode 100644
>> index 000000000000..a4781a118906
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>> [...]
>> diff --git a/Documentation/wmi/devices/uniwill-laptop.rst b/Documentation/wmi/devices/uniwill-laptop.rst
>> new file mode 100644
>> index 000000000000..2be598030a5e
>> --- /dev/null
>> +++ b/Documentation/wmi/devices/uniwill-laptop.rst
>> @@ -0,0 +1,109 @@
>> +.. SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +============================================
>> +Uniwill WMI Notebook driver (uniwill-laptop)
>> +============================================
>> +
>> +Introduction
>> +============
>> +
>> +Many notebooks manufactured by Uniwill (either directly or as ODM) provide an WMI-based
>> +EC interface for controlling various platform settings like sensors and fan control.
>> +This interface is used by the ``uniwill-laptop`` driver to map those features onto standard
>> +kernel interfaces.
>> +
>> +WMI interface description
>> +=========================
>> +
>> +The WMI interface description can be decoded from the embedded binary MOF (bmof)
>> +data using the `bmfdec <https://github.com/pali/bmfdec>`_ utility:
>> +
>> +::
>> +
>> +  [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"),
>> +   Description("Class used to operate methods on a ULong"),
>> +   guid("{ABBC0F6F-8EA1-11d1-00A0-C90629100000}")]
>> +  class AcpiTest_MULong {
>> +    [key, read] string InstanceName;
>> +    [read] boolean Active;
>> +
>> +    [WmiMethodId(1), Implemented, read, write, Description("Return the contents of a ULong")]
>> +    void GetULong([out, Description("Ulong Data")] uint32 Data);
>> +
>> +    [WmiMethodId(2), Implemented, read, write, Description("Set the contents of a ULong")]
>> +    void SetULong([in, Description("Ulong Data")] uint32 Data);
>> +
>> +    [WmiMethodId(3), Implemented, read, write,
>> +     Description("Generate an event containing ULong data")]
>> +    void FireULong([in, Description("WMI requires a parameter")] uint32 Hack);
>> +
>> +    [WmiMethodId(4), Implemented, read, write, Description("Get and Set the contents of a ULong")]
>> +    void GetSetULong([in, Description("Ulong Data")] uint64 Data,
>> +                     [out, Description("Ulong Data")] uint32 Return);
>> +
>> +    [WmiMethodId(5), Implemented, read, write,
>> +     Description("Get and Set the contents of a ULong for Dollby button")]
>> +    void GetButton([in, Description("Ulong Data")] uint64 Data,
>> +                   [out, Description("Ulong Data")] uint32 Return);
>> +  };
>> +
>> +Most of the WMI-related code was copied from the Windows driver samples, which unfortunately means
>> +that the WMI-GUID is not unique. This makes the WMI-GUID unusable for autoloading.
>> +
>> +WMI method GetULong()
>> +---------------------
>> +
>> +This WMI method was copied from the Windows driver samples and has no function.
>> +
>> +WMI method SetULong()
>> +---------------------
>> +
>> +This WMI method was copied from the Windows driver samples and has no function.
>> +
>> +WMI method FireULong()
>> +----------------------
>> +
>> +This WMI method allows to inject a WMI event with a 32-bit payload. Its primary purpose seems
>> +to be debugging.
>> +
>> +WMI method GetSetULong()
>> +------------------------
>> +
>> +This WMI method is used to communicate with the EC. The ``Data`` argument hold the following
>> +information (starting with the least significant byte):
>> +
>> +1. 16-bit address
>> +2. 16-bit data (set to ``0x0000`` when reading)
>> +3. 16-bit operation (``0x0100`` for reading and ``0x0000`` for writing)
>> +4. 16-bit reserved (set to ``0x0000``)
>> +
>> +The first 8 bits of the ``Return`` value contain the data returned by the EC when reading.
>> +The special value ``0xFEFEFEFE`` is used to indicate a communication failure with the EC.
> I think that should be 0xFEFEFEFEFEFEFEFE.

The ACPI firmware itself indeed returns 0xFEFEFEFEFEFEFEFE, but the WMI method itself only returns
a 32-bit value. This is the reason why i am only checking for 0xFEFEFEFE.

>> +
>> +WMI method GetButton()
>> +----------------------
>> +
>> +This WMI method is not implemented on all machines and has an unknown purpose.
>> +
>> +Reverse-Engineering the Uniwill WMI interface
>> +=============================================
>> +
>> +.. warning:: Randomly poking the EC can potentially cause damage to the machine and other unwanted
>> +             side effects, please be careful.
>> +
>> +The EC behind the ``GetSetULong`` method is used by the OEM software supplied by the manufacturer.
>> +Reverse-engineering of this software is difficult since it uses an obfuscator, however some parts
>> +are not obfuscated.
> I used https://github.com/dnSpy/dnSpy to attach to it when running, which made
> it quite simple to access the underlying byte code. Of course if you don't have
> the hardware, then that is difficult to do...

Interesting, i will add that to the documentation as well.

>> +
>> +The EC can be accessed under Windows using powershell (requires admin privileges):
>> +
>> +::
>> +
>> +  > $obj = Get-CimInstance -Namespace root/wmi -ClassName AcpiTest_MULong | Select-Object -First 1
>> +  > Invoke-CimMethod -InputObject $obj -MethodName GetSetULong -Arguments @{Data = <input>}
>> +
>> +Special thanks go to github user `pobrn` which developed the
>> +`qc71_laptop <https://github.com/pobrn/qc71_laptop>`_ driver on which this driver is partly based.
>> +The same is true for Tuxedo Computers, which developed the
>> +`tuxedo-drivers <https://github.com/tuxedocomputers/tuxedo-drivers>`_ package which also served as
>> +a foundation for this driver.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 53876ec2d111..5b12cc498d56 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -25496,6 +25496,14 @@ L:	linux-scsi@...r.kernel.org
>>    S:	Maintained
>>    F:	drivers/ufs/host/ufs-renesas.c
>>
>> +UNIWILL LAPTOP DRIVER
>> +M:	Armin Wolf <W_Armin@....de>
>> +L:	platform-driver-x86@...r.kernel.org
>> +S:	Maintained
>> +F:	Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>> +F:	Documentation/wmi/devices/uniwill-laptop.rst
>> +F:	drivers/platform/x86/uniwill/uniwill-laptop.c
>> +
>>    UNIWILL WMI DRIVER
>>    M:	Armin Wolf <W_Armin@....de>
>>    L:	platform-driver-x86@...r.kernel.org
>> [...]
>> +
>> +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.

>> +		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.

>> +{
>> +	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.

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).

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_probe(struct wmi_device *wdev, const void *context)
>> +{
>> +	struct uniwill_data *data;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->wdev = wdev;
>> +	dev_set_drvdata(&wdev->dev, data);
>> +
>> +	regmap = devm_regmap_init(&wdev->dev, &uniwill_ec_bus, data, &uniwill_ec_config);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	data->regmap = regmap;
>> +	ret = devm_mutex_init(&wdev->dev, &data->super_key_lock);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = uniwill_ec_init(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = uniwill_battery_init(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = uniwill_led_init(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = uniwill_hwmon_init(data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return uniwill_notifier_init(data);
>> +}
>> +
>> +static void uniwill_shutdown(struct wmi_device *wdev)
>> +{
>> +	struct uniwill_data *data = dev_get_drvdata(&wdev->dev);
>> +
>> +	regmap_clear_bits(data->regmap, EC_ADDR_AP_OEM, ENABLE_MANUAL_CTRL);
>> +}
>> +
>> +static int uniwill_suspend_keyboard(struct uniwill_data *data)
>> +{
>> +	if (!(supported_features & UNIWILL_FEATURE_SUPER_KEY_LOCK))
>> +		return 0;
>> +
>> +	/*
>> +	 * The EC_ADDR_SWITCH_STATUS is maked as volatile, so we have to restore it
>> +	 * ourself.
>> +	 */
>> +	return regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &data->last_switch_status);
>> +}
>> +
>> +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.

Thanks,
Armin Wolf

>> +	return regmap_read(data->regmap, EC_ADDR_CHARGE_CTRL, &data->last_charge_ctrl);
>> +}
>> [...]
>>
>
> Regards,
> Barnabás Pőcze
>

Powered by blists - more mailing lists