[PATCH v2 00/11] Win 8 support for digitizers

[PATCH v2 00/11] Win 8 support for digitizers

[Benjamin Tissoires]

Hi guys,

Ok, now, it should work with Jiri's tree.

v1 introduction:
So, this is an update for supporting Win 8 multitouch devices in the kernel.
As I wanted to reliably forward the resolution, I noticed a bug in the
processing of the unit_exponent in the hid core layer.
Thus the fixes for hid-core and hid-input.

v2 changes:
* added missing initial patch that prevents the series to be applied on top of Jiri's tree
* update to include latest hid changes
* taken into account Alan's patch: "hid: put the case in the right switch statement"

Sorry for the noise...

Cheers,
Benjamin

---

Benjamin Tissoires (11):
  HID: hid-input: export hidinput_calc_abs_res
  HID: core: fix unit exponent parsing
  HID: hid-input: add usage_index argument in input_mapping and event.
  HID: hid-multitouch: support arrays for the split of the touches in a
    report
  HID: hid-multitouch: get maxcontacts also from logical_max value
  HID: hid-multitouch: support T and C for win8 devices
  HID: hid-multitouch: move ALWAYS_VALID quirk check
  HID: hid-multitouch: fix Win 8 protocol
  HID: hid-multitouch: support for hovering devices
  HID: introduce Scan Time
  HID: hid-multitouch: get rid of usbhid depedency for general path

 Documentation/input/event-codes.txt |   7 ++
 drivers/hid/hid-a4tech.c            |   2 +-
 drivers/hid/hid-apple.c             |   4 +-
 drivers/hid/hid-belkin.c            |   2 +-
 drivers/hid/hid-cherry.c            |   2 +-
 drivers/hid/hid-chicony.c           |   2 +-
 drivers/hid/hid-core.c              |  26 +++--
 drivers/hid/hid-cypress.c           |   2 +-
 drivers/hid/hid-ezkey.c             |   4 +-
 drivers/hid/hid-gyration.c          |   4 +-
 drivers/hid/hid-input.c             |  44 ++++++---
 drivers/hid/hid-kensington.c        |   2 +-
 drivers/hid/hid-lcpower.c           |   2 +-
 drivers/hid/hid-lenovo-tpkbd.c      |   3 +-
 drivers/hid/hid-lg.c                |   4 +-
 drivers/hid/hid-magicmouse.c        |   3 +-
 drivers/hid/hid-microsoft.c         |   4 +-
 drivers/hid/hid-monterey.c          |   2 +-
 drivers/hid/hid-multitouch.c        | 191 ++++++++++++++++++++++++++----------
 drivers/hid/hid-ntrig.c             |   4 +-
 drivers/hid/hid-petalynx.c          |   2 +-
 drivers/hid/hid-prodikeys.c         |   2 +-
 drivers/hid/hid-ps3remote.c         |   3 +-
 drivers/hid/hid-samsung.c           |   2 +-
 drivers/hid/hid-speedlink.c         |   6 +-
 drivers/hid/hid-sunplus.c           |   2 +-
 drivers/hid/hid-tivo.c              |   2 +-
 drivers/hid/hid-topseed.c           |   2 +-
 drivers/hid/hid-twinhan.c           |   2 +-
 drivers/hid/hid-zydacron.c          |   2 +-
 include/linux/hid.h                 |   8 +-
 include/linux/input.h               |   1 +
 32 files changed, 244 insertions(+), 104 deletions(-)

-- 
1.7.11.7

[PATCH v2 01/11] HID: hid-input: export hidinput_calc_abs_res

[Benjamin Tissoires]

Exporting this function allows us to calculate the resolution in third
party drivers like hid-multitouch.
This patch also complete the function with additional valid axes.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-input.c      | 11 +++++++++--
 drivers/hid/hid-multitouch.c |  1 +
 include/linux/hid.h          |  1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d917c0d..1b0adc3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -208,7 +208,7 @@ static int hidinput_setkeycode(struct input_dev *dev,
  * Only exponent 1 length units are processed. Centimeters and inches are
  * converted to millimeters. Degrees are converted to radians.
  */
-static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
+__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 {
 	__s32 unit_exponent = field->unit_exponent;
 	__s32 logical_extents = field->logical_maximum -
@@ -229,6 +229,12 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	case ABS_X:
 	case ABS_Y:
 	case ABS_Z:
+	case ABS_MT_POSITION_X:
+	case ABS_MT_POSITION_Y:
+	case ABS_MT_TOOL_X:
+	case ABS_MT_TOOL_Y:
+	case ABS_MT_TOUCH_MAJOR:
+	case ABS_MT_TOUCH_MINOR:
 		if (field->unit == 0x11) {		/* If centimeters */
 			/* Convert to millimeters */
 			unit_exponent += 1;
@@ -281,8 +287,9 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	}
 
 	/* Calculate resolution */
-	return logical_extents / physical_extents;
+	return DIV_ROUND_CLOSEST(logical_extents, physical_extents);
 }
+EXPORT_SYMBOL_GPL(hidinput_calc_abs_res);
 
 #ifdef CONFIG_HID_BATTERY_STRENGTH
 static enum power_supply_property hidinput_battery_props[] = {
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c97011c..375a38d 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -298,6 +298,7 @@ static void set_abs(struct input_dev *input, unsigned int code,
 	int fmax = field->logical_maximum;
 	int fuzz = snratio ? (fmax - fmin) / snratio : 0;
 	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
+	input_abs_set_res(input, code, hidinput_calc_abs_res(field, code));
 }
 
 static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7e1f37d..9edb06c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -743,6 +743,7 @@ int hid_input_report(struct hid_device *, int type, u8 *, int, int);
 int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
 struct hid_field *hidinput_get_led_field(struct hid_device *hid);
 unsigned int hidinput_count_leds(struct hid_device *hid);
+__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
 void hid_output_report(struct hid_report *report, __u8 *data);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
-- 
1.7.11.7

[PATCH v2 02/11] HID: core: fix unit exponent parsing

[Benjamin Tissoires]

HID spec details special values for the HID field unit exponent.
Basically, the range [0x8..0xf] correspond to [-8..-1].

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-core.c  | 10 +++++++++-
 drivers/hid/hid-input.c | 19 +++++++++++++------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9904776..6bde6e4 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -315,6 +315,7 @@ static s32 item_sdata(struct hid_item *item)
 
 static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 {
+	__u32 raw_value;
 	switch (item->tag) {
 	case HID_GLOBAL_ITEM_TAG_PUSH:
 
@@ -365,7 +366,14 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
-		parser->global.unit_exponent = item_sdata(item);
+		/* Units exponent negative numbers are given through a special
+		 * table.
+		 * See "6.2.2.7 Global Items" for more information. */
+		raw_value = item_udata(item);
+		if ((raw_value >> 3) == 1)
+			parser->global.unit_exponent = raw_value - 16;
+		else
+			parser->global.unit_exponent = raw_value;
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT:
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 1b0adc3..fc9f2b5 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -215,7 +215,7 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 					field->logical_minimum;
 	__s32 physical_extents = field->physical_maximum -
 					field->physical_minimum;
-	__s32 prev;
+	__s32 prev, tmp_exponent;
 
 	/* Check if the extents are sane */
 	if (logical_extents <= 0 || physical_extents <= 0)
@@ -235,17 +235,24 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	case ABS_MT_TOOL_Y:
 	case ABS_MT_TOUCH_MAJOR:
 	case ABS_MT_TOUCH_MINOR:
-		if (field->unit == 0x11) {		/* If centimeters */
+		if (field->unit & 0xffffff00)		/* Not a length */
+			return 0;
+		tmp_exponent = field->unit >> 4;
+		if ((tmp_exponent >> 3) == 1)
+			tmp_exponent -= 16;
+		switch (field->unit & 0xf) {
+		case 0x1:				/* If centimeters */
 			/* Convert to millimeters */
-			unit_exponent += 1;
-		} else if (field->unit == 0x13) {	/* If inches */
+			unit_exponent += tmp_exponent;
+			break;
+		case 0x3:				/* If inches */
 			/* Convert to millimeters */
 			prev = physical_extents;
 			physical_extents *= 254;
 			if (physical_extents < prev)
 				return 0;
-			unit_exponent -= 1;
-		} else {
+			unit_exponent += tmp_exponent - 2;
+		default:
 			return 0;
 		}
 		break;
-- 
1.7.11.7

[PATCH v2 03/11] HID: hid-input: add usage_index argument in input_mapping and event.

[Benjamin Tissoires]

Currently, there is no way to know the index of the current field
in the .input_mapping and .event callbacks  when this field is inside
an array of HID fields.
This patch forwards this index to the input_mapping and event
callbacks.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-a4tech.c       |  2 +-
 drivers/hid/hid-apple.c        |  4 ++--
 drivers/hid/hid-belkin.c       |  2 +-
 drivers/hid/hid-cherry.c       |  2 +-
 drivers/hid/hid-chicony.c      |  2 +-
 drivers/hid/hid-core.c         | 16 +++++++++++-----
 drivers/hid/hid-cypress.c      |  2 +-
 drivers/hid/hid-ezkey.c        |  4 ++--
 drivers/hid/hid-gyration.c     |  4 ++--
 drivers/hid/hid-input.c        | 10 ++++++----
 drivers/hid/hid-kensington.c   |  2 +-
 drivers/hid/hid-lcpower.c      |  2 +-
 drivers/hid/hid-lenovo-tpkbd.c |  3 ++-
 drivers/hid/hid-lg.c           |  4 ++--
 drivers/hid/hid-magicmouse.c   |  3 ++-
 drivers/hid/hid-microsoft.c    |  4 ++--
 drivers/hid/hid-monterey.c     |  2 +-
 drivers/hid/hid-multitouch.c   |  4 ++--
 drivers/hid/hid-ntrig.c        |  4 +++-
 drivers/hid/hid-petalynx.c     |  2 +-
 drivers/hid/hid-prodikeys.c    |  2 +-
 drivers/hid/hid-ps3remote.c    |  3 ++-
 drivers/hid/hid-samsung.c      |  2 +-
 drivers/hid/hid-speedlink.c    |  6 +++---
 drivers/hid/hid-sunplus.c      |  2 +-
 drivers/hid/hid-tivo.c         |  2 +-
 drivers/hid/hid-topseed.c      |  2 +-
 drivers/hid/hid-twinhan.c      |  2 +-
 drivers/hid/hid-zydacron.c     |  2 +-
 include/linux/hid.h            |  6 ++++--
 30 files changed, 61 insertions(+), 46 deletions(-)

diff --git a/drivers/hid/hid-a4tech.c b/drivers/hid/hid-a4tech.c
index 0a23988..62e2aa5 100644
--- a/drivers/hid/hid-a4tech.c
+++ b/drivers/hid/hid-a4tech.c
@@ -48,7 +48,7 @@ static int a4_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 }
 
 static int a4_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	struct a4tech_sc *a4 = hid_get_drvdata(hdev);
 	struct input_dev *input;
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 06ebdbb..01e8592 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -247,7 +247,7 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
 }
 
 static int apple_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	struct apple_sc *asc = hid_get_drvdata(hdev);
 
@@ -310,7 +310,7 @@ static void apple_setup_input(struct input_dev *input)
 
 static int apple_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if (usage->hid == (HID_UP_CUSTOM | 0x0003)) {
 		/* The fn key on Apple USB keyboards */
diff --git a/drivers/hid/hid-belkin.c b/drivers/hid/hid-belkin.c
index a1a5a12..6999a64 100644
--- a/drivers/hid/hid-belkin.c
+++ b/drivers/hid/hid-belkin.c
@@ -28,7 +28,7 @@
 					EV_KEY, (c))
 static int belkin_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
 
diff --git a/drivers/hid/hid-cherry.c b/drivers/hid/hid-cherry.c
index af034d3..1feec49 100644
--- a/drivers/hid/hid-cherry.c
+++ b/drivers/hid/hid-cherry.c
@@ -40,7 +40,7 @@ static __u8 *ch_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 					EV_KEY, (c))
 static int ch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER)
 		return 0;
diff --git a/drivers/hid/hid-chicony.c b/drivers/hid/hid-chicony.c
index a2abb8e..4510ea5 100644
--- a/drivers/hid/hid-chicony.c
+++ b/drivers/hid/hid-chicony.c
@@ -27,7 +27,7 @@
 					EV_KEY, (c))
 static int ch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_MSVENDOR)
 		return 0;
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 6bde6e4..f1720a0 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1009,7 +1009,8 @@ static int hid_match_usage(struct hid_device *hid, struct hid_usage *usage)
 }
 
 static void hid_process_event(struct hid_device *hid, struct hid_field *field,
-		struct hid_usage *usage, __s32 value, int interrupt)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value,
+		int interrupt)
 {
 	struct hid_driver *hdrv = hid->driver;
 	int ret;
@@ -1018,7 +1019,7 @@ static void hid_process_event(struct hid_device *hid, struct hid_field *field,
 		hid_dump_input(hid, usage, value);
 
 	if (hdrv && hdrv->event && hid_match_usage(hid, usage)) {
-		ret = hdrv->event(hid, field, usage, value);
+		ret = hdrv->event(hid, field, usage, usage_index, value);
 		if (ret != 0) {
 			if (ret < 0)
 				hid_err(hid, "%s's event failed with %d\n",
@@ -1071,19 +1072,24 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
 	for (n = 0; n < count; n++) {
 
 		if (HID_MAIN_ITEM_VARIABLE & field->flags) {
-			hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
+			hid_process_event(hid, field, &field->usage[n], n,
+				value[n], interrupt);
 			continue;
 		}
 
 		if (field->value[n] >= min && field->value[n] <= max
 			&& field->usage[field->value[n] - min].hid
 			&& search(value, field->value[n], count))
-				hid_process_event(hid, field, &field->usage[field->value[n] - min], 0, interrupt);
+				hid_process_event(hid, field,
+					&field->usage[field->value[n] - min], n,
+					0, interrupt);
 
 		if (value[n] >= min && value[n] <= max
 			&& field->usage[value[n] - min].hid
 			&& search(field->value, value[n], count))
-				hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
+				hid_process_event(hid, field,
+					&field->usage[value[n] - min], n,
+					1, interrupt);
 	}
 
 	memcpy(field->value, value, count * sizeof(__s32));
diff --git a/drivers/hid/hid-cypress.c b/drivers/hid/hid-cypress.c
index 3e159a5..453548a 100644
--- a/drivers/hid/hid-cypress.c
+++ b/drivers/hid/hid-cypress.c
@@ -70,7 +70,7 @@ static int cp_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 }
 
 static int cp_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
 
diff --git a/drivers/hid/hid-ezkey.c b/drivers/hid/hid-ezkey.c
index 6540af2..3bd9675 100644
--- a/drivers/hid/hid-ezkey.c
+++ b/drivers/hid/hid-ezkey.c
@@ -27,7 +27,7 @@
 
 static int ez_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER)
 		return 0;
@@ -48,7 +48,7 @@ static int ez_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 }
 
 static int ez_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput ||
 			!usage->type)
diff --git a/drivers/hid/hid-gyration.c b/drivers/hid/hid-gyration.c
index 4442c30..255e5dd 100644
--- a/drivers/hid/hid-gyration.c
+++ b/drivers/hid/hid-gyration.c
@@ -26,7 +26,7 @@
 					EV_KEY, (c))
 static int gyration_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_LOGIVENDOR)
 		return 0;
@@ -55,7 +55,7 @@ static int gyration_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 }
 
 static int gyration_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 
 	if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index fc9f2b5..16cc89a 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -464,7 +464,8 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
 #endif	/* CONFIG_HID_BATTERY_STRENGTH */
 
 static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_field *field,
-				     struct hid_usage *usage)
+				     struct hid_usage *usage,
+				     unsigned int usage_index)
 {
 	struct input_dev *input = hidinput->input;
 	struct hid_device *device = input_get_drvdata(input);
@@ -484,7 +485,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 	if (device->driver->input_mapping) {
 		int ret = device->driver->input_mapping(device, hidinput, field,
-				usage, &bit, &max);
+				usage, usage_index, &bit, &max);
 		if (ret > 0)
 			goto mapped;
 		if (ret < 0)
@@ -1233,8 +1234,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 
 			for (i = 0; i < report->maxfield; i++)
 				for (j = 0; j < report->field[i]->maxusage; j++)
-					hidinput_configure_usage(hidinput, report->field[i],
-								 report->field[i]->usage + j);
+					hidinput_configure_usage(hidinput,
+						report->field[i],
+						report->field[i]->usage + j, j);
 
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT) {
 				/* This will leave hidinput NULL, so that it
diff --git a/drivers/hid/hid-kensington.c b/drivers/hid/hid-kensington.c
index a5b4016..af9b9da 100644
--- a/drivers/hid/hid-kensington.c
+++ b/drivers/hid/hid-kensington.c
@@ -22,7 +22,7 @@
 
 static int ks_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_MSVENDOR)
 		return 0;
diff --git a/drivers/hid/hid-lcpower.c b/drivers/hid/hid-lcpower.c
index 22bc14a..5cfbef8 100644
--- a/drivers/hid/hid-lcpower.c
+++ b/drivers/hid/hid-lcpower.c
@@ -22,7 +22,7 @@
 					EV_KEY, (c))
 static int ts_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_LOGIVENDOR)
 		return 0;
diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
index cea016e..7c12fd4 100644
--- a/drivers/hid/hid-lenovo-tpkbd.c
+++ b/drivers/hid/hid-lenovo-tpkbd.c
@@ -39,7 +39,8 @@ struct tpkbd_data_pointer {
 
 static int tpkbd_input_mapping(struct hid_device *hdev,
 		struct hid_input *hi, struct hid_field *field,
-		struct hid_usage *usage, unsigned long **bit, int *max)
+		struct hid_usage *usage, unsigned int usage_index,
+		unsigned long **bit, int *max)
 {
 	struct usbhid_device *uhdev;
 
diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index a2f8e88..0c5acc6 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -263,7 +263,7 @@ static int lg_wireless_mapping(struct hid_input *hi, struct hid_usage *usage,
 
 static int lg_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	/* extended mapping for certain Logitech hardware (Logitech cordless
 	   desktop LX500) */
@@ -332,7 +332,7 @@ static int lg_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 }
 
 static int lg_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
 
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 25ddf3e..aca56ee 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -447,7 +447,8 @@ static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hd
 
 static int magicmouse_input_mapping(struct hid_device *hdev,
 		struct hid_input *hi, struct hid_field *field,
-		struct hid_usage *usage, unsigned long **bit, int *max)
+		struct hid_usage *usage, unsigned int usage_index,
+		unsigned long **bit, int *max)
 {
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
 
diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index 3acdcfc..2dfe9fa 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -89,7 +89,7 @@ static int ms_presenter_8k_quirk(struct hid_input *hi, struct hid_usage *usage,
 
 static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
 
@@ -122,7 +122,7 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 }
 
 static int ms_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
 
diff --git a/drivers/hid/hid-monterey.c b/drivers/hid/hid-monterey.c
index cd3643e..cbcbcc7 100644
--- a/drivers/hid/hid-monterey.c
+++ b/drivers/hid/hid-monterey.c
@@ -35,7 +35,7 @@ static __u8 *mr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 					EV_KEY, (c))
 static int mr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER)
 		return 0;
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 375a38d..725d155 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -314,7 +314,7 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 
 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)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
 	struct mt_class *cls = &td->mtclass;
