Re: [PATCH v2] HID: rmi: Scan the report descriptor to determine if the device is suitable for the hid-rmi driver

[Benjamin Tissoires <benjamin.tissoires@gmail.com>]

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