Message-ID: <bb149ff1-5fbc-41ff-a4e8-51f6b8631b5e@gmail.com>
Date: Tue, 14 Oct 2025 00:06:30 +0200
From: Denis Benato <benato.denis96@...il.com>
To: Antheas Kapenekakis <lkml@...heas.dev>
Cc: platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>,
 Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
 <luke@...nes.dev>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v6 4/7] HID: asus: listen to the asus-wmi brightness
 device instead of creating one


On 10/13/25 23:57, Antheas Kapenekakis wrote:
> On Mon, 13 Oct 2025 at 23:44, Denis Benato <benato.denis96@...il.com> wrote:
>>
>> On 10/13/25 22:15, Antheas Kapenekakis wrote:
>>> Some ROG laptops expose multiple interfaces for controlling the
>>> keyboard/RGB brightness. This creates a name conflict under
>>> asus::kbd_brightness, where the second device ends up being
>>> named asus::kbd_brightness_1 and they are both broken.
>> Can you please reference a bug report and/or an analysis of why they ends
>> up being broken?
> You can reference the V1 description [1]
>
> [1] https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
oh okay thanks. I would suggest to keep relevant parts in successive revisions,
and most importantly repeat (a shorter description of) relevant parts on the proper
commit since commit messages will (hopefully) become part of the kernel,
because just reading messages of the current revision doesn't give the full picture
of the what and why,

