[PATCH 0/3] 64-bit Digitizer Serial Numbers

[PATCH 0/3] 64-bit Digitizer Serial Numbers

[Kenneth Albanowski]

Still as RFC, here's a patch set to implement support for 64-bit
Digitizer Serial Numbers, following on to the prior discussion of how
we can support Usage(Digitizers.Transducer Serial Number) larger
than 32 bits.

My primary goal is making the serial number available via the
power_supply node interface, and the third patch implements that: if
CONFIG_HID_BATTERY_STRENGTH is enabled, and a hid report includes
both Usage(Digitizers.Battery Strength) and Usage(Digitizers.
Transducer Serial Number) then the generated power_supply node
includes a SERIAL_NUMBER of "DG-ABCDEF..." of the last seen digitizer.

For this patch set, I have not implemented any changes to MSC_SERIAL*,
however everything should be available for a future implementation. This
patch set does not change any events emitted by evdev.

After some conversation with Dmitry, I am leaning towards serial number
integration with the hid-multitouch protocol being the better long term
solution for evdev, as that fits better with combined touch and stylus
operation, as well as providing a clear means to providing multiple
serial numbers for simultaneous styluses.

This set includes general modifications to hid-core to decode and
represent HID fields larger than 32-bits as multiple consecutive 32-bit
words, allowing arbitrarily long fields (or at least as many as can fit
into HID_MAX_BUFFER_SIZE) to be passed upstream and (potentially)
processed by hid-input and other subsystems.

(After trying a 64-bit implementation, I decided staying with 32-bit
words kept the rest of the hid-core cleaner, and it was worth 
supporting arbitrary length fields instead of only upgrading
from 32 -> 64.)

Kenneth Albanowski (3):
  [hid] Minor cleanup
  [hid] Enable processing of fields larger than 32 bits.
  [hid] Emit digitizer serial number through power_supply

 Documentation/hid/hiddev.rst |   6 +-
 drivers/hid/hid-core.c       |  99 +++++++++++++++++++++-----------
 drivers/hid/hid-debug.c      |  27 ++++++---
 drivers/hid/hid-input.c      | 108 +++++++++++++++++++++++++++++++++--
 include/linux/hid-debug.h    |   4 +-
 include/linux/hid.h          |  10 +++-
 6 files changed, 199 insertions(+), 55 deletions(-)

-- 
2.31.0

[PATCH 1/3] [hid] Minor cleanup

[Kenneth Albanowski]

No functional changes, just cleanup.

Clarify actual integer size limits in a few comments.

Make one constant explicit to improve readability.

Replace open-coded sign-extension with common routine.

Signed-off-by: Kenneth Albanowski <kenalba@google.com>
---

 drivers/hid/hid-core.c | 19 +++++++------------
 include/linux/hid.h    |  3 ++-
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5b30783e2353d..19b2b0aae0c7e 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -337,7 +337,8 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 }
 
 /*
- * Read data value from item.
+ * Read data value from global items, which are
+ * a maximum of 32 bits in size.
  */
 
 static u32 item_udata(struct hid_item *item)
@@ -709,7 +710,7 @@ static void hid_device_release(struct device *dev)
 
 /*
  * Fetch a report description item from the data stream. We support long
- * items, though they are not used yet.
+ * items, though there are not yet any defined uses for them.
  */
 
 static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
@@ -745,6 +746,7 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 	item->format = HID_ITEM_FORMAT_SHORT;
 	item->size = b & 3;
 
