Messages in this thread |  | | Date | Tue, 28 Apr 2026 23:12:38 +0200 | | Subject | Re: [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver | | From | Armin Wolf <> |
| |
Am 27.04.26 um 04:25 schrieb Mario Limonciello (AMD):
> 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:rgb:light_bar/multi_intensity > /sys/class/leds/amd_halo:rgb:light_bar/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: Leo.Lin@amd.com > Signed-off-by: Leo.Lin@amd.com > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org> > --- > MAINTAINERS | 7 + > drivers/platform/x86/amd/Kconfig | 12 + > drivers/platform/x86/amd/Makefile | 1 + > drivers/platform/x86/amd/amd_halo_led.c | 375 ++++++++++++++++++++++++ > 4 files changed, 395 insertions(+) > create mode 100644 drivers/platform/x86/amd/amd_halo_led.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 2fb1c75afd163..c35b654a65041 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: Leo Lin <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 b813f92653686..d642386938055 100644 > --- a/drivers/platform/x86/amd/Kconfig > +++ b/drivers/platform/x86/amd/Kconfig > @@ -34,6 +34,18 @@ 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 > + select LEDS_CLASS_MULTICOLOR
I do not think selecting whole subsystems is a good idea. Consider changing this to "depends on".
> + 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 f6ff0c837f345..2f467dbbfc8a3 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 0000000000000..01b4583ca70f3 > --- /dev/null > +++ b/drivers/platform/x86/amd/amd_halo_led.c > @@ -0,0 +1,375 @@ > +// 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/wmi.h> > + > +#define AMD_HALO_GUID "081E747B-E028-4232-AF24-EAAEAB2B1E86" > + > +/* WMI method IDs from MOF */ > +#define AMD_HALO_WMI_GET_LIGHTBAR 0x01 > +#define AMD_HALO_WMI_SET_LIGHTBAR 0x02 > +#define AMD_HALO_WMI_TURN_ON 0x03 > +#define AMD_HALO_WMI_TURN_OFF 0x04
Hi,
can you use an enum for that? Also i suggest you create a small piece of documentation under Documetnation/wmi/devices so that future developers understand how this WMI device is supposed to work.
> + > +/* Channel selectors for Arg0 */ > +#define AMD_HALO_CHANNEL_RED 0x01 > +#define AMD_HALO_CHANNEL_GREEN 0x02 > +#define AMD_HALO_CHANNEL_BLUE 0x03 > + > +/* 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 > + > +/** > + * 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, > + }; > + const struct acpi_buffer input = { > + .length = sizeof(args), > + .pointer = &args, > + }; > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + acpi_status status; > + u16 result_status; > + int ret = 0; > + > + /* Validate input per spec */ > + if (channel < AMD_HALO_CHANNEL_RED || > + channel > AMD_HALO_CHANNEL_BLUE || > + brightness > AMD_HALO_MAX_HW_BRIGHTNESS) > + return -EINVAL; > + > + status = wmidev_evaluate_method(wdev, 0, AMD_HALO_WMI_SET_LIGHTBAR, > + &input, &output);
Please use wmidev_invoke_method() instead. Take a look at Documentation/wmi/driver-development-guide.rst on how to use this new API.
> + if (ACPI_FAILURE(status)) { > + dev_err(&wdev->dev, "SetLightBar failed: %s\n", > + acpi_format_exception(status));
Please remove dev_err() here, the return value is enough.
> + return -EIO; > + } > + > + /* Parse return buffer per spec: Bytes[0:1] = Status */ > + obj = output.pointer; > + if (!obj || obj->type != ACPI_TYPE_BUFFER || obj->buffer.length < 2 || > + !obj->buffer.pointer) { > + dev_err(&wdev->dev, "Invalid return buffer\n"); > + ret = -EIO; > + goto out; > + } > + > + result_status = obj->buffer.pointer[0] | > + (obj->buffer.pointer[1] << 8); > + if (result_status != AMD_HALO_STATUS_SUCCESS) { > + dev_err(&wdev->dev, "WMI returned error: 0x%04x\n", > + result_status); > + ret = -EIO;
Please model result_status as a __le16 and perform a pointer cast from the buffer (after the size check of course).
> + } > + > +out: > + kfree(output.pointer); > + return ret; > +} > + > +/** > + * amd_halo_wmi_get_channel - Get a single RGB channel value > + * @wdev: WMI device pointer > + * @channel: Channel selector (1=Red, 2=Green, 3=Blue) > + * @value: Pointer to store the read value > + * > + * Return: 0 on success, negative error code on failure > + */ > +static int amd_halo_wmi_get_channel(struct wmi_device *wdev, u32 channel, > + u8 *value) > +{ > + struct amd_halo_wmi_args args = { > + .arg0 = channel, > + .arg1 = 0, /* Reserved */ > + }; > + const struct acpi_buffer input = { > + .length = sizeof(args), > + .pointer = &args, > + }; > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + union acpi_object *obj; > + acpi_status status; > + u16 result_status; > + int ret = 0; > + > + if (channel < AMD_HALO_CHANNEL_RED || channel > AMD_HALO_CHANNEL_BLUE) > + return -EINVAL; > + > + status = wmidev_evaluate_method(wdev, 0, AMD_HALO_WMI_GET_LIGHTBAR, > + &input, &output); > + if (ACPI_FAILURE(status)) > + return -EIO;
Same as above.
> + > + obj = output.pointer; > + if (!obj || obj->type != ACPI_TYPE_BUFFER || obj->buffer.length < 3 || > + !obj->buffer.pointer) { > + ret = -EIO; > + goto out; > + } > + > + result_status = obj->buffer.pointer[0] | > + (obj->buffer.pointer[1] << 8); > + if (result_status != AMD_HALO_STATUS_SUCCESS) { > + ret = -EIO; > + goto out; > + }
Same as above.
> + > + /* Value returned in Byte[2] per spec */ > + *value = obj->buffer.pointer[2]; > + > +out: > + kfree(output.pointer); > + return ret; > +} > + > +/** > + * 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 = { > + .arg0 = 0, > + .arg1 = 0, > + }; > + const struct acpi_buffer input = { > + .length = sizeof(args), > + .pointer = &args, > + }; > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_status status; > + > + status = wmidev_evaluate_method(wdev, 0, AMD_HALO_WMI_TURN_OFF, > + &input, &output); > + kfree(output.pointer);
This looks strange, does the WMI method return any data? If yes then please perform error checking, otherwise use wmidev_invoke_procedure().
> + > + return ACPI_FAILURE(status) ? -EIO : 0; > +} > + > +/** > + * amd_halo_brightness_set - Set LED brightness and color > + * @cdev: LED class device > + * @brightness: Brightness value (0 = off, >0 = on with color) > + * > + * 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); > + > + if (brightness == 0) > + return amd_halo_wmi_turn_off(data->wdev);
I suggest that you update the RGB color first, otherwise the LED will return wrong RGB values when turned off.
> + > + 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_brightness_get - Get current LED brightness state > + * @cdev: LED class device > + * > + * Return: brightness last set by the user, or AMD_HALO_MAX_HW_BRIGHTNESS if > + * it was set by BIOS, or LED_OFF if all LEDs are off. > + */ > +static enum led_brightness amd_halo_brightness_get(struct led_classdev *cdev) > +{ > + 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); > + u8 red_hw = 0, green_hw = 0, blue_hw = 0; > + int ret; > + > + guard(mutex)(&data->lock); > + > + /* Read each channel */ > + ret = amd_halo_wmi_get_channel(data->wdev, AMD_HALO_CHANNEL_RED, > + &red_hw); > + if (ret) > + return LED_OFF; > + > + ret = amd_halo_wmi_get_channel(data->wdev, AMD_HALO_CHANNEL_GREEN, > + &green_hw); > + if (ret) > + return LED_OFF; > + > + ret = amd_halo_wmi_get_channel(data->wdev, AMD_HALO_CHANNEL_BLUE, > + &blue_hw); > + if (ret) > + return LED_OFF; > + > + if (mc_cdev->subled_info[0].brightness == red_hw > + && mc_cdev->subled_info[1].brightness == green_hw > + && mc_cdev->subled_info[2].brightness == blue_hw) { > + return cdev->brightness; > + }
I do not think that this is a good idea. Maybe you should initialize the brightness values during probing and just not implement brightness_get at all as the hardware seems to have no possibility to read the global brightness.
> + > + mc_cdev->subled_info[0].intensity = red_hw; > + mc_cdev->subled_info[1].intensity = green_hw; > + mc_cdev->subled_info[2].intensity = blue_hw; > + > + return AMD_HALO_MAX_HW_BRIGHTNESS; > +} > + > +/** > + * amd_halo_color_set_default - Set LED to initial color and brightness > + * @cdev: LED class device > + * > + * Sets all RGB channels to 20% intensity as an initial state. > + * > + * Return: 0 on success, negative error code on failure > + */ > +static int amd_halo_color_set_default(struct led_classdev *cdev) > +{ > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > + > + mc_cdev->subled_info[0].intensity = AMD_HALO_MAX_HW_BRIGHTNESS / 5; > + mc_cdev->subled_info[1].intensity = AMD_HALO_MAX_HW_BRIGHTNESS / 5; > + mc_cdev->subled_info[2].intensity = AMD_HALO_MAX_HW_BRIGHTNESS / 5; > + > + return amd_halo_brightness_set(cdev, AMD_HALO_MAX_HW_BRIGHTNESS);
Please fold this into amd_halo_probe().
> +} > + > +/** > + * 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 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 = 0; > + data->subled_info[1].intensity = 0; > + data->subled_info[2].intensity = 0; > + > + data->led_mc.led_cdev.name = "amd_halo:rgb:light_bar";
I think the lightbar could be viewed as a status LED, so "amd_halo:multicolor:status" would be more suitable here. This would also mirror the uniwill-laptop driver.
> + 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.brightness_get = amd_halo_brightness_get; > + data->led_mc.led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN; > + data->led_mc.num_colors = 3;
Please use ARRAY_SIZE() here.
> + data->led_mc.subled_info = data->subled_info; > + > + ret = devm_led_classdev_multicolor_register(&wdev->dev, &data->led_mc); > + if (ret) > + return dev_err_probe(&wdev->dev, ret, > + "Failed to register multicolor LED\n");
Why not using devm_led_classdev_multicolor_register_ext() here?
> + > + ret = amd_halo_color_set_default(&data->led_mc.led_cdev);
Please do this _before_ registering the LED device, otherwise userspace applications might have already set a RGB color themself. Failing to set the initial hardware state should also result in probe failure.
> + if (ret) > + dev_warn(&wdev->dev, "Unable to set default LED intensity through WMI: %d\n", ret); > + > + 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,
Please set .no_singleton = true here.
Otherwise the driver looks good to me.
Thanks, Armin Wolf
> + .probe = amd_halo_probe, > +}; > + > +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");
|  |