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

[Rong Zhang <i@rong.moe>]

Hi Armin,

On Sun, 2026-10-11 at 02:52 +0200, Armin Wolf wrote:
> 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.

[..snip..]

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

Whoops, I didn't notice that.

> I can try to find a workaround for this.

(+CC Lee, Pavel, linux-leds)

IMO the best way to solve the dilemma is to introduce class_group for
led-class-multicolor and led-class-flash, or else we will have to
introduce (or have already introduced?) more workarounds for other
drivers.

For this cycle let's keep the attribute as is.

[..snip..]

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

The name "uniwill-rainbow" sounds irrelevant to keyboard backlight
animation. In this case the latter needs a new private trigger with a new
name, and it can have different [de]activate callbacks. IOW, we don't
need hw_control_set() either.

Otherwise, using hw_control_set() makes sense to me as long as the
current name is relevant.



Since I've CC'ed leds folks:

Per-key lighting is becoming increasingly common for keyboard backlight.

I am actually thinking about introducing LED array/matrix support to the
leds subsystem. An LED array/matrix device is just a special LED device
that controls all constituent LED devices, so that we can:

- implement keyboard backlight animation (or LampArray autonomous mode)
as an array/matrix hardware control trigger [1]
- tune the lighting of the whole array/matrix at once by tuning the LED
array/matrix device [2]
- tune per-key lighting by tuning constituent LED devices
- apply generic LED triggers to the whole array/matrix [1]
- apply different generic LED triggers to different constituent LED
devices
- properly deconflict the above operations

It definitely requires a lot of work to get things (especially locking
and atomicity) right, but it should be very helpful for devices with per-
key lighting once done. I believe missing proper LED array/matrix support
was the main reason why tuxedo_nb04_wmi_ab [3] implemented a virtual
LampArray HID device far before any userspace LampArray tool emerged.

[1]: The constituent LED devices will be inoperable when a trigger is
active on their array/matrix LED device.

[2]: The leds subsystem won't emulate this. The driver must implement
brightness_get() and brightness_set[_blocking]() for the LED array/matrix
device.

[3]: I know Texudo folks are working on adding hid-lamparray to connect
it to the LED subsystem. I've had another independent implementation with
HIDRAW sniff, no hid-generic dependency, autonomous mode as a trigger,
non-RGB LampArray support, and per-key LED device support in my local
tree, but that's another story. I will firstly push it further without
LED array/matrix support. Then I will try to add LED array/matrix
support, with hid-lamparray being the first user.

Thanks,
Rong

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