@@ -520,7 +520,7 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 }
 
 static int mt_event(struct hid_device *hid, struct hid_field *field,
-				struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	struct mt_device *td = hid_get_drvdata(hid);
 	__s32 quirks = td->mtclass.quirks;
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 86a969f..14794b9 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -456,6 +456,7 @@ static struct attribute_group ntrig_attribute_group = {
 
 static int ntrig_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			       struct hid_field *field, struct hid_usage *usage,
+			       unsigned int usage_index,
 			       unsigned long **bit, int *max)
 {
 	struct ntrig_data *nd = hid_get_drvdata(hdev);
@@ -567,7 +568,8 @@ static int ntrig_input_mapped(struct hid_device *hdev, struct hid_input *hi,
  * and call input_mt_sync after each point if necessary
  */
 static int ntrig_event (struct hid_device *hid, struct hid_field *field,
-			struct hid_usage *usage, __s32 value)
+			struct hid_usage *usage, unsigned int usage_index,
+			__s32 value)
 {
 	struct ntrig_data *nd = hid_get_drvdata(hid);
 	struct input_dev *input;
diff --git a/drivers/hid/hid-petalynx.c b/drivers/hid/hid-petalynx.c
index 4c521de..590db7d 100644
--- a/drivers/hid/hid-petalynx.c
+++ b/drivers/hid/hid-petalynx.c
@@ -39,7 +39,7 @@ static __u8 *pl_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 					EV_KEY, (c))
 static int pl_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_LOGIVENDOR) {
 		switch (usage->hid & HID_USAGE) {
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index ec8ca33..b687f4e 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -757,7 +757,7 @@ static __u8 *pk_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static int pk_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	struct pk_device *pk = hid_get_drvdata(hdev);
 	struct pcmidi_snd *pm;
diff --git a/drivers/hid/hid-ps3remote.c b/drivers/hid/hid-ps3remote.c
index 03811e5..24d3196 100644
--- a/drivers/hid/hid-ps3remote.c
+++ b/drivers/hid/hid-ps3remote.c
@@ -151,7 +151,8 @@ static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static int ps3remote_mapping(struct hid_device *hdev, struct hid_input *hi,
 			     struct hid_field *field, struct hid_usage *usage,
-			     unsigned long **bit, int *max)
+			     unsigned int usage_index, unsigned long **bit,
+			     int *max)
 {
 	unsigned int key = usage->hid & HID_USAGE;
 
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index a5821d3..b88a27a 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -140,7 +140,7 @@ static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	struct hid_field *field, struct hid_usage *usage,
-	unsigned long **bit, int *max)
+	unsigned int usage_index, unsigned long **bit, int *max)
 {
 	int ret = 0;
 
diff --git a/drivers/hid/hid-speedlink.c b/drivers/hid/hid-speedlink.c
index 6020137..da50735 100644
--- a/drivers/hid/hid-speedlink.c
+++ b/drivers/hid/hid-speedlink.c
@@ -27,8 +27,8 @@ static const struct hid_device_id speedlink_devices[] = {
 };
 
 static int speedlink_input_mapping(struct hid_device *hdev,
-		struct hid_input *hi,
-		struct hid_field *field, struct hid_usage *usage,
+		struct hid_input *hi, struct hid_field *field,
+		struct hid_usage *usage, unsigned int usage_index,
 		unsigned long **bit, int *max)
 {
 	/*
@@ -45,7 +45,7 @@ static int speedlink_input_mapping(struct hid_device *hdev,
 }
 
 static int speedlink_event(struct hid_device *hdev, struct hid_field *field,
-		struct hid_usage *usage, __s32 value)
+		struct hid_usage *usage, unsigned int usage_index, __s32 value)
 {
 	/* No other conditions due to usage_table. */
 	/* Fix "jumpy" cursor (invalid events sent by device). */
diff --git a/drivers/hid/hid-sunplus.c b/drivers/hid/hid-sunplus.c
index 45b4b06..9d36bc0 100644
--- a/drivers/hid/hid-sunplus.c
+++ b/drivers/hid/hid-sunplus.c
@@ -37,7 +37,7 @@ static __u8 *sp_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		EV_KEY, (c))
 static int sp_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER)
 		return 0;
diff --git a/drivers/hid/hid-tivo.c b/drivers/hid/hid-tivo.c
index 9f85f82..e7684fd 100644
--- a/drivers/hid/hid-tivo.c
+++ b/drivers/hid/hid-tivo.c
@@ -24,7 +24,7 @@
 
 static int tivo_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	switch (usage->hid & HID_USAGE_PAGE) {
 	case HID_UP_TIVOVENDOR:
diff --git a/drivers/hid/hid-topseed.c b/drivers/hid/hid-topseed.c
index 613ff7b..e924b7d 100644
--- a/drivers/hid/hid-topseed.c
+++ b/drivers/hid/hid-topseed.c
@@ -28,7 +28,7 @@
 					EV_KEY, (c))
 static int ts_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_LOGIVENDOR)
 		return 0;
diff --git a/drivers/hid/hid-twinhan.c b/drivers/hid/hid-twinhan.c
index f23456b..b38e6b3 100644
--- a/drivers/hid/hid-twinhan.c
+++ b/drivers/hid/hid-twinhan.c
@@ -62,7 +62,7 @@
 					EV_KEY, (c))
 static int twinhan_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
+		unsigned int usage_index, unsigned long **bit, int *max)
 {
 	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_KEYBOARD)
 		return 0;
diff --git a/drivers/hid/hid-zydacron.c b/drivers/hid/hid-zydacron.c
index 1ad85f2..af0e171 100644
--- a/drivers/hid/hid-zydacron.c
+++ b/drivers/hid/hid-zydacron.c
@@ -47,7 +47,7 @@ static __u8 *zc_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static int zc_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	struct hid_field *field, struct hid_usage *usage,
-	unsigned long **bit, int *max)
+	unsigned int usage_index, unsigned long **bit, int *max)
 {
 	int i;
 	struct zc_device *zc = hid_get_drvdata(hdev);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9edb06c..6216529 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -660,14 +660,16 @@ struct hid_driver {
 			u8 *data, int size);
 	const struct hid_usage_id *usage_table;
 	int (*event)(struct hid_device *hdev, struct hid_field *field,
-			struct hid_usage *usage, __s32 value);
+			struct hid_usage *usage, unsigned int usage_index,
+			__s32 value);
 
 	__u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf,
 			unsigned int *size);
 
 	int (*input_mapping)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
-			struct hid_usage *usage, unsigned long **bit, int *max);
+			struct hid_usage *usage, unsigned int usage_index,
+			unsigned long **bit, int *max);
 	int (*input_mapped)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
 			struct hid_usage *usage, unsigned long **bit, int *max);
-- 
1.7.11.7

[PATCH v2 04/11] HID: hid-multitouch: support arrays for the split of the touches in a report

[Benjamin Tissoires]

Win8 certification introduced the ability to transmit two X and two Y per
touch. The specification precises that it must be in an array, with a
report count == 2.

This test guarantees that we split the touches on the last element
in this array.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 725d155..95562d8 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -577,12 +577,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 			return 0;
 		}
 
-		if (usage->hid == td->last_slot_field)
-			mt_complete_slot(td, field->hidinput->input);
-
-		if (field->index == td->last_field_index
-			&& td->num_received >= td->num_expected)
-			mt_sync_frame(td, field->hidinput->input);
+		if (usage_index + 1 == field->report_count) {
+			/* we only take into account the last report
+			 * of a field if report_count > 1 */
+			if (usage->hid == td->last_slot_field)
+				mt_complete_slot(td, field->hidinput->input);
+
+			if (field->index == td->last_field_index
+				&& td->num_received >= td->num_expected)
+				mt_sync_frame(td, field->hidinput->input);
+		}
 
 	}
 
-- 
1.7.11.7

[PATCH v2 05/11] HID: hid-multitouch: get maxcontacts also from logical_max value

[Benjamin Tissoires]

Win8 devices are required to present the feature "Maximum Contact Number".
Fortunately all win7 devices I've seen presents this feature.
If the current value is 0, then, the driver can get the actual supported
contact count by refering to the logical_max.
This win8 specification ensures that logical_max may not be above 250.
This also allows us to detect when devices like irtouch or stantum reports
an obviously wrong value of 255.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 95562d8..41f2981 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -121,6 +121,7 @@ struct mt_device {
 #define MT_CLS_GENERALTOUCH_PWT_TENFINGERS	0x0109
 
 #define MT_DEFAULT_MAXCONTACT	10
+#define MT_MAX_MAXCONTACT	250
 
 #define MT_USB_DEVICE(v, p)	HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH, v, p)
 #define MT_BT_DEVICE(v, p)	HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_MULTITOUCH, v, p)
@@ -283,6 +284,9 @@ static void mt_feature_mapping(struct hid_device *hdev,
 	case HID_DG_CONTACTMAX:
 		td->maxcontact_report_id = field->report->id;
 		td->maxcontacts = field->value[0];
+		if (!td->maxcontacts &&
+		    field->logical_maximum <= MT_MAX_MAXCONTACT)
+			td->maxcontacts = field->logical_maximum;
 		if (td->mtclass.maxcontacts)
 			/* check if the maxcontacts is given by the class */
 			td->maxcontacts = td->mtclass.maxcontacts;
-- 
1.7.11.7

[PATCH v2 06/11] HID: hid-multitouch: support T and C for win8 devices

[Benjamin Tissoires]

Win8 input specification clarifies the X and Y sent by devices.
It distincts the position where the user wants to Touch (T) from
the center of the ellipsoide (C). This patch enable supports for this
distinction in hid-multitouch.

We recognize Win8 certified devices from their vendor feature 0xff0000c5
where Microsoft put a signed blob in the report to check if the device
passed the certification.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 53 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 41f2981..000c979 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -52,9 +52,10 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_VALID_IS_CONFIDENCE	(1 << 6)
 #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE	(1 << 8)
 #define MT_QUIRK_NO_AREA		(1 << 9)
+#define MT_QUIRK_WIN_8_CERTIFIED	(1 << 10)
 
 struct mt_slot {
-	__s32 x, y, p, w, h;
+	__s32 x, y, cx, cy, p, w, h;
 	__s32 contactid;	/* the device ContactID assigned to this slot */
 	bool touch_state;	/* is the touch valid? */
 };
@@ -292,6 +293,10 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			td->maxcontacts = td->mtclass.maxcontacts;
 
 		break;
+	case 0xff0000c5:
+		if (field->report_count == 256 && field->report_size == 8)
+			td->mtclass.quirks |= MT_QUIRK_WIN_8_CERTIFIED;
+		break;
 	}
 }
 
@@ -350,18 +355,36 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_GENDESK:
 		switch (usage->hid) {
 		case HID_GD_X:
-			hid_map_usage(hi, usage, bit, max,
+			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED &&
+				usage_index) {
+				hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_TOOL_X);
+				set_abs(hi->input, ABS_MT_TOOL_X, field,
+					cls->sn_move);
+			} else {
+				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);
+				set_abs(hi->input, ABS_MT_POSITION_X, field,
+					cls->sn_move);
+			}
+
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_GD_Y:
-			hid_map_usage(hi, usage, bit, max,
+			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED &&
+				usage_index) {
+				hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_TOOL_Y);
+				set_abs(hi->input, ABS_MT_TOOL_Y, field,
+					cls->sn_move);
+			} else {
+				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);
+				set_abs(hi->input, ABS_MT_POSITION_Y, field,
+					cls->sn_move);
+			}
+
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
@@ -502,6 +525,12 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 
 			input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
 			input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
+			if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED) {
+				input_event(input, EV_ABS, ABS_MT_TOOL_X,
+					s->cx);
+				input_event(input, EV_ABS, ABS_MT_TOOL_Y,
+					s->cy);
+			}
 			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
 			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
 			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
@@ -553,10 +582,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 			td->curdata.p = value;
 			break;
 		case HID_GD_X:
-			td->curdata.x = value;
+			if (usage->code == ABS_MT_POSITION_X)
+				td->curdata.x = value;
+			else
+				td->curdata.cx = value;
 			break;
 		case HID_GD_Y:
-			td->curdata.y = value;
+			if (usage->code == ABS_MT_POSITION_Y)
+				td->curdata.y = value;
+			else
+				td->curdata.cy = value;
 			break;
 		case HID_DG_WIDTH:
 			td->curdata.w = value;
-- 
1.7.11.7

[PATCH v2 07/11] HID: hid-multitouch: move ALWAYS_VALID quirk check

[Benjamin Tissoires]

Win 8 device specification changed the requirements for the hid usages
of the multitouch devices. Now InRange is optional and must be only
used when the device supports hovering.

