Re: [PATCH v4 4/5] HID: asus: add support for xgm led

[Antheas Kapenekakis]

On Wed, 17 Jun 2026 at 02:18, Denis Benato <denis.benato@xxxxxxxxx> wrote:
>
>
> On 6/15/26 23:55, Antheas Kapenekakis wrote:
> > On Mon, 15 Jun 2026 at 18:51, 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 | 91 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 91 insertions(+)
> >>
> >> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> >> index 6896730efafc..e68a6d93369d 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
> >> @@ -143,6 +145,11 @@ struct asus_worker {
> >>         bool removed;
> >>  };
> >>
> >> +struct asus_xgm_led {
> >> +       struct led_classdev cdev;
> >> +       struct hid_device *hdev;
> >> +};
> >> +
> >>  struct asus_touchpad_info {
> >>         int max_x;
> >>         int max_y;
> >> @@ -169,6 +176,7 @@ struct asus_drvdata {
> >>         unsigned long battery_next_query;
> >>         struct asus_hid_listener listener;
> >>         bool fn_lock;
> >> +       struct asus_xgm_led *xgm_led;
> >>  };
> >>
> >>  static int asus_report_battery(struct asus_drvdata *, u8 *, int);
> >> @@ -1119,6 +1127,26 @@ 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 < 0) {
> >> +               hid_err(xgm->hdev, "Unable to set XG mobile led state: %d\n", ret);
> >> +               return ret;
> >> +       } else if (ret != ROG_XGM_REPORT_SIZE) {
> >> +               hid_err(xgm->hdev, "Unexpected partial transfer to XG mobile: %d\n", ret);
> >> +               return -EIO;
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>  static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> >>  {
> >>         struct input_dev *input = hi->input;
> >> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
> >>         return 0;
> >>  }
> >>
> >> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
> >> +{
> >> +       const char *name;
> >> +       int ret;
> >> +
> >> +       drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
> >> +       if (drvdata->xgm_led == NULL)
> >> +               return -ENOMEM;
> >> +
> >> +       name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "asus:xgm-%s:led",
> >> +                             strlen(hdev->uniq) ? hdev->uniq : dev_name(&hdev->dev));
> >> +
> >> +       if (name == NULL) {
> >> +               ret = -ENOMEM;
> >> +               goto asus_xgm_init_err;
> >> +       }
> >> +
> >> +       drvdata->xgm_led->hdev = hdev;
> >> +       drvdata->xgm_led->cdev.name = name;
> >> +       drvdata->xgm_led->cdev.brightness = 1;
> >> +       drvdata->xgm_led->cdev.max_brightness = 1;
> >> +       drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> >> +
> >> +       /* LED state is arbitrary on boot, set a default */
> >> +       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 asus_xgm_init_err;
> >> +       }
> >> +
> >> +       ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
> >> +       if (ret) {
> >> +               hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> >> +               goto asus_xgm_init_err;
> >> +       }
> >> +
> >> +       return 0;
> >> +asus_xgm_init_err:
> >> +       drvdata->xgm_led = NULL;
> >> +       return ret;
> >> +}
> >> +
> >>  static int __maybe_unused asus_resume(struct hid_device *hdev)
> >>  {
> >>         struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> >> +       int ret;
> >>
> >>         /*
> >>          * If we have a backlight listener registered, restore the previous state,
> >> @@ -1355,7 +1426,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
> >>         if (drvdata->listener.brightness_set)
> >>                 asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
> >>
> >> +       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;
> >> +               }
> >> +       }
> >> +
> >>         return 0;
> >> +asus_resume_err:
> >> +       return ret;
> >>  }
> >>
> >>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> >> @@ -1484,6 +1565,16 @@ 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))) {
> >> +               ret = asus_xgm_init(hdev, drvdata);
> >> +               if (ret) {
> >> +                       hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> >> +                       goto err_stop_hw;
> >> +               }
> >> +       }
> >> +
> > reset_resume has a special meaning and perhaps should not have been
> > used for this driver at all. Check if resume works and if it does use
> > that instead.
> I am not using reset_resume for xgm.
>
> reset_resume will become important to recover the ally,

For that, review the V2 s2idle series I have sent, it should drop that
codepath and work for all ally units

Sorry, misread the hunk again, I was looking at probe and confused it
with the resume check. The patch should be fine, you use resume.

Antheas

> but for xg mobile is unused (and useless).
> > Other than that and the refactor patch I'd rather see separately, the
> > other 4 patches look fine to me.
> Fair, I'll wait for maintainers to tell what they like the most
> and go from there.
>
> I agree there are things not really much related here
> but at least this way the order to apply them is clear.
> > Reviewed-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> Thanks,
> Denis
> > Best,
> > Antheas
> >
> >>         /* 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)) &&
> >> --
> >> 2.47.3
> >>
> >>
>