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

[Armin Wolf <W_Armin@gmx.de>]

Am 06.05.26 um 11:26 schrieb Lin, Leo:
> AMD General
> 
>> -----Original Message-----
>> From: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>> Sent: Wednesday, May 6, 2026 12:34 PM
>> To: Lin, Leo <Leo.Lin@amd.com>; Hans de Goede <hansg@kernel.org>; Ilpo
>> Järvinen <ilpo.jarvinen@linux.intel.com>; Mario Limonciello (AMD)
>> <superm1@kernel.org>
>> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org
>> Subject: Re: [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver
>>
>>
>>
>> On 5/4/2026 19:51, Yo-Jung Leo Lin (AMD) wrote:
>>> [You don't often get email from leo.lin@amd.com. 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@kernel.org>
>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>> Signed-off-by: Yo-Jung Leo Lin (AMD) <Leo.Lin@amd.com>
>>> ---
>>> 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@kernel.org>
>>> +R:     Yo-Jung Leo Lin (AMD) <Leo.Lin@amd.com>
>>> +L:     platform-driver-x86@vger.kernel.org
>>> +S:     Supported
>>> +F:     drivers/platform/x86/amd/amd_halo_led.c
>>> +
>>>   AMD HSMP DRIVER
>>>   M:     Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>>>   R:     Carlos Bilbao <carlos.bilbao@kernel.org>
>>> 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.
> 
> Will remove this in the next version

I think this is an error, turning the LED on and off should be done by 
the driver when setting the brightness. I suggest that the LED is turned 
on when the brightness is greater than 0, otherwise it is turned off.

Thanks,
Armin Wolf

> 
>>
>>> +};
>>> +
>>> +/* 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.
> 
> Ok, will (see below) keep this macro and do what you suggested.
> 
>>
>>> +
>>> +/* 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?
> 
> It is intentional. They are tuned this way to compensate the color-filtering from the materials of the chassis. This is actually much closer to white than the values might appear to be at first look.
> 
>>
>>> +
>>> +/**
>>> + * 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);

The WMI driver core already ensures that the le16 is properly aligned, 
you can safely cast output.data to a __le16 * and then use le16_to_cpu().

>>> +       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.
> 
> Will keep the macro and do this in the next version.

I would prefer if we could avoid having a macro that affects control 
flow, a inline function that turns the status code into a errno
is better.

> 
>>
>>> +
>>> +       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
> 
> Not sure if it's a good idea to use the same (already failing) WMI method to recover the WMI error here, but I guess it's probably better than not recovering it at all. Will add the recovery logic in the next version.
> 
>>
>>> +}
>>> +
>>> +/**
>>> + * 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?
> 
> BIOS may change LED color during suspend, so this has to be restored after a resume.
> 
>>
>>> +       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?
> 
> It is intentional. For some reasons, the WMI values returned from BIOS won't be accurate after a cold boot, so set them to some known values here.

I dont know if reusing the callback with a partially uninitalized LED 
device is a good idea. Why not manually calling 
amd_halo_wmi_set_channel() and friends?

Thanks,
Armin Wolf

> 
>>
>>> +       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@kernel.org>");
>>> +MODULE_AUTHOR("Leo Lin <Leo.Lin@amd.com>");
>> 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@amd.com>
>>>
>>>
>