Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the reportdescriptors
From: Srinivas Pandruvada
Date: Wed Aug 14 2013 - 12:00:12 EST
On 08/13/2013 11:37 AM, Alexander Holler wrote:
Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
hid_scan_report() implements its own HID report descriptor parsing.
It was
fine until we added the SENSOR_HUB detection. It is going to be even
worse
with the detection of Win 8 certified touchscreen, as this detection
relies on a special feature and on the report_size and report_count
fields.
Sorry, but I can't agree with your wording here.
If you look at the if expression I've added to support HID sensor
hubs, you will notice that the first expression was a test for the
usage page of sensor hubs. That wasn't the first test by accident,
instead I've choosen the order of those 4 tests very carefully to make
the impact on the existing parsing of other HID devices very small. So
it might not have be pleasing to your eyes, but it was for sure an
appropriate solution.
Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the
detection of sensor hubs doesn't work anymore with your patch applied.
Up to now I only had a quick look at it, but it looks like the test in
hid_scan_open_collection() isn't hit for my device.
Another problem is that I don't have any commercial sensor hub and I'm
therefor not a very relvant as tester (I've implemented the firmware
for my HID (sensor hub) device too). Therefor I've added Srinivas
Pandruvada to cc, because he's the only one I know who has HID sensor
hubs.
Tested on 3.11-rc5. Sensor hubs are no longer detected. If you have some
patch to fix this, I can give a try.
And, as said, I've implemented the other side here too, therefor I've
added the descriptor I'm using below.
Regards,
Alexander Holler
--my-descriptor--
0x05, 0x20, // HID_USAGE_PAGE_SENSOR
0xa1, 0x01, // COLLECTION (Application)
0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
0xa1, 0x00, // COLLECTION (Physical)
#ifndef USE_FULL_YEAR
0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
0x95, 0x01, // REPORT_COUNT (1)
0x85, HID_REPORT_ID_TIME, // REPORT_ID
0x0a, 0x21, 0x05, // USAGE (Year)
#ifdef USE_FULL_YEAR
0x75, 0x10, // REPORT_SIZE (16 bits)
#else
0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not
specified in HUTRR39)
0x25, 0x63, // LOGICAL_MAXIMUM (99)
#endif
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x22, 0x05, // USAGE (Month)
#ifdef USE_FULL_YEAR
0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
0x15, 0x01, // LOGICAL_MINIMUM (1)
0x25, 0x0c, // LOGICAL_MAXIMUM (12)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x23, 0x05, // USAGE (Day)
0x15, 0x01, // LOGICAL_MINIMUM (1)
0x25, 0x1f, // LOGICAL_MAXIMUM (31)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x24, 0x05, // USAGE (Day of Week)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x06, // LOGICAL_MAXIMUM (6)
0xa1, 0x02, // COLLECTION (Logical)
0x0a, 0xc0, 0x08, // Day of Week: Sunday
0x0a, 0xc1, 0x08, // Day of Week: Monday
0x0a, 0xc2, 0x08, // Day of Week: Tuesday
0x0a, 0xc3, 0x08, // Day of Week: Wednesday
0x0a, 0xc4, 0x08, // Day of Week: Thursday
0x0a, 0xc5, 0x08, // Day of Week: Friday
0x0a, 0xc6, 0x08, // Day of Week: Saturday
0x81, 0x02, // INPUT (Const,Arr,Abs)
0xc0, // END_COLLECTION
0x0a, 0x25, 0x05, // USAGE (Hour)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x17, // LOGICAL_MAXIMUM (23)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x26, 0x05, // USAGE (Minute)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x3b, // LOGICAL_MAXIMUM (59)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x27, 0x05, // USAGE (Second)
0x15, 0x00, // LOGICAL_MINIMUM (0)
0x25, 0x3b, // LOGICAL_MAXIMUM (59)
0x65, 0x00, // UNIT (None)
//0x66, 0x10, 0x01, // UNIT (second)
0x81, 0x02, // INPUT (Data,Var,Abs)
0x0a, 0x28, 0x05, // USAGE (Millisecond)
0x75, 0x10, // REPORT_SIZE (16 bits)
0x65, 0x00, // UNIT (None)
0x81, 0x02, // INPUT (Data,Var,Abs)
0xc0, // END_COLLECTION
0xc0, // END_COLLECTION
--my-descriptor--
We can use the existing HID parser in hid-core for hid_scan_report()
by re-using the code from hid_open_report(). hid_parser_global,
hid_parser_local and hid_parser_reserved does not have any side effects.
We just need to reimplement the MAIN_ITEM callback to have a proper
parsing without side effects.
Instead of directly overwriting the ->group field, this patch introduce
a ->flags field and then decide which group the device belongs to,
depending on the whole parsing (not just the local item). This will be
useful for Win 8 multitouch devices, which are multitouch devices and
Win 8 certified (so 2 flags to check).
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
---
drivers/hid/hid-core.c | 131
+++++++++++++++++++++++++++++++++++--------------
include/linux/hid.h | 4 ++
2 files changed, 97 insertions(+), 38 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3efe19f..d8cdb0a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -677,10 +677,49 @@ static u8 *fetch_item(__u8 *start, __u8 *end,
struct hid_item *item)
return NULL;
}
-static void hid_scan_usage(struct hid_device *hid, u32 usage)
+static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
{
if (usage == HID_DG_CONTACTID)
- hid->group = HID_GROUP_MULTITOUCH;
+ parser->flags |= HID_FLAG_MULTITOUCH;
+}
+
+static void hid_scan_open_collection(struct hid_parser *parser,
unsigned type)
+{
+ if (parser->global.usage_page == HID_UP_SENSOR &&
+ type == HID_COLLECTION_PHYSICAL)
+ parser->flags |= HID_FLAG_SENSOR_HUB;
+}
+
+static int hid_scan_main(struct hid_parser *parser, struct hid_item
*item)
+{
+ __u32 data;
+ int i;
+
+ data = item_udata(item);
+
+ switch (item->tag) {
+ case HID_MAIN_ITEM_TAG_BEGIN_COLLECTION:
+ hid_scan_open_collection(parser, data & 0xff);
+ break;
+ case HID_MAIN_ITEM_TAG_END_COLLECTION:
+ break;
+ case HID_MAIN_ITEM_TAG_INPUT:
+ for (i = 0; i < parser->local.usage_index; i++)
+ hid_scan_input_usage(parser, parser->local.usage[i]);
+ break;
+ case HID_MAIN_ITEM_TAG_OUTPUT:
+ break;
+ case HID_MAIN_ITEM_TAG_FEATURE:
+ break;
+ default:
+ hid_err(parser->device, "unknown main item tag 0x%x\n",
+ item->tag);
+ }
+
+ /* Reset the local parser environment */
+ memset(&parser->local, 0, sizeof(parser->local));
+
+ return 0;
}
/*
@@ -690,49 +729,65 @@ static void hid_scan_usage(struct hid_device
*hid, u32 usage)
*/
static int hid_scan_report(struct hid_device *hid)
{
- unsigned int page = 0, delim = 0;
+ struct hid_parser *parser;
+ struct hid_item item;
__u8 *start = hid->dev_rdesc;
__u8 *end = start + hid->dev_rsize;
- unsigned int u, u_min = 0, u_max = 0;
- struct hid_item item;
+ int ret;
+ static int (*dispatch_type[])(struct hid_parser *parser,
+ struct hid_item *item) = {
+ hid_scan_main,
+ hid_parser_global,
+ hid_parser_local,
+ hid_parser_reserved
+ };
- hid->group = HID_GROUP_GENERIC;
+ parser = vzalloc(sizeof(struct hid_parser));
+ if (!parser)
+ return -ENOMEM;
+
+ parser->device = hid;
+
+ ret = -EINVAL;
while ((start = fetch_item(start, end, &item)) != NULL) {
- if (item.format != HID_ITEM_FORMAT_SHORT)
- return -EINVAL;
- if (item.type == HID_ITEM_TYPE_GLOBAL) {
- if (item.tag == HID_GLOBAL_ITEM_TAG_USAGE_PAGE)
- page = item_udata(&item) << 16;
- } else if (item.type == HID_ITEM_TYPE_LOCAL) {
- if (delim > 1)
- break;
- u = item_udata(&item);
- if (item.size <= 2)
- u += page;
- switch (item.tag) {
- case HID_LOCAL_ITEM_TAG_DELIMITER:
- delim += !!u;
- break;
- case HID_LOCAL_ITEM_TAG_USAGE:
- hid_scan_usage(hid, u);
- break;
- case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
- u_min = u;
- break;
- case HID_LOCAL_ITEM_TAG_USAGE_MAXIMUM:
- u_max = u;
- for (u = u_min; u <= u_max; u++)
- hid_scan_usage(hid, u);
- break;
+
+ if (item.format != HID_ITEM_FORMAT_SHORT) {
+ hid_err(hid, "unexpected long global item\n");
+ goto out;
+ }
+
+ if (dispatch_type[item.type](parser, &item)) {
+ hid_err(hid, "item %u %u %u %u parsing failed\n",
+ item.format, (unsigned)item.size,
+ (unsigned)item.type, (unsigned)item.tag);
+ goto out;
+ }
+
+ if (start == end) {
+ if (parser->local.delimiter_depth) {
+ hid_err(hid, "unbalanced delimiter at end of report
description\n");
+ goto out;
}
- } else if (page == HID_UP_SENSOR &&
- item.type == HID_ITEM_TYPE_MAIN &&
- item.tag == HID_MAIN_ITEM_TAG_BEGIN_COLLECTION &&
- (item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL)
- hid->group = HID_GROUP_SENSOR_HUB;
+ ret = 0;
+ goto out;
+ }
}
- return 0;
+ hid_err(hid, "item fetching failed at offset %d\n", (int)(end -
start));
+out:
+ switch (parser->flags) {
+ case HID_FLAG_MULTITOUCH:
+ hid->group = HID_GROUP_MULTITOUCH;
+ break;
+ case HID_FLAG_SENSOR_HUB:
+ hid->group = HID_GROUP_SENSOR_HUB;
+ break;
+ default:
+ hid->group = HID_GROUP_GENERIC;
+ }
+
+ vfree(parser);
+ return ret;
}
/**
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5a4e789..7d823db 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct
hid_device *hdev, void *data)
#define HID_GLOBAL_STACK_SIZE 4
#define HID_COLLECTION_STACK_SIZE 4
+#define HID_FLAG_MULTITOUCH 0x0001
+#define HID_FLAG_SENSOR_HUB 0x0002
+
struct hid_parser {
struct hid_global global;
struct hid_global global_stack[HID_GLOBAL_STACK_SIZE];
@@ -541,6 +544,7 @@ struct hid_parser {
unsigned collection_stack[HID_COLLECTION_STACK_SIZE];
unsigned collection_stack_ptr;
struct hid_device *device;
+ unsigned flags;
};
struct hid_class_descriptor {
Thanks,
Srinivas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/