* Re: [RFC PATCH v2] platform/x86: acer-wmi: Add 4-zone RGB keyboard support for Nitro AN515-58
2026-11-11 19:09 [RFC PATCH v2] platform/x86: acer-wmi: Add 4-zone RGB keyboard support for Nitro AN515-58 Batuhan
@ 2026-11-13 11:41 ` Ilpo Järvinen
0 siblings, 0 replies; 2+ messages in thread
From: Ilpo Järvinen @ 2026-11-13 11:41 UTC (permalink / raw)
To: Batuhan; +Cc: Hans de Goede, Lee, Chun-Yi, platform-driver-x86, LKML
[-- Attachment #1: Type: text/plain, Size: 16125 bytes --]
On Wed, 11 Nov 2026, Batuhan wrote:
> From: Batuhan Güvener <tuugspeedman@gmail.com>
>
> Add support for the 4-zone RGB keyboard backlights found on Acer Nitro
> AN515-58 laptops.
>
> The hardware operates via a dual-control mechanism: global hardware
> brightness is managed by the EC via RAM offset 0x31 with a range of 0 to 4
> (where 0x04 is maximum intensity), while per-zone RGB color control is
> handled via WMI Method 6.
>
> Exposing max_brightness = 4 to match the EC hardware level causes the
> led-class-multicolor subsystem to scale color components down, restricting
> the palette to 5x5x5 (125) total color combinations. Keeping max_brightness
> at 255 allows fine-grained 8-bit per-channel control from userspace, though
> it creates a dual-control scenario where hardware EC brightness and
> userspace multicolor brightness operate concurrently (e.g. EC brightness at
> 4 while userspace intensity is 0).
>
> Additionally, when the system resumes from a shutdown or power cycle
> while brightness is set to 0, the hardware initializes with 0
> brightness but cannot derive valid multi-intensity values. To ensure
> consistent state evaluation, it defaults to max intensity
> (255, 255, 255 - white).
>
> This implementation builds upon initial findings and reverse-engineering
> efforts by Dirga Yuza and open-source community driver implementations.
>
> Link: https://lore.kernel.org/r/20260503062131.158944-1-dirgayuza123@gmail.com/
> Link: https://github.com/0x7375646F/Linuwu-Sense
> Link: https://github.com/JafarAkhondali/acer-predator-turbo-and-rgb-keyboard-linux-module
>
> Signed-off-by: Batuhan Güvener <tuugspeedman@gmail.com>
>
> ---
> Changes since v1:
> - Clear non-English comments
> - Rebase onto linux-next
> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/acer-wmi.c | 264 ++++++++++++++++++++++++++++++++
> 2 files changed, 265 insertions(+)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4154045595d0..f6af91ede346 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -169,6 +169,7 @@ config ACER_WMI
> select LEDS_CLASS
> select NEW_LEDS
> select PLATFORM_PROFILE
> + select LEDS_CLASS_MULTICOLOR
> help
> This is a driver for newer Acer (and Wistron) laptops. It adds
> wireless radio and bluetooth control, and on some laptops,
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 3a018fdf5138..a374f4a6c979 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -20,6 +20,7 @@
> #include <linux/fixp-arith.h>
> #include <linux/backlight.h>
> #include <linux/leds.h>
> +#include <linux/led-class-multicolor.h>
> #include <linux/platform_device.h>
> #include <linux/platform_profile.h>
> #include <linux/acpi.h>
> @@ -36,6 +37,9 @@
> #include <linux/unaligned.h>
> #include <linux/bitfield.h>
> #include <linux/bitmap.h>
> +#include <linux/bits.h>
> +#include <linux/container_of.h>
> +#include <linux/mutex.h>
Please try to put these into where they're even remotely close to where
they'd be in alphabetical order. Alternatively, add a patch which sorts
these alphabetically per group (linux/, acpi/, asm/, etc), with one empty
line in between each group.
>
> MODULE_AUTHOR("Carlos Corbacho");
> MODULE_DESCRIPTION("Acer Laptop WMI Extras Driver");
> @@ -77,6 +81,41 @@ MODULE_LICENSE("GPL");
> #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
> #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
>
> +/*Method IDs for 4 zone keyboard RGB*/
> +#define WMID_GUID4 "7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56"
> +#define ACER_WMID_SET_GAMING_STATIC_LED_METHODID 6
> +#define ACER_WMID_SET_GAMING_DYNAMIC_LED_METHODID 5
> +#define ACER_WMID_SET_GAMINGKBBL_METHODID 20
> +
> +/*
> + * Gaming keyboard backlight enable command payload:
> + * Bit 3: Controller enable flag
> + * Bits [43:40]: Target 4-zone keyboard backlight interface
Please define these separately. Use GENMASK() for the second field.
You can likely drop the comments when using good names. Especially with
the bit 3.
> + */
> +#define ACER_GAMING_LED_KBBL_ENABLE (BIT_ULL(3) | (15ULL << 40))
KBBL ? If it that's short for "keyboard backlight", use KBD_BL instead.
...Then use named ones (xx | FIELD_PREP(yy, FIELD_MAX(yy)))
You'll likely need to split the construct to multiple lines to keep it
readable.
> +
> +/*
> + * Method 6 Gaming Static LED payload format:
> + * Bits [23:16]: Red intensity
> + * Bits [15:8]: Green intensity
> + * Bits [7:0]: Blue intensity
Same as GENMASK() already tells us so keep only the first line, if
anything.
> + */
> +#define ACER_GAMING_STATIC_LED_RED_MASK GENMASK(23, 16)
> +#define ACER_GAMING_STATIC_LED_GREEN_MASK GENMASK(15, 8)
> +#define ACER_GAMING_STATIC_LED_BLUE_MASK GENMASK(7, 0)
> +
> +/* 4-Zone RGB Keyboard EC RAM Offsets */
Does this add some value over the define name, if you think it does,
consider if define could be named better. You can then likely drop the
comment as redundant.
We only comment non-obvious/tricky bits, not repeat what can be read from
the code. Try to make naming and code so readable on its own, you only
rarely need to add a comment!
> +#define ACER_EC_ZONE_COLOR_BASE_ADDR 0x3C
> +
> +/* 4-Zone RGB Keyboard Protocol Constants */
> +#define ACER_KBD_STATIC_PREAMBLE_BRIGHTNESS 100
> +#define ACER_KBD_RGB_MAX_INTENSITY 255
> +#define ACER_KBD_NUM_ZONES 4
> +#define ACER_KBD_NUM_COLOR_CHANNELS 3
> +
> +/* Keyboard Backlight Modes */
> +#define ACER_KBD_MODE_STATIC 0
> +
> #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
> #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
>
> @@ -389,6 +428,131 @@ struct wmi_interface {
> /* The static interface pointer, points to the currently detected interface */
> static struct wmi_interface *interface;
>
> +/*Defining payload packages, zone information and general LED protocols */
> +static acpi_status WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out);
> +
> +struct led_information {
> + struct led_classdev_mc led_general;
> + struct mc_subled led_colors[3];
> + struct rgb_wmi *parent;
> + u8 zone_id;
> + u8 current_brightness;
> +};
> +
> +struct rgb_wmi {
> + struct mutex led_lock; /* making sure EC receives single command at a time */
> + struct led_information zones[4];
> +};
> +
> +static int acer_kbd_set_static_preamble(u8 ec_brightness)
> +{
> + u8 r = ACER_KBD_RGB_MAX_INTENSITY;
> + u8 g = ACER_KBD_RGB_MAX_INTENSITY;
> + u8 b = ACER_KBD_RGB_MAX_INTENSITY;
> +
> + ec_read(ACER_EC_ZONE_COLOR_BASE_ADDR, &r);
> + ec_read(ACER_EC_ZONE_COLOR_BASE_ADDR + 1, &g);
> + ec_read(ACER_EC_ZONE_COLOR_BASE_ADDR + 2, &b);
> +
> + u8 gm_input[16] = {
> + ACER_KBD_MODE_STATIC, /* mode = static */
> + 0, /* speed */
> + ec_brightness, /* brightness, 0-100 */
> + 0, /* reserved */
> + 0, /* direction */
> + r, g, b, /* color readings from EC */
> + 3, 1, 0, 0, 0, 0, 0, 0
Can you make a __packed struct out of this since so member names out
document what the thing is about. Don't forget to check if you've the
include for __packed if you end up using it.
> + };
> + struct acpi_buffer input = { .length = sizeof(gm_input), .pointer = gm_input };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + acpi_status status;
> + u64 resp = 0;
> +
> + status = wmi_evaluate_method(WMID_GUID4, 0,
/**
* wmi_evaluate_method - Evaluate a WMI method (deprecated)
Please do not add users for the deprecated WMI interface.
> + ACER_WMID_SET_GAMINGKBBL_METHODID,
> + &input, &output);
Misindented.
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + obj = output.pointer;
> + if (obj && obj->type == ACPI_TYPE_BUFFER) {
> + if (obj->buffer.length >= 8)
> + resp = get_unaligned_le64(obj->buffer.pointer);
> + else if (obj->buffer.length >= 4)
> + resp = get_unaligned_le32(obj->buffer.pointer);
> + else if (obj->buffer.length >= 1)
> + resp = ((u8 *)obj->buffer.pointer)[0];
> + }
> +
> + kfree(obj);
Use __free(), declare the variable mid-function where assigning to it.
> +
> + return resp ? -EIO : 0;
> +}
> +
> +static int send_zone_color(u8 zone_bit, u8 r, u8 g, u8 b)
> +{
> + acpi_status status;
> + u64 resp = 0;
> + u64 payload;
> + u32 rgb;
> +
> + rgb = FIELD_PREP(ACER_GAMING_STATIC_LED_RED_MASK, r) |
> + FIELD_PREP(ACER_GAMING_STATIC_LED_GREEN_MASK, g) |
> + FIELD_PREP(ACER_GAMING_STATIC_LED_BLUE_MASK, b);
Misindented.
> +
> + payload = (u64)cpu_to_be32(rgb) | zone_bit;
Consider __packed struct so you don't need to do crazy casts like that.
Only cast when passing it to execute.
> +
> + status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_STATIC_LED_METHODID,
> + payload, &resp);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + return resp ? -EIO : 0;
Would this be cleaner if you'd use || resp.
> +}
> +
> +static enum led_brightness rgb_get(struct led_classdev *led_cdev)
> +{
> + struct led_classdev_mc *general_mcolor = lcdev_to_mccdev(led_cdev);
> + struct led_information *led_info = container_of(general_mcolor,
> + struct led_information,
> + led_general);
Misaligned.
> + return led_info->current_brightness;
> +}
> +
> +static int rgb_set(struct led_classdev *led_cdev, enum led_brightness brightness)
> +{
> + struct led_classdev_mc *general_mcolor = lcdev_to_mccdev(led_cdev);
> + struct led_information *led_info = container_of(general_mcolor,
> + struct led_information,
> + led_general);
Misaligned. Please check if you have more copy-pasted? entries like this.
> + struct rgb_wmi *data = led_info->parent;
> + u8 r, g, b;
> + int ret;
> +
> + mutex_lock(&data->led_lock);
> +
> + if (brightness == 0) {
> + r = 0;
> + g = 0;
> + b = 0;
> + } else {
> + led_mc_calc_color_components(general_mcolor, brightness);
> + r = led_info->led_colors[0].brightness;
> + g = led_info->led_colors[1].brightness;
> + b = led_info->led_colors[2].brightness;
> + }
> +
> + ret = send_zone_color((u8)led_info->zone_id, r, g, b);
Isn't it already u8???.
> +
> + if (!ret) {
Reverse logic and handle errors first. Keep call and it's error handling
together without empty lines in between.
Since you now need to unlock to return immediately, you should also
convert this to guard() (which is preferred way for doing locking anyway).
> + led_info->current_brightness = brightness;
> + led_cdev->brightness = brightness;
> + }
> +
> + mutex_unlock(&data->led_lock);
> + return ret;
> +}
> /*
> * Embedded Controller quirks
> * Some laptops require us to directly access the EC to either enable or query
> @@ -406,10 +570,94 @@ struct quirk_entry {
> bool hwmon;
> bool platform_profile;
> bool pwm;
> + u8 kbl_four_zone;
> };
>
> static struct quirk_entry *quirks;
>
> +static int acer_kbd_rgb_init(struct platform_device *device, struct rgb_wmi *rgb_data)
> +{
> + static const char * const zone_names[] = {
> + "acer-wmi:rgb:kbd_backlight-left",
> + "acer-wmi:rgb:kbd_backlight-center-left",
> + "acer-wmi:rgb:kbd_backlight-center-right",
> + "acer-wmi:rgb:kbd_backlight-right",
> + };
> + u64 gaming_sysinfo;
> + acpi_status status;
> + int ret, i;
> +
> + if (!quirks || !quirks->kbl_four_zone)
> + return -ENODEV;
> +
> + status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_SYS_INFO_METHODID, 0, &gaming_sysinfo);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + status = WMI_gaming_execute_u64(ACER_WMID_SET_GAMING_LED_METHODID,
> + ACER_GAMING_LED_KBBL_ENABLE, NULL);
> + if (ACPI_FAILURE(status))
> + return -EIO;
> +
> + ret = acer_kbd_set_static_preamble(ACER_KBD_STATIC_PREAMBLE_BRIGHTNESS);
> + if (ret)
> + pr_warn("Failed to set static preamble on init: %d\n", ret);
> +
> + mutex_init(&rgb_data->led_lock);
devm_mutex_init() + error handling.
> +
> + for (i = 0; i < ACER_KBD_NUM_ZONES; i++) {
> + struct led_information *zone = &rgb_data->zones[i];
> + u8 r = 0, g = 0, b = 0;
> + u8 max_val;
> + u8 start_offset = ACER_EC_ZONE_COLOR_BASE_ADDR + (i * ACER_KBD_NUM_COLOR_CHANNELS);
Reverse xmas-tree order.
> +
> + zone->zone_id = BIT(i);
> + zone->parent = rgb_data;
> +
> + if (ec_read(start_offset, &r) != 0 ||
> + ec_read(start_offset + 1, &g) != 0 ||
> + ec_read(start_offset + 2, &b) != 0) {
Misaligned.
> + r = ACER_KBD_RGB_MAX_INTENSITY;
> + g = ACER_KBD_RGB_MAX_INTENSITY;
> + b = ACER_KBD_RGB_MAX_INTENSITY;
> + }
> +
> + max_val = max3(r, g, b);
> +
> + if (max_val == 0) {
> + zone->led_colors[0].intensity = ACER_KBD_RGB_MAX_INTENSITY;
> + zone->led_colors[1].intensity = ACER_KBD_RGB_MAX_INTENSITY;
> + zone->led_colors[2].intensity = ACER_KBD_RGB_MAX_INTENSITY;
> + zone->current_brightness = 0;
> + } else {
> + zone->led_colors[0].intensity = (r * ACER_KBD_RGB_MAX_INTENSITY) / max_val;
> + zone->led_colors[1].intensity = (g * ACER_KBD_RGB_MAX_INTENSITY) / max_val;
> + zone->led_colors[2].intensity = (b * ACER_KBD_RGB_MAX_INTENSITY) / max_val;
> + zone->current_brightness = max_val;
> + }
> +
> + zone->led_colors[0].color_index = LED_COLOR_ID_RED;
> + zone->led_colors[1].color_index = LED_COLOR_ID_GREEN;
> + zone->led_colors[2].color_index = LED_COLOR_ID_BLUE;
> +
> + zone->led_general.subled_info = zone->led_colors;
> + zone->led_general.num_colors = ACER_KBD_NUM_COLOR_CHANNELS;
> +
> + zone->led_general.led_cdev.name = zone_names[i];
> + zone->led_general.led_cdev.max_brightness = ACER_KBD_RGB_MAX_INTENSITY;
> + zone->led_general.led_cdev.brightness = zone->current_brightness;
> + zone->led_general.led_cdev.brightness_set_blocking = rgb_set;
> + zone->led_general.led_cdev.brightness_get = rgb_get;
> + zone->led_general.led_cdev.flags = LED_RETAIN_AT_SHUTDOWN;
> +
> + ret = devm_led_classdev_multicolor_register(&device->dev,
> + &zone->led_general);
Misaligned.
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +
> static void __init set_quirks(void)
> {
> if (quirks->mailled)
> @@ -466,6 +714,7 @@ static struct quirk_entry quirk_acer_nitro_an515_58 = {
> .hwmon = true,
> .platform_profile = true,
> .pwm = true,
> + .kbl_four_zone = 1,
> };
>
> static struct quirk_entry quirk_acer_predator_ph315_53 = {
> @@ -2815,6 +3064,8 @@ static int acer_wmi_hwmon_init(void);
> /*
> * Platform device
> */
> +static struct rgb_wmi acer_rgb_data;
> +
> static int acer_platform_probe(struct platform_device *device)
> {
> int err;
> @@ -2846,6 +3097,11 @@ static int acer_platform_probe(struct platform_device *device)
> if (err)
> goto error_hwmon;
> }
> + if (quirks->kbl_four_zone) {
> + err = acer_kbd_rgb_init(device, &acer_rgb_data);
> + if (err)
> + pr_warn("Failed to initialize 4-zone RGB keyboard: %d\n", err);
dev_warn()
> + }
>
> return 0;
>
> @@ -2898,6 +3154,7 @@ static int acer_suspend(struct device *dev)
> static int acer_resume(struct device *dev)
> {
> struct acer_data *data = &interface->data;
> + int ret;
>
> if (!data)
> return -ENOMEM;
> @@ -2911,6 +3168,13 @@ static int acer_resume(struct device *dev)
> if (acer_wmi_accel_dev)
> acer_gsensor_init();
>
> + /* Ensuring keyboard is at static mode after resuming from sleep. */
> + if (quirks && quirks->kbl_four_zone) {
> + ret = acer_kbd_set_static_preamble(ACER_KBD_STATIC_PREAMBLE_BRIGHTNESS);
> + if (ret)
> + pr_warn("Failed to restore keyboard static mode on resume: %d\n", ret);
dev_warn()
> + }
> +
> return 0;
> }
> #else
>
--
i.
^ permalink raw reply [flat|nested] 2+ messages in thread