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

[Armin Wolf <W_Armin@gmx.de>]

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");
>>
>
>