diff mbox series

[3/5] HID: core: Export some report item parsing functions.

Message ID 20210228012643.69944-4-ronald@innovation.ch (mailing list archive)
State New, archived
Headers show
Series Touch Bar and ALS support for MacBook Pro's | expand

Commit Message

Life is hard, and then you die Feb. 28, 2021, 1:26 a.m. UTC 
These are useful to drivers that need to scan or parse reports
themselves.

Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
 drivers/hid/hid-core.c | 54 +++++++++++++++++++++++++-----------------
 include/linux/hid.h    |  4 ++++
 2 files changed, 36 insertions(+), 22 deletions(-)

Comments

Andy Shevchenko March 1, 2021, 2:18 p.m. UTC | #1 
On Sun, Feb 28, 2021 at 3:30 AM Ronald Tschalär <ronald@innovation.ch> wrote:
>
> These are useful to drivers that need to scan or parse reports
> themselves.

...

> -       while ((start = fetch_item(start, end, &item)) != NULL)
> +       while ((start = hid_fetch_item(start, end, &item)) != NULL)
>                 dispatch_type[item.type](parser, &item);

> -       while ((next = fetch_item(start, end, &item)) != NULL) {
> +       while ((next = hid_fetch_item(start, end, &item)) != NULL) {
>                 start = next;

I don't see the full picture, but perhaps you may also introduce
for_each_hid_item() or so.
Benjamin Tissoires March 1, 2021, 2:27 p.m. UTC | #2 
On Mon, Mar 1, 2021 at 3:18 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Feb 28, 2021 at 3:30 AM Ronald Tschalär <ronald@innovation.ch> wrote:
> >
> > These are useful to drivers that need to scan or parse reports
> > themselves.
>
> ...
>
> > -       while ((start = fetch_item(start, end, &item)) != NULL)
> > +       while ((start = hid_fetch_item(start, end, &item)) != NULL)
> >                 dispatch_type[item.type](parser, &item);
>
> > -       while ((next = fetch_item(start, end, &item)) != NULL) {
> > +       while ((next = hid_fetch_item(start, end, &item)) != NULL) {
> >                 start = next;
>
> I don't see the full picture, but perhaps you may also introduce
> for_each_hid_item() or so.

Same here, I don't see the full picture, but I would suggest to not
export those functions at all.

From 4/5, I can see that you are using them in
appleib_find_collection(), which is only called by
appleib_add_device(), which in turn is always called with a parsed and
started HID device. Why can you not rely on the core parsing and
iterate over the already parsed hid_field?

Cheers,
Benjamin
diff mbox series

Patch

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a96b252f97366..b4c3d71a0ddda 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -340,7 +340,7 @@  static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
  * Read data value from item.
  */
 
-static u32 item_udata(struct hid_item *item)
+u32 hid_item_udata(struct hid_item *item)
 {
 	switch (item->size) {
 	case 1: return item->data.u8;
@@ -349,8 +349,9 @@  static u32 item_udata(struct hid_item *item)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(hid_item_udata);
 
-static s32 item_sdata(struct hid_item *item)
+s32 hid_item_sdata(struct hid_item *item)
 {
 	switch (item->size) {
 	case 1: return item->data.s8;
@@ -359,6 +360,7 @@  static s32 item_sdata(struct hid_item *item)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(hid_item_sdata);
 
 /*
  * Process a global item.
@@ -391,29 +393,29 @@  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
-		parser->global.usage_page = item_udata(item);
+		parser->global.usage_page = hid_item_udata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
-		parser->global.logical_minimum = item_sdata(item);
+		parser->global.logical_minimum = hid_item_sdata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_LOGICAL_MAXIMUM:
 		if (parser->global.logical_minimum < 0)
-			parser->global.logical_maximum = item_sdata(item);
+			parser->global.logical_maximum = hid_item_sdata(item);
 		else
-			parser->global.logical_maximum = item_udata(item);
+			parser->global.logical_maximum = hid_item_udata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_PHYSICAL_MINIMUM:
-		parser->global.physical_minimum = item_sdata(item);
+		parser->global.physical_minimum = hid_item_sdata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_PHYSICAL_MAXIMUM:
 		if (parser->global.physical_minimum < 0)
-			parser->global.physical_maximum = item_sdata(item);
+			parser->global.physical_maximum = hid_item_sdata(item);
 		else
-			parser->global.physical_maximum = item_udata(item);
+			parser->global.physical_maximum = hid_item_udata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
@@ -421,7 +423,7 @@  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		 * nibble due to the common misunderstanding of HID
 		 * specification 1.11, 6.2.2.7 Global Items. Attempt to handle
 		 * both this and the standard encoding. */
-		raw_value = item_sdata(item);
+		raw_value = hid_item_sdata(item);
 		if (!(raw_value & 0xfffffff0))
 			parser->global.unit_exponent = hid_snto32(raw_value, 4);
 		else
@@ -429,11 +431,11 @@  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT:
-		parser->global.unit = item_udata(item);
+		parser->global.unit = hid_item_udata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_REPORT_SIZE:
-		parser->global.report_size = item_udata(item);
+		parser->global.report_size = hid_item_udata(item);
 		if (parser->global.report_size > 256) {
 			hid_err(parser->device, "invalid report_size %d\n",
 					parser->global.report_size);
@@ -442,7 +444,7 @@  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_REPORT_COUNT:
-		parser->global.report_count = item_udata(item);
+		parser->global.report_count = hid_item_udata(item);
 		if (parser->global.report_count > HID_MAX_USAGES) {
 			hid_err(parser->device, "invalid report_count %d\n",
 					parser->global.report_count);
@@ -451,7 +453,7 @@  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_REPORT_ID:
-		parser->global.report_id = item_udata(item);
+		parser->global.report_id = hid_item_udata(item);
 		if (parser->global.report_id == 0 ||
 		    parser->global.report_id >= HID_MAX_IDS) {
 			hid_err(parser->device, "report_id %u is invalid\n",
@@ -476,7 +478,7 @@  static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
 	unsigned n;
 	__u32 count;
 
-	data = item_udata(item);
+	data = hid_item_udata(item);
 
 	switch (item->tag) {
 	case HID_LOCAL_ITEM_TAG_DELIMITER:
@@ -608,7 +610,7 @@  static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
 
 	hid_concatenate_last_usage_page(parser);
 
-	data = item_udata(item);
+	data = hid_item_udata(item);
 
 	switch (item->tag) {
 	case HID_MAIN_ITEM_TAG_BEGIN_COLLECTION:
@@ -707,12 +709,19 @@  static void hid_device_release(struct device *dev)
 	kfree(hid);
 }
 
-/*
+/**
+ * hid_fetch_item - fetch an item from a report
+ *
+ * @start: the current position in the report buffer to read the next item from
+ * @end: the end of the report buffer
+ * @item: the item struct to fill in
+ *
  * Fetch a report description item from the data stream. We support long
  * items, though they are not used yet.
+ *
+ * Return: the position of the next item in the report
  */
-
-static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
+u8 *hid_fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 {
 	u8 b;
 
@@ -773,6 +782,7 @@  static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(hid_fetch_item);
 
 static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
 {
@@ -831,7 +841,7 @@  static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
 
 	hid_concatenate_last_usage_page(parser);
 
-	data = item_udata(item);
+	data = hid_item_udata(item);
 
 	switch (item->tag) {
 	case HID_MAIN_ITEM_TAG_BEGIN_COLLECTION:
@@ -891,7 +901,7 @@  static int hid_scan_report(struct hid_device *hid)
 	 * be robust against hid errors. Those errors will be raised by
 	 * hid_open_report() anyway.
 	 */
-	while ((start = fetch_item(start, end, &item)) != NULL)
+	while ((start = hid_fetch_item(start, end, &item)) != NULL)
 		dispatch_type[item.type](parser, &item);
 
 	/*
@@ -1250,7 +1260,7 @@  int hid_open_report(struct hid_device *device)
 	device->collection_size = HID_DEFAULT_NUM_COLLECTIONS;
 
 	ret = -EINVAL;
-	while ((next = fetch_item(start, end, &item)) != NULL) {
+	while ((next = hid_fetch_item(start, end, &item)) != NULL) {
 		start = next;
 
 		if (item.format != HID_ITEM_FORMAT_SHORT) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index c39d71eb1fd0a..919f595084610 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -898,6 +898,10 @@  struct hid_report *hid_validate_values(struct hid_device *hid,
 				       unsigned int field_index,
 				       unsigned int report_counts);
 
+u32 hid_item_udata(struct hid_item *item);
+s32 hid_item_sdata(struct hid_item *item);
+u8 *hid_fetch_item(__u8 *start, __u8 *end, struct hid_item *item);
+
 void hid_setup_resolution_multiplier(struct hid_device *hid);
 int hid_open_report(struct hid_device *device);
 int hid_check_keys_pressed(struct hid_device *hid);