[patch 0/5] Autohandling of multitouch devices through hid-multitouch

[patch 0/5] Autohandling of multitouch devices through hid-multitouch

[benjamin.tissoires]

Hi Guys,

In this patch series, I introduce 1 trivial patch plus 4 patches that need review I think:
- the first is trivial (I missed it this morning, sorry)
- the second fixes a problem detected with LG panels. The detection of the end of the
  report was not accurate, and needed to be rewritten. I'm worried about it because I got
  warnings under Ubuntu for this patch (-Warray-bounds) whereas I don't get any warnings
  under Fedora.
- the third should not be a problem as it relies on the presence of the QUIRK_MULTITOUCH
  and will impact only unknown devices.
- the fourth worries me more. I introduce changes in hid_device, and I'd like to have your opinion.
- Then, the last one allows us to be compliant with Microsoft's description for Win 7 devices.

Cheers,
Benjamin

PS: Thanks Henrik and Jiri for the series of this morning.

[PATCH 1/5] HID: multitouch: add support for eGalax 0x722a

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-ids.h        |    1 +
 drivers/hid/hid-multitouch.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 6656516..e1fada2 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -240,6 +240,7 @@
 #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7207	0x7207
 #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C	0x720c
 #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224	0x7224
+#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A	0x722A
 #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E	0x725e
 #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7262	0x7262
 #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B	0x726b
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0eda37f..019de83 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -797,6 +797,9 @@ static const struct hid_device_id mt_devices[] = {
 	{ .driver_data = MT_CLS_EGALAX_SERIAL,
 		HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
 			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
+	{ .driver_data = MT_CLS_EGALAX_SERIAL,
+		HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
+			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A) },
 	{ .driver_data = MT_CLS_EGALAX,
 		HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
 			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },
-- 
1.7.7.6

Re: [PATCH 1/5] HID: multitouch: add support for eGalax 0x722a

[Jiri Kosina]

On Tue, 6 Mar 2012, benjamin.tissoires wrote:

> From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
>  drivers/hid/hid-ids.h        |    1 +
>  drivers/hid/hid-multitouch.c |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 6656516..e1fada2 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -240,6 +240,7 @@
>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7207	0x7207
>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C	0x720c
>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224	0x7224
> +#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A	0x722A
>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E	0x725e
>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7262	0x7262
>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B	0x726b
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 0eda37f..019de83 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -797,6 +797,9 @@ static const struct hid_device_id mt_devices[] = {
>  	{ .driver_data = MT_CLS_EGALAX_SERIAL,
>  		HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>  			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
> +	{ .driver_data = MT_CLS_EGALAX_SERIAL,
> +		HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
> +			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A) },
>  	{ .driver_data = MT_CLS_EGALAX,
>  		HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>  			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },

Applied.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/5] HID: multitouch: add support for eGalax 0x722a

[Benjamin Tissoires]

On Fri, Mar 9, 2012 at 13:29, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 6 Mar 2012, benjamin.tissoires wrote:
>
>> From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>>  drivers/hid/hid-ids.h        |    1 +
>>  drivers/hid/hid-multitouch.c |    3 +++
>>  2 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 6656516..e1fada2 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -240,6 +240,7 @@
>>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7207    0x7207
>>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C    0x720c
>>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224    0x7224
>> +#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A    0x722A
>>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E    0x725e
>>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7262    0x7262
>>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B    0x726b
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 0eda37f..019de83 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -797,6 +797,9 @@ static const struct hid_device_id mt_devices[] = {
>>       { .driver_data = MT_CLS_EGALAX_SERIAL,
>>               HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>>                       USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
>> +     { .driver_data = MT_CLS_EGALAX_SERIAL,
>> +             HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>> +                     USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A) },
>>       { .driver_data = MT_CLS_EGALAX,
>>               HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>>                       USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },
>
> Applied.
>

Thanks Jiri.
How about patches 2,3 and 5?
Patch 2 is waited for LG users since a long time now (though it gives warnings).
Patch 3 can be included without patch 4: just loading hid-multitouch
makes the device working. And even if it's a little "hackish", it
won't hurt any other hid devices.
Patch 5 will introduce more robustness for the default class, but
requires patch 3.

Cheers,
Benjamin

> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH 1/5] HID: multitouch: add support for eGalax 0x722a

[Jiri Kosina]

On Sat, 10 Mar 2012, Benjamin Tissoires wrote:

> >>  drivers/hid/hid-ids.h        |    1 +
> >>  drivers/hid/hid-multitouch.c |    3 +++
> >>  2 files changed, 4 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> >> index 6656516..e1fada2 100644
> >> --- a/drivers/hid/hid-ids.h
> >> +++ b/drivers/hid/hid-ids.h
> >> @@ -240,6 +240,7 @@
> >>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7207    0x7207
> >>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_720C    0x720c
> >>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224    0x7224
> >> +#define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A    0x722A
> >>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_725E    0x725e
> >>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7262    0x7262
> >>  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B    0x726b
> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >> index 0eda37f..019de83 100644
> >> --- a/drivers/hid/hid-multitouch.c
> >> +++ b/drivers/hid/hid-multitouch.c
> >> @@ -797,6 +797,9 @@ static const struct hid_device_id mt_devices[] = {
> >>       { .driver_data = MT_CLS_EGALAX_SERIAL,
> >>               HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
> >>                       USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_7224) },
> >> +     { .driver_data = MT_CLS_EGALAX_SERIAL,
> >> +             HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
> >> +                     USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_722A) },
> >>       { .driver_data = MT_CLS_EGALAX,
> >>               HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
> >>                       USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_726B) },
> >
> > Applied.
> >
> 
> Thanks Jiri.
> How about patches 2,3 and 5?

I just needed some more time to review those. They are now in my tree as 
well.

Patch #4 is still under discussion, and thus on hold.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
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

Re: [PATCH 1/5] HID: multitouch: add support for eGalax 0x722a

[Henrik Rydberg]

> > Thanks Jiri.
> > How about patches 2,3 and 5?
> 
> I just needed some more time to review those. They are now in my tree as 
> well.
> 
> Patch #4 is still under discussion, and thus on hold.

I am ok with the approach, given that a better one would require a
substantial amount of extra work. Also, since this is all internal
changes, the current patchset does not hinder an eventual improvement
later on. For patch 3, it would be good to at least put the test for
the hid-multitouch driver in a separate function. For patch 4, the
variable name "multitouch" is a bit too generic given the very
specific usage.

Thanks,
Henrik

[PATCH 2/5] HID: multitouch: fix handling of buggy reports descriptors for Dell ST2220T

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

LG multitouch panels, such as the one found in Dell ST2220T, has buggy
reports descriptors. With the previous implementation, it was impossible
to rely on the reports descriptors to determine how the different
touches are emitted from the device.

This patch changes the splitting of the different touches in the report
in a more robust way.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---

This one gives me warning under Ubuntu (11.10 and 12.04 at least):
drivers/hid/hid-multitouch.c: In function 'mt_input_mapping':
arch/x86/include/asm/bitops.h:312:8: warning: array subscript is above array bounds [-Warray-bounds]
arch/x86/include/asm/bitops.h:312:8: warning: array subscript is above array bounds [-Warray-bounds]

I don't get any warnings under Fedora. Any help would be great.

Thanks,
Benjamin


 drivers/hid/hid-multitouch.c |   64 ++++++++++++++++-------------------------
 1 files changed, 25 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 019de83..2088ab4 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -75,7 +75,6 @@ struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
-	int last_mt_collection;	/* last known mt-related collection */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
 	__s8 maxcontact_report_id;	/* Maximum Contact Number HID feature,
 				   -1 if non-existent */
@@ -273,6 +272,13 @@ static void set_abs(struct input_dev *input, unsigned int code,
 	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
 }
 
+static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
+		struct hid_input *hi)
+{
+	if (!test_bit(usage->hid, hi->input->absbit))
+		td->last_slot_field = usage->hid;
+}
+
 static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
@@ -321,10 +327,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_X, field, cls->sn_move);
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_GD_Y:
 			hid_map_usage(hi, usage, bit, max,
@@ -333,10 +337,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_Y, field, cls->sn_move);
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		}
 		return 0;
@@ -344,24 +346,18 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONFIDENCE:
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPSWITCH:
 			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
 			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTID:
 			if (!td->maxcontacts)
@@ -369,17 +365,14 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			input_mt_init_slots(hi->input, td->maxcontacts);
 			td->last_slot_field = usage->hid;
 			td->last_field_index = field->index;
-			td->last_mt_collection = usage->collection_index;
 			return 1;
 		case HID_DG_WIDTH:
 			hid_map_usage(hi, usage, bit, max,
 					EV_ABS, ABS_MT_TOUCH_MAJOR);
 			set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
 				cls->sn_width);
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_HEIGHT:
 			hid_map_usage(hi, usage, bit, max,
@@ -388,10 +381,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_height);
 			input_set_abs_params(hi->input,
 					ABS_MT_ORIENTATION, 0, 1, 0, 0);
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPPRESSURE:
 			hid_map_usage(hi, usage, bit, max,
@@ -401,20 +392,16 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_PRESSURE, field,
 				cls->sn_pressure);
-			if (td->last_mt_collection == usage->collection_index) {
-				td->last_slot_field = usage->hid;
-				td->last_field_index = field->index;
-			}
+			set_last_slot_field(usage, td, hi);
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTCOUNT:
-			if (td->last_mt_collection == usage->collection_index)
-				td->last_field_index = field->index;
+			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTMAX:
 			/* we don't set td->last_slot_field as contactcount and
 			 * contact max are global to the report */
-			if (td->last_mt_collection == usage->collection_index)
-				td->last_field_index = field->index;
+			td->last_field_index = field->index;
 			return -1;
 		}
 		case HID_DG_TOUCH:
@@ -670,7 +657,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	td->mtclass = *mtclass;
 	td->inputmode = -1;
 	td->maxcontact_report_id = -1;
-	td->last_mt_collection = -1;
 	hid_set_drvdata(hdev, td);
 
 	ret = hid_parse(hdev);
-- 
1.7.7.6

Re: [PATCH 2/5] HID: multitouch: fix handling of buggy reports descriptors for Dell ST2220T

[Jiri Kosina]

On Tue, 6 Mar 2012, benjamin.tissoires wrote:

> From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> 
> LG multitouch panels, such as the one found in Dell ST2220T, has buggy
> reports descriptors. With the previous implementation, it was impossible
> to rely on the reports descriptors to determine how the different
> touches are emitted from the device.
> 
> This patch changes the splitting of the different touches in the report
> in a more robust way.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
> 
> This one gives me warning under Ubuntu (11.10 and 12.04 at least):
> drivers/hid/hid-multitouch.c: In function 'mt_input_mapping':
> arch/x86/include/asm/bitops.h:312:8: warning: array subscript is above array bounds [-Warray-bounds]
> arch/x86/include/asm/bitops.h:312:8: warning: array subscript is above array bounds [-Warray-bounds]
> 
> I don't get any warnings under Fedora. Any help would be great.

I am not getting any with my 4.3.4 and I am not able to spot any problem 
in that function. Seems like a gcc bug to me.

-- 
Jiri Kosina
SUSE Labs

[PATCH 3/5] HID: handle all multitouch devices through hid-multitouch

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

When the quirk HID_QUIRK_MULTITOUCH is present and when hid-multitouch
is loaded, let's pass the device to hid-multitouch even if it has
not been registered in hid-multitouch.

If any other driver wants to take precedence over hid-multitouch,
the usual way of adding it to hid_have_special_driver will work as
the quirk HID_QUIRK_MULTITOUCH won't be set by the generic hid layer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-core.c       |   12 +++++++++---
 drivers/hid/hid-multitouch.c |   11 +++++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 171e6ed..2390e00 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -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;
 	}
 
@@ -1663,6 +1662,10 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
 	struct hid_driver *hdrv = container_of(drv, struct hid_driver, driver);
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 
+	if ((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
+		!strncmp(hdrv->name, "hid-multitouch", 14))
+		return 1;
+
 	if (!hid_match_device(hdev, hdrv))
 		return 0;
 
@@ -1687,8 +1690,11 @@ static int hid_device_probe(struct device *dev)
 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
 		if (id == NULL) {
-			ret = -ENODEV;
-			goto unlock;
+			if (!((hdev->quirks & HID_QUIRK_MULTITOUCH) &&
+				!strncmp(hdrv->name, "hid-multitouch", 14))) {
+				ret = -ENODEV;
+				goto unlock;
+			}
 		}
 
 		hdev->driver = hdrv;
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2088ab4..a61ba42 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -637,10 +637,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 */
 
-	for (i = 0; mt_classes[i].name ; i++) {
-		if (id->driver_data == mt_classes[i].name) {
-			mtclass = &(mt_classes[i]);
-			break;
+	if (id) {
+		for (i = 0; mt_classes[i].name ; i++) {
+			if (id->driver_data == mt_classes[i].name) {
+				mtclass = &(mt_classes[i]);
+				break;
+			}
 		}
 	}
 
@@ -648,6 +650,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	 * that emit events over several HID messages.
 	 */
 	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+	hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
 
 	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
-- 
1.7.7.6

[PATCH 4/5] HID: autoload hid-multitouch as needed

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

The code is inspired from the one present in the bttv module.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---

As I mentioned in the mail 0/5, I'd really like to have your opinion on this
one. I copied the code from bttv, but it forces us to change hid_device which
is not very good for ABI changes reasons.

Thanks,
Benjamin

 drivers/hid/hid-core.c |   27 +++++++++++++++++++++++++++
 include/linux/hid.h    |    4 ++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 2390e00..0dcb6c6 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1204,6 +1204,28 @@ static struct bin_attribute dev_bin_attr_report_desc = {
 	.size = HID_MAX_DESCRIPTOR_SIZE,
 };
 
+#if defined(CONFIG_MODULES) && defined(MODULE)
+static void hid_request_module_async(struct work_struct *work)
+{
+	request_module("hid-multitouch");
+}
+
+static void hid_request_hid_multitouch(struct hid_device *dev)
+{
+	dev->multitouch = 1;
+	INIT_WORK(&dev->request_module_wk, hid_request_module_async);
+	schedule_work(&dev->request_module_wk);
+}
+
+static void hid_flush_request_modules(struct hid_device *dev)
+{
+	flush_work_sync(&dev->request_module_wk);
+}
+#else
+#define hid_request_hid_multitouch(dev)
+#define hid_flush_request_modules(dev)
+#endif /* CONFIG_MODULES */
+
 int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 {
 	static const char *types[] = { "Device", "Pointer", "Mouse", "Device",
@@ -1230,6 +1252,7 @@ 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 */
+		hid_request_hid_multitouch(hdev);
 		return -ENODEV;
 	}
 
@@ -2089,6 +2112,8 @@ struct hid_device *hid_allocate_device(void)
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
 
+	hdev->multitouch = 0;
+
 	return hdev;
 err:
 	put_device(&hdev->dev);
@@ -2103,6 +2128,8 @@ static void hid_remove_device(struct hid_device *hdev)
 		hid_debug_unregister(hdev);
 		hdev->status &= ~HID_STAT_ADDED;
 	}
+	if (hdev->multitouch)
+		hid_flush_request_modules(hdev);
 }
 
 /**
diff --git a/include/linux/hid.h b/include/linux/hid.h
index c235e4e..79f484b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -498,6 +498,10 @@ struct hid_device {							/* device report descriptor */
 
 	void *driver_data;
 
+	/* used to make hid-multitouch autoloadable */
+	struct work_struct request_module_wk;
+	bool multitouch;
+
 	/* temporary hid_ff handling (until moved to the drivers) */
 	int (*ff_init)(struct hid_device *);
 
-- 
1.7.7.6

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Jiri Kosina]

On Tue, 6 Mar 2012, benjamin.tissoires wrote:

> From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> 
> The code is inspired from the one present in the bttv module.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
> 
> As I mentioned in the mail 0/5, I'd really like to have your opinion on this
> one. I copied the code from bttv, but it forces us to change hid_device which
> is not very good for ABI changes reasons.

Haven't closely inspected the patch yet, I will comment on it later. But 
generally, I wouldn't be worried about changing hid_device per se ... it's 
not really an ABI, it's not exported to userspace.

It's internal kernel-ABI, yes. But we are free to change that in any way 
we wish.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Henrik Rydberg]

