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

[Denis Benato <denis.benato@linux.dev>]

On 6/15/26 23:55, Antheas Kapenekakis wrote:
> On Mon, 15 Jun 2026 at 18:51, Denis Benato <denis.benato@linux.dev> 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@linux.dev>
>> ---
>>  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,
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@antheas.dev>
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
>>
>>