+	/* Map size values 0,1,2,3 to actual sizes 0,1,2,4 */
 	switch (item->size) {
 	case 0:
 		return start;
@@ -763,7 +765,7 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 		return start;
 
 	case 3:
-		item->size++;
+		item->size = 4;
 		if ((end - start) < 4)
 			return NULL;
 		item->data.u32 = get_unaligned_le32(start);
@@ -1300,9 +1302,7 @@ int hid_open_report(struct hid_device *device)
 EXPORT_SYMBOL_GPL(hid_open_report);
 
 /*
- * Convert a signed n-bit integer to signed 32-bit integer. Common
- * cases are done through the compiler, the screwed things has to be
- * done by hand.
+ * Convert a signed n-bit integer to signed 32-bit integer.
  */
 
 static s32 snto32(__u32 value, unsigned n)
@@ -1310,12 +1310,7 @@ static s32 snto32(__u32 value, unsigned n)
 	if (!value || !n)
 		return 0;
 
-	switch (n) {
-	case 8:  return ((__s8)value);
-	case 16: return ((__s16)value);
-	case 32: return ((__s32)value);
-	}
-	return value & (1 << (n - 1)) ? value | (~0U << n) : value;
+	return sign_extend32(value, n - 1);
 }
 
 s32 hid_snto32(__u32 value, unsigned n)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7a13e9d3441a4..59828a6080e83 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -391,6 +391,7 @@ struct hid_item {
 
 struct hid_global {
 	unsigned usage_page;
+	/* HID Global fields are constrained by spec to 32-bits */
 	__s32    logical_minimum;
 	__s32    logical_maximum;
 	__s32    physical_minimum;
@@ -457,7 +458,7 @@ struct hid_field {
 	unsigned  maxusage;		/* maximum usage index */
 	unsigned  flags;		/* main-item flags (i.e. volatile,array,constant) */
 	unsigned  report_offset;	/* bit offset in the report */
-	unsigned  report_size;		/* size of this field in the report */
+	unsigned  report_size;		/* size of this field in the report, in bits */
 	unsigned  report_count;		/* number of this field in the report */
 	unsigned  report_type;		/* (input,output,feature) */
 	__s32    *value;		/* last known value(s) */
-- 
2.31.0

[PATCH 2/3] [hid] Enable processing of fields larger than 32 bits.

[Kenneth Albanowski]

Extends hid-core to process HID fields larger than 32 bits,
by storing them as multiple consecutive 32-bit values.

hid-input and hid-debug are specifically changed to match.
Currently, no other hid consumers get an interface to see
fields larger than 32 bits.

All existing code, including hid-input processing, will by
default see the least significant 32-bits of the field, so they
have unchanged behaviour (sign extension does not change: for
any field longer than 32 bits, extension was already a no-op,
so the least significant 32-bit value will be identical. The most
significant value, at the far end, will now be extended to fill
its 32-bit value.)

Logical min/max interaction with larger fields is limited,
as min/max and other item parameters can only be described with
32-bit values (sometimes signed). hid-input is expected to
ignore min/max in scenarios where a specific larger field is
involved.

hid-debug 'events' debugfs text report format is changed, but
only for HID fields larger than 32 bits.

Signed-off-by: Kenneth Albanowski <kenalba@google.com>
---

 Documentation/hid/hiddev.rst |  6 ++-
 drivers/hid/hid-core.c       | 80 ++++++++++++++++++++++++++----------
 drivers/hid/hid-debug.c      | 27 ++++++++----
 drivers/hid/hid-input.c      | 10 ++++-
 include/linux/hid-debug.h    |  4 +-
 include/linux/hid.h          |  2 +-
 6 files changed, 92 insertions(+), 37 deletions(-)

diff --git a/Documentation/hid/hiddev.rst b/Documentation/hid/hiddev.rst
index 209e6ba4e019e..f22fcf1b55b9e 100644
--- a/Documentation/hid/hiddev.rst
+++ b/Documentation/hid/hiddev.rst
@@ -72,8 +72,10 @@ The hiddev API uses a read() interface, and a set of ioctl() calls.
 
 HID devices exchange data with the host computer using data
 bundles called "reports".  Each report is divided into "fields",
-each of which can have one or more "usages".  In the hid-core,
-each one of these usages has a single signed 32 bit value.
+each of which can have one or more "usages". Each of these usages
+has a value, usually a 32 bit or smaller signed value. (The
+hid-core can process larger values, but these are not currently
+exposed through hiddev.)
 
 read():
 -------
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 19b2b0aae0c7e..e588c3a35a593 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -86,22 +86,35 @@ struct hid_report *hid_register_report(struct hid_device *device,
 }
 EXPORT_SYMBOL_GPL(hid_register_report);
 
+// How many 32-bit values are needed to store a field?
+static unsigned hid_field_size_in_values(unsigned flags, unsigned size_in_bits)
+{
+	if (!(flags & HID_MAIN_ITEM_VARIABLE)) {
+		return 1;
+	} else {
+		return DIV_ROUND_UP(size_in_bits, 32);
+	}
+}
+
 /*
  * Register a new field for this report.
  */
 
-static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages)
+static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned flags, unsigned size_in_bits)
 {
 	struct hid_field *field;
+	unsigned size_in_values;
 
 	if (report->maxfield == HID_MAX_FIELDS) {
 		hid_err(report->device, "too many fields in report\n");
 		return NULL;
 	}
 
+	size_in_values = hid_field_size_in_values(flags, size_in_bits);
+
 	field = kzalloc((sizeof(struct hid_field) +
 			 usages * sizeof(struct hid_usage) +
-			 usages * sizeof(unsigned)), GFP_KERNEL);
+			 usages * size_in_values * sizeof(s32)), GFP_KERNEL);
 	if (!field)
 		return NULL;
 
