Re: [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver

[Shyam Sundar S K]

On 5/4/2026 19:51, Yo-Jung Leo Lin (AMD) wrote:
> [You don't often get email from leo.lin@xxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> The Halo Box features an RGB LED light bar that can be controlled
> through WMI methods to display any color combination.
> 
> The driver exposes the LED through the LED multicolor subsystem,
> allowing userspace to control RGB values via sysfs:
>   /sys/class/leds/amd_halo:multicolor:status/multi_intensity
>   /sys/class/leds/amd_halo:multicolor:status/brightness
> 
> Hardware interface:
> - Three separate RGB channels (Red, Green, Blue)
> - Each channel controlled via individual WMI method calls
> - Value range: 0-100 (matching hardware range directly)
> 
> Co-developed-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
> Signed-off-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
> Signed-off-by: Yo-Jung Leo Lin (AMD) <Leo.Lin@xxxxxxx>
> ---
> v1 -> v2:
> - Fix Kconfig dependencies
> - Remove amd_halo_wmi_turn_off(). Handle the case where brightness=0 with the
>   same logic where brightness != 0
> - Remove the brightness_get callback, because currently there isn't a
>   well-defined global bightness in hardware. Note that the sysfs brightness
>   stil works. It just caches the value last set.
> - Convert to wmidev_invoke_method()
> - Convert to ARRAY_SIZE()
> - Convert some macros to enum
> - Convert to devm_led_classdev_multicolor_register_ext()
> - Rename sysfs path to amd_halo:multicolor:status
> - Fold default LED colors setting in probe, before registration
> - Add no_singleton option
> - Make default RGB values into macros and adjust their values
> ---
>  MAINTAINERS                             |   7 +
>  drivers/platform/x86/amd/Kconfig        |  11 ++
>  drivers/platform/x86/amd/Makefile       |   1 +
>  drivers/platform/x86/amd/amd_halo_led.c | 222 ++++++++++++++++++++++++++++++++
>  4 files changed, 241 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c871acf2179c..464bd4d16461 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1124,6 +1124,13 @@ F:       drivers/char/hw_random/geode-rng.c
>  F:     drivers/crypto/geode*
>  F:     drivers/video/fbdev/geode/
> 
> +AMD HALO BOX RGB LED DRIVER
> +M:     Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
> +R:     Yo-Jung Leo Lin (AMD) <Leo.Lin@xxxxxxx>
> +L:     platform-driver-x86@xxxxxxxxxxxxxxx
> +S:     Supported
> +F:     drivers/platform/x86/amd/amd_halo_led.c
> +
>  AMD HSMP DRIVER
>  M:     Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx>
>  R:     Carlos Bilbao <carlos.bilbao@xxxxxxxxxx>
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index b813f9265368..a1a74ef6c859 100644
> --- a/drivers/platform/x86/amd/Kconfig
> +++ b/drivers/platform/x86/amd/Kconfig
> @@ -34,6 +34,17 @@ config AMD_WBRF
>           This mechanism will only be activated on platforms that advertise a
>           need for it.
> 
> +config AMD_HALO_LED
> +       tristate "AMD Halo Box RGB LED Driver"
> +       depends on ACPI_WMI && LEDS_CLASS_MULTICOLOR
> +       help
> +         This driver provides RGB LED control for AMD Halo Box devices
> +         through the LED multicolor subsystem. The Halo Box light bar can
> +         be controlled via sysfs to display any RGB color combination.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called amd_halo_led.
> +
>  config AMD_ISP_PLATFORM
>         tristate "AMD ISP4 platform driver"
>         depends on I2C && X86_64 && ACPI
> diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
> index f6ff0c837f34..2f467dbbfc8a 100644
> --- a/drivers/platform/x86/amd/Makefile
> +++ b/drivers/platform/x86/amd/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_AMD_PMC)         += pmc/
>  obj-$(CONFIG_AMD_HSMP)         += hsmp/
>  obj-$(CONFIG_AMD_PMF)          += pmf/
>  obj-$(CONFIG_AMD_WBRF)         += wbrf.o
> +obj-$(CONFIG_AMD_HALO_LED)     += amd_halo_led.o
>  obj-$(CONFIG_AMD_ISP_PLATFORM) += amd_isp4.o
>  obj-$(CONFIG_AMD_HFI)          += hfi/
> diff --git a/drivers/platform/x86/amd/amd_halo_led.c b/drivers/platform/x86/amd/amd_halo_led.c
> new file mode 100644
> index 000000000000..12188571c7d9
> --- /dev/null
> +++ b/drivers/platform/x86/amd/amd_halo_led.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Halo Box RGB LED Driver
> + *
> + * Copyright (C) 2026 Advanced Micro Devices, Inc.
> + *
> + * This driver provides RGB LED control for AMD Halo Box devices through
> + * the LED multicolor subsystem. The Halo Box light bar can be controlled
> + * via sysfs to display any RGB color combination.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/unaligned.h>
> +#include <linux/wmi.h>
> +
> +#define AMD_HALO_GUID "081E747B-E028-4232-AF24-EAAEAB2B1E86"
> +
> +/* WMI method IDs from MOF */
> +enum {
> +       AMD_HALO_WMI_GET_LIGHTBAR = 0x01,
> +       AMD_HALO_WMI_SET_LIGHTBAR,
> +       AMD_HALO_WMI_TURN_ON,
> +       AMD_HALO_WMI_TURN_OFF

v2 has turn_off calls removed but the enums are not updated.

AMD_HALO_WMI_TURN_ON/OFF values are not in use.

> +};
> +
> +/* Channel selectors for Arg0 */
> +enum {
> +       AMD_HALO_CHANNEL_RED = 0x01,
> +       AMD_HALO_CHANNEL_GREEN,
> +       AMD_HALO_CHANNEL_BLUE
> +};
> +
> +/* Status codes from spec */
> +#define AMD_HALO_STATUS_SUCCESS                0x0000
> +#define AMD_HALO_STATUS_INVALID_PARAM  0xFFFD

This macro is not used. Please see below for more comments.

> +
> +/* Brightness uses 0-100 range */
> +#define AMD_HALO_MAX_HW_BRIGHTNESS     100
> +
> +/* Default RGB brightness */
> +#define AMD_HALO_DEFAULT_RED           50
> +#define AMD_HALO_DEFAULT_GREEN         30
> +#define AMD_HALO_DEFAULT_BLUE          30

50/30/30 means a reddish-orange on the startup. Intentional? Should it
be just 50/50/50 to look white?

> +
> +/**
> + * struct amd_halo_led_data - Driver private data
> + * @wdev: WMI device pointer
> + * @led_mc: LED multicolor class device
> + * @subled_info: RGB channel information
> + * @lock: Mutex to protect WMI calls
> + */
> +struct amd_halo_led_data {
> +       struct wmi_device *wdev;
> +       struct led_classdev_mc led_mc;
> +       struct mc_subled subled_info[3];
> +       struct mutex lock;      /* Protects WMI method calls */
> +};
> +
> +struct amd_halo_wmi_args {
> +       u32 arg0;
> +       u32 arg1;
> +};
> +
> +/**
> + * amd_halo_wmi_set_channel - Set a single RGB channel value
> + * @wdev: WMI device pointer
> + * @channel: Channel selector (1=Red, 2=Green, 3=Blue)
> + * @brightness: brightness to set (0-100)
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int amd_halo_wmi_set_channel(struct wmi_device *wdev, u32 channel,
> +                                   u32 brightness)
> +{
> +       struct amd_halo_wmi_args args = {
> +               .arg0 = channel,
> +               .arg1 = brightness,
> +       };
> +       struct wmi_buffer input = {
> +               .length = sizeof(args),
> +               .data = &args,
> +       };
> +       struct wmi_buffer output = { 0 };
> +       u16 result_status;
> +       int ret;
> +
> +       /* Validate input per spec */
> +       if (channel < AMD_HALO_CHANNEL_RED ||
> +           channel > AMD_HALO_CHANNEL_BLUE ||
> +           brightness > AMD_HALO_MAX_HW_BRIGHTNESS)
> +               return -EINVAL;
> +
> +       ret = wmidev_invoke_method(wdev, 0, AMD_HALO_WMI_SET_LIGHTBAR,
> +                                       &input, &output, sizeof(result_status));
> +       if (ret)
> +               return ret;
> +
> +       /* Return buffer per spec: Bytes[0:1] = Status (little-endian) */
> +       result_status = get_unaligned_le16(output.data);
> +       ret = (result_status == AMD_HALO_STATUS_SUCCESS) ? 0 : -EIO;

AMD_HALO_STATUS_INVALID_PARAM macro usage could be something like this?

if (result_status == AMD_HALO_STATUS_SUCCESS)
    ret = 0;
else if (result_status == AMD_HALO_STATUS_INVALID_PARAM)
    ret = -EINVAL;
else
    ret = -EIO

if no, the macro can be entirely dropped.

> +
> +       kfree(output.data);
> +       return ret;
> +}
> +
> +/**
> + * amd_halo_brightness_set - Set LED brightness and color
> + * @cdev: LED class device
> + * @brightness: Brightness value
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int amd_halo_brightness_set(struct led_classdev *cdev,
> +                                  enum led_brightness brightness)
> +{
> +       struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +       struct amd_halo_led_data *data = container_of(mc_cdev,
> +                                                      struct amd_halo_led_data,
> +                                                      led_mc);
> +       u32 red_hw, green_hw, blue_hw;
> +       int ret;
> +
> +       guard(mutex)(&data->lock);
> +
> +       led_mc_calc_color_components(mc_cdev, brightness);
> +       red_hw = mc_cdev->subled_info[0].brightness;
> +       green_hw = mc_cdev->subled_info[1].brightness;
> +       blue_hw = mc_cdev->subled_info[2].brightness;
> +
> +       /* Set each channel individually - 3 WMI calls required */
> +       ret = amd_halo_wmi_set_channel(data->wdev, AMD_HALO_CHANNEL_RED,
> +                                      red_hw);
> +       if (ret)
> +               return ret;
> +
> +       ret = amd_halo_wmi_set_channel(data->wdev, AMD_HALO_CHANNEL_GREEN,
> +                                      green_hw);
> +       if (ret)
> +               return ret;
> +
> +       return amd_halo_wmi_set_channel(data->wdev, AMD_HALO_CHANNEL_BLUE,
> +                                       blue_hw);

what if one of the above set_channel fails? 3 things I can think of:

- Restore previous state on failure, or
- Accept partial updates (current behavior), or
- Add a comment documenting this is intentional

> +}
> +
> +/**
> + * amd_halo_probe - Driver probe function
> + * @wdev: WMI device
> + * @context: Context data (unused)
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int amd_halo_probe(struct wmi_device *wdev, const void *context)
> +{
> +       struct led_init_data led_init_data = {
> +               .devicename = "amd_halo",
> +               .default_label = "multicolor:" LED_FUNCTION_STATUS,
> +               .devname_mandatory = true
> +       };
> +       struct amd_halo_led_data *data;
> +       int ret;
> +
> +       data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->wdev = wdev;
> +       mutex_init(&data->lock);
> +       dev_set_drvdata(&wdev->dev, data);
> +
> +       data->subled_info[0].color_index = LED_COLOR_ID_RED;
> +       data->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> +       data->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> +       data->subled_info[0].intensity = AMD_HALO_DEFAULT_RED;
> +       data->subled_info[1].intensity = AMD_HALO_DEFAULT_GREEN;
> +       data->subled_info[2].intensity = AMD_HALO_DEFAULT_BLUE;
> +
> +       data->led_mc.led_cdev.brightness = AMD_HALO_MAX_HW_BRIGHTNESS;
> +       data->led_mc.led_cdev.max_brightness = AMD_HALO_MAX_HW_BRIGHTNESS;
> +       data->led_mc.led_cdev.brightness_set_blocking = amd_halo_brightness_set;
> +       data->led_mc.led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;

Are these suspend/resume flags required?

Does the hardware retain LED state across suspend/resume, or does the
LED core need to restore it?

> +       data->led_mc.num_colors = ARRAY_SIZE(data->subled_info);
> +       data->led_mc.subled_info = data->subled_info;
> +
> +       ret = amd_halo_brightness_set(&data->led_mc.led_cdev,
> +                                     AMD_HALO_MAX_HW_BRIGHTNESS);

brightness_set is called before the LED registration. Is this intentional?

> +       if (ret)
> +               return dev_err_probe(&wdev->dev, ret,
> +                                    "Failed to set default LED colors\n");
> +
> +       ret = devm_led_classdev_multicolor_register_ext(&wdev->dev, &data->led_mc,
> +                                                       &led_init_data);
> +       if (ret)
> +               return dev_err_probe(&wdev->dev, ret,
> +                                    "Failed to register multicolor LED\n");
> +       return 0;
> +}
> +
> +static const struct wmi_device_id amd_halo_id_table[] = {
> +       { .guid_string = AMD_HALO_GUID },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(wmi, amd_halo_id_table);
> +
> +static struct wmi_driver amd_halo_driver = {
> +       .driver = {
> +               .name = "amd_halo_led",
> +       },
> +       .id_table = amd_halo_id_table,
> +       .probe = amd_halo_probe,
> +       .no_singleton = true,
> +};
> +
> +module_wmi_driver(amd_halo_driver);
> +
> +MODULE_AUTHOR("Mario Limonciello (AMD) <superm1@xxxxxxxxxx>");
> +MODULE_AUTHOR("Leo Lin <Leo.Lin@xxxxxxx>");
> +MODULE_DESCRIPTION("AMD Halo Box RGB LED Control Driver");
> +MODULE_LICENSE("GPL");
> 
> ---
> base-commit: 39704f00f747aba3144289870b5fd8ac230a9aaf
> change-id: 20260429-halo-leds-v2-plus-722c8083afe8
> 
> Best regards,
> --
> Yo-Jung Leo Lin (AMD) <leo.lin@xxxxxxx>
> 
>