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

[Lin, Leo]

>
>
>
>________________________________________
>From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>Sent: Thursday, May 28, 2026 8:29 PM
>To: Lin, Leo
>Cc: Hans de Goede; Mario Limonciello (AMD); LKML; platform-driver-x86@xxxxxxxxxxxxxxx; S-k, Shyam-sundar; Armin Wolf
>Subject: Re: [PATCH v5] platform/x86/amd: Introduce Halo Box RGB LED driver
>
>On Sat, 23 May 2026, Yo-Jung Leo Lin (AMD) wrote:
>
>> 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)
>> - All 3 channels are configured at once with a single WMI method call
>> - Value range: 0-100 (matching hardware range directly)
>>
>> Co-developed-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx>
>> Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>> Reviewed-by: Armin Wolf <W_Armin@xxxxxx>
>> Signed-off-by: Yo-Jung Leo Lin (AMD) <Leo.Lin@xxxxxxx>
>
>This seems mostly okay, there are a few minor things still to address
>marked below.
>
>> ---
>> v4 -> v5
>> No code chang. Only email etiquette fixes
>> - Fix the "From: " in the mail from leo.lin@xxxxxxx to Leo.Lin@xxxxxxx,
>>   so that it matches s-o-b
>> - Pull the Reviewed-by trailer from previous threads.
>> - Link to v4: https://lore.kernel.org/r/20260521-halo-leds-v2-plus-v4-1-b498bbab36a2@xxxxxxx
>>
>> v3 -> v4
>> There's a new WMI method we introduced in BIOS for color setting, to
>> replace the original ones, so in this version:
>> - Introduce a new "AMD_HALO_WMI_RGB" method ID. This WMI method takes 1
>>   command argument (GET/SET) and 3 RGB values. This method is capable of
>>   turning on and setting all RGB channels at once in a single call, reducing
>>   some undesirable visual effects when setting them separately.
>> - Replace color setting code with this new "AMD_HALO_WMI_RGB" method.
>> - Remove implementations of unused WMI methods.
>>
>> This version also fixes things pointed out in the previous version:
>> - Explicltly specify the endianess of WMI arguments as __le32, and use
>>   endian-conversion helpers accordingly.
>> - Use scope-based cleanup helper for WMI buffer.
>> - Remove cleanup for channel setting WMI in probe.
>> - Explicitly include headers for kfree and le16_to_cpu
>> - Fix white spaces
>> - Fix designated initializor style
>> - Link to v3: https://lore.kernel.org/r/20260508-halo-leds-v2-plus-v3-1-91df458966ea@xxxxxxx
>>
>> (Didn't pull Shyam's Reviewed-by because this is a relatively bigger change.)
>>
>> v2 -> v3:
>> - Re-introduce OF/OFF method: implement them with wmidev_invoke_method
>> - Explicitly call the ON method on brightness_set callback for
>>   brightness != 0; call the OFF method when brightness == 0
>> - Turn off light bar if TURN_ON/SET_LIGHTBAR can't function normally
>> - Replace get_unaligned_le64 with casting and le16_to_cpu()
>> - Add an inline function (wmi_status_to_err) to convert WMI return
>>   values to errno
>> - Use amd_halo_wmi_set_channel in probe to set default color
>> - Use devm_mutex_init() instead
>> - Remove unused includes
>> - Link to v2: https://lore.kernel.org/r/20260504-halo-leds-v2-plus-v2-1-af027727605b@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 | 267 ++++++++++++++++++++++++++++++++
>>  4 files changed, 286 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8063cb56feef..cde3865addb4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1138,6 +1138,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..f9ee6368f8ea
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/amd_halo_led.c
>> @@ -0,0 +1,267 @@
>> +// 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/led-class-multicolor.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/wmi.h>
>> +#include <linux/byteorder/generic.h>
>> +#include <linux/slab.h>
>
>These must be in alphabetical order.

Ok, will fix it.

>
>Put the byteorder one into a separate group separated with a blank line.
>
>> +
>> +#define AMD_HALO_GUID "081E747B-E028-4232-AF24-EAAEAB2B1E86"
>> +
>> +/* WMI method IDs from MOF */
>> +enum {
>> +     AMD_HALO_WMI_TURN_OFF = 0x04,
>> +     AMD_HALO_WMI_RGB = 0x07
>> +};
>> +
>> +/* Arg0 of the AMD_HALO_WMI_RGB Method */
>> +enum {
>> +     AMD_HALO_RGB_CMD_GET,
>> +     AMD_HALO_RGB_CMD_SET
>> +};
>> +
>> +/* Status codes from spec */
>> +#define AMD_HALO_STATUS_SUCCESS              0x0000
>> +#define AMD_HALO_STATUS_INVALID_PARAM        0xFFFD
>> +
>> +/* 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
>> +
>> +/**
>> + * 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 {
>> +     __le32 arg0;
>> +     __le32 arg1;
>
>Please add types.h

Will add all the includes you mention here in the next version (and sort them accordingly).

>
>> +};
>> +
>> +struct amd_halo_wmi_args_rgb {
>> +     __le32 cmd;
>> +     __le32 red;
>> +     __le32 green;
>> +     __le32 blue;
>> +};
>> +
>> +static inline int wmi_status_to_err(u16 status)
>> +{
>> +     switch (status) {
>> +     case AMD_HALO_STATUS_SUCCESS:
>> +             return 0;
>> +     case AMD_HALO_STATUS_INVALID_PARAM:
>> +             return -EINVAL;
>> +     default:
>> +             return -EIO;
>> +     }
>> +}
>> +
>> +static int __amd_halo_wmi_call(struct wmi_device *wdev,
>> +                            u32 method_id, void *data, size_t length)
>> +{
>> +     struct wmi_buffer input = {
>> +             .length = length,
>> +             .data = data,
>> +     };
>> +     __le16 *result_status __free(kfree) = NULL;
>> +     struct wmi_buffer output = {};
>> +     int ret;
>> +
>> +     ret = wmidev_invoke_method(wdev, 0, method_id,
>> +                                &input, &output, sizeof(*result_status));
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Return buffer per spec: Bytes[0:1] = Status (little-endian) */
>> +     result_status = output.data;
>
>Please move the variable declaration here (mid-function) as explained in
>the long comment in cleanup.h.

