* [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver
@ 2026-04-27 2:25 Mario Limonciello (AMD)
2026-04-28 21:12 ` Armin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Mario Limonciello (AMD) @ 2026-04-27 2:25 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, open list, open list:X86 PLATFORM DRIVERS,
Mario Limonciello (AMD), Leo.Lin
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
+ 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
+
+/* 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);
+ if (ACPI_FAILURE(status)) {
+ dev_err(&wdev->dev, "SetLightBar failed: %s\n",
+ acpi_format_exception(status));
+ 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;
+ }
+
+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;
+
+ 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;
+ }
+
+ /* 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);
+
+ 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);
+
+ 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;
+ }
+
+ 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);
+}
+
+/**
+ * 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";
+ 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;
+ 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");
+
+ ret = amd_halo_color_set_default(&data->led_mc.led_cdev);
+ 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,
+ .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");
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver
2026-04-27 2:25 [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver Mario Limonciello (AMD)
@ 2026-04-28 21:12 ` Armin Wolf
2026-04-28 21:26 ` Mario Limonciello
0 siblings, 1 reply; 4+ messages in thread
From: Armin Wolf @ 2026-04-28 21:12 UTC (permalink / raw)
To: Mario Limonciello (AMD), Ilpo Järvinen
Cc: Hans de Goede, open list, open list:X86 PLATFORM DRIVERS, Leo.Lin
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");
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver
2026-04-28 21:12 ` Armin Wolf
@ 2026-04-28 21:26 ` Mario Limonciello
2026-04-28 21:35 ` Armin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Mario Limonciello @ 2026-04-28 21:26 UTC (permalink / raw)
To: Armin Wolf, Ilpo Järvinen
Cc: Hans de Goede, open list, open list:X86 PLATFORM DRIVERS, Leo.Lin
On 4/28/26 16:12, Armin Wolf wrote:
> 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".
Sashiko also noted that you can't select LEDS_CLASS_MULTICOLOR withou
selecting LEDS. I think switching to depends solves it sufficiently though.
>
>> + 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.
Sure, no problem.
I guess in the future you envision decompiling and parsing the BMOF in
kernel and this can be cross referenced, right?
If you have a specific schema that you want it put in LMK.
>
>> +
>> +/* 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.
Ack.
>
>> + 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.
Ack.
>
>> + 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).
>
Ack.
>> + }
>> +
>> +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().
Will double check.
>
>> +
>> + 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.
OK thanks.
>
>> +
>> + 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.
It's mostly an BIOS/EC issue. The initial state value when the system
boots from S5 is wrong. But if you go into s0i3, reboot, or s4
everything is fine and state is correct.
That's why amd_halo_color_set_default() existed. Will fold that into probe.
>
>> +
>> + 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.
OK.
>
>> + 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.
OK.
>
>> + 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.
Thanks - yeah this was something I noticed raised by Sashiko as well.
>
>> + 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.
Thank you very much for the review.
>
> 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");
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver
2026-04-28 21:26 ` Mario Limonciello
@ 2026-04-28 21:35 ` Armin Wolf
0 siblings, 0 replies; 4+ messages in thread
From: Armin Wolf @ 2026-04-28 21:35 UTC (permalink / raw)
To: Mario Limonciello, Ilpo Järvinen
Cc: Hans de Goede, open list, open list:X86 PLATFORM DRIVERS, Leo.Lin
Am 28.04.26 um 23:26 schrieb Mario Limonciello:
> On 4/28/26 16:12, Armin Wolf wrote:
>> 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".
>
> Sashiko also noted that you can't select LEDS_CLASS_MULTICOLOR withou
> selecting LEDS. I think switching to depends solves it sufficiently
> though.
>
>>
>>> + 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.
>
> Sure, no problem.
>
> I guess in the future you envision decompiling and parsing the BMOF in
> kernel and this can be cross referenced, right?
Yes, i already wrote a python tool for this, but it needs some more work :/
>
> If you have a specific schema that you want it put in LMK.
>
Not really, you can use the existing device documentations as a reference.
>>
>>> +
>>> +/* 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.
>
> Ack.
>
>>
>>> + 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.
>
> Ack.
>
>>
>>> + 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).
>>
>
> Ack.
>
>>> + }
>>> +
>>> +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().
>
> Will double check.
>
>>
>>> +
>>> + 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.
>
> OK thanks.
>
>>
>>> +
>>> + 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.
>
> It's mostly an BIOS/EC issue. The initial state value when the system
> boots from S5 is wrong. But if you go into s0i3, reboot, or s4
> everything is fine and state is correct.
>
As long as you cannot read the gobal brightness, brightness_get() should not be populated.
> That's why amd_halo_color_set_default() existed. Will fold that into
> probe.
Sound fine to me.
>
>>
>>> +
>>> + 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.
>
> OK.
>
>>
>>> + 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.
>
> OK.
>
>>
>>> + 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.
>
> Thanks - yeah this was something I noticed raised by Sashiko as well.
>
>>
>>> + 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.
>
> Thank you very much for the review.
>
Thank you for contributing this driver :)
Thanks,
Armin Wolf
>>
>> 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");
>>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-04-28 21:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-27 2:25 [PATCH] platform/x86/amd: Introduce Halo Box RGB LED driver Mario Limonciello (AMD)
2026-04-28 21:12 ` Armin Wolf
2026-04-28 21:26 ` Mario Limonciello
2026-04-28 21:35 ` Armin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox