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

[Rong Zhang <i@rong.moe>]

Hi Armin,

On Mon, 2026-10-12 at 00:48 +0200, Armin Wolf wrote:
> Am 11.10.26 um 21:47 schrieb Rong Zhang:
> 
> > 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.
> 
> I think we should rather use a separate device_type for multicolor/flash LED devices,
> because this allows us to inject the sysfs groups without having to introduce yet
> another field.

Sounds neat. Nice inspiration!

> 
> Would it be acceptable if multicolor/flash LED support cannot be build as a module anymore?

In fact, even LEDS_CLASS is tristate (though rarely built as a module
because LEDS_TRIGGERS promotes it to bool).

From my perspective, forcing LEDS_CLASS_FLASH and LEDS_CLASS_MULTICOLOR
as bool will be too harsh for distros. We don't need to build them into
vmlinux, as long as an internal register function with an extra
device_type parameter is exported. We've already had several glue exports
declared in drivers/leds/leds.h.

Thanks,
Rong

> 
> Thanks,
> Armin Wolf
> 
> > 
> > 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);
> > > > >