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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8201 bytes --]

On Thu, 9 Jul 2026, Lin, Leo wrote:

> >
> >
> >
> >________________________________________
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Thursday, July 9, 2026 6:25 PM
> >To: Lin, Leo
> >Cc: Hans de Goede; Mario Limonciello (AMD); LKML; platform-driver-x86@vger.kernel.org; S-k, Shyam-sundar; Armin Wolf
> >Subject: Re: [PATCH v8] platform/x86/amd: Introduce Halo Box RGB LED driver
> >
> >On Thu, 9 Jul 2026, Yo-Jung Leo Lin (AMD) wrote:
> >
> >> The Halo Box features an RGB LED light bar that can be controlled
> >> through WMI methods to display any color combination.
> >>
> >> The driver exposes the LED through the LED multicolor subsystem,
> >> allowing userspace to control RGB values via sysfs:
> >>   /sys/class/leds/amd_halo:multicolor:status/multi_intensity
> >>   /sys/class/leds/amd_halo:multicolor:status/brightness
> >>
> >> Hardware interface:
> >> - Three separate RGB channels (Red, Green, Blue)
> >> - All 3 channels are configured at once with a single WMI method call
> >> - Value range: 0-100 (matching hardware range directly)
> >>
> >> Co-developed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> >> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> >> Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
> >> Signed-off-by: Yo-Jung Leo Lin (AMD) <Leo.Lin@amd.com>
> >> ---
> >> To: Hans de Goede <hansg@kernel.org>
> >> To: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >> To: Mario Limonciello (AMD) <superm1@kernel.org>
> >> To: Yo-Jung Leo Lin (AMD) <Leo.Lin@amd.com>
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: platform-driver-x86@vger.kernel.org
> >>
> >> v7 -> v8
> >> - Move local variables to the beginnign of function. (Ilpo)
> >> - Add trailing commas to enum declaratiion. (Ilpo)
> >> - Fix more white spaces
> >> - Remove redundant comments
> >> - Link to v7: https://lore.kernel.org/r/20260611-halo-leds-v2-plus-v7-1-8aad73fccb6b@amd.com
> >>
> >> v6 -> v7
> >> - Introduce a amd_halo_wmi_get_rgb() to get WMI values of colors, and
> >>   read them on probe.
> >> - Remove hard-coded RGB values.
> >> - Link to v6: https://lore.kernel.org/r/20260529-halo-leds-v2-plus-v6-1-b7bf7b904a16@amd.com
> >>
> >> v5 -> v6
> >> - Add missing includes and sort/group them accordingly
> >> - In __amd_halo_wmi_call, remove some unnecessary intermedaite variable assignments
> >> - In __amd_halo_wmi_call, move declaration of pointer managed by scope-based cleanup
> >>   helpers closer to where it is used
> >> - Add trailing comma to designated initializers
> >> - Link to v5: https://lore.kernel.org/r/20260523-halo-leds-v2-plus-v5-1-497dc9719a18@amd.com
> >> ---
> >>  MAINTAINERS                             |   7 +
> >>  drivers/platform/x86/amd/Kconfig        |  11 ++
> >>  drivers/platform/x86/amd/Makefile       |   1 +
> >>  drivers/platform/x86/amd/amd_halo_led.c | 315 ++++++++++++++++++++++++++++++++
> >>  4 files changed, 334 insertions(+)
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 15011f5752a99..3b774bc8889c5 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -1138,6 +1138,13 @@ F:     drivers/char/hw_random/geode-rng.c
> >>  F:   drivers/crypto/geode*
> >>  F:   drivers/video/fbdev/geode/
> >>
> >> +AMD HALO BOX RGB LED DRIVER
> >> +M:   Mario Limonciello (AMD) <superm1@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 b813f92653686..a1a74ef6c8593 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 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..b21d956c7d96c
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/amd/amd_halo_led.c
> >> @@ -0,0 +1,315 @@
> >> +// 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/array_size.h>
> >> +#include <linux/cleanup.h>
> >> +#include <linux/compiler_attributes.h>
> >> +#include <linux/container_of.h>
> >> +#include <linux/dev_printk.h>
> >> +#include <linux/device.h>
> >> +#include <linux/device/devres.h>
> >> +#include <linux/led-class-multicolor.h>
> >> +#include <linux/leds.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/types.h>
> >> +#include <linux/wmi.h>
> >> +
> >> +#include <linux/byteorder/generic.h>
> >> +
> >> +#define AMD_HALO_GUID "081E747B-E028-4232-AF24-EAAEAB2B1E86"
> >> +
> >> +/* WMI method IDs from MOF */
> >> +enum {
> >> +     AMD_HALO_WMI_TURN_OFF   = 0x04,
> >> +     AMD_HALO_WMI_RGB        = 0x07,
> >> +};
> >> +
> >> +/* Arg0 of the AMD_HALO_WMI_RGB Method */
> >> +enum {
> >> +     AMD_HALO_RGB_CMD_GET    = 0x0,
> >> +     AMD_HALO_RGB_CMD_SET    = 0x1,
> >> +};
> >> +
> >> +/* 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
> >> +
> 
> Hi Ilpo,
> 
> >> +/**
> >> + * 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
> 
> Here's the comment for the lock, which basically means the same thing as the removed inline comment.
> 
> >> + */
> >> +struct amd_halo_led_data {
> >> +     struct wmi_device *wdev;
> >> +     struct led_classdev_mc led_mc;
> >> +     struct mc_subled subled_info[3];
> >> +     struct mutex lock;
> >
> >Please put the comment back to explain what this lock protects.
> 
> There's already kdoc comment in the beginning of this function that 
> explains this, and that's why I removed the inline comment. 

Ah. I was only looking at the between versions diff. I'm sorry about not 
noticing the other comment.

-- 
 i.