On Wed, Mar 07, 2012 at 10:36:41PM +0100, Jiri Kosina wrote:
> On Tue, 6 Mar 2012, benjamin.tissoires wrote:
> 
> > From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> > 
> > The code is inspired from the one present in the bttv module.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> > ---
> > 
> > As I mentioned in the mail 0/5, I'd really like to have your opinion on this
> > one. I copied the code from bttv, but it forces us to change hid_device which
> > is not very good for ABI changes reasons.
> 
> Haven't closely inspected the patch yet, I will comment on it later. But 
> generally, I wouldn't be worried about changing hid_device per se ... it's 
> not really an ABI, it's not exported to userspace.
> 
> It's internal kernel-ABI, yes. But we are free to change that in any way 
> we wish.
> 
> -- 

Thanks for having another go at this problem. Obviously, it is hard to
find a completely satisfactory solution to auto-loading in the current
hid framework. This latest attempt somehow comes close to the smallest
hackery that makes it possible. If we want to go beyond that, I think
we need to restructure more than the internal hid device
representation.

What if we were to change the definition of a HID device on the
modalias level?

In practise, a HID device can be either an usb device, a hid device, a
single interface on a usb bus, a special class determined by examining
the reports, etc. Yet, the hid modalias contains only bus type, vendor
and product id. This is true for the generic usb and bluetooth drivers
(and some very special drivers), but not really for the other devices.
If we were to extend the modalias description, we could cater for a
whole tree of hid devices. For instance, the usb id 1234 could be
handled by the generic usb bus driver. The multitouch sub-device
1234:MT could be handled by hid-multitouch. The mouse device
1234:Mouse could be handled by some other driver, etc. All the driver
handling could be automated in userland using the same udev mechanism
we have today, if only the hid uevents were modified to incorporate
the needed extra information.

Would this be a problematic change in userland, or would it be a
feasible, in principle?

Thanks,
Henrik

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Stéphane Chatty]

Le 8 mars 2012 à 11:57, Henrik Rydberg a écrit :

> On Wed, Mar 07, 2012 at 10:36:41PM +0100, Jiri Kosina wrote:
>> On Tue, 6 Mar 2012, benjamin.tissoires wrote:
>> 
>>> From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>>> 
>>> The code is inspired from the one present in the bttv module.
>>> 
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>>> ---
>>> 
>>> As I mentioned in the mail 0/5, I'd really like to have your opinion on this
>>> one. I copied the code from bttv, but it forces us to change hid_device which
>>> is not very good for ABI changes reasons.
>> 
>> Haven't closely inspected the patch yet, I will comment on it later. But 
>> generally, I wouldn't be worried about changing hid_device per se ... it's 
>> not really an ABI, it's not exported to userspace.
>> 
>> It's internal kernel-ABI, yes. But we are free to change that in any way 
>> we wish.
>> 
>> -- 
> 
> Thanks for having another go at this problem. Obviously, it is hard to
> find a completely satisfactory solution to auto-loading in the current
> hid framework. This latest attempt somehow comes close to the smallest
> hackery that makes it possible. If we want to go beyond that, I think
> we need to restructure more than the internal hid device
> representation.

Yes. I'm not sure it is what you have in mind, but my personal feeling is that the parsing of report descriptors could be restructured to make things clearer. It was designed with a simple HID-field-to-evdev-field mapping in mind, and multitouch breaks this.


> 
> What if we were to change the definition of a HID device on the
> modalias level?
> 
> In practise, a HID device can be either an usb device, a hid device,

Just to be sure: do you mean "bluetooth device"? or is there such a thing as a hid device per se? I'm asking because I've always been surprised at seeing usbhid/ in hid/, which kind of breaks the potential symmetry between USB and Bluetooth wrt hid.

> a single interface on a usb bus, a special class determined by examining
> the reports, etc. Yet, the hid modalias contains only bus type, vendor
> and product id. This is true for the generic usb and bluetooth drivers
> (and some very special drivers), but not really for the other devices.
> If we were to extend the modalias description, we could cater for a
> whole tree of hid devices. For instance, the usb id 1234 could be
> handled by the generic usb bus driver. The multitouch sub-device
> 1234:MT could be handled by hid-multitouch. The mouse device
> 1234:Mouse could be handled by some other driver, etc. All the driver
> handling could be automated in userland using the same udev mechanism
> we have today, if only the hid uevents were modified to incorporate
> the needed extra information.

No comments on this, I need to read up on modaliases before being able to comment at all. I have a vague feeling that we are going to end up debating where it is decided to assign a device to a driver (today it's done in the kernel, you seem to suggest userland), but I know too little about modaliases to be sure.


> 
> Would this be a problematic change in userland, or would it be a
> feasible, in principle?
> 
> Thanks,
> 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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Henrik Rydberg]

Hi Stéphane,

> > What if we were to change the definition of a HID device on the
> > modalias level?
> > 
> > In practise, a HID device can be either an usb device, a hid device,
> 
> Just to be sure: do you mean "bluetooth device"? or is there such a
> thing as a hid device per se? I'm asking because I've always been
> surprised at seeing usbhid/ in hid/, which kind of breaks the
> potential symmetry between USB and Bluetooth wrt hid.

Oops, yes, I meant bluetooth devices.

> > a single interface on a usb bus, a special class determined by examining
> > the reports, etc. Yet, the hid modalias contains only bus type, vendor
> > and product id. This is true for the generic usb and bluetooth drivers
> > (and some very special drivers), but not really for the other devices.
> > If we were to extend the modalias description, we could cater for a
> > whole tree of hid devices. For instance, the usb id 1234 could be
> > handled by the generic usb bus driver. The multitouch sub-device
> > 1234:MT could be handled by hid-multitouch. The mouse device
> > 1234:Mouse could be handled by some other driver, etc. All the driver
> > handling could be automated in userland using the same udev mechanism
> > we have today, if only the hid uevents were modified to incorporate
> > the needed extra information.
> 
> No comments on this, I need to read up on modaliases before being
> able to comment at all. I have a vague feeling that we are going to
> end up debating where it is decided to assign a device to a driver
> (today it's done in the kernel, you seem to suggest userland), but I
> know too little about modaliases to be sure.

The device-driver matching is done in the kernel, but driver (aka
module) loading is done from userland. The crux is to be able to tell
userland what driver to load for a certain device. In this case, it
means giving more information to userland via the device/modalias
construct. Or at least, that is the question. :-)

Thanks,
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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Stéphane Chatty]

Le 8 mars 2012 à 12:30, Henrik Rydberg a écrit :
> 
> The device-driver matching is done in the kernel, but driver (aka
> module) loading is done from userland. The crux is to be able to tell
> userland what driver to load for a certain device. In this case, it
> means giving more information to userland via the device/modalias
> construct. Or at least, that is the question. :-)
> 

Oook, I understand. The current solution used by distros is to load the hid module statically, but then we don't want to do it for every new kind of device-specific module, right? Is this your point? 

I had the feeling that the current hid code was somehow trying to be universal, thus making the point moot, and figured that we would need to reintegrate hid-multitouch into hid-core at some point. You're suggesting another, more distributed option, in which new categories of HID devices appear from time to time and are handled in new modules, right? Sounds interesting to me, I'd like to hear what the original designers of the hid module think of it.

Cheers,

St.

--
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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Henrik Rydberg]

On Thu, Mar 08, 2012 at 12:48:24PM +0100, Stéphane Chatty wrote:
> 
> Le 8 mars 2012 à 12:30, Henrik Rydberg a écrit :
> > 
> > The device-driver matching is done in the kernel, but driver (aka
> > module) loading is done from userland. The crux is to be able to tell
> > userland what driver to load for a certain device. In this case, it
> > means giving more information to userland via the device/modalias
> > construct. Or at least, that is the question. :-)
> > 
> 
> Oook, I understand. The current solution used by distros is to load
> the hid module statically, but then we don't want to do it for every
> new kind of device-specific module, right? Is this your point?

Yes, half of it. The other half is the observation that the problem of
device-driver assignment we face for hid devices seems to originate in
the definition of what a hid device is, i.e., the hid device
identifier. If the identifier was more detailed and one could
construct a new hid device dynamically from the reports, the problem
would go away. Consequentually, a more detailed identifier would solve
the module-loading problem as well. The major concern I have is if
this is feasible from a compatibility point of view.

> I had the feeling that the current hid code was somehow trying to be
> universal, thus making the point moot, and figured that we would
> need to reintegrate hid-multitouch into hid-core at some
> point. You're suggesting another, more distributed option, in which
> new categories of HID devices appear from time to time and are
> handled in new modules, right? Sounds interesting to me, I'd like to
> hear what the original designers of the hid module think of it.

Me too. Reading through the module code, it really seems to put
userland in charge, only providing sufficient information to be able
to load the right modules. If this was fully possible in hid, at least
one of the special blacklists would disappear. One would also be able
to split the current hid-input into several different drivers, for
instance, gaining some clarity in addition to memory savings.

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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Stéphane Chatty]

Le 8 mars 2012 à 13:23, Henrik Rydberg a écrit :

> On Thu, Mar 08, 2012 at 12:48:24PM +0100, Stéphane Chatty wrote:
>> 
>> Le 8 mars 2012 à 12:30, Henrik Rydberg a écrit :
>>> 
>>> The device-driver matching is done in the kernel, but driver (aka
>>> module) loading is done from userland. The crux is to be able to tell
>>> userland what driver to load for a certain device. In this case, it
>>> means giving more information to userland via the device/modalias
>>> construct. Or at least, that is the question. :-)
>>> 
>> 
>> Oook, I understand. The current solution used by distros is to load
>> the hid module statically, but then we don't want to do it for every
>> new kind of device-specific module, right? Is this your point?
> 
> Yes, half of it. The other half is the observation that the problem of
> device-driver assignment we face for hid devices seems to originate in
> the definition of what a hid device is, i.e., the hid device
> identifier. If the identifier was more detailed and one could
> construct a new hid device dynamically from the reports, the problem
> would go away. Consequentually, a more detailed identifier would solve
> the module-loading problem as well. The major concern I have is if
> this is feasible from a compatibility point of view.

