* Re: [PATCH v1] HID: Fix Report Descriptor for Evision Wireless Receiver 320f:226f
From: Artem @ 2025-11-21 22:58 UTC (permalink / raw)
To: Terry Junge; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20251120014931.580340-1-linuxhid@cosmicgizmosystems.com>
Hello Terry,
I have successfully compiled and tested the patch.
It works perfectly! The side buttons (Forward/Back) are now correctly
detected and functioning immediately after loading the patched module.
I haven't encountered any issues afterwards, other functionality is
unchanged.
dmesg confirms the fix is applied:
[10307.732966] evision 0003:320F:226F.0009: fixing EVision:TeLink
Receiver report descriptor
Thank you very much for your quick help and the fix.
Tested-by: Artem <temabiill@gmail.com>
Best regards,
Btema2
^ permalink raw reply
* Keycodes for Factory reset button and USB Copy Button
From: Markus Probst @ 2025-11-21 21:09 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
Hi Dmitry,
I am currently writing a "multi function device" driver for synology
NAS devices. They have an on-board chip that handles things like, leds,
fan speed, buzzer, proper shutdown/restart and button presses:
- Power button for shutdown
- Reset button for triggering a factory reset
- USB Copy button for transfering files between a usb stick (not
available on all devices).
With the exception of the Power button (KEY_POWER), I am not sure which
keycodes I should use for the buttons mentioned above in the driver.
What would you recommend?
Thanks
- Markus Probst
^ permalink raw reply
* Re: [PATCH v3 1/4] firmware_loader: expand firmware error codes with up-to-date error
From: Marco Felsch @ 2025-11-21 14:00 UTC (permalink / raw)
To: Konstantin Ryabitsev
Cc: Russ Weight, Rob Herring, Conor Dooley, Henrik Rydberg,
Kamel Bouhara, Rafael J. Wysocki, devicetree, Greg Kroah-Hartman,
Dmitry Torokhov, linux-kernel, Luis Chamberlain, Marco Felsch,
linux-input, Andrew Morton, Krzysztof Kozlowski, Danilo Krummrich
In-Reply-To: <20251119-elated-caribou-of-witchcraft-0508ed@lemur>
Hi Russ,
On 25-11-19, Konstantin Ryabitsev wrote:
> On Wed, Nov 19, 2025 at 11:10:07AM +0100, Marco Felsch wrote:
> > > Marco - I would recommend adding the Reviewed-by tags that you
> > > have received and then resubmitting the patch.
> >
> > I can do this albeit I thought this will be collected autom. by b4.
>
> You have to run `b4 trailers -u` for it to be collected.
Is this okay to run it on your site or do I have to add and resend it?
Regards,
Marco
>
> -K
>
--
#gernperDu
#CallMeByMyFirstName
Pengutronix e.K. | |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
^ permalink raw reply
* [PATCH v5 2/2] HID: input: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-21 13:15 UTC (permalink / raw)
To: linux-input
Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
Sebastian Reichel, Bastien Nocera, linux-pm
In-Reply-To: <20251121131556.601130-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 struct hid_battery replaces the legacy dev->battery_* fields with
a batteries list.
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.
Suggested-by: Benjamin Tissoires <bentiss@kernel.org>
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 | 163 +++++++++++++++++++++--------------
drivers/hid/hid-magicmouse.c | 10 ++-
include/linux/hid.h | 54 +++++++++---
6 files changed, 178 insertions(+), 102 deletions(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 61404d7a43ee..fb09b616f8cc 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -618,17 +618,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 a5b3a8ca2fcb..76d628547e9a 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 6f5c71660d82..c92008dafddf 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 5f313c3c35e2..9eeaba0229d5 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -418,18 +418,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;
@@ -438,14 +438,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;
}
@@ -454,7 +454,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;
@@ -465,13 +466,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;
@@ -482,20 +483,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:
@@ -510,33 +511,52 @@ 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;
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;
+ 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 = devm_kasprintf(&dev->dev, GFP_KERNEL, "hid-%s-battery",
+ psy_desc->name = devm_kasprintf(&dev->dev, GFP_KERNEL,
+ "hid-%s-battery-%d",
strlen(dev->uniq) ?
- dev->uniq : dev_name(&dev->dev));
+ dev->uniq : dev_name(&dev->dev),
+ field->report->id);
if (!psy_desc->name)
return -ENOMEM;
@@ -557,77 +577,80 @@ 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;
+ bat->avoid_query = true;
- dev->battery = devm_power_supply_register(&dev->dev, psy_desc, &psy_cfg);
- if (IS_ERR(dev->battery)) {
+ 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(dev->battery));
- return PTR_ERR(dev->battery);
+ PTR_ERR(bat->ps));
+ return PTR_ERR(bat->ps);
}
- power_supply_powers(dev->battery, &dev->dev);
+ power_supply_powers(bat->ps, &dev->dev);
+
+ list_add_tail(&bat->list, &dev->batteries);
+
return 0;
}
-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 */
@@ -637,8 +660,14 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
return 0;
}
-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 */
@@ -1506,7 +1535,11 @@ 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 7d4a25c6de0e..b495f7a4bc6c 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 a4ddb94e3ee5..3e33ef74c3c1 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.1
^ permalink raw reply related
* [PATCH v5 0/2] HID: Add support for multiple batteries per device
From: Lucas Zampieri @ 2025-11-21 13:15 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 and named as hid-{uniq}-battery-{id}. The
implementation is fully backwards compatible with single-battery devices
through a helper function. The series first converts the battery code to
devm_* as preparatory cleanup, which simplifies the subsequent refactoring
and reduces risk of memory management bugs.
## 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 (2):
HID: input: Convert battery code to devm_*
HID: Refactor battery code to use struct hid_battery and add
multi-battery support
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
Changes in v5:
- Split the monolithic v4 patch into two logical patches as suggested by
Benjamin, devm_* conversion, then struct refactor and multi-battery support
combined
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, 180 insertions(+), 133 deletions(-)
base-commit: 8b690556d8fe0ee15151cc37ec49c5bbfe41d5b1
--
2.51.1
^ permalink raw reply
* [PATCH v5 1/2] HID: input: Convert battery code to devm_*
From: Lucas Zampieri @ 2025-11-21 13:15 UTC (permalink / raw)
To: linux-input
Cc: Lucas Zampieri, linux-kernel, Jiri Kosina, Benjamin Tissoires,
Sebastian Reichel, Bastien Nocera, linux-pm
In-Reply-To: <20251121131556.601130-1-lzampier@redhat.com>
Convert the HID battery code to use devm_* managed resource APIs.
This changes the following allocations:
- kzalloc() -> devm_kzalloc() for power_supply_desc
- kasprintf() -> devm_kasprintf() for battery name
- power_supply_register() -> devm_power_supply_register()
No functional behavior changes.
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
drivers/hid/hid-input.c | 49 +++++++++--------------------------------
1 file changed, 10 insertions(+), 39 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index e56e7de53279..5f313c3c35e2 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -530,17 +530,15 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
if (quirks & HID_BATTERY_QUIRK_IGNORE)
return 0;
- psy_desc = kzalloc(sizeof(*psy_desc), GFP_KERNEL);
+ 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",
+ strlen(dev->uniq) ?
+ dev->uniq : dev_name(&dev->dev));
+ if (!psy_desc->name)
+ return -ENOMEM;
psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
psy_desc->properties = hidinput_battery_props;
@@ -576,36 +574,15 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
if (quirks & HID_BATTERY_QUIRK_AVOID_QUERY)
dev->battery_avoid_query = true;
- dev->battery = power_supply_register(&dev->dev, psy_desc, &psy_cfg);
+ dev->battery = devm_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;
+ hid_warn(dev, "can't register power supply: %ld\n",
+ PTR_ERR(dev->battery));
+ return PTR_ERR(dev->battery);
}
power_supply_powers(dev->battery, &dev->dev);
return 0;
-
-err_free_name:
- kfree(psy_desc->name);
-err_free_mem:
- kfree(psy_desc);
- dev->battery = NULL;
- return error;
-}
-
-static void hidinput_cleanup_battery(struct hid_device *dev)
-{
- const struct power_supply_desc *psy_desc;
-
- if (!dev->battery)
- return;
-
- psy_desc = dev->battery->desc;
- power_supply_unregister(dev->battery);
- kfree(psy_desc->name);
- kfree(psy_desc);
- dev->battery = NULL;
}
static bool hidinput_update_battery_charge_status(struct hid_device *dev,
@@ -660,10 +637,6 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
return 0;
}
-static void hidinput_cleanup_battery(struct hid_device *dev)
-{
-}
-
static void hidinput_update_battery(struct hid_device *dev, unsigned int usage,
int value)
{
@@ -2379,8 +2352,6 @@ void hidinput_disconnect(struct hid_device *hid)
{
struct hid_input *hidinput, *next;
- hidinput_cleanup_battery(hid);
-
list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
list_del(&hidinput->list);
if (hidinput->registered)
--
2.51.1
^ permalink raw reply related
* Re: [PATCH v9 04/11] HID: asus: fortify keyboard handshake
From: Ilpo Järvinen @ 2025-11-21 10:11 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: Denis Benato, platform-driver-x86, linux-input, LKML, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede
In-Reply-To: <CAGwozwHSFy6UhbEGBSvYewoFXozd8=MrbpKv5qexeo0yA+4NkQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5797 bytes --]
On Thu, 20 Nov 2025, Antheas Kapenekakis wrote:
> On Thu, 20 Nov 2025 at 17:41, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Thu, 20 Nov 2025, Antheas Kapenekakis wrote:
> >
> > > On Thu, 20 Nov 2025 at 15:15, Denis Benato <benato.denis96@gmail.com> wrote:
> > > >
> > > >
> > > > 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,
> >
> > It's also about always doing the right thing so others will pick up the
> > pattern (for the cases when it's needed).
> >
> > > > 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.
> > >
> > > It was ambiguous. I interpreted Ilpo's email as a dismissal
> >
> > I tried to explain how to use it, not to suggest cleanup.h shouldn't be
> > used.
>
> Ok, I'll wait a few days and do another revision, doing some rewording
> as well. Hopefully that will cover everything. To that extent, try to
> finish reviewing the latter part of the series before that revision.
>
> I'm a bit concerned with introducing kfree here because I do not know
> how to use it and it might regress, but it should be ok.
You probably meant to say __free(), there are other things that could be
put inside __free() beyond kfree (no need to ack if that was the case).
It's not rocket science, basically the compiler performs the freeing
function when the scope the variable is in goes away. For error handling
rollback, this avoids need to do the cleanup call manually and no path is
left accidently without cleanup.
For cases, where you want to preserve a pointer to the allocated thing,
there are additional wrappers that one uses when returning the pointer
out of function (only gotcha here is that it will NULL the local
variable in question, so the variable cannot be dereferenced after that
point; been there, done that and had to debug a boot issue :-)).
> I'd rather push the init down instead of pulling it up. Referencing
> other code samples for kfree it is acceptable to push the variable
> definition down, right?
Yes.
Mid-function definitions used to be forbidding on the compiler level
but it was changed exactly because adding this auto cleanup framework. So
while top definition is still the general requirement, using __free() is an
exception to that rule and should be defined at the spot where we make
the "alloc" to ensure the teardown order is exactly reverse of the alloc
order (the order you introduce variables is the reverse of the order in
which the compiler will perform each tear down).
--
i.
^ permalink raw reply
* [PATCH 3/3] HID: multitouch: set INPUT_PROP_PRESSUREPAD based on Digitizer/Button Type
From: Peter Hutterer @ 2025-11-21 1:07 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Dmitry Torokhov, linux-input, linux-kselftest, linux-kernel,
Vadim Klishko, Peter Hutterer
In-Reply-To: <20251121-wip-hid-pressurepad-v1-0-e32e5565a527@who-t.net>
A Digitizer/Button Type value of 1 indicates the device is a
pressurepad, see
https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-capabilities-feature-report
For the selftests we have to resort to a bit of a hack: python-libevdev
gets the properties from libevdev at module init time. If libevdev
hasn't been rebuilt with the new property it won't be automatically
populated. So we hack around this by constructing the property manually.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
drivers/hid/hid-multitouch.c | 12 ++++++-
.../testing/selftests/hid/tests/test_multitouch.py | 39 +++++++++++++++++++---
2 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 179dc316b4b518d78bdc900d9fd15756c5eba83e..382e6f50c4f7e663af7d028abb8be7cb2e6e7b8e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -81,6 +81,7 @@ MODULE_LICENSE("GPL");
#define MT_INPUTMODE_TOUCHPAD 0x03
#define MT_BUTTONTYPE_CLICKPAD 0
+#define MT_BUTTONTYPE_PRESSUREPAD 1
enum latency_mode {
HID_LATENCY_NORMAL = 0,
@@ -179,6 +180,7 @@ struct mt_device {
__u8 inputmode_value; /* InputMode HID feature value */
__u8 maxcontacts;
bool is_buttonpad; /* is this device a button pad? */
+ bool is_pressurepad; /* is this device a pressurepad? */
bool is_haptic_touchpad; /* is this device a haptic touchpad? */
bool serial_maybe; /* need to check for serial protocol */
@@ -530,8 +532,14 @@ static void mt_feature_mapping(struct hid_device *hdev,
}
mt_get_feature(hdev, field->report);
- if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
+ switch (field->value[usage->usage_index]) {
+ case MT_BUTTONTYPE_CLICKPAD:
td->is_buttonpad = true;
+ break;
+ case MT_BUTTONTYPE_PRESSUREPAD:
+ td->is_pressurepad = true;
+ break;
+ }
break;
case 0xff0000c5:
@@ -1393,6 +1401,8 @@ static int mt_touch_input_configured(struct hid_device *hdev,
if (td->is_buttonpad)
__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
+ if (td->is_pressurepad)
+ __set_bit(INPUT_PROP_PRESSUREPAD, input->propbit);
app->pending_palm_slots = devm_kcalloc(&hi->input->dev,
BITS_TO_LONGS(td->maxcontacts),
diff --git a/tools/testing/selftests/hid/tests/test_multitouch.py b/tools/testing/selftests/hid/tests/test_multitouch.py
index a06a087f00b6991f7514adf7f8c713bef1a43563..fa4fb2054bd4febb1d2497f2787944f538b27889 100644
--- a/tools/testing/selftests/hid/tests/test_multitouch.py
+++ b/tools/testing/selftests/hid/tests/test_multitouch.py
@@ -979,15 +979,36 @@ class BaseTest:
assert libevdev.InputEvent(libevdev.EV_ABS.ABS_MT_ORIENTATION, 90) in events
class TestPTP(TestWin8Multitouch):
+ def test_buttontype(self):
+ """Check for the right ButtonType."""
+ uhdev = self.uhdev
+ assert uhdev is not None
+ evdev = uhdev.get_evdev()
+
+ # If libevdev.so is not yet compiled with INPUT_PROP_PRESSUREPAD
+ # python-libevdev won't have it either, let's fake it
+ if not getattr(libevdev, "INPUT_PROP_PRESSUREPAD", None):
+ prop = libevdev.InputProperty(name="INPUT_PROP_PRESSUREPAD", value=0x7)
+ libevdev.INPUT_PROP_PRESSUREPAD = prop
+ libevdev.props.append(prop)
+
+ if uhdev.buttontype == HIDButtonType.CLICKPAD:
+ assert libevdev.INPUT_PROP_BUTTONPAD in evdev.properties
+ elif uhdev.buttontype == HIDButtonType.PRESSUREPAD:
+ assert libevdev.INPUT_PROP_PRESSUREPAD in evdev.properties
+ else:
+ assert libevdev.INPUT_PROP_PRESSUREPAD not in evdev.properties
+ assert libevdev.INPUT_PROP_BUTTONPAD not in evdev.properties
+
def test_ptp_buttons(self):
"""check for button reliability.
- There are 2 types of touchpads: the click pads and the pressure pads.
- Each should reliably report the BTN_LEFT events.
+ There are 3 types of touchpads: click pads + pressure pads and
+ those with discrete buttons. Each should reliably report the BTN_LEFT events.
"""
uhdev = self.uhdev
evdev = uhdev.get_evdev()
- if uhdev.buttontype == HIDButtonType.CLICKPAD:
+ if uhdev.buttontype in [HIDButtonType.CLICKPAD, HIDButtonType.PRESSUREPAD]:
r = uhdev.event(click=True)
events = uhdev.next_sync_events()
self.debug_reports(r, uhdev, events)
@@ -999,7 +1020,7 @@ class BaseTest:
self.debug_reports(r, uhdev, events)
assert libevdev.InputEvent(libevdev.EV_KEY.BTN_LEFT, 0) in events
assert evdev.value[libevdev.EV_KEY.BTN_LEFT] == 0
- else:
+ elif uhdev.buttontype == HIDButtonType.DISCRETE_BUTTONS:
r = uhdev.event(left=True)
events = uhdev.next_sync_events()
self.debug_reports(r, uhdev, events)
@@ -2062,6 +2083,16 @@ class Testite_06cb_2968(BaseTest.TestPTP):
)
+class Testven_0488_108c(BaseTest.TestPTP):
+ def create_device(self):
+ return PTP(
+ "uhid test ven_0488_108c",
+ rdesc="05 01 09 02 a1 01 85 06 09 01 a1 00 05 09 19 01 29
03 15 00 25 01 95 03 75 01 81 02 95 01 75 05 81 03 05 01 09 30 09 31 09
38 15 81 25 7f 75 08 95 03 81 06 c0 c0 05 0d 09 05 a1 01 85 01 05 0d 09
22 a1 02 15 00 25 01 09 47 09 42 95 02 75 01 81 02 95 01 75 03 25 05 09
51 81 02 81 03 05 01 15 00 26 ba 0d 75 10 55 0e 65 11 09 30 35 00 46 d0
05 95 01 81 02 26 d0 06 46 bb 02 09 31 81 02 05 0d 95 01 75 10 26 ff 7f
46 ff 7f 09 30 81 02 c0 05 0d 09 22 a1 02 15 00 25 01 09 47 09 42 95 02
75 01 81 02 95 01 75 03 25 05 09 51 81 02 81 03 05 01 15 00 26 ba 0d 75
10 55 0e 65 11 09 30 35 00 46 d0 05 95 01 81 02 26 d0 06 46 bb 02 09 31
81 02 05 0d 95 01 75 10 26 ff 7f 46 ff 7f 09 30 81 02 c0 05 0d 09 22 a1
02 15 00 25 01 09 47 09 42 95 02 75 01 81 02 95 01 75 03 25 05 09 51 81
02 81 03 05 01 15 00 26 ba 0d 75 10 55 0e 65 11 09 30 35 00 46 d0 05 95
01 81 02 26 d0 06 46 bb 02 09 31 81 02 05 0d 95 01 75 10 26 ff 7f 46 ff
7f 09 30 81 02 c0 55 0c 66 01 10 47 ff ff 00 00 27 ff ff 00 00 75 10 95
01 05 0d 09 56 81 02 09 54 25 05 95 01 75 08 81 02 05 09 09 01 25 01 75
01 95 01 81 02 95 07 81 03 05 0d 85 02 09 55 75 08 95 01 25 05 b1 02 09
59 b1 02 06 00 ff 85 03 09 c5 15 00 26 ff 00 75 08 96 00 01 b1 02 05 0e
09 01 a1 02 85 13 09 23 15 00 25 64 75 08 95 01 b1 02 c0 c0 05 0d 09 0e
a1 01 85 04 09 22 a1 02 09 52 15 00 25 0a 75 08 95 01 b1 02 c0 09 22 a1
00 85 05 09 57 09 58 75 01 95 02 25 01 b1 02 95 06 b1 03 c0 c0 06 01 ff
09 02 a1 01 09 00 85 07 15 00 26 ff 00 75 08 96 12 02 b1 02 c0 06 00 ff
09 01 a1 01 85 0d 15 00 26 ff 00 75 08 95 11 09 01 81 02 09 01 91 02 c0
05 0e 09 01 a1 01 85 11 09 35 15 00 26 ff 00 75 08 95 17 b1 02 c0 06 81
ff 09 01 a1 01 09 20 85 17 15 00 26 ff 00 75 08 95 3f 09 01 81 02 09 01
91 02 c0",
+ input_info=(0x18, 0x0488, 0x108C),
+ buttontype=HIDButtonType.PRESSUREPAD,
+ )
+
+
class Testn_trig_1b96_0c01(BaseTest.TestWin8Multitouch):
def create_device(self):
return Digitizer(
--
2.51.1
^ permalink raw reply related
* [PATCH 2/3] selftests/hid: use a enum class for the different button types
From: Peter Hutterer @ 2025-11-21 1:07 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Dmitry Torokhov, linux-input, linux-kselftest, linux-kernel,
Vadim Klishko, Peter Hutterer
In-Reply-To: <20251121-wip-hid-pressurepad-v1-0-e32e5565a527@who-t.net>
Instead of multiple spellings of a string-provided argument, let's make
this a tad more type-safe and use an enum here.
And while we do this fix the two wrong devices:
- elan_04f3_313a (HP ZBook Fury 15) is discrete button pad
- dell_044e_1220 (Dell Precision 7740) is a discrete button pad
Equivalent hid-tools commit
https://gitlab.freedesktop.org/libevdev/hid-tools/-/commit/8300a55bf4213c6a252cab8cb5b34c9ddb191625
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
.../testing/selftests/hid/tests/test_multitouch.py | 24 +++++++++++++---------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_multitouch.py b/tools/testing/selftests/hid/tests/test_multitouch.py
index ece0ba8e7d34b75d42245e5936ecf804c46b0846..a06a087f00b6991f7514adf7f8c713bef1a43563 100644
--- a/tools/testing/selftests/hid/tests/test_multitouch.py
+++ b/tools/testing/selftests/hid/tests/test_multitouch.py
@@ -9,6 +9,7 @@
from . import base
from hidtools.hut import HUT
from hidtools.util import BusType
+import enum
import libevdev
import logging
import pytest
@@ -232,11 +233,17 @@ class Digitizer(base.UHIDTestDevice):
return 0
+class HIDButtonType(enum.IntEnum):
+ CLICKPAD = 0
+ PRESSUREPAD = 1
+ DISCRETE_BUTTONS = 2
+
+
class PTP(Digitizer):
def __init__(
self,
name,
- type="Click Pad",
+ buttontype=HIDButtonType.CLICKPAD,
rdesc_str=None,
rdesc=None,
application="Touch Pad",
@@ -244,11 +251,8 @@ class PTP(Digitizer):
max_contacts=None,
input_info=None,
):
- self.type = type.lower().replace(" ", "")
- if self.type == "clickpad":
- self.buttontype = 0
- else: # pressurepad
- self.buttontype = 1
+ self.buttontype = buttontype
+
self.clickpad_state = False
self.left_state = False
self.right_state = False
@@ -983,7 +987,7 @@ class BaseTest:
uhdev = self.uhdev
evdev = uhdev.get_evdev()
- if uhdev.type == "clickpad":
+ if uhdev.buttontype == HIDButtonType.CLICKPAD:
r = uhdev.event(click=True)
events = uhdev.next_sync_events()
self.debug_reports(r, uhdev, events)
@@ -1918,7 +1922,7 @@ class Testdell_044e_1220(BaseTest.TestPTP):
def create_device(self):
return PTP(
"uhid test dell_044e_1220",
- type="pressurepad",
+ buttontype=HIDButtonType.DISCRETE_BUTTONS,
rdesc="05 01 09 02 a1 01 85 01 09 01 a1 00 05 09 19 01 29
03 15 00 25 01 75 01 95 03 81 02 95 05 81 01 05 01 09 30 09 31 15 81 25
7f 75 08 95 02 81 06 09 38 95 01 81 06 05 0c 0a 38 02 81 06 c0 c0 05 0d
09 05 a1 01 85 08 09 22 a1 02 15 00 25 01 09 47 09 42 95 02 75 01 81 02
95 01 75 03 25 05 09 51 81 02 75 01 95 03 81 03 05 01 15 00 26 af 04 75
10 55 0e 65 11 09 30 35 00 46 e8 03 95 01 81 02 26 7b 02 46 12 02 09 31
81 02 c0 55 0c 66 01 10 47 ff ff 00 00 27 ff ff 00 00 75 10 95 01 05 0d
09 56 81 02 09 54 25 05 95 01 75 08 81 02 05 09 19 01 29 03 25 01 75 01
95 03 81 02 95 05 81 03 05 0d 85 09 09 55 75 08 95 01 25 05 b1 02 06 00
ff 85 0a 09 c5 15 00 26 ff 00 75 08 96 00 01 b1 02 c0 06 01 ff 09 01 a1
01 85 03 09 01 15 00 26 ff 00 95 1b 81 02 85 04 09 02 95 50 81 02 85 05
09 03 95 07 b1 02 85 06 09 04 81 02 c0 06 02 ff 09 01 a1 01 85 07 09 02
95 86 75 08 b1 02 c0 05 0d 09 0e a1 01 85 0b 09 22 a1 02 09 52 15 00 25
0a 75 08 95 01 b1 02 c0 09 22 a1 00 85 0c 09 57 09 58 75 01 95 02 25 01
b1 02 95 06 b1 03 c0 c0",
)
@@ -2018,7 +2022,7 @@ class Testelan_04f3_313a(BaseTest.TestPTP):
def create_device(self):
return PTP(
"uhid test elan_04f3_313a",
- type="touchpad",
+ buttontype=HIDButtonType.DISCRETE_BUTTONS,
input_info=(BusType.I2C, 0x04F3, 0x313A),
rdesc="05 01 09 02 a1 01 85 01 09 01 a1 00 05 09 19 01 29
03 15 00 25 01 75 01 95 03 81 02 95 05 81 03 05 01 09 30 09 31 15 81 25
7f 75 08 95 02 81 06 75 08 95 05 81 03 c0 06 00 ff 09 01 85 0e 09 c5 15
00 26 ff 00 75 08 95 04 b1 02 85 0a 09 c6 15 00 26 ff 00 75 08 95 04 b1
02 c0 06 00 ff 09 01 a1 01 85 5c 09 01 95 0b 75 08 81 06 85 0d 09 c5 15
00 26 ff 00 75 08 95 04 b1 02 85 0c 09 c6 96 80 03 75 08 b1 02 85 0b 09
c7 95 82 75 08 b1 02 c0 05 0d 09 05 a1 01 85 04 09 22 a1 02 15 00 25 01
09 47 09 42 95 02 75 01 81 02 05 09 09 02 09 03 15 00 25 01 75 01 95 02
81 02 05 0d 95 01 75 04 25 0f 09 51 81 02 05 01 15 00 26 d7 0e 75 10 55
0d 65 11 09 30 35 00 46 44 2f 95 01 81 02 46 12 16 26 eb 06 26 eb 06 09
31 81 02 05 0d 15 00 25 64 95 03 c0 55 0c 66 01 10 47 ff ff 00 00 27 ff
ff 00 00 75 10 95 01 09 56 81 02 09 54 25 7f 95 01 75 08 81 02 25 01 75
01 95 08 81 03 09 c5 75 08 95 02 81 03 05 0d 85 02 09 55 09 59 75 04 95
02 25 0f b1 02 85 07 09 60 75 01 95 01 15 00 25 01 b1 02 95 0f b1 03 06
00 ff 06 00 ff 85 06 09 c5 15 00 26 ff 00 75 08 96 00 01 b1 02 c0 05 0d
09 0e a1 01 85 03 09 22 a1 00 09 52 15 00 25 0a 75 10 95 01 b1 02 c0 09
22 a1 00 85 05 09 57 09 58 75 01 95 02 25 01 b1 02 95 0e b1 03 c0 c0 05
01 09 02 a1 01 85 2a 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95
03 81 02 95 05 81 03 05 01 09 30 09 31 15 81 25 7f 35 81 45 7f 55 00 65
13 75 08 95 02 81 06 75 08 95 05 81 03 c0 c0",
)
@@ -2110,7 +2114,7 @@ class Testsipodev_0603_0002(BaseTest.TestPTP):
def create_device(self):
return PTP(
"uhid test sipodev_0603_0002",
- type="clickpad",
+ buttontype=HIDButtonType.CLICKPAD,
rdesc="05 01 09 02 a1 01 85 03 09 01 a1 00 05 09 19 01 29
02 25 01 75 01 95 02 81 02 95 06 81 03 05 01 09 30 09 31 15 80 25 7f 75
08 95 02 81 06 c0 c0 05 0d 09 05 a1 01 85 04 09 22 a1 02 15 00 25 01 09
47 09 42 95 02 75 01 81 02 75 01 95 02 81 03 95 01 75 04 25 05 09 51 81
02 05 01 15 00 26 44 0a 75 0c 55 0e 65 11 09 30 35 00 46 ac 03 95 01 81
02 46 fe 01 26 34 05 75 0c 09 31 81 02 05 0d c0 55 0c 66 01 10 47 ff ff
00 00 27 ff ff 00 00 75 10 95 01 09 56 81 02 09 54 25 0a 95 01 75 04 81
02 75 01 95 03 81 03 05 09 09 01 25 01 75 01 95 01 81 02 05 0d 85 0a 09
55 09 59 75 04 95 02 25 0f b1 02 85 0b 09 60 75 01 95 01 15 00 25 01 b1
02 95 07 b1 03 85 09 06 00 ff 09 c5 15 00 26 ff 00 75 08 96 00 01 b1 02
c0 05 0d 09 0e a1 01 85 06 09 22 a1 02 09 52 15 00 25 0a 75 08 95 01 b1
02 c0 09 22 a1 00 85 07 09 57 09 58 75 01 95 02 25 01 b1 02 95 06 b1 03
c0 c0 05 01 09 0c a1 01 85 08 15 00 25 01 09 c6 75 01 95 01 81 06 75 07
81 03 c0 05 01 09 80 a1 01 85 01 15 00 25 01 75 01 0a 81 00 0a 82 00 0a
83 00 95 03 81 06 95 05 81 01 c0 06 0c 00 09 01 a1 01 85 02 25 01 15 00
75 01 0a b5 00 0a b6 00 0a b7 00 0a cd 00 0a e2 00 0a a2 00 0a e9 00 0a
ea 00 95 08 81 02 0a 83 01 0a 6f 00 0a 70 00 0a 88 01 0a 8a 01 0a 92 01
0a a8 02 0a 24 02 95 08 81 02 0a 21 02 0a 23 02 0a 96 01 0a 25 02 0a 26
02 0a 27 02 0a 23 02 0a b1 02 95 08 81 02 c0 06 00 ff 09 01 a1 01 85 05
15 00 26 ff 00 19 01 29 02 75 08 95 05 b1 02 c0",
)
--
2.51.1
^ permalink raw reply related
* [PATCH 1/3] selftests/hid: require hidtools 0.12
From: Peter Hutterer @ 2025-11-21 1:07 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Dmitry Torokhov, linux-input, linux-kselftest, linux-kernel,
Vadim Klishko, Peter Hutterer
In-Reply-To: <20251121-wip-hid-pressurepad-v1-0-e32e5565a527@who-t.net>
Not all our tests really require it but since it's likely pip-installed
anyway it's trivial to require the new version, just in case we want to
start cleaning up other bits.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
tools/testing/selftests/hid/tests/conftest.py | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/hid/tests/conftest.py b/tools/testing/selftests/hid/tests/conftest.py
index 1361ec981db6f79a58cf91e8732dcd7c05c47d38..985a535324b2fbe322e754e561d7af6898345b27 100644
--- a/tools/testing/selftests/hid/tests/conftest.py
+++ b/tools/testing/selftests/hid/tests/conftest.py
@@ -5,6 +5,7 @@
# Copyright (c) 2017 Benjamin Tissoires <benjamin.tissoires@gmail.com>
# Copyright (c) 2017 Red Hat, Inc.
+from packaging.version import Version
import platform
import pytest
import re
@@ -14,6 +15,19 @@ from .base import HIDTestUdevRule
from pathlib import Path
+@pytest.fixture(autouse=True)
+def hidtools_version_check():
+ HIDTOOLS_VERSION = "0.12"
+ try:
+ import hidtools
+
+ version = hidtools.__version__ # type: ignore
+ if Version(version) < Version(HIDTOOLS_VERSION):
+ pytest.skip(reason=f"have hidtools {version}, require >={HIDTOOLS_VERSION}")
+ except Exception:
+ pytest.skip(reason=f"hidtools >={HIDTOOLS_VERSION} required")
+
+
# See the comment in HIDTestUdevRule, this doesn't set up but it will clean
# up once the last test exited.
@pytest.fixture(autouse=True, scope="session")
--
2.51.1
^ permalink raw reply related
* [PATCH 0/3] HID: multitouch: set INPUT_PROP_PRESSUREPAD on compatible touchpads
From: Peter Hutterer @ 2025-11-21 1:07 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Dmitry Torokhov, linux-input, linux-kselftest, linux-kernel,
Vadim Klishko, Peter Hutterer
Note: this requires INPUT_PROP_PRESSUREPAD [1] which is not yet
available in Linus' tree but it is in Dmitry's for-linus tree.
Nicely enough MS defines a button type for a pressurepad touchpad [2]
and it looks like most touchpad vendors fill this in.
The selftests require a bit of prep work (and a hack for the test
itself) - hidtools 0.12 requires python-libevdev 0.13 which in turn
provides constructors for unknown properties.
[1] https://lore.kernel.org/linux-input/20251030011735.GA969565@quokka/T/#m9d9b2e09eae5262cae202a5b917b543faa6fd996
[2] https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-capabilities-feature-report
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
Peter Hutterer (3):
selftests/hid: require hidtools 0.12
selftests/hid: use a enum class for the different button types
HID: multitouch: set INPUT_PROP_PRESSUREPAD based on Digitizer/Button Type
drivers/hid/hid-multitouch.c | 12 ++++-
tools/testing/selftests/hid/tests/conftest.py | 14 +++++
.../testing/selftests/hid/tests/test_multitouch.py | 61 +++++++++++++++++-----
3 files changed, 73 insertions(+), 14 deletions(-)
---
base-commit: 2bc4c50a42f8b83f611d0475598dc72740e87640
change-id: 20251111-wip-hid-pressurepad-8a800cdf1813
Best regards,
--
Peter Hutterer <peter.hutterer@who-t.net>
^ permalink raw reply
* [PATCH v2] selftests: hid: tests: test_wacom_generic: add tests for display devices and opaque devices
From: Alex Tran @ 2025-11-21 0:04 UTC (permalink / raw)
To: jikos, bentiss, shuah
Cc: linux-input, linux-kselftest, linux-kernel, Alex Tran
In-Reply-To: <20251117014721.3142490-1-alex.t.tran@gmail.com>
Verify Wacom devices set INPUT_PROP_DIRECT on display devices and
INPUT_PROP_POINTER on opaque devices. Moved test_prop_pointer into
TestOpaqueTablet. Created a DirectTabletTest mixin class for
test_prop_direct that can be inherited by display tablet test classes.
Used DirectTabletTest for TestDTH2452Tablet case.
Signed-off-by: Alex Tran <alex.t.tran@gmail.com>
---
Changes in v2:
- Removed the tests from the BaseTest class
- Removed disabling tests for certain subclasses
- Moved test_prop_pointer under TestOpaqueTablet
- Created DirectTabletTest mixin class
- Moved test_prop_direct under TestDTH2452Tablet
.../selftests/hid/tests/test_wacom_generic.py | 30 +++++++++++--------
1 file changed, 17 insertions(+), 13 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..9d0b0802d 100644
--- a/tools/testing/selftests/hid/tests/test_wacom_generic.py
+++ b/tools/testing/selftests/hid/tests/test_wacom_generic.py
@@ -598,18 +598,6 @@ class BaseTest:
if unit_set:
assert required[usage].contains(field)
- def test_prop_direct(self):
- """
- Todo: Verify that INPUT_PROP_DIRECT is set on display devices.
- """
- pass
-
- def test_prop_pointer(self):
- """
- Todo: Verify that INPUT_PROP_POINTER is set on opaque devices.
- """
- pass
-
class PenTabletTest(BaseTest.TestTablet):
def assertName(self, uhdev):
@@ -677,6 +665,13 @@ class TestOpaqueTablet(PenTabletTest):
uhdev.event(130, 240, pressure=0), [], auto_syn=False, strict=True
)
+ def test_prop_pointer(self):
+ """
+ Verify that INPUT_PROP_POINTER is set on opaque devices.
+ """
+ evdev = self.uhdev.get_evdev()
+ assert libevdev.INPUT_PROP_POINTER in evdev.properties
+
class TestOpaqueCTLTablet(TestOpaqueTablet):
def create_device(self):
@@ -862,7 +857,16 @@ class TestPTHX60_Pen(TestOpaqueCTLTablet):
)
-class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest):
+class DirectTabletTest():
+ def test_prop_direct(self):
+ """
+ Verify that INPUT_PROP_DIRECT is set on display devices.
+ """
+ evdev = self.uhdev.get_evdev()
+ assert libevdev.INPUT_PROP_DIRECT in evdev.properties
+
+
+class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest, DirectTabletTest):
ContactIds = namedtuple("ContactIds", "contact_id, tracking_id, slot_num")
def create_device(self):
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v9 04/11] HID: asus: fortify keyboard handshake
From: Antheas Kapenekakis @ 2025-11-20 21:54 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Denis Benato, platform-driver-x86, linux-input, LKML, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede
In-Reply-To: <04075ef3-3fba-c308-871f-619972ffe5ff@linux.intel.com>
On Thu, 20 Nov 2025 at 17:41, Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Thu, 20 Nov 2025, Antheas Kapenekakis wrote:
>
> > On Thu, 20 Nov 2025 at 15:15, Denis Benato <benato.denis96@gmail.com> wrote:
> > >
> > >
> > > 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,
>
> It's also about always doing the right thing so others will pick up the
> pattern (for the cases when it's needed).
>
> > > 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.
> >
> > It was ambiguous. I interpreted Ilpo's email as a dismissal
>
> I tried to explain how to use it, not to suggest cleanup.h shouldn't be
> used.
Ok, I'll wait a few days and do another revision, doing some rewording
as well. Hopefully that will cover everything. To that extent, try to
finish reviewing the latter part of the series before that revision.
I'm a bit concerned with introducing kfree here because I do not know
how to use it and it might regress, but it should be ok.
I'd rather push the init down instead of pulling it up. Referencing
other code samples for kfree it is acceptable to push the variable
definition down, right?
Antheas
> > I will try to incorporate it if I do another revision. Although I do
> > not think it improves things in this case as the function does not
> > have multiple return statements.
> >
> > > > + 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;
> > > > }
> > > >
> > >
> >
>
> --
> i.
>
>
^ permalink raw reply
* Re: [PATCH 1/2 v2] HID: input: map HID_GD_Z to ABS_DISTANCE for stylus/pen
From: Jiri Kosina @ 2025-11-20 21:48 UTC (permalink / raw)
To: Ping Cheng
Cc: linux-input, Benjamin Tissoires, benjamin.tissoires,
Dmitry.Torokhov, stable, Ping Cheng
In-Reply-To: <CAF8JNh+H=Duf-DnbU5tonMLx2yeSrOQUSMFs8VA9SrzN13Bu_Q@mail.gmail.com>
On Wed, 19 Nov 2025, Ping Cheng wrote:
> Hi Benjamin and Jiri,
>
> Can one of you review and merge the patchset?
>
> This patch was from me. The second patch for selftest followed
> Benjamin's suggestion
Sorry for the delay. This is now queued in hid.git#for-6.19/core.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: ili2130: touchscreen-size-x/y DT properties are ignored, firmware reports 16384x16384 range
From: Dmitry Torokhov @ 2025-11-20 19:30 UTC (permalink / raw)
To: Fabio Estevam
Cc: open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
joe_hung
In-Reply-To: <CAOMZO5DhfCk9=uMONdwndrYgEXtYj6L6-mRbYyP-q5M4J9_DAg@mail.gmail.com>
Hi Fabio,
On Thu, Nov 20, 2025 at 09:40:14AM -0300, Fabio Estevam wrote:
> 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 */
Touchscreen controller coordinates/resolution is separate from the
display resolution, userspace is responsible for the conversion.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v1] selftests: hid: tests: test_wacom_generic: add base test for display devices and opaque devices
From: Alex Tran @ 2025-11-20 17:42 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: jikos, shuah, linux-input, linux-kselftest, linux-kernel
In-Reply-To: <awbmhna3hkra2eoc7lcl23d3mzfsk7qty5t4zl3m7s6hau3v4u@uzqkqudwttlp>
On Thu, Nov 20, 2025 at 2:53 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> 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
> >
Thanks for the review. Yes, it seems like a good idea to restructure
the class hierarchy instead of disabling the
tests like this. I'll send in a v2 with the changes soon.
Regards,
--
Alex Tran
^ permalink raw reply
* Re: [PATCH v9 04/11] HID: asus: fortify keyboard handshake
From: Ilpo Järvinen @ 2025-11-20 16:41 UTC (permalink / raw)
To: Antheas Kapenekakis
Cc: Denis Benato, platform-driver-x86, linux-input, LKML, Jiri Kosina,
Benjamin Tissoires, Corentin Chary, Luke D . Jones, Hans de Goede
In-Reply-To: <CAGwozwF6wegwHy=W1zaTEVksQYaw4L7V27w2aaZBMMoDjUjRYg@mail.gmail.com>
On Thu, 20 Nov 2025, Antheas Kapenekakis wrote:
> On Thu, 20 Nov 2025 at 15:15, Denis Benato <benato.denis96@gmail.com> wrote:
> >
> >
> > 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,
It's also about always doing the right thing so others will pick up the
pattern (for the cases when it's needed).
> > 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.
>
> It was ambiguous. I interpreted Ilpo's email as a dismissal
I tried to explain how to use it, not to suggest cleanup.h shouldn't be
used.
> I will try to incorporate it if I do another revision. Although I do
> not think it improves things in this case as the function does not
> have multiple return statements.
>
> > > + 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;
> > > }
> > >
> >
>
--
i.
^ permalink raw reply
* Re: [PATCH v9 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Ilpo Järvinen @ 2025-11-20 16:38 UTC (permalink / raw)
To: Denis Benato
Cc: Antheas Kapenekakis, platform-driver-x86, linux-input, LKML,
Jiri Kosina, Benjamin Tissoires, Corentin Chary, Luke D . Jones,
Hans de Goede, Ilpo Järvinen
In-Reply-To: <89dd8c0f-dfe2-4209-af38-01a3ef6df7ba@gmail.com>
On Thu, 20 Nov 2025, Denis Benato wrote:
>
> 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"?
Also some making it more than one paragraphs would help reading that bit
of text. You're stating like 4 important things here so I'd put each into
own paragraph.
I'm not entiry sure what the Except even refers to, to other variables or
read-onlyness, the next sentence explains it a bit more but it's too late
to understand the previous sentence.
--
i.
> 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 00/10] HID: bpf: sync up with current udev-hid-bpf programs
From: Jiri Kosina @ 2025-11-20 15:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-kernel, linux-input, Peter Hutterer, Nicholas LaPointe,
Higgins Dragon, Curran Muhlberger, Jan Felix Langenbach,
Hannah Pittman, Colin Ian King
In-Reply-To: <20251118-wip-sync-udev-hid-bpf-v1-0-0f8105c54835@kernel.org>
On Tue, 18 Nov 2025, Benjamin Tissoires wrote:
> As I need to get a new release of udev-hid-bpf, it would be good to
> mark the currently "testing" HID-BPF programs into stable.
>
> So I've taken all of them, formatted them to the LKML format and sent
> here.
>
> Again, no need to backport any of those into stable, we are putting them
> here mostly for code archiving purpose and have a public central point
> for them.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> Benjamin Tissoires (10):
> HID: bpf: Add support for the Inspiroy 2M
> HID: bpf: add support for Huion Kamvas 13 (Gen 3) (model GS1333)
> HID: bpf: support for Huion Kamvas 16 Gen 3
> HID: bpf: Add fixup for Logitech SpaceNavigator variants
> HID: bpf: Add support for the Waltop Batteryless Tablet
> HID: bpf: Add support for the XP-Pen Deco 01 V3
> HID: bpf: Add support for XP-Pen Deco02
> HID: bpf: add heuristics to the Huion Inspiroy 2S eraser button
> HID: bpf: add the Huion Kamvas 27 Pro
> HID: bpf: fix typo in HID usage table
Now in hid.git#for-6.19/hid-bpf, thanks!
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v9 06/11] HID: asus: early return for ROG devices
From: Antheas Kapenekakis @ 2025-11-20 14:43 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: <5e577dcf-995e-441b-9351-11d6da097fcc@gmail.com>
On Thu, 20 Nov 2025 at 15:29, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> On 11/20/25 15:15, Antheas Kapenekakis wrote:
> > 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.
> I will ask for logs, but I recall someone with the same model sent dmesg already,
> I'll try to find it, but if this is true... Are we lending control of LEDs to a bugged WMI
> implementation for this laptop?
>
Yes, the asus-wmi driver is bugged in certain laptops. This does not
mean it should not be activated-the device has RGB. It means that it
should be fixed eventually.
^ permalink raw reply
* Re: [PATCH v9 07/11] platform/x86: asus-wmi: Add support for multiple kbd led handlers
From: Antheas Kapenekakis @ 2025-11-20 14:40 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: <89dd8c0f-dfe2-4209-af38-01a3ef6df7ba@gmail.com>
On Thu, 20 Nov 2025 at 14:46, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> 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"?
I think it is pretty clear that both of them refer to the led cdev
device. Phrasing could be a bit better but it is not ambiguous.
> On "ref is dropped" I would continue with ": since .asus .....".
Add spaces in-between your replies, these are easy to miss.
Since... is a separate sentence and its meaning is correct. It should
not be chained with the first sentence.
Essentially, this paragraph says three things: 1) .asus,
.asus.kbd_led_{wk,notify}, and .listener refs are protected by the
spinlock. 2) the led cdev is not, because it is either initialized
during probe before asus.ref is registered so hid-asus cannot touch it
or in the workqueue (if not kbd_led_avail) which is single threaded
and guaranteed to continue until asus-wmi touches that variable again
because it will have destroyed the workqueue first. 3) A spinlock is
used because these variables are accessed in an IRQ context.
> > + .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?
Yes. After you get a reference to asus_ref.asus you need to hold a
spinlock until it is dropped so asus-wmi is not able to exit or mutate
itself.
For the same reason, the workqueue can be triggered by both asus-wmi
and multiple hid-asus devices, so it needs to be bound as well.
> > + 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 v9 04/11] HID: asus: fortify keyboard handshake
From: Denis Benato @ 2025-11-20 14:31 UTC (permalink / raw)
To: Antheas Kapenekakis
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: <CAGwozwF6wegwHy=W1zaTEVksQYaw4L7V27w2aaZBMMoDjUjRYg@mail.gmail.com>
On 11/20/25 15:28, Antheas Kapenekakis wrote:
> On Thu, 20 Nov 2025 at 15:15, Denis Benato <benato.denis96@gmail.com> wrote:
>>
>> 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.
> It was ambiguous. I interpreted Ilpo's email as a dismissal
>
> I will try to incorporate it if I do another revision. Although I do
> not think it improves things in this case as the function does not
> have multiple return statements.
I will leave this decision to Ilpo, if he thinks there is no point in using
__free here I will add my Reviewed-by tag.
>>> + 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 v9 06/11] HID: asus: early return for ROG devices
From: Denis Benato @ 2025-11-20 14:29 UTC (permalink / raw)
To: Antheas Kapenekakis
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: <CAGwozwEk0k3K8v2GOX2+9Rrcx_pp4xAmiJExzoRNADeridRTfA@mail.gmail.com>
On 11/20/25 15:15, Antheas Kapenekakis wrote:
> 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.
I will ask for logs, but I recall someone with the same model sent dmesg already,
I'll try to find it, but if this is true... Are we lending control of LEDs to a bugged WMI
implementation for this laptop?
^ permalink raw reply
* Re: [PATCH v9 04/11] HID: asus: fortify keyboard handshake
From: Antheas Kapenekakis @ 2025-11-20 14:28 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: <967761fb-3f55-4d51-be0b-23ad03258eff@gmail.com>
On Thu, 20 Nov 2025 at 15:15, Denis Benato <benato.denis96@gmail.com> wrote:
>
>
> 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.
It was ambiguous. I interpreted Ilpo's email as a dismissal
I will try to incorporate it if I do another revision. Although I do
not think it improves things in this case as the function does not
have multiple return statements.
> > + 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 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
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