This ensures that the quirk ALWAYS_VALID is taken into account and
also ensures its precedence over the other VALID* quirks.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 000c979..43bd8e8 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -506,7 +506,7 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
  */
 static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 {
-	if (td->curvalid) {
+	if (td->curvalid || td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID) {
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
 
@@ -561,9 +561,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	if (hid->claimed & HID_CLAIMED_INPUT) {
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-			if (quirks & MT_QUIRK_ALWAYS_VALID)
-				td->curvalid = true;
-			else if (quirks & MT_QUIRK_VALID_IS_INRANGE)
+			if (quirks & MT_QUIRK_VALID_IS_INRANGE)
 				td->curvalid = value;
 			break;
 		case HID_DG_TIPSWITCH:
-- 
1.7.11.7

[PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

Win 8 specification is much more precise than the Win 7 one.
Moreover devices that need to take certification must be submitted
to Microsoft.

The result is a better protocol support and we can rely on that to
skip all the messy tests we used to do.

The protocol specify the fact that each valid touch must be reported
within a frame, and that the release touch coordinate must be the
same than the last known touch.
So we can use the always_valid quirk and dismiss reports when we
touch coordiante do not follow this rule.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 43bd8e8..bd23f19 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -510,6 +510,18 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
 
+		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
+		    !s->touch_state) {
+			struct input_mt_slot *slot = &input->mt->slots[slotnum];
+			int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
+			int prv_y = input_mt_get_value(slot, ABS_MT_POSITION_Y);
+			/* valid releases occurs when the last x and y positions
+			 * are the same as the last known touch. */
+			if (!input_mt_is_active(slot) ||
+			    prv_x != s->x || prv_y != s->y)
+				return;
+		}
+
 		if (slotnum < 0 || slotnum >= td->maxcontacts)
 			return;
 
@@ -681,8 +693,8 @@ static void mt_post_parse_default_settings(struct mt_device *td)
 {
 	__s32 quirks = td->mtclass.quirks;
 
-	/* unknown serial device needs special quirks */
-	if (td->touches_by_report == 1) {
+	/* unknown serial devices or win8 ones need special quirks */
+	if (td->touches_by_report == 1 || quirks & MT_QUIRK_WIN_8_CERTIFIED) {
 		quirks |= MT_QUIRK_ALWAYS_VALID;
 		quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
 		quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
-- 
1.7.11.7

[PATCH v2 09/11] HID: hid-multitouch: support for hovering devices

[Benjamin Tissoires]

Win8 devices supporting hovering must provides InRange HID field.
The information that the finger is here but is not touching the surface
is sent to the user space through ABS_MT_DISTANCE as required by the
multitouch protocol.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index bd23f19..c0ab1c6 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -55,7 +55,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_WIN_8_CERTIFIED	(1 << 10)
 
 struct mt_slot {
-	__s32 x, y, cx, cy, p, w, h;
+	__s32 x, y, cx, cy, z, p, w, h;
 	__s32 contactid;	/* the device ContactID assigned to this slot */
 	bool touch_state;	/* is the touch valid? */
 };
@@ -394,6 +394,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
+			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED) {
+				hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_DISTANCE);
+				input_set_abs_params(hi->input,
+					ABS_MT_DISTANCE, 0, 1, 0, 0);
+			}
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
@@ -511,6 +517,11 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 		struct mt_slot *s = &td->curdata;
 
 		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
+		    !test_bit(ABS_MT_DISTANCE, input->absbit))
+			/* If InRange is not present, rely on TipSwitch */
+			s->touch_state = !s->z;
+
+		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
 		    !s->touch_state) {
 			struct input_mt_slot *slot = &input->mt->slots[slotnum];
 			int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
@@ -543,6 +554,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 				input_event(input, EV_ABS, ABS_MT_TOOL_Y,
 					s->cy);
 			}
+			input_event(input, EV_ABS, ABS_MT_DISTANCE, s->z);
 			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
 			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
 			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
@@ -575,11 +587,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 		case HID_DG_INRANGE:
 			if (quirks & MT_QUIRK_VALID_IS_INRANGE)
 				td->curvalid = value;
+			if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
+				td->curdata.touch_state = value;
 			break;
 		case HID_DG_TIPSWITCH:
 			if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
 				td->curvalid = value;
-			td->curdata.touch_state = value;
+			if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
+				td->curdata.z = !value;
+			else
+				td->curdata.touch_state = value;
 			break;
 		case HID_DG_CONFIDENCE:
 			if (quirks & MT_QUIRK_VALID_IS_CONFIDENCE)
-- 
1.7.11.7

[PATCH v2 10/11] HID: introduce Scan Time

[Benjamin Tissoires]

Win 8 digitizer devices provides the actual scan time computed by the
hardware itself. The value is global to the frame and is not specific
to the multitouch protocol (though only touch, not pen, should use it
according to the specification).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 Documentation/input/event-codes.txt |  7 +++++++
 drivers/hid/hid-input.c             |  4 ++++
 drivers/hid/hid-multitouch.c        | 11 +++++++++--
 include/linux/hid.h                 |  1 +
 include/linux/input.h               |  1 +
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
index 53305bd..8f8c908 100644
--- a/Documentation/input/event-codes.txt
+++ b/Documentation/input/event-codes.txt
@@ -174,6 +174,13 @@ A few EV_ABS codes have special meanings:
     the input device may be used freely in three dimensions, consider ABS_Z
     instead.
 
+* ABS_SCAN_TIME:
+  - Used when the device provides a timestamp for each frame. The unit must be
+    100us, and may be reset when the device don't send any events for a
+    period of time. The values increment at each frame and thus, it can roll
+    back to 0 when reach logical_max. If the device does not provide this
+    information, the driver must not provide it to the user space.
+
 * ABS_MT_<name>:
   - Used to describe multitouch input events. Please see
     multi-touch-protocol.txt for details.
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 16cc89a..5fe7bd3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -675,6 +675,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_key_clear(BTN_STYLUS2);
 			break;
 
+		case 0x56: /* Scan Time */
+			map_abs(ABS_SCAN_TIME);
+			break;
+
 		default:  goto unknown;
 		}
 		break;
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c0ab1c6..21a120b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -447,12 +447,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
+		case HID_DG_SCANTIME:
+			hid_map_usage(hi, usage, bit, max,
+				EV_ABS, ABS_SCAN_TIME);
+			set_abs(hi->input, ABS_SCAN_TIME, field, 0);
+			td->last_field_index = field->index;
+			return 1;
 		case HID_DG_CONTACTCOUNT:
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTMAX:
-			/* we don't set td->last_slot_field as contactcount and
-			 * contact max are global to the report */
+			/* we don't set td->last_slot_field as scan time,
+			 * contactcount and contact max are global to the
+			 * report */
 			td->last_field_index = field->index;
 			return -1;
 		case HID_DG_TOUCH:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 6216529..99a6418 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -279,6 +279,7 @@ struct hid_item {
 #define HID_DG_DEVICEINDEX	0x000d0053
 #define HID_DG_CONTACTCOUNT	0x000d0054
 #define HID_DG_CONTACTMAX	0x000d0055
+#define HID_DG_SCANTIME		0x000d0056
 
 /*
  * HID report types --- Ouch! HID spec says 1 2 3!
diff --git a/include/linux/input.h b/include/linux/input.h
index ba48743..73c3a96 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -796,6 +796,7 @@ struct input_keymap_entry {
 #define ABS_TILT_X		0x1a
 #define ABS_TILT_Y		0x1b
 #define ABS_TOOL_WIDTH		0x1c
+#define ABS_SCAN_TIME		0x1d
 
 #define ABS_VOLUME		0x20
 
-- 
1.7.11.7

[PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Benjamin Tissoires]

This patch factorizes the hid set_feature command by using
hid_device->hid_output_raw_report instead of direclty relying on
usbhid. This makes the driver usb independant.

However I still can't remove the 2 usb related headers because the
function mt_resume has a specific patch for usb devices.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 21a120b..33038c5 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -670,47 +670,58 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	return 1;
 }
 
-static void mt_set_input_mode(struct hid_device *hdev)
+static void mt_set_feature(struct hid_device *hdev, __s8 feature_id,
+	u8 value, size_t index)
 {
-	struct mt_device *td = hid_get_drvdata(hdev);
 	struct hid_report *r;
 	struct hid_report_enum *re;
+	u8 *data;
+	u8 max_value;
+	int len;
+
+	if (feature_id < 0 || !hdev->hid_output_raw_report)
+		return;
+
+	re = &hdev->report_enum[HID_FEATURE_REPORT];
+	r = re->report_id_hash[feature_id];
+	if (!r)
+		return;
+
+	len = ((r->size - 1) >> 3) + 1 + (r->id > 0);
+	max_value = r->field[0]->logical_maximum;
+	value = min(value, max_value);
 
-	if (td->inputmode < 0)
+	if (r->field[0]->value[index] == value || len < 2 || index + 1 >= len)
 		return;
 
-	re = &(hdev->report_enum[HID_FEATURE_REPORT]);
-	r = re->report_id_hash[td->inputmode];
-	if (r) {
-		r->field[0]->value[td->inputmode_index] = 0x02;
-		usbhid_submit_report(hdev, r, USB_DIR_OUT);
+	data = kzalloc(len, GFP_ATOMIC);
+	if (!data) {
+		hid_warn(hdev, "output queueing failed\n");
+		return;
 	}
+
+	data[0] = r->id;
+	data[index + 1] = value;
+	hdev->hid_output_raw_report(hdev, data, len, HID_FEATURE_REPORT);
+	kfree(data);
 }
 
-static void mt_set_maxcontacts(struct hid_device *hdev)
+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;
-	int fieldmax, max;
 
-	if (td->maxcontact_report_id < 0)
-		return;
+	mt_set_feature(hdev, td->inputmode, 0x02, td->inputmode_index);
+}
 
-	if (!td->mtclass.maxcontacts)
+static void mt_set_maxcontacts(struct hid_device *hdev)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	int max = td->mtclass.maxcontacts;
+
+	if (!max)
 		return;
 
-	re = &hdev->report_enum[HID_FEATURE_REPORT];
-	r = re->report_id_hash[td->maxcontact_report_id];
-	if (r) {
-		max = td->mtclass.maxcontacts;
-		fieldmax = r->field[0]->logical_maximum;
-		max = min(fieldmax, max);
-		if (r->field[0]->value[0] != max) {
-			r->field[0]->value[0] = max;
-			usbhid_submit_report(hdev, r, USB_DIR_OUT);
-		}
-	}
+	mt_set_feature(hdev, td->maxcontact_report_id, max, 0);
 }
 
 static void mt_post_parse_default_settings(struct mt_device *td)
-- 
1.7.11.7

Re: [PATCH v2 01/11] HID: hid-input: export hidinput_calc_abs_res

[Henrik Rydberg]

Hi Benjamin,

> Exporting this function allows us to calculate the resolution in third
> party drivers like hid-multitouch.
> This patch also complete the function with additional valid axes.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---

Nice, but please see comment below.

>  drivers/hid/hid-input.c      | 11 +++++++++--
>  drivers/hid/hid-multitouch.c |  1 +
>  include/linux/hid.h          |  1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d917c0d..1b0adc3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -208,7 +208,7 @@ static int hidinput_setkeycode(struct input_dev *dev,
>   * Only exponent 1 length units are processed. Centimeters and inches are
>   * converted to millimeters. Degrees are converted to radians.
>   */
> -static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
> +__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  {
>  	__s32 unit_exponent = field->unit_exponent;
>  	__s32 logical_extents = field->logical_maximum -
> @@ -229,6 +229,12 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  	case ABS_X:
>  	case ABS_Y:
>  	case ABS_Z:
> +	case ABS_MT_POSITION_X:
> +	case ABS_MT_POSITION_Y:
> +	case ABS_MT_TOOL_X:
> +	case ABS_MT_TOOL_Y:
> +	case ABS_MT_TOUCH_MAJOR:
> +	case ABS_MT_TOUCH_MINOR:
>  		if (field->unit == 0x11) {		/* If centimeters */
>  			/* Convert to millimeters */
>  			unit_exponent += 1;
> @@ -281,8 +287,9 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  	}
>  
>  	/* Calculate resolution */
> -	return logical_extents / physical_extents;
> +	return DIV_ROUND_CLOSEST(logical_extents, physical_extents);

This line might be best left alone; if the round-off matters, you now
risk regressing a carefully tuned userland.

>  }
> +EXPORT_SYMBOL_GPL(hidinput_calc_abs_res);
>  
>  #ifdef CONFIG_HID_BATTERY_STRENGTH
>  static enum power_supply_property hidinput_battery_props[] = {
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c97011c..375a38d 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -298,6 +298,7 @@ static void set_abs(struct input_dev *input, unsigned int code,
>  	int fmax = field->logical_maximum;
>  	int fuzz = snratio ? (fmax - fmin) / snratio : 0;
>  	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
> +	input_abs_set_res(input, code, hidinput_calc_abs_res(field, code));
>  }
>  
>  static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 7e1f37d..9edb06c 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -743,6 +743,7 @@ int hid_input_report(struct hid_device *, int type, u8 *, int, int);
>  int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
>  struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>  unsigned int hidinput_count_leds(struct hid_device *hid);
> +__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>  void hid_output_report(struct hid_report *report, __u8 *data);
>  struct hid_device *hid_allocate_device(void);
>  struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v2 02/11] HID: core: fix unit exponent parsing

[Henrik Rydberg]

Hi Benjamin,

> HID spec details special values for the HID field unit exponent.
> Basically, the range [0x8..0xf] correspond to [-8..-1].
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---

Standard two's complement, in other words?

>  drivers/hid/hid-core.c  | 10 +++++++++-
>  drivers/hid/hid-input.c | 19 +++++++++++++------
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9904776..6bde6e4 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -315,6 +315,7 @@ static s32 item_sdata(struct hid_item *item)
>  
>  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
>  {
> +	__u32 raw_value;
>  	switch (item->tag) {
>  	case HID_GLOBAL_ITEM_TAG_PUSH:
>  
> @@ -365,7 +366,14 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
>  		return 0;
>  
>  	case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
> -		parser->global.unit_exponent = item_sdata(item);
> +		/* Units exponent negative numbers are given through a special
> +		 * table.
> +		 * See "6.2.2.7 Global Items" for more information. */
> +		raw_value = item_udata(item);
> +		if ((raw_value >> 3) == 1)
> +			parser->global.unit_exponent = raw_value - 16;
> +		else
> +			parser->global.unit_exponent = raw_value;

I beleive the function you want is snto32(value, 4).

>  		return 0;
>  
>  	case HID_GLOBAL_ITEM_TAG_UNIT:
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 1b0adc3..fc9f2b5 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -215,7 +215,7 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  					field->logical_minimum;
>  	__s32 physical_extents = field->physical_maximum -
>  					field->physical_minimum;
> -	__s32 prev;
> +	__s32 prev, tmp_exponent;
>  
>  	/* Check if the extents are sane */
>  	if (logical_extents <= 0 || physical_extents <= 0)
> @@ -235,17 +235,24 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  	case ABS_MT_TOOL_Y:
>  	case ABS_MT_TOUCH_MAJOR:
>  	case ABS_MT_TOUCH_MINOR:
> -		if (field->unit == 0x11) {		/* If centimeters */
> +		if (field->unit & 0xffffff00)		/* Not a length */
> +			return 0;
> +		tmp_exponent = field->unit >> 4;
> +		if ((tmp_exponent >> 3) == 1)
> +			tmp_exponent -= 16;

Ditto.

> +		switch (field->unit & 0xf) {
> +		case 0x1:				/* If centimeters */
>  			/* Convert to millimeters */
> -			unit_exponent += 1;
> -		} else if (field->unit == 0x13) {	/* If inches */
> +			unit_exponent += tmp_exponent;
> +			break;
> +		case 0x3:				/* If inches */
>  			/* Convert to millimeters */
>  			prev = physical_extents;
>  			physical_extents *= 254;
>  			if (physical_extents < prev)
>  				return 0;
> -			unit_exponent -= 1;
> -		} else {
> +			unit_exponent += tmp_exponent - 2;
> +		default:
>  			return 0;
>  		}
>  		break;
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v2 03/11] HID: hid-input: add usage_index argument in input_mapping and event.

[Henrik Rydberg]

Hi Benjamin,

> Currently, there is no way to know the index of the current field
> in the .input_mapping and .event callbacks  when this field is inside
> an array of HID fields.
> This patch forwards this index to the input_mapping and event
> callbacks.

I agree with the idea, but the function argument list is becoming
ridiculously long... Could we remove the usage pointer argument, at
least?

	int (*event)(struct hid_device *hdev, struct hid_field *field,
			unsigned int usage_index, __s32 value);


> @@ -1071,19 +1072,24 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
>  	for (n = 0; n < count; n++) {
>  
>  		if (HID_MAIN_ITEM_VARIABLE & field->flags) {
> -			hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
> +			hid_process_event(hid, field, &field->usage[n], n,
> +				value[n], interrupt);
>  			continue;
>  		}
>  
>  		if (field->value[n] >= min && field->value[n] <= max
>  			&& field->usage[field->value[n] - min].hid
>  			&& search(value, field->value[n], count))
> -				hid_process_event(hid, field, &field->usage[field->value[n] - min], 0, interrupt);
> +				hid_process_event(hid, field,
> +					&field->usage[field->value[n] - min], n,
> +					0, interrupt);

Wrong index?

>  
>  		if (value[n] >= min && value[n] <= max
>  			&& field->usage[value[n] - min].hid
>  			&& search(field->value, value[n], count))
> -				hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
> +				hid_process_event(hid, field,
> +					&field->usage[value[n] - min], n,
> +					1, interrupt);

Wrong index?

>  	}
>  
>  	memcpy(field->value, value, count * sizeof(__s32));

Thanks,
Henrik

Re: [PATCH v2 04/11] HID: hid-multitouch: support arrays for the split of the touches in a report

[Henrik Rydberg]

Hi Benjamin,

> Win8 certification introduced the ability to transmit two X and two Y per
> touch. The specification precises that it must be in an array, with a
> report count == 2.

The number two never really enters the patch, so maybe it should be
dropped to avoid confusion. It probably makes more sense to comment on
in a later patch, when the reports are actually used.

> 
> This test guarantees that we split the touches on the last element
> in this array.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
>  drivers/hid/hid-multitouch.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 725d155..95562d8 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -577,12 +577,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>  			return 0;
>  		}
>  
> -		if (usage->hid == td->last_slot_field)
> -			mt_complete_slot(td, field->hidinput->input);
> -
> -		if (field->index == td->last_field_index
> -			&& td->num_received >= td->num_expected)
> -			mt_sync_frame(td, field->hidinput->input);
> +		if (usage_index + 1 == field->report_count) {
> +			/* we only take into account the last report
> +			 * of a field if report_count > 1 */

Seems we could drop "of a field if report_count > 1" here, and be even
more correct.

> +			if (usage->hid == td->last_slot_field)
> +				mt_complete_slot(td, field->hidinput->input);
> +
> +			if (field->index == td->last_field_index
> +				&& td->num_received >= td->num_expected)
> +				mt_sync_frame(td, field->hidinput->input);
> +		}
>  
>  	}
>  
> -- 
> 1.7.11.7

Thanks,
Henrik

Re: [PATCH v2 05/11] HID: hid-multitouch: get maxcontacts also from logical_max value

[Henrik Rydberg]

On Fri, Oct 26, 2012 at 10:44:21AM +0200, Benjamin Tissoires wrote:
> Win8 devices are required to present the feature "Maximum Contact Number".
> Fortunately all win7 devices I've seen presents this feature.
> If the current value is 0, then, the driver can get the actual supported
> contact count by refering to the logical_max.
> This win8 specification ensures that logical_max may not be above 250.
> This also allows us to detect when devices like irtouch or stantum reports
> an obviously wrong value of 255.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 95562d8..41f2981 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -121,6 +121,7 @@ struct mt_device {
>  #define MT_CLS_GENERALTOUCH_PWT_TENFINGERS	0x0109
>  
>  #define MT_DEFAULT_MAXCONTACT	10
> +#define MT_MAX_MAXCONTACT	250
>  
>  #define MT_USB_DEVICE(v, p)	HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH, v, p)
>  #define MT_BT_DEVICE(v, p)	HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_MULTITOUCH, v, p)
> @@ -283,6 +284,9 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  	case HID_DG_CONTACTMAX:
>  		td->maxcontact_report_id = field->report->id;
>  		td->maxcontacts = field->value[0];
> +		if (!td->maxcontacts &&
> +		    field->logical_maximum <= MT_MAX_MAXCONTACT)
> +			td->maxcontacts = field->logical_maximum;
>  		if (td->mtclass.maxcontacts)
>  			/* check if the maxcontacts is given by the class */
>  			td->maxcontacts = td->mtclass.maxcontacts;
> -- 
> 1.7.11.7
> 

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

Re: [PATCH v2 06/11] HID: hid-multitouch: support T and C for win8 devices

[Henrik Rydberg]

Hi Benjamin,

> Win8 input specification clarifies the X and Y sent by devices.
> It distincts the position where the user wants to Touch (T) from
> the center of the ellipsoide (C). This patch enable supports for this
> distinction in hid-multitouch.
> 
> We recognize Win8 certified devices from their vendor feature 0xff0000c5
> where Microsoft put a signed blob in the report to check if the device
> passed the certification.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 53 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 44 insertions(+), 9 deletions(-)

This is great, just a few comments below.

> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 41f2981..000c979 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -52,9 +52,10 @@ MODULE_LICENSE("GPL");
>  #define MT_QUIRK_VALID_IS_CONFIDENCE	(1 << 6)
>  #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE	(1 << 8)
>  #define MT_QUIRK_NO_AREA		(1 << 9)
> +#define MT_QUIRK_WIN_8_CERTIFIED	(1 << 10)
>  
>  struct mt_slot {
> -	__s32 x, y, p, w, h;
> +	__s32 x, y, cx, cy, p, w, h;
>  	__s32 contactid;	/* the device ContactID assigned to this slot */
>  	bool touch_state;	/* is the touch valid? */
>  };
> @@ -292,6 +293,10 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  			td->maxcontacts = td->mtclass.maxcontacts;
>  
>  		break;
> +	case 0xff0000c5:
> +		if (field->report_count == 256 && field->report_size == 8)
> +			td->mtclass.quirks |= MT_QUIRK_WIN_8_CERTIFIED;
> +		break;
>  	}
>  }
>  
> @@ -350,18 +355,36 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	case HID_UP_GENDESK:
>  		switch (usage->hid) {
>  		case HID_GD_X:
> -			hid_map_usage(hi, usage, bit, max,
> +			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED &&

Parenthesis, please. Precedence is not always enough.

> +				usage_index) {
> +				hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_TOOL_X);
> +				set_abs(hi->input, ABS_MT_TOOL_X, field,
> +					cls->sn_move);
> +			} else {
> +				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);
> +				set_abs(hi->input, ABS_MT_POSITION_X, field,
> +					cls->sn_move);
> +			}
> +

Do we really want to do the latter several times, even if the device is not a win8 one?

>  			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_GD_Y:
> -			hid_map_usage(hi, usage, bit, max,
> +			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED &&

Ditto.

> +				usage_index) {
> +				hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_TOOL_Y);
> +				set_abs(hi->input, ABS_MT_TOOL_Y, field,
> +					cls->sn_move);
> +			} else {
> +				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);
> +				set_abs(hi->input, ABS_MT_POSITION_Y, field,
> +					cls->sn_move);
> +			}
> +

Ditto.

>  			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
> @@ -502,6 +525,12 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  
>  			input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
>  			input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
> +			if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED) {
> +				input_event(input, EV_ABS, ABS_MT_TOOL_X,
> +					s->cx);

Won't this fit on one line?

> +				input_event(input, EV_ABS, ABS_MT_TOOL_Y,
> +					s->cy);
> +			}
>  			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
>  			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> @@ -553,10 +582,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>  			td->curdata.p = value;
>  			break;
>  		case HID_GD_X:
> -			td->curdata.x = value;
> +			if (usage->code == ABS_MT_POSITION_X)
> +				td->curdata.x = value;
> +			else
> +				td->curdata.cx = value;

Since cx is the new value, reversing the logic would make sense here.

>  			break;
>  		case HID_GD_Y:
> -			td->curdata.y = value;
> +			if (usage->code == ABS_MT_POSITION_Y)
> +				td->curdata.y = value;
> +			else
> +				td->curdata.cy = value;
>  			break;
>  		case HID_DG_WIDTH:
>  			td->curdata.w = value;
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v2 07/11] HID: hid-multitouch: move ALWAYS_VALID quirk check

[Henrik Rydberg]

Hi Benjamin,

> Win 8 device specification changed the requirements for the hid usages
> of the multitouch devices. Now InRange is optional and must be only
> used when the device supports hovering.
> 
> This ensures that the quirk ALWAYS_VALID is taken into account and
> also ensures its precedence over the other VALID* quirks.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Since we seem to never actually reset td, this seems equivalent,
unless some device added ALWAYS VALID by mistake, even if INRANGE is
not part of the report descriptor.

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 000c979..43bd8e8 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -506,7 +506,7 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
>   */
>  static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  {
> -	if (td->curvalid) {
> +	if (td->curvalid || td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID) {

I found at least one presence of this construct in the kernel, but I
think the overwhelming majority use parenthesis.

>  		int slotnum = mt_compute_slot(td, input);
>  		struct mt_slot *s = &td->curdata;
>  
> @@ -561,9 +561,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>  	if (hid->claimed & HID_CLAIMED_INPUT) {
>  		switch (usage->hid) {
>  		case HID_DG_INRANGE:
> -			if (quirks & MT_QUIRK_ALWAYS_VALID)
> -				td->curvalid = true;
> -			else if (quirks & MT_QUIRK_VALID_IS_INRANGE)
> +			if (quirks & MT_QUIRK_VALID_IS_INRANGE)
>  				td->curvalid = value;
>  			break;
>  		case HID_DG_TIPSWITCH:
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Henrik Rydberg]

On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote:
> Win 8 specification is much more precise than the Win 7 one.
> Moreover devices that need to take certification must be submitted
> to Microsoft.
> 
> The result is a better protocol support and we can rely on that to
> skip all the messy tests we used to do.
> 
> The protocol specify the fact that each valid touch must be reported
> within a frame, and that the release touch coordinate must be the
> same than the last known touch.
> So we can use the always_valid quirk and dismiss reports when we
> touch coordiante do not follow this rule.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

Why do we need this patch?

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 43bd8e8..bd23f19 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -510,6 +510,18 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  		int slotnum = mt_compute_slot(td, input);
>  		struct mt_slot *s = &td->curdata;
>  
> +		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
> +		    !s->touch_state) {
> +			struct input_mt_slot *slot = &input->mt->slots[slotnum];
> +			int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
> +			int prv_y = input_mt_get_value(slot, ABS_MT_POSITION_Y);
> +			/* valid releases occurs when the last x and y positions
> +			 * are the same as the last known touch. */
> +			if (!input_mt_is_active(slot) ||
> +			    prv_x != s->x || prv_y != s->y)
> +				return;
> +		}
> +
>  		if (slotnum < 0 || slotnum >= td->maxcontacts)
>  			return;
>  
> @@ -681,8 +693,8 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>  {
>  	__s32 quirks = td->mtclass.quirks;
>  
> -	/* unknown serial device needs special quirks */
> -	if (td->touches_by_report == 1) {
> +	/* unknown serial devices or win8 ones need special quirks */
> +	if (td->touches_by_report == 1 || quirks & MT_QUIRK_WIN_8_CERTIFIED) {
>  		quirks |= MT_QUIRK_ALWAYS_VALID;
>  		quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
>  		quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
> -- 
> 1.7.11.7
> 

Henrik

Re: [PATCH v2 09/11] HID: hid-multitouch: support for hovering devices

[Henrik Rydberg]

Hi Benjamin,

> Win8 devices supporting hovering must provides InRange HID field.
> The information that the finger is here but is not touching the surface
> is sent to the user space through ABS_MT_DISTANCE as required by the
> multitouch protocol.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)

I suppose the idea here is to send position and distance values even
when there is no touch, but the code does not seem to do that?

> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index bd23f19..c0ab1c6 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -55,7 +55,7 @@ MODULE_LICENSE("GPL");
>  #define MT_QUIRK_WIN_8_CERTIFIED	(1 << 10)
>  
>  struct mt_slot {
> -	__s32 x, y, cx, cy, p, w, h;
> +	__s32 x, y, cx, cy, z, p, w, h;
>  	__s32 contactid;	/* the device ContactID assigned to this slot */
>  	bool touch_state;	/* is the touch valid? */
>  };
> @@ -394,6 +394,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	case HID_UP_DIGITIZER:
>  		switch (usage->hid) {
>  		case HID_DG_INRANGE:
> +			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED) {
> +				hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_DISTANCE);
> +				input_set_abs_params(hi->input,
> +					ABS_MT_DISTANCE, 0, 1, 0, 0);
> +			}
>  			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
> @@ -511,6 +517,11 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  		struct mt_slot *s = &td->curdata;
>  
>  		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
> +		    !test_bit(ABS_MT_DISTANCE, input->absbit))
> +			/* If InRange is not present, rely on TipSwitch */
> +			s->touch_state = !s->z;
> +
> +		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
>  		    !s->touch_state) {
>  			struct input_mt_slot *slot = &input->mt->slots[slotnum];
>  			int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
> @@ -543,6 +554,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  				input_event(input, EV_ABS, ABS_MT_TOOL_Y,
>  					s->cy);
>  			}
> +			input_event(input, EV_ABS, ABS_MT_DISTANCE, s->z);

This only happens if touch_state is true?

>  			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
>  			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> @@ -575,11 +587,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>  		case HID_DG_INRANGE:
>  			if (quirks & MT_QUIRK_VALID_IS_INRANGE)
>  				td->curvalid = value;
> +			if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
> +				td->curdata.touch_state = value;
>  			break;
>  		case HID_DG_TIPSWITCH:
>  			if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
>  				td->curvalid = value;
> -			td->curdata.touch_state = value;
> +			if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
> +				td->curdata.z = !value;

Here, z is a boolean?

> +			else
> +				td->curdata.touch_state = value;
>  			break;
>  		case HID_DG_CONFIDENCE:
>  			if (quirks & MT_QUIRK_VALID_IS_CONFIDENCE)
> -- 
> 1.7.11.7
> 

Henrik

Re: [PATCH v2 10/11] HID: introduce Scan Time

[Henrik Rydberg]

Hi Benjamin,

> Win 8 digitizer devices provides the actual scan time computed by the
> hardware itself. The value is global to the frame and is not specific
> to the multitouch protocol (though only touch, not pen, should use it
> according to the specification).
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  Documentation/input/event-codes.txt |  7 +++++++
>  drivers/hid/hid-input.c             |  4 ++++
>  drivers/hid/hid-multitouch.c        | 11 +++++++++--
>  include/linux/hid.h                 |  1 +
>  include/linux/input.h               |  1 +
>  5 files changed, 22 insertions(+), 2 deletions(-)

This is a nice feature, useful in many other contexts. As such, I
think it should be defined in the context of the input subsystem, with
a more specific definition added to the documentation. For instance,
is 100us suitable? When should it start at zero, at BTN_TOUCH? Or
should it perhaps wrap around on unsigned integer instead? Or display
the difference from the last event?

> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
> index 53305bd..8f8c908 100644
> --- a/Documentation/input/event-codes.txt
> +++ b/Documentation/input/event-codes.txt
> @@ -174,6 +174,13 @@ A few EV_ABS codes have special meanings:
>      the input device may be used freely in three dimensions, consider ABS_Z
>      instead.
>  
> +* ABS_SCAN_TIME:
> +  - Used when the device provides a timestamp for each frame. The unit must be
> +    100us, and may be reset when the device don't send any events for a
> +    period of time. The values increment at each frame and thus, it can roll
> +    back to 0 when reach logical_max. If the device does not provide this
> +    information, the driver must not provide it to the user space.
> +
>  * ABS_MT_<name>:
>    - Used to describe multitouch input events. Please see
>      multi-touch-protocol.txt for details.
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 16cc89a..5fe7bd3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -675,6 +675,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  			map_key_clear(BTN_STYLUS2);
>  			break;
>  
> +		case 0x56: /* Scan Time */
> +			map_abs(ABS_SCAN_TIME);
> +			break;
> +

Is it not enough to map it in the case below? Or you mean this is
picked up by hid core?

>  		default:  goto unknown;
>  		}
>  		break;
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c0ab1c6..21a120b 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -447,12 +447,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
> +		case HID_DG_SCANTIME:
> +			hid_map_usage(hi, usage, bit, max,
> +				EV_ABS, ABS_SCAN_TIME);
> +			set_abs(hi->input, ABS_SCAN_TIME, field, 0);
> +			td->last_field_index = field->index;
> +			return 1;
>  		case HID_DG_CONTACTCOUNT:
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_CONTACTMAX:
> -			/* we don't set td->last_slot_field as contactcount and
> -			 * contact max are global to the report */
> +			/* we don't set td->last_slot_field as scan time,
> +			 * contactcount and contact max are global to the
> +			 * report */
>  			td->last_field_index = field->index;
>  			return -1;
>  		case HID_DG_TOUCH:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 6216529..99a6418 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -279,6 +279,7 @@ struct hid_item {
>  #define HID_DG_DEVICEINDEX	0x000d0053
>  #define HID_DG_CONTACTCOUNT	0x000d0054
>  #define HID_DG_CONTACTMAX	0x000d0055
> +#define HID_DG_SCANTIME		0x000d0056
>  
>  /*
>   * HID report types --- Ouch! HID spec says 1 2 3!
> diff --git a/include/linux/input.h b/include/linux/input.h
> index ba48743..73c3a96 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -796,6 +796,7 @@ struct input_keymap_entry {
>  #define ABS_TILT_X		0x1a
>  #define ABS_TILT_Y		0x1b
>  #define ABS_TOOL_WIDTH		0x1c
> +#define ABS_SCAN_TIME		0x1d
>  
>  #define ABS_VOLUME		0x20
>  
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Henrik Rydberg]

Hi Benjamin,

> This patch factorizes the hid set_feature command by using
> hid_device->hid_output_raw_report instead of direclty relying on
> usbhid. This makes the driver usb independant.
> 
> However I still can't remove the 2 usb related headers because the
> function mt_resume has a specific patch for usb devices.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 26 deletions(-)

In my drawer, I have a patchset that aims to remove all usbhid
dependence, from all the drivers. Perhaps the attached patch is
something to consider here?

> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 21a120b..33038c5 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -670,47 +670,58 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>  	return 1;
>  }
>  
> -static void mt_set_input_mode(struct hid_device *hdev)
> +static void mt_set_feature(struct hid_device *hdev, __s8 feature_id,
> +	u8 value, size_t index)
>  {
> -	struct mt_device *td = hid_get_drvdata(hdev);
>  	struct hid_report *r;
>  	struct hid_report_enum *re;
> +	u8 *data;
> +	u8 max_value;
> +	int len;
> +
> +	if (feature_id < 0 || !hdev->hid_output_raw_report)
> +		return;
> +
> +	re = &hdev->report_enum[HID_FEATURE_REPORT];
> +	r = re->report_id_hash[feature_id];
> +	if (!r)
> +		return;
> +
> +	len = ((r->size - 1) >> 3) + 1 + (r->id > 0);
> +	max_value = r->field[0]->logical_maximum;
> +	value = min(value, max_value);
>  
> -	if (td->inputmode < 0)
> +	if (r->field[0]->value[index] == value || len < 2 || index + 1 >= len)
>  		return;
>  
> -	re = &(hdev->report_enum[HID_FEATURE_REPORT]);
> -	r = re->report_id_hash[td->inputmode];
> -	if (r) {
> -		r->field[0]->value[td->inputmode_index] = 0x02;
> -		usbhid_submit_report(hdev, r, USB_DIR_OUT);
> +	data = kzalloc(len, GFP_ATOMIC);
> +	if (!data) {
> +		hid_warn(hdev, "output queueing failed\n");
> +		return;
>  	}
> +
> +	data[0] = r->id;
> +	data[index + 1] = value;
> +	hdev->hid_output_raw_report(hdev, data, len, HID_FEATURE_REPORT);
> +	kfree(data);
>  }
>  
> -static void mt_set_maxcontacts(struct hid_device *hdev)
> +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;
> -	int fieldmax, max;
>  
> -	if (td->maxcontact_report_id < 0)
> -		return;
> +	mt_set_feature(hdev, td->inputmode, 0x02, td->inputmode_index);
> +}
>  
> -	if (!td->mtclass.maxcontacts)
> +static void mt_set_maxcontacts(struct hid_device *hdev)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	int max = td->mtclass.maxcontacts;
> +
> +	if (!max)
>  		return;
>  
> -	re = &hdev->report_enum[HID_FEATURE_REPORT];
> -	r = re->report_id_hash[td->maxcontact_report_id];
> -	if (r) {
> -		max = td->mtclass.maxcontacts;
> -		fieldmax = r->field[0]->logical_maximum;
> -		max = min(fieldmax, max);
> -		if (r->field[0]->value[0] != max) {
> -			r->field[0]->value[0] = max;
> -			usbhid_submit_report(hdev, r, USB_DIR_OUT);
> -		}
> -	}
> +	mt_set_feature(hdev, td->maxcontact_report_id, max, 0);
>  }
>  
>  static void mt_post_parse_default_settings(struct mt_device *td)
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

---

>From 5d9a791e0a9e41bcea0fcb286e2849b403675f37 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Mon, 2 Jul 2012 20:38:21 +0200
Subject: [PATCH 3/7] hid: extend interface with report requests

---
 drivers/hid/usbhid/hid-core.c | 14 ++++++++++++++
 include/linux/hid.h           | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index de1f9ac..3c618da 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1251,6 +1251,18 @@ static int usbhid_power(struct hid_device *hid, int lvl)
 	return r;
 }
 
+static void usbhid_send(struct hid_device *hid, struct hid_report *rep, int req)
+{
+	switch (req) {
+	case HID_REQ_GET_REPORT:
+		usbhid_submit_report(hid, rep, USB_DIR_IN);
+		break;
+	case HID_REQ_SET_REPORT:
+		usbhid_submit_report(hid, rep, USB_DIR_OUT);
+		break;
+	}
+}
+
 static struct hid_ll_driver usb_hid_driver = {
 	.parse = usbhid_parse,
 	.start = usbhid_start,
@@ -1259,6 +1271,8 @@ static struct hid_ll_driver usb_hid_driver = {
 	.close = usbhid_close,
 	.power = usbhid_power,
 	.hidinput_input_event = usb_hidinput_input_event,
+	.send = usbhid_send,
+	.wait = usbhid_wait_io,
 };
 
 static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 241eb40..5e169c1 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -686,6 +686,8 @@ struct hid_driver {
  * @hidinput_input_event: event input event (e.g. ff or leds)
  * @parse: this method is called only once to parse the device data,
  *	   shouldn't allocate anything to not leak memory
+ * @send: send report request to device (e.g. feature report)
+ * @wait: wait for buffered io to complete (send/recv reports)
  */
 struct hid_ll_driver {
 	int (*start)(struct hid_device *hdev);
@@ -700,6 +702,11 @@ struct hid_ll_driver {
 			unsigned int code, int value);
 
 	int (*parse)(struct hid_device *hdev);
+
+	void (*send)(struct hid_device *hdev,
+		     struct hid_report *report, int req);
+	int (*wait)(struct hid_device *hdev);
+
 };
 
 #define	PM_HINT_FULLON	1<<5
@@ -892,6 +899,32 @@ static inline int hid_hw_power(struct hid_device *hdev, int level)
 	return hdev->ll_driver->power ? hdev->ll_driver->power(hdev, level) : 0;
 }
 
+
+/**
+ * hid_hw_send - send report request to device
+ *
+ * @hdev: hid device
+ * @report: report to send
+ * @req: hid request type
+ */
+static inline void hid_hw_send(struct hid_device *hdev,
+			       struct hid_report *report, int req)
+{
+	if (hdev->ll_driver->send)
+		hdev->ll_driver->send(hdev, report, req);
+}
+
+/**
+ * hid_hw_wait - wait for buffered io to complete
+ *
+ * @hdev: hid device
+ */
+static inline void hid_hw_wait(struct hid_device *hdev)
+{
+	if (hdev->ll_driver->wait)
+		hdev->ll_driver->wait(hdev);
+}
+
 int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
 		int interrupt);
 
-- 
1.7.11.1

Re: [PATCH v2 01/11] HID: hid-input: export hidinput_calc_abs_res

[Benjamin Tissoires]

Hi Henrik,

first thanks for the full review of the patch series.

If you think it's better, I'll split the patch in 2 to put aside the
DIV_ROUND_CLOSEST.

Cheers,
Benjamin

On Mon, Oct 29, 2012 at 7:57 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Exporting this function allows us to calculate the resolution in third
>> party drivers like hid-multitouch.
>> This patch also complete the function with additional valid axes.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>
> Nice, but please see comment below.
>
>>  drivers/hid/hid-input.c      | 11 +++++++++--
>>  drivers/hid/hid-multitouch.c |  1 +
>>  include/linux/hid.h          |  1 +
>>  3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index d917c0d..1b0adc3 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -208,7 +208,7 @@ static int hidinput_setkeycode(struct input_dev *dev,
>>   * Only exponent 1 length units are processed. Centimeters and inches are
>>   * converted to millimeters. Degrees are converted to radians.
>>   */
>> -static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>> +__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>>  {
>>       __s32 unit_exponent = field->unit_exponent;
>>       __s32 logical_extents = field->logical_maximum -
>> @@ -229,6 +229,12 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>>       case ABS_X:
>>       case ABS_Y:
>>       case ABS_Z:
>> +     case ABS_MT_POSITION_X:
>> +     case ABS_MT_POSITION_Y:
>> +     case ABS_MT_TOOL_X:
>> +     case ABS_MT_TOOL_Y:
>> +     case ABS_MT_TOUCH_MAJOR:
>> +     case ABS_MT_TOUCH_MINOR:
>>               if (field->unit == 0x11) {              /* If centimeters */
>>                       /* Convert to millimeters */
>>                       unit_exponent += 1;
>> @@ -281,8 +287,9 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>>       }
>>
>>       /* Calculate resolution */
>> -     return logical_extents / physical_extents;
>> +     return DIV_ROUND_CLOSEST(logical_extents, physical_extents);
>
> This line might be best left alone; if the round-off matters, you now
> risk regressing a carefully tuned userland.
>
>>  }
>> +EXPORT_SYMBOL_GPL(hidinput_calc_abs_res);
>>
>>  #ifdef CONFIG_HID_BATTERY_STRENGTH
>>  static enum power_supply_property hidinput_battery_props[] = {
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index c97011c..375a38d 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -298,6 +298,7 @@ static void set_abs(struct input_dev *input, unsigned int code,
>>       int fmax = field->logical_maximum;
>>       int fuzz = snratio ? (fmax - fmin) / snratio : 0;
>>       input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
>> +     input_abs_set_res(input, code, hidinput_calc_abs_res(field, code));
>>  }
>>
>>  static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 7e1f37d..9edb06c 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -743,6 +743,7 @@ int hid_input_report(struct hid_device *, int type, u8 *, int, int);
>>  int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
>>  struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>>  unsigned int hidinput_count_leds(struct hid_device *hid);
>> +__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>>  void hid_output_report(struct hid_report *report, __u8 *data);
>>  struct hid_device *hid_allocate_device(void);
>>  struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 02/11] HID: core: fix unit exponent parsing

[Benjamin Tissoires]

Hi Henrik,

On Mon, Oct 29, 2012 at 8:05 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> HID spec details special values for the HID field unit exponent.
>> Basically, the range [0x8..0xf] correspond to [-8..-1].
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>
> Standard two's complement, in other words?

yes, but for 4-bits values. The thing is that I didn't managed to
figure out if it was allowed to give unit exponent with more than 4
bits...

>
>>  drivers/hid/hid-core.c  | 10 +++++++++-
>>  drivers/hid/hid-input.c | 19 +++++++++++++------
>>  2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 9904776..6bde6e4 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -315,6 +315,7 @@ static s32 item_sdata(struct hid_item *item)
>>
>>  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
>>  {
>> +     __u32 raw_value;
>>       switch (item->tag) {
>>       case HID_GLOBAL_ITEM_TAG_PUSH:
>>
>> @@ -365,7 +366,14 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
>>               return 0;
>>
>>       case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
>> -             parser->global.unit_exponent = item_sdata(item);
>> +             /* Units exponent negative numbers are given through a special
>> +              * table.
>> +              * See "6.2.2.7 Global Items" for more information. */
>> +             raw_value = item_udata(item);
>> +             if ((raw_value >> 3) == 1)
>> +                     parser->global.unit_exponent = raw_value - 16;
>> +             else
>> +                     parser->global.unit_exponent = raw_value;
>
> I beleive the function you want is snto32(value, 4).

but to be sure, we should still do a test against (raw_value & 0xf0)
for values with more than 4 bits, no?

Cheers,
Benjamin

>
>>               return 0;
>>
>>       case HID_GLOBAL_ITEM_TAG_UNIT:
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 1b0adc3..fc9f2b5 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -215,7 +215,7 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>>                                       field->logical_minimum;
>>       __s32 physical_extents = field->physical_maximum -
>>                                       field->physical_minimum;
>> -     __s32 prev;
>> +     __s32 prev, tmp_exponent;
>>
>>       /* Check if the extents are sane */
>>       if (logical_extents <= 0 || physical_extents <= 0)
>> @@ -235,17 +235,24 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>>       case ABS_MT_TOOL_Y:
>>       case ABS_MT_TOUCH_MAJOR:
>>       case ABS_MT_TOUCH_MINOR:
>> -             if (field->unit == 0x11) {              /* If centimeters */
>> +             if (field->unit & 0xffffff00)           /* Not a length */
>> +                     return 0;
>> +             tmp_exponent = field->unit >> 4;
>> +             if ((tmp_exponent >> 3) == 1)
>> +                     tmp_exponent -= 16;
>
> Ditto.
>
>> +             switch (field->unit & 0xf) {
>> +             case 0x1:                               /* If centimeters */
>>                       /* Convert to millimeters */
>> -                     unit_exponent += 1;
>> -             } else if (field->unit == 0x13) {       /* If inches */
>> +                     unit_exponent += tmp_exponent;
>> +                     break;
>> +             case 0x3:                               /* If inches */
>>                       /* Convert to millimeters */
>>                       prev = physical_extents;
>>                       physical_extents *= 254;
>>                       if (physical_extents < prev)
>>                               return 0;
>> -                     unit_exponent -= 1;
>> -             } else {
>> +                     unit_exponent += tmp_exponent - 2;
>> +             default:
>>                       return 0;
>>               }
>>               break;
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 03/11] HID: hid-input: add usage_index argument in input_mapping and event.

[Benjamin Tissoires]

On Mon, Oct 29, 2012 at 8:25 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Currently, there is no way to know the index of the current field
>> in the .input_mapping and .event callbacks  when this field is inside
>> an array of HID fields.
>> This patch forwards this index to the input_mapping and event
>> callbacks.
>
> I agree with the idea, but the function argument list is becoming
> ridiculously long... Could we remove the usage pointer argument, at
> least?

yeah, totally agree. Let me just check whether it will not introduce
more problems than it solves for my driver.

>
>         int (*event)(struct hid_device *hdev, struct hid_field *field,
>                         unsigned int usage_index, __s32 value);
>
>
>> @@ -1071,19 +1072,24 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
>>       for (n = 0; n < count; n++) {
>>
>>               if (HID_MAIN_ITEM_VARIABLE & field->flags) {
>> -                     hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
>> +                     hid_process_event(hid, field, &field->usage[n], n,
>> +                             value[n], interrupt);
>>                       continue;
>>               }
>>
>>               if (field->value[n] >= min && field->value[n] <= max
>>                       && field->usage[field->value[n] - min].hid
>>                       && search(value, field->value[n], count))
>> -                             hid_process_event(hid, field, &field->usage[field->value[n] - min], 0, interrupt);
>> +                             hid_process_event(hid, field,
>> +                                     &field->usage[field->value[n] - min], n,
>> +                                     0, interrupt);
>
> Wrong index?

oops, I'll have to check that.

Thanks,
Benjamin

>
>>
>>               if (value[n] >= min && value[n] <= max
>>                       && field->usage[value[n] - min].hid
>>                       && search(field->value, value[n], count))
>> -                             hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
>> +                             hid_process_event(hid, field,
>> +                                     &field->usage[value[n] - min], n,
>> +                                     1, interrupt);
>
> Wrong index?
>
>>       }
>>
>>       memcpy(field->value, value, count * sizeof(__s32));
>
> Thanks,
> Henrik

Re: [PATCH v2 04/11] HID: hid-multitouch: support arrays for the split of the touches in a report

[Benjamin Tissoires]

Hi Henrik,

On Mon, Oct 29, 2012 at 10:49 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Win8 certification introduced the ability to transmit two X and two Y per
>> touch. The specification precises that it must be in an array, with a
>> report count == 2.
>
> The number two never really enters the patch, so maybe it should be
> dropped to avoid confusion. It probably makes more sense to comment on
> in a later patch, when the reports are actually used.

Yep, it seems that the commit message is not good. I'll rewrite it in v3.

>
>>
>> This test guarantees that we split the touches on the last element
>> in this array.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>>  drivers/hid/hid-multitouch.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 725d155..95562d8 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -577,12 +577,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>                       return 0;
>>               }
>>
>> -             if (usage->hid == td->last_slot_field)
>> -                     mt_complete_slot(td, field->hidinput->input);
>> -
>> -             if (field->index == td->last_field_index
>> -                     && td->num_received >= td->num_expected)
>> -                     mt_sync_frame(td, field->hidinput->input);
>> +             if (usage_index + 1 == field->report_count) {
>> +                     /* we only take into account the last report
>> +                      * of a field if report_count > 1 */
>
> Seems we could drop "of a field if report_count > 1" here, and be even
> more correct.

oops ;-)

Cheers,
Benjamin

>
>> +                     if (usage->hid == td->last_slot_field)
>> +                             mt_complete_slot(td, field->hidinput->input);
>> +
>> +                     if (field->index == td->last_field_index
>> +                             && td->num_received >= td->num_expected)
>> +                             mt_sync_frame(td, field->hidinput->input);
>> +             }
>>
>>       }
>>
>> --
>> 1.7.11.7
>
> Thanks,
> Henrik

Re: [PATCH v2 06/11] HID: hid-multitouch: support T and C for win8 devices

[Benjamin Tissoires]

Hi Henrik,

On Mon, Oct 29, 2012 at 11:00 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Win8 input specification clarifies the X and Y sent by devices.
>> It distincts the position where the user wants to Touch (T) from
>> the center of the ellipsoide (C). This patch enable supports for this
>> distinction in hid-multitouch.
>>
>> We recognize Win8 certified devices from their vendor feature 0xff0000c5
>> where Microsoft put a signed blob in the report to check if the device
>> passed the certification.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 53 ++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 44 insertions(+), 9 deletions(-)
>
> This is great, just a few comments below.
>
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 41f2981..000c979 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -52,9 +52,10 @@ MODULE_LICENSE("GPL");
>>  #define MT_QUIRK_VALID_IS_CONFIDENCE (1 << 6)
>>  #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE (1 << 8)
>>  #define MT_QUIRK_NO_AREA             (1 << 9)
>> +#define MT_QUIRK_WIN_8_CERTIFIED     (1 << 10)
>>
>>  struct mt_slot {
>> -     __s32 x, y, p, w, h;
>> +     __s32 x, y, cx, cy, p, w, h;
>>       __s32 contactid;        /* the device ContactID assigned to this slot */
>>       bool touch_state;       /* is the touch valid? */
>>  };
>> @@ -292,6 +293,10 @@ static void mt_feature_mapping(struct hid_device *hdev,
>>                       td->maxcontacts = td->mtclass.maxcontacts;
>>
>>               break;
>> +     case 0xff0000c5:
>> +             if (field->report_count == 256 && field->report_size == 8)
>> +                     td->mtclass.quirks |= MT_QUIRK_WIN_8_CERTIFIED;
>> +             break;
>>       }
>>  }
>>
>> @@ -350,18 +355,36 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>       case HID_UP_GENDESK:
>>               switch (usage->hid) {
>>               case HID_GD_X:
>> -                     hid_map_usage(hi, usage, bit, max,
>> +                     if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED &&
>
> Parenthesis, please. Precedence is not always enough.

oops

>
>> +                             usage_index) {
>> +                             hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_TOOL_X);
>> +                             set_abs(hi->input, ABS_MT_TOOL_X, field,
>> +                                     cls->sn_move);
>> +                     } else {
>> +                             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);
>> +                             set_abs(hi->input, ABS_MT_POSITION_X, field,
>> +                                     cls->sn_move);
>> +                     }
>> +
>
> Do we really want to do the latter several times, even if the device is not a win8 one?

I don't get your point here. The only difference with the previous
release is that it will treat differently the first in the array than
the others. For non win8 devices, there is no changes in the behavior.
Could you elaborate a little bit more, please?

>
>>                       mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_GD_Y:
>> -                     hid_map_usage(hi, usage, bit, max,
>> +                     if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED &&
>
> Ditto.
>
>> +                             usage_index) {
>> +                             hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_TOOL_Y);
>> +                             set_abs(hi->input, ABS_MT_TOOL_Y, field,
>> +                                     cls->sn_move);
>> +                     } else {
>> +                             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);
>> +                             set_abs(hi->input, ABS_MT_POSITION_Y, field,
>> +                                     cls->sn_move);
>> +                     }
>> +
>
> Ditto.
>
>>                       mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>> @@ -502,6 +525,12 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>
>>                       input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
>>                       input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
>> +                     if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED) {
>> +                             input_event(input, EV_ABS, ABS_MT_TOOL_X,
>> +                                     s->cx);
>
> Won't this fit on one line?

I'm afraid not: 81 columns... ;-)

>
>> +                             input_event(input, EV_ABS, ABS_MT_TOOL_Y,
>> +                                     s->cy);
>> +                     }
>>                       input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
>>                       input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>>                       input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
>> @@ -553,10 +582,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>                       td->curdata.p = value;
>>                       break;
>>               case HID_GD_X:
>> -                     td->curdata.x = value;
>> +                     if (usage->code == ABS_MT_POSITION_X)
>> +                             td->curdata.x = value;
>> +                     else
>> +                             td->curdata.cx = value;
>
> Since cx is the new value, reversing the logic would make sense here.

ok

Cheers,
Benjamin

>
>>                       break;
>>               case HID_GD_Y:
>> -                     td->curdata.y = value;
>> +                     if (usage->code == ABS_MT_POSITION_Y)
>> +                             td->curdata.y = value;
>> +                     else
>> +                             td->curdata.cy = value;
>>                       break;
>>               case HID_DG_WIDTH:
>>                       td->curdata.w = value;
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 07/11] HID: hid-multitouch: move ALWAYS_VALID quirk check

[Benjamin Tissoires]

On Mon, Oct 29, 2012 at 11:16 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Win 8 device specification changed the requirements for the hid usages
>> of the multitouch devices. Now InRange is optional and must be only
>> used when the device supports hovering.
>>
>> This ensures that the quirk ALWAYS_VALID is taken into account and
>> also ensures its precedence over the other VALID* quirks.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> Since we seem to never actually reset td, this seems equivalent,
> unless some device added ALWAYS VALID by mistake, even if INRANGE is
> not part of the report descriptor.

Yes, it is equivalent for Win7 devices. The field InRange was
mandatory, and currently, nobody mixed ALWAYS_VALID with other quirks.
But now, InRange is optional, and device not supporting hovering
should not use it, so it's mandatory to move it at a place we can be
sure it will be taken into account.

>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 000c979..43bd8e8 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -506,7 +506,7 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input)
>>   */
>>  static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>  {
>> -     if (td->curvalid) {
>> +     if (td->curvalid || td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID) {
>
> I found at least one presence of this construct in the kernel, but I
> think the overwhelming majority use parenthesis.

oops, I was really angry against parenthesis in this patch series :)

Cheers,
Benjamin

>
>>               int slotnum = mt_compute_slot(td, input);
>>               struct mt_slot *s = &td->curdata;
>>
>> @@ -561,9 +561,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>       if (hid->claimed & HID_CLAIMED_INPUT) {
>>               switch (usage->hid) {
>>               case HID_DG_INRANGE:
>> -                     if (quirks & MT_QUIRK_ALWAYS_VALID)
>> -                             td->curvalid = true;
>> -                     else if (quirks & MT_QUIRK_VALID_IS_INRANGE)
>> +                     if (quirks & MT_QUIRK_VALID_IS_INRANGE)
>>                               td->curvalid = value;
>>                       break;
>>               case HID_DG_TIPSWITCH:
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

Hi Henrik,

On Mon, Oct 29, 2012 at 11:19 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote:
>> Win 8 specification is much more precise than the Win 7 one.
>> Moreover devices that need to take certification must be submitted
>> to Microsoft.
>>
>> The result is a better protocol support and we can rely on that to
>> skip all the messy tests we used to do.
>>
>> The protocol specify the fact that each valid touch must be reported
>> within a frame, and that the release touch coordinate must be the
>> same than the last known touch.
>> So we can use the always_valid quirk and dismiss reports when we
>> touch coordiante do not follow this rule.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> Why do we need this patch?

This patch prevents a corner case where the device use contactID 0 for
it's first reported touch.
Once you got the invalid touches, most of the time, contactID will be
0, x, y, and other fields too. So this ensures to avoid conflict
between valid values and garbage values. The problem lies in the fact
that we don't have the whole overview of the frame (with the contact
count) to decide which contacts are good and which are not.

Cheers,
Benjamin

>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 43bd8e8..bd23f19 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -510,6 +510,18 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>               int slotnum = mt_compute_slot(td, input);
>>               struct mt_slot *s = &td->curdata;
>>
>> +             if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
>> +                 !s->touch_state) {
>> +                     struct input_mt_slot *slot = &input->mt->slots[slotnum];
>> +                     int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
>> +                     int prv_y = input_mt_get_value(slot, ABS_MT_POSITION_Y);
>> +                     /* valid releases occurs when the last x and y positions
>> +                      * are the same as the last known touch. */
>> +                     if (!input_mt_is_active(slot) ||
>> +                         prv_x != s->x || prv_y != s->y)
>> +                             return;
>> +             }
>> +
>>               if (slotnum < 0 || slotnum >= td->maxcontacts)
>>                       return;
>>
>> @@ -681,8 +693,8 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>>  {
>>       __s32 quirks = td->mtclass.quirks;
>>
>> -     /* unknown serial device needs special quirks */
>> -     if (td->touches_by_report == 1) {
>> +     /* unknown serial devices or win8 ones need special quirks */
>> +     if (td->touches_by_report == 1 || quirks & MT_QUIRK_WIN_8_CERTIFIED) {
>>               quirks |= MT_QUIRK_ALWAYS_VALID;
>>               quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
>>               quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
>> --
>> 1.7.11.7
>>
>
> Henrik

Re: [PATCH v2 09/11] HID: hid-multitouch: support for hovering devices

[Benjamin Tissoires]

On Mon, Oct 29, 2012 at 11:31 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Win8 devices supporting hovering must provides InRange HID field.
>> The information that the finger is here but is not touching the surface
>> is sent to the user space through ABS_MT_DISTANCE as required by the
>> multitouch protocol.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> I suppose the idea here is to send position and distance values even
> when there is no touch, but the code does not seem to do that?

Well, the code does it, but I agree it's not very clear.

The touch_state field is not for win8 with hovering the fact that the
finger is touching the surface, but the fact that a finger is in range
of the device.
This is why tipswitch is filling z instead of touch_state.

I agree, it's not that clear, I'll try to do better in v3.

>
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index bd23f19..c0ab1c6 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -55,7 +55,7 @@ MODULE_LICENSE("GPL");
>>  #define MT_QUIRK_WIN_8_CERTIFIED     (1 << 10)
>>
>>  struct mt_slot {
>> -     __s32 x, y, cx, cy, p, w, h;
>> +     __s32 x, y, cx, cy, z, p, w, h;
>>       __s32 contactid;        /* the device ContactID assigned to this slot */
>>       bool touch_state;       /* is the touch valid? */
>>  };
>> @@ -394,6 +394,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>       case HID_UP_DIGITIZER:
>>               switch (usage->hid) {
>>               case HID_DG_INRANGE:
>> +                     if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED) {
>> +                             hid_map_usage(hi, usage, bit, max,
>> +                                     EV_ABS, ABS_MT_DISTANCE);
>> +                             input_set_abs_params(hi->input,
>> +                                     ABS_MT_DISTANCE, 0, 1, 0, 0);
>> +                     }
>>                       mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>> @@ -511,6 +517,11 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>               struct mt_slot *s = &td->curdata;
>>
>>               if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
>> +                 !test_bit(ABS_MT_DISTANCE, input->absbit))
>> +                     /* If InRange is not present, rely on TipSwitch */
>> +                     s->touch_state = !s->z;
>> +
>> +             if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED &&
>>                   !s->touch_state) {
>>                       struct input_mt_slot *slot = &input->mt->slots[slotnum];
>>                       int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
>> @@ -543,6 +554,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>                               input_event(input, EV_ABS, ABS_MT_TOOL_Y,
>>                                       s->cy);
>>                       }
>> +                     input_event(input, EV_ABS, ABS_MT_DISTANCE, s->z);
>
> This only happens if touch_state is true?

mmm, yes, we should move it outside this condition.

>
>>                       input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
>>                       input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
>>                       input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
>> @@ -575,11 +587,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>               case HID_DG_INRANGE:
>>                       if (quirks & MT_QUIRK_VALID_IS_INRANGE)
>>                               td->curvalid = value;
>> +                     if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
>> +                             td->curdata.touch_state = value;
>>                       break;
>>               case HID_DG_TIPSWITCH:
>>                       if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
>>                               td->curvalid = value;
>> -                     td->curdata.touch_state = value;
>> +                     if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
>> +                             td->curdata.z = !value;
>
> Here, z is a boolean?

this is what is written in Win 8 specification. So ABS_MT_DISTANCE
will have a range of [0..1] and this behavior is right. When we will
have device with real Z hovering measures, then it will be the time to
change this, but currently, we only have a boolean in InRange.

Cheers,
Benjamin

>
>> +                     else
>> +                             td->curdata.touch_state = value;
>>                       break;
>>               case HID_DG_CONFIDENCE:
>>                       if (quirks & MT_QUIRK_VALID_IS_CONFIDENCE)
>> --
>> 1.7.11.7
>>
>
> Henrik

Re: [PATCH v2 10/11] HID: introduce Scan Time

[Benjamin Tissoires]

On Mon, Oct 29, 2012 at 11:43 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Win 8 digitizer devices provides the actual scan time computed by the
>> hardware itself. The value is global to the frame and is not specific
>> to the multitouch protocol (though only touch, not pen, should use it
>> according to the specification).
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  Documentation/input/event-codes.txt |  7 +++++++
>>  drivers/hid/hid-input.c             |  4 ++++
>>  drivers/hid/hid-multitouch.c        | 11 +++++++++--
>>  include/linux/hid.h                 |  1 +
>>  include/linux/input.h               |  1 +
>>  5 files changed, 22 insertions(+), 2 deletions(-)
>
> This is a nice feature, useful in many other contexts. As such, I
> think it should be defined in the context of the input subsystem, with
> a more specific definition added to the documentation. For instance,
> is 100us suitable? When should it start at zero, at BTN_TOUCH? Or
> should it perhaps wrap around on unsigned integer instead? Or display
> the difference from the last event?

Well, the thing is that I didn't wanted to copy/paste win 8 definition
for ScanTime. So I put it with my words and not in a very
understandable way :)
This allows me to forward the incoming events without having to do
anything on them...

- 100us: suitable, not sure, but it would be good to define a standard
unit for time too
- start at zero at BTN_TOUCH: no. This information allows us to do
much better false release detection. If we reset this counter, then we
will loose the time between the two touch/release. The spec says that
it is up to the device to reset it after a period of time (not
defined, but can be one second for instance). Last, BTN_TOUCH is not
reliable for hovering devices because we will still get finger
information without BTN_TOUCH.
- difference from the last event: not sure how it is implemented in
windows, but I'm not sure it's a good way of doing if the gesture
engine needs the time from the beginning of the touch...

Anyway, I would be happy to have other comments/proposals for this event code.

>
>> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
>> index 53305bd..8f8c908 100644
>> --- a/Documentation/input/event-codes.txt
>> +++ b/Documentation/input/event-codes.txt
>> @@ -174,6 +174,13 @@ A few EV_ABS codes have special meanings:
>>      the input device may be used freely in three dimensions, consider ABS_Z
>>      instead.
>>
>> +* ABS_SCAN_TIME:
>> +  - Used when the device provides a timestamp for each frame. The unit must be
>> +    100us, and may be reset when the device don't send any events for a
>> +    period of time. The values increment at each frame and thus, it can roll
>> +    back to 0 when reach logical_max. If the device does not provide this
>> +    information, the driver must not provide it to the user space.
>> +
>>  * ABS_MT_<name>:
>>    - Used to describe multitouch input events. Please see
>>      multi-touch-protocol.txt for details.
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 16cc89a..5fe7bd3 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -675,6 +675,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>>                       map_key_clear(BTN_STYLUS2);
>>                       break;
>>
>> +             case 0x56: /* Scan Time */
>> +                     map_abs(ABS_SCAN_TIME);
>> +                     break;
>> +
>
> Is it not enough to map it in the case below? Or you mean this is
> picked up by hid core?
>

in hidinput_configure_usage, it's enough to just map it.

In hid-multitouch, We also just need to map it, and it will be handled
by hid-core in the .event callback.

Cheers,
Benjamin

>>               default:  goto unknown;
>>               }
>>               break;
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index c0ab1c6..21a120b 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -447,12 +447,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                       mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>> +             case HID_DG_SCANTIME:
>> +                     hid_map_usage(hi, usage, bit, max,
>> +                             EV_ABS, ABS_SCAN_TIME);
>> +                     set_abs(hi->input, ABS_SCAN_TIME, field, 0);
>> +                     td->last_field_index = field->index;
>> +                     return 1;
>>               case HID_DG_CONTACTCOUNT:
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_CONTACTMAX:
>> -                     /* we don't set td->last_slot_field as contactcount and
>> -                      * contact max are global to the report */
>> +                     /* we don't set td->last_slot_field as scan time,
>> +                      * contactcount and contact max are global to the
>> +                      * report */
>>                       td->last_field_index = field->index;
>>                       return -1;
>>               case HID_DG_TOUCH:
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 6216529..99a6418 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -279,6 +279,7 @@ struct hid_item {
>>  #define HID_DG_DEVICEINDEX   0x000d0053
>>  #define HID_DG_CONTACTCOUNT  0x000d0054
>>  #define HID_DG_CONTACTMAX    0x000d0055
>> +#define HID_DG_SCANTIME              0x000d0056
>>
>>  /*
>>   * HID report types --- Ouch! HID spec says 1 2 3!
>> diff --git a/include/linux/input.h b/include/linux/input.h
>> index ba48743..73c3a96 100644
>> --- a/include/linux/input.h
>> +++ b/include/linux/input.h
>> @@ -796,6 +796,7 @@ struct input_keymap_entry {
>>  #define ABS_TILT_X           0x1a
>>  #define ABS_TILT_Y           0x1b
>>  #define ABS_TOOL_WIDTH               0x1c
>> +#define ABS_SCAN_TIME                0x1d
>>
>>  #define ABS_VOLUME           0x20
>>
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Benjamin Tissoires]

Hi Henrik,

On Mon, Oct 29, 2012 at 11:57 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> This patch factorizes the hid set_feature command by using
>> hid_device->hid_output_raw_report instead of direclty relying on
>> usbhid. This makes the driver usb independant.
>>
>> However I still can't remove the 2 usb related headers because the
>> function mt_resume has a specific patch for usb devices.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 26 deletions(-)
>
> In my drawer, I have a patchset that aims to remove all usbhid
> dependence, from all the drivers. Perhaps the attached patch is
> something to consider here?

yep, removing usbhid dependencies is a good thing.
See my review below :)

>
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 21a120b..33038c5 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -670,47 +670,58 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>       return 1;
>>  }
>>
>> -static void mt_set_input_mode(struct hid_device *hdev)
>> +static void mt_set_feature(struct hid_device *hdev, __s8 feature_id,
>> +     u8 value, size_t index)
>>  {
>> -     struct mt_device *td = hid_get_drvdata(hdev);
>>       struct hid_report *r;
>>       struct hid_report_enum *re;
>> +     u8 *data;
>> +     u8 max_value;
>> +     int len;
>> +
>> +     if (feature_id < 0 || !hdev->hid_output_raw_report)
>> +             return;
>> +
>> +     re = &hdev->report_enum[HID_FEATURE_REPORT];
>> +     r = re->report_id_hash[feature_id];
>> +     if (!r)
>> +             return;
>> +
>> +     len = ((r->size - 1) >> 3) + 1 + (r->id > 0);
>> +     max_value = r->field[0]->logical_maximum;
>> +     value = min(value, max_value);
>>
>> -     if (td->inputmode < 0)
>> +     if (r->field[0]->value[index] == value || len < 2 || index + 1 >= len)
>>               return;
>>
>> -     re = &(hdev->report_enum[HID_FEATURE_REPORT]);
>> -     r = re->report_id_hash[td->inputmode];
>> -     if (r) {
>> -             r->field[0]->value[td->inputmode_index] = 0x02;
>> -             usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> +     data = kzalloc(len, GFP_ATOMIC);
>> +     if (!data) {
>> +             hid_warn(hdev, "output queueing failed\n");
>> +             return;
>>       }
>> +
>> +     data[0] = r->id;
>> +     data[index + 1] = value;
>> +     hdev->hid_output_raw_report(hdev, data, len, HID_FEATURE_REPORT);
>> +     kfree(data);
>>  }
>>
>> -static void mt_set_maxcontacts(struct hid_device *hdev)
>> +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;
>> -     int fieldmax, max;
>>
>> -     if (td->maxcontact_report_id < 0)
>> -             return;
>> +     mt_set_feature(hdev, td->inputmode, 0x02, td->inputmode_index);
>> +}
>>
>> -     if (!td->mtclass.maxcontacts)
>> +static void mt_set_maxcontacts(struct hid_device *hdev)
>> +{
>> +     struct mt_device *td = hid_get_drvdata(hdev);
>> +     int max = td->mtclass.maxcontacts;
>> +
>> +     if (!max)
>>               return;
>>
>> -     re = &hdev->report_enum[HID_FEATURE_REPORT];
>> -     r = re->report_id_hash[td->maxcontact_report_id];
>> -     if (r) {
>> -             max = td->mtclass.maxcontacts;
>> -             fieldmax = r->field[0]->logical_maximum;
>> -             max = min(fieldmax, max);
>> -             if (r->field[0]->value[0] != max) {
>> -                     r->field[0]->value[0] = max;
>> -                     usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> -             }
>> -     }
>> +     mt_set_feature(hdev, td->maxcontact_report_id, max, 0);
>>  }
>>
>>  static void mt_post_parse_default_settings(struct mt_device *td)
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik
>
> ---
>
> From 5d9a791e0a9e41bcea0fcb286e2849b403675f37 Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Mon, 2 Jul 2012 20:38:21 +0200
> Subject: [PATCH 3/7] hid: extend interface with report requests
>
> ---
>  drivers/hid/usbhid/hid-core.c | 14 ++++++++++++++
>  include/linux/hid.h           | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index de1f9ac..3c618da 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1251,6 +1251,18 @@ static int usbhid_power(struct hid_device *hid, int lvl)
>         return r;
>  }
>
> +static void usbhid_send(struct hid_device *hid, struct hid_report *rep, int req)
> +{
> +       switch (req) {
> +       case HID_REQ_GET_REPORT:
> +               usbhid_submit_report(hid, rep, USB_DIR_IN);
> +               break;
> +       case HID_REQ_SET_REPORT:
> +               usbhid_submit_report(hid, rep, USB_DIR_OUT);
> +               break;
> +       }
> +}
> +
>  static struct hid_ll_driver usb_hid_driver = {
>         .parse = usbhid_parse,
>         .start = usbhid_start,
> @@ -1259,6 +1271,8 @@ static struct hid_ll_driver usb_hid_driver = {
>         .close = usbhid_close,
>         .power = usbhid_power,
>         .hidinput_input_event = usb_hidinput_input_event,
> +       .send = usbhid_send,

the name here is a little bit misleading. You are using "send" to also
"get" reports...
Maybe "hid_request" is a better name. This will allows us to add the
missing function:
Get_Descriptor, Set_Descriptor, Get_Report Request, Set_Report
Request, Get_Idle, Set_Idle, Get_Protocol, Set_Protocol and maybe
others - even WAIT for instance :)

> +       .wait = usbhid_wait_io,

is it really necessary to wait for IO? (I think it will not be one
line for hid over i2c...).

Cheers,
Benjamin

>  };
>
>  static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 241eb40..5e169c1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -686,6 +686,8 @@ struct hid_driver {
>   * @hidinput_input_event: event input event (e.g. ff or leds)
>   * @parse: this method is called only once to parse the device data,
>   *        shouldn't allocate anything to not leak memory
> + * @send: send report request to device (e.g. feature report)
> + * @wait: wait for buffered io to complete (send/recv reports)
>   */
>  struct hid_ll_driver {
>         int (*start)(struct hid_device *hdev);
> @@ -700,6 +702,11 @@ struct hid_ll_driver {
>                         unsigned int code, int value);
>
>         int (*parse)(struct hid_device *hdev);
> +
> +       void (*send)(struct hid_device *hdev,
> +                    struct hid_report *report, int req);
> +       int (*wait)(struct hid_device *hdev);
> +
>  };
>
>  #define        PM_HINT_FULLON  1<<5
> @@ -892,6 +899,32 @@ static inline int hid_hw_power(struct hid_device *hdev, int level)
>         return hdev->ll_driver->power ? hdev->ll_driver->power(hdev, level) : 0;
>  }
>
> +
> +/**
> + * hid_hw_send - send report request to device
> + *
> + * @hdev: hid device
> + * @report: report to send
> + * @req: hid request type
> + */
> +static inline void hid_hw_send(struct hid_device *hdev,
> +                              struct hid_report *report, int req)
> +{
> +       if (hdev->ll_driver->send)
> +               hdev->ll_driver->send(hdev, report, req);
> +}
> +
> +/**
> + * hid_hw_wait - wait for buffered io to complete
> + *
> + * @hdev: hid device
> + */
> +static inline void hid_hw_wait(struct hid_device *hdev)
> +{
> +       if (hdev->ll_driver->wait)
> +               hdev->ll_driver->wait(hdev);
> +}
> +
>  int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>                 int interrupt);
>
> --
> 1.7.11.1
>