Understood. This converges with my concerns about how device classes are handled in the hid core: there is no explicit device class or description of features, and with the new kinds of devices this leads to more and more complex tests in hid-input and in X drivers to reconstruct that information from the evdev axes so as to decide what to do with each device.

A few people in my group, including Benjamin, are developing an experimental event routing server in user space, to support new patterns of input processing. Given that plugging a device is just a particular kind of input, I'd be interested to see what it could do with hotplug events that contain more details about device types :-) BTW, HID reports constitute a richer language than evdev axes, and MacOS and Windows give a growing role to HID. It would be good if at some point we  could define device types based on something with the same kind of expressive power. 

> 
>> I had the feeling that the current hid code was somehow trying to be
>> universal, thus making the point moot, and figured that we would
>> need to reintegrate hid-multitouch into hid-core at some
>> point. You're suggesting another, more distributed option, in which
>> new categories of HID devices appear from time to time and are
>> handled in new modules, right? Sounds interesting to me, I'd like to
>> hear what the original designers of the hid module think of it.
> 
> Me too. Reading through the module code, it really seems to put
> userland in charge, only providing sufficient information to be able
> to load the right modules. If this was fully possible in hid, at least
> one of the special blacklists would disappear. One would also be able
> to split the current hid-input into several different drivers, for
> instance, gaining some clarity in addition to memory savings.

This would be a radical change from the current hid code that tends to restrict "true hid" to a limited range of HID usages and to reject the rest (cf the IS_INPUT_APPLICATION that caused us some difficulties three years ago). But this definitely would make sense to me. Jiri, Dmitry, what do you think?

St.


--
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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Jiri Kosina]

On Thu, 8 Mar 2012, Stéphane Chatty wrote:

> >> I had the feeling that the current hid code was somehow trying to be
> >> universal, thus making the point moot, and figured that we would
> >> need to reintegrate hid-multitouch into hid-core at some
> >> point. You're suggesting another, more distributed option, in which
> >> new categories of HID devices appear from time to time and are
> >> handled in new modules, right? Sounds interesting to me, I'd like to
> >> hear what the original designers of the hid module think of it.
> > 
> > Me too. Reading through the module code, it really seems to put
> > userland in charge, only providing sufficient information to be able
> > to load the right modules. If this was fully possible in hid, at least
> > one of the special blacklists would disappear. One would also be able
> > to split the current hid-input into several different drivers, for
> > instance, gaining some clarity in addition to memory savings.
> 
> This would be a radical change from the current hid code that tends to 
> restrict "true hid" to a limited range of HID usages and to reject the 
> rest (cf the IS_INPUT_APPLICATION that caused us some difficulties three 
> years ago). But this definitely would make sense to me. Jiri, Dmitry, 
> what do you think?

Frankly, I'd personally love IS_INPUT_APPLICATION() to die, it's just 
horribly ugly and doesn't scale with rapidly growing number of HID devices 
appearing every day (with multitouch definitely being the most active 
group).

So restructuring a HID core in a way that makes the whole process much 
more generic and offloads the responsibility into modaliases seems like a 
proper way to go for me.

I'll be thinking about it a little bit more in the upcoming days, other 
ideas are of course very welcome.

Thanks everybody for initiating this discussion,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Jiri Kosina]

On Thu, 8 Mar 2012, Stéphane Chatty wrote:

> > What if we were to change the definition of a HID device on the
> > modalias level?
> > 
> > In practise, a HID device can be either an usb device, a hid device,
> 
> Just to be sure: do you mean "bluetooth device"? or is there such a 
> thing as a hid device per se? I'm asking because I've always been 
> surprised at seeing usbhid/ in hid/, which kind of breaks the potential 
> symmetry between USB and Bluetooth wrt hid.

Please don't get confused by the directory layout ... this has mostly 
non-technical reasons -- Marcel wanted to keep bluetooth/hidp under his 
wings, and I didn't have reasons to object strongly.

I am adding Marcel and Gustavo to CC just in case they have changed their 
mind, but it's definitely a side-topic in this discussion.
Marcel, Gustavo -- if you are interested in the whole discussion regarding 
potential extension of HID core infrastructure to better suit multitouch, 
please see the whole thread at http://marc.info/?t=133105322800006&r=1&w=4

-- 
Jiri Kosina
SUSE Labs
--
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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Marcel Holtmann]

Hi Jiri,

> > > What if we were to change the definition of a HID device on the
> > > modalias level?
> > > 
> > > In practise, a HID device can be either an usb device, a hid device,
> > 
> > Just to be sure: do you mean "bluetooth device"? or is there such a 
> > thing as a hid device per se? I'm asking because I've always been 
> > surprised at seeing usbhid/ in hid/, which kind of breaks the potential 
> > symmetry between USB and Bluetooth wrt hid.
> 
> Please don't get confused by the directory layout ... this has mostly 
> non-technical reasons -- Marcel wanted to keep bluetooth/hidp under his 
> wings, and I didn't have reasons to object strongly.
> 
> I am adding Marcel and Gustavo to CC just in case they have changed their 
> mind, but it's definitely a side-topic in this discussion.

