* Re: [PATCH v9 11/11] HID: asus: add support for the asus-wmi brightness handler
From: Denis Benato @ 2025-11-20 14:22 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
In-Reply-To: <20251120094617.11672-12-lkml@antheas.dev>
On 11/20/25 10:46, Antheas Kapenekakis wrote:
> If the asus-wmi brightness handler is available, send the
> keyboard brightness events to it instead of passing them
> to userspace. If it is not, fall back to sending them to it.
>
> Reviewed-by: Luke D. Jones <luke@ljones.dev>
> Tested-by: Luke D. Jones <luke@ljones.dev>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/hid/hid-asus.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6a355c174f29..ff5aaebc38e3 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -324,6 +324,17 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
> usage->hid & HID_USAGE);
> }
>
> + if (usage->type == EV_KEY && value) {
> + switch (usage->code) {
> + case KEY_KBDILLUMUP:
> + return !asus_hid_event(ASUS_EV_BRTUP);
> + case KEY_KBDILLUMDOWN:
> + return !asus_hid_event(ASUS_EV_BRTDOWN);
> + case KEY_KBDILLUMTOGGLE:
> + return !asus_hid_event(ASUS_EV_BRTTOGGLE);
> + }
> + }
> +
> return 0;
> }
>
Reviewed-by: Denis Benato <benato.denis96@gmail.com>
^ permalink raw reply
* Re: [PATCH v9 06/11] HID: asus: early return for ROG devices
From: Antheas Kapenekakis @ 2025-11-20 14:15 UTC (permalink / raw)
To: Denis Benato
Cc: platform-driver-x86, linux-input, linux-kernel, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede,
Ilpo Järvinen
In-Reply-To: <f13a53d9-e5b8-4380-b134-16aa81eb0a88@gmail.com>
On Thu, 20 Nov 2025 at 14:29, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 11/20/25 10:46, Antheas Kapenekakis wrote:
> > Some ROG devices have a new dynamic backlight interface for control by
> > Windows. This interface does not create an ->input device, causing the
> > kernel to print an error message and to eject it. In addition, ROG
> > devices have proper HID names in their descriptors so renaming them is
> > not necessary.
> Is this patchset supposed to work without the renaming, correct?
>
> If so consider dropping the drop of renames, taking required time
> to organize with Derek and resubmit when things are ready:
> there is no point for the rename to stall the rest and quit renaming
> is not urgent at all.
I feel like two months is enough of a timeframe for a simple rename
fix to go in.
I do not want to have to reorder the checks just so the rename can
stay in _for now_. Skipping the ->input check is important for both
Xbox Ally/Z13 as it causes errors and the device to stay partially
uninitialized.
> > Therefore, if a device is identified as ROG, early return from probe to
> > skip renaming and ->input checks.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > ---
> > drivers/hid/hid-asus.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index 3047bc54bf2e..6193c9483bec 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -1236,6 +1236,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > asus_kbd_register_leds(hdev))
> > hid_warn(hdev, "Failed to initialize backlight.\n");
> >
> > + /*
> > + * For ROG keyboards, skip rename for consistency and ->input check as
> > + * some devices do not have inputs.
> > + */
> > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
> > + return 0;
> > +
> > /*
> > * Check that input registration succeeded. Checking that
> > * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
> Just for clarity is this supposed to fix this: https://gitlab.com/asus-linux/asusctl/-/issues/700 ?
> This model works once in windows users disable that new feature.
>
> Note: that kernel the person submitting the bug is using contains your v8
> and asus-armoury.
>
No. This user has a laptop that has at least a WMI implementation of
RGB controls (this is why you can see rgb settings). Since you did not
ask for logs, it is not clear if it also has a HID implementation that
is skipped due to e.g., a missing product ID. Very likely it is a bug
on the WMI implementation that is out of scope for this series.
^ permalink raw reply
* Re: [PATCH v9 04/11] HID: asus: fortify keyboard handshake
From: Denis Benato @ 2025-11-20 14:15 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
In-Reply-To: <20251120094617.11672-5-lkml@antheas.dev>
On 11/20/25 10:46, Antheas Kapenekakis wrote:
> Handshaking with an Asus device involves sending it a feature report
> with the string "ASUS Tech.Inc." and then reading it back to verify the
> handshake was successful, under the feature ID the interaction will
> take place.
>
> Currently, the driver only does the first part. Add the readback to
> verify the handshake was successful. As this could cause breakages,
> allow the verification to fail with a dmesg error until we verify
> all devices work with it (they seem to).
>
> Since the response is more than 16 bytes, increase the buffer size
> to 64 as well to avoid overflow errors.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/hid/hid-asus.c | 32 +++++++++++++++++++++++++++++---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 6de402d215d0..5149dc7edfc5 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define FEATURE_REPORT_ID 0x0d
> #define INPUT_REPORT_ID 0x5d
> #define FEATURE_KBD_REPORT_ID 0x5a
> -#define FEATURE_KBD_REPORT_SIZE 16
> +#define FEATURE_KBD_REPORT_SIZE 64
> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> @@ -394,14 +394,40 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
>
> static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
> {
> + /*
> + * The handshake is first sent as a set_report, then retrieved
> + * from a get_report. They should be equal.
> + */
> const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
> 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> + u8 *readbuf;
> int ret;
>
> ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> - if (ret < 0)
> - hid_err(hdev, "Asus failed to send init command: %d\n", ret);
> + if (ret < 0) {
> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret);
> + return ret;
> + }
> +
> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
I see my suggestion to use __free here didn't materialize in code using
it even after Ilpo kindly wrote how to correctly use it.
I think you can move the readbuf assignment right below buf and
take into account what Ilpo said.
I don't expect new variables will be added here ever again,
but I agree with Ilpo that it's a good idea here to write code
accounting for that possibility.
It is my understanding that who proposes patches is expected to
resolve discussions when changes are proposed or to take into
account requested changes and submit a modified version.
> + if (!readbuf)
> + return -ENOMEM;
> +
> + ret = hid_hw_raw_request(hdev, report_id, readbuf,
> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> + if (ret < 0) {
> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret);
> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) {
> + hid_warn(hdev, "Asus handshake returned invalid response: %*ph\n",
> + FEATURE_KBD_REPORT_SIZE, readbuf);
> + /*
> + * Do not return error if handshake is wrong until this is
> + * verified to work for all devices.
> + */
> + }
>
> + kfree(readbuf);
> return ret;
> }
>
^ permalink raw reply
* Re: [PATCH v5 00/11] mfd: macsmc: add rtc, hwmon and hid subdevices
From: Lee Jones @ 2025-11-20 13:47 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Janne Grunau, Alyssa Rosenzweig, Neal Gompa,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, Jonathan Corbet,
asahi, linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, linux-doc, Mark Kettenis, Hector Martin
In-Reply-To: <20251112-macsmc-subdevs-v5-0-728e4b91fe81@gmail.com>
On Wed, 12 Nov 2025, James Calligeros wrote:
> Hi all,
>
> This series adds support for the remaining SMC subdevices. These are the
> RTC, hwmon, and HID devices. They are being submitted together as the RTC
> and hwmon drivers both require changes to the SMC DT schema.
>
> The RTC driver is responsible for getting and setting the system clock,
> and requires an NVMEM cell. This series replaces Sven's original RTC driver
> submission [1].
>
> The hwmon function is an interesting one. While each Apple Silicon device
> exposes pretty similar sets of sensors, these all seem to be paired to
> different SMC keys in the firmware interface. This is true even when the
> sensors are on the SoC. For example, an M1 MacBook Pro will use different
> keys to access the LITTLE core temperature sensors to an M1 Mac mini. This
> necessitates describing which keys correspond to which sensors for each
> device individually, and populating the hwmon structs at runtime. We do
> this with a node in the device tree. This series includes only the keys
> for sensors which we know to be common to all devices. The SMC is also
> responsible for monitoring and controlling fan speeds on systems with fans,
> which we expose via the hwmon driver.
>
> The SMC also handles the hardware power button and lid switch. Power
> button presses and lid opening/closing are emitted as HID events, so we
> add an input subdevice to handle them.
>
> Since there are no real dependencies between the components of this series,
> it should be fine for each subsystem to take the relevant patches through
> their trees. The mfd one-liners should be taken in order to avoid trivial
> conflicts. Per [2], the hwmon driver should be merged along with the preceding
> mfd patch adding the __SMC_KEY macro to avoid build errors.
Apart from my (perhaps naive) question on patch 5, the other MFD patches
look okay to me. Once my question has been answered, I can apply the
MFD, or at least 3 of them, orthogonally.
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v9 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Denis Benato @ 2025-11-20 13:46 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
In-Reply-To: <20251120094617.11672-8-lkml@antheas.dev>
On 11/20/25 10:46, Antheas Kapenekakis wrote:
> Some devices, such as the Z13 have multiple Aura devices connected
> to them by USB. In addition, they might have a WMI interface for
> RGB. In Windows, Armoury Crate exposes a unified brightness slider
> for all of them, with 3 brightness levels.
>
> Therefore, to be synergistic in Linux, and support existing tooling
> such as UPower, allow adding listeners to the RGB device of the WMI
> interface. If WMI does not exist, lazy initialize the interface.
>
> Since hid-asus and asus-wmi can both interact with the led objects
> including from an atomic context, protect the brightness access with a
> spinlock and update the values from a workqueue. Use this workqueue to
> also process WMI keyboard events, so they are handled asynchronously.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/platform/x86/asus-wmi.c | 174 ++++++++++++++++++---
> include/linux/platform_data/x86/asus-wmi.h | 17 ++
> 2 files changed, 167 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index e72a2b5d158e..5f23aedbf34f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -36,6 +36,7 @@
> #include <linux/rfkill.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> +#include <linux/spinlock.h>
> #include <linux/types.h>
> #include <linux/units.h>
>
> @@ -258,6 +259,9 @@ struct asus_wmi {
> int tpd_led_wk;
> struct led_classdev kbd_led;
> int kbd_led_wk;
> + bool kbd_led_notify;
> + bool kbd_led_avail;
> + bool kbd_led_registered;
> struct led_classdev lightbar_led;
> int lightbar_led_wk;
> struct led_classdev micmute_led;
> @@ -266,6 +270,7 @@ struct asus_wmi {
> struct work_struct tpd_led_work;
> struct work_struct wlan_led_work;
> struct work_struct lightbar_led_work;
> + struct work_struct kbd_led_work;
>
> struct asus_rfkill wlan;
> struct asus_rfkill bluetooth;
> @@ -1530,6 +1535,99 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
>
> /* LEDs ***********************************************************************/
>
> +struct asus_hid_ref {
> + struct list_head listeners;
> + struct asus_wmi *asus;
> + /* Protects concurrent access from hid-asus and asus-wmi to leds */
> + spinlock_t lock;
> +};
> +
> +static struct asus_hid_ref asus_ref = {
> + .listeners = LIST_HEAD_INIT(asus_ref.listeners),
> + .asus = NULL,
> + /*
> + * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
> + * asus variables are read-only after .asus is set. Except the led cdev
> + * device if not kbd_led_avail. That becomes read-only after the
> + * first hid-asus listener registers and triggers the work queue. It is
> + * then not referenced again until unregistering, which happens after
> + * .asus ref is dropped. Since .asus needs to be accessed by hid-asus
> + * IRQs to check if forwarding events is possible, a spinlock is used.
> + */
What are "That" and "It" referring to in this context?
Are you absolutely sure you want to begin a sentence with "Except"?
On "ref is dropped" I would continue with ": since .asus .....".
> + .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
> +};
> +
> +/*
> + * Allows registering hid-asus listeners that want to be notified of
> + * keyboard backlight changes.
> + */
> +int asus_hid_register_listener(struct asus_hid_listener *bdev)
> +{
> + struct asus_wmi *asus;
> +
> + guard(spinlock_irqsave)(&asus_ref.lock);
> + list_add_tail(&bdev->list, &asus_ref.listeners);
> + asus = asus_ref.asus;
> + if (asus)
> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
Are you sure this has to be protected by the guard too?
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_hid_register_listener);
> +
> +/*
> + * Allows unregistering hid-asus listeners that were added with
> + * asus_hid_register_listener().
> + */
> +void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> +{
> + guard(spinlock_irqsave)(&asus_ref.lock);
> + list_del(&bdev->list);
> +}
> +EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
> +
> +static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
> +
> +static void kbd_led_update_all(struct work_struct *work)
> +{
> + struct asus_wmi *asus;
> + bool registered, notify;
> + int ret, value;
> +
> + asus = container_of(work, struct asus_wmi, kbd_led_work);
> +
> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> + registered = asus->kbd_led_registered;
> + value = asus->kbd_led_wk;
> + notify = asus->kbd_led_notify;
> + }
> +
> + if (!registered) {
> + /*
> + * This workqueue runs under asus-wmi, which means probe has
> + * completed and asus-wmi will keep running until it finishes.
> + * Therefore, we can safely register the LED without holding
> + * a spinlock.
> + */
> + ret = devm_led_classdev_register(&asus->platform_device->dev,
> + &asus->kbd_led);
> + if (!ret) {
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + asus->kbd_led_registered = true;
> + } else {
> + pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
> + return;
> + }
> + }
> +
> + if (value >= 0)
> + do_kbd_led_set(&asus->kbd_led, value);
> + if (notify) {
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + asus->kbd_led_notify = false;
> + led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
> + }
> +}
> +
> /*
> * These functions actually update the LED's, and are called from a
> * workqueue. By doing this as separate work rather than when the LED
> @@ -1576,7 +1674,8 @@ static void kbd_led_update(struct asus_wmi *asus)
> {
> int ctrl_param = 0;
>
> - ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> }
>
> @@ -1609,14 +1708,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
>
> static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> {
> + struct asus_hid_listener *listener;
> struct asus_wmi *asus;
> int max_level;
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> max_level = asus->kbd_led.max_brightness;
>
> - asus->kbd_led_wk = clamp_val(value, 0, max_level);
> - kbd_led_update(asus);
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + asus->kbd_led_wk = clamp_val(value, 0, max_level);
> +
> + if (asus->kbd_led_avail)
> + kbd_led_update(asus);
> +
> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> + list_for_each_entry(listener, &asus_ref.listeners, list)
> + listener->brightness_set(listener, asus->kbd_led_wk);
> + }
> }
>
> static void kbd_led_set(struct led_classdev *led_cdev,
> @@ -1631,10 +1739,11 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
> static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> {
> - struct led_classdev *led_cdev = &asus->kbd_led;
> -
> - do_kbd_led_set(led_cdev, value);
> - led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> + asus->kbd_led_wk = value;
> + asus->kbd_led_notify = true;
> + }
> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> }
>
> static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> @@ -1644,10 +1753,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> + if (!asus->kbd_led_avail)
> + return asus->kbd_led_wk;
> + }
> +
> retval = kbd_led_read(asus, &value, NULL);
> if (retval < 0)
> return retval;
>
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + asus->kbd_led_wk = value;
> +
> return value;
> }
>
> @@ -1759,7 +1876,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
>
> static void asus_wmi_led_exit(struct asus_wmi *asus)
> {
> - led_classdev_unregister(&asus->kbd_led);
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + asus_ref.asus = NULL;
> +
> led_classdev_unregister(&asus->tpd_led);
> led_classdev_unregister(&asus->wlan_led);
> led_classdev_unregister(&asus->lightbar_led);
> @@ -1797,22 +1916,25 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> goto error;
> }
>
> - if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
> - pr_info("using asus-wmi for asus::kbd_backlight\n");
> - asus->kbd_led_wk = led_val;
> - asus->kbd_led.name = "asus::kbd_backlight";
> - asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> - asus->kbd_led.brightness_set = kbd_led_set;
> - asus->kbd_led.brightness_get = kbd_led_get;
> - asus->kbd_led.max_brightness = 3;
> + asus->kbd_led.name = "asus::kbd_backlight";
> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> + asus->kbd_led.brightness_set = kbd_led_set;
> + asus->kbd_led.brightness_get = kbd_led_get;
> + asus->kbd_led.max_brightness = 3;
> + asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
> + INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
>
> + if (asus->kbd_led_avail) {
> + asus->kbd_led_wk = led_val;
> if (num_rgb_groups != 0)
> asus->kbd_led.groups = kbd_rgb_mode_groups;
> + } else
> + asus->kbd_led_wk = -1;
>
> - rv = led_classdev_register(&asus->platform_device->dev,
> - &asus->kbd_led);
> - if (rv)
> - goto error;
> + scoped_guard(spinlock_irqsave, &asus_ref.lock) {
> + asus_ref.asus = asus;
> + if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
> + queue_work(asus->led_workqueue, &asus->kbd_led_work);
> }
>
> if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
> @@ -4272,6 +4394,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
>
> static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> {
> + enum led_brightness led_value;
> unsigned int key_value = 1;
> bool autorelease = 1;
>
> @@ -4288,19 +4411,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> return;
> }
>
> + scoped_guard(spinlock_irqsave, &asus_ref.lock)
> + led_value = asus->kbd_led_wk;
> +
> if (code == NOTIFY_KBD_BRTUP) {
> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> + kbd_led_set_by_kbd(asus, led_value + 1);
> return;
> }
> if (code == NOTIFY_KBD_BRTDWN) {
> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
> + kbd_led_set_by_kbd(asus, led_value - 1);
> return;
> }
> if (code == NOTIFY_KBD_BRTTOGGLE) {
> - if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
> + if (led_value == asus->kbd_led.max_brightness)
> kbd_led_set_by_kbd(asus, 0);
> else
> - kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> + kbd_led_set_by_kbd(asus, led_value + 1);
> return;
> }
>
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 8a515179113d..1165039013b1 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -163,11 +163,20 @@ enum asus_ally_mcu_hack {
> ASUS_WMI_ALLY_MCU_HACK_DISABLED,
> };
>
> +/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
> +struct asus_hid_listener {
> + struct list_head list;
> + void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
> +};
> +
> #if IS_REACHABLE(CONFIG_ASUS_WMI)
> void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
> void set_ally_mcu_powersave(bool enabled);
> int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
> int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
> +
> +int asus_hid_register_listener(struct asus_hid_listener *cdev);
> +void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
> #else
> static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
> {
> @@ -184,6 +193,14 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
> {
> return -ENODEV;
> }
> +
> +static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
> +{
> + return -ENODEV;
> +}
> +static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
> +{
> +}
> #endif
>
> /* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
^ permalink raw reply
* Re: [PATCH v5 05/11] mfd: macsmc: Add new __SMC_KEY macro
From: Lee Jones @ 2025-11-20 13:44 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Janne Grunau, Alyssa Rosenzweig, Neal Gompa,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Alexandre Belloni,
Jean Delvare, Guenter Roeck, Dmitry Torokhov, Jonathan Corbet,
asahi, linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, linux-doc
In-Reply-To: <20251112-macsmc-subdevs-v5-5-728e4b91fe81@gmail.com>
On Wed, 12 Nov 2025, James Calligeros wrote:
> When using the _SMC_KEY macro in switch/case statements, GCC 15.2.1 errors
> out with 'case label does not reduce to an integer constant'. Introduce
> a new __SMC_KEY macro that can be used instead.
>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> include/linux/mfd/macsmc.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
> index 6b13f01a8592..f6f80c33b5cf 100644
> --- a/include/linux/mfd/macsmc.h
> +++ b/include/linux/mfd/macsmc.h
> @@ -41,6 +41,7 @@ typedef u32 smc_key;
> */
> #define SMC_KEY(s) (smc_key)(_SMC_KEY(#s))
> #define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
> +#define __SMC_KEY(a, b, c, d) (((u32)(a) << 24) | ((u32)(b) << 16) | ((u32)(c) << 8) | ((u32)(d)))
Are we expecting users/consumers to be able to tell the difference
between SMC_KEY and __SMC_KEY (assuming that _SMC_KEY is just an
internal)?
I have not tested this and it is just off the top of my head, but does
this work:
#define _SMC_KEY(s) __SMC_KEY((s)[0], (s)[1], (s)[2], (s)[3])
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v9 06/11] HID: asus: early return for ROG devices
From: Denis Benato @ 2025-11-20 13:29 UTC (permalink / raw)
To: Antheas Kapenekakis, platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen
In-Reply-To: <20251120094617.11672-7-lkml@antheas.dev>
On 11/20/25 10:46, Antheas Kapenekakis wrote:
> Some ROG devices have a new dynamic backlight interface for control by
> Windows. This interface does not create an ->input device, causing the
> kernel to print an error message and to eject it. In addition, ROG
> devices have proper HID names in their descriptors so renaming them is
> not necessary.
Is this patchset supposed to work without the renaming, correct?
If so consider dropping the drop of renames, taking required time
to organize with Derek and resubmit when things are ready:
there is no point for the rename to stall the rest and quit renaming
is not urgent at all.
> Therefore, if a device is identified as ROG, early return from probe to
> skip renaming and ->input checks.
>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
> drivers/hid/hid-asus.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 3047bc54bf2e..6193c9483bec 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1236,6 +1236,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> asus_kbd_register_leds(hdev))
> hid_warn(hdev, "Failed to initialize backlight.\n");
>
> + /*
> + * For ROG keyboards, skip rename for consistency and ->input check as
> + * some devices do not have inputs.
> + */
> + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
> + return 0;
> +
> /*
> * Check that input registration succeeded. Checking that
> * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
Just for clarity is this supposed to fix this: https://gitlab.com/asus-linux/asusctl/-/issues/700 ?
This model works once in windows users disable that new feature.
Note: that kernel the person submitting the bug is using contains your v8
and asus-armoury.
^ permalink raw reply
* Re: [PATCH v4 0/1] HID: Add support for multiple batteries per device
From: Benjamin Tissoires @ 2025-11-20 12:56 UTC (permalink / raw)
To: Lucas Zampieri
Cc: linux-input, linux-kernel, Jiri Kosina, Sebastian Reichel,
Bastien Nocera, linux-pm
In-Reply-To: <CAOOg__BCM_s+Mm8e6NcwwVhQLyZ3z2iMwX1FOwC_7drNd8gEjg@mail.gmail.com>
On Nov 20 2025, Lucas Zampieri wrote:
> On Thu, Nov 20, 2025 at 11:05 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > On Nov 20 2025, Lucas Zampieri wrote:
> > > This series adds support for HID devices with multiple batteries.
> > >
> > > Currently, the HID battery reporting subsystem only supports one battery per
> > > device. There are several devices with multiple batteries that would benefit
> > > from this support:
> > > - Gaming headsets with batteries in both the headset and charging dock
> > > - Wireless earbuds with per-earbud batteries plus charging case
> > > - Split keyboards with per-side batteries
> > >
> > > ## Proposed Solution
> > >
> > > This series introduces struct hid_battery to encapsulate individual battery
> > > state, replaces the old battery fields with a list-based approach, and adds
> > > support for multiple batteries tracked within struct hid_device. Batteries
> > > are identified by report ID. The implementation is fully backwards compatible
> > > with single-battery devices through a helper function.
> > >
> > > ## Testing
> > >
> > > Tested with split keyboard hardware (Dactyl 5x6) using custom ZMK firmware
> > > that implements per-side HID battery reporting. Each battery (left and right
> > > keyboard halves) reports independently through the power supply interface with
> > > distinct report IDs (0x05 and 0x06).
> > >
> > > Test firmware available on my personal fork at:
> > > https://github.com/zampierilucas/zmk/tree/feat/individual-hid-battery-reporting
> > > If this series gets merged, these changes will be proposed to upstream ZMK.
> > >
> > > HID descriptor and recording captured with hid-recorder:
> > >
> > > D: 0
> > > R: 162 05 01 09 06 a1 01 85 01 05 07 19 e0 29 e7 15 00 25 01 75 01 95 08 81 02 05 07 75 08 95 01 81 03 05 07 15 00 25 01 19 00 29 67 75 01 95 68 81 02 c0 05 0c 09 01 a1 01 85 02 05 0c 15 00 26 ff 0f 19 00 2a ff 0f 75 10 95 06 81 00 c0 05 84 09 05 a1 01 05 85 85 05 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0 05 84 09 05 a1 01 05 85 85 06 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0
> > > N: ZMK Project Dactyl 5x6
> > > P: usb-0000:2d:00.3-4.2/input2
> > > I: 3 1d50 615e
> > > D: 0
> > > E: 0.000000 3 05 00 56
> > > E: 0.000977 3 05 00 56
> > > E: 1.490974 3 06 00 52
> > > E: 1.491958 3 06 00 52
> > > E: 6.492979 3 06 00 53
> > > E: 6.493962 3 06 00 53
> > >
> > > The recording shows both batteries reporting with different charge levels
> > > (Report ID 05: 86%, Report ID 06: 82%-83%), demonstrating the multi-battery
> > > functionality. This can be used to verify UPower compatibility.
> > >
> > > ## Future Work: Userspace Integration
> > >
> > > As suggested by Bastien, semantic battery differentiation (e.g., "left
> > > earbud" vs "right earbud") requires userspace coordination, as HID
> > > reports typically lack role metadata.
> > >
> > > This will require:
> > > 1. systemd/hwdb entries for device-specific battery role mappings
> > > 2. UPower updates to enumerate and group multi-battery devices
> > > 3. Desktop environment changes to display batteries with meaningful labels
> > >
> > > This kernel infrastructure is a prerequisite for that userspace work.
> > >
> > > Lucas Zampieri (1):
> > > HID: input: Add support for multiple batteries per device
> > >
> > > Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> > >
> > > Changes in v4:
> > > - Added missing hidinput_update_battery() stub in #else block for
> > > CONFIG_HID_BATTERY_STRENGTH=n builds
> > > - Reported-by: kernel test robot <lkp@intel.com>
> > > - Closes: https://lore.kernel.org/oe-kbuild-all/202511201624.yUv4VtBv-lkp@intel.com/
> >
> > I don't think you need to keep the formal tags here in changlogs of the
> > cover letter. A simple "as reported by kernel test robot
> > <lkp@intel.com>" should be sufficient (given that usually we end up
> > dropping the cover letter as we apply the patches only).
> >
> > >
> > > Changes in v3:
> > > - Squashed the three v2 patches into a single patch as suggested by
> > > Benjamin
> >
> > Sorry we didn't understood each other: I was asking you to squash
> > patches 1 and 2 only, and keep 3 separated. I was just complaining about
> > a blank header change. Separating the non functional changes from the
> > functional ones is important.
> >
> I get it, but the reason I made it one patch instead of two was to
> have one functional patch hid-multi-battery patch, instead of one just
> adding the structure, and then another one to really make use of the
> multi-battery stuff, but I guess I can make it two again, even tho the
> first one will have no real change in behavior
The first one is not just adding the structure, it's also a clean
refactor of the existing code where the battery information is nicely
regrouped into a single struct. And if that patch is all by itself, we
can again bisect it because we know it's not supposed to break anything
because there is no functional changes. So yes, no changes in bahavior
seems pointless but it is very important for your next patch so it's
reviewable.
>
> > > - Removed all legacy dev->battery_* fields, using list-based storage only
> > > - Changed battery naming to include report ID: hid-{uniq}-battery-{report_id}
> > > - Converted battery memory management to devm_* for automatic cleanup
> >
> > Haven't checked the code yet but that would require probably a separate
> > patch so we can bisect to it if anything breaks.
> >
> I guess I kinda coded myself into a corner here, as with the new
> devm_* changes, I wrote the v4/v5 hid-multi-battery feature to be made
> for it, so I would have to go back to the v2 implementation (which is
> fine, its just that the new devm_* makes it much cleaner), and then
> have another patch converting the whole to use devm_*, which might as
> well be a separate series at that points so it's easier to
> review/test/bisect. Likewise, I'm fine with either approach, your
> call.
Up to you. I didn't mean necessarily to make the devm changes right now.
In an ideal world we would have:
- first patch which convert the existing code into devm
- second patch which introduces struct hid_battery
- third patch with the multi-battery feature.
But the order doesn't really matter (except 2 needs to come before 3).
The devm patch can be inserted anywhere. Having it first will simplify a
bit your 2 other patches, but having it last is also fine because we can
see the cleanup easily as well.
So if you want to postpone the devm patch, that's fine by me. But the
more you postpone it the higher the chances are that you forgot one kfree
or introduced a use-after-free, which means a lot more complications for
everybody :)
Cheers,
Benjamin
>
> > > - Updated hidinput_update_battery() to take struct hid_battery directly
> > > - Added hid_get_first_battery() helper for external driver compatibility
> > > - Updated hid-apple.c and hid-magicmouse.c to use new battery API
> > > - Simplified cover letter based on feedback
> >
> > Heh, thanks :)
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Changes in v2:
> > > - Split the monolithic v1 patch into three logical patches for easier review:
> > > 1. Introduce struct hid_battery (pure structure addition)
> > > 2. Refactor existing code to use the new structure (internal changes)
> > > 3. Add multi-battery support (new functionality)
> > > - Added detailed testing section with hardware specifics
> > > - Added hid-recorder output (dactyl-hid-recording.txt) demonstrating two-battery
> > > HID descriptor for UPower validation
> > > - Added "Future Work: Userspace Integration" section addressing Bastien's feedback
> > > about semantic battery differentiation
> > > - Added hardware examples with product links to commit messages (per Bastien's
> > > suggestion)
> > > - No functional changes from v1, only improved patch organization and documentation
> > >
> > > drivers/hid/hid-apple.c | 10 +-
> > > drivers/hid/hid-core.c | 4 +
> > > drivers/hid/hid-input-test.c | 39 ++++---
> > > drivers/hid/hid-input.c | 196 +++++++++++++++++++----------------
> > > drivers/hid/hid-magicmouse.c | 10 +-
> > > include/linux/hid.h | 54 +++++++---
> > > 6 files changed, 187 insertions(+), 126 deletions(-)
> > >
> > >
> > > base-commit: 0678f5630429a5049d7663703b897e1bf8d13cd1
> > > --
> > > 2.51.0
> > >
> >
>
^ permalink raw reply
* ili2130: touchscreen-size-x/y DT properties are ignored, firmware reports 16384x16384 range
From: Fabio Estevam @ 2025-11-20 12:40 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
joe_hung
Hi,
I am working on an i.MX8MP-based system equipped with an Ilitek ILI2130
touch controller. The device tree contains:
touchscreen@41 {
compatible = "ilitek,ili2130";
reg = <0x41>;
interrupt-parent = <&gpio5>;
interrupts = <13 IRQ_TYPE_EDGE_RISING>;
reset-gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
touchscreen-size-x = <800>;
touchscreen-size-y = <480>;
wakeup-source;
};
The device probes correctly:
$ cat /sys/bus/i2c/devices/2-0041/product_id
product id: [2130], module: [ILI2130000000000]
$ cat /sys/bus/i2c/devices/2-0041/firmware_version
fw version: [0700.0000.0000.0000]
Using evtest, I see valid multitouch events. However, the reported
coordinate range is always 0–16383 for both X and Y, regardless of the
touchscreen-size-x/y values provided in the DT.
The ili2130 driver exposes screen_min_x/y and screen_max_x/y but these
are all initialized from firmware data. The DT properties appear to be
ignored for this model.
To make the touchscreen work correctly with Weston on an 800x480 panel,
I currently need the following hack:
x = (raw_x * 25) >> 9; /* 16384 * 25/512 = 800 */
y = (raw_y * 15) >> 9; /* 16384 * 15/512 = 480 */
--- a/drivers/input/touchscreen/ilitek_ts_i2c.c
+++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
@@ -194,8 +194,8 @@ static int ilitek_process_and_report_v6(struct
ilitek_ts_data *ts)
id = buf[i * packet_len + 1] & 0x3F;
- x = get_unaligned_le16(buf + i * packet_len + 2);
- y = get_unaligned_le16(buf + i * packet_len + 4);
+ x = (get_unaligned_le16(buf + i * packet_len + 2) * 25) >> 9;
+ y = (get_unaligned_le16(buf + i * packet_len + 4) * 15) >> 9;
if (x > ts->screen_max_x || x < ts->screen_min_x ||
y > ts->screen_max_y || y < ts->screen_min_y) {
Without this, the compositor receives coordinates in the 0–16383 range.
My questions:
1. Should the ili2130 driver apply scaling based on
touchscreen-size-x/y when firmware reports a fixed 14-bit coordinate
range?
2. If yes, where should this scaling occur so that existing users of
the ili2xxx family are not broken?
- in ilitek_process_and_report_v6?
- via input_set_abs_params() using adjusted min/max values?
3. Is the ILI2130 expected to report already-scaled coordinates
normally, or is the 0–16383 range the intended firmware behavior?
I am happy to implement/test any proposed changes. Please let me know what the
correct approach would be for supporting panels whose resolution differs
from the firmware's coordinate space.
Thanks,
Fabio Estevam
^ permalink raw reply
* Re: [PATCH v4 0/1] HID: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-20 12:03 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, linux-kernel, Jiri Kosina, Sebastian Reichel,
Bastien Nocera, linux-pm
In-Reply-To: <pr3w6zp5hoza27fncktfj3qx3wp2nhdglbx46fbfx6swm4772d@7hg7a6dxom7p>
On Thu, Nov 20, 2025 at 11:05 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On Nov 20 2025, Lucas Zampieri wrote:
> > This series adds support for HID devices with multiple batteries.
> >
> > Currently, the HID battery reporting subsystem only supports one battery per
> > device. There are several devices with multiple batteries that would benefit
> > from this support:
> > - Gaming headsets with batteries in both the headset and charging dock
> > - Wireless earbuds with per-earbud batteries plus charging case
> > - Split keyboards with per-side batteries
> >
> > ## Proposed Solution
> >
> > This series introduces struct hid_battery to encapsulate individual battery
> > state, replaces the old battery fields with a list-based approach, and adds
> > support for multiple batteries tracked within struct hid_device. Batteries
> > are identified by report ID. The implementation is fully backwards compatible
> > with single-battery devices through a helper function.
> >
> > ## Testing
> >
> > Tested with split keyboard hardware (Dactyl 5x6) using custom ZMK firmware
> > that implements per-side HID battery reporting. Each battery (left and right
> > keyboard halves) reports independently through the power supply interface with
> > distinct report IDs (0x05 and 0x06).
> >
> > Test firmware available on my personal fork at:
> > https://github.com/zampierilucas/zmk/tree/feat/individual-hid-battery-reporting
> > If this series gets merged, these changes will be proposed to upstream ZMK.
> >
> > HID descriptor and recording captured with hid-recorder:
> >
> > D: 0
> > R: 162 05 01 09 06 a1 01 85 01 05 07 19 e0 29 e7 15 00 25 01 75 01 95 08 81 02 05 07 75 08 95 01 81 03 05 07 15 00 25 01 19 00 29 67 75 01 95 68 81 02 c0 05 0c 09 01 a1 01 85 02 05 0c 15 00 26 ff 0f 19 00 2a ff 0f 75 10 95 06 81 00 c0 05 84 09 05 a1 01 05 85 85 05 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0 05 84 09 05 a1 01 05 85 85 06 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0
> > N: ZMK Project Dactyl 5x6
> > P: usb-0000:2d:00.3-4.2/input2
> > I: 3 1d50 615e
> > D: 0
> > E: 0.000000 3 05 00 56
> > E: 0.000977 3 05 00 56
> > E: 1.490974 3 06 00 52
> > E: 1.491958 3 06 00 52
> > E: 6.492979 3 06 00 53
> > E: 6.493962 3 06 00 53
> >
> > The recording shows both batteries reporting with different charge levels
> > (Report ID 05: 86%, Report ID 06: 82%-83%), demonstrating the multi-battery
> > functionality. This can be used to verify UPower compatibility.
> >
> > ## Future Work: Userspace Integration
> >
> > As suggested by Bastien, semantic battery differentiation (e.g., "left
> > earbud" vs "right earbud") requires userspace coordination, as HID
> > reports typically lack role metadata.
> >
> > This will require:
> > 1. systemd/hwdb entries for device-specific battery role mappings
> > 2. UPower updates to enumerate and group multi-battery devices
> > 3. Desktop environment changes to display batteries with meaningful labels
> >
> > This kernel infrastructure is a prerequisite for that userspace work.
> >
> > Lucas Zampieri (1):
> > HID: input: Add support for multiple batteries per device
> >
> > Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> >
> > Changes in v4:
> > - Added missing hidinput_update_battery() stub in #else block for
> > CONFIG_HID_BATTERY_STRENGTH=n builds
> > - Reported-by: kernel test robot <lkp@intel.com>
> > - Closes: https://lore.kernel.org/oe-kbuild-all/202511201624.yUv4VtBv-lkp@intel.com/
>
> I don't think you need to keep the formal tags here in changlogs of the
> cover letter. A simple "as reported by kernel test robot
> <lkp@intel.com>" should be sufficient (given that usually we end up
> dropping the cover letter as we apply the patches only).
>
> >
> > Changes in v3:
> > - Squashed the three v2 patches into a single patch as suggested by
> > Benjamin
>
> Sorry we didn't understood each other: I was asking you to squash
> patches 1 and 2 only, and keep 3 separated. I was just complaining about
> a blank header change. Separating the non functional changes from the
> functional ones is important.
>
I get it, but the reason I made it one patch instead of two was to
have one functional patch hid-multi-battery patch, instead of one just
adding the structure, and then another one to really make use of the
multi-battery stuff, but I guess I can make it two again, even tho the
first one will have no real change in behavior
> > - Removed all legacy dev->battery_* fields, using list-based storage only
> > - Changed battery naming to include report ID: hid-{uniq}-battery-{report_id}
> > - Converted battery memory management to devm_* for automatic cleanup
>
> Haven't checked the code yet but that would require probably a separate
> patch so we can bisect to it if anything breaks.
>
I guess I kinda coded myself into a corner here, as with the new
devm_* changes, I wrote the v4/v5 hid-multi-battery feature to be made
for it, so I would have to go back to the v2 implementation (which is
fine, its just that the new devm_* makes it much cleaner), and then
have another patch converting the whole to use devm_*, which might as
well be a separate series at that points so it's easier to
review/test/bisect. Likewise, I'm fine with either approach, your
call.
> > - Updated hidinput_update_battery() to take struct hid_battery directly
> > - Added hid_get_first_battery() helper for external driver compatibility
> > - Updated hid-apple.c and hid-magicmouse.c to use new battery API
> > - Simplified cover letter based on feedback
>
> Heh, thanks :)
>
> Cheers,
> Benjamin
>
> >
> > Changes in v2:
> > - Split the monolithic v1 patch into three logical patches for easier review:
> > 1. Introduce struct hid_battery (pure structure addition)
> > 2. Refactor existing code to use the new structure (internal changes)
> > 3. Add multi-battery support (new functionality)
> > - Added detailed testing section with hardware specifics
> > - Added hid-recorder output (dactyl-hid-recording.txt) demonstrating two-battery
> > HID descriptor for UPower validation
> > - Added "Future Work: Userspace Integration" section addressing Bastien's feedback
> > about semantic battery differentiation
> > - Added hardware examples with product links to commit messages (per Bastien's
> > suggestion)
> > - No functional changes from v1, only improved patch organization and documentation
> >
> > drivers/hid/hid-apple.c | 10 +-
> > drivers/hid/hid-core.c | 4 +
> > drivers/hid/hid-input-test.c | 39 ++++---
> > drivers/hid/hid-input.c | 196 +++++++++++++++++++----------------
> > drivers/hid/hid-magicmouse.c | 10 +-
> > include/linux/hid.h | 54 +++++++---
> > 6 files changed, 187 insertions(+), 126 deletions(-)
> >
> >
> > base-commit: 0678f5630429a5049d7663703b897e1bf8d13cd1
> > --
> > 2.51.0
> >
>
^ permalink raw reply
* Re: [PATCH v4 0/1] HID: Add support for multiple batteries per device
From: Benjamin Tissoires @ 2025-11-20 11:05 UTC (permalink / raw)
To: Lucas Zampieri
Cc: linux-input, linux-kernel, Jiri Kosina, Sebastian Reichel,
Bastien Nocera, linux-pm
In-Reply-To: <20251120105442.150508-1-lzampier@redhat.com>
On Nov 20 2025, Lucas Zampieri wrote:
> This series adds support for HID devices with multiple batteries.
>
> Currently, the HID battery reporting subsystem only supports one battery per
> device. There are several devices with multiple batteries that would benefit
> from this support:
> - Gaming headsets with batteries in both the headset and charging dock
> - Wireless earbuds with per-earbud batteries plus charging case
> - Split keyboards with per-side batteries
>
> ## Proposed Solution
>
> This series introduces struct hid_battery to encapsulate individual battery
> state, replaces the old battery fields with a list-based approach, and adds
> support for multiple batteries tracked within struct hid_device. Batteries
> are identified by report ID. The implementation is fully backwards compatible
> with single-battery devices through a helper function.
>
> ## Testing
>
> Tested with split keyboard hardware (Dactyl 5x6) using custom ZMK firmware
> that implements per-side HID battery reporting. Each battery (left and right
> keyboard halves) reports independently through the power supply interface with
> distinct report IDs (0x05 and 0x06).
>
> Test firmware available on my personal fork at:
> https://github.com/zampierilucas/zmk/tree/feat/individual-hid-battery-reporting
> If this series gets merged, these changes will be proposed to upstream ZMK.
>
> HID descriptor and recording captured with hid-recorder:
>
> D: 0
> R: 162 05 01 09 06 a1 01 85 01 05 07 19 e0 29 e7 15 00 25 01 75 01 95 08 81 02 05 07 75 08 95 01 81 03 05 07 15 00 25 01 19 00 29 67 75 01 95 68 81 02 c0 05 0c 09 01 a1 01 85 02 05 0c 15 00 26 ff 0f 19 00 2a ff 0f 75 10 95 06 81 00 c0 05 84 09 05 a1 01 05 85 85 05 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0 05 84 09 05 a1 01 05 85 85 06 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0
> N: ZMK Project Dactyl 5x6
> P: usb-0000:2d:00.3-4.2/input2
> I: 3 1d50 615e
> D: 0
> E: 0.000000 3 05 00 56
> E: 0.000977 3 05 00 56
> E: 1.490974 3 06 00 52
> E: 1.491958 3 06 00 52
> E: 6.492979 3 06 00 53
> E: 6.493962 3 06 00 53
>
> The recording shows both batteries reporting with different charge levels
> (Report ID 05: 86%, Report ID 06: 82%-83%), demonstrating the multi-battery
> functionality. This can be used to verify UPower compatibility.
>
> ## Future Work: Userspace Integration
>
> As suggested by Bastien, semantic battery differentiation (e.g., "left
> earbud" vs "right earbud") requires userspace coordination, as HID
> reports typically lack role metadata.
>
> This will require:
> 1. systemd/hwdb entries for device-specific battery role mappings
> 2. UPower updates to enumerate and group multi-battery devices
> 3. Desktop environment changes to display batteries with meaningful labels
>
> This kernel infrastructure is a prerequisite for that userspace work.
>
> Lucas Zampieri (1):
> HID: input: Add support for multiple batteries per device
>
> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
>
> Changes in v4:
> - Added missing hidinput_update_battery() stub in #else block for
> CONFIG_HID_BATTERY_STRENGTH=n builds
> - Reported-by: kernel test robot <lkp@intel.com>
> - Closes: https://lore.kernel.org/oe-kbuild-all/202511201624.yUv4VtBv-lkp@intel.com/
I don't think you need to keep the formal tags here in changlogs of the
cover letter. A simple "as reported by kernel test robot
<lkp@intel.com>" should be sufficient (given that usually we end up
dropping the cover letter as we apply the patches only).
>
> Changes in v3:
> - Squashed the three v2 patches into a single patch as suggested by
> Benjamin
Sorry we didn't understood each other: I was asking you to squash
patches 1 and 2 only, and keep 3 separated. I was just complaining about
a blank header change. Separating the non functional changes from the
functional ones is important.
> - Removed all legacy dev->battery_* fields, using list-based storage only
> - Changed battery naming to include report ID: hid-{uniq}-battery-{report_id}
> - Converted battery memory management to devm_* for automatic cleanup
Haven't checked the code yet but that would require probably a separate
patch so we can bisect to it if anything breaks.
> - Updated hidinput_update_battery() to take struct hid_battery directly
> - Added hid_get_first_battery() helper for external driver compatibility
> - Updated hid-apple.c and hid-magicmouse.c to use new battery API
> - Simplified cover letter based on feedback
Heh, thanks :)
Cheers,
Benjamin
>
> Changes in v2:
> - Split the monolithic v1 patch into three logical patches for easier review:
> 1. Introduce struct hid_battery (pure structure addition)
> 2. Refactor existing code to use the new structure (internal changes)
> 3. Add multi-battery support (new functionality)
> - Added detailed testing section with hardware specifics
> - Added hid-recorder output (dactyl-hid-recording.txt) demonstrating two-battery
> HID descriptor for UPower validation
> - Added "Future Work: Userspace Integration" section addressing Bastien's feedback
> about semantic battery differentiation
> - Added hardware examples with product links to commit messages (per Bastien's
> suggestion)
> - No functional changes from v1, only improved patch organization and documentation
>
> drivers/hid/hid-apple.c | 10 +-
> drivers/hid/hid-core.c | 4 +
> drivers/hid/hid-input-test.c | 39 ++++---
> drivers/hid/hid-input.c | 196 +++++++++++++++++++----------------
> drivers/hid/hid-magicmouse.c | 10 +-
> include/linux/hid.h | 54 +++++++---
> 6 files changed, 187 insertions(+), 126 deletions(-)
>
>
> base-commit: 0678f5630429a5049d7663703b897e1bf8d13cd1
> --
> 2.51.0
>
^ permalink raw reply
* [PATCH v4 0/1] HID: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-20 10:54 UTC (permalink / raw)
To: linux-input
Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
Sebastian Reichel, Bastien Nocera, linux-pm
This series adds support for HID devices with multiple batteries.
Currently, the HID battery reporting subsystem only supports one battery per
device. There are several devices with multiple batteries that would benefit
from this support:
- Gaming headsets with batteries in both the headset and charging dock
- Wireless earbuds with per-earbud batteries plus charging case
- Split keyboards with per-side batteries
## Proposed Solution
This series introduces struct hid_battery to encapsulate individual battery
state, replaces the old battery fields with a list-based approach, and adds
support for multiple batteries tracked within struct hid_device. Batteries
are identified by report ID. The implementation is fully backwards compatible
with single-battery devices through a helper function.
## Testing
Tested with split keyboard hardware (Dactyl 5x6) using custom ZMK firmware
that implements per-side HID battery reporting. Each battery (left and right
keyboard halves) reports independently through the power supply interface with
distinct report IDs (0x05 and 0x06).
Test firmware available on my personal fork at:
https://github.com/zampierilucas/zmk/tree/feat/individual-hid-battery-reporting
If this series gets merged, these changes will be proposed to upstream ZMK.
HID descriptor and recording captured with hid-recorder:
D: 0
R: 162 05 01 09 06 a1 01 85 01 05 07 19 e0 29 e7 15 00 25 01 75 01 95 08 81 02 05 07 75 08 95 01 81 03 05 07 15 00 25 01 19 00 29 67 75 01 95 68 81 02 c0 05 0c 09 01 a1 01 85 02 05 0c 15 00 26 ff 0f 19 00 2a ff 0f 75 10 95 06 81 00 c0 05 84 09 05 a1 01 05 85 85 05 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0 05 84 09 05 a1 01 05 85 85 06 09 44 15 00 25 01 35 00 45 01 75 08 95 01 81 02 09 65 15 00 25 64 35 00 45 64 75 08 95 01 81 02 c0
N: ZMK Project Dactyl 5x6
P: usb-0000:2d:00.3-4.2/input2
I: 3 1d50 615e
D: 0
E: 0.000000 3 05 00 56
E: 0.000977 3 05 00 56
E: 1.490974 3 06 00 52
E: 1.491958 3 06 00 52
E: 6.492979 3 06 00 53
E: 6.493962 3 06 00 53
The recording shows both batteries reporting with different charge levels
(Report ID 05: 86%, Report ID 06: 82%-83%), demonstrating the multi-battery
functionality. This can be used to verify UPower compatibility.
## Future Work: Userspace Integration
As suggested by Bastien, semantic battery differentiation (e.g., "left
earbud" vs "right earbud") requires userspace coordination, as HID
reports typically lack role metadata.
This will require:
1. systemd/hwdb entries for device-specific battery role mappings
2. UPower updates to enumerate and group multi-battery devices
3. Desktop environment changes to display batteries with meaningful labels
This kernel infrastructure is a prerequisite for that userspace work.
Lucas Zampieri (1):
HID: input: Add support for multiple batteries per device
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
Changes in v4:
- Added missing hidinput_update_battery() stub in #else block for
CONFIG_HID_BATTERY_STRENGTH=n builds
- Reported-by: kernel test robot <lkp@intel.com>
- Closes: https://lore.kernel.org/oe-kbuild-all/202511201624.yUv4VtBv-lkp@intel.com/
Changes in v3:
- Squashed the three v2 patches into a single patch as suggested by
Benjamin
- Removed all legacy dev->battery_* fields, using list-based storage only
- Changed battery naming to include report ID: hid-{uniq}-battery-{report_id}
- Converted battery memory management to devm_* for automatic cleanup
- Updated hidinput_update_battery() to take struct hid_battery directly
- Added hid_get_first_battery() helper for external driver compatibility
- Updated hid-apple.c and hid-magicmouse.c to use new battery API
- Simplified cover letter based on feedback
Changes in v2:
- Split the monolithic v1 patch into three logical patches for easier review:
1. Introduce struct hid_battery (pure structure addition)
2. Refactor existing code to use the new structure (internal changes)
3. Add multi-battery support (new functionality)
- Added detailed testing section with hardware specifics
- Added hid-recorder output (dactyl-hid-recording.txt) demonstrating two-battery
HID descriptor for UPower validation
- Added "Future Work: Userspace Integration" section addressing Bastien's feedback
about semantic battery differentiation
- Added hardware examples with product links to commit messages (per Bastien's
suggestion)
- No functional changes from v1, only improved patch organization and documentation
drivers/hid/hid-apple.c | 10 +-
drivers/hid/hid-core.c | 4 +
drivers/hid/hid-input-test.c | 39 ++++---
drivers/hid/hid-input.c | 196 +++++++++++++++++++----------------
drivers/hid/hid-magicmouse.c | 10 +-
include/linux/hid.h | 54 +++++++---
6 files changed, 187 insertions(+), 126 deletions(-)
base-commit: 0678f5630429a5049d7663703b897e1bf8d13cd1
--
2.51.0
^ permalink raw reply
* [PATCH v4 1/1] HID: input: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-20 10:54 UTC (permalink / raw)
To: linux-input
Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
Sebastian Reichel, Bastien Nocera, linux-pm
In-Reply-To: <20251120105442.150508-1-lzampier@redhat.com>
Introduce struct hid_battery to encapsulate individual battery state and
enable HID devices to register multiple batteries, each identified by
its report ID.
This change adds struct hid_battery with all battery-related fields and
replaces the legacy dev->battery_* fields with a batteries list. All
memory management is converted to use devm_* for simpler cleanup.
Batteries are named using their report ID with the pattern
hid-{uniq}-battery-{report_id}. External drivers hid-apple and
hid-magicmouse are updated to use the new battery API via the
hid_get_first_battery() helper, and hid-input-test is updated for the
new battery structure.
This enables proper battery reporting for devices with multiple
batteries such as split keyboards, gaming headsets with charging docks,
and wireless earbuds with per-earbud batteries.
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
drivers/hid/hid-apple.c | 10 +-
drivers/hid/hid-core.c | 4 +
drivers/hid/hid-input-test.c | 39 ++++---
drivers/hid/hid-input.c | 196 +++++++++++++++++++----------------
drivers/hid/hid-magicmouse.c | 10 +-
include/linux/hid.h | 54 +++++++---
6 files changed, 187 insertions(+), 126 deletions(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 57da4f86a..42ff0be7c 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -619,17 +619,19 @@ static int apple_fetch_battery(struct hid_device *hdev)
struct apple_sc *asc = hid_get_drvdata(hdev);
struct hid_report_enum *report_enum;
struct hid_report *report;
+ struct hid_battery *bat;
- if (!(asc->quirks & APPLE_RDESC_BATTERY) || !hdev->battery)
+ bat = hid_get_first_battery(hdev);
+ if (!(asc->quirks & APPLE_RDESC_BATTERY) || !bat)
return -1;
- report_enum = &hdev->report_enum[hdev->battery_report_type];
- report = report_enum->report_id_hash[hdev->battery_report_id];
+ report_enum = &hdev->report_enum[bat->report_type];
+ report = report_enum->report_id_hash[bat->report_id];
if (!report || report->maxfield < 1)
return -1;
- if (hdev->battery_capacity == hdev->battery_max)
+ if (bat->capacity == bat->max)
return -1;
hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a5b3a8ca2..76d628547 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2990,6 +2990,10 @@ struct hid_device *hid_allocate_device(void)
mutex_init(&hdev->ll_open_lock);
kref_init(&hdev->ref);
+#ifdef CONFIG_HID_BATTERY_STRENGTH
+ INIT_LIST_HEAD(&hdev->batteries);
+#endif
+
ret = hid_bpf_device_init(hdev);
if (ret)
goto out_err;
diff --git a/drivers/hid/hid-input-test.c b/drivers/hid/hid-input-test.c
index 6f5c71660..c92008daf 100644
--- a/drivers/hid/hid-input-test.c
+++ b/drivers/hid/hid-input-test.c
@@ -9,54 +9,59 @@
static void hid_test_input_update_battery_charge_status(struct kunit *test)
{
- struct hid_device *dev;
+ struct hid_battery *bat;
bool handled;
- dev = kunit_kzalloc(test, sizeof(*dev), GFP_KERNEL);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+ bat = kunit_kzalloc(test, sizeof(*bat), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bat);
- handled = hidinput_update_battery_charge_status(dev, HID_DG_HEIGHT, 0);
+ handled = hidinput_update_battery_charge_status(bat, HID_DG_HEIGHT, 0);
KUNIT_EXPECT_FALSE(test, handled);
- KUNIT_EXPECT_EQ(test, dev->battery_charge_status, POWER_SUPPLY_STATUS_UNKNOWN);
+ KUNIT_EXPECT_EQ(test, bat->charge_status, POWER_SUPPLY_STATUS_UNKNOWN);
- handled = hidinput_update_battery_charge_status(dev, HID_BAT_CHARGING, 0);
+ handled = hidinput_update_battery_charge_status(bat, HID_BAT_CHARGING, 0);
KUNIT_EXPECT_TRUE(test, handled);
- KUNIT_EXPECT_EQ(test, dev->battery_charge_status, POWER_SUPPLY_STATUS_DISCHARGING);
+ KUNIT_EXPECT_EQ(test, bat->charge_status, POWER_SUPPLY_STATUS_DISCHARGING);
- handled = hidinput_update_battery_charge_status(dev, HID_BAT_CHARGING, 1);
+ handled = hidinput_update_battery_charge_status(bat, HID_BAT_CHARGING, 1);
KUNIT_EXPECT_TRUE(test, handled);
- KUNIT_EXPECT_EQ(test, dev->battery_charge_status, POWER_SUPPLY_STATUS_CHARGING);
+ KUNIT_EXPECT_EQ(test, bat->charge_status, POWER_SUPPLY_STATUS_CHARGING);
}
static void hid_test_input_get_battery_property(struct kunit *test)
{
struct power_supply *psy;
+ struct hid_battery *bat;
struct hid_device *dev;
union power_supply_propval val;
int ret;
dev = kunit_kzalloc(test, sizeof(*dev), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
- dev->battery_avoid_query = true;
+
+ bat = kunit_kzalloc(test, sizeof(*bat), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bat);
+ bat->dev = dev;
+ bat->avoid_query = true;
psy = kunit_kzalloc(test, sizeof(*psy), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, psy);
- psy->drv_data = dev;
+ psy->drv_data = bat;
- dev->battery_status = HID_BATTERY_UNKNOWN;
- dev->battery_charge_status = POWER_SUPPLY_STATUS_CHARGING;
+ bat->status = HID_BATTERY_UNKNOWN;
+ bat->charge_status = POWER_SUPPLY_STATUS_CHARGING;
ret = hidinput_get_battery_property(psy, POWER_SUPPLY_PROP_STATUS, &val);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_EQ(test, val.intval, POWER_SUPPLY_STATUS_UNKNOWN);
- dev->battery_status = HID_BATTERY_REPORTED;
- dev->battery_charge_status = POWER_SUPPLY_STATUS_CHARGING;
+ bat->status = HID_BATTERY_REPORTED;
+ bat->charge_status = POWER_SUPPLY_STATUS_CHARGING;
ret = hidinput_get_battery_property(psy, POWER_SUPPLY_PROP_STATUS, &val);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_EQ(test, val.intval, POWER_SUPPLY_STATUS_CHARGING);
- dev->battery_status = HID_BATTERY_REPORTED;
- dev->battery_charge_status = POWER_SUPPLY_STATUS_DISCHARGING;
+ bat->status = HID_BATTERY_REPORTED;
+ bat->charge_status = POWER_SUPPLY_STATUS_DISCHARGING;
ret = hidinput_get_battery_property(psy, POWER_SUPPLY_PROP_STATUS, &val);
KUNIT_EXPECT_EQ(test, ret, 0);
KUNIT_EXPECT_EQ(test, val.intval, POWER_SUPPLY_STATUS_DISCHARGING);
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2bbb645c2..9c1afe88f 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -419,18 +419,18 @@ static unsigned find_battery_quirk(struct hid_device *hdev)
return quirks;
}
-static int hidinput_scale_battery_capacity(struct hid_device *dev,
+static int hidinput_scale_battery_capacity(struct hid_battery *bat,
int value)
{
- if (dev->battery_min < dev->battery_max &&
- value >= dev->battery_min && value <= dev->battery_max)
- value = ((value - dev->battery_min) * 100) /
- (dev->battery_max - dev->battery_min);
+ if (bat->min < bat->max &&
+ value >= bat->min && value <= bat->max)
+ value = ((value - bat->min) * 100) /
+ (bat->max - bat->min);
return value;
}
-static int hidinput_query_battery_capacity(struct hid_device *dev)
+static int hidinput_query_battery_capacity(struct hid_battery *bat)
{
u8 *buf;
int ret;
@@ -439,14 +439,14 @@ static int hidinput_query_battery_capacity(struct hid_device *dev)
if (!buf)
return -ENOMEM;
- ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 4,
- dev->battery_report_type, HID_REQ_GET_REPORT);
+ ret = hid_hw_raw_request(bat->dev, bat->report_id, buf, 4,
+ bat->report_type, HID_REQ_GET_REPORT);
if (ret < 2) {
kfree(buf);
return -ENODATA;
}
- ret = hidinput_scale_battery_capacity(dev, buf[1]);
+ ret = hidinput_scale_battery_capacity(bat, buf[1]);
kfree(buf);
return ret;
}
@@ -455,7 +455,8 @@ static int hidinput_get_battery_property(struct power_supply *psy,
enum power_supply_property prop,
union power_supply_propval *val)
{
- struct hid_device *dev = power_supply_get_drvdata(psy);
+ struct hid_battery *bat = power_supply_get_drvdata(psy);
+ struct hid_device *dev = bat->dev;
int value;
int ret = 0;
@@ -466,13 +467,13 @@ static int hidinput_get_battery_property(struct power_supply *psy,
break;
case POWER_SUPPLY_PROP_CAPACITY:
- if (dev->battery_status != HID_BATTERY_REPORTED &&
- !dev->battery_avoid_query) {
- value = hidinput_query_battery_capacity(dev);
+ if (bat->status != HID_BATTERY_REPORTED &&
+ !bat->avoid_query) {
+ value = hidinput_query_battery_capacity(bat);
if (value < 0)
return value;
} else {
- value = dev->battery_capacity;
+ value = bat->capacity;
}
val->intval = value;
@@ -483,20 +484,20 @@ static int hidinput_get_battery_property(struct power_supply *psy,
break;
case POWER_SUPPLY_PROP_STATUS:
- if (dev->battery_status != HID_BATTERY_REPORTED &&
- !dev->battery_avoid_query) {
- value = hidinput_query_battery_capacity(dev);
+ if (bat->status != HID_BATTERY_REPORTED &&
+ !bat->avoid_query) {
+ value = hidinput_query_battery_capacity(bat);
if (value < 0)
return value;
- dev->battery_capacity = value;
- dev->battery_status = HID_BATTERY_QUERIED;
+ bat->capacity = value;
+ bat->status = HID_BATTERY_QUERIED;
}
- if (dev->battery_status == HID_BATTERY_UNKNOWN)
+ if (bat->status == HID_BATTERY_UNKNOWN)
val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
else
- val->intval = dev->battery_charge_status;
+ val->intval = bat->charge_status;
break;
case POWER_SUPPLY_PROP_SCOPE:
@@ -511,37 +512,54 @@ static int hidinput_get_battery_property(struct power_supply *psy,
return ret;
}
+static struct hid_battery *hidinput_find_battery(struct hid_device *dev,
+ int report_id)
+{
+ struct hid_battery *bat;
+
+ list_for_each_entry(bat, &dev->batteries, list) {
+ if (bat->report_id == report_id)
+ return bat;
+ }
+ return NULL;
+}
+
static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
struct hid_field *field, bool is_percentage)
{
+ struct hid_battery *bat;
struct power_supply_desc *psy_desc;
- struct power_supply_config psy_cfg = { .drv_data = dev, };
+ struct power_supply_config psy_cfg = { 0 };
unsigned quirks;
s32 min, max;
- int error;
- if (dev->battery)
- return 0; /* already initialized? */
+ if (hidinput_find_battery(dev, field->report->id))
+ return 0; /* already initialized */
quirks = find_battery_quirk(dev);
- hid_dbg(dev, "device %x:%x:%x %d quirks %d\n",
- dev->bus, dev->vendor, dev->product, dev->version, quirks);
+ hid_dbg(dev, "device %x:%x:%x %d quirks %d report_id %d\n",
+ dev->bus, dev->vendor, dev->product, dev->version, quirks,
+ field->report->id);
if (quirks & HID_BATTERY_QUIRK_IGNORE)
return 0;
- psy_desc = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
+ bat = devm_kzalloc(&dev->dev, sizeof(*bat), GFP_KERNEL);
+ if (!bat)
+ return -ENOMEM;
+
+ psy_desc = devm_kzalloc(&dev->dev, sizeof(*psy_desc), GFP_KERNEL);
if (!psy_desc)
return -ENOMEM;
- psy_desc->name = kasprintf(GFP_KERNEL, "hid-%s-battery",
- strlen(dev->uniq) ?
- dev->uniq : dev_name(&dev->dev));
- if (!psy_desc->name) {
- error = -ENOMEM;
- goto err_free_mem;
- }
+ psy_desc->name = devm_kasprintf(&dev->dev, GFP_KERNEL,
+ "hid-%s-battery-%d",
+ strlen(dev->uniq) ?
+ dev->uniq : dev_name(&dev->dev),
+ field->report->id);
+ if (!psy_desc->name)
+ return -ENOMEM;
psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
psy_desc->properties = hidinput_battery_props;
@@ -560,98 +578,89 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
if (quirks & HID_BATTERY_QUIRK_FEATURE)
report_type = HID_FEATURE_REPORT;
- dev->battery_min = min;
- dev->battery_max = max;
- dev->battery_report_type = report_type;
- dev->battery_report_id = field->report->id;
- dev->battery_charge_status = POWER_SUPPLY_STATUS_DISCHARGING;
+ bat->dev = dev;
+ bat->min = min;
+ bat->max = max;
+ bat->report_type = report_type;
+ bat->report_id = field->report->id;
+ bat->charge_status = POWER_SUPPLY_STATUS_DISCHARGING;
+ bat->status = HID_BATTERY_UNKNOWN;
/*
* Stylus is normally not connected to the device and thus we
* can't query the device and get meaningful battery strength.
* We have to wait for the device to report it on its own.
*/
- dev->battery_avoid_query = report_type == HID_INPUT_REPORT &&
- field->physical == HID_DG_STYLUS;
+ bat->avoid_query = report_type == HID_INPUT_REPORT &&
+ field->physical == HID_DG_STYLUS;
if (quirks & HID_BATTERY_QUIRK_AVOID_QUERY)
- dev->battery_avoid_query = true;
-
- dev->battery = power_supply_register(&dev->dev, psy_desc, &psy_cfg);
- if (IS_ERR(dev->battery)) {
- error = PTR_ERR(dev->battery);
- hid_warn(dev, "can't register power supply: %d\n", error);
- goto err_free_name;
+ bat->avoid_query = true;
+
+ psy_cfg.drv_data = bat;
+ bat->ps = devm_power_supply_register(&dev->dev, psy_desc, &psy_cfg);
+ if (IS_ERR(bat->ps)) {
+ hid_warn(dev, "can't register power supply: %ld\n",
+ PTR_ERR(bat->ps));
+ return PTR_ERR(bat->ps);
}
- power_supply_powers(dev->battery, &dev->dev);
- return 0;
+ power_supply_powers(bat->ps, &dev->dev);
-err_free_name:
- kfree(psy_desc->name);
-err_free_mem:
- kfree(psy_desc);
- dev->battery = NULL;
- return error;
+ list_add_tail(&bat->list, &dev->batteries);
+
+ return 0;
}
static void hidinput_cleanup_battery(struct hid_device *dev)
{
- const struct power_supply_desc *psy_desc;
-
- if (!dev->battery)
- return;
+ struct hid_battery *bat, *next;
- psy_desc = dev->battery->desc;
- power_supply_unregister(dev->battery);
- kfree(psy_desc->name);
- kfree(psy_desc);
- dev->battery = NULL;
+ list_for_each_entry_safe(bat, next, &dev->batteries, list) {
+ list_del(&bat->list);
+ }
}
-static bool hidinput_update_battery_charge_status(struct hid_device *dev,
+static bool hidinput_update_battery_charge_status(struct hid_battery *bat,
unsigned int usage, int value)
{
switch (usage) {
case HID_BAT_CHARGING:
- dev->battery_charge_status = value ?
- POWER_SUPPLY_STATUS_CHARGING :
- POWER_SUPPLY_STATUS_DISCHARGING;
+ bat->charge_status = value ?
+ POWER_SUPPLY_STATUS_CHARGING :
+ POWER_SUPPLY_STATUS_DISCHARGING;
return true;
}
return false;
}
-static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
- int value)
+static void hidinput_update_battery(struct hid_battery *bat,
+ unsigned int usage, int value)
{
int capacity;
- if (!dev->battery)
- return;
-
- if (hidinput_update_battery_charge_status(dev, usage, value)) {
- power_supply_changed(dev->battery);
+ if (hidinput_update_battery_charge_status(bat, usage, value)) {
+ power_supply_changed(bat->ps);
return;
}
if ((usage & HID_USAGE_PAGE) == HID_UP_DIGITIZER && value == 0)
return;
- if (value < dev->battery_min || value > dev->battery_max)
+ if (value < bat->min || value > bat->max)
return;
- capacity = hidinput_scale_battery_capacity(dev, value);
+ capacity = hidinput_scale_battery_capacity(bat, value);
- if (dev->battery_status != HID_BATTERY_REPORTED ||
- capacity != dev->battery_capacity ||
- ktime_after(ktime_get_coarse(), dev->battery_ratelimit_time)) {
- dev->battery_capacity = capacity;
- dev->battery_status = HID_BATTERY_REPORTED;
- dev->battery_ratelimit_time =
+ if (bat->status != HID_BATTERY_REPORTED ||
+ capacity != bat->capacity ||
+ ktime_after(ktime_get_coarse(), bat->ratelimit_time)) {
+ bat->capacity = capacity;
+ bat->status = HID_BATTERY_REPORTED;
+ bat->ratelimit_time =
ktime_add_ms(ktime_get_coarse(), 30 * 1000);
- power_supply_changed(dev->battery);
+ power_supply_changed(bat->ps);
}
}
#else /* !CONFIG_HID_BATTERY_STRENGTH */
@@ -665,8 +674,14 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
{
}
-static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
- int value)
+static struct hid_battery *hidinput_find_battery(struct hid_device *dev,
+ int report_id)
+{
+ return NULL;
+}
+
+static void hidinput_update_battery(struct hid_battery *bat,
+ unsigned int usage, int value)
{
}
#endif /* CONFIG_HID_BATTERY_STRENGTH */
@@ -1534,7 +1549,10 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
return;
if (usage->type == EV_PWR) {
- hidinput_update_battery(hid, usage->hid, value);
+ struct hid_battery *bat = hidinput_find_battery(hid, report->id);
+
+ if (bat)
+ hidinput_update_battery(bat, usage->hid, value);
return;
}
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 7d4a25c6d..b495f7a4b 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -812,19 +812,21 @@ static int magicmouse_fetch_battery(struct hid_device *hdev)
#ifdef CONFIG_HID_BATTERY_STRENGTH
struct hid_report_enum *report_enum;
struct hid_report *report;
+ struct hid_battery *bat;
- if (!hdev->battery ||
+ bat = hid_get_first_battery(hdev);
+ if (!bat ||
(!is_usb_magicmouse2(hdev->vendor, hdev->product) &&
!is_usb_magictrackpad2(hdev->vendor, hdev->product)))
return -1;
- report_enum = &hdev->report_enum[hdev->battery_report_type];
- report = report_enum->report_id_hash[hdev->battery_report_id];
+ report_enum = &hdev->report_enum[bat->report_type];
+ report = report_enum->report_id_hash[bat->report_id];
if (!report || report->maxfield < 1)
return -1;
- if (hdev->battery_capacity == hdev->battery_max)
+ if (bat->capacity == bat->max)
return -1;
hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index a4ddb94e3..3e33ef74c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -634,6 +634,36 @@ enum hid_battery_status {
HID_BATTERY_REPORTED, /* Device sent unsolicited battery strength report */
};
+/**
+ * struct hid_battery - represents a single battery power supply
+ * @dev: pointer to the parent hid_device
+ * @ps: the power supply instance
+ * @min: minimum battery value from HID descriptor
+ * @max: maximum battery value from HID descriptor
+ * @report_type: HID report type (input/feature)
+ * @report_id: HID report ID for this battery
+ * @charge_status: current charging status
+ * @status: battery reporting status
+ * @capacity: current battery capacity (0-100)
+ * @avoid_query: if true, avoid querying battery (e.g., for stylus)
+ * @ratelimit_time: rate limiting for battery reports
+ * @list: list node for linking into hid_device's battery list
+ */
+struct hid_battery {
+ struct hid_device *dev;
+ struct power_supply *ps;
+ __s32 min;
+ __s32 max;
+ __s32 report_type;
+ __s32 report_id;
+ __s32 charge_status;
+ enum hid_battery_status status;
+ __s32 capacity;
+ bool avoid_query;
+ ktime_t ratelimit_time;
+ struct list_head list;
+};
+
struct hid_driver;
struct hid_ll_driver;
@@ -670,19 +700,10 @@ struct hid_device {
#ifdef CONFIG_HID_BATTERY_STRENGTH
/*
* Power supply information for HID devices which report
- * battery strength. power_supply was successfully registered if
- * battery is non-NULL.
+ * battery strength. Each battery is tracked separately in the
+ * batteries list.
*/
- struct power_supply *battery;
- __s32 battery_capacity;
- __s32 battery_min;
- __s32 battery_max;
- __s32 battery_report_type;
- __s32 battery_report_id;
- __s32 battery_charge_status;
- enum hid_battery_status battery_status;
- bool battery_avoid_query;
- ktime_t battery_ratelimit_time;
+ struct list_head batteries;
#endif
unsigned long status; /* see STAT flags above */
@@ -743,6 +764,15 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
dev_set_drvdata(&hdev->dev, data);
}
+#ifdef CONFIG_HID_BATTERY_STRENGTH
+static inline struct hid_battery *hid_get_first_battery(struct hid_device *hdev)
+{
+ if (list_empty(&hdev->batteries))
+ return NULL;
+ return list_first_entry(&hdev->batteries, struct hid_battery, list);
+}
+#endif
+
#define HID_GLOBAL_STACK_SIZE 4
#define HID_COLLECTION_STACK_SIZE 4
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v1] selftests: hid: tests: test_wacom_generic: add base test for display devices and opaque devices
From: Benjamin Tissoires @ 2025-11-20 10:53 UTC (permalink / raw)
To: Alex Tran; +Cc: jikos, shuah, linux-input, linux-kselftest, linux-kernel
In-Reply-To: <20251117014721.3142490-1-alex.t.tran@gmail.com>
Hi Alex,
On Nov 16 2025, Alex Tran wrote:
> Verify Wacom devices set INPUT_PROP_DIRECT appropriately on display devices
> and INPUT_PROP_POINTER appropriately on opaque devices. Tests are defined
> in the base class and disabled for inapplicable device types.
>
> Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
> ---
> .../selftests/hid/tests/test_wacom_generic.py | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/tests/test_wacom_generic.py b/tools/testing/selftests/hid/tests/test_wacom_generic.py
> index 2d6d04f0f..aa2a175f2 100644
> --- a/tools/testing/selftests/hid/tests/test_wacom_generic.py
> +++ b/tools/testing/selftests/hid/tests/test_wacom_generic.py
> @@ -600,15 +600,17 @@ class BaseTest:
>
> def test_prop_direct(self):
> """
> - Todo: Verify that INPUT_PROP_DIRECT is set on display devices.
> + Verify that INPUT_PROP_DIRECT is set on display devices.
> """
> - pass
> + evdev = self.uhdev.get_evdev()
> + assert libevdev.INPUT_PROP_DIRECT in evdev.properties
>
> def test_prop_pointer(self):
> """
> - Todo: Verify that INPUT_PROP_POINTER is set on opaque devices.
> + Verify that INPUT_PROP_POINTER is set on opaque devices.
> """
> - pass
> + evdev = self.uhdev.get_evdev()
> + assert libevdev.INPUT_PROP_POINTER in evdev.properties
>
>
> class PenTabletTest(BaseTest.TestTablet):
> @@ -622,6 +624,8 @@ class TouchTabletTest(BaseTest.TestTablet):
>
>
> class TestOpaqueTablet(PenTabletTest):
> + test_prop_direct = None
That seems very awkward to do.
Why not drop the 2 tests from the TestTablet class, move
`test_prop_direct()` in that subclass (TestOpaqueTablet), and add a new
TestDirectTablet class that TestDTH2452Tablet will be a subclass of?
Basically try to make better use of subclassing instead of adding the
tests at the top level class and selectively remove them in the
subclasses.
Cheers,
Benjamin
> +
> def create_device(self):
> return OpaqueTablet()
>
> @@ -864,6 +868,7 @@ class TestPTHX60_Pen(TestOpaqueCTLTablet):
>
> class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest):
> ContactIds = namedtuple("ContactIds", "contact_id, tracking_id, slot_num")
> + test_prop_pointer = None
>
> def create_device(self):
> return test_multitouch.Digitizer(
> --
> 2.51.0
>
^ permalink raw reply
* [PATCH v9 11/11] HID: asus: add support for the asus-wmi brightness handler
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
If the asus-wmi brightness handler is available, send the
keyboard brightness events to it instead of passing them
to userspace. If it is not, fall back to sending them to it.
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Tested-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 6a355c174f29..ff5aaebc38e3 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -324,6 +324,17 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
usage->hid & HID_USAGE);
}
+ if (usage->type == EV_KEY && value) {
+ switch (usage->code) {
+ case KEY_KBDILLUMUP:
+ return !asus_hid_event(ASUS_EV_BRTUP);
+ case KEY_KBDILLUMDOWN:
+ return !asus_hid_event(ASUS_EV_BRTDOWN);
+ case KEY_KBDILLUMTOGGLE:
+ return !asus_hid_event(ASUS_EV_BRTTOGGLE);
+ }
+ }
+
return 0;
}
--
2.52.0
^ permalink raw reply related
* [PATCH v9 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
Some devices, such as the Z13 have multiple Aura devices connected
to them by USB. In addition, they might have a WMI interface for
RGB. In Windows, Armoury Crate exposes a unified brightness slider
for all of them, with 3 brightness levels.
Therefore, to be synergistic in Linux, and support existing tooling
such as UPower, allow adding listeners to the RGB device of the WMI
interface. If WMI does not exist, lazy initialize the interface.
Since hid-asus and asus-wmi can both interact with the led objects
including from an atomic context, protect the brightness access with a
spinlock and update the values from a workqueue. Use this workqueue to
also process WMI keyboard events, so they are handled asynchronously.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/platform/x86/asus-wmi.c | 174 ++++++++++++++++++---
include/linux/platform_data/x86/asus-wmi.h | 17 ++
2 files changed, 167 insertions(+), 24 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index e72a2b5d158e..5f23aedbf34f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -36,6 +36,7 @@
#include <linux/rfkill.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/units.h>
@@ -258,6 +259,9 @@ struct asus_wmi {
int tpd_led_wk;
struct led_classdev kbd_led;
int kbd_led_wk;
+ bool kbd_led_notify;
+ bool kbd_led_avail;
+ bool kbd_led_registered;
struct led_classdev lightbar_led;
int lightbar_led_wk;
struct led_classdev micmute_led;
@@ -266,6 +270,7 @@ struct asus_wmi {
struct work_struct tpd_led_work;
struct work_struct wlan_led_work;
struct work_struct lightbar_led_work;
+ struct work_struct kbd_led_work;
struct asus_rfkill wlan;
struct asus_rfkill bluetooth;
@@ -1530,6 +1535,99 @@ static void asus_wmi_battery_exit(struct asus_wmi *asus)
/* LEDs ***********************************************************************/
+struct asus_hid_ref {
+ struct list_head listeners;
+ struct asus_wmi *asus;
+ /* Protects concurrent access from hid-asus and asus-wmi to leds */
+ spinlock_t lock;
+};
+
+static struct asus_hid_ref asus_ref = {
+ .listeners = LIST_HEAD_INIT(asus_ref.listeners),
+ .asus = NULL,
+ /*
+ * Protects .asus, .asus.kbd_led_{wk,notify}, and .listener refs. Other
+ * asus variables are read-only after .asus is set. Except the led cdev
+ * device if not kbd_led_avail. That becomes read-only after the
+ * first hid-asus listener registers and triggers the work queue. It is
+ * then not referenced again until unregistering, which happens after
+ * .asus ref is dropped. Since .asus needs to be accessed by hid-asus
+ * IRQs to check if forwarding events is possible, a spinlock is used.
+ */
+ .lock = __SPIN_LOCK_UNLOCKED(asus_ref.lock),
+};
+
+/*
+ * Allows registering hid-asus listeners that want to be notified of
+ * keyboard backlight changes.
+ */
+int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+ struct asus_wmi *asus;
+
+ guard(spinlock_irqsave)(&asus_ref.lock);
+ list_add_tail(&bdev->list, &asus_ref.listeners);
+ asus = asus_ref.asus;
+ if (asus)
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(asus_hid_register_listener);
+
+/*
+ * Allows unregistering hid-asus listeners that were added with
+ * asus_hid_register_listener().
+ */
+void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+ guard(spinlock_irqsave)(&asus_ref.lock);
+ list_del(&bdev->list);
+}
+EXPORT_SYMBOL_GPL(asus_hid_unregister_listener);
+
+static void do_kbd_led_set(struct led_classdev *led_cdev, int value);
+
+static void kbd_led_update_all(struct work_struct *work)
+{
+ struct asus_wmi *asus;
+ bool registered, notify;
+ int ret, value;
+
+ asus = container_of(work, struct asus_wmi, kbd_led_work);
+
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ registered = asus->kbd_led_registered;
+ value = asus->kbd_led_wk;
+ notify = asus->kbd_led_notify;
+ }
+
+ if (!registered) {
+ /*
+ * This workqueue runs under asus-wmi, which means probe has
+ * completed and asus-wmi will keep running until it finishes.
+ * Therefore, we can safely register the LED without holding
+ * a spinlock.
+ */
+ ret = devm_led_classdev_register(&asus->platform_device->dev,
+ &asus->kbd_led);
+ if (!ret) {
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_registered = true;
+ } else {
+ pr_warn("Failed to register keyboard backlight LED: %d\n", ret);
+ return;
+ }
+ }
+
+ if (value >= 0)
+ do_kbd_led_set(&asus->kbd_led, value);
+ if (notify) {
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_notify = false;
+ led_classdev_notify_brightness_hw_changed(&asus->kbd_led, value);
+ }
+}
+
/*
* These functions actually update the LED's, and are called from a
* workqueue. By doing this as separate work rather than when the LED
@@ -1576,7 +1674,8 @@ static void kbd_led_update(struct asus_wmi *asus)
{
int ctrl_param = 0;
- ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
}
@@ -1609,14 +1708,23 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
{
+ struct asus_hid_listener *listener;
struct asus_wmi *asus;
int max_level;
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
max_level = asus->kbd_led.max_brightness;
- asus->kbd_led_wk = clamp_val(value, 0, max_level);
- kbd_led_update(asus);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_wk = clamp_val(value, 0, max_level);
+
+ if (asus->kbd_led_avail)
+ kbd_led_update(asus);
+
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ list_for_each_entry(listener, &asus_ref.listeners, list)
+ listener->brightness_set(listener, asus->kbd_led_wk);
+ }
}
static void kbd_led_set(struct led_classdev *led_cdev,
@@ -1631,10 +1739,11 @@ static void kbd_led_set(struct led_classdev *led_cdev,
static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
{
- struct led_classdev *led_cdev = &asus->kbd_led;
-
- do_kbd_led_set(led_cdev, value);
- led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ asus->kbd_led_wk = value;
+ asus->kbd_led_notify = true;
+ }
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
}
static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
@@ -1644,10 +1753,18 @@ static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ if (!asus->kbd_led_avail)
+ return asus->kbd_led_wk;
+ }
+
retval = kbd_led_read(asus, &value, NULL);
if (retval < 0)
return retval;
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus->kbd_led_wk = value;
+
return value;
}
@@ -1759,7 +1876,9 @@ static int camera_led_set(struct led_classdev *led_cdev,
static void asus_wmi_led_exit(struct asus_wmi *asus)
{
- led_classdev_unregister(&asus->kbd_led);
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ asus_ref.asus = NULL;
+
led_classdev_unregister(&asus->tpd_led);
led_classdev_unregister(&asus->wlan_led);
led_classdev_unregister(&asus->lightbar_led);
@@ -1797,22 +1916,25 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
goto error;
}
- if (!kbd_led_read(asus, &led_val, NULL) && !dmi_check_system(asus_use_hid_led_dmi_ids)) {
- pr_info("using asus-wmi for asus::kbd_backlight\n");
- asus->kbd_led_wk = led_val;
- asus->kbd_led.name = "asus::kbd_backlight";
- asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
- asus->kbd_led.brightness_set = kbd_led_set;
- asus->kbd_led.brightness_get = kbd_led_get;
- asus->kbd_led.max_brightness = 3;
+ asus->kbd_led.name = "asus::kbd_backlight";
+ asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
+ asus->kbd_led.brightness_set = kbd_led_set;
+ asus->kbd_led.brightness_get = kbd_led_get;
+ asus->kbd_led.max_brightness = 3;
+ asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
+ INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
+ if (asus->kbd_led_avail) {
+ asus->kbd_led_wk = led_val;
if (num_rgb_groups != 0)
asus->kbd_led.groups = kbd_rgb_mode_groups;
+ } else
+ asus->kbd_led_wk = -1;
- rv = led_classdev_register(&asus->platform_device->dev,
- &asus->kbd_led);
- if (rv)
- goto error;
+ scoped_guard(spinlock_irqsave, &asus_ref.lock) {
+ asus_ref.asus = asus;
+ if (asus->kbd_led_avail || !list_empty(&asus_ref.listeners))
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
}
if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_WIRELESS_LED)
@@ -4272,6 +4394,7 @@ static int asus_wmi_get_event_code(union acpi_object *obj)
static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
{
+ enum led_brightness led_value;
unsigned int key_value = 1;
bool autorelease = 1;
@@ -4288,19 +4411,22 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
+ scoped_guard(spinlock_irqsave, &asus_ref.lock)
+ led_value = asus->kbd_led_wk;
+
if (code == NOTIFY_KBD_BRTUP) {
- kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+ kbd_led_set_by_kbd(asus, led_value + 1);
return;
}
if (code == NOTIFY_KBD_BRTDWN) {
- kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
+ kbd_led_set_by_kbd(asus, led_value - 1);
return;
}
if (code == NOTIFY_KBD_BRTTOGGLE) {
- if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
+ if (led_value == asus->kbd_led.max_brightness)
kbd_led_set_by_kbd(asus, 0);
else
- kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+ kbd_led_set_by_kbd(asus, led_value + 1);
return;
}
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 8a515179113d..1165039013b1 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -163,11 +163,20 @@ enum asus_ally_mcu_hack {
ASUS_WMI_ALLY_MCU_HACK_DISABLED,
};
+/* Used to notify hid-asus when asus-wmi changes keyboard backlight */
+struct asus_hid_listener {
+ struct list_head list;
+ void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
+};
+
#if IS_REACHABLE(CONFIG_ASUS_WMI)
void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
void set_ally_mcu_powersave(bool enabled);
int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval);
int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
+
+int asus_hid_register_listener(struct asus_hid_listener *cdev);
+void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
#else
static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
{
@@ -184,6 +193,14 @@ static inline int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1,
{
return -ENODEV;
}
+
+static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
+{
+ return -ENODEV;
+}
+static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
+{
+}
#endif
/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
--
2.52.0
^ permalink raw reply related
* [PATCH v9 05/11] HID: asus: move vendor initialization to probe
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
ROG NKEY devices have multiple HID endpoints, around 3-4. One of those
endpoints has a usage page of 0xff31, and is the one that emits keyboard
shortcuts and controls RGB/backlight. Currently, this driver places
the usage page check under asus_input_mapping and then inits backlight
in asus_input_configured which is unnecessarily complicated and prevents
probe from performing customizations on the vendor endpoint.
Simplify the logic by introducing an is_vendor variable into probe that
checks for usage page 0xff31. Then, use this variable to move backlight
initialization into probe instead of asus_input_configured, and remove
the backlight check from asus_input_mapping.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 5149dc7edfc5..3047bc54bf2e 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define T100CHI_MOUSE_REPORT_ID 0x06
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
+#define HID_USAGE_PAGE_VENDOR 0xff310000
#define FEATURE_KBD_REPORT_ID 0x5a
#define FEATURE_KBD_REPORT_SIZE 64
#define FEATURE_KBD_LED_REPORT_ID1 0x5d
@@ -126,7 +127,6 @@ struct asus_drvdata {
struct input_dev *tp_kbd_input;
struct asus_kbd_leds *kbd_backlight;
const struct asus_touchpad_info *tp;
- bool enable_backlight;
struct power_supply *battery;
struct power_supply_desc battery_desc;
int battery_capacity;
@@ -317,7 +317,7 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
static int asus_event(struct hid_device *hdev, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
- if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
+ if ((usage->hid & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR &&
(usage->hid & HID_USAGE) != 0x00 &&
(usage->hid & HID_USAGE) != 0xff && !usage->type) {
hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
@@ -942,11 +942,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
drvdata->input = input;
- if (drvdata->enable_backlight &&
- !asus_kbd_wmi_led_control_present(hdev) &&
- asus_kbd_register_leds(hdev))
- hid_warn(hdev, "Failed to initialize backlight.\n");
-
return 0;
}
@@ -1019,15 +1014,6 @@ static int asus_input_mapping(struct hid_device *hdev,
return -1;
}
- /*
- * Check and enable backlight only on devices with UsagePage ==
- * 0xff31 to avoid initializing the keyboard firmware multiple
- * times on devices with multiple HID descriptors but same
- * PID/VID.
- */
- if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
- drvdata->enable_backlight = true;
-
set_bit(EV_REP, hi->input->evbit);
return 1;
}
@@ -1144,8 +1130,11 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
- int ret;
+ struct hid_report_enum *rep_enum;
struct asus_drvdata *drvdata;
+ struct hid_report *rep;
+ bool is_vendor = false;
+ int ret;
drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
if (drvdata == NULL) {
@@ -1229,12 +1218,24 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
+ /* Check for vendor for RGB init and handle generic devices properly. */
+ rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
+ list_for_each_entry(rep, &rep_enum->report_list, list) {
+ if ((rep->application & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR)
+ is_vendor = true;
+ }
+
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret) {
hid_err(hdev, "Asus hw start failed: %d\n", ret);
return ret;
}
+ 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");
+
/*
* Check that input registration succeeded. Checking that
* HID_CLAIMED_INPUT is set prevents a UAF when all input devices
--
2.52.0
^ permalink raw reply related
* [PATCH v9 10/11] platform/x86: asus-wmi: add keyboard brightness event handler
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
The keyboard brightness control of Asus WMI keyboards is handled in
kernel, which leads to the shortcut going from brightness 0, to 1,
to 2, and 3.
However, for HID keyboards it is exposed as a key and handled by the
user's desktop environment. For the toggle button, this means that
brightness control becomes on/off. In addition, in the absence of a
DE, the keyboard brightness does not work.
Therefore, expose an event handler for the keyboard brightness control
which can then be used by hid-asus. Since this handler is called from
an interrupt context, defer the actual work to a workqueue.
In the process, introduce ASUS_EV_MAX_BRIGHTNESS to hold the constant
for maximum brightness since it is shared between hid-asus/asus-wmi.
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Tested-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/platform/x86/asus-wmi.c | 46 +++++++++++++++++++---
include/linux/platform_data/x86/asus-wmi.h | 13 ++++++
2 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 5f23aedbf34f..a7482c97cc5b 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1628,6 +1628,44 @@ static void kbd_led_update_all(struct work_struct *work)
}
}
+/*
+ * This function is called from hid-asus to inform asus-wmi of brightness
+ * changes initiated by the keyboard backlight keys.
+ */
+int asus_hid_event(enum asus_hid_event event)
+{
+ struct asus_wmi *asus;
+ int brightness;
+
+ guard(spinlock_irqsave)(&asus_ref.lock);
+ asus = asus_ref.asus;
+ if (!asus || !asus->kbd_led_registered)
+ return -EBUSY;
+
+ brightness = asus->kbd_led_wk;
+
+ switch (event) {
+ case ASUS_EV_BRTUP:
+ brightness += 1;
+ break;
+ case ASUS_EV_BRTDOWN:
+ brightness -= 1;
+ break;
+ case ASUS_EV_BRTTOGGLE:
+ if (brightness >= ASUS_EV_MAX_BRIGHTNESS)
+ brightness = 0;
+ else
+ brightness += 1;
+ break;
+ }
+
+ asus->kbd_led_wk = clamp_val(brightness, 0, ASUS_EV_MAX_BRIGHTNESS);
+ asus->kbd_led_notify = true;
+ queue_work(asus->led_workqueue, &asus->kbd_led_work);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(asus_hid_event);
+
/*
* These functions actually update the LED's, and are called from a
* workqueue. By doing this as separate work rather than when the LED
@@ -1710,13 +1748,11 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
{
struct asus_hid_listener *listener;
struct asus_wmi *asus;
- int max_level;
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
- max_level = asus->kbd_led.max_brightness;
scoped_guard(spinlock_irqsave, &asus_ref.lock)
- asus->kbd_led_wk = clamp_val(value, 0, max_level);
+ asus->kbd_led_wk = clamp_val(value, 0, ASUS_EV_MAX_BRIGHTNESS);
if (asus->kbd_led_avail)
kbd_led_update(asus);
@@ -1920,7 +1956,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
asus->kbd_led.brightness_set = kbd_led_set;
asus->kbd_led.brightness_get = kbd_led_get;
- asus->kbd_led.max_brightness = 3;
+ asus->kbd_led.max_brightness = ASUS_EV_MAX_BRIGHTNESS;
asus->kbd_led_avail = !kbd_led_read(asus, &led_val, NULL);
INIT_WORK(&asus->kbd_led_work, kbd_led_update_all);
@@ -4423,7 +4459,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
if (code == NOTIFY_KBD_BRTTOGGLE) {
- if (led_value == asus->kbd_led.max_brightness)
+ if (led_value == ASUS_EV_MAX_BRIGHTNESS)
kbd_led_set_by_kbd(asus, 0);
else
kbd_led_set_by_kbd(asus, led_value + 1);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index d8c5269854b0..3f679598b629 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -169,6 +169,14 @@ struct asus_hid_listener {
void (*brightness_set)(struct asus_hid_listener *listener, int brightness);
};
+enum asus_hid_event {
+ ASUS_EV_BRTUP,
+ ASUS_EV_BRTDOWN,
+ ASUS_EV_BRTTOGGLE,
+};
+
+#define ASUS_EV_MAX_BRIGHTNESS 3
+
#if IS_REACHABLE(CONFIG_ASUS_WMI)
void set_ally_mcu_hack(enum asus_ally_mcu_hack status);
void set_ally_mcu_powersave(bool enabled);
@@ -177,6 +185,7 @@ int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval);
int asus_hid_register_listener(struct asus_hid_listener *cdev);
void asus_hid_unregister_listener(struct asus_hid_listener *cdev);
+int asus_hid_event(enum asus_hid_event event);
#else
static inline void set_ally_mcu_hack(enum asus_ally_mcu_hack status)
{
@@ -201,6 +210,10 @@ static inline int asus_hid_register_listener(struct asus_hid_listener *bdev)
static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
{
}
+static inline int asus_hid_event(enum asus_hid_event event)
+{
+ return -ENODEV;
+}
#endif
#endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
--
2.52.0
^ permalink raw reply related
* [PATCH v9 09/11] platform/x86: asus-wmi: remove unused keyboard backlight quirk
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
The quirk for selecting whether keyboard backlight should be controlled
by HID or WMI is not needed anymore, so remove it.
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
include/linux/platform_data/x86/asus-wmi.h | 40 ----------------------
1 file changed, 40 deletions(-)
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 1165039013b1..d8c5269854b0 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -203,44 +203,4 @@ static inline void asus_hid_unregister_listener(struct asus_hid_listener *bdev)
}
#endif
-/* To be used by both hid-asus and asus-wmi to determine which controls kbd_brightness */
-static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Zephyrus"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Strix"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Flow"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_PRODUCT_FAMILY, "ProArt P16"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_BOARD_NAME, "GA403U"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_BOARD_NAME, "GU605M"),
- },
- },
- {
- .matches = {
- DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
- },
- },
- { },
-};
-
#endif /* __PLATFORM_DATA_X86_ASUS_WMI_H */
--
2.52.0
^ permalink raw reply related
* [PATCH v9 03/11] HID: asus: use same report_id in response
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
Currently, asus_kbd_get_functions prods the device using feature
report report_id, but then is hardcoded to check the response through
FEATURE_KBD_REPORT_ID. This only works if report_id is that value
(currently true). So, use report_id in the response as well to
maintain functionality if that value changes in the future.
Reviewed-by: Denis Benato <benato.denis96@gmail.com>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index db2cc9922447..6de402d215d0 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -423,7 +423,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
if (!readbuf)
return -ENOMEM;
- ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
+ ret = hid_hw_raw_request(hdev, report_id, readbuf,
FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
HID_REQ_GET_REPORT);
if (ret < 0) {
--
2.52.0
^ permalink raw reply related
* [PATCH v9 02/11] HID: asus: initialize additional endpoints only for legacy devices
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
Currently, ID1/ID2 initializations are performed for all NKEY devices.
However, ID1 initializations are only required for RGB control and are
only supported for RGB capable devices. ID2 initializations are only
required for initializing the Anime display endpoint which is only
supported on devices with an Anime display. Both of these
initializations are out of scope for this driver (this is a brightness
control and keyboard shortcut driver) and they should not be performed
for devices that do not support them in any case.
At the same time, there are older NKEY devices that have only been
tested with these initializations in the kernel and it is not possible
to recheck them. There is a possibility that especially with the ID1
initialization, certain laptop models might have their shortcuts stop
working (currently unproven).
For an abundance of caution, only initialize ID1/ID2 for those older
NKEY devices by introducing a quirk for them and replacing the NKEY
quirk in the block that performs the inits with that.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 415d067938e3..db2cc9922447 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -89,6 +89,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
#define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
#define QUIRK_ROG_ALLY_XPAD BIT(13)
+#define QUIRK_ROG_NKEY_LEGACY BIT(14)
#define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
QUIRK_NO_INIT_REPORTS | \
@@ -651,7 +652,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
return -ENODEV;
- if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
+ if (drvdata->quirks & QUIRK_ROG_NKEY_LEGACY) {
ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
if (ret < 0)
return ret;
@@ -1375,10 +1376,10 @@ static const struct hid_device_id asus_devices[] = {
QUIRK_USE_KBD_BACKLIGHT },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
- QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+ QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2),
- QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
+ QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_LEGACY },
{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
--
2.52.0
^ permalink raw reply related
* [PATCH v9 08/11] HID: asus: listen to the asus-wmi brightness device instead of creating one
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
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.
Therefore, register a listener to the asus-wmi brightness device
instead of creating a new one.
Reviewed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Antheas Kapenekakis <lkml@antheas.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 6193c9483bec..6a355c174f29 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;
struct hid_device *hdev;
struct work_struct work;
unsigned int brightness;
@@ -494,11 +494,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);
@@ -508,20 +508,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);
@@ -538,34 +524,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
@@ -710,14 +668,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);
@@ -1106,7 +1061,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);
@@ -1232,7 +1187,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");
@@ -1279,6 +1233,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);
--
2.52.0
^ permalink raw reply related
* [PATCH v9 01/11] HID: asus: simplify RGB init sequence
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
Currently, RGB initialization forks depending on whether a device is
NKEY. However, in reality both initialization forks are the same, other
than the NKEY initialization initializing the LED_REPORT_ID1,
LED_REPORT_ID2 endpoints, and the non-NKEY initialization having a
functionality check which is skipped for the NKEY path.
Therefore, merge the if blocks, gate the ID1/ID2 initializations
behind the NKEY quirk instead, and introduce the functionality check
for NKEY devices (it is supported by them).
There should be no functional change with this patch.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 52 ++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 30 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index a444d41e53b6..415d067938e3 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -638,13 +638,20 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
unsigned char kbd_func;
int ret;
- if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
- /* Initialize keyboard */
- ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
- if (ret < 0)
- return ret;
+ ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
+ if (ret < 0)
+ return ret;
- /* The LED endpoint is initialised in two HID */
+ /* Get keyboard functions */
+ ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
+ if (ret < 0)
+ return ret;
+
+ /* Check for backlight support */
+ if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
+ return -ENODEV;
+
+ if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
if (ret < 0)
return ret;
@@ -652,34 +659,19 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
if (ret < 0)
return ret;
+ }
- if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
- ret = asus_kbd_disable_oobe(hdev);
- if (ret < 0)
- return ret;
- }
-
- if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
- intf = to_usb_interface(hdev->dev.parent);
- udev = interface_to_usbdev(intf);
- validate_mcu_fw_version(hdev,
- le16_to_cpu(udev->descriptor.idProduct));
- }
-
- } else {
- /* Initialize keyboard */
- ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
- if (ret < 0)
- return ret;
-
- /* Get keyboard functions */
- ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
+ if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
+ ret = asus_kbd_disable_oobe(hdev);
if (ret < 0)
return ret;
+ }
- /* Check for backlight support */
- if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
- return -ENODEV;
+ if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
+ intf = to_usb_interface(hdev->dev.parent);
+ udev = interface_to_usbdev(intf);
+ validate_mcu_fw_version(
+ hdev, le16_to_cpu(udev->descriptor.idProduct));
}
drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
--
2.52.0
^ permalink raw reply related
* [PATCH v9 06/11] HID: asus: early return for ROG devices
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
Some ROG devices have a new dynamic backlight interface for control by
Windows. This interface does not create an ->input device, causing the
kernel to print an error message and to eject it. In addition, ROG
devices have proper HID names in their descriptors so renaming them is
not necessary.
Therefore, if a device is identified as ROG, early return from probe to
skip renaming and ->input checks.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 3047bc54bf2e..6193c9483bec 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1236,6 +1236,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
asus_kbd_register_leds(hdev))
hid_warn(hdev, "Failed to initialize backlight.\n");
+ /*
+ * For ROG keyboards, skip rename for consistency and ->input check as
+ * some devices do not have inputs.
+ */
+ if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
+ return 0;
+
/*
* Check that input registration succeeded. Checking that
* HID_CLAIMED_INPUT is set prevents a UAF when all input devices
--
2.52.0
^ permalink raw reply related
* [PATCH v9 04/11] HID: asus: fortify keyboard handshake
From: Antheas Kapenekakis @ 2025-11-20 9:46 UTC (permalink / raw)
To: platform-driver-x86, linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Corentin Chary,
Luke D . Jones, Hans de Goede, Ilpo Järvinen, Denis Benato,
Antheas Kapenekakis
In-Reply-To: <20251120094617.11672-1-lkml@antheas.dev>
Handshaking with an Asus device involves sending it a feature report
with the string "ASUS Tech.Inc." and then reading it back to verify the
handshake was successful, under the feature ID the interaction will
take place.
Currently, the driver only does the first part. Add the readback to
verify the handshake was successful. As this could cause breakages,
allow the verification to fail with a dmesg error until we verify
all devices work with it (they seem to).
Since the response is more than 16 bytes, increase the buffer size
to 64 as well to avoid overflow errors.
Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
---
drivers/hid/hid-asus.c | 32 +++++++++++++++++++++++++++++---
1 file changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 6de402d215d0..5149dc7edfc5 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
#define FEATURE_REPORT_ID 0x0d
#define INPUT_REPORT_ID 0x5d
#define FEATURE_KBD_REPORT_ID 0x5a
-#define FEATURE_KBD_REPORT_SIZE 16
+#define FEATURE_KBD_REPORT_SIZE 64
#define FEATURE_KBD_LED_REPORT_ID1 0x5d
#define FEATURE_KBD_LED_REPORT_ID2 0x5e
@@ -394,14 +394,40 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
{
+ /*
+ * The handshake is first sent as a set_report, then retrieved
+ * from a get_report. They should be equal.
+ */
const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+ u8 *readbuf;
int ret;
ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
- if (ret < 0)
- hid_err(hdev, "Asus failed to send init command: %d\n", ret);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to send handshake: %d\n", ret);
+ return ret;
+ }
+
+ readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
+ if (!readbuf)
+ return -ENOMEM;
+
+ ret = hid_hw_raw_request(hdev, report_id, readbuf,
+ FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
+ HID_REQ_GET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret);
+ } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) {
+ hid_warn(hdev, "Asus handshake returned invalid response: %*ph\n",
+ FEATURE_KBD_REPORT_SIZE, readbuf);
+ /*
+ * Do not return error if handshake is wrong until this is
+ * verified to work for all devices.
+ */
+ }
+ kfree(readbuf);
return ret;
}
--
2.52.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox