Re: [RFC PATCH] platform/x86: acer-wmi: Add 4-zone RGB keyboard support for Acer Nitro AN515-58

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sun, 3 May 2026, Dirga Yuza wrote:

> This RFC adds support for the 4-zone RGB keyboard on Acer Nitro AN515-58.
> 
> The current acer-wmi driver does not provide support for keyboard
> backlighting. This patch introduces support for the multicolor LED class
> to expose the four zones to userspace.
> 
> The implementation is based on prior reverse engineering efforts of WMI
> method ID 6, particularly from Linuwu-sense and
> JafarAkhondali's acer-predator-turbo-and-rgb-keyboard-linux-module.
> I do not have access to a Windows environment to capture traces myself.
> 
> This has been tested on AN515-58 under regular usage, including
> suspend/resume cycles.
> 
> I would appreciate feedback on the following points:
> 
> 1. acer-wmi currently relies heavily on global state. This patch follows
> the same approach for consistency. However, given that this introduces
> new structures, would it be preferable to group them into a private
> per-device structure, or is continuing with global data acceptable
> for this driver?
> 
> 2. The current implementation registers four independent
> led_classdev_mc instances (acer-wmi::kbd_backlight_N). In testing with
> KDE Plasma, color updates propagate correctly across all zones, but
> brightness control appears to affect only the last registered zone.
> 
> Is independent registration the preferred approach, or would a
> single aggregate device controlling all zones be more appropriate?
> 
> 3. The patch currently hardcodes four zones via a model-specific quirk.
> Is this the preferred approach, or is there a more scalable way to handle
> varying zone counts across devices?
> 
> 4. Kconfig is updated to add `imply LEDS_CLASS_MULTICOLOR` for acer-wmi
> and the code is wrapped with IS_REACHABLE() to handle optional dependency
> on LEDS_CLASS. Is this correct approach for optional multicolor
> LED support?
> 
> On this hardware, brightness keys (Fn+F9/F10) generate KEY_KBDILLUMUP/DOWN
> input events. In my testing, these events do not appear to generate
> corresponding release events, unlike other keys
> (e.g., touchpad toggle or rfkill), which emit proper press/release pairs.
> 
> In KDE Plasma, this is interpreted as a held key, causing continuous
> brightness changes.
> 
> Synthesizing press/release events would result in both the EC and
> userspace adjusting their own brightness, causing duplicate changes.
> The current behavior (no release events) also leads to incorrect
> interpretation as a held key in KDE Plasma.
> 
> Therefore it is unclear whether these events should be ignored
> entirely or handled in some other way. Guidance on the preferred approach
> would be appreciated.
> 
> This is my first contribution in the kernel, so any feedback on design,
> coding style, or submission process would be appreciated.
> 
> Signed-off-by: Dirga Yuza <dirgayuza123@gmail.com>
> ---
>  drivers/platform/x86/Kconfig    |   1 +
>  drivers/platform/x86/acer-wmi.c | 141 ++++++++++++++++++++++++++++++++
>  2 files changed, 142 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2ffa4ecf6..c7eb3a587 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -179,6 +179,7 @@ config ACER_WMI
>  	select INPUT_SPARSEKMAP
>  	select LEDS_CLASS
>  	select NEW_LEDS
> +	imply LEDS_CLASS_MULTICOLOR

Maybe it would be better to just use depends on and remove the ifdeffery.

Nothing else seem to use imply for LEDS_CLASS_MULTICOLOR.

>  	select ACPI_PLATFORM_PROFILE
>  	help
>  	  This is a driver for newer Acer (and Wistron) laptops. It adds
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index e0eaaefb1..b7c3d6c28 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -36,6 +36,7 @@
>  #include <linux/unaligned.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitmap.h>
> +#include <linux/led-class-multicolor.h>

These should be eventually changed to the alphabetical order. I see there 
are pre-existing violations but place it new one next to leds.h please.

If you want, you can make change that corrects the include ordering (in 
a separate patch).
  