I have not changed my mind. HIDP is Bluetooth specific and should stay
there. Especially since we are currently discussing changes to make
things also work over Bluetooth Low Energy (LE), but that is a complete
different topic and it just started.

> Marcel, Gustavo -- if you are interested in the whole discussion regarding 
> potential extension of HID core infrastructure to better suit multitouch, 
> please see the whole thread at http://marc.info/?t=133105322800006&r=1&w=4

HIDP is just a transport driver in the end. And I heard that the Apple
Magic Trackpad actually does work nicely. Never tried it by myself.

Regards

Marcel

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Stéphane Chatty]

Hi Marcel and Jiri

> Hi Jiri,
> 
>>>> What if we were to change the definition of a HID device on the
>>>> modalias level?
>>>> 
>>>> In practise, a HID device can be either an usb device, a hid device,
>>> 
>>> Just to be sure: do you mean "bluetooth device"? or is there such a 
>>> thing as a hid device per se? I'm asking because I've always been 
>>> surprised at seeing usbhid/ in hid/, which kind of breaks the potential 
>>> symmetry between USB and Bluetooth wrt hid.
>> 
>> Please don't get confused by the directory layout ... this has mostly 
>> non-technical reasons -- Marcel wanted to keep bluetooth/hidp under his 
>> wings, and I didn't have reasons to object strongly.
>> 
>> I am adding Marcel and Gustavo to CC just in case they have changed their 
>> mind, but it's definitely a side-topic in this discussion.
> 
> I have not changed my mind. HIDP is Bluetooth specific and should stay
> there. Especially since we are currently discussing changes to make
> things also work over Bluetooth Low Energy (LE), but that is a complete
> different topic and it just started.

Just in case it makes a difference, I knew nothing about HIDP when I wrote the message above. My point was rather that hid looks like a bus to which several transport layers can connect (USB, Bluetooth, ZigBee, etc), and that having USB-specific code in hid/ (as opposed to having it in usb/) seems biased towards USB. I was (and am still) wondering how much it limits future uses of the hid core by making it USB-dependent.

In other words: is the hid core generic enough or are there steps to take to make it more generic wrt transport layers? If we are talking about restructuring parts of it, this seems like the right time to ask :-)

Cheers,

St.

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Jiri Kosina]

On Mon, 12 Mar 2012, Stéphane Chatty wrote:

> Just in case it makes a difference, I knew nothing about HIDP when I 
> wrote the message above. My point was rather that hid looks like a bus 
> to which several transport layers can connect (USB, Bluetooth, ZigBee, 
> etc), and that having USB-specific code in hid/ (as opposed to having it 
> in usb/) seems biased towards USB. I was (and am still) wondering how 
> much it limits future uses of the hid core by making it USB-dependent.
> 
> In other words: is the hid core generic enough or are there steps to 
> take to make it more generic wrt transport layers? If we are talking 
> about restructuring parts of it, this seems like the right time to ask 
> :-)

Let me answer by a bit of history here. Originally, there have been two 
copies of HID code in the kernel -- one for USB HID devices, one for 
Bluetooth HID devices.
The parsers were not kept in sync, and there was a lot of code 
duplication, creating quite some mess.

What I did back then in 2006 was that I have extracted the abstract HID 
parts into HID core, and made it transport-independent in principle, so 
that both USB HID and Bluetooth HID shared the common infrastructure, 
while implementing different transport protocols.

Then we extended it a little bit further, making HID core a proper bus, to 
which individual drivers (independently on underlying transport protocol 
used) can register.

Currently there are just Bluetooth (hidp) and USB (usbhid) transport 
implementations, with HID core being transport independent.

Hope this helps,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Stéphane Chatty]

Le 12 mars 2012 à 23:21, Jiri Kosina a écrit :

> On Mon, 12 Mar 2012, Stéphane Chatty wrote:
> 
>> Just in case it makes a difference, I knew nothing about HIDP when I 
>> wrote the message above. My point was rather that hid looks like a bus 
>> to which several transport layers can connect (USB, Bluetooth, ZigBee, 
>> etc), and that having USB-specific code in hid/ (as opposed to having it 
>> in usb/) seems biased towards USB. I was (and am still) wondering how 
>> much it limits future uses of the hid core by making it USB-dependent.
>> 
>> In other words: is the hid core generic enough or are there steps to 
>> take to make it more generic wrt transport layers? If we are talking 
>> about restructuring parts of it, this seems like the right time to ask 
>> :-)
> 
> Let me answer by a bit of history here. Originally, there have been two 
> copies of HID code in the kernel -- one for USB HID devices, one for 
> Bluetooth HID devices.
> The parsers were not kept in sync, and there was a lot of code 
> duplication, creating quite some mess.
> 
> What I did back then in 2006 was that I have extracted the abstract HID 
> parts into HID core, and made it transport-independent in principle, so 
> that both USB HID and Bluetooth HID shared the common infrastructure, 
> while implementing different transport protocols.
> 
> Then we extended it a little bit further, making HID core a proper bus, to 
> which individual drivers (independently on underlying transport protocol 
> used) can register.
> 
> Currently there are just Bluetooth (hidp) and USB (usbhid) transport 
> implementations, with HID core being transport independent.
> 
> Hope this helps,

