Re: [PATCH v2 2/5] platform/x86: uniwill-laptop: Implement rainbow animation as trigger

[Armin Wolf <W_Armin@gmx.de>]

Am 10.10.26 um 13:20 schrieb Rong Zhang:

> Hi Armin,
>
> On Wed, 2026-10-07 at 16:38 +0200, Armin Wolf wrote:
>> It turns out that some models require the lightbar to be activated
>> in order to display the rainbow animation, so users have to access
>> both the "rainbow_animation" sysfs attribute and the LED sysfs
>> interface when enabling the rainbow animation. This can cause short
>> glitches when the lightbar was previously disabled.
>>
>> Fix this by replacing the sysfs attribute with a custom LED trigger
>> called "uniwill-rainbow". Selecting this trigger will atomically
>> activate the lightbar and enable the rainbow animation mode. This
>> can potentially break userspace applications using the old sysfs
>> attribute, but so far no such application is known to exist (yet).
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   .../ABI/removed/sysfs-driver-uniwill-laptop   |  10 ++
>>   .../ABI/testing/sysfs-driver-uniwill-laptop   |  11 --
>>   .../admin-guide/laptops/uniwill-laptop.rst    |   5 +-
>>   MAINTAINERS                                   |   1 +
>>   drivers/platform/x86/uniwill/Kconfig          |   1 +
>>   drivers/platform/x86/uniwill/uniwill-acpi.c   | 133 +++++++++++-------
>>   6 files changed, 95 insertions(+), 66 deletions(-)
>>   create mode 100644 Documentation/ABI/removed/sysfs-driver-uniwill-laptop
>>
>> diff --git a/Documentation/ABI/removed/sysfs-driver-uniwill-laptop b/Documentation/ABI/removed/sysfs-driver-uniwill-laptop
>> new file mode 100644
>> index 000000000000..c226efc23b59
>> --- /dev/null
>> +++ b/Documentation/ABI/removed/sysfs-driver-uniwill-laptop
>> @@ -0,0 +1,10 @@
>> +What:		/sys/bus/platform/devices/INOU0000:XX/rainbow_animation
>> +Date:		November 2025
>> +KernelVersion:	6.19
>> +Contact:	Armin Wolf <W_Armin@gmx.de>
>> +Description:
>> +		Forces the integrated lightbar to display a rainbow animation when the machine
>> +		is not suspended. Writing "1"/"0" into this file enables/disables this
>> +		functionality.
>> +
>> +		Reading this file returns the current status of the rainbow animation functionality.
> It'd be more helpful by documenting the removal date, version and what
> has replaced it.

True, i will write a patch to address this.

