From: Sean Young <sean@mess.org> Date: 2011-10-26 21:36:48
Since this commit:
commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
HID: multitouch: decide if hid-multitouch needs to handle mt devices
I get the following when I insert a smartjoy device (hid-sjoy driver):
[ 3727.405037] usb 7-1: new low speed USB device number 2 using uhci_hcd
[ 3727.709082] usb 7-1: New USB device found, idVendor=6666, idProduct=8802
[ 3727.709087] usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 3727.709092] usb 7-1: Product: TigerGame PS/PS2 Game Controller Adapter
[ 3727.709095] usb 7-1: Manufacturer: WiseGroup.,Ltd
[ 3738.002095] hid-multitouch 0003:6666:8802.0005: timeout initializing reports
[ 3738.007861] input: WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1:1.0/input/input17
[ 3738.008137] smartjoyplus 0003:6666:8802.0005: input,hidraw4: USB HID v1.00 Joystick [WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter] on usb-0000:00:1d.1-1/input0
[ 3738.008163] smartjoyplus 0003:6666:8802.0005: Force feedback for SmartJoy PLUS PS2/USB adapter
Note the 10 second delay caused by the hid-multitouch error.
If I understand it correctly, the problem is that hid-multitouch now has
a catch-all usb-id field, and does a usbhid_init_reports() on the device
without the quirk HID_QUIRK_NOGET.
The HID_QUIRK_NOGET for this device is listed in the hid-sjoy.c driver itself
rather than in hid-quirks.c; presumably the latter is handled correctly.
Is there a different way of handling this rather than hid-multitouch
messing with every usb device which identifies itself as hid? Alternatively,
should all quirks for all devices be specified in hid-quirks.c and not in
individual drivers?
Sean
From: Benjamin Tissoires <hidden> Date: 2011-10-27 09:25:17
Hi Sean,
On Wed, Oct 26, 2011 at 23:37, Sean Young [off-list ref] wrote:
Since this commit:
commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
HID: multitouch: decide if hid-multitouch needs to handle mt devices
I get the following when I insert a smartjoy device (hid-sjoy driver):
[ 3727.405037] usb 7-1: new low speed USB device number 2 using uhci_hcd
[ 3727.709082] usb 7-1: New USB device found, idVendor=6666, idProduct=8802
[ 3727.709087] usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 3727.709092] usb 7-1: Product: TigerGame PS/PS2 Game Controller Adapter
[ 3727.709095] usb 7-1: Manufacturer: WiseGroup.,Ltd
[ 3738.002095] hid-multitouch 0003:6666:8802.0005: timeout initializing reports
[ 3738.007861] input: WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1:1.0/input/input17
[ 3738.008137] smartjoyplus 0003:6666:8802.0005: input,hidraw4: USB HID v1.00 Joystick [WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter] on usb-0000:00:1d.1-1/input0
[ 3738.008163] smartjoyplus 0003:6666:8802.0005: Force feedback for SmartJoy PLUS PS2/USB adapter
Note the 10 second delay caused by the hid-multitouch error.
Thanks for this bug report. It's great that this bug that concerns
only 9 devices has been reported.
To sum up, I'll have to redo my patch.
If I understand it correctly, the problem is that hid-multitouch now has
a catch-all usb-id field, and does a usbhid_init_reports() on the device
without the quirk HID_QUIRK_NOGET.
yep
The HID_QUIRK_NOGET for this device is listed in the hid-sjoy.c driver itself
rather than in hid-quirks.c; presumably the latter is handled correctly.
It should work, but this patch was a little bit too invasive.
Is there a different way of handling this rather than hid-multitouch
messing with every usb device which identifies itself as hid? Alternatively,
should all quirks for all devices be specified in hid-quirks.c and not in
individual drivers?
I'm working on a better solution than this patch (I've just found it
but I need some time to format and send it...)
Cheers,
Benjamin
Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Benjamin Tissoires <hidden> Date: 2011-10-27 11:54:28
On 10/27/2011 11:25 AM, Benjamin Tissoires wrote:
Hi Sean,
On Wed, Oct 26, 2011 at 23:37, Sean Young[off-list ref] wrote:
quoted
Since this commit:
commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
HID: multitouch: decide if hid-multitouch needs to handle mt devices
I get the following when I insert a smartjoy device (hid-sjoy driver):
[ 3727.405037] usb 7-1: new low speed USB device number 2 using uhci_hcd
[ 3727.709082] usb 7-1: New USB device found, idVendor=6666, idProduct=8802
[ 3727.709087] usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 3727.709092] usb 7-1: Product: TigerGame PS/PS2 Game Controller Adapter
[ 3727.709095] usb 7-1: Manufacturer: WiseGroup.,Ltd
[ 3738.002095] hid-multitouch 0003:6666:8802.0005: timeout initializing reports
[ 3738.007861] input: WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1:1.0/input/input17
[ 3738.008137] smartjoyplus 0003:6666:8802.0005: input,hidraw4: USB HID v1.00 Joystick [WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter] on usb-0000:00:1d.1-1/input0
[ 3738.008163] smartjoyplus 0003:6666:8802.0005: Force feedback for SmartJoy PLUS PS2/USB adapter
Note the 10 second delay caused by the hid-multitouch error.
Thanks for this bug report. It's great that this bug that concerns
only 9 devices has been reported.
To sum up, I'll have to redo my patch.
quoted
If I understand it correctly, the problem is that hid-multitouch now has
a catch-all usb-id field, and does a usbhid_init_reports() on the device
without the quirk HID_QUIRK_NOGET.
yep
quoted
The HID_QUIRK_NOGET for this device is listed in the hid-sjoy.c driver itself
rather than in hid-quirks.c; presumably the latter is handled correctly.
It should work, but this patch was a little bit too invasive.
quoted
Is there a different way of handling this rather than hid-multitouch
messing with every usb device which identifies itself as hid? Alternatively,
should all quirks for all devices be specified in hid-quirks.c and not in
individual drivers?
I'm working on a better solution than this patch (I've just found it
but I need some time to format and send it...)
Cheers,
Benjamin
quoted
Sean
Hi Sean, can you test the following patch please:
From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <redacted>
Date: Thu, 27 Oct 2011 13:36:05 +0200
Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers
The commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
(HID: multitouch: decide if hid-multitouch needs to handle mt devices)
was too invasive in its relationship with other drivers.
For instance, hid-sjoy specify the quirk HID_QUIRK_NOGET
and hid-multitouch ignores it, thus adding a 10 seconds wait.
This patch allows hid-multitouch to infer how the device landed here:
* if it was manually added to the supported devices of hid-multitouch
and in hid_have_special_driver in hid-core, then it has to be taken.
* if it was rejected by hid-core due to the VID/PID in
hid_have_special_driver and is not present in the manual list,
then another driver will take care of it.
* if it was rejected by hid-core due to the presence of the CONTACT_ID
hid field, then no other driver will handle it, and hid-multitouch
can safely handle it.
The mt_have_special_driver list is also obsolete now.
Reported-by: Sean Young <sean@mess.org>
Signed-off-by: Benjamin Tissoires <redacted>
---
drivers/hid/hid-core.c | 1 -
drivers/hid/hid-multitouch.c | 35 +++++------------------------------
2 files changed, 5 insertions(+), 31 deletions(-)
@@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev, unsigned
int connect_mask)
hdev->claimed |= HID_CLAIMED_INPUT;
if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
/* this device should be handled by hid-multitouch, skip it */
- hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
return -ENODEV;
}
@@ -530,33 +530,6 @@ static void mt_set_input_mode(struct hid_device *hdev)}}-/* a list of devices for which there is a specialized multitouch driver */-staticconststructhid_device_idmt_have_special_driver[]={-{HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,0x0001)},-{HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,0x0006)},-{HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,-USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN)},-{HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,-USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH)},-{}-};--staticboolmt_match_one_id(structhid_device*hdev,-conststructhid_device_id*id)-{-returnid->bus==hdev->bus&&-(id->vendor==HID_ANY_ID||id->vendor==hdev->vendor)&&-(id->product==HID_ANY_ID||id->product==hdev->product);-}--staticconststructhid_device_id*mt_match_id(structhid_device*hdev,-conststructhid_device_id*id)-{-for(;id->bus;id++)-if(mt_match_one_id(hdev,id))-returnid;--returnNULL;}staticintmt_probe(structhid_device*hdev,conststruct
hid_device_id *id)
@@ -565,7 +538,10 @@ static int mt_probe(struct hid_device *hdev, const
struct hid_device_id *id)
struct mt_device *td;
struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
- if (mt_match_id(hdev, mt_have_special_driver))
+ if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
+ /* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
+ * and either in hid_have_special_driver
+ * or not detected as multitouch by hid-core */
return -ENODEV;
for (i = 0; mt_classes[i].name ; i++) {
From: Sean Young <sean@mess.org> Date: 2011-10-27 20:35:34
On Thu, Oct 27, 2011 at 01:54:23PM +0200, Benjamin Tissoires wrote:
Hi Sean, can you test the following patch please:
From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <redacted>
Date: Thu, 27 Oct 2011 13:36:05 +0200
Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers
@@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev,
unsigned int connect_mask)
Your patch got mangled -- for example, the line above got wrapped. I had
to apply it manually.
I've tested it and it works fine. I do not get the error nor the delay
with this applied, which is great.
Thanks!
Sean
From: Henrik Rydberg <hidden> Date: 2011-10-28 11:16:06
Hi Benjamin,
Hi Sean, can you test the following patch please:
From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <redacted>
Date: Thu, 27 Oct 2011 13:36:05 +0200
Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers
I took this patch, fixed it up, applied it to jikos/multitouch, and
did a "git diff HEAD~3" to see the actual changes applied so far for
generic hid-mt support. That diff is quite small, so I would recommend
rewinding the tree once things settle down. I have commented on the
diff below, and at the end there are three alternative (untested)
patches, as a suggestion.
@@ -1212,6 +1212,11 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)if((connect_mask&HID_CONNECT_HIDINPUT)&&!hidinput_connect(hdev,connect_mask&HID_CONNECT_HIDINPUT_FORCE))hdev->claimed|=HID_CLAIMED_INPUT;+if(hdev->quirks&HID_QUIRK_MULTITOUCH){+/* this device should be handled by hid-multitouch, skip it */+return-ENODEV;+}+if((connect_mask&HID_CONNECT_HIDDEV)&&hdev->hiddev_connect&&!hdev->hiddev_connect(hdev,connect_mask&HID_CONNECT_HIDDEV_FORCE))
The regret here is that hid-core needs to know about hit-mt at
all. What it needs to know is whether the device should be dropped.
@@ -978,6 +982,13 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) } }+ if (hid->quirks & HID_QUIRK_MULTITOUCH) {+ /* generic hid does not know how to handle multitouch devices */+ if (hidinput)+ goto out_cleanup;+ goto out_unwind;+ }+ if (hidinput && input_register_device(hidinput->input)) goto out_cleanup;
One could instead drop handling based on a quirk designed for that
purpose (HID_QUIRK_HIDINPUT_DROP).
Although correct per se, it is clearer to reset this flag in mt_probe().
@@ -535,6 +536,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) struct mt_device *td; struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */+ if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))+ /* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),+ * and either in hid_have_special_driver+ * or not detected as multitouch by hid-core */+ return -ENODEV;+ for (i = 0; mt_classes[i].name ; i++) { if (id->driver_data == mt_classes[i].name) { mtclass = &(mt_classes[i]);
Very neat solution indeed!
@@ -542,10 +549,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) } }- /* This allows the driver to correctly support devices- * that emit events over several HID messages.- */- hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC; td = kzalloc(sizeof(struct mt_device), GFP_KERNEL); if (!td) {
It seems there is no longer any reason to move this line around, since
we now only come here when the device is really meant for this driver.
This is unnecessary and makes the logic blurred.
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret)
goto fail;
+ /* This allows the driver to correctly support devices
+ * that emit events over several HID messages.
+ */
+ hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+
td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
GFP_KERNEL);
if (!td->slots) {
In addition to not needing to be moved, this line introduces a race
with hid-input, since the device has already started when this line is
executed.
@@ -758,6 +767,9 @@ static const struct hid_device_id mt_devices[] = { HID_USB_DEVICE(USB_VENDOR_ID_XAT, USB_DEVICE_ID_XAT_CSR) },+ /* Rest of the world */+ { HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },+ { } }; MODULE_DEVICE_TABLE(hid, mt_devices);
As an alternative, here are three untested and uncommited patches
which implements the comments above.
Cheers,
Henrik
--
From defdac444919b99a932368ee1a8ad290dc724933 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <redacted>
Date: Fri, 28 Oct 2011 11:36:36 +0200
Subject: [PATCH 1/3] hid: Allow an input device to be dropped after parsing
Some devices need a special driver based on the input mapping of the
device. This patch enables a mechanism where hidinput can set
HID_QUIRK_HIDINPUT_DROP to leave a device to be picked up by a special
driver which intercepts the input mapping.
---
drivers/hid/hid-core.c | 3 +++
drivers/hid/hid-input.c | 6 ++++++
include/linux/hid.h | 1 +
3 files changed, 10 insertions(+), 0 deletions(-)
From: Benjamin Tissoires <hidden> Date: 2011-10-28 13:19:08
Hi Henrik,
well, most of the comments you in-lined addressed the fact that I
tried to make the smallest patch possible.
On Fri, Oct 28, 2011 at 13:16, Henrik Rydberg [off-list ref] wrote:
quoted hunk
Hi Benjamin,
quoted
Hi Sean, can you test the following patch please:
From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <redacted>
Date: Thu, 27 Oct 2011 13:36:05 +0200
Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers
I took this patch, fixed it up, applied it to jikos/multitouch, and
did a "git diff HEAD~3" to see the actual changes applied so far for
generic hid-mt support. That diff is quite small, so I would recommend
rewinding the tree once things settle down. I have commented on the
diff below, and at the end there are three alternative (untested)
patches, as a suggestion.
@@ -1212,6 +1212,11 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
connect_mask & HID_CONNECT_HIDINPUT_FORCE))
hdev->claimed |= HID_CLAIMED_INPUT;
+ if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
+ /* this device should be handled by hid-multitouch, skip it */
+ return -ENODEV;
+ }
+
if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
!hdev->hiddev_connect(hdev,
connect_mask & HID_CONNECT_HIDDEV_FORCE))
The regret here is that hid-core needs to know about hit-mt at
all. What it needs to know is whether the device should be dropped.
map_key_clear(BTN_STYLUS2);
break;
+ case 0x51: /* ContactID */
+ device->quirks |= HID_QUIRK_MULTITOUCH;
+ goto unknown;
+
default: goto unknown;
}
break;
So in addition to the detection here,
@@ -978,6 +982,13 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
}
}
+ if (hid->quirks & HID_QUIRK_MULTITOUCH) {
+ /* generic hid does not know how to handle multitouch devices */
+ if (hidinput)
+ goto out_cleanup;
+ goto out_unwind;
+ }
+
if (hidinput && input_register_device(hidinput->input))
goto out_cleanup;
One could instead drop handling based on a quirk designed for that
purpose (HID_QUIRK_HIDINPUT_DROP).
td->last_slot_field = usage->hid;
td->last_field_index = field->index;
td->last_mt_collection = usage->collection_index;
+ hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
return 1;
case HID_DG_WIDTH:
hid_map_usage(hi, usage, bit, max,
Although correct per se, it is clearer to reset this flag in mt_probe().
struct mt_device *td;
struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
+ if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
+ /* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
+ * and either in hid_have_special_driver
+ * or not detected as multitouch by hid-core */
+ return -ENODEV;
+
for (i = 0; mt_classes[i].name ; i++) {
if (id->driver_data == mt_classes[i].name) {
mtclass = &(mt_classes[i]);
Very neat solution indeed!
}
}
- /* This allows the driver to correctly support devices
- * that emit events over several HID messages.
- */
- hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
if (!td) {
It seems there is no longer any reason to move this line around, since
we now only come here when the device is really meant for this driver.
if (ret != 0)
goto fail;
+ hdev->quirks |= HID_QUIRK_MULTITOUCH;
This is unnecessary and makes the logic blurred.
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret)
goto fail;
+ /* This allows the driver to correctly support devices
+ * that emit events over several HID messages.
+ */
+ hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+
td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
GFP_KERNEL);
if (!td->slots) {
In addition to not needing to be moved, this line introduces a race
with hid-input, since the device has already started when this line is
executed.
Well, we should use instead the new input_register callback to avoid any races.
I've got the patch, I've tested it, but I never sent it.... shame on me.
#define HID_QUIRK_BADPAD 0x00000020
#define HID_QUIRK_MULTI_INPUT 0x00000040
#define HID_QUIRK_HIDINPUT_FORCE 0x00000080
+#define HID_QUIRK_MULTITOUCH 0x00000100
#define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
#define HID_QUIRK_FULLSPEED_INTERVAL 0x10000000
#define HID_QUIRK_NO_INIT_REPORTS 0x20000000
As an alternative, here are three untested and uncommited patches
which implements the comments above.
Except a comment in the second patch, I've tested it, and it worked
without any surprises ;-)
But I'm not sure we can rewind the tree as those patches are in
for-next since nearly a month.
I'll let you do the reverts, because on my local trees, it was quite
difficult (and I'm still on a 3.0.x, so I can not test against
for-next)
quoted hunk
--
From defdac444919b99a932368ee1a8ad290dc724933 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <redacted>
Date: Fri, 28 Oct 2011 11:36:36 +0200
Subject: [PATCH 1/3] hid: Allow an input device to be dropped after parsing
Some devices need a special driver based on the input mapping of the
device. This patch enables a mechanism where hidinput can set
HID_QUIRK_HIDINPUT_DROP to leave a device to be picked up by a special
driver which intercepts the input mapping.
---
drivers/hid/hid-core.c | 3 +++
drivers/hid/hid-input.c | 6 ++++++
include/linux/hid.h | 1 +
3 files changed, 10 insertions(+), 0 deletions(-)
#define HID_QUIRK_BADPAD 0x00000020
#define HID_QUIRK_MULTI_INPUT 0x00000040
#define HID_QUIRK_HIDINPUT_FORCE 0x00000080
+#define HID_QUIRK_HIDINPUT_DROP 0x00000100
#define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
#define HID_QUIRK_FULLSPEED_INTERVAL 0x10000000
#define HID_QUIRK_NO_INIT_REPORTS 0x20000000
--
1.7.7
From 5f4fe6ef4cab9721c6b277f57d5374d6b549359d Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <redacted>
Date: Fri, 28 Oct 2011 11:42:39 +0200
Subject: [PATCH 2/3] hid-input: Drop generic handling of hid-mt multitouch
devices
The hid-mt devices are recognized by the ContactID field. This patch
sets HID_QUIRK_MULTITOUCH accordingly, and leaves the device to be
picked up by any driver which intercepts the ContactID field.
All in-tree hid-mt drivers intercept the ContactID, so this patch has
no other effect than to skip generic handling of hid-mt devices.
struct mt_device *td;
struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
+ if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
+ /* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
+ * and either in hid_have_special_driver
+ * or not detected as multitouch by hid-core */
+ return -ENODEV;
+
+ /* pick up the device that was dropped by hid-core */
+ hdev->quirks &= ~HID_QUIRK_HIDINPUT_DROP;
+
for (i = 0; mt_classes[i].name ; i++) {
if (id->driver_data == mt_classes[i].name) {
mtclass = &(mt_classes[i]);
HID_USB_DEVICE(USB_VENDOR_ID_XAT,
USB_DEVICE_ID_XAT_CSR) },
+ /* Rest of the world */
+ { HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
+
{ }
};
MODULE_DEVICE_TABLE(hid, mt_devices);
--
1.7.7
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Henrik Rydberg <hidden> Date: 2011-10-28 14:00:37
Hi Benjamin,
well, most of the comments you in-lined addressed the fact that I
tried to make the smallest patch possible.
Admittedly, HID_QUIRK_HIDINPUT_DROP makes the patch larger, but all
other comments make it smaller.
quoted
+ /* This allows the driver to correctly support devices
+ * that emit events over several HID messages.
+ */
+ hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+
td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
GFP_KERNEL);
if (!td->slots) {
In addition to not needing to be moved, this line introduces a race
with hid-input, since the device has already started when this line is
executed.
Well, we should use instead the new input_register callback to avoid
any races. I've got the patch, I've tested it, but I never sent
it.... shame on me.
I guess it is moot now anyways.
Except a comment in the second patch, I've tested it, and it worked
without any surprises ;-)
The code still mainly follows your initial idea, so of course it
works. ;-)
But I'm not sure we can rewind the tree as those patches are in
for-next since nearly a month.
I'll let you do the reverts, because on my local trees, it was quite
difficult (and I'm still on a 3.0.x, so I can not test against
for-next)
Ok, let's see how Jiri wants to play it, and I can prepare either a
rewind-and-patch or a revert-and-patch or a keep-and-patch set.
quoted
Subject: [PATCH 2/3] hid-input: Drop generic handling of hid-mt multitouch
devices
The hid-mt devices are recognized by the ContactID field. This patch
sets HID_QUIRK_MULTITOUCH accordingly, and leaves the device to be
picked up by any driver which intercepts the ContactID field.
All in-tree hid-mt drivers intercept the ContactID, so this patch has
no other effect than to skip generic handling of hid-mt devices.
Thanks, I will also make sure authorship is preserved where appropriate.
Cheers,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
for-next since nearly a month.
I'll let you do the reverts, because on my local trees, it was quite
difficult (and I'm still on a 3.0.x, so I can not test against
for-next)
Ok, let's see how Jiri wants to play it, and I can prepare either a
rewind-and-patch or a revert-and-patch or a keep-and-patch set.
As I have already flushed my initial 3.1 queue to Linus already, I'd like
to ask you guys to send me a patch based on Linus' tree (or my
'upstream-fixes' branch, which is what I am going to be sending as fixes
for 3.2 still).
I will then make sure that it reaches Linus before 3.2 has been released.
From: Henrik Rydberg <hidden> Date: 2011-11-01 14:07:51
From bc81ba588149a0058ca083f673cb22ff543e5af8 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <redacted>
Date: Tue, 1 Nov 2011 13:20:59 +0100
Subject: [PATCH 1/2] Revert "HID: multitouch: decide if hid-multitouch needs
to handle mt devices"
This reverts commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205.
The generic detection of hid-mt devices has two major flaws, and was
merged prematurely. Firstly, the hid-multitouch gets loaded even when
the device is handled by a special device. Secondly, the patch only
partially duplicates the device whitelist already present in hid-core,
effectively rendering a number of devices non-functional.
Reported-by: Sean Young <sean@mess.org>
Tested-by: Benjamin Tissoires <redacted>
Signed-off-by: Henrik Rydberg <redacted>
---
Hi Jiri,
Unfortunately, the recently discussed solution does not seem to work,
so there is only a revert for now.
I was planning to send a second patch, very small and concise, based
on Benjamins patch, but
1. It still does not resolve the
hid-multitouch-gets-loaded-for-every-hid-device problem, and
2. It did not work after removing the tested device from the hid core
whitelist; the device quirk seemed to get lost in the process.
In other words, there is currently no viable solution.
Thanks,
Henrik
drivers/hid/hid-multitouch.c | 47 +++--------------------------------------
1 files changed, 4 insertions(+), 43 deletions(-)
@@ -530,44 +529,12 @@ static void mt_set_input_mode(struct hid_device *hdev)}}-/* a list of devices for which there is a specialized multitouch driver */-staticconststructhid_device_idmt_have_special_driver[]={-{HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,0x0001)},-{HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,0x0006)},-{HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,-USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN)},-{HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,-USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH)},-{}-};--staticboolmt_match_one_id(structhid_device*hdev,-conststructhid_device_id*id)-{-returnid->bus==hdev->bus&&-(id->vendor==HID_ANY_ID||id->vendor==hdev->vendor)&&-(id->product==HID_ANY_ID||id->product==hdev->product);-}--staticconststructhid_device_id*mt_match_id(structhid_device*hdev,-conststructhid_device_id*id)-{-for(;id->bus;id++)-if(mt_match_one_id(hdev,id))-returnid;--returnNULL;-}-staticintmt_probe(structhid_device*hdev,conststructhid_device_id*id){intret,i;structmt_device*td;structmt_class*mtclass=mt_classes;/* MT_CLS_DEFAULT */-if(mt_match_id(hdev,mt_have_special_driver))-return-ENODEV;-for(i=0;mt_classes[i].name;i++){if(id->driver_data==mt_classes[i].name){mtclass=&(mt_classes[i]);
@@ -575,6 +542,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)}}+/* This allows the driver to correctly support devices+*thatemiteventsoverseveralHIDmessages.+*/+hdev->quirks|=HID_QUIRK_NO_INPUT_SYNC;td=kzalloc(sizeof(structmt_device),GFP_KERNEL);if(!td){
@@ -590,16 +561,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)if(ret!=0)gotofail;-hdev->quirks|=HID_QUIRK_MULTITOUCH;ret=hid_hw_start(hdev,HID_CONNECT_DEFAULT);if(ret)gotofail;-/* This allows the driver to correctly support devices-*thatemiteventsoverseveralHIDmessages.-*/-hdev->quirks|=HID_QUIRK_NO_INPUT_SYNC;-td->slots=kzalloc(td->maxcontacts*sizeof(structmt_slot),GFP_KERNEL);if(!td->slots){
@@ -793,10 +758,6 @@ static const struct hid_device_id mt_devices[] = {HID_USB_DEVICE(USB_VENDOR_ID_XAT,USB_DEVICE_ID_XAT_CSR)},-/* Rest of the world */-{.driver_data=MT_CLS_DEFAULT,-HID_USB_DEVICE(HID_ANY_ID,HID_ANY_ID)},-{}};MODULE_DEVICE_TABLE(hid,mt_devices);
This reverts commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205.
The generic detection of hid-mt devices has two major flaws, and was
merged prematurely. Firstly, the hid-multitouch gets loaded even when
the device is handled by a special device. Secondly, the patch only
partially duplicates the device whitelist already present in hid-core,
effectively rendering a number of devices non-functional.
Reported-by: Sean Young <sean@mess.org>
Tested-by: Benjamin Tissoires <redacted>
Signed-off-by: Henrik Rydberg <redacted>
---
Hi Jiri,
Unfortunately, the recently discussed solution does not seem to work,
so there is only a revert for now.
I was planning to send a second patch, very small and concise, based
on Benjamins patch, but
1. It still does not resolve the
hid-multitouch-gets-loaded-for-every-hid-device problem, and
Hmm, I thought it should work. What is the catch?
In other words, there is currently no viable solution.
Thanks, I will be including the patch in my next pile of fixes for 3.2
that will go to Linus.
--
Jiri Kosina
SUSE Labs
From: Benjamin Tissoires <hidden> Date: 2011-11-02 08:23:45
Hi Henrik
On Tue, Nov 1, 2011 at 16:33, Henrik Rydberg [off-list ref] wrote:
quoted
quoted
1. It still does not resolve the
hid-multitouch-gets-loaded-for-every-hid-device problem, and
Hmm, I thought it should work. What is the catch?
Everyone with an umatched hid device, even completely unrelated to
touch, will be surprised to find the hid-multitouch module loaded.
Well, this is a problem that can not be easily solved: IIRC, we can
not force the load of an external driver from within the kernel.
The best solution would be to merge hid-input and hid-multitouch.
Indeed both systems aim at handling generic devices.However, I'd
rather not doing it now as we are not as "good" as the Win 7 driver
(i.e. there are some fallback modes that allow every devices to be
handled even if they don't send clean hid reports).
And, even if hid-multitouch is loaded, only 2 or 3 lines of codes will
be executed to reject the driver in mt_probe, which won't be very time
consuming for end user.
For your second point:
quoted
quoted
2. It did not work after removing the tested device from the hid core
whitelist; the device quirk seemed to get lost in the process.
@@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev,
unsigned int connect_mask)
hdev->claimed |= HID_CLAIMED_INPUT;
if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
/* this device should be handled by hid-multitouch, skip it */
- hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
return -ENODEV;
}
If not, that's maybe that you encountered the only case that is not
correctly handled:
if you register hid-multitouch before hid, then it will be the first
driver tested, and hid-input won't set the quirk correctly.
BTW, it's not a big deal, because if systems do have this behavior, we
can easily put the device from the user space by using
/sys/module/hid_multitouch/drivers/hid\:hid-multitouch/new_id
Cheers,
Benjamin
From: Henrik Rydberg <hidden> Date: 2011-11-02 10:03:18
Hi Benjamin,
quoted
Everyone with an umatched hid device, even completely unrelated to
touch, will be surprised to find the hid-multitouch module loaded.
Well, this is a problem that can not be easily solved: IIRC, we can
not force the load of an external driver from within the kernel.
The best solution would be to merge hid-input and hid-multitouch.
Yes, or actually adding a dynamic mechanism. With hid, it would
clearly be beneficial to be able to load modules based on the result
of the report parsing.
Indeed both systems aim at handling generic devices.However, I'd
rather not doing it now as we are not as "good" as the Win 7 driver
(i.e. there are some fallback modes that allow every devices to be
handled even if they don't send clean hid reports).
What's wrong with having a generic handling in addition to the
specific device list in hid-multitouch?
And, even if hid-multitouch is loaded, only 2 or 3 lines of codes will
be executed to reject the driver in mt_probe, which won't be very time
consuming for end user.
The time to load the module will hit _every_ user, which is worse than
having the code merged with hid-input. Not to mention the annoyance,
it is simply unacceptable.
quoted hunk
For your second point:
quoted
quoted
quoted
2. It did not work after removing the tested device from the hid core
whitelist; the device quirk seemed to get lost in the process.
@@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev,
unsigned int connect_mask)
hdev->claimed |= HID_CLAIMED_INPUT;
if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
/* this device should be handled by hid-multitouch, skip it */
- hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
return -ENODEV;
}
Gah, how did that end up there? Yes, I missed that line in my testing,
which explains it (although I won't test again right now).
If not, that's maybe that you encountered the only case that is not
correctly handled:
if you register hid-multitouch before hid, then it will be the first
driver tested, and hid-input won't set the quirk correctly.
BTW, it's not a big deal, because if systems do have this behavior, we
can easily put the device from the user space by using
/sys/module/hid_multitouch/drivers/hid\:hid-multitouch/new_id
This is also hackish. I _do_ understand the benefits of what we are
aiming at here, but we are piling up crap in the kernel.
To summarize, the idea looked good at first glance, but I think it
creates unacceptable dependencies between modules.
To try again later on, at the very least one should move the essence
of hid-multitouch to something like hid-input-mt, and have hid-input
either include it, select it or depend dynamically on it, in a
try_module_get fashion. The hid_multitouch would then simply contain
the device white list, leaving the general case to hid-input.
Henrik