>  MODULE_AUTHOR("Carlos Corbacho");
>  MODULE_DESCRIPTION("Acer Laptop WMI Extras Driver");
> @@ -76,10 +77,16 @@ MODULE_LICENSE("GPL");
>  #define ACER_WMID_GET_GAMING_FAN_SPEED_METHODID 17
>  #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>  #define ACER_WMID_GET_GAMING_MISC_SETTING_METHODID 23
> +#define ACER_WMID_SET_GAMING_STATIC_LED_METHODID 6
> +
> +#define ACER_GAMING_KBL_ZONES 4
>  
>  #define ACER_GAMING_FAN_BEHAVIOR_CPU BIT(0)
>  #define ACER_GAMING_FAN_BEHAVIOR_GPU BIT(3)
>  
> +/* Bit 3 enables keyboard backlight update */
> +#define ACER_GAMING_KBL_SET_ON BIT(3)

Please add bits.h. This is a pre-existing problem but lets fix it now 
since you're adding new BIT() & GENMASK() users.

>  #define ACER_GAMING_FAN_BEHAVIOR_STATUS_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_FAN_BEHAVIOR_ID_MASK GENMASK_ULL(15, 0)
>  #define ACER_GAMING_FAN_BEHAVIOR_SET_CPU_MODE_MASK GENMASK(17, 16)
> @@ -91,6 +98,9 @@ MODULE_LICENSE("GPL");
>  #define ACER_GAMING_FAN_SPEED_ID_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_FAN_SPEED_VALUE_MASK GENMASK_ULL(15, 8)
>  
> +/* Bits [43:40] selects target zones, setting all bits targets all zones*/
> +#define ACER_GAMING_KBL_SET_ALL_ZONES GENMASK(43, 40)
> +
>  #define ACER_GAMING_MISC_SETTING_STATUS_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_MISC_SETTING_INDEX_MASK GENMASK_ULL(7, 0)
>  #define ACER_GAMING_MISC_SETTING_VALUE_MASK GENMASK_ULL(15, 8)
> @@ -310,6 +320,7 @@ struct hotkey_function_type_aa {
>  #define ACER_CAP_PLATFORM_PROFILE	BIT(10)
>  #define ACER_CAP_HWMON			BIT(11)
>  #define ACER_CAP_PWM			BIT(12)
> +#define ACER_CAP_KBL_FOUR_ZONE_RGB	BIT(13)
>  
>  /*
>   * Interface type flags
> @@ -405,6 +416,7 @@ struct quirk_entry {
>  	u8 gpu_fans;
>  	u8 predator_v4;
>  	u8 pwm;
> +	u8 kbl_four_zone_rgb;
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -427,6 +439,9 @@ static void __init set_quirks(void)
>  
>  	if (quirks->pwm)
>  		interface->capability |= ACER_CAP_PWM;
> +
> +	if (quirks->kbl_four_zone_rgb)
> +		interface->capability |= ACER_CAP_KBL_FOUR_ZONE_RGB;
>  }
>  
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -458,6 +473,7 @@ static struct quirk_entry quirk_acer_travelmate_2490 = {
>  static struct quirk_entry quirk_acer_nitro_an515_58 = {
>  	.predator_v4 = 1,
>  	.pwm = 1,
> +	.kbl_four_zone_rgb = 1,
>  };
>  
>  static struct quirk_entry quirk_acer_predator_ph315_53 = {
> @@ -2762,6 +2778,118 @@ static u32 get_wmid_devices(void)
>  
>  static int acer_wmi_hwmon_init(void);
>  
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS_MULTICOLOR)
> +
> +static int acer_wmi_poll_and_enable_zones(void)
> +{
> +	acpi_status status;
> +
> +	status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_SYS_INFO_METHODID,
> +		0, NULL);

Please align these continuations to ( unless you've very very good reason 
for not to.

> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +	status = WMI_gaming_execute_u64(ACER_WMID_GET_GAMING_LED_METHODID,
> +		ACER_GAMING_KBL_SET_ON |
> +		ACER_GAMING_KBL_SET_ALL_ZONES,
> +		NULL);
> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +struct acer_wmi_led_zone {
> +	struct led_classdev_mc mc_cdev;
> +	struct mc_subled subled_info[3];
> +	u8 zone_id;
> +};
> +
> +struct led_four_zone_set_param {
> +	u8 zone;
> +	u8 red;
> +	u8 green;
> +	u8 blue;
> +} __packed;
> +
> +static struct acer_wmi_led_zone kbl_zones[ACER_GAMING_KBL_ZONES];
> +
> +static int acer_wmi_mc_brightness_set(struct led_classdev *led_cdev,
> +	enum led_brightness brightness)
> +{
> +	int err;

Please try to use reverse xmas-tree order for local variable declarations.

> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
> +	struct acer_wmi_led_zone *zone = container_of(mc_cdev,
> +		struct acer_wmi_led_zone, mc_cdev);

Either make this one line (it's just below 100 cols), or put the entire 
container_of to the second line.

Please also add linux/container_of.h.

> +	struct led_four_zone_set_param params;
> +	struct acpi_buffer input;
> +	acpi_status status;
> +
> +	err = led_mc_calc_color_components(mc_cdev, brightness);
> +	if (err)
> +		return err;
> +
> +	led_cdev->brightness = brightness;
> +
> +	params.zone = zone->zone_id;
> +	params.red = mc_cdev->subled_info[0].brightness;
> +	params.green = mc_cdev->subled_info[1].brightness;
> +	params.blue = mc_cdev->subled_info[2].brightness;
> +
> +	input.length = sizeof(params);
> +	input.pointer = &params;
> +
> +	status = wmi_evaluate_method(WMID_GUID4, 0,
> +		ACER_WMID_SET_GAMING_STATIC_LED_METHODID, &input, NULL);

Please align this to (

> +	if (ACPI_FAILURE(status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static enum led_brightness
> +acer_wmi_mc_brightness_get(struct led_classdev *led_cdev)
> +{
> +	return led_cdev->brightness;
> +}
> +
> +static int acer_wmi_register_four_zone_leds(struct device *dev)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ACER_GAMING_KBL_ZONES; i++) {
> +		struct acer_wmi_led_zone *zone = &kbl_zones[i];
> +
> +		memset(zone, 0, sizeof(*zone));

Why is this required?

> +		zone->subled_info[0].color_index = LED_COLOR_ID_RED;
> +		zone->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> +		zone->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> +
> +		zone->mc_cdev.subled_info = zone->subled_info;
> +		zone->mc_cdev.num_colors = 3;
> +
> +		/* WMI uses a bitmask as for zones. BIT(i) selects zone i */
> +		zone->zone_id = BIT(i);
> +
> +		zone->mc_cdev.led_cdev.name = devm_kasprintf(dev, GFP_KERNEL,
> +			"acer-wmi::kbd_backlight_%d", i + 1);

Please make sure the naming matches what is documented in:

https://lore.kernel.org/linux-leds/20260504145434.12746-1-johannes.goede@oss.qualcomm.com/

> +		zone->mc_cdev.led_cdev.dev = dev;
> +		zone->mc_cdev.led_cdev.brightness_set_blocking =
> +			acer_wmi_mc_brightness_set;
> +		zone->mc_cdev.led_cdev.brightness_get =
> +			acer_wmi_mc_brightness_get;
> +		zone->mc_cdev.led_cdev.max_brightness = 255;
> +
> +		ret = devm_led_classdev_multicolor_register(dev,
> +			&zone->mc_cdev);

Fits to one line.

> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +#endif /* IS_REACHABLE(CONFIG_LEDS_CLASS_MULTICOLOR) */
> +
>  /*
>   * Platform device
>   */
> @@ -2797,8 +2925,21 @@ static int acer_platform_probe(struct platform_device *device)
>  			goto error_hwmon;
>  	}
>  
> +	if (has_cap(ACER_CAP_KBL_FOUR_ZONE_RGB)) {
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS_MULTICOLOR)
> +		err = acer_wmi_poll_and_enable_zones();
> +		if (err)
> +			goto error_kbl_four_zone_rgb;
> +
> +		err = acer_wmi_register_four_zone_leds(&device->dev);
> +		if (err)
> +			goto error_kbl_four_zone_rgb;
> +#endif /* IS_REACHABLE(CONFIG_LEDS_CLASS_MULTICOLOR) */
> +	}
> +
>  	return 0;
>  
> +error_kbl_four_zone_rgb:
>  error_hwmon:
>  error_platform_profile:
>  	acer_rfkill_exit();
> 

-- 
 i.