* [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver
@ 2014-11-24 23:37 Andrew Duggan
2014-12-09 0:16 ` Andrew Duggan
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Duggan @ 2014-11-24 23:37 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Andrew Duggan, Jiri Kosina, Benjamin Tissoires
On composite HID devices there may be multiple HID devices on separate interfaces, but hid-rmi
should only bind to the touchpad. The previous version simply checked that the interface protocol
was set to mouse. Unfortuately, it is not always the case that the touchpad has the mouse interface
protocol set. This patch takes a different approach and scans the report descriptor looking for
the Generic Desktop Pointer usage and the Vendor Specific Top Level Collection needed by the
hid-rmi driver to interface with the device.
Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
This is a second attempt at the patch I submitted back in October. Instead of looking for specific
HID reports this patch looks for the Generic Desktop Pointer usage. Having that usage along with
the vendor specific collection seems to be a distinctive enough to id the Synaptics touchpad in
the composite USB device.
drivers/hid/hid-core.c | 21 ++++++++++++++++-----
include/linux/hid.h | 4 +++-
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 12b6e67..ba9dc59 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -698,10 +698,20 @@ static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
static void hid_scan_collection(struct hid_parser *parser, unsigned type)
{
struct hid_device *hid = parser->device;
+ int i;
if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
type == HID_COLLECTION_PHYSICAL)
hid->group = HID_GROUP_SENSOR_HUB;
+
+ if ((parser->global.usage_page << 16) == HID_UP_GENDESK)
+ for (i = 0; i < parser->local.usage_index; i++)
+ if (parser->local.usage[i] == HID_GD_POINTER)
+ parser->scan_flags
+ |= HID_SCAN_FLAG_GENDESK_POINTER;
+
+ if ((parser->global.usage_page << 16) == HID_UP_MSVENDOR)
+ parser->scan_flags |= HID_SCAN_FLAG_VENDOR_SPECIFIC;
}
static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
@@ -783,11 +793,12 @@ static int hid_scan_report(struct hid_device *hid)
* Vendor specific handlings
*/
if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
- (hid->group == HID_GROUP_GENERIC) &&
- /* only bind to the mouse interface of composite USB devices */
- (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
- /* hid-rmi should take care of them, not hid-generic */
- hid->group = HID_GROUP_RMI;
+ (hid->group == HID_GROUP_GENERIC)) {
+ if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
+ && (parser->scan_flags & HID_SCAN_FLAG_GENDESK_POINTER))
+ /* hid-rmi should take care of them, not hid-generic */
+ hid->group = HID_GROUP_RMI;
+ }
/*
* Vendor specific handlings
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f53c4a9..b019f15 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -547,7 +547,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_SCAN_FLAG_MT_WIN_8 0x00000001
+#define HID_SCAN_FLAG_MT_WIN_8 BIT(0)
+#define HID_SCAN_FLAG_VENDOR_SPECIFIC BIT(1)
+#define HID_SCAN_FLAG_GENDESK_POINTER BIT(2)
struct hid_parser {
struct hid_global global;
--
2.1.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver
2014-11-24 23:37 [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver Andrew Duggan
@ 2014-12-09 0:16 ` Andrew Duggan
2014-12-10 20:42 ` Benjamin Tissoires
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Duggan @ 2014-12-09 0:16 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Jiri Kosina, Benjamin Tissoires
On 11/24/2014 03:37 PM, Andrew Duggan wrote:
> On composite HID devices there may be multiple HID devices on separate interfaces, but hid-rmi
> should only bind to the touchpad. The previous version simply checked that the interface protocol
> was set to mouse. Unfortuately, it is not always the case that the touchpad has the mouse interface
> protocol set. This patch takes a different approach and scans the report descriptor looking for
> the Generic Desktop Pointer usage and the Vendor Specific Top Level Collection needed by the
> hid-rmi driver to interface with the device.
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> ---
> This is a second attempt at the patch I submitted back in October. Instead of looking for specific
> HID reports this patch looks for the Generic Desktop Pointer usage. Having that usage along with
> the vendor specific collection seems to be a distinctive enough to id the Synaptics touchpad in
> the composite USB device.
Any comments on this patch?
Thanks,
Andrew
> drivers/hid/hid-core.c | 21 ++++++++++++++++-----
> include/linux/hid.h | 4 +++-
> 2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 12b6e67..ba9dc59 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -698,10 +698,20 @@ static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
> static void hid_scan_collection(struct hid_parser *parser, unsigned type)
> {
> struct hid_device *hid = parser->device;
> + int i;
>
> if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
> type == HID_COLLECTION_PHYSICAL)
> hid->group = HID_GROUP_SENSOR_HUB;
> +
> + if ((parser->global.usage_page << 16) == HID_UP_GENDESK)
> + for (i = 0; i < parser->local.usage_index; i++)
> + if (parser->local.usage[i] == HID_GD_POINTER)
> + parser->scan_flags
> + |= HID_SCAN_FLAG_GENDESK_POINTER;
> +
> + if ((parser->global.usage_page << 16) == HID_UP_MSVENDOR)
> + parser->scan_flags |= HID_SCAN_FLAG_VENDOR_SPECIFIC;
> }
>
> static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
> @@ -783,11 +793,12 @@ static int hid_scan_report(struct hid_device *hid)
> * Vendor specific handlings
> */
> if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
> - (hid->group == HID_GROUP_GENERIC) &&
> - /* only bind to the mouse interface of composite USB devices */
> - (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
> - /* hid-rmi should take care of them, not hid-generic */
> - hid->group = HID_GROUP_RMI;
> + (hid->group == HID_GROUP_GENERIC)) {
> + if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
> + && (parser->scan_flags & HID_SCAN_FLAG_GENDESK_POINTER))
> + /* hid-rmi should take care of them, not hid-generic */
> + hid->group = HID_GROUP_RMI;
> + }
>
> /*
> * Vendor specific handlings
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index f53c4a9..b019f15 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -547,7 +547,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_SCAN_FLAG_MT_WIN_8 0x00000001
> +#define HID_SCAN_FLAG_MT_WIN_8 BIT(0)
> +#define HID_SCAN_FLAG_VENDOR_SPECIFIC BIT(1)
> +#define HID_SCAN_FLAG_GENDESK_POINTER BIT(2)
>
> struct hid_parser {
> struct hid_global global;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver
2014-12-09 0:16 ` Andrew Duggan
@ 2014-12-10 20:42 ` Benjamin Tissoires
2014-12-12 9:44 ` Jiri Kosina
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Tissoires @ 2014-12-10 20:42 UTC (permalink / raw)
To: Andrew Duggan
Cc: linux-input, linux-kernel@vger.kernel.org, Jiri Kosina,
Benjamin Tissoires
Hi Andrew,
On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@synaptics.com> wrote:
> On 11/24/2014 03:37 PM, Andrew Duggan wrote:
>>
>> On composite HID devices there may be multiple HID devices on separate
>> interfaces, but hid-rmi
>> should only bind to the touchpad. The previous version simply checked that
>> the interface protocol
>> was set to mouse. Unfortuately, it is not always the case that the
>> touchpad has the mouse interface
>> protocol set. This patch takes a different approach and scans the report
>> descriptor looking for
>> the Generic Desktop Pointer usage and the Vendor Specific Top Level
>> Collection needed by the
>> hid-rmi driver to interface with the device.
>>
>> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
>> ---
>> This is a second attempt at the patch I submitted back in October. Instead
>> of looking for specific
>> HID reports this patch looks for the Generic Desktop Pointer usage. Having
>> that usage along with
>> the vendor specific collection seems to be a distinctive enough to id the
>> Synaptics touchpad in
>> the composite USB device.
>
> Any comments on this patch?
Sorry for the lag on this one.
>
> Thanks,
> Andrew
>
>> drivers/hid/hid-core.c | 21 ++++++++++++++++-----
>> include/linux/hid.h | 4 +++-
>> 2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 12b6e67..ba9dc59 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -698,10 +698,20 @@ static void hid_scan_feature_usage(struct hid_parser
>> *parser, u32 usage)
>> static void hid_scan_collection(struct hid_parser *parser, unsigned
>> type)
>> {
>> struct hid_device *hid = parser->device;
>> + int i;
>> if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
>> type == HID_COLLECTION_PHYSICAL)
>> hid->group = HID_GROUP_SENSOR_HUB;
>> +
>> + if ((parser->global.usage_page << 16) == HID_UP_GENDESK)
>> + for (i = 0; i < parser->local.usage_index; i++)
>> + if (parser->local.usage[i] == HID_GD_POINTER)
>> + parser->scan_flags
>> + |= HID_SCAN_FLAG_GENDESK_POINTER;
Change GENDESK into GD (or remove it) and the line will fit on 80 chars :)
>> +
>> + if ((parser->global.usage_page << 16) == HID_UP_MSVENDOR)
>> + parser->scan_flags |= HID_SCAN_FLAG_VENDOR_SPECIFIC;
This, I don't completely like it.
We should have "if ((parser->global.usage_page << 16) >=
HID_UP_MSVENDOR)". The HUT 1.12 specifies that usage pages between
0xff00 and 0xffff are vendor specific. So if we want to put a flag on
it, then using ">="is more appropriate.
Side note, we should really rename this #define HID_UP_MSVENDOR, this
has nothing to do with Microsoft.
>> }
>> static int hid_scan_main(struct hid_parser *parser, struct hid_item
>> *item)
>> @@ -783,11 +793,12 @@ static int hid_scan_report(struct hid_device *hid)
>> * Vendor specific handlings
>> */
>> if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
>> - (hid->group == HID_GROUP_GENERIC) &&
>> - /* only bind to the mouse interface of composite USB devices
>> */
>> - (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
>> - /* hid-rmi should take care of them, not hid-generic */
>> - hid->group = HID_GROUP_RMI;
>> + (hid->group == HID_GROUP_GENERIC)) {
>> + if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
>> + && (parser->scan_flags &
>> HID_SCAN_FLAG_GENDESK_POINTER))
>> + /* hid-rmi should take care of them, not
>> hid-generic */
>> + hid->group = HID_GROUP_RMI;
>> + }
I prefer it. The differentiation is done at the report descriptor
level, it's better for me.
>> /*
>> * Vendor specific handlings
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index f53c4a9..b019f15 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -547,7 +547,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_SCAN_FLAG_MT_WIN_8 0x00000001
>> +#define HID_SCAN_FLAG_MT_WIN_8 BIT(0)
>> +#define HID_SCAN_FLAG_VENDOR_SPECIFIC BIT(1)
>> +#define HID_SCAN_FLAG_GENDESK_POINTER BIT(2)
>> struct hid_parser {
>> struct hid_global global;
>
>
With the minors comments I expressed, this patch is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Thanks Andrew!
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-12 9:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-24 23:37 [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver Andrew Duggan
2014-12-09 0:16 ` Andrew Duggan
2014-12-10 20:42 ` Benjamin Tissoires
2014-12-12 9:44 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).