* [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver @ 2026-05-04 14:21 Yo-Jung Leo Lin (AMD) 2026-05-06 4:33 ` Shyam Sundar S K 0 siblings, 1 reply; 4+ messages in thread From: Yo-Jung Leo Lin (AMD) @ 2026-05-04 14:21 UTC (permalink / raw) To: Hans de Goede, Ilpo Järvinen, Mario Limonciello (AMD), Yo-Jung Leo Lin (AMD) Cc: linux-kernel, platform-driver-x86 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 +}; + +/* 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 + +/* 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 { + 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; + + 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); +} + +/** + * 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; + 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); + 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> ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver 2026-05-04 14:21 [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver Yo-Jung Leo Lin (AMD) @ 2026-05-06 4:33 ` Shyam Sundar S K 2026-05-06 9:26 ` Lin, Leo 0 siblings, 1 reply; 4+ messages in thread From: Shyam Sundar S K @ 2026-05-06 4:33 UTC (permalink / raw) To: Yo-Jung Leo Lin (AMD), Hans de Goede, Ilpo Järvinen, Mario Limonciello (AMD) Cc: linux-kernel, platform-driver-x86 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. > +}; > + > +/* 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@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> > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver 2026-05-06 4:33 ` Shyam Sundar S K @ 2026-05-06 9:26 ` Lin, Leo 2026-05-06 10:41 ` Armin Wolf 0 siblings, 1 reply; 4+ messages in thread From: Lin, Leo @ 2026-05-06 9:26 UTC (permalink / raw) To: S-k, Shyam-sundar, Hans de Goede, Ilpo Järvinen, Mario Limonciello (AMD) Cc: linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org 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 > > > +}; > > + > > +/* 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); > > + 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. > > > + > > + 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. > > > + 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> > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver 2026-05-06 9:26 ` Lin, Leo @ 2026-05-06 10:41 ` Armin Wolf 0 siblings, 0 replies; 4+ messages in thread From: Armin Wolf @ 2026-05-06 10:41 UTC (permalink / raw) To: Lin, Leo, S-k, Shyam-sundar, Hans de Goede, Ilpo Järvinen, Mario Limonciello (AMD) Cc: linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org 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> >>> >>> > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-05-06 10:41 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-04 14:21 [PATCH v2] platform/x86/amd: Introduce Halo Box RGB LED driver Yo-Jung Leo Lin (AMD) 2026-05-06 4:33 ` Shyam Sundar S K 2026-05-06 9:26 ` Lin, Leo 2026-05-06 10:41 ` Armin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox