[RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Benjamin Tissoires]

Hi,

here is the v3.

all changes are coming from the mails with Henrik:

- restricted to new drivers only
- delete mt_buffer struct
- replace/adapt prev_valid in favor of seen_in_this_frame
- integrated fuzz
- introduce MT_QUIRKS
- other small fixes

Cheers,
Benjamin

[RFC v3 1/5] hid: add feature_mapping callback

[Benjamin Tissoires]

Currently hid doesn't export the features it knows to the specific modules.
Some information can be really important in such features: MosArt and
Cypress devices are by default not in a multitouch mode.
We have to send the value 2 on the right feature.

This patch exports to the module the features report so they can find the
right feature to set up the correct mode.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-input.c |   17 +++++++++++++----
 include/linux/hid.h     |    4 ++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index bb0b365..e1b48e4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -287,6 +287,14 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		goto ignore;
 	}
 
+	if (field->report_type == HID_FEATURE_REPORT) {
+		if (device->driver->feature_mapping) {
+			device->driver->feature_mapping(device, hidinput, field,
+                                usage);
+		}
+		goto ignore;
+	}
+
 	if (device->driver->input_mapping) {
 		int ret = device->driver->input_mapping(device, hidinput, field,
 				usage, &bit, &max);
@@ -836,7 +844,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 	struct hid_input *hidinput = NULL;
 	struct input_dev *input_dev;
 	int i, j, k;
-	int max_report_type = HID_OUTPUT_REPORT;
 
 	INIT_LIST_HEAD(&hid->inputs);
 
@@ -853,10 +860,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			return -1;
 	}
 
-	if (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
-		max_report_type = HID_INPUT_REPORT;
+	for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) {
+		if (k == HID_OUTPUT_REPORT &&
+			hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
+			continue;
 
-	for (k = HID_INPUT_REPORT; k <= max_report_type; k++)
 		list_for_each_entry(report, &hid->report_enum[k].report_list, list) {
 
 			if (!report->maxfield)
@@ -909,6 +917,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 				hidinput = NULL;
 			}
 		}
+	}
 
 	if (hidinput && input_register_device(hidinput->input))
 		goto out_cleanup;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index bb0f56f..75303b0 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -593,6 +593,7 @@ struct hid_usage_id {
  * @report_fixup: called before report descriptor parsing (NULL means nop)
  * @input_mapping: invoked on input registering before mapping an usage
  * @input_mapped: invoked on input registering after mapping an usage
+ * @feature_mapping: invoked on feature registering
  * @suspend: invoked on suspend (NULL means nop)
  * @resume: invoked on resume if device was not reset (NULL means nop)
  * @reset_resume: invoked on resume if device was reset (NULL means nop)
@@ -636,6 +637,9 @@ struct hid_driver {
 	int (*input_mapped)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
 			struct hid_usage *usage, unsigned long **bit, int *max);
+	void (*feature_mapping)(struct hid_device *hdev,
+ 			struct hid_input *hidinput, struct hid_field *field,
+ 			struct hid_usage *usage);
 #ifdef CONFIG_PM
 	int (*suspend)(struct hid_device *hdev, pm_message_t message);
 	int (*resume)(struct hid_device *hdev);
-- 
1.7.3.4

Re: [RFC v3 1/5] hid: add feature_mapping callback

[Henrik Rydberg]

On Fri, Jan 07, 2011 at 07:42:38PM +0100, Benjamin Tissoires wrote:
> Currently hid doesn't export the features it knows to the specific modules.
> Some information can be really important in such features: MosArt and
> Cypress devices are by default not in a multitouch mode.
> We have to send the value 2 on the right feature.
> 
> This patch exports to the module the features report so they can find the
> right feature to set up the correct mode.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---

Hi Benjamin,

the patch series has some impurities (see checkpatch), and some minor comments below, but all in all,

Acked-by: Henrik Rydberg <rydberg@euromail.se>

>  drivers/hid/hid-input.c |   17 +++++++++++++----
>  include/linux/hid.h     |    4 ++++
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index bb0b365..e1b48e4 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -287,6 +287,14 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  		goto ignore;
>  	}
>  
> +	if (field->report_type == HID_FEATURE_REPORT) {
> +		if (device->driver->feature_mapping) {
> +			device->driver->feature_mapping(device, hidinput, field,
> +                                usage);
> +		}
> +		goto ignore;
> +	}
> +
>  	if (device->driver->input_mapping) {
>  		int ret = device->driver->input_mapping(device, hidinput, field,
>  				usage, &bit, &max);
> @@ -836,7 +844,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  	struct hid_input *hidinput = NULL;
>  	struct input_dev *input_dev;
>  	int i, j, k;
> -	int max_report_type = HID_OUTPUT_REPORT;
>  
>  	INIT_LIST_HEAD(&hid->inputs);
>  
> @@ -853,10 +860,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  			return -1;
>  	}
>  
> -	if (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
> -		max_report_type = HID_INPUT_REPORT;
> +	for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) {
> +		if (k == HID_OUTPUT_REPORT &&
> +			hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)

Omitting the parenthesis here is not wrong, but not really customary either.

> +			continue;
>  
> -	for (k = HID_INPUT_REPORT; k <= max_report_type; k++)
>  		list_for_each_entry(report, &hid->report_enum[k].report_list, list) {
>  
>  			if (!report->maxfield)
> @@ -909,6 +917,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  				hidinput = NULL;
>  			}
>  		}
> +	}
>  
>  	if (hidinput && input_register_device(hidinput->input))
>  		goto out_cleanup;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index bb0f56f..75303b0 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -593,6 +593,7 @@ struct hid_usage_id {
>   * @report_fixup: called before report descriptor parsing (NULL means nop)
>   * @input_mapping: invoked on input registering before mapping an usage
>   * @input_mapped: invoked on input registering after mapping an usage
> + * @feature_mapping: invoked on feature registering
>   * @suspend: invoked on suspend (NULL means nop)
>   * @resume: invoked on resume if device was not reset (NULL means nop)
>   * @reset_resume: invoked on resume if device was reset (NULL means nop)
> @@ -636,6 +637,9 @@ struct hid_driver {
>  	int (*input_mapped)(struct hid_device *hdev,
>  			struct hid_input *hidinput, struct hid_field *field,
>  			struct hid_usage *usage, unsigned long **bit, int *max);
> +	void (*feature_mapping)(struct hid_device *hdev,
> + 			struct hid_input *hidinput, struct hid_field *field,
> + 			struct hid_usage *usage);
>  #ifdef CONFIG_PM
>  	int (*suspend)(struct hid_device *hdev, pm_message_t message);
>  	int (*resume)(struct hid_device *hdev);
> -- 
> 1.7.3.4
> 

Thanks,
Henrik

Re: [RFC v3 1/5] hid: add feature_mapping callback

[Benjamin Tissoires]

Oops, I missed the checkpatch for this one.

thanks,
Benjamin

On Fri, Jan 7, 2011 at 7:59 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Jan 07, 2011 at 07:42:38PM +0100, Benjamin Tissoires wrote:
>> Currently hid doesn't export the features it knows to the specific modules.
>> Some information can be really important in such features: MosArt and
>> Cypress devices are by default not in a multitouch mode.
>> We have to send the value 2 on the right feature.
>>
>> This patch exports to the module the features report so they can find the
>> right feature to set up the correct mode.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>
> Hi Benjamin,
>
> the patch series has some impurities (see checkpatch), and some minor comments below, but all in all,
>
> Acked-by: Henrik Rydberg <rydberg@euromail.se>
>
>>  drivers/hid/hid-input.c |   17 +++++++++++++----
>>  include/linux/hid.h     |    4 ++++
>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index bb0b365..e1b48e4 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -287,6 +287,14 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>>               goto ignore;
>>       }
>>
>> +     if (field->report_type == HID_FEATURE_REPORT) {
>> +             if (device->driver->feature_mapping) {
>> +                     device->driver->feature_mapping(device, hidinput, field,
>> +                                usage);
>> +             }
>> +             goto ignore;
>> +     }
>> +
>>       if (device->driver->input_mapping) {
>>               int ret = device->driver->input_mapping(device, hidinput, field,
>>                               usage, &bit, &max);
>> @@ -836,7 +844,6 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>>       struct hid_input *hidinput = NULL;
>>       struct input_dev *input_dev;
>>       int i, j, k;
>> -     int max_report_type = HID_OUTPUT_REPORT;
>>
>>       INIT_LIST_HEAD(&hid->inputs);
>>
>> @@ -853,10 +860,11 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>>                       return -1;
>>       }
>>
>> -     if (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
>> -             max_report_type = HID_INPUT_REPORT;
>> +     for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) {
>> +             if (k == HID_OUTPUT_REPORT &&
>> +                     hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
>
> Omitting the parenthesis here is not wrong, but not really customary either.
>
>> +                     continue;
>>
>> -     for (k = HID_INPUT_REPORT; k <= max_report_type; k++)
>>               list_for_each_entry(report, &hid->report_enum[k].report_list, list) {
>>
>>                       if (!report->maxfield)
>> @@ -909,6 +917,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>>                               hidinput = NULL;
>>                       }
>>               }
>> +     }
>>
>>       if (hidinput && input_register_device(hidinput->input))
>>               goto out_cleanup;
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index bb0f56f..75303b0 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -593,6 +593,7 @@ struct hid_usage_id {
>>   * @report_fixup: called before report descriptor parsing (NULL means nop)
>>   * @input_mapping: invoked on input registering before mapping an usage
>>   * @input_mapped: invoked on input registering after mapping an usage
>> + * @feature_mapping: invoked on feature registering
>>   * @suspend: invoked on suspend (NULL means nop)
>>   * @resume: invoked on resume if device was not reset (NULL means nop)
>>   * @reset_resume: invoked on resume if device was reset (NULL means nop)
>> @@ -636,6 +637,9 @@ struct hid_driver {
>>       int (*input_mapped)(struct hid_device *hdev,
>>                       struct hid_input *hidinput, struct hid_field *field,
>>                       struct hid_usage *usage, unsigned long **bit, int *max);
>> +     void (*feature_mapping)(struct hid_device *hdev,
>> +                     struct hid_input *hidinput, struct hid_field *field,
>> +                     struct hid_usage *usage);
>>  #ifdef CONFIG_PM
>>       int (*suspend)(struct hid_device *hdev, pm_message_t message);
>>       int (*resume)(struct hid_device *hdev);
>> --
>> 1.7.3.4
>>
>
> 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
>
--
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

[RFC v3 2/5] hid: set HID_MAX_FIELD at 128

[Benjamin Tissoires]

Stantums multitouch panels sends more than 64 reports and this results
in not being able to handle all the touches given by this device.

This patch is required to be able to include Stantum panels in the
unified hid-multitouch driver.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
Acked-by: Henrik Rydberg <rydberg@euromail.se>
---
 include/linux/hid.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 75303b0..b76af97 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -402,7 +402,7 @@ struct hid_field {
 	__u16 dpad;			/* dpad input code */
 };
 
-#define HID_MAX_FIELDS 64
+#define HID_MAX_FIELDS 128
 
 struct hid_report {
 	struct list_head list;
-- 
1.7.3.4

[RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Benjamin Tissoires]

Created a driver for PixCir based dual-touch panels, including the one
in the Hanvon tablet.  This is done in a code structure aimed at unifying
support for several existing HID multitouch panels.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
Signed-off-by: Stéphane Chatty <chatty@enac.fr>
---
 drivers/hid/Kconfig          |    9 +
 drivers/hid/Makefile         |    1 +
 drivers/hid/hid-core.c       |    2 +
 drivers/hid/hid-ids.h        |    4 +
 drivers/hid/hid-multitouch.c |  468 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 484 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-multitouch.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 401acec..511554d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -285,6 +285,15 @@ config HID_MONTEREY
 	---help---
 	Support for Monterey Genius KB29E.
 
+config HID_MULTITOUCH
+	tristate "HID Multitouch panels"
+	depends on USB_HID
+	---help---
+	  Generic support for HID multitouch panels.
+
+	  Say Y here if you have one of the following devices:
+	  - PixCir touchscreen
+
 config HID_NTRIG
 	tristate "N-Trig touch screen"
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index c335605..ec991d4 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
 obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
 obj-$(CONFIG_HID_MOSART)	+= hid-mosart.o
+obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
 obj-$(CONFIG_HID_NTRIG)		+= hid-ntrig.o
 obj-$(CONFIG_HID_ORTEK)		+= hid-ortek.o
 obj-$(CONFIG_HID_PRODIKEYS)	+= hid-prodikeys.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 88668ae..2b4d9b9 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1286,6 +1286,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
@@ -1312,6 +1313,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 0f150c7..17b444b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -134,6 +134,7 @@
 #define USB_DEVICE_ID_BTC_EMPREX_REMOTE_2	0x5577
 
 #define USB_VENDOR_ID_CANDO		0x2087
+#define USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH 0x0703
 #define USB_DEVICE_ID_CANDO_MULTI_TOUCH	0x0a01
 #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6 0x0b03
 #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6 0x0f01
@@ -306,6 +307,9 @@
 #define USB_DEVICE_ID_HANWANG_TABLET_FIRST	0x5000
 #define USB_DEVICE_ID_HANWANG_TABLET_LAST	0x8fff
 
+#define USB_VENDOR_ID_HANVON		0x20b3
+#define USB_DEVICE_ID_HANVON_MULTITOUCH	0x0a18
+
 #define USB_VENDOR_ID_HAPP		0x078b
 #define USB_DEVICE_ID_UGCI_DRIVING	0x0010
 #define USB_DEVICE_ID_UGCI_FLYING	0x0020
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
new file mode 100644
index 0000000..3b05dfe
--- /dev/null
+++ b/drivers/hid/hid-multitouch.c
@@ -0,0 +1,468 @@
+/*
+ *  HID driver for multitouch panels
+ *
+ *  Copyright (c) 2010-2011 Stephane Chatty <chatty@enac.fr>
+ *  Copyright (c) 2010-2011 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ *  Copyright (c) 2010-2011 Ecole Nationale de l'Aviation Civile, France
+ *
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/input/mt.h>
+#include "usbhid/usbhid.h"
+
+
+MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");
+MODULE_DESCRIPTION("HID multitouch panels");
+MODULE_LICENSE("GPL");
+
+#include "hid-ids.h"
+
+/* quirks to control the device */
+#define MT_QUIRK_NOT_SEEN_MEANS_UP	(1 << 0)
+#define MT_QUIRK_SLOT_IS_CONTACTID	(1 << 1)
+
+struct mt_slot {
+	__s32 x, y, p, w, h;
+	__s32 contactid;	/* the device ContactID assigned to this slot */
+	bool touch_state;	/* is the touch valid? */
+	bool seen_in_this_frame;/* has this slot been updated */
+};
+
+struct mt_device {
+	struct mt_slot curdata;	/* placeholder of incoming data */
+	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 */
+	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
+	__u8 num_received;	/* how many contacts we received */
+	__u8 num_expected;	/* expected last contact index */
+	bool curvalid;		/* is the current contact valid? */
+	struct mt_slot slots[0];	/* first slot */
+};
+
+struct mt_class {
+	__s32 quirks;
+	__s32 sn_move;	/* Signal/noise ratio for move events */
+	__s32 sn_pressure;	/* Signal/noise ratio for pressure events */
+	__u8 maxcontacts;
+};
+
+/* classes of device behavior */
+#define MT_CLS_DEFAULT 0
+#define MT_CLS_DUAL1 1
+
+/*
+ * these device-dependent functions determine what slot corresponds
+ * to a valid contact that was just read.
+ */
+
+static int slot_is_contactid(struct mt_device *td)
+{
+	return td->curdata.contactid;
+}
+
+static int find_slot_from_contactid(struct mt_device *td)
+{
+	int i;
+	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
+		if (td->slots[i].contactid == td->curdata.contactid &&
+			td->slots[i].touch_state)
+			return i;
+	}
+	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
+		if (!td->slots[i].seen_in_this_frame &&
+			!td->slots[i].touch_state)
+			return i;
+	}
+	return -1;
+	/* should not occurs. If this happens that means
+	 * that the device sent more touches that it says
+	 * in the report descriptor. It is ignored then. */
+}
+
+struct mt_class mt_classes[] = {
+	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
+	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
+};
+
+static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage)
+{
+	if (usage->hid == HID_DG_INPUTMODE) {
+		struct mt_device *td = hid_get_drvdata(hdev);
+		td->inputmode = field->report->id;
+	}
+}
+
+static void set_abs(struct input_dev *input, unsigned int code,
+		struct hid_field *field, int snratio)
+{
+	int fmin = field->logical_minimum;
+	int fmax = field->logical_maximum;
+	int fuzz = snratio ? (fmax - fmin) / snratio : 0;
+	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
+}
+
+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)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	struct mt_class *cls = td->mtclass;
+	switch (usage->hid & HID_USAGE_PAGE) {
+
+	case HID_UP_GENDESK:
+		switch (usage->hid) {
+		case HID_GD_X:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_POSITION_X);
+			set_abs(hi->input, ABS_MT_POSITION_X, field,
+				cls->sn_move);
+			/* touchscreen emulation */
+			set_abs(hi->input, ABS_X, field, cls->sn_move);
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_GD_Y:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_POSITION_Y);
+			set_abs(hi->input, ABS_MT_POSITION_Y, field,
+				cls->sn_move);
+			/* touchscreen emulation */
+			set_abs(hi->input, ABS_Y, field, cls->sn_move);
+			td->last_slot_field = usage->hid;
+			return 1;
+		}
+		return 0;
+
+	case HID_UP_DIGITIZER:
+		switch (usage->hid) {
+		case HID_DG_INRANGE:
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_DG_CONFIDENCE:
+			td->last_slot_field = usage->hid;
+			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);
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_DG_CONTACTID:
+			input_mt_init_slots(hi->input,
+					td->mtclass->maxcontacts);
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_DG_WIDTH:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_TOUCH_MAJOR);
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_DG_HEIGHT:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_TOUCH_MINOR);
+			field->logical_maximum = 1;
+			field->logical_minimum = 1;
+			set_abs(hi->input, ABS_MT_ORIENTATION, field, 0);
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_DG_TIPPRESSURE:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_PRESSURE);
+			set_abs(hi->input, ABS_MT_PRESSURE, field,
+				cls->sn_pressure);
+			/* touchscreen emulation */
+			set_abs(hi->input, ABS_PRESSURE, field,
+				cls->sn_pressure);
+			td->last_slot_field = usage->hid;
+			return 1;
+		case HID_DG_CONTACTCOUNT:
+			td->last_field_index = field->report->maxfield - 1;
+			return 1;
+		case HID_DG_CONTACTMAX:
+			/* we don't set td->last_slot_field as contactcount and
+			 * contact max are global to the report */
+			return -1;
+		}
+		/* let hid-input decide for the others */
+		return 0;
+
+	case 0xff000000:
+		/* we do not want to map these: no input-oriented meaning */
+		return -1;
+	}
+
+	return 0;
+}
+
+static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	if (usage->type == EV_KEY || usage->type == EV_ABS)
+		set_bit(usage->type, hi->input->evbit);
+
+	return -1;
+}
+
+static int mt_compute_slot(struct mt_device *td)
+{
+	struct mt_class *cls = td->mtclass;
+
+	if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
+		return slot_is_contactid(td);
+
+	return find_slot_from_contactid(td);
+}
+
+/*
+ * this function is called when a whole contact has been processed,
+ * so that it can assign it to a slot and store the data there
+ */
+static void mt_complete_slot(struct mt_device *td)
+{
+	if (td->curvalid) {
+		struct mt_slot *slot;
+		int slotnum = mt_compute_slot(td);
+
+		if (slotnum >= 0 && slotnum < td->mtclass->maxcontacts) {
+			slot = td->slots + slotnum;
+
+			memcpy(slot, &(td->curdata), sizeof(struct mt_slot));
+			slot->seen_in_this_frame = true;
+		}
+	}
+	td->num_received++;
+}
+
+
+/*
+ * this function is called when a whole packet has been received and processed,
+ * so that it can decide what to send to the input layer.
+ */
+static void mt_emit_event(struct mt_device *td, struct input_dev *input)
+{
+	int i;
+
+	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
+		struct mt_slot *s = &(td->slots[i]);
+		if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
+			!s->seen_in_this_frame) {
+			/*
+			 * this slot does not contain useful data,
+			 * notify its closure
+			 */
+			s->touch_state = false;
+		}
+
+		input_mt_slot(input, i);
+		input_mt_report_slot_state(input, MT_TOOL_FINGER,
+			s->touch_state);
+		input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
+		input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
+		input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
+		input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, s->w);
+		input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, s->h);
+		s->seen_in_this_frame = false;
+
+	}
+
+	input_mt_report_pointer_emulation(input, true);
+	input_sync(input);
+	td->num_received = 0;
+}
+
+
+
+static int mt_event(struct hid_device *hid, struct hid_field *field,
+				struct hid_usage *usage, __s32 value)
+{
+	struct mt_device *td = hid_get_drvdata(hid);
+
+	if (hid->claimed & HID_CLAIMED_INPUT) {
+		switch (usage->hid) {
+		case HID_DG_INRANGE:
+			break;
+		case HID_DG_TIPSWITCH:
+			td->curvalid = value;
+			td->curdata.touch_state = value;
+			break;
+		case HID_DG_CONFIDENCE:
+			break;
+		case HID_DG_CONTACTID:
+			td->curdata.contactid = value;
+			break;
+		case HID_DG_TIPPRESSURE:
+			td->curdata.p = value;
+			break;
+		case HID_GD_X:
+			td->curdata.x = value;
+			break;
+		case HID_GD_Y:
+			td->curdata.y = value;
+			break;
+		case HID_DG_WIDTH:
+			td->curdata.w = value;
+			break;
+		case HID_DG_HEIGHT:
+			td->curdata.h = value;
+			break;
+		case HID_DG_CONTACTCOUNT:
+			/*
+			 * We must not overwrite the previous value (some
+			 * devices send one sequence splitted over several
+			 * messages)
+			 */
+			if (value)
+				td->num_expected = value - 1;
+			break;
+
+		default:
+			/* fallback to the generic hidinput handling */
+			return 0;
+		}
+	}
+
+	if (usage->hid == td->last_slot_field)
+		mt_complete_slot(td);
+
+	if (field->index == td->last_field_index
+		&& td->num_received > td->num_expected)
+		mt_emit_event(td, field->hidinput->input);
+
+	/* we have handled the hidinput part, now remains hiddev */
+	if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
+		hid->hiddev_hid_event(hid, field, usage, value);
+
+	return 1;
+}
+
+static void mt_set_input_mode(struct hid_device *hdev)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	struct hid_report *r;
+	struct hid_report_enum *re;
+
+	if (td->inputmode < 0)
+		return;
+
+	re = &(hdev->report_enum[HID_FEATURE_REPORT]);
+	r = re->report_id_hash[td->inputmode];
+	if (r) {
+		r->field[0]->value[0] = 0x02;
+		usbhid_submit_report(hdev, r, USB_DIR_OUT);
+	}
+}
+
+static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	struct mt_device *td;
+	struct mt_class *mtclass = mt_classes + id->driver_data;
+
+	/* 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) +
+				mtclass->maxcontacts * sizeof(struct mt_slot),
+				GFP_KERNEL);
+	if (!td) {
+		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
+		return -ENOMEM;
+	}
+	td->mtclass = mtclass;
+	td->inputmode = -1;
+	hid_set_drvdata(hdev, td);
+
+	ret = hid_parse(hdev);
+	if (ret != 0)
+		goto fail;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret != 0)
+		goto fail;
+
+	mt_set_input_mode(hdev);
+
+	return 0;
+
+fail:
+	kfree(td);
+	return ret;
+}
+
+#ifdef CONFIG_PM
+static int mt_reset_resume(struct hid_device *hdev)
+{
+	mt_set_input_mode(hdev);
+	return 0;
+}
+#endif
+
+static void mt_remove(struct hid_device *hdev)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	hid_hw_stop(hdev);
+	kfree(td);
+	hid_set_drvdata(hdev, NULL);
+}
+
+static const struct hid_device_id mt_devices[] = {
+
+	/* PixCir-based panels */
+	{ .driver_data = MT_CLS_DUAL1,
+		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
+			USB_DEVICE_ID_HANVON_MULTITOUCH) },
+	{ .driver_data = MT_CLS_DUAL1,
+		HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
+			USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
+
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, mt_devices);
+
+static const struct hid_usage_id mt_grabbed_usages[] = {
+	{ HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
+	{ HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
+};
+
+static struct hid_driver mt_driver = {
+	.name = "hid-multitouch",
+	.id_table = mt_devices,
+	.probe = mt_probe,
+	.remove = mt_remove,
+	.input_mapping = mt_input_mapping,
+	.input_mapped = mt_input_mapped,
+	.feature_mapping = mt_feature_mapping,
+	.usage_table = mt_grabbed_usages,
+	.event = mt_event,
+#ifdef CONFIG_PM
+	.reset_resume = mt_reset_resume,
+#endif
+};
+
+static int __init mt_init(void)
+{
+	return hid_register_driver(&mt_driver);
+}
+
+static void __exit mt_exit(void)
+{
+	hid_unregister_driver(&mt_driver);
+}
+
+module_init(mt_init);
+module_exit(mt_exit);
-- 
1.7.3.4

Re: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Henrik Rydberg]

On Fri, Jan 07, 2011 at 07:42:40PM +0100, Benjamin Tissoires wrote:
> Created a driver for PixCir based dual-touch panels, including the one
> in the Hanvon tablet.  This is done in a code structure aimed at unifying
> support for several existing HID multitouch panels.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> Signed-off-by: Stéphane Chatty <chatty@enac.fr>
> ---

It looks much better now. Just a few comments and some nit-picks inline.

>  drivers/hid/Kconfig          |    9 +
>  drivers/hid/Makefile         |    1 +
>  drivers/hid/hid-core.c       |    2 +
>  drivers/hid/hid-ids.h        |    4 +
>  drivers/hid/hid-multitouch.c |  468 ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 484 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hid/hid-multitouch.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 401acec..511554d 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -285,6 +285,15 @@ config HID_MONTEREY
>  	---help---
>  	Support for Monterey Genius KB29E.
>  
> +config HID_MULTITOUCH
> +	tristate "HID Multitouch panels"
> +	depends on USB_HID
> +	---help---
> +	  Generic support for HID multitouch panels.
> +
> +	  Say Y here if you have one of the following devices:
> +	  - PixCir touchscreen

Should also mention how to compile as module, and what the module will be called.

> +
>  config HID_NTRIG
>  	tristate "N-Trig touch screen"
>  	depends on USB_HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index c335605..ec991d4 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
>  obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
>  obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
>  obj-$(CONFIG_HID_MOSART)	+= hid-mosart.o
> +obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
>  obj-$(CONFIG_HID_NTRIG)		+= hid-ntrig.o
>  obj-$(CONFIG_HID_ORTEK)		+= hid-ortek.o
>  obj-$(CONFIG_HID_PRODIKEYS)	+= hid-prodikeys.o
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 88668ae..2b4d9b9 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1286,6 +1286,7 @@ static const struct hid_device_id hid_blacklist[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
> @@ -1312,6 +1313,7 @@ static const struct hid_device_id hid_blacklist[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 0f150c7..17b444b 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -134,6 +134,7 @@
>  #define USB_DEVICE_ID_BTC_EMPREX_REMOTE_2	0x5577
>  
>  #define USB_VENDOR_ID_CANDO		0x2087
> +#define USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH 0x0703
>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH	0x0a01
>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6 0x0b03
>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6 0x0f01
> @@ -306,6 +307,9 @@
>  #define USB_DEVICE_ID_HANWANG_TABLET_FIRST	0x5000
>  #define USB_DEVICE_ID_HANWANG_TABLET_LAST	0x8fff
>  
> +#define USB_VENDOR_ID_HANVON		0x20b3
> +#define USB_DEVICE_ID_HANVON_MULTITOUCH	0x0a18
> +
>  #define USB_VENDOR_ID_HAPP		0x078b
>  #define USB_DEVICE_ID_UGCI_DRIVING	0x0010
>  #define USB_DEVICE_ID_UGCI_FLYING	0x0020
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> new file mode 100644
> index 0000000..3b05dfe
> --- /dev/null
> +++ b/drivers/hid/hid-multitouch.c
> @@ -0,0 +1,468 @@
> +/*
> + *  HID driver for multitouch panels
> + *
> + *  Copyright (c) 2010-2011 Stephane Chatty <chatty@enac.fr>
> + *  Copyright (c) 2010-2011 Benjamin Tissoires <benjamin.tissoires@gmail.com>
> + *  Copyright (c) 2010-2011 Ecole Nationale de l'Aviation Civile, France
> + *
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/input/mt.h>
> +#include "usbhid/usbhid.h"
> +
> +
> +MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");
> +MODULE_DESCRIPTION("HID multitouch panels");
> +MODULE_LICENSE("GPL");
> +
> +#include "hid-ids.h"
> +
> +/* quirks to control the device */
> +#define MT_QUIRK_NOT_SEEN_MEANS_UP	(1 << 0)
> +#define MT_QUIRK_SLOT_IS_CONTACTID	(1 << 1)
> +
> +struct mt_slot {
> +	__s32 x, y, p, w, h;
> +	__s32 contactid;	/* the device ContactID assigned to this slot */
> +	bool touch_state;	/* is the touch valid? */
> +	bool seen_in_this_frame;/* has this slot been updated */
> +};
> +
> +struct mt_device {
> +	struct mt_slot curdata;	/* placeholder of incoming data */
> +	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 */
> +	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
> +	__u8 num_received;	/* how many contacts we received */
> +	__u8 num_expected;	/* expected last contact index */

Any particular reason these are u8?

> +	bool curvalid;		/* is the current contact valid? */
> +	struct mt_slot slots[0];	/* first slot */
> +};
> +
> +struct mt_class {
> +	__s32 quirks;
> +	__s32 sn_move;	/* Signal/noise ratio for move events */
> +	__s32 sn_pressure;	/* Signal/noise ratio for pressure events */
> +	__u8 maxcontacts;

Same here - its not like we allocate a million of these anyways. Simple ints should do fine.

> +};
> +
> +/* classes of device behavior */
> +#define MT_CLS_DEFAULT 0
> +#define MT_CLS_DUAL1 1
> +
> +/*
> + * these device-dependent functions determine what slot corresponds
> + * to a valid contact that was just read.
> + */
> +
> +static int slot_is_contactid(struct mt_device *td)
> +{
> +	return td->curdata.contactid;
> +}
> +
> +static int find_slot_from_contactid(struct mt_device *td)
> +{
> +	int i;
> +	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
> +		if (td->slots[i].contactid == td->curdata.contactid &&
> +			td->slots[i].touch_state)
> +			return i;
> +	}
> +	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
> +		if (!td->slots[i].seen_in_this_frame &&
> +			!td->slots[i].touch_state)
> +			return i;
> +	}
> +	return -1;
> +	/* should not occurs. If this happens that means
> +	 * that the device sent more touches that it says
> +	 * in the report descriptor. It is ignored then. */

I would put the comment above the return statement.

> +}
> +
> +struct mt_class mt_classes[] = {
> +	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
> +	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
> +};
> +
> +static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
> +		struct hid_field *field, struct hid_usage *usage)
> +{
> +	if (usage->hid == HID_DG_INPUTMODE) {
> +		struct mt_device *td = hid_get_drvdata(hdev);
> +		td->inputmode = field->report->id;
> +	}
> +}
> +
> +static void set_abs(struct input_dev *input, unsigned int code,
> +		struct hid_field *field, int snratio)
> +{
> +	int fmin = field->logical_minimum;
> +	int fmax = field->logical_maximum;
> +	int fuzz = snratio ? (fmax - fmin) / snratio : 0;
> +	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
> +}
> +
> +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)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	struct mt_class *cls = td->mtclass;
> +	switch (usage->hid & HID_USAGE_PAGE) {
> +
> +	case HID_UP_GENDESK:
> +		switch (usage->hid) {
> +		case HID_GD_X:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_POSITION_X);
> +			set_abs(hi->input, ABS_MT_POSITION_X, field,
> +				cls->sn_move);
> +			/* touchscreen emulation */
> +			set_abs(hi->input, ABS_X, field, cls->sn_move);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_GD_Y:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_POSITION_Y);
> +			set_abs(hi->input, ABS_MT_POSITION_Y, field,
> +				cls->sn_move);
> +			/* touchscreen emulation */
> +			set_abs(hi->input, ABS_Y, field, cls->sn_move);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		}
> +		return 0;
> +
> +	case HID_UP_DIGITIZER:
> +		switch (usage->hid) {
> +		case HID_DG_INRANGE:
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_DG_CONFIDENCE:
> +			td->last_slot_field = usage->hid;
> +			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);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_DG_CONTACTID:
> +			input_mt_init_slots(hi->input,
> +					td->mtclass->maxcontacts);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_DG_WIDTH:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_TOUCH_MAJOR);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_DG_HEIGHT:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_TOUCH_MINOR);
> +			field->logical_maximum = 1;
> +			field->logical_minimum = 1;

minimum should be zero here.

> +			set_abs(hi->input, ABS_MT_ORIENTATION, field, 0);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_DG_TIPPRESSURE:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_PRESSURE);
> +			set_abs(hi->input, ABS_MT_PRESSURE, field,
> +				cls->sn_pressure);
> +			/* touchscreen emulation */
> +			set_abs(hi->input, ABS_PRESSURE, field,
> +				cls->sn_pressure);
> +			td->last_slot_field = usage->hid;
> +			return 1;
> +		case HID_DG_CONTACTCOUNT:
> +			td->last_field_index = field->report->maxfield - 1;
> +			return 1;
> +		case HID_DG_CONTACTMAX:
> +			/* we don't set td->last_slot_field as contactcount and
> +			 * contact max are global to the report */
> +			return -1;
> +		}
> +		/* let hid-input decide for the others */
> +		return 0;
> +
> +	case 0xff000000:
> +		/* we do not want to map these: no input-oriented meaning */
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
> +		struct hid_field *field, struct hid_usage *usage,
> +		unsigned long **bit, int *max)
> +{
> +	if (usage->type == EV_KEY || usage->type == EV_ABS)
> +		set_bit(usage->type, hi->input->evbit);
> +
> +	return -1;
> +}
> +
> +static int mt_compute_slot(struct mt_device *td)
> +{
> +	struct mt_class *cls = td->mtclass;
> +
> +	if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
> +		return slot_is_contactid(td);

No real need for a function call here...

> +
> +	return find_slot_from_contactid(td);
> +}
> +
> +/*
> + * this function is called when a whole contact has been processed,
> + * so that it can assign it to a slot and store the data there
> + */
> +static void mt_complete_slot(struct mt_device *td)
> +{

Adding "td->curdata.seen_in_this_frame = true;" here...

> +	if (td->curvalid) {
> +		struct mt_slot *slot;

Skipping this...

> +		int slotnum = mt_compute_slot(td);
> +
> +		if (slotnum >= 0 && slotnum < td->mtclass->maxcontacts) {
> +			slot = td->slots + slotnum;

and this...

> +
> +			memcpy(slot, &(td->curdata), sizeof(struct mt_slot));

and "td->slots[slotnum] = td->curdata" here...

> +			slot->seen_in_this_frame = true;

and dropping this... looks simpler, ne?

> +		}
> +	}
> +	td->num_received++;
> +}

Writing it explicitly here so you can judge for yourself:

	td->curdata.seen_in_this_frame = true;
	if (td->curvalid) {
		int slot = mt_compute_slot(td);

		if (slot >= 0 && slot < td->mtclass->maxcontacts)
			td->slots[slot] = td->curdata;
	}
	td->num_received++;

> +
> +
> +/*
> + * this function is called when a whole packet has been received and processed,
> + * so that it can decide what to send to the input layer.
> + */
> +static void mt_emit_event(struct mt_device *td, struct input_dev *input)
> +{
> +	int i;
> +
> +	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
> +		struct mt_slot *s = &(td->slots[i]);
> +		if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
> +			!s->seen_in_this_frame) {
> +			/*
> +			 * this slot does not contain useful data,
> +			 * notify its closure
> +			 */

It does contain useful data, it is just assumed to be in the up
state. I would drop the comment and the brackets here, the quirk name
speaks for itself.

> +			s->touch_state = false;
> +		}
> +
> +		input_mt_slot(input, i);
> +		input_mt_report_slot_state(input, MT_TOOL_FINGER,
> +			s->touch_state);

The values below should not be updated for an inactive slot.

> +		input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
> +		input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
> +		input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
> +		input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, s->w);
> +		input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, s->h);
> +		s->seen_in_this_frame = false;
> +
> +	}
> +
> +	input_mt_report_pointer_emulation(input, true);
> +	input_sync(input);
> +	td->num_received = 0;
> +}
> +
> +
> +
> +static int mt_event(struct hid_device *hid, struct hid_field *field,
> +				struct hid_usage *usage, __s32 value)
> +{
> +	struct mt_device *td = hid_get_drvdata(hid);
> +
> +	if (hid->claimed & HID_CLAIMED_INPUT) {
> +		switch (usage->hid) {
> +		case HID_DG_INRANGE:
> +			break;
> +		case HID_DG_TIPSWITCH:
> +			td->curvalid = value;

I do not think curvalid should depend on the touch state (which is
what tipswitch is). Either move to INRANGE, or simply set to true.

> +			td->curdata.touch_state = value;
> +			break;
> +		case HID_DG_CONFIDENCE:
> +			break;
> +		case HID_DG_CONTACTID:
> +			td->curdata.contactid = value;
> +			break;
> +		case HID_DG_TIPPRESSURE:
> +			td->curdata.p = value;
> +			break;
> +		case HID_GD_X:
> +			td->curdata.x = value;
> +			break;
> +		case HID_GD_Y:
> +			td->curdata.y = value;
> +			break;
> +		case HID_DG_WIDTH:
> +			td->curdata.w = value;
> +			break;
> +		case HID_DG_HEIGHT:
> +			td->curdata.h = value;
> +			break;
> +		case HID_DG_CONTACTCOUNT:
> +			/*
> +			 * We must not overwrite the previous value (some
> +			 * devices send one sequence splitted over several
> +			 * messages)
> +			 */

How about "Includes multi-packet support where subsequent packets are sent with zero contactcount."

> +			if (value)
> +				td->num_expected = value - 1;

The - 1 should be dropped here.

> +			break;
> +
> +		default:
> +			/* fallback to the generic hidinput handling */
> +			return 0;
> +		}
> +	}
> +
> +	if (usage->hid == td->last_slot_field)
> +		mt_complete_slot(td);
> +
> +	if (field->index == td->last_field_index
> +		&& td->num_received > td->num_expected)

A ">=" here.

> +		mt_emit_event(td, field->hidinput->input);
> +
> +	/* we have handled the hidinput part, now remains hiddev */
> +	if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
> +		hid->hiddev_hid_event(hid, field, usage, value);
> +
> +	return 1;
> +}
> +
> +static void mt_set_input_mode(struct hid_device *hdev)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	struct hid_report *r;
> +	struct hid_report_enum *re;
> +
> +	if (td->inputmode < 0)
> +		return;
> +
> +	re = &(hdev->report_enum[HID_FEATURE_REPORT]);
> +	r = re->report_id_hash[td->inputmode];
> +	if (r) {
> +		r->field[0]->value[0] = 0x02;
> +		usbhid_submit_report(hdev, r, USB_DIR_OUT);
> +	}
> +}
> +
> +static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int ret;
> +	struct mt_device *td;
> +	struct mt_class *mtclass = mt_classes + id->driver_data;
> +
> +	/* 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) +
> +				mtclass->maxcontacts * sizeof(struct mt_slot),
> +				GFP_KERNEL);
> +	if (!td) {
> +		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
> +		return -ENOMEM;
> +	}
> +	td->mtclass = mtclass;
> +	td->inputmode = -1;
> +	hid_set_drvdata(hdev, td);
> +
> +	ret = hid_parse(hdev);
> +	if (ret != 0)
> +		goto fail;

"if (ret)" is very standard.

> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret != 0)
> +		goto fail;
> +
> +	mt_set_input_mode(hdev);
> +
> +	return 0;
> +
> +fail:
> +	kfree(td);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM
> +static int mt_reset_resume(struct hid_device *hdev)
> +{
> +	mt_set_input_mode(hdev);
> +	return 0;
> +}
> +#endif
> +
> +static void mt_remove(struct hid_device *hdev)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	hid_hw_stop(hdev);
> +	kfree(td);
> +	hid_set_drvdata(hdev, NULL);
> +}
> +
> +static const struct hid_device_id mt_devices[] = {
> +
> +	/* PixCir-based panels */
> +	{ .driver_data = MT_CLS_DUAL1,
> +		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
> +			USB_DEVICE_ID_HANVON_MULTITOUCH) },
> +	{ .driver_data = MT_CLS_DUAL1,
> +		HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
> +			USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, mt_devices);
> +
> +static const struct hid_usage_id mt_grabbed_usages[] = {
> +	{ HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
> +	{ HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
> +};
> +
> +static struct hid_driver mt_driver = {
> +	.name = "hid-multitouch",
> +	.id_table = mt_devices,
> +	.probe = mt_probe,
> +	.remove = mt_remove,
> +	.input_mapping = mt_input_mapping,
> +	.input_mapped = mt_input_mapped,
> +	.feature_mapping = mt_feature_mapping,
> +	.usage_table = mt_grabbed_usages,
> +	.event = mt_event,
> +#ifdef CONFIG_PM
> +	.reset_resume = mt_reset_resume,
> +#endif
> +};
> +
> +static int __init mt_init(void)
> +{
> +	return hid_register_driver(&mt_driver);
> +}
> +
> +static void __exit mt_exit(void)
> +{
> +	hid_unregister_driver(&mt_driver);
> +}
> +
> +module_init(mt_init);
> +module_exit(mt_exit);
> -- 
> 1.7.3.4
> 

Thanks!
Henrik

Re: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Benjamin Tissoires]

On Fri, Jan 7, 2011 at 8:49 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Jan 07, 2011 at 07:42:40PM +0100, Benjamin Tissoires wrote:
>> Created a driver for PixCir based dual-touch panels, including the one
>> in the Hanvon tablet.  This is done in a code structure aimed at unifying
>> support for several existing HID multitouch panels.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> Signed-off-by: Stéphane Chatty <chatty@enac.fr>
>> ---
>
> It looks much better now. Just a few comments and some nit-picks inline.
>
>>  drivers/hid/Kconfig          |    9 +
>>  drivers/hid/Makefile         |    1 +
>>  drivers/hid/hid-core.c       |    2 +
>>  drivers/hid/hid-ids.h        |    4 +
>>  drivers/hid/hid-multitouch.c |  468 ++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 484 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/hid/hid-multitouch.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 401acec..511554d 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -285,6 +285,15 @@ config HID_MONTEREY
>>       ---help---
>>       Support for Monterey Genius KB29E.
>>
>> +config HID_MULTITOUCH
>> +     tristate "HID Multitouch panels"
>> +     depends on USB_HID
>> +     ---help---
>> +       Generic support for HID multitouch panels.
>> +
>> +       Say Y here if you have one of the following devices:
>> +       - PixCir touchscreen
>
> Should also mention how to compile as module, and what the module will be called.

ok, will do in v4

>
>> +
>>  config HID_NTRIG
>>       tristate "N-Trig touch screen"
>>       depends on USB_HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index c335605..ec991d4 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_HID_MAGICMOUSE)    += hid-magicmouse.o
>>  obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
>>  obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o
>>  obj-$(CONFIG_HID_MOSART)     += hid-mosart.o
>> +obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
>>  obj-$(CONFIG_HID_NTRIG)              += hid-ntrig.o
>>  obj-$(CONFIG_HID_ORTEK)              += hid-ortek.o
>>  obj-$(CONFIG_HID_PRODIKEYS)  += hid-prodikeys.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 88668ae..2b4d9b9 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1286,6 +1286,7 @@ static const struct hid_device_id hid_blacklist[] = {
>>       { HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
>> +     { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
>> @@ -1312,6 +1313,7 @@ static const struct hid_device_id hid_blacklist[] = {
>>       { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
>> +     { HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 0f150c7..17b444b 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -134,6 +134,7 @@
>>  #define USB_DEVICE_ID_BTC_EMPREX_REMOTE_2    0x5577
>>
>>  #define USB_VENDOR_ID_CANDO          0x2087
>> +#define USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH 0x0703
>>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH      0x0a01
>>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6 0x0b03
>>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6 0x0f01
>> @@ -306,6 +307,9 @@
>>  #define USB_DEVICE_ID_HANWANG_TABLET_FIRST   0x5000
>>  #define USB_DEVICE_ID_HANWANG_TABLET_LAST    0x8fff
>>
>> +#define USB_VENDOR_ID_HANVON         0x20b3
>> +#define USB_DEVICE_ID_HANVON_MULTITOUCH      0x0a18
>> +
>>  #define USB_VENDOR_ID_HAPP           0x078b
>>  #define USB_DEVICE_ID_UGCI_DRIVING   0x0010
>>  #define USB_DEVICE_ID_UGCI_FLYING    0x0020
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> new file mode 100644
>> index 0000000..3b05dfe
>> --- /dev/null
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -0,0 +1,468 @@
>> +/*
>> + *  HID driver for multitouch panels
>> + *
>> + *  Copyright (c) 2010-2011 Stephane Chatty <chatty@enac.fr>
>> + *  Copyright (c) 2010-2011 Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> + *  Copyright (c) 2010-2011 Ecole Nationale de l'Aviation Civile, France
>> + *
>> + */
>> +
>> +/*
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/usb.h>
>> +#include <linux/input/mt.h>
>> +#include "usbhid/usbhid.h"
>> +
>> +
>> +MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");
>> +MODULE_DESCRIPTION("HID multitouch panels");
>> +MODULE_LICENSE("GPL");
>> +
>> +#include "hid-ids.h"
>> +
>> +/* quirks to control the device */
>> +#define MT_QUIRK_NOT_SEEN_MEANS_UP   (1 << 0)
>> +#define MT_QUIRK_SLOT_IS_CONTACTID   (1 << 1)
>> +
>> +struct mt_slot {
>> +     __s32 x, y, p, w, h;
>> +     __s32 contactid;        /* the device ContactID assigned to this slot */
>> +     bool touch_state;       /* is the touch valid? */
>> +     bool seen_in_this_frame;/* has this slot been updated */
>> +};
>> +
>> +struct mt_device {
>> +     struct mt_slot curdata; /* placeholder of incoming data */
>> +     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 */
>> +     __s8 inputmode;         /* InputMode HID feature, -1 if non-existent */
>> +     __u8 num_received;      /* how many contacts we received */
>> +     __u8 num_expected;      /* expected last contact index */
>
> Any particular reason these are u8?

Just because we don't need s32 here.

>
>> +     bool curvalid;          /* is the current contact valid? */
>> +     struct mt_slot slots[0];        /* first slot */
>> +};
>> +
>> +struct mt_class {
>> +     __s32 quirks;
>> +     __s32 sn_move;  /* Signal/noise ratio for move events */
>> +     __s32 sn_pressure;      /* Signal/noise ratio for pressure events */
>> +     __u8 maxcontacts;
>
> Same here - its not like we allocate a million of these anyways. Simple ints should do fine.
>
>> +};
>> +
>> +/* classes of device behavior */
>> +#define MT_CLS_DEFAULT 0
>> +#define MT_CLS_DUAL1 1
>> +
>> +/*
>> + * these device-dependent functions determine what slot corresponds
>> + * to a valid contact that was just read.
>> + */
>> +
>> +static int slot_is_contactid(struct mt_device *td)
>> +{
>> +     return td->curdata.contactid;
>> +}
>> +
>> +static int find_slot_from_contactid(struct mt_device *td)
>> +{
>> +     int i;
>> +     for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> +             if (td->slots[i].contactid == td->curdata.contactid &&
>> +                     td->slots[i].touch_state)
>> +                     return i;
>> +     }
>> +     for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> +             if (!td->slots[i].seen_in_this_frame &&
>> +                     !td->slots[i].touch_state)
>> +                     return i;
>> +     }
>> +     return -1;
>> +     /* should not occurs. If this happens that means
>> +      * that the device sent more touches that it says
>> +      * in the report descriptor. It is ignored then. */
>
> I would put the comment above the return statement.

ack.

>
>> +}
>> +
>> +struct mt_class mt_classes[] = {
>> +     { 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
>> +     { MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
>> +};
>> +
>> +static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
>> +             struct hid_field *field, struct hid_usage *usage)
>> +{
>> +     if (usage->hid == HID_DG_INPUTMODE) {
>> +             struct mt_device *td = hid_get_drvdata(hdev);
>> +             td->inputmode = field->report->id;
>> +     }
>> +}
>> +
>> +static void set_abs(struct input_dev *input, unsigned int code,
>> +             struct hid_field *field, int snratio)
>> +{
>> +     int fmin = field->logical_minimum;
>> +     int fmax = field->logical_maximum;
>> +     int fuzz = snratio ? (fmax - fmin) / snratio : 0;
>> +     input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
>> +}
>> +
>> +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)
>> +{
>> +     struct mt_device *td = hid_get_drvdata(hdev);
>> +     struct mt_class *cls = td->mtclass;
>> +     switch (usage->hid & HID_USAGE_PAGE) {
>> +
>> +     case HID_UP_GENDESK:
>> +             switch (usage->hid) {
>> +             case HID_GD_X:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_POSITION_X);
>> +                     set_abs(hi->input, ABS_MT_POSITION_X, field,
>> +                             cls->sn_move);
>> +                     /* touchscreen emulation */
>> +                     set_abs(hi->input, ABS_X, field, cls->sn_move);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_GD_Y:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_POSITION_Y);
>> +                     set_abs(hi->input, ABS_MT_POSITION_Y, field,
>> +                             cls->sn_move);
>> +                     /* touchscreen emulation */
>> +                     set_abs(hi->input, ABS_Y, field, cls->sn_move);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             }
>> +             return 0;
>> +
>> +     case HID_UP_DIGITIZER:
>> +             switch (usage->hid) {
>> +             case HID_DG_INRANGE:
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_DG_CONFIDENCE:
>> +                     td->last_slot_field = usage->hid;
>> +                     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);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_DG_CONTACTID:
>> +                     input_mt_init_slots(hi->input,
>> +                                     td->mtclass->maxcontacts);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_DG_WIDTH:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_TOUCH_MAJOR);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_DG_HEIGHT:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_TOUCH_MINOR);
>> +                     field->logical_maximum = 1;
>> +                     field->logical_minimum = 1;
>
> minimum should be zero here.

Maybe a typo in the original code: we hade set_abs_input_param(..., 1,1,0,0).
I just reorganized.

Changed in v4.

>
>> +                     set_abs(hi->input, ABS_MT_ORIENTATION, field, 0);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_DG_TIPPRESSURE:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_PRESSURE);
>> +                     set_abs(hi->input, ABS_MT_PRESSURE, field,
>> +                             cls->sn_pressure);
>> +                     /* touchscreen emulation */
>> +                     set_abs(hi->input, ABS_PRESSURE, field,
>> +                             cls->sn_pressure);
>> +                     td->last_slot_field = usage->hid;
>> +                     return 1;
>> +             case HID_DG_CONTACTCOUNT:
>> +                     td->last_field_index = field->report->maxfield - 1;
>> +                     return 1;
>> +             case HID_DG_CONTACTMAX:
>> +                     /* we don't set td->last_slot_field as contactcount and
>> +                      * contact max are global to the report */
>> +                     return -1;
>> +             }
>> +             /* let hid-input decide for the others */
>> +             return 0;
>> +
>> +     case 0xff000000:
>> +             /* we do not want to map these: no input-oriented meaning */
>> +             return -1;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>> +             struct hid_field *field, struct hid_usage *usage,
>> +             unsigned long **bit, int *max)
>> +{
>> +     if (usage->type == EV_KEY || usage->type == EV_ABS)
>> +             set_bit(usage->type, hi->input->evbit);
>> +
>> +     return -1;
>> +}
>> +
>> +static int mt_compute_slot(struct mt_device *td)
>> +{
>> +     struct mt_class *cls = td->mtclass;
>> +
>> +     if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
>> +             return slot_is_contactid(td);
>
> No real need for a function call here...

just to prepare for the other patches

>
>> +
>> +     return find_slot_from_contactid(td);
>> +}
>> +
>> +/*
>> + * this function is called when a whole contact has been processed,
>> + * so that it can assign it to a slot and store the data there
>> + */
>> +static void mt_complete_slot(struct mt_device *td)
>> +{
>
> Adding "td->curdata.seen_in_this_frame = true;" here...
>
>> +     if (td->curvalid) {
>> +             struct mt_slot *slot;
>
> Skipping this...
>
>> +             int slotnum = mt_compute_slot(td);
>> +
>> +             if (slotnum >= 0 && slotnum < td->mtclass->maxcontacts) {
>> +                     slot = td->slots + slotnum;
>
> and this...
>
>> +
>> +                     memcpy(slot, &(td->curdata), sizeof(struct mt_slot));
>
> and "td->slots[slotnum] = td->curdata" here...
>
>> +                     slot->seen_in_this_frame = true;
>
> and dropping this... looks simpler, ne?
>
>> +             }
>> +     }
>> +     td->num_received++;
>> +}
>
> Writing it explicitly here so you can judge for yourself:
>
>        td->curdata.seen_in_this_frame = true;
>        if (td->curvalid) {
>                int slot = mt_compute_slot(td);
>
>                if (slot >= 0 && slot < td->mtclass->maxcontacts)
>                        td->slots[slot] = td->curdata;
>        }
>        td->num_received++;

arg. Elementary C: struct are passed by value. snif.
I'll take your code.

>
>> +
>> +
>> +/*
>> + * this function is called when a whole packet has been received and processed,
>> + * so that it can decide what to send to the input layer.
>> + */
>> +static void mt_emit_event(struct mt_device *td, struct input_dev *input)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>> +             struct mt_slot *s = &(td->slots[i]);
>> +             if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
>> +                     !s->seen_in_this_frame) {
>> +                     /*
>> +                      * this slot does not contain useful data,
>> +                      * notify its closure
>> +                      */
>
> It does contain useful data, it is just assumed to be in the up
> state. I would drop the comment and the brackets here, the quirk name
> speaks for itself.

We can drop the comment if you want.

>
>> +                     s->touch_state = false;
>> +             }
>> +
>> +             input_mt_slot(input, i);
>> +             input_mt_report_slot_state(input, MT_TOOL_FINGER,
>> +                     s->touch_state);
>
> The values below should not be updated for an inactive slot.

ack.

>
>> +             input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
>> +             input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
>> +             input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>> +             input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, s->w);
>> +             input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, s->h);
>> +             s->seen_in_this_frame = false;
>> +
>> +     }
>> +
>> +     input_mt_report_pointer_emulation(input, true);
>> +     input_sync(input);
>> +     td->num_received = 0;
>> +}
>> +
>> +
>> +
>> +static int mt_event(struct hid_device *hid, struct hid_field *field,
>> +                             struct hid_usage *usage, __s32 value)
>> +{
>> +     struct mt_device *td = hid_get_drvdata(hid);
>> +
>> +     if (hid->claimed & HID_CLAIMED_INPUT) {
>> +             switch (usage->hid) {
>> +             case HID_DG_INRANGE:
>> +                     break;
>> +             case HID_DG_TIPSWITCH:
>> +                     td->curvalid = value;
>
> I do not think curvalid should depend on the touch state (which is
> what tipswitch is). Either move to INRANGE, or simply set to true.

INRANGE is not all the time used as curvalid as you are saying.
At least for Stantum, curvalid is given by CONFIDENCE.

The solution of giving the value true to curvalid is not working too:
Cypress devices can send their touches over 2 reports of 7 touches,
even if it handles 10 touches.

Solution:
1) td->curvalid = td->num_received < td->mtclass->maxcontacts;
(not sure it will work for all devices)

or 2) introduce MT_QUIRK_VALID_IS_INRANGE and MT_QUIRK_VALID_IS_CONFIDENCE

or 3) let curvalid=value and disable the quirk
MT_QUIRK_NOT_SEEN_MEANS_UP (adding a FixMe comment above)

For v4, I'll use solution 3 (safer for 2.6.38 and after)

>
>> +                     td->curdata.touch_state = value;
>> +                     break;
>> +             case HID_DG_CONFIDENCE:
>> +                     break;
>> +             case HID_DG_CONTACTID:
>> +                     td->curdata.contactid = value;
>> +                     break;
>> +             case HID_DG_TIPPRESSURE:
>> +                     td->curdata.p = value;
>> +                     break;
>> +             case HID_GD_X:
>> +                     td->curdata.x = value;
>> +                     break;
>> +             case HID_GD_Y:
>> +                     td->curdata.y = value;
>> +                     break;
>> +             case HID_DG_WIDTH:
>> +                     td->curdata.w = value;
>> +                     break;
>> +             case HID_DG_HEIGHT:
>> +                     td->curdata.h = value;
>> +                     break;
>> +             case HID_DG_CONTACTCOUNT:
>> +                     /*
>> +                      * We must not overwrite the previous value (some
>> +                      * devices send one sequence splitted over several
>> +                      * messages)
>> +                      */
>
> How about "Includes multi-packet support where subsequent packets are sent with zero contactcount."

if you want

>
>> +                     if (value)
>> +                             td->num_expected = value - 1;
>
> The - 1 should be dropped here.

Here is the really tricky one:

if I drop -1,

the test below will be:
 if (field->index == td->last_field_index
      && td->num_received >= td->num_expected)

but during the init of the device, the kernel receive 1 event though
the driver is not ready.

During this time, num_expected is at 0, and when we receive the
last_field_index: segfault!

To sum up, no, we can't.


>
>> +                     break;
>> +
>> +             default:
>> +                     /* fallback to the generic hidinput handling */
>> +                     return 0;
>> +             }
>> +     }
>> +
>> +     if (usage->hid == td->last_slot_field)
>> +             mt_complete_slot(td);
>> +
>> +     if (field->index == td->last_field_index
>> +             && td->num_received > td->num_expected)
>
> A ">=" here.
>
>> +             mt_emit_event(td, field->hidinput->input);
>> +
>> +     /* we have handled the hidinput part, now remains hiddev */
>> +     if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
>> +             hid->hiddev_hid_event(hid, field, usage, value);
>> +
>> +     return 1;
>> +}
>> +
>> +static void mt_set_input_mode(struct hid_device *hdev)
>> +{
>> +     struct mt_device *td = hid_get_drvdata(hdev);
>> +     struct hid_report *r;
>> +     struct hid_report_enum *re;
>> +
>> +     if (td->inputmode < 0)
>> +             return;
>> +
>> +     re = &(hdev->report_enum[HID_FEATURE_REPORT]);
>> +     r = re->report_id_hash[td->inputmode];
>> +     if (r) {
>> +             r->field[0]->value[0] = 0x02;
>> +             usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> +     }
>> +}
>> +
>> +static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> +     int ret;
>> +     struct mt_device *td;
>> +     struct mt_class *mtclass = mt_classes + id->driver_data;
>> +
>> +     /* 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) +
>> +                             mtclass->maxcontacts * sizeof(struct mt_slot),
>> +                             GFP_KERNEL);
>> +     if (!td) {
>> +             dev_err(&hdev->dev, "cannot allocate multitouch data\n");
>> +             return -ENOMEM;
>> +     }
>> +     td->mtclass = mtclass;
>> +     td->inputmode = -1;
>> +     hid_set_drvdata(hdev, td);
>> +
>> +     ret = hid_parse(hdev);
>> +     if (ret != 0)
>> +             goto fail;
>
> "if (ret)" is very standard.

ok

>
>> +
>> +     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> +     if (ret != 0)
>> +             goto fail;
>> +
>> +     mt_set_input_mode(hdev);
>> +
>> +     return 0;
>> +
>> +fail:
>> +     kfree(td);
>> +     return ret;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int mt_reset_resume(struct hid_device *hdev)
>> +{
>> +     mt_set_input_mode(hdev);
>> +     return 0;
>> +}
>> +#endif
>> +
>> +static void mt_remove(struct hid_device *hdev)
>> +{
>> +     struct mt_device *td = hid_get_drvdata(hdev);
>> +     hid_hw_stop(hdev);
>> +     kfree(td);
>> +     hid_set_drvdata(hdev, NULL);
>> +}
>> +
>> +static const struct hid_device_id mt_devices[] = {
>> +
>> +     /* PixCir-based panels */
>> +     { .driver_data = MT_CLS_DUAL1,
>> +             HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
>> +                     USB_DEVICE_ID_HANVON_MULTITOUCH) },
>> +     { .driver_data = MT_CLS_DUAL1,
>> +             HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
>> +                     USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>> +
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, mt_devices);
>> +
>> +static const struct hid_usage_id mt_grabbed_usages[] = {
>> +     { HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
>> +     { HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
>> +};
>> +
>> +static struct hid_driver mt_driver = {
>> +     .name = "hid-multitouch",
>> +     .id_table = mt_devices,
>> +     .probe = mt_probe,
>> +     .remove = mt_remove,
>> +     .input_mapping = mt_input_mapping,
>> +     .input_mapped = mt_input_mapped,
>> +     .feature_mapping = mt_feature_mapping,
>> +     .usage_table = mt_grabbed_usages,
>> +     .event = mt_event,
>> +#ifdef CONFIG_PM
>> +     .reset_resume = mt_reset_resume,
>> +#endif
>> +};
>> +
>> +static int __init mt_init(void)
>> +{
>> +     return hid_register_driver(&mt_driver);
>> +}
>> +
>> +static void __exit mt_exit(void)
>> +{
>> +     hid_unregister_driver(&mt_driver);
>> +}
>> +
>> +module_init(mt_init);
>> +module_exit(mt_exit);
>> --
>> 1.7.3.4
>>
>
> Thanks!
> Henrik


Thanks for the review,
Benjamin
--
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: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Henrik Rydberg]

> >> +static int mt_compute_slot(struct mt_device *td)
> >> +{
> >> +     struct mt_class *cls = td->mtclass;
> >> +
> >> +     if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
> >> +             return slot_is_contactid(td);
> >
> > No real need for a function call here...
> 
> just to prepare for the other patches

I mean the line could simply read

	return td->curdata.contactid;

[...]
> >> +static int mt_event(struct hid_device *hid, struct hid_field *field,
> >> +                             struct hid_usage *usage, __s32 value)
> >> +{
> >> +     struct mt_device *td = hid_get_drvdata(hid);
> >> +
> >> +     if (hid->claimed & HID_CLAIMED_INPUT) {
> >> +             switch (usage->hid) {
> >> +             case HID_DG_INRANGE:
> >> +                     break;
> >> +             case HID_DG_TIPSWITCH:
> >> +                     td->curvalid = value;
> >
> > I do not think curvalid should depend on the touch state (which is
> > what tipswitch is). Either move to INRANGE, or simply set to true.
> 
> INRANGE is not all the time used as curvalid as you are saying.
> At least for Stantum, curvalid is given by CONFIDENCE.

Yes.

> The solution of giving the value true to curvalid is not working too:
> Cypress devices can send their touches over 2 reports of 7 touches,
> even if it handles 10 touches.

Yes.
 
> Solution:
> 1) td->curvalid = td->num_received < td->mtclass->maxcontacts;
> (not sure it will work for all devices)

For the ones where NOT_SEEN_MEANS_UP, it ought to be true. For egalax,
it is not true.

> 
> or 2) introduce MT_QUIRK_VALID_IS_INRANGE and MT_QUIRK_VALID_IS_CONFIDENCE

Yes, I believe this is a good way to go.
 
> or 3) let curvalid=value and disable the quirk
> MT_QUIRK_NOT_SEEN_MEANS_UP (adding a FixMe comment above)

This would mean setting curvalid = touch_state again, which according
to the discussion is precisely what we should not do. We know we need
to check whether the incoming packet represents data is whether it is
just filling up the transfer protocol size. We just need to identify,
per device, how that is done.

> >> +                     if (value)
> >> +                             td->num_expected = value - 1;
> >
> > The - 1 should be dropped here.
> 
> Here is the really tricky one:
> 
> if I drop -1,
> 
> the test below will be:
>  if (field->index == td->last_field_index
>       && td->num_received >= td->num_expected)
> 
> but during the init of the device, the kernel receive 1 event though
> the driver is not ready.

So the current code really needs to also check whether the driver is
ready to events. Adding a state for it would be appropriate. As it
stands, an unrelated syntatic oddity is covering up this fact.

> 
> During this time, num_expected is at 0, and when we receive the
> last_field_index: segfault!
> 
> To sum up, no, we can't.

Yes we can. :-)

> >> +     if (field->index == td->last_field_index
> >> +             && td->num_received > td->num_expected)
> >
> > A ">=" here.

still applies then.

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: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Benjamin Tissoires]

On Tue, Jan 11, 2011 at 2:00 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>> >> +static int mt_compute_slot(struct mt_device *td)
>> >> +{
>> >> +     struct mt_class *cls = td->mtclass;
>> >> +
>> >> +     if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
>> >> +             return slot_is_contactid(td);
>> >
>> > No real need for a function call here...
>>
>> just to prepare for the other patches
>
> I mean the line could simply read
>
>        return td->curdata.contactid;

If you want, but we will loose consistency in between the different
slot computation functions.

>
> [...]
>> >> +static int mt_event(struct hid_device *hid, struct hid_field *field,
>> >> +                             struct hid_usage *usage, __s32 value)
>> >> +{
>> >> +     struct mt_device *td = hid_get_drvdata(hid);
>> >> +
>> >> +     if (hid->claimed & HID_CLAIMED_INPUT) {
>> >> +             switch (usage->hid) {
>> >> +             case HID_DG_INRANGE:
>> >> +                     break;
>> >> +             case HID_DG_TIPSWITCH:
>> >> +                     td->curvalid = value;
>> >
>> > I do not think curvalid should depend on the touch state (which is
>> > what tipswitch is). Either move to INRANGE, or simply set to true.
>>
>> INRANGE is not all the time used as curvalid as you are saying.
>> At least for Stantum, curvalid is given by CONFIDENCE.
>
> Yes.
>
>> The solution of giving the value true to curvalid is not working too:
>> Cypress devices can send their touches over 2 reports of 7 touches,
>> even if it handles 10 touches.
>
> Yes.
>
>> Solution:
>> 1) td->curvalid = td->num_received < td->mtclass->maxcontacts;
>> (not sure it will work for all devices)
>
> For the ones where NOT_SEEN_MEANS_UP, it ought to be true. For egalax,
> it is not true.

After thinking about it, this case doesn't work at all. It's working
for cypress devices only because we are not using the default slot
retrieval function.
This device sends contactid=0 when the touch is not in range, and
starts for the in range touches at 0 too. The MT_CLS_CYPRESS saved us
here, but it won't save other devices.

>
>>
>> or 2) introduce MT_QUIRK_VALID_IS_INRANGE and MT_QUIRK_VALID_IS_CONFIDENCE
>
> Yes, I believe this is a good way to go.

Will do this way, but it might implies another patch for generaltouch
devices as I don't know if inrange or confidence is used.

>
>> or 3) let curvalid=value and disable the quirk
>> MT_QUIRK_NOT_SEEN_MEANS_UP (adding a FixMe comment above)
>
> This would mean setting curvalid = touch_state again, which according
> to the discussion is precisely what we should not do. We know we need
> to check whether the incoming packet represents data is whether it is
> just filling up the transfer protocol size. We just need to identify,
> per device, how that is done.
>
>> >> +                     if (value)
>> >> +                             td->num_expected = value - 1;
>> >
>> > The - 1 should be dropped here.
>>
>> Here is the really tricky one:
>>
>> if I drop -1,
>>
>> the test below will be:
>>  if (field->index == td->last_field_index
>>       && td->num_received >= td->num_expected)
>>
>> but during the init of the device, the kernel receive 1 event though
>> the driver is not ready.
>
> So the current code really needs to also check whether the driver is
> ready to events. Adding a state for it would be appropriate. As it
> stands, an unrelated syntatic oddity is covering up this fact.
>
>>
>> During this time, num_expected is at 0, and when we receive the
>> last_field_index: segfault!
>>
>> To sum up, no, we can't.
>
> Yes we can. :-)

I'll look into it but I promise nothing.

Thanks,
Benjamin

>
>> >> +     if (field->index == td->last_field_index
>> >> +             && td->num_received > td->num_expected)
>> >
>> > A ">=" here.
>
> still applies then.
>
> 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
>
--
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: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Benjamin Tissoires]

On Tue, Jan 11, 2011 at 2:53 PM, Benjamin Tissoires
<benjamin.tissoires@enac.fr> wrote:
> On Tue, Jan 11, 2011 at 2:00 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>>> >> [...]
>>
>>> >> +                     if (value)
>>> >> +                             td->num_expected = value - 1;
>>> >
>>> > The - 1 should be dropped here.
>>>
>>> Here is the really tricky one:
>>>
>>> if I drop -1,
>>>
>>> the test below will be:
>>>  if (field->index == td->last_field_index
>>>       && td->num_received >= td->num_expected)
>>>
>>> but during the init of the device, the kernel receive 1 event though
>>> the driver is not ready.
>>
>> So the current code really needs to also check whether the driver is
>> ready to events. Adding a state for it would be appropriate. As it
>> stands, an unrelated syntatic oddity is covering up this fact.
>>
>>>
>>> During this time, num_expected is at 0, and when we receive the
>>> last_field_index: segfault!
>>>
>>> To sum up, no, we can't.
>>
>> Yes we can. :-)
>
> I'll look into it but I promise nothing.
>

Hehe, I found the bug:

My tests aginst last field in slot and last field in report were not
guarded by (hid->claimed & HID_CLAIMED_INPUT).
Now it works without the -1.

Cheers,
Benjamin


>>> >> +     if (field->index == td->last_field_index
>>> >> +             && td->num_received > td->num_expected)
>>> >
>>> > A ">=" here.
>>
>> still applies then.
>>
>> 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
>>
>
--
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

[RFC v3 4/5] hid-multitouch: added support for Cypress TrueTouch panels

[Benjamin Tissoires]

Added support for Cypress TrueTouch panels, which detect up to 10 fingers

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
Signed-off-by: Stéphane Chatty <chatty@enac.fr>
---
 drivers/hid/Kconfig          |    1 +
 drivers/hid/hid-core.c       |    1 +
 drivers/hid/hid-ids.h        |    1 +
 drivers/hid/hid-multitouch.c |   19 +++++++++++++++++++
 4 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 511554d..de31d75 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -293,6 +293,7 @@ config HID_MULTITOUCH
 
 	  Say Y here if you have one of the following devices:
 	  - PixCir touchscreen
+	  - Cypress TrueTouch
 
 config HID_NTRIG
 	tristate "N-Trig touch screen"
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 2b4d9b9..e6a86bf 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1298,6 +1298,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH1) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 17b444b..c258c42 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -180,6 +180,7 @@
 #define USB_DEVICE_ID_CYPRESS_BARCODE_1	0xde61
 #define USB_DEVICE_ID_CYPRESS_BARCODE_2	0xde64
 #define USB_DEVICE_ID_CYPRESS_BARCODE_3	0xbca1
+#define USB_DEVICE_ID_CYPRESS_TRUETOUCH	0xc001
 
 #define USB_VENDOR_ID_DEALEXTREAME	0x10c5
 #define USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701	0x819a
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 3b05dfe..7af9f71 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -32,6 +32,7 @@ MODULE_LICENSE("GPL");
 /* quirks to control the device */
 #define MT_QUIRK_NOT_SEEN_MEANS_UP	(1 << 0)
 #define MT_QUIRK_SLOT_IS_CONTACTID	(1 << 1)
