Re: [PATCH] Input: Fix multitouch support for Type Cover 3

From: Benjamin Tissoires
Date: Mon Apr 13 2015 - 10:16:39 EST

On Sun, Apr 12, 2015 at 6:04 PM, Felipe <felipe.otamendi@xxxxxxxxx> wrote:
> Hi Benjamin,
>
> On Sat, Apr 11, 2015 at 11:08 AM, Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxx> wrote:
>> Hi Felipe,
>>
>> On Sat, Apr 11, 2015 at 12:17 AM, Felipe Otamendi
>> <felipe.otamendi@xxxxxxxxx> wrote:
>>> Make the Type Cover 3 use the hid multitouch driver, which is better suited for the touchpad. Also, since it has multiple reports under the same interface, allow the generic hid driver to handle non-multitouch inputs such as the keyboard's.
>>
>> IIRC, the point of having hid-microsoft was to have better support of
>> the keyboard special functions and shortcuts. Can you please confirm
>> that you do not lose any functionality?
>>
>
> I've checked and all the keys work as they used to with the previous
> patch. The only thing that doesn't work is the led on the Caps Lock
> key. That's because the output from the keyboard report is being
> mapped as a different input than the input from the same report
> because of how inputs are mapped when HID_QUIRK_MULTI_INPUT is
> enabled.

That is worrisome. It means that there will be a regression with the patch.
If I understand correctly, with hid-microsoft, the Caps Lock LED
works, and not with hid-multitouch?

Can you share the report descriptors of the device? I might have had
one, but I can not find it.

>
>>>
>>> Signed-off-by: Felipe Otamendi <felipe.otamendi@xxxxxxxxx>
>>> ---
>>> drivers/hid/hid-core.c | 6 ++----
>>> drivers/hid/hid-microsoft.c | 3 ---
>>> drivers/hid/hid-multitouch.c | 5 +++++
>>> 3 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 56ce8c2..5a80896 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -705,9 +705,8 @@ static void hid_scan_collection(struct hid_parser *parser, unsigned type)
>>> hid->group = HID_GROUP_SENSOR_HUB;
>>>
>>> if (hid->vendor == USB_VENDOR_ID_MICROSOFT &&
>>> - (hid->product == USB_DEVICE_ID_MS_TYPE_COVER_3 ||
>>> - hid->product == USB_DEVICE_ID_MS_TYPE_COVER_3_JP) &&
>>> - hid->group == HID_GROUP_MULTITOUCH)
>>> + hid->product == USB_DEVICE_ID_MS_TYPE_COVER_3_JP &&
>>> + hid->group == HID_GROUP_MULTITOUCH)
>>> hid->group = HID_GROUP_GENERIC;
>>>
>>> if ((parser->global.usage_page << 16) == HID_UP_GENDESK)
>>> @@ -1878,7 +1877,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K) },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_WIRELESS_OPTICAL_DESKTOP_3_0) },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_OFFICE_KB) },
>>> - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_3) },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_3_JP) },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL) },
>>> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
>>> index af935eb..7e84463 100644
>>> --- a/drivers/hid/hid-microsoft.c
>>> +++ b/drivers/hid/hid-microsoft.c
>>> @@ -276,11 +276,8 @@ static const struct hid_device_id ms_devices[] = {
>>> .driver_data = MS_NOGET },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500),
>>> .driver_data = MS_DUPLICATE_USAGES },
>>> - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_3),
>>> - .driver_data = MS_HIDINPUT },
>>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_3_JP),
>>> .driver_data = MS_HIDINPUT },
>>> -
>>
>> Please keep this line, it adds extra to the commit and might have been
>> put on purpose by the original author.
>>
>
> Sorry about that. I'll correct the patch without this change.
>
>>> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT),
>>> .driver_data = MS_PRESENTER },
>>> { }
>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>>> index f65e78b..d93c766 100644
>>> --- a/drivers/hid/hid-multitouch.c
>>> +++ b/drivers/hid/hid-multitouch.c
>>> @@ -1235,6 +1235,11 @@ static const struct hid_device_id mt_devices[] = {
>>> MT_USB_DEVICE(USB_VENDOR_ID_ILITEK,
>>> USB_DEVICE_ID_ILITEK_MULTITOUCH) },
>>>
>>> + /* Microsoft Type Cover 3 */
>>> + { .driver_data = MT_CLS_EXPORT_ALL_INPUTS,
>>> + MT_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
>>> + USB_DEVICE_ID_MS_TYPE_COVER_3) },
>>> +
>>> /* MosArt panels */
>>> { .driver_data = MT_CLS_CONFIDENCE_MINUS_ONE,
>>> MT_USB_DEVICE(USB_VENDOR_ID_ASUS,
>>> --
>>> 2.1.0
>>
>> I had a similar patch in my tree at the time when we were deciding
>> what to do for those devices.
>> This patch had an extra hunk (sorry gmail will cut the lines and everything):
>>
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -961,6 +961,9 @@ static void mt_input_configured(struct hid_device
>> *hdev, struct hid_input *hi)
>> case HID_DG_TOUCHSCREEN:
>> /* we do not set suffix = "Touchscreen" */
>> break;
>> + case HID_DG_TOUCHPAD:
>> + suffix = "Touchpad";
>> + break;
>> case HID_GD_SYSTEM_CONTROL:
>> suffix = "System Control";
>> break;
>>
>> Can you please test/add this with your current patch. Your touchpad
>> might appear as "UNKNOWN", which is not very appealing :)
>>
>
> It works, now it appears with the Touchscreen suffix. I should send
> the new patch as a response to this thread right?

I guess you meant "Touchpad" here.

No need to send the v2 as a reply to this thread. Just use the subject
prefix "PATCH v2" and that should be enough.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/