Message-ID: <7b29df39-8146-4913-83ff-d71db26983c8@gmx.de>
Date: Sat, 28 Jun 2025 01:09:52 +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 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@...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?

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.

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

Alright, i will add support for those two events as well. Maybe this helps when adding new devices
to this driver in the future.

Thanks,
Armin Wolf

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