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

[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@xxxxxx>
> ---
>  .../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@xxxxxx>
> +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.

> 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@xxxxxx>
> -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@xxxxxx>
>  L:	platform-driver-x86@xxxxxxxxxxxxxxx
>  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.

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

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

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