+#define MT_QUIRK_CYPRESS	(1 << 2)
 
 struct mt_slot {
 	__s32 x, y, p, w, h;
@@ -62,6 +63,7 @@ struct mt_class {
 /* classes of device behavior */
 #define MT_CLS_DEFAULT 0
 #define MT_CLS_DUAL1 1
+#define MT_CLS_CYPRESS 2
 
 /*
  * these device-dependent functions determine what slot corresponds
@@ -73,6 +75,14 @@ static int slot_is_contactid(struct mt_device *td)
 	return td->curdata.contactid;
 }
 
+static int cypress_compute_slot(struct mt_device *td)
+{
+	if (td->curdata.contactid != 0 || td->num_received == 0)
+		return td->curdata.contactid;
+	else
+		return -1;
+}
+
 static int find_slot_from_contactid(struct mt_device *td)
 {
 	int i;
@@ -95,6 +105,7 @@ static int find_slot_from_contactid(struct mt_device *td)
 struct mt_class mt_classes[] = {
 	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
 	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
+	{ MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
 };
 
 static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -223,6 +234,9 @@ static int mt_compute_slot(struct mt_device *td)
 	if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
 		return slot_is_contactid(td);
 
+	if (cls->quirks & MT_QUIRK_CYPRESS)
+		return cypress_compute_slot(td);
+
 	return find_slot_from_contactid(td);
 }
 
@@ -422,6 +436,11 @@ static void mt_remove(struct hid_device *hdev)
 
 static const struct hid_device_id mt_devices[] = {
 
+	/* Cypress panel */
+	{ .driver_data = MT_CLS_CYPRESS,
+		HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
+			USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
+
 	/* PixCir-based panels */
 	{ .driver_data = MT_CLS_DUAL1,
 		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
-- 
1.7.3.4

Re: [RFC v3 4/5] hid-multitouch: added support for Cypress TrueTouch panels

[Henrik Rydberg]

On Fri, Jan 07, 2011 at 07:42:41PM +0100, Benjamin Tissoires wrote:
> Added support for Cypress TrueTouch panels, which detect up to 10 fingers
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> Signed-off-by: Stéphane Chatty <chatty@enac.fr>
> ---

Hi, just minor things.

>  drivers/hid/Kconfig          |    1 +
>  drivers/hid/hid-core.c       |    1 +
>  drivers/hid/hid-ids.h        |    1 +
>  drivers/hid/hid-multitouch.c |   19 +++++++++++++++++++
>  4 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 511554d..de31d75 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -293,6 +293,7 @@ config HID_MULTITOUCH
>  
>  	  Say Y here if you have one of the following devices:
>  	  - PixCir touchscreen
> +	  - Cypress TrueTouch
>  
>  config HID_NTRIG
>  	tristate "N-Trig touch screen"
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 2b4d9b9..e6a86bf 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1298,6 +1298,7 @@ static const struct hid_device_id hid_blacklist[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH1) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 17b444b..c258c42 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -180,6 +180,7 @@
>  #define USB_DEVICE_ID_CYPRESS_BARCODE_1	0xde61
>  #define USB_DEVICE_ID_CYPRESS_BARCODE_2	0xde64
>  #define USB_DEVICE_ID_CYPRESS_BARCODE_3	0xbca1
> +#define USB_DEVICE_ID_CYPRESS_TRUETOUCH	0xc001
>  
>  #define USB_VENDOR_ID_DEALEXTREAME	0x10c5
>  #define USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701	0x819a
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 3b05dfe..7af9f71 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -32,6 +32,7 @@ MODULE_LICENSE("GPL");
>  /* quirks to control the device */
>  #define MT_QUIRK_NOT_SEEN_MEANS_UP	(1 << 0)
>  #define MT_QUIRK_SLOT_IS_CONTACTID	(1 << 1)
> +#define MT_QUIRK_CYPRESS	(1 << 2)

Missing tab.

>  
>  struct mt_slot {
>  	__s32 x, y, p, w, h;
> @@ -62,6 +63,7 @@ struct mt_class {
>  /* classes of device behavior */
>  #define MT_CLS_DEFAULT 0
>  #define MT_CLS_DUAL1 1
> +#define MT_CLS_CYPRESS 2

Missing tabs... goes for the previous patch as well, coming to think of it.

It does seem slightly complicated, doesn't it. How about dropping
these, and referring to explicit static structures instead?

>  
>  /*
>   * these device-dependent functions determine what slot corresponds
> @@ -73,6 +75,14 @@ static int slot_is_contactid(struct mt_device *td)
>  	return td->curdata.contactid;
>  }
>  
> +static int cypress_compute_slot(struct mt_device *td)
> +{
> +	if (td->curdata.contactid != 0 || td->num_received == 0)
> +		return td->curdata.contactid;
> +	else
> +		return -1;
> +}
> +
>  static int find_slot_from_contactid(struct mt_device *td)
>  {
>  	int i;
> @@ -95,6 +105,7 @@ static int find_slot_from_contactid(struct mt_device *td)
>  struct mt_class mt_classes[] = {
>  	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
>  	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
> +	{ MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
>  };

These could be named structs instead of an array, e.g.,

static const struct mt_class mt_cls_dual1 = {
       .quirks = MT_QUIRK_SLOT_IS_CONTACTID,
       .max_contacts = 2,
};

>  
>  static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
> @@ -223,6 +234,9 @@ static int mt_compute_slot(struct mt_device *td)
>  	if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
>  		return slot_is_contactid(td);
>  
> +	if (cls->quirks & MT_QUIRK_CYPRESS)
> +		return cypress_compute_slot(td);
> +
>  	return find_slot_from_contactid(td);
>  }
>  
> @@ -422,6 +436,11 @@ static void mt_remove(struct hid_device *hdev)
>  
>  static const struct hid_device_id mt_devices[] = {
>  
> +	/* Cypress panel */
> +	{ .driver_data = MT_CLS_CYPRESS,
> +		HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
> +			USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
> +
>  	/* PixCir-based panels */
>  	{ .driver_data = MT_CLS_DUAL1,
>  		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,

Could use pointers here instead.

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: [RFC v3 4/5] hid-multitouch: added support for Cypress TrueTouch panels

[Benjamin Tissoires]

On Fri, Jan 7, 2011 at 9:00 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Jan 07, 2011 at 07:42:41PM +0100, Benjamin Tissoires wrote:
>> Added support for Cypress TrueTouch panels, which detect up to 10 fingers
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> Signed-off-by: Stéphane Chatty <chatty@enac.fr>
>> ---
>
> Hi, just minor things.
>
>>  drivers/hid/Kconfig          |    1 +
>>  drivers/hid/hid-core.c       |    1 +
>>  drivers/hid/hid-ids.h        |    1 +
>>  drivers/hid/hid-multitouch.c |   19 +++++++++++++++++++
>>  4 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 511554d..de31d75 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -293,6 +293,7 @@ config HID_MULTITOUCH
>>
>>         Say Y here if you have one of the following devices:
>>         - PixCir touchscreen
>> +       - Cypress TrueTouch
>>
>>  config HID_NTRIG
>>       tristate "N-Trig touch screen"
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 2b4d9b9..e6a86bf 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1298,6 +1298,7 @@ static const struct hid_device_id hid_blacklist[] = {
>>       { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_2) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_BARCODE_3) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_MOUSE) },
>> +     { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DWAV, USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH1) },
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 17b444b..c258c42 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -180,6 +180,7 @@
>>  #define USB_DEVICE_ID_CYPRESS_BARCODE_1      0xde61
>>  #define USB_DEVICE_ID_CYPRESS_BARCODE_2      0xde64
>>  #define USB_DEVICE_ID_CYPRESS_BARCODE_3      0xbca1
>> +#define USB_DEVICE_ID_CYPRESS_TRUETOUCH      0xc001
>>
>>  #define USB_VENDOR_ID_DEALEXTREAME   0x10c5
>>  #define USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701      0x819a
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 3b05dfe..7af9f71 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -32,6 +32,7 @@ MODULE_LICENSE("GPL");
>>  /* quirks to control the device */
>>  #define MT_QUIRK_NOT_SEEN_MEANS_UP   (1 << 0)
>>  #define MT_QUIRK_SLOT_IS_CONTACTID   (1 << 1)
>> +#define MT_QUIRK_CYPRESS     (1 << 2)
>
> Missing tab.

Ok

>
>>
>>  struct mt_slot {
>>       __s32 x, y, p, w, h;
>> @@ -62,6 +63,7 @@ struct mt_class {
>>  /* classes of device behavior */
>>  #define MT_CLS_DEFAULT 0
>>  #define MT_CLS_DUAL1 1
>> +#define MT_CLS_CYPRESS 2
>
> Missing tabs... goes for the previous patch as well, coming to think of it.

ok and ok

>
> It does seem slightly complicated, doesn't it. How about dropping
> these, and referring to explicit static structures instead?

I don't want people to place those static structures anywhere in the code.

In v4, I've added a field .name and I retrieve it in the mt_probe function.

>
>>
>>  /*
>>   * these device-dependent functions determine what slot corresponds
>> @@ -73,6 +75,14 @@ static int slot_is_contactid(struct mt_device *td)
>>       return td->curdata.contactid;
>>  }
>>
>> +static int cypress_compute_slot(struct mt_device *td)
>> +{
>> +     if (td->curdata.contactid != 0 || td->num_received == 0)
>> +             return td->curdata.contactid;
>> +     else
>> +             return -1;
>> +}
>> +
>>  static int find_slot_from_contactid(struct mt_device *td)
>>  {
>>       int i;
>> @@ -95,6 +105,7 @@ static int find_slot_from_contactid(struct mt_device *td)
>>  struct mt_class mt_classes[] = {
>>       { 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
>>       { MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
>> +     { MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
>>  };
>
> These could be named structs instead of an array, e.g.,
>
> static const struct mt_class mt_cls_dual1 = {
>       .quirks = MT_QUIRK_SLOT_IS_CONTACTID,
>       .max_contacts = 2,
> };
>
>>
>>  static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
>> @@ -223,6 +234,9 @@ static int mt_compute_slot(struct mt_device *td)
>>       if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
>>               return slot_is_contactid(td);
>>
>> +     if (cls->quirks & MT_QUIRK_CYPRESS)
>> +             return cypress_compute_slot(td);
>> +
>>       return find_slot_from_contactid(td);
>>  }
>>
>> @@ -422,6 +436,11 @@ static void mt_remove(struct hid_device *hdev)
>>
>>  static const struct hid_device_id mt_devices[] = {
>>
>> +     /* Cypress panel */
>> +     { .driver_data = MT_CLS_CYPRESS,
>> +             HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>> +                     USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
>> +
>>       /* PixCir-based panels */
>>       { .driver_data = MT_CLS_DUAL1,
>>               HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
>
> Could use pointers here instead.
>
> Thanks!
> Henrik


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

[RFC v3 5/5] hid-mulitouch: added support for the 'Sensing Win7-TwoFinger'

[Benjamin Tissoires]

Added support for the 'Sensing Win7-TwoFinger' panel by GeneralTouch found on some tablets.

Because of conflicting VID/PID, this conflicts with previous support for some single-touch panels by GeneralTouch

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
Signed-off-by: Stéphane Chatty <chatty@enac.fr>
---
 drivers/hid/Kconfig          |    1 +
 drivers/hid/hid-core.c       |    2 +-
 drivers/hid/hid-ids.h        |    1 +
 drivers/hid/hid-multitouch.c |   18 +++++++++++++++++-
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index de31d75..9bd2148 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -294,6 +294,7 @@ config HID_MULTITOUCH
 	  Say Y here if you have one of the following devices:
 	  - PixCir touchscreen
 	  - Cypress TrueTouch
+	  - 'Sensing Win7-TwoFinger' panel by GeneralTouch
 
 config HID_NTRIG
 	tristate "N-Trig touch screen"
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index e6a86bf..a41290d 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1309,6 +1309,7 @@ static const struct hid_device_id hid_blacklist[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_EZKEY, USB_DEVICE_ID_BTC_8193) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PSX_ADAPTOR) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GAMERON, USB_DEVICE_ID_GAMERON_DUAL_PCS_ADAPTOR) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0003) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GREENASIA, 0x0012) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
@@ -1612,7 +1613,6 @@ static const struct hid_device_id hid_ignore_list[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC5UH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC4UM) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, 0x0001) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, 0x0002) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, 0x0003) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH, 0x0004) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index c258c42..a97983e 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -226,6 +226,7 @@
 #define USB_DEVICE_ID_GAMERON_DUAL_PCS_ADAPTOR	0x0002
 
 #define USB_VENDOR_ID_GENERAL_TOUCH	0x0dfc
+#define USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS 0x0001
 
 #define USB_VENDOR_ID_GLAB		0x06c2
 #define USB_DEVICE_ID_4_PHIDGETSERVO_30	0x0038
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 7af9f71..3442ed5 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -33,6 +33,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_NOT_SEEN_MEANS_UP	(1 << 0)
 #define MT_QUIRK_SLOT_IS_CONTACTID	(1 << 1)
 #define MT_QUIRK_CYPRESS	(1 << 2)
+#define MT_QUIRK_SLOT_IS_CONTACTNUMBER	(1 << 3)
 
 struct mt_slot {
 	__s32 x, y, p, w, h;
@@ -63,7 +64,8 @@ struct mt_class {
 /* classes of device behavior */
 #define MT_CLS_DEFAULT 0
 #define MT_CLS_DUAL1 1
-#define MT_CLS_CYPRESS 2
+#define MT_CLS_DUAL2 2
+#define MT_CLS_CYPRESS 3
 
 /*
  * these device-dependent functions determine what slot corresponds
@@ -75,6 +77,11 @@ static int slot_is_contactid(struct mt_device *td)
 	return td->curdata.contactid;
 }
 
+static int slot_is_contactnumber(struct mt_device *td)
+{
+	return td->num_received;
+}
+
 static int cypress_compute_slot(struct mt_device *td)
 {
 	if (td->curdata.contactid != 0 || td->num_received == 0)
@@ -105,6 +112,7 @@ static int find_slot_from_contactid(struct mt_device *td)
 struct mt_class mt_classes[] = {
 	{ 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
 	{ MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
+	{ MT_QUIRK_SLOT_IS_CONTACTNUMBER, 0, 0, 10 },    /* MT_CLS_DUAL2 */
 	{ MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
 };
 
@@ -237,6 +245,9 @@ static int mt_compute_slot(struct mt_device *td)
 	if (cls->quirks & MT_QUIRK_CYPRESS)
 		return cypress_compute_slot(td);
 
+	if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTNUMBER)
+		return slot_is_contactnumber(td);
+
 	return find_slot_from_contactid(td);
 }
 
@@ -441,6 +452,11 @@ static const struct hid_device_id mt_devices[] = {
 		HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
 			USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
 
+	/* GeneralTouch panel */
+	{ .driver_data = MT_CLS_DUAL2,
+		HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
+			USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
+
 	/* PixCir-based panels */
 	{ .driver_data = MT_CLS_DUAL1,
 		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
-- 
1.7.3.4

Re: [RFC v3 5/5] hid-mulitouch: added support for the 'Sensing Win7-TwoFinger'

[Henrik Rydberg]

On Fri, Jan 07, 2011 at 07:42:42PM +0100, Benjamin Tissoires wrote:
> Added support for the 'Sensing Win7-TwoFinger' panel by GeneralTouch found on some tablets.
> 
> Because of conflicting VID/PID, this conflicts with previous support for some single-touch panels by GeneralTouch
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> Signed-off-by: Stéphane Chatty <chatty@enac.fr>
> ---

Other than the class proposal from the previous patch, this looks nice.

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: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Benjamin Tissoires]

I forgot to mention that I keep the flag RFC as I'm waiting for more
tests in the begining of the next week (only Cypress has been tested
with the last release).

Cheers,
Benjamin

On Fri, Jan 7, 2011 at 7:42 PM, Benjamin Tissoires
<benjamin.tissoires@enac.fr> wrote:
> Hi,
>
> here is the v3.
>
> all changes are coming from the mails with Henrik:
>
> - restricted to new drivers only
> - delete mt_buffer struct
> - replace/adapt prev_valid in favor of seen_in_this_frame
> - integrated fuzz
> - introduce MT_QUIRKS
> - other small fixes
>
> Cheers,
> Benjamin
>
>

Re: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Henrik Rydberg]

Hi Benjamin,

> here is the v3.
> 
> all changes are coming from the mails with Henrik:
> 
> - restricted to new drivers only
> - delete mt_buffer struct
> - replace/adapt prev_valid in favor of seen_in_this_frame
> - integrated fuzz
> - introduce MT_QUIRKS
> - other small fixes

A lot of progress in one day - thank you very much for making the
changes. I will be online for quite a bit longer in case there is more
coming this way. With some testing over the weekend (I will be
traveling, sorry), it doesn't look impossible to have this ready early
next week.

Cheers,
Henrik

Re: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Benjamin Tissoires]

On Fri, Jan 7, 2011 at 9:08 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> here is the v3.
>>
>> all changes are coming from the mails with Henrik:
>>
>> - restricted to new drivers only
>> - delete mt_buffer struct
>> - replace/adapt prev_valid in favor of seen_in_this_frame
>> - integrated fuzz
>> - introduce MT_QUIRKS
>> - other small fixes
>
> A lot of progress in one day - thank you very much for making the
> changes. I will be online for quite a bit longer in case there is more
> coming this way. With some testing over the weekend (I will be
> traveling, sorry), it doesn't look impossible to have this ready early
> next week.

For my part, I'm a bit tired, and I don't have any device at home to
test. I wont publish anything for the evening as I will for sure
introduce bugs.

Good night,
Benjamin

>
> 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
>
--
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: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Jiri Kosina]

