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

[Armin Wolf <W_Armin@gmx.de>]

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.

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

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