Very useful clarification, thanks. Now, I guess I understand why Marcel wants to keep hidp in bluetooth/. And, to be honest, things would have been clearer to me when I explored the handling of the USB/HID class if I had found a hid (or usbhid) directory in usb/ rather than a usbhid subdirectory in hid/: it did not make the above situation very obvious to me. Don't you think we could go along with Marcel and move usbhid to usb/?

St.




--
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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Jiri Kosina]

On Tue, 13 Mar 2012, Stéphane Chatty wrote:

> > Let me answer by a bit of history here. Originally, there have been 
> > two copies of HID code in the kernel -- one for USB HID devices, one 
> > for Bluetooth HID devices. The parsers were not kept in sync, and 
> > there was a lot of code duplication, creating quite some mess.
> > 
> > What I did back then in 2006 was that I have extracted the abstract HID 
> > parts into HID core, and made it transport-independent in principle, so 
> > that both USB HID and Bluetooth HID shared the common infrastructure, 
> > while implementing different transport protocols.
> > 
> > Then we extended it a little bit further, making HID core a proper bus, to 
> > which individual drivers (independently on underlying transport protocol 
> > used) can register.
> > 
> > Currently there are just Bluetooth (hidp) and USB (usbhid) transport 
> > implementations, with HID core being transport independent.
> > 
> > Hope this helps,
> 
> Very useful clarification, thanks. Now, I guess I understand why Marcel 
> wants to keep hidp in bluetooth/. And, to be honest, things would have 
> been clearer to me when I explored the handling of the USB/HID class if 
> I had found a hid (or usbhid) directory in usb/ rather than a usbhid 
> subdirectory in hid/: it did not make the above situation very obvious 
> to me. Don't you think we could go along with Marcel and move usbhid to 
> usb/?

It seemed to be convenient back then, and it's still convenient for me in 
some sense, as I am maintaining both HID core and USB HID, but bluetooth 
guys are maintaining the bluetooth transport.

But I don't have strong opinion either way, I can as well move usbhid to 
drivers/usb and maintain it there (will need to talk to Greg about it 
first of course). But frankly, I don't see it making things magically 
clear for everyone :)

-- 
Jiri Kosina
SUSE Labs
--
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

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Stéphane Chatty]

Le 13 mars 2012 à 17:13, Jiri Kosina wrote:

> On Tue, 13 Mar 2012, Stéphane Chatty wrote:
>> 
>> Very useful clarification, thanks. Now, I guess I understand why Marcel 
>> wants to keep hidp in bluetooth/. And, to be honest, things would have 
>> been clearer to me when I explored the handling of the USB/HID class if 
>> I had found a hid (or usbhid) directory in usb/ rather than a usbhid 
>> subdirectory in hid/: it did not make the above situation very obvious 
>> to me. Don't you think we could go along with Marcel and move usbhid to 
>> usb/?
> 
> It seemed to be convenient back then, and it's still convenient for me in 
> some sense, as I am maintaining both HID core and USB HID, but bluetooth 
> guys are maintaining the bluetooth transport.
> 
> But I don't have strong opinion either way, I can as well move usbhid to 
> drivers/usb and maintain it there (will need to talk to Greg about it 
> first of course). But frankly, I don't see it making things magically 
> clear for everyone :)


I agree it's neither very important nor a magic bullet. But this confused at least one person (me) when exploring the code for the first time :-)

St.

Re: [PATCH 4/5] HID: autoload hid-multitouch as needed

[Jiri Kosina]

On Tue, 6 Mar 2012, benjamin.tissoires wrote:

> From: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> 
> The code is inspired from the one present in the bttv module.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
> 
> As I mentioned in the mail 0/5, I'd really like to have your opinion on this
> one. I copied the code from bttv, but it forces us to change hid_device which
> is not very good for ABI changes reasons.

It's indeed probably the best and most "lightweight" solution before we 
come up with more robust attitude (with userspace involvement).

Could you please resend the patch with proper changelog entry, and perhaps 
add some more commentary into the code to better explain the reasons for 
this gymnastics?

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH 5/5] HID: multitouch: detect serial protocol

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

Microsoft's documentation about multitouch protocols tells that
if a device presents one touch per report, then it should be treated
as a serial protocol.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-multitouch.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a61ba42..6fb46d7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -81,6 +81,9 @@ struct mt_device {
 	__u8 num_received;	/* how many contacts we received */
 	__u8 num_expected;	/* expected last contact index */
 	__u8 maxcontacts;
+	__u8 touches_by_report;	/* how many touches are present in one report:
+				* 1 means we should use a serial protocol
+				* > 1 means hybrid (multitouch) protocol */
 	bool curvalid;		/* is the current contact valid? */
 	struct mt_slot *slots;
 };
@@ -365,6 +368,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			input_mt_init_slots(hi->input, td->maxcontacts);
 			td->last_slot_field = usage->hid;
 			td->last_field_index = field->index;
+			td->touches_by_report++;
 			return 1;
 		case HID_DG_WIDTH:
 			hid_map_usage(hi, usage, bit, max,
@@ -670,6 +674,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret)
 		goto fail;
 
+	if (!id && td->touches_by_report == 1) {
+		/* the device has been sent by hid-generic */
+		mtclass = &td->mtclass;
+		mtclass->quirks |= MT_QUIRK_ALWAYS_VALID;
+		mtclass->quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
+		mtclass->quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
+		mtclass->quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
+	}
+
 	td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
 				GFP_KERNEL);
 	if (!td->slots) {
-- 
1.7.7.6