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

[Denis Benato <benato.denis96@gmail.com>]

On 6/12/26 16:39, Antheas Kapenekakis wrote:
> On Fri, 12 Jun 2026 at 16:23, 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 | 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.
yeah better spelling. I agree.
>> +               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.
In asus_input_configured? Will take a look in the next days. I tought
the other was in asus_resume since at resume they resets back...
> 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.
Cold boot sets them to ON, while rebooting keep them at what they were.

After exiting from sleep they are always ON, but this is on an ally,
I don't know if on an old rog flow it's the same.
> 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.
I don't see realistic for this to fail if it was successful at probe so it
shouldn't matter. As for systemd restoring them it would have to
be informed that they changed (but there is no read-back) so either
way something has to happen at resume, but doing this means no
additional software is necessary and user preference is being
respected regardless of anything else.
>> +
>> +               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
>>
>>
Thanks,
Denis