* [PATCH v2 1/3] HID: Use hid_parser for pre-scanning the report descriptors
2013-08-21 15:40 [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Benjamin Tissoires
@ 2013-08-21 15:40 ` Benjamin Tissoires
2013-08-21 18:15 ` Henrik Rydberg
2013-08-21 15:40 ` [PATCH v2 2/3] HID: detect Win 8 multitouch devices in core Benjamin Tissoires
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2013-08-21 15:40 UTC (permalink / raw)
To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input, linux-kernel
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.
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.tissoires@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);
+ }
+
+ /* 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/3] HID: Use hid_parser for pre-scanning the report descriptors
2013-08-21 15:40 ` [PATCH v2 1/3] HID: Use hid_parser for pre-scanning the report descriptors Benjamin Tissoires
@ 2013-08-21 18:15 ` Henrik Rydberg
2013-08-22 8:31 ` Benjamin Tissoires
0 siblings, 1 reply; 10+ messages in thread
From: Henrik Rydberg @ 2013-08-21 18:15 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input, linux-kernel
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...'
>
> 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.tissoires@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.
> + }
> +
> + /* 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/3] HID: Use hid_parser for pre-scanning the report descriptors
2013-08-21 18:15 ` Henrik Rydberg
@ 2013-08-22 8:31 ` Benjamin Tissoires
0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Tissoires @ 2013-08-22 8:31 UTC (permalink / raw)
To: Henrik Rydberg
Cc: Benjamin Tissoires, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input,
linux-kernel@vger.kernel.org
Hi Henrik,
On Wed, Aug 21, 2013 at 8:15 PM, Henrik Rydberg <rydberg@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.tissoires@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] HID: detect Win 8 multitouch devices in core
2013-08-21 15:40 [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Benjamin Tissoires
2013-08-21 15:40 ` [PATCH v2 1/3] HID: Use hid_parser for pre-scanning the report descriptors Benjamin Tissoires
@ 2013-08-21 15:40 ` Benjamin Tissoires
2013-08-21 18:26 ` Henrik Rydberg
2013-08-21 15:40 ` [PATCH v2 3/3] HID: do not init input reports for Win 8 multitouch devices Benjamin Tissoires
2013-08-21 21:24 ` [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Srinivas Pandruvada
3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2013-08-21 15:40 UTC (permalink / raw)
To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input, linux-kernel
Detecting Win 8 multitouch devices in core allows us to set quirks
before the device is parsed through hid_hw_start().
It also simplifies the detection of those devices in hid-multitouch and
makes the handling of those devices cleaner.
As Win 8 multitouch panels are in the group multitouch and rely on a
special feature to be detected, this patch also temporary convert
hid->group into a bitfield, before reverting it to the actual
group information. If we ever have more than 16 groups to detect,
we can then add a .flags field in hid_parser for instance, and then
use this to get the actual group.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
changes in v2:
- moved "flags" processing here
- do not introduce parser->flags, but use hid->group as a temporary flag placeholder
- #define and #undef HID_FLAG_* in hid-core.c instead of hid.h
- amended commit message
drivers/hid/hid-core.c | 41 +++++++++++++++++++++++++++++++++++++++--
drivers/hid/hid-multitouch.c | 24 +++++++++++-------------
include/linux/hid.h | 1 +
3 files changed, 51 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index e072b15..c749966 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -677,12 +677,25 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
return NULL;
}
+#define HID_FLAG_MULTITOUCH (1 << HID_GROUP_MULTITOUCH)
+#define HID_FLAG_WIN_8_CERTIFIED (1 << HID_GROUP_MULTITOUCH_WIN_8)
+#define HID_FLAG_SENSOR_HUB (1 << HID_GROUP_SENSOR_HUB)
+
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;
+ hid->group |= HID_FLAG_MULTITOUCH;
+}
+
+static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
+{
+ struct hid_device *hid = parser->device;
+
+ if (usage == 0xff0000c5 && parser->global.report_count == 256 &&
+ parser->global.report_size == 8)
+ hid->group |= HID_FLAG_WIN_8_CERTIFIED;
}
static void hid_scan_collection(struct hid_parser *parser, unsigned type)
@@ -691,7 +704,7 @@ static void hid_scan_collection(struct hid_parser *parser, unsigned type)
if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
type == HID_COLLECTION_PHYSICAL)
- hid->group = HID_GROUP_SENSOR_HUB;
+ hid->group |= HID_FLAG_SENSOR_HUB;
}
static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
@@ -714,6 +727,8 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
case HID_MAIN_ITEM_TAG_OUTPUT:
break;
case HID_MAIN_ITEM_TAG_FEATURE:
+ for (i = 0; i < parser->local.usage_index; i++)
+ hid_scan_feature_usage(parser, parser->local.usage[i]);
break;
default:
hid_err(parser->device, "unknown main item tag 0x%x\n",
@@ -759,10 +774,32 @@ static int hid_scan_report(struct hid_device *hid)
while ((start = fetch_item(start, end, &item)) != NULL)
dispatch_type[item.type](parser, &item);
+ /*
+ * Re-interprete the group field to keep the same group definitions than
+ * we had in previous kernels.
+ */
+ switch (hid->group) {
+ case HID_FLAG_MULTITOUCH:
+ hid->group = HID_GROUP_MULTITOUCH;
+ break;
+ case HID_FLAG_MULTITOUCH | HID_FLAG_WIN_8_CERTIFIED:
+ hid->group = HID_GROUP_MULTITOUCH_WIN_8;
+ break;
+ case HID_FLAG_SENSOR_HUB:
+ hid->group = HID_GROUP_SENSOR_HUB;
+ break;
+ default:
+ hid->group = HID_GROUP_GENERIC;
+ }
+
vfree(parser);
return 0;
}
+#undef HID_FLAG_MULTITOUCH
+#undef HID_FLAG_WIN_8_CERTIFIED
+#undef HID_FLAG_SENSOR_HUB
+
/**
* hid_parse_report - parse device report
*
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0fe00e2..c28ef86 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -133,6 +133,7 @@ static void mt_post_parse(struct mt_device *td);
#define MT_CLS_NSMU 0x000a
#define MT_CLS_DUAL_CONTACT_NUMBER 0x0010
#define MT_CLS_DUAL_CONTACT_ID 0x0011
+#define MT_CLS_WIN_8 0x0012
/* vendor specific classes */
#define MT_CLS_3M 0x0101
@@ -205,6 +206,11 @@ static struct mt_class mt_classes[] = {
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_SLOT_IS_CONTACTID,
.maxcontacts = 2 },
+ { .name = MT_CLS_WIN_8,
+ .quirks = MT_QUIRK_ALWAYS_VALID |
+ MT_QUIRK_IGNORE_DUPLICATES |
+ MT_QUIRK_HOVERING |
+ MT_QUIRK_CONTACT_CNT_ACCURATE },
/*
* vendor specific classes
@@ -332,19 +338,6 @@ static void mt_feature_mapping(struct hid_device *hdev,
td->maxcontacts = td->mtclass.maxcontacts;
break;
- case 0xff0000c5:
- if (field->report_count == 256 && field->report_size == 8) {
- /* Win 8 devices need special quirks */
- __s32 *quirks = &td->mtclass.quirks;
- *quirks |= MT_QUIRK_ALWAYS_VALID;
- *quirks |= MT_QUIRK_IGNORE_DUPLICATES;
- *quirks |= MT_QUIRK_HOVERING;
- *quirks |= MT_QUIRK_CONTACT_CNT_ACCURATE;
- *quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
- *quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
- *quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
- }
- break;
}
}
@@ -1346,6 +1339,11 @@ static const struct hid_device_id mt_devices[] = {
/* Generic MT device */
{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
+
+ /* Generic Win 8 certified MT device */
+ { .driver_data = MT_CLS_WIN_8,
+ HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8,
+ HID_ANY_ID, HID_ANY_ID) },
{ }
};
MODULE_DEVICE_TABLE(hid, mt_devices);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5a4e789..45b7f3f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -295,6 +295,7 @@ struct hid_item {
#define HID_GROUP_GENERIC 0x0001
#define HID_GROUP_MULTITOUCH 0x0002
#define HID_GROUP_SENSOR_HUB 0x0003
+#define HID_GROUP_MULTITOUCH_WIN_8 0x0004
/*
* This is the global environment of the parser. This information is
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] HID: detect Win 8 multitouch devices in core
2013-08-21 15:40 ` [PATCH v2 2/3] HID: detect Win 8 multitouch devices in core Benjamin Tissoires
@ 2013-08-21 18:26 ` Henrik Rydberg
2013-08-22 12:01 ` Benjamin Tissoires
0 siblings, 1 reply; 10+ messages in thread
From: Henrik Rydberg @ 2013-08-21 18:26 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input, linux-kernel
Hi Benjamin,
> Detecting Win 8 multitouch devices in core allows us to set quirks
> before the device is parsed through hid_hw_start().
> It also simplifies the detection of those devices in hid-multitouch and
> makes the handling of those devices cleaner.
>
> As Win 8 multitouch panels are in the group multitouch and rely on a
> special feature to be detected, this patch also temporary convert
> hid->group into a bitfield, before reverting it to the actual
> group information. If we ever have more than 16 groups to detect,
> we can then add a .flags field in hid_parser for instance, and then
> use this to get the actual group.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> changes in v2:
> - moved "flags" processing here
> - do not introduce parser->flags, but use hid->group as a temporary flag placeholder
> - #define and #undef HID_FLAG_* in hid-core.c instead of hid.h
> - amended commit message
>
> drivers/hid/hid-core.c | 41 +++++++++++++++++++++++++++++++++++++++--
> drivers/hid/hid-multitouch.c | 24 +++++++++++-------------
> include/linux/hid.h | 1 +
> 3 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index e072b15..c749966 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -677,12 +677,25 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
> return NULL;
> }
>
> +#define HID_FLAG_MULTITOUCH (1 << HID_GROUP_MULTITOUCH)
> +#define HID_FLAG_WIN_8_CERTIFIED (1 << HID_GROUP_MULTITOUCH_WIN_8)
> +#define HID_FLAG_SENSOR_HUB (1 << HID_GROUP_SENSOR_HUB)
> +
Why not use the existing defines directly? If you worry about being
able to combine bit positions in a simple mask rather than using the
existing bitmask (which could be good), how about a priority list for
the groups instead? My own preference would be to leave structure for
the future to the future, and use a single state in the parser for the
case you are adding.
> 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;
> + hid->group |= HID_FLAG_MULTITOUCH;
> +}
Dynamic semantics, please no :-) My suggestion: leave this line be...
> +
> +static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
> +{
> + struct hid_device *hid = parser->device;
> +
> + if (usage == 0xff0000c5 && parser->global.report_count == 256 &&
> + parser->global.report_size == 8)
> + hid->group |= HID_FLAG_WIN_8_CERTIFIED;
And set 'parser->is_win8 = true' here.
> }
>
> static void hid_scan_collection(struct hid_parser *parser, unsigned type)
> @@ -691,7 +704,7 @@ static void hid_scan_collection(struct hid_parser *parser, unsigned type)
>
> if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
> type == HID_COLLECTION_PHYSICAL)
> - hid->group = HID_GROUP_SENSOR_HUB;
> + hid->group |= HID_FLAG_SENSOR_HUB;
Leave this...
> }
>
> static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
> @@ -714,6 +727,8 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
> case HID_MAIN_ITEM_TAG_OUTPUT:
> break;
> case HID_MAIN_ITEM_TAG_FEATURE:
> + for (i = 0; i < parser->local.usage_index; i++)
> + hid_scan_feature_usage(parser, parser->local.usage[i]);
> break;
> default:
> hid_err(parser->device, "unknown main item tag 0x%x\n",
> @@ -759,10 +774,32 @@ static int hid_scan_report(struct hid_device *hid)
> while ((start = fetch_item(start, end, &item)) != NULL)
> dispatch_type[item.type](parser, &item);
>
> + /*
> + * Re-interprete the group field to keep the same group definitions than
> + * we had in previous kernels.
> + */
> + switch (hid->group) {
> + case HID_FLAG_MULTITOUCH:
> + hid->group = HID_GROUP_MULTITOUCH;
> + break;
> + case HID_FLAG_MULTITOUCH | HID_FLAG_WIN_8_CERTIFIED:
> + hid->group = HID_GROUP_MULTITOUCH_WIN_8;
> + break;
> + case HID_FLAG_SENSOR_HUB:
> + hid->group = HID_GROUP_SENSOR_HUB;
> + break;
> + default:
> + hid->group = HID_GROUP_GENERIC;
> + }
> +
... and replace this with 'if (parser->is_win8) hid->group = HID_GROUP_MULTITOUCH_WIN_8'.
> vfree(parser);
> return 0;
> }
>
> +#undef HID_FLAG_MULTITOUCH
> +#undef HID_FLAG_WIN_8_CERTIFIED
> +#undef HID_FLAG_SENSOR_HUB
> +
No comment ;-)
> /**
> * hid_parse_report - parse device report
> *
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 0fe00e2..c28ef86 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -133,6 +133,7 @@ static void mt_post_parse(struct mt_device *td);
> #define MT_CLS_NSMU 0x000a
> #define MT_CLS_DUAL_CONTACT_NUMBER 0x0010
> #define MT_CLS_DUAL_CONTACT_ID 0x0011
> +#define MT_CLS_WIN_8 0x0012
>
> /* vendor specific classes */
> #define MT_CLS_3M 0x0101
> @@ -205,6 +206,11 @@ static struct mt_class mt_classes[] = {
> MT_QUIRK_CONTACT_CNT_ACCURATE |
> MT_QUIRK_SLOT_IS_CONTACTID,
> .maxcontacts = 2 },
> + { .name = MT_CLS_WIN_8,
> + .quirks = MT_QUIRK_ALWAYS_VALID |
> + MT_QUIRK_IGNORE_DUPLICATES |
> + MT_QUIRK_HOVERING |
> + MT_QUIRK_CONTACT_CNT_ACCURATE },
>
> /*
> * vendor specific classes
> @@ -332,19 +338,6 @@ static void mt_feature_mapping(struct hid_device *hdev,
> td->maxcontacts = td->mtclass.maxcontacts;
>
> break;
> - case 0xff0000c5:
> - if (field->report_count == 256 && field->report_size == 8) {
> - /* Win 8 devices need special quirks */
> - __s32 *quirks = &td->mtclass.quirks;
> - *quirks |= MT_QUIRK_ALWAYS_VALID;
> - *quirks |= MT_QUIRK_IGNORE_DUPLICATES;
> - *quirks |= MT_QUIRK_HOVERING;
> - *quirks |= MT_QUIRK_CONTACT_CNT_ACCURATE;
> - *quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
> - *quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
> - *quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
> - }
> - break;
> }
> }
>
> @@ -1346,6 +1339,11 @@ static const struct hid_device_id mt_devices[] = {
>
> /* Generic MT device */
> { HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
> +
> + /* Generic Win 8 certified MT device */
> + { .driver_data = MT_CLS_WIN_8,
> + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8,
> + HID_ANY_ID, HID_ANY_ID) },
> { }
> };
> MODULE_DEVICE_TABLE(hid, mt_devices);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5a4e789..45b7f3f 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -295,6 +295,7 @@ struct hid_item {
> #define HID_GROUP_GENERIC 0x0001
> #define HID_GROUP_MULTITOUCH 0x0002
> #define HID_GROUP_SENSOR_HUB 0x0003
> +#define HID_GROUP_MULTITOUCH_WIN_8 0x0004
>
> /*
> * This is the global environment of the parser. This information is
> --
> 1.8.3.1
>
Thanks,
Henrik
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] HID: detect Win 8 multitouch devices in core
2013-08-21 18:26 ` Henrik Rydberg
@ 2013-08-22 12:01 ` Benjamin Tissoires
0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Tissoires @ 2013-08-22 12:01 UTC (permalink / raw)
To: Henrik Rydberg
Cc: Benjamin Tissoires, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input,
linux-kernel@vger.kernel.org
Hi Henrik,
On Wed, Aug 21, 2013 at 8:26 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Detecting Win 8 multitouch devices in core allows us to set quirks
>> before the device is parsed through hid_hw_start().
>> It also simplifies the detection of those devices in hid-multitouch and
>> makes the handling of those devices cleaner.
>>
>> As Win 8 multitouch panels are in the group multitouch and rely on a
>> special feature to be detected, this patch also temporary convert
>> hid->group into a bitfield, before reverting it to the actual
>> group information. If we ever have more than 16 groups to detect,
>> we can then add a .flags field in hid_parser for instance, and then
>> use this to get the actual group.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>
>> changes in v2:
>> - moved "flags" processing here
>> - do not introduce parser->flags, but use hid->group as a temporary flag placeholder
>> - #define and #undef HID_FLAG_* in hid-core.c instead of hid.h
>> - amended commit message
>>
>> drivers/hid/hid-core.c | 41 +++++++++++++++++++++++++++++++++++++++--
>> drivers/hid/hid-multitouch.c | 24 +++++++++++-------------
>> include/linux/hid.h | 1 +
>> 3 files changed, 51 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index e072b15..c749966 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -677,12 +677,25 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
>> return NULL;
>> }
>>
>> +#define HID_FLAG_MULTITOUCH (1 << HID_GROUP_MULTITOUCH)
>> +#define HID_FLAG_WIN_8_CERTIFIED (1 << HID_GROUP_MULTITOUCH_WIN_8)
>> +#define HID_FLAG_SENSOR_HUB (1 << HID_GROUP_SENSOR_HUB)
>> +
>
> Why not use the existing defines directly? If you worry about being
Just because I need to check both ((1 << HID_GROUP_MULTITOUCH_WIN_8)
&& (1 << HID_GROUP_MULTITOUCH)) which starts being a little bit
difficult to read without the defines.
> able to combine bit positions in a simple mask rather than using the
> existing bitmask (which could be good), how about a priority list for
> the groups instead? My own preference would be to leave structure for
> the future to the future, and use a single state in the parser for the
> case you are adding.
I'd go with a scanning_flags (so a bitmask) in the parser. This will
allows us to add other special case later if required. I don't really
like the idea of having just a boolean is_win8 because IMO the name
should be more explicit, and this will encourage people to add various
flags in the parser.
>
>> 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;
>> + hid->group |= HID_FLAG_MULTITOUCH;
>> +}
>
> Dynamic semantics, please no :-) My suggestion: leave this line be...
ok, will revert it
>
>> +
>> +static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
>> +{
>> + struct hid_device *hid = parser->device;
>> +
>> + if (usage == 0xff0000c5 && parser->global.report_count == 256 &&
>> + parser->global.report_size == 8)
>> + hid->group |= HID_FLAG_WIN_8_CERTIFIED;
>
> And set 'parser->is_win8 = true' here.
>
Again, I'd prefer to go with "parser->scanning_flags |=
HID_SCAN_FLAG_WIN_8_MT_PANEL" (or sth better for the name).
>> }
>>
>> static void hid_scan_collection(struct hid_parser *parser, unsigned type)
>> @@ -691,7 +704,7 @@ static void hid_scan_collection(struct hid_parser *parser, unsigned type)
>>
>> if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
>> type == HID_COLLECTION_PHYSICAL)
>> - hid->group = HID_GROUP_SENSOR_HUB;
>> + hid->group |= HID_FLAG_SENSOR_HUB;
>
> Leave this...
ok
>
>> }
>>
>> static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
>> @@ -714,6 +727,8 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
>> case HID_MAIN_ITEM_TAG_OUTPUT:
>> break;
>> case HID_MAIN_ITEM_TAG_FEATURE:
>> + for (i = 0; i < parser->local.usage_index; i++)
>> + hid_scan_feature_usage(parser, parser->local.usage[i]);
>> break;
>> default:
>> hid_err(parser->device, "unknown main item tag 0x%x\n",
>> @@ -759,10 +774,32 @@ static int hid_scan_report(struct hid_device *hid)
>> while ((start = fetch_item(start, end, &item)) != NULL)
>> dispatch_type[item.type](parser, &item);
>>
>> + /*
>> + * Re-interprete the group field to keep the same group definitions than
>> + * we had in previous kernels.
>> + */
>> + switch (hid->group) {
>> + case HID_FLAG_MULTITOUCH:
>> + hid->group = HID_GROUP_MULTITOUCH;
>> + break;
>> + case HID_FLAG_MULTITOUCH | HID_FLAG_WIN_8_CERTIFIED:
>> + hid->group = HID_GROUP_MULTITOUCH_WIN_8;
>> + break;
>> + case HID_FLAG_SENSOR_HUB:
>> + hid->group = HID_GROUP_SENSOR_HUB;
>> + break;
>> + default:
>> + hid->group = HID_GROUP_GENERIC;
>> + }
>> +
>
> ... and replace this with 'if (parser->is_win8) hid->group = HID_GROUP_MULTITOUCH_WIN_8'.
The test should be:
if ((hid->group == HID_GROUP_MULTITOUCH) && (parser->scanning_flags &
HID_SCAN_FLAG_WIN_8_MT_PANEL))
>
>> vfree(parser);
>> return 0;
>> }
>>
>> +#undef HID_FLAG_MULTITOUCH
>> +#undef HID_FLAG_WIN_8_CERTIFIED
>> +#undef HID_FLAG_SENSOR_HUB
>> +
>
> No comment ;-)
alright. I tried :)
Cheers,
Benjamin
>
>> /**
>> * hid_parse_report - parse device report
>> *
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 0fe00e2..c28ef86 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -133,6 +133,7 @@ static void mt_post_parse(struct mt_device *td);
>> #define MT_CLS_NSMU 0x000a
>> #define MT_CLS_DUAL_CONTACT_NUMBER 0x0010
>> #define MT_CLS_DUAL_CONTACT_ID 0x0011
>> +#define MT_CLS_WIN_8 0x0012
>>
>> /* vendor specific classes */
>> #define MT_CLS_3M 0x0101
>> @@ -205,6 +206,11 @@ static struct mt_class mt_classes[] = {
>> MT_QUIRK_CONTACT_CNT_ACCURATE |
>> MT_QUIRK_SLOT_IS_CONTACTID,
>> .maxcontacts = 2 },
>> + { .name = MT_CLS_WIN_8,
>> + .quirks = MT_QUIRK_ALWAYS_VALID |
>> + MT_QUIRK_IGNORE_DUPLICATES |
>> + MT_QUIRK_HOVERING |
>> + MT_QUIRK_CONTACT_CNT_ACCURATE },
>>
>> /*
>> * vendor specific classes
>> @@ -332,19 +338,6 @@ static void mt_feature_mapping(struct hid_device *hdev,
>> td->maxcontacts = td->mtclass.maxcontacts;
>>
>> break;
>> - case 0xff0000c5:
>> - if (field->report_count == 256 && field->report_size == 8) {
>> - /* Win 8 devices need special quirks */
>> - __s32 *quirks = &td->mtclass.quirks;
>> - *quirks |= MT_QUIRK_ALWAYS_VALID;
>> - *quirks |= MT_QUIRK_IGNORE_DUPLICATES;
>> - *quirks |= MT_QUIRK_HOVERING;
>> - *quirks |= MT_QUIRK_CONTACT_CNT_ACCURATE;
>> - *quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
>> - *quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
>> - *quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
>> - }
>> - break;
>> }
>> }
>>
>> @@ -1346,6 +1339,11 @@ static const struct hid_device_id mt_devices[] = {
>>
>> /* Generic MT device */
>> { HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
>> +
>> + /* Generic Win 8 certified MT device */
>> + { .driver_data = MT_CLS_WIN_8,
>> + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8,
>> + HID_ANY_ID, HID_ANY_ID) },
>> { }
>> };
>> MODULE_DEVICE_TABLE(hid, mt_devices);
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 5a4e789..45b7f3f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -295,6 +295,7 @@ struct hid_item {
>> #define HID_GROUP_GENERIC 0x0001
>> #define HID_GROUP_MULTITOUCH 0x0002
>> #define HID_GROUP_SENSOR_HUB 0x0003
>> +#define HID_GROUP_MULTITOUCH_WIN_8 0x0004
>>
>> /*
>> * This is the global environment of the parser. This information is
>> --
>> 1.8.3.1
>>
>
> Thanks,
> Henrik
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] HID: do not init input reports for Win 8 multitouch devices
2013-08-21 15:40 [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Benjamin Tissoires
2013-08-21 15:40 ` [PATCH v2 1/3] HID: Use hid_parser for pre-scanning the report descriptors Benjamin Tissoires
2013-08-21 15:40 ` [PATCH v2 2/3] HID: detect Win 8 multitouch devices in core Benjamin Tissoires
@ 2013-08-21 15:40 ` Benjamin Tissoires
2013-08-21 21:24 ` [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Srinivas Pandruvada
3 siblings, 0 replies; 10+ messages in thread
From: Benjamin Tissoires @ 2013-08-21 15:40 UTC (permalink / raw)
To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
Alexander Holler, Srinivas Pandruvada, linux-input, linux-kernel
Some multitouch screens do not like to be polled for input reports.
However, the Win8 spec says that all touches should be sent during
each report, making the initialization of reports unnecessary.
The Win7 spec is less precise, so do not use this for those devices.
Add the quirk HID_QUIRK_NO_INIT_INPUT_REPORTS so that we do not have to
introduce a quirk for each problematic device. This quirk makes the driver
behave the same way the Win 8 does. It actually retrieves the features,
but not the inputs.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
no changes in v2
drivers/hid/hid-multitouch.c | 12 ++++++++++++
drivers/hid/usbhid/hid-core.c | 11 ++++++++---
include/linux/hid.h | 1 +
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c28ef86..ac28f08 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -951,6 +951,18 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
hdev->quirks |= HID_QUIRK_MULTI_INPUT;
hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
+ /*
+ * Handle special quirks for Windows 8 certified devices.
+ */
+ if (id->group == HID_GROUP_MULTITOUCH_WIN_8)
+ /*
+ * Some multitouch screens do not like to be polled for input
+ * reports. Fortunately, the Win8 spec says that all touches
+ * should be sent during each report, making the initialization
+ * of input reports unnecessary.
+ */
+ hdev->quirks |= HID_QUIRK_NO_INIT_INPUT_REPORTS;
+
td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
if (!td) {
dev_err(&hdev->dev, "cannot allocate multitouch data\n");
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index bd38cdf..44df131 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -750,12 +750,17 @@ void usbhid_init_reports(struct hid_device *hid)
{
struct hid_report *report;
struct usbhid_device *usbhid = hid->driver_data;
+ struct hid_report_enum *report_enum;
int err, ret;
- list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].report_list, list)
- usbhid_submit_report(hid, report, USB_DIR_IN);
+ if (!(hid->quirks & HID_QUIRK_NO_INIT_INPUT_REPORTS)) {
+ report_enum = &hid->report_enum[HID_INPUT_REPORT];
+ list_for_each_entry(report, &report_enum->report_list, list)
+ usbhid_submit_report(hid, report, USB_DIR_IN);
+ }
- list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
+ report_enum = &hid->report_enum[HID_FEATURE_REPORT];
+ list_for_each_entry(report, &report_enum->report_list, list)
usbhid_submit_report(hid, report, USB_DIR_IN);
err = 0;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 45b7f3f..654627d 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -283,6 +283,7 @@ struct hid_item {
#define HID_QUIRK_MULTI_INPUT 0x00000040
#define HID_QUIRK_HIDINPUT_FORCE 0x00000080
#define HID_QUIRK_NO_EMPTY_INPUT 0x00000100
+#define HID_QUIRK_NO_INIT_INPUT_REPORTS 0x00000200
#define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
#define HID_QUIRK_FULLSPEED_INTERVAL 0x10000000
#define HID_QUIRK_NO_INIT_REPORTS 0x20000000
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core
2013-08-21 15:40 [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Benjamin Tissoires
` (2 preceding siblings ...)
2013-08-21 15:40 ` [PATCH v2 3/3] HID: do not init input reports for Win 8 multitouch devices Benjamin Tissoires
@ 2013-08-21 21:24 ` Srinivas Pandruvada
2013-08-22 8:37 ` Benjamin Tissoires
3 siblings, 1 reply; 10+ messages in thread
From: Srinivas Pandruvada @ 2013-08-21 21:24 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
Alexander Holler, linux-input, linux-kernel
On 08/21/2013 08:40 AM, Benjamin Tissoires wrote:
> Hi guys,
>
> this is the v2 of the rework of the pre-scanning of the hid report descriptors.
> This allows us to be able to detect Win 8 multitouch panels.
> I tried to take into account all of the previous reviews, and I think the patch
> series is in a better shape now.
>
> Alexander, Srinivas, could you please review/test patches 1/3 and 2/3 as they
> will both impact hid_sensor_hub detection now. From the report descriptors
> Alexander sent, I would say that it will work now, but it's always better to
> have different testers :)
Tested sensor hub. It works.
>
> Cheers,
> Benjamin
>
> Changes in v2:
> - moved "flags" processing in patch 2/3
> - do not introduce parser->flags, but use hid->group as a temporary flag placeholder
> - 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 messages
> - #define and #undef HID_FLAG_* in hid-core.c instead of hid.h
>
> Benjamin Tissoires (3):
> HID: Use hid_parser for pre-scanning the report descriptors
> HID: detect Win 8 multitouch devices in core
> HID: do not init input reports for Win 8 multitouch devices
>
> drivers/hid/hid-core.c | 143 ++++++++++++++++++++++++++++++------------
> drivers/hid/hid-multitouch.c | 36 +++++++----
> drivers/hid/usbhid/hid-core.c | 11 +++-
> include/linux/hid.h | 2 +
> 4 files changed, 137 insertions(+), 55 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core
2013-08-21 21:24 ` [PATCH v2 0/3] HID: Win 8 multitouch panels detection in core Srinivas Pandruvada
@ 2013-08-22 8:37 ` Benjamin Tissoires
0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Tissoires @ 2013-08-22 8:37 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
Alexander Holler, linux-input, linux-kernel@vger.kernel.org
On Wed, Aug 21, 2013 at 11:24 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On 08/21/2013 08:40 AM, Benjamin Tissoires wrote:
>>
>> Hi guys,
>>
>> this is the v2 of the rework of the pre-scanning of the hid report
>> descriptors.
>> This allows us to be able to detect Win 8 multitouch panels.
>> I tried to take into account all of the previous reviews, and I think the
>> patch
>> series is in a better shape now.
>>
>> Alexander, Srinivas, could you please review/test patches 1/3 and 2/3 as
>> they
>> will both impact hid_sensor_hub detection now. From the report descriptors
>> Alexander sent, I would say that it will work now, but it's always better
>> to
>> have different testers :)
>
> Tested sensor hub. It works.
Thanks for the test Srinivas :)
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 10+ messages in thread