Message-ID: <7a58972f-5256-4598-b729-224f20f3ecd2@protonmail.com>
Date: Wed, 25 Jun 2025 15:59:52 +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. 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?



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

Powered by blists - more mailing lists