Re: [PATCH v2 06/11] HID: hid-multitouch: support T and C for win8 devices

[Henrik Rydberg]

> >> +                             usage_index) {
> >> +                             hid_map_usage(hi, usage, bit, max,
> >> +                                     EV_ABS, ABS_MT_TOOL_X);
> >> +                             set_abs(hi->input, ABS_MT_TOOL_X, field,
> >> +                                     cls->sn_move);
> >> +                     } else {
> >> +                             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);
> >> +                             set_abs(hi->input, ABS_MT_POSITION_X, field,
> >> +                                     cls->sn_move);
> >> +                     }
> >> +
> >
> > Do we really want to do the latter several times, even if the device is not a win8 one?
> 
> I don't get your point here. The only difference with the previous
> release is that it will treat differently the first in the array than
> the others. For non win8 devices, there is no changes in the behavior.
> Could you elaborate a little bit more, please?

I was wondering what we want to do about multiple reports in the
general casel. Not that important though, the patch will probably look
fine in your next version.

Thanks,
Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Henrik Rydberg]

Hi Benjamin,

> On Mon, Oct 29, 2012 at 11:19 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> > On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote:
> >> Win 8 specification is much more precise than the Win 7 one.
> >> Moreover devices that need to take certification must be submitted
> >> to Microsoft.
> >>
> >> The result is a better protocol support and we can rely on that to
> >> skip all the messy tests we used to do.
> >>
> >> The protocol specify the fact that each valid touch must be reported
> >> within a frame, and that the release touch coordinate must be the
> >> same than the last known touch.
> >> So we can use the always_valid quirk and dismiss reports when we
> >> touch coordiante do not follow this rule.
> >>
> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >> ---
> >>  drivers/hid/hid-multitouch.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > Why do we need this patch?
> 
> This patch prevents a corner case where the device use contactID 0 for
> it's first reported touch.
> Once you got the invalid touches, most of the time, contactID will be
> 0, x, y, and other fields too. So this ensures to avoid conflict
> between valid values and garbage values. The problem lies in the fact
> that we don't have the whole overview of the frame (with the contact
> count) to decide which contacts are good and which are not.

I am sorry, but your explanation did not make me any wiser. :-) Are
you saying that touch state changes and touch property changes are
mutually exclusive? For all win8 devices, or just for the serial
protocol ones? For what devices is the current implementation a
problem?

I am asking because this looks more like a device quirk than anything
else.

Thanks,
Henrik

Re: [PATCH v2 10/11] HID: introduce Scan Time

[Henrik Rydberg]

Hi Benjamin,

> > This is a nice feature, useful in many other contexts. As such, I
> > think it should be defined in the context of the input subsystem, with
> > a more specific definition added to the documentation. For instance,
> > is 100us suitable? When should it start at zero, at BTN_TOUCH? Or
> > should it perhaps wrap around on unsigned integer instead? Or display
> > the difference from the last event?
> 
> Well, the thing is that I didn't wanted to copy/paste win 8 definition
> for ScanTime. So I put it with my words and not in a very
> understandable way :)
> This allows me to forward the incoming events without having to do
> anything on them...
> 
> - 100us: suitable, not sure, but it would be good to define a standard
> unit for time too

Units of 100us might be fine, but maybe 64 or 128 or even 1 might be
better suited for implementations.

> - start at zero at BTN_TOUCH: no. This information allows us to do
> much better false release detection. If we reset this counter, then we
> will loose the time between the two touch/release.

Good point.

> The spec says that
> it is up to the device to reset it after a period of time (not
> defined, but can be one second for instance). Last, BTN_TOUCH is not
> reliable for hovering devices because we will still get finger
> information without BTN_TOUCH.

Agreed.

> - difference from the last event: not sure how it is implemented in
> windows, but I'm not sure it's a good way of doing if the gesture
> engine needs the time from the beginning of the touch...

Probably more complicated than it needs to be, yes.

> Anyway, I would be happy to have other comments/proposals for this event code.

Here is my proposal: Let ABS_SCAN_TIME be the number of microseconds
since the last reset. Let it be coded as an uint32 value, which is
allowed to wrap around with no special consequence. It is assumed that
the time difference between two consecutive events is reliable on a
reasonable time scale (hours).  A reset to zero can happen, in which
case the time since the last event is unknown. A definition like
time_valid = (time || (time - prev_time) < MAX_SCAN_INTERVAL) ought to
work for most cases.

> >> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
> >> index 53305bd..8f8c908 100644
> >> --- a/Documentation/input/event-codes.txt
> >> +++ b/Documentation/input/event-codes.txt
> >> @@ -174,6 +174,13 @@ A few EV_ABS codes have special meanings:
> >>      the input device may be used freely in three dimensions, consider ABS_Z
> >>      instead.
> >>
> >> +* ABS_SCAN_TIME:
> >> +  - Used when the device provides a timestamp for each frame. The unit must be
> >> +    100us, and may be reset when the device don't send any events for a
> >> +    period of time. The values increment at each frame and thus, it can roll
> >> +    back to 0 when reach logical_max. If the device does not provide this
> >> +    information, the driver must not provide it to the user space.
> >> +
> >>  * ABS_MT_<name>:
> >>    - Used to describe multitouch input events. Please see
> >>      multi-touch-protocol.txt for details.
> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> >> index 16cc89a..5fe7bd3 100644
> >> --- a/drivers/hid/hid-input.c
> >> +++ b/drivers/hid/hid-input.c
> >> @@ -675,6 +675,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
> >>                       map_key_clear(BTN_STYLUS2);
> >>                       break;
> >>
> >> +             case 0x56: /* Scan Time */
> >> +                     map_abs(ABS_SCAN_TIME);
> >> +                     break;
> >> +
> >
> > Is it not enough to map it in the case below? Or you mean this is
> > picked up by hid core?
> >
> 
> in hidinput_configure_usage, it's enough to just map it.
> 
> In hid-multitouch, We also just need to map it, and it will be handled
> by hid-core in the .event callback.

I think you should intercept it, convert it, and send it out with the touch frame.

Thanks,
Henrik

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Henrik Rydberg]

> > In my drawer, I have a patchset that aims to remove all usbhid
> > dependence, from all the drivers. Perhaps the attached patch is
> > something to consider here?
> 
> yep, removing usbhid dependencies is a good thing.
> See my review below :)
> 
> > From 5d9a791e0a9e41bcea0fcb286e2849b403675f37 Mon Sep 17 00:00:00 2001
> > From: Henrik Rydberg <rydberg@euromail.se>
> > Date: Mon, 2 Jul 2012 20:38:21 +0200
> > Subject: [PATCH 3/7] hid: extend interface with report requests
> >
> > ---
> >  drivers/hid/usbhid/hid-core.c | 14 ++++++++++++++
> >  include/linux/hid.h           | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index de1f9ac..3c618da 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -1251,6 +1251,18 @@ static int usbhid_power(struct hid_device *hid, int lvl)
> >         return r;
> >  }
> >
> > +static void usbhid_send(struct hid_device *hid, struct hid_report *rep, int req)
> > +{
> > +       switch (req) {
> > +       case HID_REQ_GET_REPORT:
> > +               usbhid_submit_report(hid, rep, USB_DIR_IN);
> > +               break;
> > +       case HID_REQ_SET_REPORT:
> > +               usbhid_submit_report(hid, rep, USB_DIR_OUT);
> > +               break;
> > +       }
> > +}
> > +
> >  static struct hid_ll_driver usb_hid_driver = {
> >         .parse = usbhid_parse,
> >         .start = usbhid_start,
> > @@ -1259,6 +1271,8 @@ static struct hid_ll_driver usb_hid_driver = {
> >         .close = usbhid_close,
> >         .power = usbhid_power,
> >         .hidinput_input_event = usb_hidinput_input_event,
> > +       .send = usbhid_send,
> 
> the name here is a little bit misleading. You are using "send" to also
> "get" reports...
> Maybe "hid_request" is a better name. This will allows us to add the
> missing function:
> Get_Descriptor, Set_Descriptor, Get_Report Request, Set_Report
> Request, Get_Idle, Set_Idle, Get_Protocol, Set_Protocol and maybe
> others - even WAIT for instance :)

Sounds good, I'll ponder this a bit.

> 
> > +       .wait = usbhid_wait_io,
> 
> is it really necessary to wait for IO? (I think it will not be one
> line for hid over i2c...).

We can certainly skip it for the scope of your patchset, but last time
I checked, it was needed in quite a few places.

Thanks,
Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

Hi Henrik,

On Wed, Oct 31, 2012 at 7:53 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> On Mon, Oct 29, 2012 at 11:19 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote:
>> >> Win 8 specification is much more precise than the Win 7 one.
>> >> Moreover devices that need to take certification must be submitted
>> >> to Microsoft.
>> >>
>> >> The result is a better protocol support and we can rely on that to
>> >> skip all the messy tests we used to do.
>> >>
>> >> The protocol specify the fact that each valid touch must be reported
>> >> within a frame, and that the release touch coordinate must be the
>> >> same than the last known touch.
>> >> So we can use the always_valid quirk and dismiss reports when we
>> >> touch coordiante do not follow this rule.
>> >>
>> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> >> ---
>> >>  drivers/hid/hid-multitouch.c | 16 ++++++++++++++--
>> >>  1 file changed, 14 insertions(+), 2 deletions(-)
>> >
>> > Why do we need this patch?
>>
>> This patch prevents a corner case where the device use contactID 0 for
>> it's first reported touch.
>> Once you got the invalid touches, most of the time, contactID will be
>> 0, x, y, and other fields too. So this ensures to avoid conflict
>> between valid values and garbage values. The problem lies in the fact
>> that we don't have the whole overview of the frame (with the contact
>> count) to decide which contacts are good and which are not.
>
> I am sorry, but your explanation did not make me any wiser. :-) Are

Sorry, for that. Let's try with other words.

> you saying that touch state changes and touch property changes are
> mutually exclusive? For all win8 devices, or just for the serial
> protocol ones? For what devices is the current implementation a
> problem?

The goal of this patch is to implement in a reliable way Win 8
multitouch protocol (to avoid quirking many devices). Thanks to the
precision they made in the specification, I think it is feasible:
they add the dynamic part that were missing in Win 7 spec:
"""
When sending data in hybrid or parallel mode, a contact that is
delivered in one report must be delivered in all subsequent reports
until it is lifted off the screen. If time is needed to adequate
determine if the contact was lifted off the surface, the device must
report the last known position of the contact and then deliver the
“UP” state of the contact in a subsequent report. Devices should not
send a report without the information for that contact while trying to
determine its current state.
"""

Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the
device has to send the touch until it is lifted and out of range, and
the device must send the 'up' state).

The problem lies that some devices may reuse contact id 0 within the
frame for the end of the report (my Win8 device doesn't has this kind
of problem):

With the following hid usages:
I -> contact Id
T -> tip switch
X, Y -> X, Y
S -> scan time
C -> contact count

a friendly device would report:

I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01

*but*, I've already seen win 7 devices, that do send:

I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01

The difference lies in the first bit, contact id is 0.

So, the quirk always valid is not sufficient because the second touch
in the frame will override the values of the first (the valid one).

As Microsoft says that "the device must report the last known position
of the contact and then deliver the “UP” state of the contact", so we
can safely discard the second touch because X and Y do not match the
current state of the valid touch.

Then, as exposed in the "How to Design and Test Multitouch Hardware
Solutions for Windows 8" document here:
http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx
when the device attempt the certification, if the "up" is not valid,
the error "Last move location different" raises, which, I hope will
prevent the device to get the certification.

I hope it is clearer now.

>
> I am asking because this looks more like a device quirk than anything
> else.

and yes, it looks like a quirk, we could make the "Last move location
different" presented like a quirk, but it is only followed by win 8
devices (or it is by luck).

Cheers,
Benjamin


>
> Thanks,
> Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

Hi Henrik,

On Wed, Oct 31, 2012 at 7:53 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> On Mon, Oct 29, 2012 at 11:19 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote:
>> >> Win 8 specification is much more precise than the Win 7 one.
>> >> Moreover devices that need to take certification must be submitted
>> >> to Microsoft.
>> >>
>> >> The result is a better protocol support and we can rely on that to
>> >> skip all the messy tests we used to do.
>> >>
>> >> The protocol specify the fact that each valid touch must be reported
>> >> within a frame, and that the release touch coordinate must be the
>> >> same than the last known touch.
>> >> So we can use the always_valid quirk and dismiss reports when we
>> >> touch coordiante do not follow this rule.
>> >>
>> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> >> ---
>> >>  drivers/hid/hid-multitouch.c | 16 ++++++++++++++--
>> >>  1 file changed, 14 insertions(+), 2 deletions(-)
>> >
>> > Why do we need this patch?
>>
>> This patch prevents a corner case where the device use contactID 0 for
>> it's first reported touch.
>> Once you got the invalid touches, most of the time, contactID will be
>> 0, x, y, and other fields too. So this ensures to avoid conflict
>> between valid values and garbage values. The problem lies in the fact
>> that we don't have the whole overview of the frame (with the contact
>> count) to decide which contacts are good and which are not.
>
> I am sorry, but your explanation did not make me any wiser. :-) Are

Sorry, for that. Let's try with other words.

> you saying that touch state changes and touch property changes are
> mutually exclusive? For all win8 devices, or just for the serial
> protocol ones? For what devices is the current implementation a
> problem?

The goal of this patch is to implement in a reliable way Win 8
multitouch protocol (to avoid quirking many devices). Thanks to the
precision they made in the specification, I think it is feasible:
they add the dynamic part that were missing in Win 7 spec:
"""
When sending data in hybrid or parallel mode, a contact that is
delivered in one report must be delivered in all subsequent reports
until it is lifted off the screen. If time is needed to adequate
determine if the contact was lifted off the surface, the device must
report the last known position of the contact and then deliver the
“UP” state of the contact in a subsequent report. Devices should not
send a report without the information for that contact while trying to
determine its current state.
"""

Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the
device has to send the touch until it is lifted and out of range, and
the device must send the 'up' state).

The problem lies that some devices may reuse contact id 0 within the
frame for the end of the report (my Win8 device doesn't has this kind
of problem):

With the following hid usages:
I -> contact Id
T -> tip switch
X, Y -> X, Y
S -> scan time
C -> contact count

a friendly device would report:

I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01

*but*, I've already seen win 7 devices, that do send:

I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01

The difference lies in the first bit, contact id is 0.

So, the quirk always valid is not sufficient because the second touch
in the frame will override the values of the first (the valid one).

As Microsoft says that "the device must report the last known position
of the contact and then deliver the “UP” state of the contact", so we
can safely discard the second touch because X and Y do not match the
current state of the valid touch.

Then, as exposed in the "How to Design and Test Multitouch Hardware
Solutions for Windows 8" document here:
http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx
when the device attempt the certification, if the "up" is not valid,
the error "Last move location different" raises, which, I hope will
prevent the device to get the certification.

I hope it is clearer now.

>
> I am asking because this looks more like a device quirk than anything
> else.

and yes, it looks like a quirk, we could make the "Last move location
different" presented like a quirk, but it is only followed by win 8
devices (or it is by luck).

Cheers,
Benjamin


>
> Thanks,
> Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 10/11] HID: introduce Scan Time

[Benjamin Tissoires]

On Wed, Oct 31, 2012 at 8:16 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> > This is a nice feature, useful in many other contexts. As such, I
>> > think it should be defined in the context of the input subsystem, with
>> > a more specific definition added to the documentation. For instance,
>> > is 100us suitable? When should it start at zero, at BTN_TOUCH? Or
>> > should it perhaps wrap around on unsigned integer instead? Or display
>> > the difference from the last event?
>>
>> Well, the thing is that I didn't wanted to copy/paste win 8 definition
>> for ScanTime. So I put it with my words and not in a very
>> understandable way :)
>> This allows me to forward the incoming events without having to do
>> anything on them...
>>
>> - 100us: suitable, not sure, but it would be good to define a standard
>> unit for time too
>
> Units of 100us might be fine, but maybe 64 or 128 or even 1 might be
> better suited for implementations.
>
>> - start at zero at BTN_TOUCH: no. This information allows us to do
>> much better false release detection. If we reset this counter, then we
>> will loose the time between the two touch/release.
>
> Good point.
>
>> The spec says that
>> it is up to the device to reset it after a period of time (not
>> defined, but can be one second for instance). Last, BTN_TOUCH is not
>> reliable for hovering devices because we will still get finger
>> information without BTN_TOUCH.
>
> Agreed.
>
>> - difference from the last event: not sure how it is implemented in
>> windows, but I'm not sure it's a good way of doing if the gesture
>> engine needs the time from the beginning of the touch...
>
> Probably more complicated than it needs to be, yes.
>
>> Anyway, I would be happy to have other comments/proposals for this event code.
>
> Here is my proposal: Let ABS_SCAN_TIME be the number of microseconds
> since the last reset. Let it be coded as an uint32 value, which is
> allowed to wrap around with no special consequence. It is assumed that
> the time difference between two consecutive events is reliable on a
> reasonable time scale (hours).  A reset to zero can happen, in which
> case the time since the last event is unknown. A definition like
> time_valid = (time || (time - prev_time) < MAX_SCAN_INTERVAL) ought to
> work for most cases.

all right, I'm fine with it.

