Hi Andrew,
On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
Thanks Andrew!
Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