@@ -300,7 +313,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 	usages = max_t(unsigned, parser->local.usage_index,
 				 parser->global.report_count);
 
-	field = hid_register_field(report, usages);
+	field = hid_register_field(report, usages, flags, parser->global.report_size);
 	if (!field)
 		return 0;
 
@@ -1340,6 +1353,9 @@ static u32 s32ton(__s32 value, unsigned n)
  * While the USB HID spec allows unlimited length bit fields in "report
  * descriptors", most devices never use more than 16 bits.
  * One model of UPS is claimed to report "LINEV" as a 32-bit field.
+ * Some digitizers report stylus transducer IDs as 64-bit fields.
+ * The outer routines will extract multiple 32-bit parts if necessary
+ * to retrieve an entire field.
  * Search linux-kernel and linux-usb-devel archives for "hid-core extract".
  */
 
@@ -1495,16 +1511,19 @@ static int hid_match_usage(struct hid_device *hid, struct hid_usage *usage)
 }
 
 static void hid_process_event(struct hid_device *hid, struct hid_field *field,
-		struct hid_usage *usage, __s32 value, int interrupt)
+		struct hid_usage *usage, const __s32 * values, unsigned value_count, int interrupt)
 {
 	struct hid_driver *hdrv = hid->driver;
 	int ret;
 
+	if (unlikely(value_count == 0))
+		return;
+
 	if (!list_empty(&hid->debug_list))
-		hid_dump_input(hid, usage, value);
+		hid_dump_input(hid, usage, values, value_count);
 
 	if (hdrv && hdrv->event && hid_match_usage(hid, usage)) {
-		ret = hdrv->event(hid, field, usage, value);
+		ret = hdrv->event(hid, field, usage, values[0]);
 		if (ret != 0) {
 			if (ret < 0)
 				hid_err(hid, "%s's event failed with %d\n",
@@ -1514,9 +1533,9 @@ static void hid_process_event(struct hid_device *hid, struct hid_field *field,
 	}
 
 	if (hid->claimed & HID_CLAIMED_INPUT)
-		hidinput_hid_event(hid, field, usage, value);
+		hidinput_hid_event(hid, field, usage, values, value_count);
 	if (hid->claimed & HID_CLAIMED_HIDDEV && interrupt && hid->hiddev_hid_event)
-		hid->hiddev_hid_event(hid, field, usage, value);
+		hid->hiddev_hid_event(hid, field, usage, values[0]);
 }
 
 /*
@@ -1528,24 +1547,41 @@ static void hid_process_event(struct hid_device *hid, struct hid_field *field,
 static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 			    __u8 *data, int interrupt)
 {
-	unsigned n;
+	unsigned n, v;
 	unsigned count = field->report_count;
 	unsigned offset = field->report_offset;
-	unsigned size = field->report_size;
+	unsigned size_in_bits = field->report_size;
+	unsigned size_in_values; /* storage size in 32-bit values, always >= 1 */
+	unsigned last_value_size_in_bits; /* bits in most significant/last value */
+	unsigned bit_pos;
 	__s32 min = field->logical_minimum;
 	__s32 max = field->logical_maximum;
 	__s32 *value;
+	static const __s32 zero = 0;
+	static const __s32 one = 1;
+
+	size_in_values = hid_field_size_in_values(field->flags, size_in_bits);
+	last_value_size_in_bits = (size_in_bits % 32) ?: 32;
 
-	value = kmalloc_array(count, sizeof(__s32), GFP_ATOMIC);
+	value = kmalloc_array(count * size_in_values, sizeof(__s32), GFP_ATOMIC);
 	if (!value)
 		return;
 
-	for (n = 0; n < count; n++) {
+	for (n = 0; n < count * size_in_values; n += size_in_values) {
+		v = 0;
+		bit_pos = offset + (n * size_in_bits);
+
+		// Extract least significant values for fields longer than 32 bits.
+		if (size_in_values > 1) {
+			for (; v < size_in_values - 1; v++, bit_pos += 32)
+				value[n+v] = hid_field_extract(hid, data, bit_pos, 32);
+		}
 
-		value[n] = min < 0 ?
-			snto32(hid_field_extract(hid, data, offset + n * size,
-			       size), size) :
-			hid_field_extract(hid, data, offset + n * size, size);
+		// May need to sign extend the most significant value.
+		value[n+v] = min < 0 ?
+			sign_extend32(hid_field_extract(hid, data, bit_pos,
+				last_value_size_in_bits), last_value_size_in_bits) :
+			hid_field_extract(hid, data, bit_pos, last_value_size_in_bits);
 
 		/* Ignore report if ErrorRollOver */
 		if (!(field->flags & HID_MAIN_ITEM_VARIABLE) &&
@@ -1555,10 +1591,10 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 			goto exit;
 	}
 
-	for (n = 0; n < count; n++) {
+	for (n = 0; n < count * size_in_values; n += size_in_values) {
 
 		if (HID_MAIN_ITEM_VARIABLE & field->flags) {
-			hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
+			hid_process_event(hid, field, &field->usage[n], value + n, size_in_values, interrupt);
 			continue;
 		}
 
@@ -1566,16 +1602,16 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 			&& field->value[n] - min < field->maxusage
 			&& field->usage[field->value[n] - min].hid
 			&& search(value, field->value[n], count))
-				hid_process_event(hid, field, &field->usage[field->value[n] - min], 0, interrupt);
+				hid_process_event(hid, field, &field->usage[field->value[n] - min], &zero, 1, interrupt);
 
 		if (value[n] >= min && value[n] <= max
 			&& value[n] - min < field->maxusage
 			&& field->usage[value[n] - min].hid
 			&& search(field->value, value[n], count))
-				hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
+				hid_process_event(hid, field, &field->usage[value[n] - min], &one, 1, interrupt);
 	}
 
-	memcpy(field->value, value, count * sizeof(__s32));
+	memcpy(field->value, value, count * size_in_values * sizeof(__s32));
 exit:
 	kfree(value);
 }
@@ -1662,7 +1698,7 @@ int hid_set_field(struct hid_field *field, unsigned offset, __s32 value)
 
 	size = field->report_size;
 
-	hid_dump_input(field->report->device, field->usage + offset, value);
+	hid_dump_input(field->report->device, field->usage + offset, &value, 1);
 
 	if (offset >= field->report_count) {
 		hid_err(field->report->device, "offset (%d) exceeds report_count (%d)\n",
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 9453147d020db..bc25b0e8b6b63 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -692,21 +692,30 @@ void hid_dump_report(struct hid_device *hid, int type, u8 *data,
 }
 EXPORT_SYMBOL_GPL(hid_dump_report);
 
-void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage, __s32 value)
+void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage, const __s32 *values, unsigned value_count)
 {
 	char *buf;
 	int len;
+	unsigned n;
 
-	buf = hid_resolv_usage(usage->hid, NULL);
-	if (!buf)
-		return;
-	len = strlen(buf);
-	snprintf(buf + len, HID_DEBUG_BUFSIZE - len - 1, " = %d\n", value);
+	for (n = 0; n < value_count; n++) {
 
-	hid_debug_event(hdev, buf);
+		buf = hid_resolv_usage(usage->hid, NULL);
+		if (!buf)
+			return;
 
-	kfree(buf);
-	wake_up_interruptible(&hdev->debug_wait);
+		len = strlen(buf);
+
+		if (value_count == 1)
+			snprintf(buf + len, HID_DEBUG_BUFSIZE - len - 1, " = %d\n", values[n]);
+		else
+			snprintf(buf + len, HID_DEBUG_BUFSIZE - len - 1, "[%d] = %d (%08x)\n", n, values[n], values[n]);
+
+		hid_debug_event(hdev, buf);
+
+		kfree(buf);
+		wake_up_interruptible(&hdev->debug_wait);
+	}
 }
 EXPORT_SYMBOL_GPL(hid_dump_input);
 
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index bceccd75b488e..ee9e8d31a45ba 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1273,10 +1273,18 @@ static void hidinput_handle_scroll(struct hid_usage *usage,
 	input_event(input, EV_REL, usage->code, hi_res);
 }
 
-void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value)
+void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, const __s32 *values, unsigned value_count)
 {
 	struct input_dev *input;
 	unsigned *quirks = &hid->quirks;
+	__s32 value;
+
+	if (unlikely(value_count == 0))
+		return;
+
+	// The majority of this code was writen to only understand 32-bit sized
+	// values, and anything larger was truncated: we continue that tradition.
+	value = values[0];
 
 	if (!usage->type)
 		return;
diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
index ea7b23d13bfdf..b4fb1a73817b4 100644
--- a/include/linux/hid-debug.h
+++ b/include/linux/hid-debug.h
@@ -16,7 +16,7 @@
 #define HID_DEBUG_BUFSIZE 512
 #define HID_DEBUG_FIFOSIZE 512
 
-void hid_dump_input(struct hid_device *, struct hid_usage *, __s32);
+void hid_dump_input(struct hid_device *, struct hid_usage *, const __s32 *, unsigned);
 void hid_dump_report(struct hid_device *, int , u8 *, int);
 void hid_dump_device(struct hid_device *, struct seq_file *);
 void hid_dump_field(struct hid_field *, int, struct seq_file *);
@@ -37,7 +37,7 @@ struct hid_debug_list {
 
 #else
 
-#define hid_dump_input(a,b,c)		do { } while (0)
+#define hid_dump_input(a,b,c,d)		do { } while (0)
 #define hid_dump_report(a,b,c,d)	do { } while (0)
 #define hid_dump_device(a,b)		do { } while (0)
 #define hid_dump_field(a,b,c)		do { } while (0)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 59828a6080e83..8494b1995b10b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -878,7 +878,7 @@ extern void hid_unregister_driver(struct hid_driver *);
 	module_driver(__hid_driver, hid_register_driver, \
 		      hid_unregister_driver)
 
-extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct hid_usage *, __s32);
+extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct hid_usage *, const __s32 *values, unsigned value_count);
 extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
 extern int hidinput_connect(struct hid_device *hid, unsigned int force);
 extern void hidinput_disconnect(struct hid_device *);
-- 
2.31.0

[PATCH 3/3] [hid] Emit digitizer serial number through power_supply

[Kenneth Albanowski]

HID devices that expose a battery strength can have
associated power_supply nodes. This fills in the
SERIAL_NUMBER power_supply field if the same HID device
also has a Digitizer.Transducer Serial Number usage,
effectively allowing that particular stylus to be
identified.

If the field is present and non-zero, the serial number
will be 'DG-ABCD' where 'ABCD' is up to sixteen hex
digits -- field lengths of up to 64-bits are supported,
the largest currently known about.

Devices are expected to emit zero if the transducer
does not have a serial number, or the serial number
has not yet been acquired; zeros will be ignored.

Note that logical min/max (and other HID item
parameters) will be ignored for this field.

Signed-off-by: Kenneth Albanowski <kenalba@google.com>
---

 drivers/hid/hid-input.c | 100 +++++++++++++++++++++++++++++++++++++---
 include/linux/hid.h     |   5 ++
 2 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index ee9e8d31a45ba..c5767ceb4a61c 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -286,6 +286,7 @@ static enum power_supply_property hidinput_battery_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_SERIAL_NUMBER,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_SCOPE,
 };
@@ -402,6 +403,26 @@ static int hidinput_get_battery_property(struct power_supply *psy,
 		val->strval = dev->name;
 		break;
 
+	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
+		/* Serial number does not have an active HID query
+		 * mechanism like hidinput_query_battery_capacity, as the
+		 * only devices expected to have serial numbers are digitizers,
+		 * which are unlikely to be able to pull the serial number from
+		 * an untethered pen on demand.
+		 */
+		if (dev->battery_serial_number == 0) {
+			/* Make no claims about S/N format if we haven't actually seen a value yet. */
+			strcpy(dev->battery_serial_number_str, "");
+		} else {
+			snprintf(dev->battery_serial_number_str,
+				 sizeof(dev->battery_serial_number_str),
+				 "DG-%0*llX",
+				 DIV_ROUND_UP(dev->battery_serial_number_bits, 4),
+				 dev->battery_serial_number);
+		}
+		val->strval = dev->battery_serial_number_str;
+		break;
+
 	case POWER_SUPPLY_PROP_STATUS:
 		if (dev->battery_status != HID_BATTERY_REPORTED &&
 		    !dev->battery_avoid_query) {
@@ -485,6 +506,8 @@ static int hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
 	dev->battery_max = max;
 	dev->battery_report_type = report_type;
 	dev->battery_report_id = field->report->id;
+	dev->battery_changed = false;
+	dev->battery_reported = false;
 
 	/*
 	 * Stylus is normally not connected to the device and thus we
@@ -526,7 +549,8 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
 	dev->battery = NULL;
 }
 
-static void hidinput_update_battery(struct hid_device *dev, int value)
+static void hidinput_update_battery_capacity(struct hid_device *dev,
+					     __s32 value)
 {
 	int capacity;
 
@@ -538,11 +562,57 @@ static void hidinput_update_battery(struct hid_device *dev, int value)
 
 	capacity = hidinput_scale_battery_capacity(dev, value);
 
+	if (capacity != dev->battery_capacity) {
+		dev->battery_capacity = capacity;
+		dev->battery_changed = true;
+	}
+	dev->battery_reported = true;
+}
+
+static void hidinput_update_battery_serial(struct hid_device *dev,
+					   const __s32 *values, int bits)
+{
+	__u64 value;
+
+	if (!dev->battery)
+		return;
+
+	if (bits > 64)
+		bits = 64;
+
+	value = (__u64)(__u32)values[0];
+	if (bits > 32)
+		value |= (__u64)values[1] << 32;
+
+	if (value == 0)
+		return;
+
+	if (value != dev->battery_serial_number) {
+		dev->battery_serial_number = value;
+		dev->battery_serial_number_bits = bits;
+		dev->battery_changed = true;
+	}
+	dev->battery_reported = true;
+}
+
+static void hidinput_flush_battery(struct hid_device *dev)
+{
+	if (!dev->battery)
+		return;
+
+	/* Only consider pushing a battery change if there is a
+	 * battery field in this report.
+	 */
+	if (!dev->battery_reported)
+		return;
+
+	dev->battery_reported = false;
+
 	if (dev->battery_status != HID_BATTERY_REPORTED ||
-	    capacity != dev->battery_capacity ||
+	    dev->battery_changed ||
 	    ktime_after(ktime_get_coarse(), dev->battery_ratelimit_time)) {
-		dev->battery_capacity = capacity;
 		dev->battery_status = HID_BATTERY_REPORTED;
+		dev->battery_changed = false;
 		dev->battery_ratelimit_time =
 			ktime_add_ms(ktime_get_coarse(), 30 * 1000);
 		power_supply_changed(dev->battery);
@@ -559,7 +629,17 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
 {
 }
 
-static void hidinput_update_battery(struct hid_device *dev, int value)
+static void hidinput_update_battery_capacity(struct hid_device *dev,
+					     __s32 value)
+{
+}
+
+static void hidinput_update_battery_serial(struct hid_device *dev,
+					   const __s32 *values, int bits)
+{
+}
+
+static void hidinput_flush_battery(struct hid_device *dev)
 {
 }
 #endif	/* CONFIG_HID_BATTERY_STRENGTH */
@@ -1273,7 +1353,9 @@ static void hidinput_handle_scroll(struct hid_usage *usage,
 	input_event(input, EV_REL, usage->code, hi_res);
 }
 
-void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, const __s32 *values, unsigned value_count)
+void hidinput_hid_event(struct hid_device *hid, struct hid_field *field,
+			struct hid_usage *usage, const __s32 *values,
+			unsigned value_count)
 {
 	struct input_dev *input;
 	unsigned *quirks = &hid->quirks;
@@ -1290,9 +1372,13 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 
 	if (usage->type == EV_PWR) {
-		hidinput_update_battery(hid, value);
+		hidinput_update_battery_capacity(hid, value);
 		return;
 	}
+	if (usage->type == EV_MSC && usage->code == MSC_SERIAL) {
+		hidinput_update_battery_serial(hid, values, field->report_size);
+		/* fall through to normal standard MSC_SERIAL processing */
+	}
 
 	if (!field->hidinput)
 		return;
@@ -1423,6 +1509,8 @@ void hidinput_report_event(struct hid_device *hid, struct hid_report *report)
 {
 	struct hid_input *hidinput;
 
+	hidinput_flush_battery(hid);
+
 	if (hid->quirks & HID_QUIRK_NO_INPUT_SYNC)
 		return;
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 8494b1995b10b..d5585a99b5ad9 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -587,8 +587,13 @@ struct hid_device {							/* device report descriptor */
 	__s32 battery_max;
 	__s32 battery_report_type;
 	__s32 battery_report_id;
+	__u64 battery_serial_number;
+	int battery_serial_number_bits;					/* Actual number of bits in SN */
+	char battery_serial_number_str[20];				/* Space for "DG-" + max 16 hex digits */
 	enum hid_battery_status battery_status;
 	bool battery_avoid_query;
+	bool battery_changed;
+	bool battery_reported;
 	ktime_t battery_ratelimit_time;
 #endif
 
-- 
2.31.0

Re: [PATCH 0/3] 64-bit Digitizer Serial Numbers

[Kenneth Albanowski]

To follow up to myself, we've decided that general >32-bit HID field
support isn't justified at this time (and not really supported by the
spec), so this set of patches can be simplified.

At this time, I'm going to switch to a plan of treating a 64-bit
Usage(Digitizer.Transducer Serial Number) as a singular oddity that
needs to be processed, instead of adding general processing for
arbitrary length HID fields throughout the hid-input system.

As to the code, I'm going to be following up with revised patches:

Patch #1 (minor cleanup) remains unchanged.

Patch #2 (large field support) goes away

Patch #3 (power_supply S/N support) is kept, and adds some minor
changes to hid-core.c, particularly in hid_add_field() that
recognize the 64-bit Usage(Digitizer.Transducer Serial Number) and
cuts it into two fields, a 32-bit Usage(Digitizer.Transducer Serial
Number) and a new 32-bit usage for the upper half.

Kind regards,
- Kenneth Albanowski

On Wed, May 19, 2021 at 5:23 PM Kenneth Albanowski <kenalba@google.com> wrote:
>
> Still as RFC, here's a patch set to implement support for 64-bit
> Digitizer Serial Numbers, following on to the prior discussion of how
> we can support Usage(Digitizers.Transducer Serial Number) larger
> than 32 bits.
>
> My primary goal is making the serial number available via the
> power_supply node interface, and the third patch implements that: if
> CONFIG_HID_BATTERY_STRENGTH is enabled, and a hid report includes
> both Usage(Digitizers.Battery Strength) and Usage(Digitizers.
> Transducer Serial Number) then the generated power_supply node
> includes a SERIAL_NUMBER of "DG-ABCDEF..." of the last seen digitizer.
>
> For this patch set, I have not implemented any changes to MSC_SERIAL*,
> however everything should be available for a future implementation. This
> patch set does not change any events emitted by evdev.
>
> After some conversation with Dmitry, I am leaning towards serial number
> integration with the hid-multitouch protocol being the better long term
> solution for evdev, as that fits better with combined touch and stylus
> operation, as well as providing a clear means to providing multiple
> serial numbers for simultaneous styluses.
>
> This set includes general modifications to hid-core to decode and
> represent HID fields larger than 32-bits as multiple consecutive 32-bit
> words, allowing arbitrarily long fields (or at least as many as can fit
> into HID_MAX_BUFFER_SIZE) to be passed upstream and (potentially)
> processed by hid-input and other subsystems.
>
> (After trying a 64-bit implementation, I decided staying with 32-bit
> words kept the rest of the hid-core cleaner, and it was worth
> supporting arbitrary length fields instead of only upgrading
> from 32 -> 64.)
>
> Kenneth Albanowski (3):
>   [hid] Minor cleanup
>   [hid] Enable processing of fields larger than 32 bits.
>   [hid] Emit digitizer serial number through power_supply
>
>  Documentation/hid/hiddev.rst |   6 +-
>  drivers/hid/hid-core.c       |  99 +++++++++++++++++++++-----------
>  drivers/hid/hid-debug.c      |  27 ++++++---
>  drivers/hid/hid-input.c      | 108 +++++++++++++++++++++++++++++++++--
>  include/linux/hid-debug.h    |   4 +-
>  include/linux/hid.h          |  10 +++-
>  6 files changed, 199 insertions(+), 55 deletions(-)
>
> --
> 2.31.0
>

Re: [PATCH 0/3] 64-bit Digitizer Serial Numbers

[Jiri Kosina]

On Wed, 19 May 2021, Kenneth Albanowski wrote:

> Still as RFC, here's a patch set to implement support for 64-bit
> Digitizer Serial Numbers, following on to the prior discussion of how
> we can support Usage(Digitizers.Transducer Serial Number) larger
> than 32 bits.
> 
> My primary goal is making the serial number available via the
> power_supply node interface, and the third patch implements that: if
> CONFIG_HID_BATTERY_STRENGTH is enabled, and a hid report includes
> both Usage(Digitizers.Battery Strength) and Usage(Digitizers.
> Transducer Serial Number) then the generated power_supply node
> includes a SERIAL_NUMBER of "DG-ABCDEF..." of the last seen digitizer.
> 
> For this patch set, I have not implemented any changes to MSC_SERIAL*,
> however everything should be available for a future implementation. This
> patch set does not change any events emitted by evdev.
> 
> After some conversation with Dmitry, I am leaning towards serial number
> integration with the hid-multitouch protocol being the better long term
> solution for evdev, as that fits better with combined touch and stylus
> operation, as well as providing a clear means to providing multiple
> serial numbers for simultaneous styluses.
> 
> This set includes general modifications to hid-core to decode and
> represent HID fields larger than 32-bits as multiple consecutive 32-bit
> words, allowing arbitrarily long fields (or at least as many as can fit
> into HID_MAX_BUFFER_SIZE) to be passed upstream and (potentially)
> processed by hid-input and other subsystems.
> 
> (After trying a 64-bit implementation, I decided staying with 32-bit
> words kept the rest of the hid-core cleaner, and it was worth 
> supporting arbitrary length fields instead of only upgrading
> from 32 -> 64.)

Sorry for keeping this on hold for so long, it fell in between cracks.

This would need quite an extensive testing by Benjamin's testing 
infrastructure ... Benjamin, could you please put that into your queue?

Other than that, please rework the patchset so that it uses kernel comment 
styles, it can't be applied in this form.

Thanks,

-- 
Jiri Kosina
SUSE Labs