On Fri, 7 Jan 2011, Benjamin Tissoires wrote:

> Hi,
> 
> here is the v3.
> 
> all changes are coming from the mails with Henrik:
> 
> - restricted to new drivers only
> - delete mt_buffer struct
> - replace/adapt prev_valid in favor of seen_in_this_frame
> - integrated fuzz
> - introduce MT_QUIRKS
> - other small fixes

Hi Bejnamin,

thanks a lot for spinning this up so nicely and quickly.

I have now applied the patchset as-is to the 'multitouch' branch in my 
tree.

It doesn't currently build, as it depends on multitouch updates going 
through Dmitry's tree to Linus. As far as I have seen, Dmitry has already 
sent the pull request, but Linus hasn't pulled yet.
As soon as this happens, I will merge linux-2.6 into this branch to make 
it compilable.

I am planning to do two-round merging this merge window -- in the first 
round, I'll be pushing my current queue, and in the second round there 
will be hid-multitouch, in the first half of the next week, if everything 
goes smoothly.

Henrik, thanks a lot for the review, I really appreciate that.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Jiri Kosina]

On Fri, 7 Jan 2011, Jiri Kosina wrote:

> > here is the v3.
> > 
> > all changes are coming from the mails with Henrik:
> > 
> > - restricted to new drivers only
> > - delete mt_buffer struct
> > - replace/adapt prev_valid in favor of seen_in_this_frame
> > - integrated fuzz
> > - introduce MT_QUIRKS
> > - other small fixes
> 
> Hi Bejnamin,
> 
> thanks a lot for spinning this up so nicely and quickly.
> 
> I have now applied the patchset as-is to the 'multitouch' branch in my 
> tree.

Which means -- could you please send any followup patches on top of that 
branch (i.e. on top of the v3 patchset)? It would help my workflow and 
make the merging process smoother.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Jiri Kosina]

On Sat, 8 Jan 2011, Jiri Kosina wrote:

> > > all changes are coming from the mails with Henrik:
> > > 
> > > - restricted to new drivers only
> > > - delete mt_buffer struct
> > > - replace/adapt prev_valid in favor of seen_in_this_frame
> > > - integrated fuzz
> > > - introduce MT_QUIRKS
> > > - other small fixes
> > 
> > Hi Bejnamin,
> > 
> > thanks a lot for spinning this up so nicely and quickly.
> > 
> > I have now applied the patchset as-is to the 'multitouch' branch in my 
> > tree.
> 
> Which means -- could you please send any followup patches on top of that 
> branch (i.e. on top of the v3 patchset)? It would help my workflow and 
> make the merging process smoother.

'multitouch' brach now builds, as Linus has taken Dmitry's pull request 
and I have merged from Linus.

So once you have fixed for the minor issues raised by Henrik and once you 
have finished your testing, please send the additional fixes to me and I 
will be merging the branch upstream.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Jiri Kosina]

On Sat, 8 Jan 2011, Jiri Kosina wrote:

> > > I have now applied the patchset as-is to the 'multitouch' branch in my 
> > > tree.
> > 
> > Which means -- could you please send any followup patches on top of that 
> > branch (i.e. on top of the v3 patchset)? It would help my workflow and 
> > make the merging process smoother.
> 
> 'multitouch' brach now builds, as Linus has taken Dmitry's pull request 
> and I have merged from Linus.
> 
> So once you have fixed for the minor issues raised by Henrik and once you 
> have finished your testing, please send the additional fixes to me and I 
> will be merging the branch upstream.

Hi Benjamin,

we are now slowly aproaching the point in time in which I would like to 
prepare second round of HID tree merge with Linus.

Have you finished your testing / fixing the minor issues raised by Henrik 
please?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Benjamin Tissoires]

On Mon, Jan 10, 2011 at 7:32 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Sat, 8 Jan 2011, Jiri Kosina wrote:
>
>> > > I have now applied the patchset as-is to the 'multitouch' branch in my
>> > > tree.
>> >
>> > Which means -- could you please send any followup patches on top of that
>> > branch (i.e. on top of the v3 patchset)? It would help my workflow and
>> > make the merging process smoother.
>>
>> 'multitouch' brach now builds, as Linus has taken Dmitry's pull request
>> and I have merged from Linus.
>>
>> So once you have fixed for the minor issues raised by Henrik and once you
>> have finished your testing, please send the additional fixes to me and I
>> will be merging the branch upstream.
>
> Hi Benjamin,
>
> we are now slowly aproaching the point in time in which I would like to
> prepare second round of HID tree merge with Linus.
>
> Have you finished your testing / fixing the minor issues raised by Henrik
> please?
>
> Thanks,
>

Hi Jiri,

I'm still waiting for one tester (the generaltouch one).

I hope he will be able to test it tomorrow (he told me he was busy until today).

I'm sure the driver works for two of the three devices (we have them
at the lab). The generaltouch is less important as there is a conflict
in the naming of the vendorID/deviceID:
GeneralTouch sells two different devices with the same deviceID, and
the older is handled by usbtouchscreen.
Stéphane explained to me today that people that want to use multitouch
features will have to do some manipulations to enable it.

I'll send the diff between v3-v4 tomorrow if you want or before if you
really need.

Cheers,
Benjamin
--
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: [RFC v3 0/5] hid-multitouch: a first step towards multitouch unification

[Jiri Kosina]

On Mon, 10 Jan 2011, Benjamin Tissoires wrote:

> > we are now slowly aproaching the point in time in which I would like 
> > to prepare second round of HID tree merge with Linus.
> >
> > Have you finished your testing / fixing the minor issues raised by Henrik
> > please?
> >
> > Thanks,
> >
> 
> Hi Jiri,
> 
> I'm still waiting for one tester (the generaltouch one).
> 
> I hope he will be able to test it tomorrow (he told me he was busy until 
> today).
>
> 
> I'm sure the driver works for two of the three devices (we have them
> at the lab). The generaltouch is less important as there is a conflict
> in the naming of the vendorID/deviceID:
> GeneralTouch sells two different devices with the same deviceID, and
> the older is handled by usbtouchscreen.
> Stéphane explained to me today that people that want to use multitouch
> features will have to do some manipulations to enable it.
>
> I'll send the diff between v3-v4 tomorrow if you want or before if you
> really need.

Good, thanks for the update.

We still have the whole development cycle to fix potential problems in the 
driver found later by the testers, but the skeleton should rather really 
be there in a few days from now, otherwise we'll have to postpone it for 
.39.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
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