Hi Henrik, On Wed, Aug 21, 2013 at 8:15 PM, Henrik Rydberg <rydb...@euromail.se> wrote: > Hi Benjamin, > > this looks pretty good to me, just a few nitpicks below. > >> hid_scan_report() implements its own HID report descriptor parsing. It is >> going to be really bad 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. > > How about 'The Win 8 detection is sufficiently complex to warrant use > of the full parser code, in spite of the inferred memory > usage. Therefore...' >
Sure. Will change in v3. >> >> 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. >> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com> >> --- >> >> changes in v2: >> - moved "flags" processing in patch 2/3 (so use hid->group in this patch) >> - hid_scan_report() is less verbose when errors are found in the descriptor >> - hid_scan_report() is tolerant to parsing errors >> - fixed usage_page handling in hid_scan_collection(), which fixes sensors >> detection >> - amended commit message >> >> drivers/hid/hid-core.c | 106 >> +++++++++++++++++++++++++++++++------------------ >> 1 file changed, 67 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> index 3efe19f..e072b15 100644 >> --- a/drivers/hid/hid-core.c >> +++ b/drivers/hid/hid-core.c >> @@ -677,12 +677,55 @@ 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) >> { >> + struct hid_device *hid = parser->device; >> + >> if (usage == HID_DG_CONTACTID) >> hid->group = HID_GROUP_MULTITOUCH; >> } >> >> +static void hid_scan_collection(struct hid_parser *parser, unsigned type) >> +{ >> + struct hid_device *hid = parser->device; >> + >> + if (((parser->global.usage_page << 16) == HID_UP_SENSOR) && >> + type == HID_COLLECTION_PHYSICAL) >> + hid->group = HID_GROUP_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_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); > > Looks this this message is a duplicate as well. oops, forgot to remove it :) Cheers, Benjamin > >> + } >> + >> + /* Reset the local parser environment */ >> + memset(&parser->local, 0, sizeof(parser->local)); >> + >> + return 0; >> +} >> + >> /* >> * Scan a report descriptor before the device is added to the bus. >> * Sets device groups and other properties that determine what driver >> @@ -690,48 +733,33 @@ 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; >> + 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; >> - 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; >> - } >> - } 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; >> - } >> + parser = vzalloc(sizeof(struct hid_parser)); >> + if (!parser) >> + return -ENOMEM; >> + >> + parser->device = hid; >> + >> + /* >> + * The parsing is simpler than the one in hid_open_report() as we >> should >> + * be robust against hid errors. Those errors will be raised by >> + * hid_open_report() anyway. >> + */ >> + while ((start = fetch_item(start, end, &item)) != NULL) >> + dispatch_type[item.type](parser, &item); >> >> + vfree(parser); >> return 0; >> } >> >> -- >> 1.8.3.1 >> > > Thanks, > Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html