Ok, will fix this.

>
>> +     ret = wmi_status_to_err(le16_to_cpu(*result_status));
>> +
>> +     return ret;
>
>        return wmi_status_to_err(...);
>
>> +}
>> +
>> +/**
>> + * amd_halo_wmi_turn_off - Turn off all LED channels
>> + * @wdev: WMI device pointer
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int amd_halo_wmi_turn_off(struct wmi_device *wdev)
>> +{
>> +     struct amd_halo_wmi_args args = { };
>> +
>> +     return __amd_halo_wmi_call(wdev, AMD_HALO_WMI_TURN_OFF, &args, sizeof(args));
>> +}
>> +
>> +/**
>> + * amd_halo_wmi_set_rgb - Set all RGB channels atomically
>> + * @wdev: WMI device pointer
>> + * @r: brightness for red channel (0 - 100)
>> + * @g: brightness for green channel (0 - 100)
>> + * @b: brightness for blue channel (0 - 100)
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int amd_halo_wmi_set_rgb(struct wmi_device *wdev, u32 r, u32 g, u32 b)
>> +{
>> +     if (r > AMD_HALO_MAX_HW_BRIGHTNESS ||
>> +         g > AMD_HALO_MAX_HW_BRIGHTNESS ||
>> +         b > AMD_HALO_MAX_HW_BRIGHTNESS) {
>> +             return -EINVAL;
>> +     }
>> +
>> +     struct amd_halo_wmi_args_rgb args = {
>> +             .cmd = cpu_to_le32(AMD_HALO_RGB_CMD_SET),
>> +             .red = cpu_to_le32(r),
>> +             .green = cpu_to_le32(g),
>> +             .blue = cpu_to_le32(b)
>
>Add the trailing comma.

Ok, will fix this (and all the other details you mentioned).

Thanks for your review!

BR,
Leo

>
>> +     };
>> +
>> +     return __amd_halo_wmi_call(wdev, AMD_HALO_WMI_RGB, &args, sizeof(args));
>> +}
>> +
>> +/**
>> + * 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,
>
>Add container_of.h
>
>> +                                                   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);
>> +
>> +     if (brightness == 0)
>> +             return amd_halo_wmi_turn_off(data->wdev);
>> +
>> +     red_hw = mc_cdev->subled_info[0].brightness;
>> +     green_hw = mc_cdev->subled_info[1].brightness;
>> +     blue_hw = mc_cdev->subled_info[2].brightness;
>> +
>> +     ret = amd_halo_wmi_set_rgb(data->wdev, red_hw, green_hw, blue_hw);
>> +     if (ret)
>> +             goto out;
>> +
>> +     return 0;
>> +
>> +out:
>> +     /*
>> +      * Consider the light bar non-functional if AMD_HALO_WMI_RGB failed.
>> +      * Attempt to turn the LED off completely as clean-up.
>> +      */
>> +     if (amd_halo_wmi_turn_off(data->wdev))
>> +             dev_warn_ratelimited(&data->wdev->dev, "Failed to turn LED off on cleanup\n");
>
>Please add include for this.
>
>> +
>> +     return ret;
>> +}
>> +
>> +/**
>> + * 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;
>> +
>> +     ret = devm_mutex_init(&wdev->dev, &data->lock);
>> +     if (ret)
>> +             return ret;
>> +
>> +     data->wdev = wdev;
>> +     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;
>> +     data->led_mc.num_colors = ARRAY_SIZE(data->subled_info);
>
>Add include.
>
>> +     data->led_mc.subled_info = data->subled_info;
>> +
>> +     ret = amd_halo_wmi_set_rgb(wdev, AMD_HALO_DEFAULT_RED, AMD_HALO_DEFAULT_GREEN,
>> +                                AMD_HALO_DEFAULT_BLUE);
>> +     if (ret)
>> +             return ret;
>> +
>> +     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("Yo-Jung Leo Lin (AMD) <Leo.Lin@xxxxxxx>");
>> +MODULE_DESCRIPTION("AMD Halo Box RGB LED Control Driver");
>> +MODULE_LICENSE("GPL");
>>
>> ---
>> base-commit: 687da68900cd1a46549f7d9430c7d40346cb86a0
>> change-id: 20260429-halo-leds-v2-plus-722c8083afe8
>>
>> Best regards,
>>
>
>--
> i.
>
>