Message-ID: <1b79a3c3-c493-471b-aa37-92458b356e8d@protonmail.com>
Date: Sun, 22 Jun 2025 21:37:41 +0000
From: Pőcze Barnabás <pobrn@...tonmail.com>
To: Armin Wolf <W_Armin@....de>, 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

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


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


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


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


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


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


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



> +	return regmap_read(data->regmap, EC_ADDR_CHARGE_CTRL, &data->last_charge_ctrl);
> +}
> [...]
> 


Regards,
Barnabás Pőcze

Powered by blists - more mailing lists