Re: [PATCH v2 2/5] platform/x86: uniwill-laptop: Implement rainbow animation as trigger
From: Armin Wolf
Date: Sun Oct 11 2026 - 18:48:41 EST
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:[..snip..]
Hi Armin,True, i will write a patch to address this.
On Wed, 2026-10-07 at 16:38 +0200, Armin Wolf wrote:
It turns out that some models require the lightbar to be activatedIt'd be more helpful by documenting the removal date, version and what
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.
has replaced it.
Whoops, I didn't notice that.We cannot do this because this field is being used by the multicolor LED class.-It'd be better to convert breathing_in_suspend into an LED device
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;
attribute. See led_classdev->groups.
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..]
The name "uniwill-rainbow" sounds irrelevant to keyboard backlightI decided to use this callbacks in order to be able to add support for the keyboard backlight+A private trigger does not need hw_control_set, which is only meant to
+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;
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.
animation in the future (if we someday find a device that supports this feature).
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
Yes, this is intentional. LIGHTBAR_WELCOME implies !LIGHTBAR_S0_OFF, some devicesif (value & LIGHTBAR_S0_OFF)Check !LIGHTBAR_S0_OFF too?
data->led_mc_cdev.led_cdev.brightness = 0;
else
data->led_mc_cdev.led_cdev.brightness = 1;
+ if (value & LIGHTBAR_WELCOME)
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
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);