Re: [PATCH v2 3/4] HID: asus: add support for xgm led

[Antheas Kapenekakis]

On Fri, 12 Jun 2026 at 16:23, Denis Benato <denis.benato@xxxxxxxxx> wrote:
>
> XG mobile stations have very bright leds behind the fan that can be
> turned either ON or OFF: add a cled interface to allow controlling the
> brightness of those red leds.
>
> Signed-off-by: Denis Benato <denis.benato@xxxxxxxxx>
> ---
>  drivers/hid/hid-asus.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 323dc0b7f3ff..21c4a60d224e 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -51,6 +51,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define FEATURE_KBD_LED_REPORT_ID1 0x5d
>  #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> +#define ROG_XGM_REPORT_SIZE 300
> +
>  #define ROG_ALLY_REPORT_SIZE 64
>  #define ROG_ALLY_X_MIN_MCU 313
>  #define ROG_ALLY_MIN_MCU 319
> @@ -118,6 +120,11 @@ struct asus_kbd_leds {
>         bool removed;
>  };
>
> +struct asus_xgm_led {
> +       struct led_classdev cdev;
> +       struct hid_device *hdev;
> +};
> +
>  struct asus_touchpad_info {
>         int max_x;
>         int max_y;
> @@ -143,6 +150,7 @@ struct asus_drvdata {
>         unsigned long battery_next_query;
>         struct work_struct fn_lock_sync_work;
>         bool fn_lock;
> +       struct asus_xgm_led *xgm_led;
>  };
>
>  static int asus_report_battery(struct asus_drvdata *, u8 *, int);
> @@ -941,6 +949,23 @@ static int asus_battery_probe(struct hid_device *hdev)
>         return ret;
>  }
>
> +static int asus_xgm_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +       const u8 buf[ROG_XGM_REPORT_SIZE] = {
> +               FEATURE_KBD_LED_REPORT_ID2, 0xC5, (value) ? 0x50 : 0x00
> +       };
> +       struct asus_xgm_led *xgm = container_of(led_cdev, struct asus_xgm_led, cdev);
> +       int ret;
> +
> +       ret = asus_kbd_set_report(xgm->hdev, buf, ROG_XGM_REPORT_SIZE);
> +       if (ret != ROG_XGM_REPORT_SIZE) {
> +               hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  {
>         struct input_dev *input = hi->input;
> @@ -1184,6 +1209,14 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>                 }
>         }
>
> +       if (drvdata->xgm_led) {
> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> +                       goto asus_resume_err;
> +               }
> +       }
> +
>  asus_resume_err:
>         return ret;
>  }
> @@ -1310,6 +1343,40 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 }
>         }
>
> +       if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> +           ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> +            (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> +               drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> +               if (drvdata->xgm_led == NULL) {
> +                       ret = -ENOMEM;
> +                       goto err_stop_hw;
> +               }
> +               drvdata->xgm_led->hdev = hdev;
> +               drvdata->xgm_led->cdev.name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
> +                                       "asus:xgm-%s:led",
> +                                       strlen(hdev->uniq) ?
> +                                       hdev->uniq : dev_name(&hdev->dev));
> +               drvdata->xgm_led->cdev.brightness = 1;
> +               drvdata->xgm_led->cdev.max_brightness = 1;
> +               drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> +               /*
> +                * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
> +                * what the default brightness resets when doing a cold boot.
> +                */

I think this is set by the driver, so you should reformat the comment
above, so you should trim the comment.

Perhaps, "The LED state is arbitrary on boot, therefore default to the
initial brightness set above". This way it does not become outdated if
cdev.brightness changes.

> +               ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> +                       goto err_stop_hw;
> +               }

You already do this in asus_input_configured so you do it twice?
Perhaps skip one if you end up keeping them? I think that it's better
to keep this block.

Or even better return an error in _get so that on boot it is
ambiguous? I assume the leds remain to the state they had prior to the
reboot? With this change, imagine a user that turned off the leds in
windows, permabooted into Linux, and now has the lights always turn on
during boot.

Moreover, can systemd restore this or is it out of scope for its led
handler? Perhaps it is an ambitious idea though, and better skipped.

> +
> +               ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> +               if (ret) {
> +                       hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +                       goto err_stop_hw;
> +               }
> +       }
> +
>         /* Laptops keyboard backlight is always at 0x5a */
>         if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>             (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&
> @@ -1366,6 +1433,9 @@ static void asus_remove(struct hid_device *hdev)
>         if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>                 cancel_work_sync(&drvdata->fn_lock_sync_work);
>
> +       if (drvdata->xgm_led)
> +               led_classdev_unregister(&drvdata->xgm_led->cdev);
> +
>         hid_hw_stop(hdev);
>  }
>
> --
> 2.47.3
>
>