Regards,
Denis
>>> Therefore, register a listener to the asus-wmi brightness device
>>> instead of creating a new one.
>>>
>>> Reviewed-by: Luke D. Jones <luke@...nes.dev>
>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
>>> ---
>>>  drivers/hid/hid-asus.c | 64 +++++++-----------------------------------
>>>  1 file changed, 10 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index a62559e3e064..0af19c8ef035 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -102,7 +102,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>  #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
>>>
>>>  struct asus_kbd_leds {
>>> -     struct led_classdev cdev;
>>> +     struct asus_hid_listener listener;
>> It is my understanding from "register a listener .... instead of creating a new one"
>> that you are attempting to use the same listener among many devices... so why isn't
>> this a pointer? And more importantly: why do we have bool available, bool registered
>> instead of either one or the other being replaced by this field being possibly NULL?
> A listener is the handle that is passed to asus-wmi so that it can
> communicate with hid-asus. Since the flow of communication flows from
> asus-wmi -> hid-asus, the pointer is placed on asus-wmi.
>
> The boolean kbd_led_avail is used to signify whether the BIOS supports
> RGB commands. If not, we still want the common handler to be there to
> link multiple hid-asus devices together. At the same time, we need to
> skip calling the bios commands for brightness, and hold a value for
> the previous brightness outside the bios.
>
> The kbd_led_registered fixes the race condition that happens between
> hid-asus and asus-wmi. Specifically, it ensures that the rgb listener
> is only setup once, either once asus-wmi loads (if it supports RGB) or
> when the first hid device loads.
>
> Best,
> Antheas
>
>>>       struct hid_device *hdev;
>>>       struct work_struct work;
>>>       unsigned int brightness;
>>> @@ -495,11 +495,11 @@ static void asus_schedule_work(struct asus_kbd_leds *led)
>>>       spin_unlock_irqrestore(&led->lock, flags);
>>>  }
>>>
>>> -static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>>> -                                enum led_brightness brightness)
>>> +static void asus_kbd_backlight_set(struct asus_hid_listener *listener,
>>> +                                int brightness)
>>>  {
>>> -     struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
>>> -                                              cdev);
>>> +     struct asus_kbd_leds *led = container_of(listener, struct asus_kbd_leds,
>>> +                                              listener);
>>>       unsigned long flags;
>>>
>>>       spin_lock_irqsave(&led->lock, flags);
>>> @@ -509,20 +509,6 @@ static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>>>       asus_schedule_work(led);
>>>  }
>>>
>>> -static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
>>> -{
>>> -     struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
>>> -                                              cdev);
>>> -     enum led_brightness brightness;
>>> -     unsigned long flags;
>>> -
>>> -     spin_lock_irqsave(&led->lock, flags);
>>> -     brightness = led->brightness;
>>> -     spin_unlock_irqrestore(&led->lock, flags);
>>> -
>>> -     return brightness;
>>> -}
>>> -
>>>  static void asus_kbd_backlight_work(struct work_struct *work)
>>>  {
>>>       struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
>>> @@ -539,34 +525,6 @@ static void asus_kbd_backlight_work(struct work_struct *work)
>>>               hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>>  }
>>>
>>> -/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes
>>> - * precedence. We only activate HID-based backlight control when the
>>> - * WMI control is not available.
>>> - */
>>> -static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
>>> -{
>>> -     struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>> -     u32 value;
>>> -     int ret;
>>> -
>>> -     if (!IS_ENABLED(CONFIG_ASUS_WMI))
>>> -             return false;
>>> -
>>> -     if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
>>> -                     dmi_check_system(asus_use_hid_led_dmi_ids)) {
>>> -             hid_info(hdev, "using HID for asus::kbd_backlight\n");
>>> -             return false;
>>> -     }
>>> -
>>> -     ret = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS,
>>> -                                    ASUS_WMI_DEVID_KBD_BACKLIGHT, 0, &value);
>>> -     hid_dbg(hdev, "WMI backlight check: rc %d value %x", ret, value);
>>> -     if (ret)
>>> -             return false;
>>> -
>>> -     return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT);
>>> -}
>>> -
>>>  /*
>>>   * We don't care about any other part of the string except the version section.
>>>   * Example strings: FGA80100.RC72LA.312_T01, FGA80100.RC71LS.318_T01
>>> @@ -701,14 +659,11 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>       drvdata->kbd_backlight->removed = false;
>>>       drvdata->kbd_backlight->brightness = 0;
>>>       drvdata->kbd_backlight->hdev = hdev;
>>> -     drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
>>> -     drvdata->kbd_backlight->cdev.max_brightness = 3;
>>> -     drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
>>> -     drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
>>> +     drvdata->kbd_backlight->listener.brightness_set = asus_kbd_backlight_set;
>>>       INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
>>>       spin_lock_init(&drvdata->kbd_backlight->lock);
>>>
>>> -     ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
>>> +     ret = asus_hid_register_listener(&drvdata->kbd_backlight->listener);
>>>       if (ret < 0) {
>>>               /* No need to have this still around */
>>>               devm_kfree(&hdev->dev, drvdata->kbd_backlight);
>>> @@ -1105,7 +1060,7 @@ static int __maybe_unused asus_resume(struct hid_device *hdev) {
>>>
>>>       if (drvdata->kbd_backlight) {
>>>               const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4,
>>> -                             drvdata->kbd_backlight->cdev.brightness };
>>> +                             drvdata->kbd_backlight->brightness };
>>>               ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
>>>               if (ret < 0) {
>>>                       hid_err(hdev, "Asus failed to set keyboard backlight: %d\n", ret);
>>> @@ -1241,7 +1196,6 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>>       }
>>>
>>>       if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>>> -         !asus_kbd_wmi_led_control_present(hdev) &&
>>>           asus_kbd_register_leds(hdev))
>>>               hid_warn(hdev, "Failed to initialize backlight.\n");
>>>
>>> @@ -1282,6 +1236,8 @@ static void asus_remove(struct hid_device *hdev)
>>>       unsigned long flags;
>>>
>>>       if (drvdata->kbd_backlight) {
>>> +             asus_hid_unregister_listener(&drvdata->kbd_backlight->listener);
>>> +
>>>               spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
>>>               drvdata->kbd_backlight->removed = true;
>>>               spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);

Powered by blists - more mailing lists