>> diff --git a/Documentation/ABI/testing/sysfs-driver-uniwill-laptop b/Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>> index 943f92c6b561..b9b9feebe2ed 100644
>> --- a/Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>> +++ b/Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>> @@ -29,17 +29,6 @@ Description:
>>   		Reading this file returns the current enable status of the touchpad toggle
>>   		functionality.
>>   
>> -What:		/sys/bus/platform/devices/INOU0000:XX/rainbow_animation
>> -Date:		November 2025
>> -KernelVersion:	6.19
>> -Contact:	Armin Wolf <W_Armin@gmx.de>
>> -Description:
>> -		Forces the integrated lightbar to display a rainbow animation when the machine
>> -		is not suspended. Writing "1"/"0" into this file enables/disables this
>> -		functionality.
>> -
>> -		Reading this file returns the current status of the rainbow animation functionality.
>> -
>>   What:		/sys/bus/platform/devices/INOU0000:XX/breathing_in_suspend
>>   Date:		November 2025
>>   KernelVersion:	6.19
>> diff --git a/Documentation/admin-guide/laptops/uniwill-laptop.rst b/Documentation/admin-guide/laptops/uniwill-laptop.rst
>> index be50b45b82ef..aaf50e3074c1 100644
>> --- a/Documentation/admin-guide/laptops/uniwill-laptop.rst
>> +++ b/Documentation/admin-guide/laptops/uniwill-laptop.rst
>> @@ -74,8 +74,9 @@ Lightbar
>>   The ``uniwill-laptop`` driver exposes the lightbar found on some models as a standard multicolor
>>   LED class device. The default name of this LED class device is ``uniwill:multicolor:status``.
>>   
>> -See Documentation/ABI/testing/sysfs-driver-uniwill-laptop for details on how to control the various
>> -animation modes of the lightbar.
>> +The lightbar supports a separate rainbow animation mode that can be enabled by selecting the
>> +`uniwill-rainbow` LED trigger. To enable a breathing animation during suspend, see
>> +Documentation/ABI/testing/sysfs-driver-uniwill-laptop for details.
>>   
>>   Keyboard Backlight
>>   ------------------
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fa0571a5e479..f9dd979d0fa7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -28106,6 +28106,7 @@ UNIWILL LAPTOP DRIVER
>>   M:	Armin Wolf <W_Armin@gmx.de>
>>   L:	platform-driver-x86@vger.kernel.org
>>   S:	Maintained
>> +F:	Documentation/ABI/removed/sysfs-driver-uniwill-laptop
>>   F:	Documentation/ABI/testing/sysfs-driver-uniwill-laptop
>>   F:	Documentation/admin-guide/laptops/uniwill-laptop.rst
>>   F:	Documentation/wmi/devices/uniwill-laptop.rst
>> diff --git a/drivers/platform/x86/uniwill/Kconfig b/drivers/platform/x86/uniwill/Kconfig
>> index d07cc8440188..f80794bdfaef 100644
>> --- a/drivers/platform/x86/uniwill/Kconfig
>> +++ b/drivers/platform/x86/uniwill/Kconfig
>> @@ -28,6 +28,7 @@ config UNIWILL_LAPTOP
>>   	depends on DMI
>>   	select REGMAP
>>   	select INPUT_SPARSEKMAP
>> +	select LEDS_TRIGGERS
>>   	help
>>   	  This driver adds support for various extra features found on Uniwill laptops,
>>   	  like the lightbar, hwmon sensors and hotkeys. It also supports many OEM laptops
>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> index 788bf4064d34..0fb0d267664a 100644
>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> @@ -900,51 +900,6 @@ static ssize_t touchpad_toggle_enable_show(struct device *dev, struct device_att
>>   
>>   static DEVICE_ATTR_RW(touchpad_toggle_enable);
>>   
>> -static ssize_t rainbow_animation_store(struct device *dev, struct device_attribute *attr,
>> -				       const char *buf, size_t count)
>> -{
>> -	struct uniwill_data *data = dev_get_drvdata(dev);
>> -	unsigned int value;
>> -	bool enable;
>> -	int ret;
>> -
>> -	ret = kstrtobool(buf, &enable);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	if (enable)
>> -		value = LIGHTBAR_WELCOME;
>> -	else
>> -		value = 0;
>> -
>> -	guard(mutex)(&data->led_lock);
>> -
>> -	ret = regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_AC_CTRL, LIGHTBAR_WELCOME, value);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	ret = regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_BAT_CTRL, LIGHTBAR_WELCOME, value);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return count;
>> -}
>> -
>> -static ssize_t rainbow_animation_show(struct device *dev, struct device_attribute *attr, char *buf)
>> -{
>> -	struct uniwill_data *data = dev_get_drvdata(dev);
>> -	unsigned int value;
>> -	int ret;
>> -
>> -	ret = regmap_read(data->regmap, EC_ADDR_LIGHTBAR_AC_CTRL, &value);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return sysfs_emit(buf, "%d\n", !!(value & LIGHTBAR_WELCOME));
>> -}
>> -
>> -static DEVICE_ATTR_RW(rainbow_animation);
>> -
>>   static ssize_t breathing_in_suspend_store(struct device *dev, struct device_attribute *attr,
>>   					  const char *buf, size_t count)
>>   {
>> @@ -1258,7 +1213,6 @@ static struct attribute *uniwill_attrs[] = {
>>   	&dev_attr_super_key_enable.attr,
>>   	&dev_attr_touchpad_toggle_enable.attr,
>>   	/* Lightbar-related */
>> -	&dev_attr_rainbow_animation.attr,
>>   	&dev_attr_breathing_in_suspend.attr,
>>   	/* Power-management-related */
>>   	&dev_attr_ctgp_offset.attr,
>> @@ -1288,8 +1242,7 @@ static umode_t uniwill_attr_is_visible(struct kobject *kobj, struct attribute *a
>>   			return attr->mode;
>>   	}
>>   
>> -	if (attr == &dev_attr_rainbow_animation.attr ||
>> -	    attr == &dev_attr_breathing_in_suspend.attr) {
>> +	if (attr == &dev_attr_breathing_in_suspend.attr) {
>>   		if (uniwill_device_supports(data, UNIWILL_FEATURE_LIGHTBAR))
>>   			return attr->mode;
> It'd be better to convert breathing_in_suspend into an LED device
> attribute. See led_classdev->groups.

We cannot do this because this field is being used by the multicolor LED class.
I can try to find a workaround for this.

>>   	}
>> @@ -1537,6 +1490,59 @@ static int uniwill_led_brightness_set(struct led_classdev *led_cdev, enum led_br
>>   	return regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_BAT_CTRL, LIGHTBAR_S0_OFF, value);
>>   }
>>   
>> +static int uniwill_led_hw_control_set(struct led_classdev *led_cdev, unsigned long flags)
>> +{
>> +	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;
>> +
>> +	guard(mutex)(&data->led_lock);
>> +
>> +	if (flags)
>> +		value = LIGHTBAR_WELCOME;
>> +	else
>> +		value = LIGHTBAR_S0_OFF;
>> +
>> +	ret = regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_AC_CTRL,
>> +				 LIGHTBAR_S0_OFF | LIGHTBAR_WELCOME, value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(data->regmap, EC_ADDR_LIGHTBAR_BAT_CTRL,
>> +				 LIGHTBAR_S0_OFF | LIGHTBAR_WELCOME, value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* The LED is always on during hw control */
>> +	led_cdev->brightness = flags;
>> +
>> +	return 0;
>> +}
>> +
>> +static int uniwill_lightbar_trigger_activate(struct led_classdev *led_cdev)
>> +{
>> +	return led_cdev->hw_control_set(led_cdev, 1);
>> +}
>> +
>> +static void uniwill_lightbar_trigger_deactivate(struct led_classdev *led_cdev)
>> +{
>> +	int ret;
>> +
>> +	ret = led_cdev->hw_control_set(led_cdev, 0);
>> +	if (ret < 0)
>> +		dev_err(led_cdev->dev, "Failed to stop rainbow animation: %d\n", ret);
>> +}
>> +
>> +static struct led_hw_trigger_type uniwill_lightbar_trigger_type;
>> +
>> +static struct led_trigger uniwill_lightbar_trigger = {
>> +	.name = "uniwill-rainbow",
>> +	.activate = uniwill_lightbar_trigger_activate,
>> +	.deactivate = uniwill_lightbar_trigger_deactivate,
>> +	.trigger_type = &uniwill_lightbar_trigger_type,
>> +};
>> +
>>   #define LIGHTBAR_MASK	(LIGHTBAR_APP_EXISTS | LIGHTBAR_S0_OFF | LIGHTBAR_S3_OFF | LIGHTBAR_WELCOME)
>>   
>>   static int uniwill_led_init(struct uniwill_data *data)
>> @@ -1587,12 +1593,18 @@ static int uniwill_led_init(struct uniwill_data *data)
>>   	data->led_mc_cdev.led_cdev.max_brightness = 1;
>>   	data->led_mc_cdev.led_cdev.flags = LED_REJECT_NAME_CONFLICT;
>>   	data->led_mc_cdev.led_cdev.brightness_set_blocking = uniwill_led_brightness_set;
>> +	data->led_mc_cdev.led_cdev.trigger_type = &uniwill_lightbar_trigger_type;
>> +	data->led_mc_cdev.led_cdev.hw_control_trigger = uniwill_lightbar_trigger.name;
>> +	data->led_mc_cdev.led_cdev.hw_control_set = uniwill_led_hw_control_set;
> A private trigger does not need hw_control_set, which is only meant to
> coordinate between different drivers, i.e., in the case when the LED
> device and the hardware control trigger are located at different drivers.
>
> I've also updated the documentation in commit 8e04e5382aa9 ("leds:
> trigger: Add hw_offloaded() callback and provide
> trigger_may_offload_to_hw attribute"), quoted below:
>
>     Alternatively, a private trigger can be implemented along with the LED
>     driver if the LED's hardware control doesn't fit any generic trigger. To
>     associate the private trigger with the LED classdev, their `trigger_type`
>     must be the same. To declare that the private trigger provides hardware
>     control for the associated LED classdev, set the `hw_control_trigger`
>     string to the trigger's name. Since both the LED classdev and the private
>     trigger are in the same LED driver, it's not necessary for them to
>     coordinate via `hw_control_*` callbacks.
>
> I'd suggest calling uniwill_led_hw_control_set() directly from the
> trigger's [de]activate callbacks, just like other drivers.

I decided to use this callbacks in order to be able to add support for the keyboard backlight
animation in the future (if we someday find a device that supports this feature).

>>   
>>   	if (value & LIGHTBAR_S0_OFF)
>>   		data->led_mc_cdev.led_cdev.brightness = 0;
>>   	else
>>   		data->led_mc_cdev.led_cdev.brightness = 1;
>>   
>> +	if (value & LIGHTBAR_WELCOME)
> Check !LIGHTBAR_S0_OFF too?
>
> If the last state is LIGHTBAR_S0_OFF|LIGHTBAR_WELCOME (probably caused by
> an outdated driver without this patch), the trigger's activate callback
> will be called on probe, lighting up the previously off LED. Is this
> intentional?
>
> Thanks,
> Rong

Yes, this is intentional. LIGHTBAR_WELCOME implies !LIGHTBAR_S0_OFF, some devices
even assume this.

Thanks,
Armin Wolf

>> +		data->led_mc_cdev.led_cdev.default_trigger = uniwill_lightbar_trigger.name;
>> +
>>   	for (int i = 0; i < LED_CHANNELS; i++) {
>>   		data->led_mc_subled_info[i].color_index = color_indices[i];
>>   
>> @@ -3540,17 +3552,31 @@ static int __init uniwill_init(void)
>>   		pr_warn("Enabling potentially unsupported features\n");
>>   	}
>>   
>> -	ret = platform_driver_register(&uniwill_driver);
>> +	/*
>> +	 * We cannot register the trigger inside the .probe callback of the
>> +	 * platform driver, because each trigger needs a unique name.
>> +	 */
>> +	ret = led_trigger_register(&uniwill_lightbar_trigger);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> +	ret = platform_driver_register(&uniwill_driver);
>> +	if (ret < 0)
>> +		goto err_platform;
>> +
>>   	ret = uniwill_wmi_register_driver();
>> -	if (ret < 0) {
>> -		platform_driver_unregister(&uniwill_driver);
>> -		return ret;
>> -	}
>> +	if (ret < 0)
>> +		goto err_wmi;
>>   
>>   	return 0;
>> +
>> +err_wmi:
>> +	platform_driver_unregister(&uniwill_driver);
>> +
>> +err_platform:
>> +	led_trigger_unregister(&uniwill_lightbar_trigger);
>> +
>> +	return ret;
>>   }
>>   module_init(uniwill_init);
>>   
>> @@ -3558,6 +3584,7 @@ static void __exit uniwill_exit(void)
>>   {
>>   	uniwill_wmi_unregister_driver();
>>   	platform_driver_unregister(&uniwill_driver);
>> +	led_trigger_unregister(&uniwill_lightbar_trigger);
>>   }
>>   module_exit(uniwill_exit);
>>