>
>> >> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
>> >> index 53305bd..8f8c908 100644
>> >> --- a/Documentation/input/event-codes.txt
>> >> +++ b/Documentation/input/event-codes.txt
>> >> @@ -174,6 +174,13 @@ A few EV_ABS codes have special meanings:
>> >>      the input device may be used freely in three dimensions, consider ABS_Z
>> >>      instead.
>> >>
>> >> +* ABS_SCAN_TIME:
>> >> +  - Used when the device provides a timestamp for each frame. The unit must be
>> >> +    100us, and may be reset when the device don't send any events for a
>> >> +    period of time. The values increment at each frame and thus, it can roll
>> >> +    back to 0 when reach logical_max. If the device does not provide this
>> >> +    information, the driver must not provide it to the user space.
>> >> +
>> >>  * ABS_MT_<name>:
>> >>    - Used to describe multitouch input events. Please see
>> >>      multi-touch-protocol.txt for details.
>> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> >> index 16cc89a..5fe7bd3 100644
>> >> --- a/drivers/hid/hid-input.c
>> >> +++ b/drivers/hid/hid-input.c
>> >> @@ -675,6 +675,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>> >>                       map_key_clear(BTN_STYLUS2);
>> >>                       break;
>> >>
>> >> +             case 0x56: /* Scan Time */
>> >> +                     map_abs(ABS_SCAN_TIME);
>> >> +                     break;
>> >> +
>> >
>> > Is it not enough to map it in the case below? Or you mean this is
>> > picked up by hid core?
>> >
>>
>> in hidinput_configure_usage, it's enough to just map it.
>>
>> In hid-multitouch, We also just need to map it, and it will be handled
>> by hid-core in the .event callback.
>
> I think you should intercept it, convert it, and send it out with the touch frame.

With the definition inspired from Win 8, I didn't need to convert it,
thus the hid-core could handle it.
Now, it's clear that if we want to transform it, it needs to be intercepted.

Cheers,
Benjamin

>
> Thanks,
> Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Henrik Rydberg]

Hi Benjamin,

> The goal of this patch is to implement in a reliable way Win 8
> multitouch protocol (to avoid quirking many devices). Thanks to the
> precision they made in the specification, I think it is feasible:
> they add the dynamic part that were missing in Win 7 spec:
> """
> When sending data in hybrid or parallel mode, a contact that is
> delivered in one report must be delivered in all subsequent reports
> until it is lifted off the screen. If time is needed to adequate
> determine if the contact was lifted off the surface, the device must
> report the last known position of the contact and then deliver the
> “UP” state of the contact in a subsequent report. Devices should not
> send a report without the information for that contact while trying to
> determine its current state.
> """

The text seems to say that devices are not required to send touch
state information in a separate frame, but if the device needs time to
determine the touch state, the touch properties should stay the same
during that time.

> Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the
> device has to send the touch until it is lifted and out of range, and
> the device must send the 'up' state).

One could simply add another quirk which fits the win8 case exactly
instead. No need to change the existing one.

> The problem lies that some devices may reuse contact id 0 within the
> frame for the end of the report (my Win8 device doesn't has this kind
> of problem):
> 
> With the following hid usages:
> I -> contact Id
> T -> tip switch
> X, Y -> X, Y
> S -> scan time
> C -> contact count
> 
> a friendly device would report:
> 
> I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
> I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
> I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
> I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
> I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01
> 
> *but*, I've already seen win 7 devices, that do send:
> 
> I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
> I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
> I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
> I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
> I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01

I see, more brain-damaged usage. :-) Still, there seems to be a
simpler way to distinguish this case: if there are more than one touch
with the same id in the frame, use the one with T=1.

> The difference lies in the first bit, contact id is 0.
> 
> So, the quirk always valid is not sufficient because the second touch
> in the frame will override the values of the first (the valid one).
> 
> As Microsoft says that "the device must report the last known position
> of the contact and then deliver the “UP” state of the contact", so we
> can safely discard the second touch because X and Y do not match the
> current state of the valid touch.
> 
> Then, as exposed in the "How to Design and Test Multitouch Hardware
> Solutions for Windows 8" document here:
> http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx
> when the device attempt the certification, if the "up" is not valid,
> the error "Last move location different" raises, which, I hope will
> prevent the device to get the certification.

I think it would be too fragile to rely on this assumption. Hopefully
the suggestion above will work equally well.

Thanks,
Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

On Sun, Nov 4, 2012 at 9:54 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> The goal of this patch is to implement in a reliable way Win 8
>> multitouch protocol (to avoid quirking many devices). Thanks to the
>> precision they made in the specification, I think it is feasible:
>> they add the dynamic part that were missing in Win 7 spec:
>> """
>> When sending data in hybrid or parallel mode, a contact that is
>> delivered in one report must be delivered in all subsequent reports
>> until it is lifted off the screen. If time is needed to adequate
>> determine if the contact was lifted off the surface, the device must
>> report the last known position of the contact and then deliver the
>> “UP” state of the contact in a subsequent report. Devices should not
>> send a report without the information for that contact while trying to
>> determine its current state.
>> """
>
> The text seems to say that devices are not required to send touch
> state information in a separate frame, but if the device needs time to
> determine the touch state, the touch properties should stay the same
> during that time.

Yes and no:
- yes, if the device needs time to determine the "up" state, it should
send at every frame the last known properties.
- no, I never said that the information should be split in separate
frames: I think you are confused by the serial protocol. I'm talking
here about the parallel or the hybrid one.

>
>> Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the
>> device has to send the touch until it is lifted and out of range, and
>> the device must send the 'up' state).
>
> One could simply add another quirk which fits the win8 case exactly
> instead. No need to change the existing one.

I never changed the existing one. The quirk ALWAYS_VALID means that
whatever the device sends, the information are valid. The fact is that
the serial protocol can be handled with this quirk (otherwise, we
would have called this quirk SERIAL, and not ALWAYS_VALID). So the Win
8 case doesn't need a new quirk, the ALWAYS_VALID fits.

>
>> The problem lies that some devices may reuse contact id 0 within the
>> frame for the end of the report (my Win8 device doesn't has this kind
>> of problem):
>>
>> With the following hid usages:
>> I -> contact Id
>> T -> tip switch
>> X, Y -> X, Y
>> S -> scan time
>> C -> contact count
>>
>> a friendly device would report:
>>
>> I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
>> I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
>> I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
>> I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
>> I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01
>>
>> *but*, I've already seen win 7 devices, that do send:
>>
>> I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
>> I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
>> I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
>> I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
>> I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01
>
> I see, more brain-damaged usage. :-) Still, there seems to be a
> simpler way to distinguish this case: if there are more than one touch
> with the same id in the frame, use the one with T=1.

sigh... the problem in your sentence is there: "with the same id in the frame".
This forces me to introduce the function input_mt_is_used in mt.h...

>
>> The difference lies in the first bit, contact id is 0.
>>
>> So, the quirk always valid is not sufficient because the second touch
>> in the frame will override the values of the first (the valid one).
>>
>> As Microsoft says that "the device must report the last known position
>> of the contact and then deliver the “UP” state of the contact", so we
>> can safely discard the second touch because X and Y do not match the
>> current state of the valid touch.
>>
>> Then, as exposed in the "How to Design and Test Multitouch Hardware
>> Solutions for Windows 8" document here:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx
>> when the device attempt the certification, if the "up" is not valid,
>> the error "Last move location different" raises, which, I hope will
>> prevent the device to get the certification.
>
> I think it would be too fragile to rely on this assumption. Hopefully
> the suggestion above will work equally well.

I'm not sure it is fragile, because otherwise that means that the
certification is worthless.
But anyway, your solution is valid too, so I'll implement it.

Cheers,
Benjamin

>
> Thanks,
> Henrik

Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

On Sun, Nov 4, 2012 at 9:54 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> The goal of this patch is to implement in a reliable way Win 8
>> multitouch protocol (to avoid quirking many devices). Thanks to the
>> precision they made in the specification, I think it is feasible:
>> they add the dynamic part that were missing in Win 7 spec:
>> """
>> When sending data in hybrid or parallel mode, a contact that is
>> delivered in one report must be delivered in all subsequent reports
>> until it is lifted off the screen. If time is needed to adequate
>> determine if the contact was lifted off the surface, the device must
>> report the last known position of the contact and then deliver the
>> “UP” state of the contact in a subsequent report. Devices should not
>> send a report without the information for that contact while trying to
>> determine its current state.
>> """
>
> The text seems to say that devices are not required to send touch
> state information in a separate frame, but if the device needs time to
> determine the touch state, the touch properties should stay the same
> during that time.

Yes and no:
- yes, if the device needs time to determine the "up" state, it should
send at every frame the last known properties.
- no, I never said that the information should be split in separate
frames: I think you are confused by the serial protocol. I'm talking
here about the parallel or the hybrid one.

>
>> Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the
>> device has to send the touch until it is lifted and out of range, and
>> the device must send the 'up' state).
>
> One could simply add another quirk which fits the win8 case exactly
> instead. No need to change the existing one.

I never changed the existing one. The quirk ALWAYS_VALID means that
whatever the device sends, the information are valid. The fact is that
the serial protocol can be handled with this quirk (otherwise, we
would have called this quirk SERIAL, and not ALWAYS_VALID). So the Win
8 case doesn't need a new quirk, the ALWAYS_VALID fits.

>
>> The problem lies that some devices may reuse contact id 0 within the
>> frame for the end of the report (my Win8 device doesn't has this kind
>> of problem):
>>
>> With the following hid usages:
>> I -> contact Id
>> T -> tip switch
>> X, Y -> X, Y
>> S -> scan time
>> C -> contact count
>>
>> a friendly device would report:
>>
>> I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
>> I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
>> I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
>> I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
>> I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01
>>
>> *but*, I've already seen win 7 devices, that do send:
>>
>> I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01
>> I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01
>> I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01
>> I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01
>> I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01
>
> I see, more brain-damaged usage. :-) Still, there seems to be a
> simpler way to distinguish this case: if there are more than one touch
> with the same id in the frame, use the one with T=1.

sigh... the problem in your sentence is there: "with the same id in the frame".
This forces me to introduce the function input_mt_is_used in mt.h...

>
>> The difference lies in the first bit, contact id is 0.
>>
>> So, the quirk always valid is not sufficient because the second touch
>> in the frame will override the values of the first (the valid one).
>>
>> As Microsoft says that "the device must report the last known position
>> of the contact and then deliver the “UP” state of the contact", so we
>> can safely discard the second touch because X and Y do not match the
>> current state of the valid touch.
>>
>> Then, as exposed in the "How to Design and Test Multitouch Hardware
>> Solutions for Windows 8" document here:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx
>> when the device attempt the certification, if the "up" is not valid,
>> the error "Last move location different" raises, which, I hope will
>> prevent the device to get the certification.
>
> I think it would be too fragile to rely on this assumption. Hopefully
> the suggestion above will work equally well.

I'm not sure it is fragile, because otherwise that means that the
certification is worthless.
But anyway, your solution is valid too, so I'll implement it.

Cheers,
Benjamin

>
> Thanks,
> Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Henrik Rydberg]

Hi Benjamin,

> >> This patch factorizes the hid set_feature command by using
> >> hid_device->hid_output_raw_report instead of direclty relying on
> >> usbhid. This makes the driver usb independant.
> >>
> >> However I still can't remove the 2 usb related headers because the
> >> function mt_resume has a specific patch for usb devices.
> >>
> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >> ---
> >>  drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
> >>  1 file changed, 37 insertions(+), 26 deletions(-)
> >
> > In my drawer, I have a patchset that aims to remove all usbhid
> > dependence, from all the drivers. Perhaps the attached patch is
> > something to consider here?
> 
> yep, removing usbhid dependencies is a good thing.
> See my review below :)

I have a tentative patch taking your comments into account, and it is
likely that we want to go that way. However, as to not hold up your
patchset, perhaps we could do without it for now.

Regarding the hardwired usbhid dependency, I think the solution is to
move that code to usbhid itself.

Thanks,
Henrik

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Benjamin Tissoires]

Hi Henrik,

On Mon, Nov 5, 2012 at 1:57 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> >> This patch factorizes the hid set_feature command by using
>> >> hid_device->hid_output_raw_report instead of direclty relying on
>> >> usbhid. This makes the driver usb independant.
>> >>
>> >> However I still can't remove the 2 usb related headers because the
>> >> function mt_resume has a specific patch for usb devices.
>> >>
>> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> >> ---
>> >>  drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
>> >>  1 file changed, 37 insertions(+), 26 deletions(-)
>> >
>> > In my drawer, I have a patchset that aims to remove all usbhid
>> > dependence, from all the drivers. Perhaps the attached patch is
>> > something to consider here?
>>
>> yep, removing usbhid dependencies is a good thing.
>> See my review below :)
>
> I have a tentative patch taking your comments into account, and it is
> likely that we want to go that way. However, as to not hold up your
> patchset, perhaps we could do without it for now.
>
> Regarding the hardwired usbhid dependency, I think the solution is to
> move that code to usbhid itself.
>
> Thanks,
> Henrik

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

[Benjamin Tissoires]

Hi Henrik,

grrr.... damn new gmail interface, I clicked on the wrong button.

sorry for the noise.

On Mon, Nov 5, 2012 at 2:28 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi Henrik,
>
> On Mon, Nov 5, 2012 at 1:57 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> Hi Benjamin,
>>
>>> >> This patch factorizes the hid set_feature command by using
>>> >> hid_device->hid_output_raw_report instead of direclty relying on
>>> >> usbhid. This makes the driver usb independant.
>>> >>
>>> >> However I still can't remove the 2 usb related headers because the
>>> >> function mt_resume has a specific patch for usb devices.
>>> >>
>>> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>>> >> ---
>>> >>  drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
>>> >>  1 file changed, 37 insertions(+), 26 deletions(-)
>>> >
>>> > In my drawer, I have a patchset that aims to remove all usbhid
>>> > dependence, from all the drivers. Perhaps the attached patch is
>>> > something to consider here?
>>>
>>> yep, removing usbhid dependencies is a good thing.
>>> See my review below :)
>>
>> I have a tentative patch taking your comments into account, and it is
>> likely that we want to go that way. However, as to not hold up your
>> patchset, perhaps we could do without it for now.

so, Yes, this is not my blocking patch that prevents me from sending
the new patchset. I intend to let you clean this up with your new
patch.
The v3 is on it's way!

>>
>> Regarding the hardwired usbhid dependency, I think the solution is to
>> move that code to usbhid itself.

yes, maybe, but at the beginning, we didn't want to patch it that way
because it was only specific to one hid-multitouch device (though
armless for the other multitouch devices).
So maybe you will have to add a quirk in usbhid or sth like that.

Cheers,
Benjamin

>>
>> Thanks,
>> Henrik

Re: [PATCH v2 03/11] HID: hid-input: add usage_index argument in input_mapping and event.

[Benjamin Tissoires]

On Tue, Oct 30, 2012 at 11:09 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Oct 29, 2012 at 8:25 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> Hi Benjamin,
>>
>>> Currently, there is no way to know the index of the current field
>>> in the .input_mapping and .event callbacks  when this field is inside
>>> an array of HID fields.
>>> This patch forwards this index to the input_mapping and event
>>> callbacks.
>>
>> I agree with the idea, but the function argument list is becoming
>> ridiculously long... Could we remove the usage pointer argument, at
>> least?
>
> yeah, totally agree. Let me just check whether it will not introduce
> more problems than it solves for my driver.

Well, after a deeper look, it's a really bad idea. Every drivers that
implements input_mapping, input_mapped, event use at least once the
struct usage. So this would require change nearly every hid driver.

To solve that, and to minimize the impact on the other drivers, I'm
going to add a field in struct usage with the appropriate index.

Cheers,
Benjamin

>
>>
>>         int (*event)(struct hid_device *hdev, struct hid_field *field,
>>                         unsigned int usage_index, __s32 value);
>>
>>
>>> @@ -1071,19 +1072,24 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
>>>       for (n = 0; n < count; n++) {
>>>
>>>               if (HID_MAIN_ITEM_VARIABLE & field->flags) {
>>> -                     hid_process_event(hid, field, &field->usage[n], value[n], interrupt);
>>> +                     hid_process_event(hid, field, &field->usage[n], n,
>>> +                             value[n], interrupt);
>>>                       continue;
>>>               }
>>>
>>>               if (field->value[n] >= min && field->value[n] <= max
>>>                       && field->usage[field->value[n] - min].hid
>>>                       && search(value, field->value[n], count))
>>> -                             hid_process_event(hid, field, &field->usage[field->value[n] - min], 0, interrupt);
>>> +                             hid_process_event(hid, field,
>>> +                                     &field->usage[field->value[n] - min], n,
>>> +                                     0, interrupt);
>>
>> Wrong index?
>
> oops, I'll have to check that.
>
> Thanks,
> Benjamin
>
>>
>>>
>>>               if (value[n] >= min && value[n] <= max
>>>                       && field->usage[value[n] - min].hid
>>>                       && search(field->value, value[n], count))
>>> -                             hid_process_event(hid, field, &field->usage[value[n] - min], 1, interrupt);
>>> +                             hid_process_event(hid, field,
>>> +                                     &field->usage[value[n] - min], n,
>>> +                                     1, interrupt);
>>
>> Wrong index?
>>
>>>       }
>>>
>>>       memcpy(field->value, value, count * sizeof(__s32));
>>
>> Thanks,
>> Henrik

Re: [PATCH v2 03/11] HID: hid-input: add usage_index argument in input_mapping and event.

[Jiri Kosina]

On Tue, 6 Nov 2012, Benjamin Tissoires wrote:

> >>> Currently, there is no way to know the index of the current field
> >>> in the .input_mapping and .event callbacks  when this field is inside
> >>> an array of HID fields.
> >>> This patch forwards this index to the input_mapping and event
> >>> callbacks.
> >>
> >> I agree with the idea, but the function argument list is becoming
> >> ridiculously long... Could we remove the usage pointer argument, at
> >> least?
> >
> > yeah, totally agree. Let me just check whether it will not introduce
> > more problems than it solves for my driver.
> 
> Well, after a deeper look, it's a really bad idea. Every drivers that
> implements input_mapping, input_mapped, event use at least once the
> struct usage. So this would require change nearly every hid driver.
> 
> To solve that, and to minimize the impact on the other drivers, I'm
> going to add a field in struct usage with the appropriate index.

Yes please, I like that solution more as well.

Thanks,

-- 
Jiri Kosina
SUSE Labs