[PATCH v3 00/13] Win 8 support for digitizers

[PATCH v3 00/13] Win 8 support for digitizers

[Benjamin Tissoires]

Hi Guys,

here is the third version of this patchset.
I think I included most of Henrik's comments.

Happy reviewing :)

Cheers,
Benjamin

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"

v3 changes:
* splitted "round return value of hidinput_calc_abs_res" in a separate patch
* export snto32 in hid.h as we need to use it in hid-input.c
* didn't change all drivers, but add a field in hid_usage instead
* add quirk MT_QUIRK_IGNORE_DUPLICATES so that any device can rely on it
* easier understandable support of hovering devices
* changed scan time definition
* applied new definition of scan time in hid-multitouch
* some other few things.

Benjamin Tissoires (13):
  HID: hid-input: export hidinput_calc_abs_res
  HID: hid-input: round return value of hidinput_calc_abs_res
  HID: core: fix unit exponent parsing
  HID: hid-input: add usage_index in struct hid_usage.
  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: add MT_QUIRK_IGNORE_DUPLICATES
  HID: hid-multitouch: fix Win 8 protocol
  HID: hid-multitouch: support for hovering devices
  HID: introduce Scan Time
  HID: hid-multitouch: forwards ABS_SCAN_TIME

 Documentation/input/event-codes.txt |   9 +++
 drivers/hid/hid-core.c              |  20 ++++-
 drivers/hid/hid-input.c             |  28 +++++--
 drivers/hid/hid-multitouch.c        | 157 ++++++++++++++++++++++++++++++------
 include/linux/hid.h                 |   4 +
 include/linux/input.h               |   1 +
 include/linux/input/mt.h            |   6 ++
 7 files changed, 192 insertions(+), 33 deletions(-)

-- 
1.7.11.7

[PATCH v3 01/13] HID: hid-input: export hidinput_calc_abs_res

[Benjamin Tissoires]

Exporting the 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      | 9 ++++++++-
 drivers/hid/hid-multitouch.c | 1 +
 include/linux/hid.h          | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 10248cf..f5b1d57 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;
@@ -283,6 +289,7 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	/* Calculate resolution */
 	return 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 3eb02b9..9f64e36 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 v3 02/13] HID: hid-input: round return value of hidinput_calc_abs_res

[Benjamin Tissoires]

hidinput_calc_abs_res should return the closest int in the division
instead of the floor.
On a device with a logical_max of 3008 and a physical_max of 255mm,
previous implementation gave a resolution of 11 instead of 12.
With 11, user-space computes a physical size of 273.5mm and the
round_closest results gives 250.6mm.
The old implementation introduced an error of 2cm in this example.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/hid-input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f5b1d57..67044f3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -287,7 +287,7 @@ __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);
 
-- 
1.7.11.7

[PATCH v3 03/13] 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], so this is
a standard two's complement on a half-byte.

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

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3d0da29..9da3007 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
+		 * two's complement.
+		 * See "6.2.2.7 Global Items" for more information. */
+		raw_value = item_udata(item);
+		if (!(raw_value & 0xfffffff0))
+			parser->global.unit_exponent = hid_snto32(raw_value, 4);
+		else
+			parser->global.unit_exponent = raw_value;
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT:
@@ -865,6 +873,12 @@ static s32 snto32(__u32 value, unsigned n)
 	return value & (1 << (n - 1)) ? value | (-1 << n) : value;
 }
 
+s32 hid_snto32(__u32 value, unsigned n)
+{
+	return snto32(value, n);
+}
+EXPORT_SYMBOL_GPL(hid_snto32);
+
 /*
  * Convert a signed 32-bit integer to a signed n-bit integer.
  */
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 67044f3..7015080 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -192,7 +192,6 @@ static int hidinput_setkeycode(struct input_dev *dev,
 	return -EINVAL;
 }
 
-
 /**
  * hidinput_calc_abs_res - calculate an absolute axis resolution
  * @field: the HID report field to calculate resolution for
@@ -235,17 +234,23 @@ __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;
+		unit_exponent += hid_snto32(field->unit >> 4, 4) - 1;
+		switch (field->unit & 0xf) {
+		case 0x1:				/* If centimeters */
 			/* Convert to millimeters */
 			unit_exponent += 1;
-		} else if (field->unit == 0x13) {	/* If inches */
+			break;
+		case 0x3:				/* If inches */
 			/* Convert to millimeters */
 			prev = physical_extents;
 			physical_extents *= 254;
 			if (physical_extents < prev)
 				return 0;
 			unit_exponent -= 1;
-		} else {
+			break;
+		default:
 			return 0;
 		}
 		break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9edb06c..a110a3e 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -754,6 +754,7 @@ int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
 const struct hid_device_id *hid_match_id(struct hid_device *hdev,
 					 const struct hid_device_id *id);
+s32 hid_snto32(__u32 value, unsigned n);
 
 /**
  * hid_map_usage - map usage input bits
-- 
1.7.11.7

[PATCH v3 04/13] HID: hid-input: add usage_index in struct hid_usage.

[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 adds this index to the struct hid_usage so that this
information is available to input_mapping and event callbacks.

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

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9da3007..8f82c4c 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -92,6 +92,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
 static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
 {
 	struct hid_field *field;
+	int i;
 
 	if (report->maxfield == HID_MAX_FIELDS) {
 		hid_err(report->device, "too many fields in report\n");
@@ -110,6 +111,9 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
 	field->value = (s32 *)(field->usage + usages);
 	field->report = report;
 
+	for (i = 0; i < usages; i++)
+		field->usage[i].usage_index = i;
+
 	return field;
 }
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index a110a3e..6b4f322 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -380,6 +380,7 @@ struct hid_usage {
 	unsigned  hid;			/* hid usage code */
 	unsigned  collection_index;	/* index into collection array */
 	/* hidinput data */
+	unsigned  usage_index;		/* index into usage array */
 	__u16     code;			/* input driver code */
 	__u8      type;			/* input driver type */
 	__s8	  hat_min;		/* hat switch fun */
-- 
1.7.11.7

[PATCH v3 05/13] 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 used in an array.

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

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 9f64e36..a6a4e0a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -577,12 +577,15 @@ 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->usage_index + 1 == field->report_count) {
+			/* we only take into account the last report. */
+			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 v3 06/13] 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>
Acked-by: Henrik Rydberg <rydberg@euromail.se>
---
 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 a6a4e0a..ae57b8f 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 v3 07/13] 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.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index ae57b8f..54367f4 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -54,7 +54,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_NO_AREA		(1 << 9)
 
 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? */
 };
@@ -323,6 +323,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	struct mt_device *td = hid_get_drvdata(hdev);
 	struct mt_class *cls = &td->mtclass;
 	int code;
+	struct hid_usage *prev_usage = NULL;
 
 	/* Only map fields from TouchScreen or TouchPad collections.
 	* We need to ignore fields that belong to other collections
@@ -345,23 +346,42 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	if (field->physical == HID_DG_STYLUS)
 		return -1;
 
+	if (usage->usage_index)
+		prev_usage = &field->usage[usage->usage_index - 1];
+
 	switch (usage->hid & HID_USAGE_PAGE) {
 
 	case HID_UP_GENDESK:
 		switch (usage->hid) {
 		case HID_GD_X:
-			hid_map_usage(hi, usage, bit, max,
+			if (prev_usage && (prev_usage->hid == usage->hid)) {
+				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 (prev_usage && (prev_usage->hid == usage->hid)) {
+				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 +522,8 @@ 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);
+			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 +575,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_TOOL_X)
+				td->curdata.cx = value;
+			else
+				td->curdata.x = value;
 			break;
 		case HID_GD_Y:
-			td->curdata.y = value;
+			if (usage->code == ABS_MT_TOOL_Y)
+				td->curdata.cy = value;
+			else
+				td->curdata.y = value;
 			break;
 		case HID_DG_WIDTH:
 			td->curdata.w = value;
-- 
1.7.11.7

[PATCH v3 08/13] 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 54367f4..2352770 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -503,7 +503,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;
 
@@ -554,9 +554,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 v3 09/13] HID: hid-multitouch: add MT_QUIRK_IGNORE_DUPLICATES

[Benjamin Tissoires]

this prevents a device to reuse contact id 0 several time when
sending garbage values to complete the report.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2352770..351c814 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -52,6 +52,7 @@ 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_IGNORE_DUPLICATES	(1 << 10)
 
 struct mt_slot {
 	__s32 x, y, cx, cy, p, w, h;
@@ -510,6 +511,11 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 		if (slotnum < 0 || slotnum >= td->maxcontacts)
 			return;
 
+		if (td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES &&
+		    input_mt_is_active(&input->mt->slots[slotnum]) &&
+		    input_mt_is_used(input->mt, &input->mt->slots[slotnum]))
+				return;
+
 		input_mt_slot(input, slotnum);
 		input_mt_report_slot_state(input, MT_TOOL_FINGER,
 			s->touch_state);
diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
index cc5cca7..2e86bd0 100644
--- a/include/linux/input/mt.h
+++ b/include/linux/input/mt.h
@@ -69,6 +69,12 @@ static inline bool input_mt_is_active(const struct input_mt_slot *slot)
 	return input_mt_get_value(slot, ABS_MT_TRACKING_ID) >= 0;
 }
 
+static inline bool input_mt_is_used(const struct input_mt *mt,
+				    const struct input_mt_slot *slot)
+{
+	return slot->frame == mt->frame;
+}
+
 int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 			unsigned int flags);
 void input_mt_destroy_slots(struct input_dev *dev);
-- 
1.7.11.7

[PATCH v3 10/13] 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 until it is released.
So we can use the always_valid quirk and dismiss reports when we see
duplicates contact ID.

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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 351c814..b393c6c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -53,6 +53,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE	(1 << 8)
 #define MT_QUIRK_NO_AREA		(1 << 9)
 #define MT_QUIRK_IGNORE_DUPLICATES	(1 << 10)
+#define MT_QUIRK_WIN_8_CERTIFIED	(1 << 11)
 
 struct mt_slot {
 	__s32 x, y, cx, cy, p, w, h;
@@ -293,6 +294,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;
 	}
 }
 
@@ -679,14 +684,17 @@ 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;
 		quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
 	}
 
+	if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
+		quirks |= MT_QUIRK_IGNORE_DUPLICATES;
+
 	td->mtclass.quirks = quirks;
 }
 
-- 
1.7.11.7

[PATCH v3 11/13] 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 | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index b393c6c..1f3d1e0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -59,6 +59,7 @@ struct mt_slot {
 	__s32 x, y, cx, cy, p, w, h;
 	__s32 contactid;	/* the device ContactID assigned to this slot */
 	bool touch_state;	/* is the touch valid? */
+	bool inrange_state;	/* is the finger in proximity of the sensor? */
 };
 
 struct mt_class {
@@ -397,6 +398,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, -1, 1, 0, 0);
+			}
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
@@ -516,6 +523,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 		if (slotnum < 0 || slotnum >= td->maxcontacts)
 			return;
 
+		if (!test_bit(ABS_MT_DISTANCE, input->absbit))
+			/* If InRange is not present, rely on TipSwitch */
+			s->inrange_state = s->touch_state;
+
 		if (td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES &&
 		    input_mt_is_active(&input->mt->slots[slotnum]) &&
 		    input_mt_is_used(input->mt, &input->mt->slots[slotnum]))
@@ -523,9 +534,9 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 
 		input_mt_slot(input, slotnum);
 		input_mt_report_slot_state(input, MT_TOOL_FINGER,
-			s->touch_state);
-		if (s->touch_state) {
-			/* this finger is on the screen */
+			s->inrange_state);
+		if (s->inrange_state) {
+			/* this finger is in proximity of the sensor */
 			int wide = (s->w > s->h);
 			/* divided by two to match visual scale of touch */
 			int major = max(s->w, s->h) >> 1;
@@ -535,11 +546,14 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 			input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
 			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_DISTANCE,
+				!s->touch_state);
 			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);
 			input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
-		}
+		} else
+			input_event(input, EV_ABS, ABS_MT_DISTANCE, -1);
 	}
 
 	td->num_received++;
@@ -567,6 +581,8 @@ 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.inrange_state = value;
 			break;
 		case HID_DG_TIPSWITCH:
 			if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
-- 
1.7.11.7

[PATCH v3 12/13] 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 | 9 +++++++++
 drivers/hid/hid-input.c             | 4 ++++
 include/linux/hid.h                 | 1 +
 include/linux/input.h               | 1 +
 4 files changed, 15 insertions(+)

diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
index 53305bd..80c06e5 100644
--- a/Documentation/input/event-codes.txt
+++ b/Documentation/input/event-codes.txt
@@ -174,6 +174,15 @@ 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 to report the number of microseconds since the last reset. This event
+    should 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.  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 7015080..1c96238 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -677,6 +677,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/include/linux/hid.h b/include/linux/hid.h
index 6b4f322..0337e50 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 v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME

[Benjamin Tissoires]

Computes the scan time according to the specification.
It also ensures that if the time between two events is greater
than MAX_SCAN_INTERVAL, the scan time will be reset.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 1f3d1e0..5902567 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/usb.h>
 #include <linux/input/mt.h>
+#include <linux/jiffies.h>
 #include "usbhid/usbhid.h"
 
 
@@ -98,6 +99,9 @@ struct mt_device {
 	bool serial_maybe;	/* need to check for serial protocol */
 	bool curvalid;		/* is the current contact valid? */
 	unsigned mt_flags;	/* flags to pass to input-mt */
+	__s32 dev_time;		/* the scan time provided by the device */
+	unsigned long jiffies;	/* the frame's jiffies */
+	unsigned scan_time;	/* the scan time to be sent */
 };
 
 /* classes of device behavior */
@@ -126,6 +130,9 @@ struct mt_device {
 #define MT_DEFAULT_MAXCONTACT	10
 #define MT_MAX_MAXCONTACT	250
 
+#define MAX_SCAN_TIME		INT_MAX
+#define MAX_SCAN_INTERVAL	500000
+
 #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)
 
@@ -451,12 +458,20 @@ 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);
+			input_set_abs_params(hi->input,
+				ABS_SCAN_TIME, 0, MAX_SCAN_TIME, 0, 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;
 		}
@@ -565,11 +580,34 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
  */
 static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 {
+	input_event(input, EV_ABS, ABS_SCAN_TIME, td->scan_time);
 	input_mt_sync_frame(input);
 	input_sync(input);
 	td->num_received = 0;
 }
 
+static void mt_compute_scan_time(struct mt_device *td, struct hid_field *field,
+		__s32 value)
+{
+	long delta = value - td->dev_time;
+	unsigned long jdelta = jiffies_to_usecs(jiffies - td->jiffies);
+
+	td->jiffies = jiffies;
+	td->dev_time = value;
+
+	if (delta < 0)
+		delta += field->logical_maximum;
+
+	/* HID_DG_SCANTIME is expressed in 100us, we want it in ms. */
+	delta *= 100;
+
+	if (abs(delta - jdelta) > MAX_SCAN_INTERVAL)
+		/* obviously wrong clock -> the device time has been reset */
+		td->scan_time = 0;
+	else
+		td->scan_time += delta;
+}
+
 static int mt_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
@@ -617,6 +655,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 		case HID_DG_HEIGHT:
 			td->curdata.h = value;
 			break;
+		case HID_DG_SCANTIME:
+			mt_compute_scan_time(td, field, value);
+			break;
 		case HID_DG_CONTACTCOUNT:
 			/*
 			 * Includes multi-packet support where subsequent
-- 
1.7.11.7

Re: [PATCH v3 12/13] HID: introduce Scan Time

[Dmitry Torokhov]

Hi Benjamin,

On Wed, Nov 07, 2012 at 05:37:35PM +0100, Benjamin Tissoires wrote:
> 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 | 9 +++++++++
>  drivers/hid/hid-input.c             | 4 ++++
>  include/linux/hid.h                 | 1 +
>  include/linux/input.h               | 1 +
>  4 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
> index 53305bd..80c06e5 100644
> --- a/Documentation/input/event-codes.txt
> +++ b/Documentation/input/event-codes.txt
> @@ -174,6 +174,15 @@ 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 to report the number of microseconds since the last reset. This event
> +    should 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.  If the device does not provide this information, the driver must
> +    not provide it to the user space.
> +

This should not be an absolute event but rather EV_MSC/MSC_TIMESTAMP.

Thanks.

-- 
Dmitry

Re: [PATCH v3 03/13] HID: core: fix unit exponent parsing

[Jiri Kosina]

On Wed, 7 Nov 2012, Benjamin Tissoires wrote:

> HID spec details special values for the HID field unit exponent.
> Basically, the range [0x8..0xf] correspond to [-8..-1], so this is
> a standard two's complement on a half-byte.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

> ---
>  drivers/hid/hid-core.c  | 16 +++++++++++++++-
>  drivers/hid/hid-input.c | 13 +++++++++----
>  include/linux/hid.h     |  1 +
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3d0da29..9da3007 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
> +		 * two's complement.
> +		 * See "6.2.2.7 Global Items" for more information. */
> +		raw_value = item_udata(item);
> +		if (!(raw_value & 0xfffffff0))
> +			parser->global.unit_exponent = hid_snto32(raw_value, 4);
> +		else
> +			parser->global.unit_exponent = raw_value;
>  		return 0;
>  
>  	case HID_GLOBAL_ITEM_TAG_UNIT:
> @@ -865,6 +873,12 @@ static s32 snto32(__u32 value, unsigned n)
>  	return value & (1 << (n - 1)) ? value | (-1 << n) : value;
>  }
>  
> +s32 hid_snto32(__u32 value, unsigned n)
> +{
> +	return snto32(value, n);
> +}
> +EXPORT_SYMBOL_GPL(hid_snto32);
> +
>  /*
>   * Convert a signed 32-bit integer to a signed n-bit integer.
>   */
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 67044f3..7015080 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -192,7 +192,6 @@ static int hidinput_setkeycode(struct input_dev *dev,
>  	return -EINVAL;
>  }
>  
> -
>  /**
>   * hidinput_calc_abs_res - calculate an absolute axis resolution
>   * @field: the HID report field to calculate resolution for
> @@ -235,17 +234,23 @@ __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;
> +		unit_exponent += hid_snto32(field->unit >> 4, 4) - 1;
> +		switch (field->unit & 0xf) {
> +		case 0x1:				/* If centimeters */
>  			/* Convert to millimeters */
>  			unit_exponent += 1;
> -		} else if (field->unit == 0x13) {	/* If inches */
> +			break;
> +		case 0x3:				/* If inches */
>  			/* Convert to millimeters */
>  			prev = physical_extents;
>  			physical_extents *= 254;
>  			if (physical_extents < prev)
>  				return 0;
>  			unit_exponent -= 1;
> -		} else {
> +			break;
> +		default:
>  			return 0;
>  		}
>  		break;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 9edb06c..a110a3e 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -754,6 +754,7 @@ int hid_connect(struct hid_device *hid, unsigned int connect_mask);
>  void hid_disconnect(struct hid_device *hid);
>  const struct hid_device_id *hid_match_id(struct hid_device *hdev,
>  					 const struct hid_device_id *id);
> +s32 hid_snto32(__u32 value, unsigned n);
>  
>  /**
>   * hid_map_usage - map usage input bits
> -- 
> 1.7.11.7
> 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 04/13] HID: hid-input: add usage_index in struct hid_usage.

[Jiri Kosina]

On Wed, 7 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 adds this index to the struct hid_usage so that this
> information is available to input_mapping and event callbacks.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

> ---
>  drivers/hid/hid-core.c | 4 ++++
>  include/linux/hid.h    | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9da3007..8f82c4c 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -92,6 +92,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
>  static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
>  {
>  	struct hid_field *field;
> +	int i;
>  
>  	if (report->maxfield == HID_MAX_FIELDS) {
>  		hid_err(report->device, "too many fields in report\n");
> @@ -110,6 +111,9 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
>  	field->value = (s32 *)(field->usage + usages);
>  	field->report = report;
>  
> +	for (i = 0; i < usages; i++)
> +		field->usage[i].usage_index = i;
> +
>  	return field;
>  }
>  
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index a110a3e..6b4f322 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -380,6 +380,7 @@ struct hid_usage {
>  	unsigned  hid;			/* hid usage code */
>  	unsigned  collection_index;	/* index into collection array */
>  	/* hidinput data */
> +	unsigned  usage_index;		/* index into usage array */
>  	__u16     code;			/* input driver code */
>  	__u8      type;			/* input driver type */
>  	__s8	  hat_min;		/* hat switch fun */
> -- 
> 1.7.11.7
> 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 02/13] HID: hid-input: round return value of hidinput_calc_abs_res

[Jiri Kosina]

On Wed, 7 Nov 2012, Benjamin Tissoires wrote:

> hidinput_calc_abs_res should return the closest int in the division
> instead of the floor.
> On a device with a logical_max of 3008 and a physical_max of 255mm,
> previous implementation gave a resolution of 11 instead of 12.
> With 11, user-space computes a physical size of 273.5mm and the
> round_closest results gives 250.6mm.
> The old implementation introduced an error of 2cm in this example.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Acked-by: Jiri Kosina <jkosina@suse.cz>

> ---
>  drivers/hid/hid-input.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index f5b1d57..67044f3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -287,7 +287,7 @@ __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);
>  
> -- 
> 1.7.11.7
> 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 01/13] HID: hid-input: export hidinput_calc_abs_res

[Jiri Kosina]

On Wed, 7 Nov 2012, Benjamin Tissoires wrote:

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

Acked-by: Jiri Kosina <jkosina@suse.cz>

> ---
>  drivers/hid/hid-input.c      | 9 ++++++++-
>  drivers/hid/hid-multitouch.c | 1 +
>  include/linux/hid.h          | 1 +
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 10248cf..f5b1d57 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;
> @@ -283,6 +289,7 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  	/* Calculate resolution */
>  	return 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 3eb02b9..9f64e36 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
> 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 00/13] Win 8 support for digitizers

[Jiri Kosina]

On Wed, 7 Nov 2012, Benjamin Tissoires wrote:

> Hi Guys,
> 
> here is the third version of this patchset.
> I think I included most of Henrik's comments.
> 
> Happy reviewing :)
> 
> Cheers,
> Benjamin
> 
> 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"
> 
> v3 changes:
> * splitted "round return value of hidinput_calc_abs_res" in a separate patch
> * export snto32 in hid.h as we need to use it in hid-input.c
> * didn't change all drivers, but add a field in hid_usage instead
> * add quirk MT_QUIRK_IGNORE_DUPLICATES so that any device can rely on it
> * easier understandable support of hovering devices
> * changed scan time definition
> * applied new definition of scan time in hid-multitouch
> * some other few things.

Benjamin,

thanks for the patchset.

I am fine with the HID-specific changes (and I have sent out my Acks to 
the invidivual patches).

For hid-multitouch changes, I'd like to have Reviewed-by: from Henrik as 
well. Henrik, are you planning to review this patchset, please?

Also the note from Dmitry on turning ABS_SCAN_TIME into 
EV_MSC/MSC_TIMESTAMP should be addressed.

Once this is finalized, I'd like to queue this for 3.8 in my tree.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3 00/13] Win 8 support for digitizers

[Benjamin Tissoires]

Hi Jiri,

On Tue, Nov 13, 2012 at 8:47 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 7 Nov 2012, Benjamin Tissoires wrote:
>
>> Hi Guys,
>>
>> here is the third version of this patchset.
>> I think I included most of Henrik's comments.
>>
>> Happy reviewing :)
>>
>> Cheers,
>> Benjamin
>>
>> 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"
>>
>> v3 changes:
>> * splitted "round return value of hidinput_calc_abs_res" in a separate patch
>> * export snto32 in hid.h as we need to use it in hid-input.c
>> * didn't change all drivers, but add a field in hid_usage instead
>> * add quirk MT_QUIRK_IGNORE_DUPLICATES so that any device can rely on it
>> * easier understandable support of hovering devices
>> * changed scan time definition
>> * applied new definition of scan time in hid-multitouch
>> * some other few things.
>
> Benjamin,
>
> thanks for the patchset.
>
> I am fine with the HID-specific changes (and I have sent out my Acks to
> the invidivual patches).

Thanks :)

>
> For hid-multitouch changes, I'd like to have Reviewed-by: from Henrik as
> well. Henrik, are you planning to review this patchset, please?
>
> Also the note from Dmitry on turning ABS_SCAN_TIME into
> EV_MSC/MSC_TIMESTAMP should be addressed.

Yes, sure. I'll do it today.

>
> Once this is finalized, I'd like to queue this for 3.8 in my tree.

Thanks,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH v3 12/13] HID: introduce Scan Time

[Benjamin Tissoires]

On 11/09/2012 09:45 AM, Dmitry Torokhov wrote:
> Hi Benjamin,
> 
> On Wed, Nov 07, 2012 at 05:37:35PM +0100, Benjamin Tissoires wrote:
>> 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 | 9 +++++++++
>>   drivers/hid/hid-input.c             | 4 ++++
>>   include/linux/hid.h                 | 1 +
>>   include/linux/input.h               | 1 +
>>   4 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
>> index 53305bd..80c06e5 100644
>> --- a/Documentation/input/event-codes.txt
>> +++ b/Documentation/input/event-codes.txt
>> @@ -174,6 +174,15 @@ 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 to report the number of microseconds since the last reset. This event
>> +    should 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.  If the device does not provide this information, the driver must
>> +    not provide it to the user space.
>> +
> 
> This should not be an absolute event but rather EV_MSC/MSC_TIMESTAMP.
> 
> Thanks.

Hi Dmitry,

alright.
However, since we are using EV_MSC now and the hid layer is not so friendly with them,
the change in hid-input can not be done in the same way, so let's drop it for now.

How about this?


From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Date: Tue, 13 Nov 2012 15:11:22 +0100
Subject: [PATCH] Input: introduce EV_MSC Timestamp

Win 8 digitizer devices provides the actual timestamp (hid_dg_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 | 11 +++++++++++
 include/linux/input.h               |  1 +
 2 files changed, 12 insertions(+)

diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
index 53305bd..6f28e11 100644
--- a/Documentation/input/event-codes.txt
+++ b/Documentation/input/event-codes.txt
@@ -196,6 +196,17 @@ EV_MSC:
 EV_MSC events are used for input and output events that do not fall under other
 categories.
 
+A few EV_MSC codes have special meanings:
+
+* MSC_TIMESTAMP:
+  - Used to report the number of microseconds since the last reset. This event
+    should 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.  If the device does not provide this information, the driver must
+    not provide it to the user space.
+
 EV_LED:
 ----------
 EV_LED events are used for input and output to set and query the state of
diff --git a/include/linux/input.h b/include/linux/input.h
index ba48743..25354f3 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -858,6 +858,7 @@ struct input_keymap_entry {
 #define MSC_GESTURE		0x02
 #define MSC_RAW			0x03
 #define MSC_SCAN		0x04
+#define MSC_TIMESTAMP		0x05
 #define MSC_MAX			0x07
 #define MSC_CNT			(MSC_MAX+1)
 
-- 
1.8.0

Cheers,
Benjamin

Re: [PATCH v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME

[Benjamin Tissoires]

On 11/07/2012 05:37 PM, Benjamin Tissoires wrote:
> Computes the scan time according to the specification.
> It also ensures that if the time between two events is greater
> than MAX_SCAN_INTERVAL, the scan time will be reset.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>   drivers/hid/hid-multitouch.c | 45 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 43 insertions(+), 2 deletions(-)
> 
[snipped]
>   			/*
>   			 * Includes multi-packet support where subsequent
> 


Since ABS_SCAN_TIME has been replaced by MSC_TIMESTAMP, this patch is modified as this:

From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Date: Tue, 13 Nov 2012 15:12:17 +0100
Subject: [PATCH v4] HID: hid-multitouch: forwards MSC_TIMESTAMP

Computes the device timestamp according to the specification.
It also ensures that if the time between two events is greater
than MAX_TIMESTAMP_INTERVAL, the timestamp will be reset.

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

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index caf0f0b..3f8432d 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/usb.h>
 #include <linux/input/mt.h>
+#include <linux/jiffies.h>
 #include "usbhid/usbhid.h"
 
 
@@ -98,6 +99,9 @@ struct mt_device {
 	bool serial_maybe;	/* need to check for serial protocol */
 	bool curvalid;		/* is the current contact valid? */
 	unsigned mt_flags;	/* flags to pass to input-mt */
+	__s32 dev_time;		/* the scan time provided by the device */
+	unsigned long jiffies;	/* the frame's jiffies */
+	unsigned timestamp;	/* the timestamp to be sent */
 };
 
 /* classes of device behavior */
@@ -126,6 +130,8 @@ struct mt_device {
 #define MT_DEFAULT_MAXCONTACT	10
 #define MT_MAX_MAXCONTACT	250
 
+#define MAX_TIMESTAMP_INTERVAL	500000
+
 #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)
 
@@ -451,12 +457,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_MSC, MSC_TIMESTAMP);
+			set_bit(MSC_TIMESTAMP, hi->input->mscbit);
+			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;
 		}
@@ -485,7 +498,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
 {
-	if (usage->type == EV_KEY || usage->type == EV_ABS)
+	if (usage->type == EV_KEY || usage->type == EV_ABS ||
+	    usage->type == EV_MSC)
 		set_bit(usage->type, hi->input->evbit);
 
 	return -1;
@@ -565,11 +579,34 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
  */
 static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 {
+	input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp);
 	input_mt_sync_frame(input);
 	input_sync(input);
 	td->num_received = 0;
 }
 
+static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
+		__s32 value)
+{
+	long delta = value - td->dev_time;
+	unsigned long jdelta = jiffies_to_usecs(jiffies - td->jiffies);
+
+	td->jiffies = jiffies;
+	td->dev_time = value;
+
+	if (delta < 0)
+		delta += field->logical_maximum;
+
+	/* HID_DG_SCANTIME is expressed in 100us, we want it in ms. */
+	delta *= 100;
+
+	if (abs(delta - jdelta) > MAX_TIMESTAMP_INTERVAL)
+		/* obviously wrong clock -> the device time has been reset */
+		td->timestamp = 0;
+	else
+		td->timestamp += delta;
+}
+
 static int mt_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
@@ -617,6 +654,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 		case HID_DG_HEIGHT:
 			td->curdata.h = value;
 			break;
+		case HID_DG_SCANTIME:
+			mt_compute_timestamp(td, field, value);
+			break;
 		case HID_DG_CONTACTCOUNT:
 			/*
 			 * Includes multi-packet support where subsequent
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 6b4f322..0337e50 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!
-- 
1.8.0

Cheers,
Benjamin

Re: [PATCH v3 04/13] HID: hid-input: add usage_index in struct hid_usage.

[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 adds this index to the struct hid_usage so that this
> information is available to input_mapping and event callbacks.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-core.c | 4 ++++
>  include/linux/hid.h    | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9da3007..8f82c4c 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -92,6 +92,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
>  static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
>  {
>  	struct hid_field *field;
> +	int i;
>  
>  	if (report->maxfield == HID_MAX_FIELDS) {
>  		hid_err(report->device, "too many fields in report\n");
> @@ -110,6 +111,9 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
>  	field->value = (s32 *)(field->usage + usages);
>  	field->report = report;
>  
> +	for (i = 0; i < usages; i++)
> +		field->usage[i].usage_index = i;
> +
>  	return field;
>  }
>  
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index a110a3e..6b4f322 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -380,6 +380,7 @@ struct hid_usage {
>  	unsigned  hid;			/* hid usage code */
>  	unsigned  collection_index;	/* index into collection array */
>  	/* hidinput data */
> +	unsigned  usage_index;		/* index into usage array */

Is  this really hidinput data? Should it not be one line above?

>  	__u16     code;			/* input driver code */
>  	__u8      type;			/* input driver type */
>  	__s8	  hat_min;		/* hat switch fun */
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v3 05/13] HID: hid-multitouch: support arrays for the split of the touches in a report

[Henrik Rydberg]

On Wed, Nov 07, 2012 at 05:37:28PM +0100, Benjamin Tissoires wrote:
> Win8 certification introduced the ability to transmit two X and two Y per
> touch. The specification precises that it must be used in an array.
> 
> This test guarantees that we split the touches on the last element
> in this array.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 9f64e36..a6a4e0a 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -577,12 +577,15 @@ 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->usage_index + 1 == field->report_count) {
> +			/* we only take into account the last report. */
> +			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
> 

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

Thanks,
Henrik

Re: [PATCH v3 07/13] HID: hid-multitouch: support T and C for win8 devices

[Henrik Rydberg]

On Wed, Nov 07, 2012 at 05:37:30PM +0100, Benjamin Tissoires wrote:
> 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.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 46 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index ae57b8f..54367f4 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -54,7 +54,7 @@ MODULE_LICENSE("GPL");
>  #define MT_QUIRK_NO_AREA		(1 << 9)
>  
>  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? */
>  };
> @@ -323,6 +323,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	struct mt_device *td = hid_get_drvdata(hdev);
>  	struct mt_class *cls = &td->mtclass;
>  	int code;
> +	struct hid_usage *prev_usage = NULL;
>  
>  	/* Only map fields from TouchScreen or TouchPad collections.
>  	* We need to ignore fields that belong to other collections
> @@ -345,23 +346,42 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	if (field->physical == HID_DG_STYLUS)
>  		return -1;
>  
> +	if (usage->usage_index)
> +		prev_usage = &field->usage[usage->usage_index - 1];
> +
>  	switch (usage->hid & HID_USAGE_PAGE) {
>  
>  	case HID_UP_GENDESK:
>  		switch (usage->hid) {
>  		case HID_GD_X:
> -			hid_map_usage(hi, usage, bit, max,
> +			if (prev_usage && (prev_usage->hid == usage->hid)) {
> +				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 (prev_usage && (prev_usage->hid == usage->hid)) {
> +				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 +522,8 @@ 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);
> +			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 +575,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_TOOL_X)
> +				td->curdata.cx = value;
> +			else
> +				td->curdata.x = value;
>  			break;
>  		case HID_GD_Y:
> -			td->curdata.y = value;
> +			if (usage->code == ABS_MT_TOOL_Y)
> +				td->curdata.cy = value;
> +			else
> +				td->curdata.y = value;
>  			break;
>  		case HID_DG_WIDTH:
>  			td->curdata.w = value;
> -- 
> 1.7.11.7
> 

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

Thanks,
Henrik

Re: [PATCH v3 08/13] HID: hid-multitouch: move ALWAYS_VALID quirk check

[Henrik Rydberg]

On Wed, Nov 07, 2012 at 05:37:31PM +0100, Benjamin Tissoires wrote:
> 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 54367f4..2352770 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -503,7 +503,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;
>  
> @@ -554,9 +554,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
> 

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

Thanks,
Henrik

Re: [PATCH v3 09/13] HID: hid-multitouch: add MT_QUIRK_IGNORE_DUPLICATES

[Henrik Rydberg]

Hi Benjamin,

> this prevents a device to reuse contact id 0 several time when
> sending garbage values to complete the report.

This quirk allows a device to reuse a contact id when sending garbage
inactive contacts at the end of a report.

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 6 ++++++
>  include/linux/input/mt.h     | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 2352770..351c814 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -52,6 +52,7 @@ 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_IGNORE_DUPLICATES	(1 << 10)
>  
>  struct mt_slot {
>  	__s32 x, y, cx, cy, p, w, h;
> @@ -510,6 +511,11 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  		if (slotnum < 0 || slotnum >= td->maxcontacts)
>  			return;
>  
> +		if (td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES &&

Don't like parenthesis, i see :-)

> +		    input_mt_is_active(&input->mt->slots[slotnum]) &&
> +		    input_mt_is_used(input->mt, &input->mt->slots[slotnum]))
> +				return;
> +

It init_mt_slots failed earlier (no checks), input->mt can be NULL
here. Also, a local variable instead of repetition would be nice.

>  		input_mt_slot(input, slotnum);
>  		input_mt_report_slot_state(input, MT_TOOL_FINGER,
>  			s->touch_state);
> diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
> index cc5cca7..2e86bd0 100644
> --- a/include/linux/input/mt.h
> +++ b/include/linux/input/mt.h
> @@ -69,6 +69,12 @@ static inline bool input_mt_is_active(const struct input_mt_slot *slot)
>  	return input_mt_get_value(slot, ABS_MT_TRACKING_ID) >= 0;
>  }
>  
> +static inline bool input_mt_is_used(const struct input_mt *mt,
> +				    const struct input_mt_slot *slot)
> +{
> +	return slot->frame == mt->frame;
> +}
> +

This is ok, although I would prefer to also change input-mt.c to use
this helper function. Possibly that turns this hunk into a separate patch.

>  int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
>  			unsigned int flags);
>  void input_mt_destroy_slots(struct input_dev *dev);
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v3 10/13] HID: hid-multitouch: fix Win 8 protocol

[Henrik Rydberg]

Hi Benjamin,

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

You could possibly start the commit message here..

> The protocol specify the fact that each valid touch must be reported

The win8 protocol...

> within a frame until it is released.
> So we can use the always_valid quirk and dismiss reports when we see

We can therefore...

> duplicates contact ID.

duplicate contacts.

> 
> 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 | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 351c814..b393c6c 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -53,6 +53,7 @@ MODULE_LICENSE("GPL");
>  #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE	(1 << 8)
>  #define MT_QUIRK_NO_AREA		(1 << 9)
>  #define MT_QUIRK_IGNORE_DUPLICATES	(1 << 10)
> +#define MT_QUIRK_WIN_8_CERTIFIED	(1 << 11)
>  
>  struct mt_slot {
>  	__s32 x, y, cx, cy, p, w, h;
> @@ -293,6 +294,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;
>  	}
>  }
>  
> @@ -679,14 +684,17 @@ 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;

Won't this line interfere with the hovering functionality?

>  		quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
>  	}
>  
> +	if (quirks & MT_QUIRK_WIN_8_CERTIFIED)
> +		quirks |= MT_QUIRK_IGNORE_DUPLICATES;
> +
>  	td->mtclass.quirks = quirks;
>  }
>  
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices

[Henrik Rydberg]

On Wed, Nov 07, 2012 at 05:37:34PM +0100, Benjamin Tissoires wrote:
> Win8 devices supporting hovering must provides InRange HID field.

provide the

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

In the absence of more detailed information, use ABS_MT_DISTANCE with
a [0,1] interval to distinguish between presence (1) and touch (0).

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index b393c6c..1f3d1e0 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -59,6 +59,7 @@ struct mt_slot {
>  	__s32 x, y, cx, cy, p, w, h;
>  	__s32 contactid;	/* the device ContactID assigned to this slot */
>  	bool touch_state;	/* is the touch valid? */
> +	bool inrange_state;	/* is the finger in proximity of the sensor? */
>  };
>  
>  struct mt_class {
> @@ -397,6 +398,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, -1, 1, 0, 0);

Why [-1,1] here?

> +			}
>  			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
> @@ -516,6 +523,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  		if (slotnum < 0 || slotnum >= td->maxcontacts)
>  			return;
>  
> +		if (!test_bit(ABS_MT_DISTANCE, input->absbit))
> +			/* If InRange is not present, rely on TipSwitch */
> +			s->inrange_state = s->touch_state;
> +

This could be skipped, see below.

>  		if (td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES &&
>  		    input_mt_is_active(&input->mt->slots[slotnum]) &&
>  		    input_mt_is_used(input->mt, &input->mt->slots[slotnum]))
> @@ -523,9 +534,9 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  
>  		input_mt_slot(input, slotnum);
>  		input_mt_report_slot_state(input, MT_TOOL_FINGER,
> -			s->touch_state);
> -		if (s->touch_state) {
> -			/* this finger is on the screen */
> +			s->inrange_state);
> +		if (s->inrange_state) {
> +			/* this finger is in proximity of the sensor */

Using (s->touch_state || s->inrange_state) here seems simpler.

>  			int wide = (s->w > s->h);
>  			/* divided by two to match visual scale of touch */
>  			int major = max(s->w, s->h) >> 1;
> @@ -535,11 +546,14 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  			input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
>  			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_DISTANCE,
> +				!s->touch_state);
>  			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);
>  			input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> -		}
> +		} else
> +			input_event(input, EV_ABS, ABS_MT_DISTANCE, -1);

Just skip this, please.

>  	}
>  
>  	td->num_received++;
> @@ -567,6 +581,8 @@ 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.inrange_state = value;
>  			break;
>  		case HID_DG_TIPSWITCH:
>  			if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
> -- 
> 1.7.11.7
> 

Thanks,
Henrik

Re: [PATCH v3 12/13] HID: introduce Scan Time

[Henrik Rydberg]

Hi Benjamin,

> From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Date: Tue, 13 Nov 2012 15:11:22 +0100
> Subject: [PATCH] Input: introduce EV_MSC Timestamp
> 
> Win 8 digitizer devices provides the actual timestamp (hid_dg_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).

I think it is good to use win8 as an example here, but this feature is
present in many other devices as well, and this feature is generic to
all of them. It would thus make sense to start off a bit more general here.

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  Documentation/input/event-codes.txt | 11 +++++++++++
>  include/linux/input.h               |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
> index 53305bd..6f28e11 100644
> --- a/Documentation/input/event-codes.txt
> +++ b/Documentation/input/event-codes.txt
> @@ -196,6 +196,17 @@ EV_MSC:
>  EV_MSC events are used for input and output events that do not fall under other
>  categories.
>  
> +A few EV_MSC codes have special meanings:

meaning:

> +
> +* MSC_TIMESTAMP:
> +  - Used to report the number of microseconds since the last reset. This event
> +    should 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.  If the device does not provide this information, the driver must
> +    not provide it to the user space.

to user space.

> +
>  EV_LED:
>  ----------
>  EV_LED events are used for input and output to set and query the state of
> diff --git a/include/linux/input.h b/include/linux/input.h
> index ba48743..25354f3 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -858,6 +858,7 @@ struct input_keymap_entry {
>  #define MSC_GESTURE		0x02
>  #define MSC_RAW			0x03
>  #define MSC_SCAN		0x04
> +#define MSC_TIMESTAMP		0x05
>  #define MSC_MAX			0x07
>  #define MSC_CNT			(MSC_MAX+1)
>  
> -- 
> 1.8.0
> 
> Cheers,
> Benjamin

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

Thanks,
Henrik

Re: [PATCH v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME

[Henrik Rydberg]

Hi Benjamin,

> From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Date: Tue, 13 Nov 2012 15:12:17 +0100
> Subject: [PATCH v4] HID: hid-multitouch: forwards MSC_TIMESTAMP
> 
> Computes the device timestamp according to the specification.
> It also ensures that if the time between two events is greater
> than MAX_TIMESTAMP_INTERVAL, the timestamp will be reset.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 46 +++++++++++++++++++++++++++++++++++++++++---
>  include/linux/hid.h          |  1 +
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index caf0f0b..3f8432d 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -32,6 +32,7 @@
>  #include <linux/slab.h>
>  #include <linux/usb.h>
>  #include <linux/input/mt.h>
> +#include <linux/jiffies.h>

Why?

>  #include "usbhid/usbhid.h"
>  
>  
> @@ -98,6 +99,9 @@ struct mt_device {
>  	bool serial_maybe;	/* need to check for serial protocol */
>  	bool curvalid;		/* is the current contact valid? */
>  	unsigned mt_flags;	/* flags to pass to input-mt */
> +	__s32 dev_time;		/* the scan time provided by the device */
> +	unsigned long jiffies;	/* the frame's jiffies */
> +	unsigned timestamp;	/* the timestamp to be sent */

Why not just dev_time here?

>  };
>  
>  /* classes of device behavior */
> @@ -126,6 +130,8 @@ struct mt_device {
>  #define MT_DEFAULT_MAXCONTACT	10
>  #define MT_MAX_MAXCONTACT	250
>  
> +#define MAX_TIMESTAMP_INTERVAL	500000
> +

Needs to be done in userland anyways, so no need to check this in the kernel.

>  #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)
>  
> @@ -451,12 +457,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_MSC, MSC_TIMESTAMP);
> +			set_bit(MSC_TIMESTAMP, hi->input->mscbit);
> +			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;
>  		}
> @@ -485,7 +498,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>  		struct hid_field *field, struct hid_usage *usage,
>  		unsigned long **bit, int *max)
>  {
> -	if (usage->type == EV_KEY || usage->type == EV_ABS)
> +	if (usage->type == EV_KEY || usage->type == EV_ABS ||
> +	    usage->type == EV_MSC)
>  		set_bit(usage->type, hi->input->evbit);
>  
>  	return -1;
> @@ -565,11 +579,34 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>   */
>  static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
>  {
> +	input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp);

I think this should go after the frame sync,

>  	input_mt_sync_frame(input);

And the computation (100 * td->dev_time) should work fine. It will wrap
properly.

>  	input_sync(input);
>  	td->num_received = 0;
>  }
>  
> +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
> +		__s32 value)
> +{
> +	long delta = value - td->dev_time;
> +	unsigned long jdelta = jiffies_to_usecs(jiffies - td->jiffies);
> +
> +	td->jiffies = jiffies;
> +	td->dev_time = value;
> +
> +	if (delta < 0)
> +		delta += field->logical_maximum;
> +
> +	/* HID_DG_SCANTIME is expressed in 100us, we want it in ms. */
> +	delta *= 100;
> +
> +	if (abs(delta - jdelta) > MAX_TIMESTAMP_INTERVAL)
> +		/* obviously wrong clock -> the device time has been reset */
> +		td->timestamp = 0;
> +	else
> +		td->timestamp += delta;
> +}
> +

Can be skipped entirely.

>  static int mt_event(struct hid_device *hid, struct hid_field *field,
>  				struct hid_usage *usage, __s32 value)
>  {
> @@ -617,6 +654,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>  		case HID_DG_HEIGHT:
>  			td->curdata.h = value;
>  			break;
> +		case HID_DG_SCANTIME:
> +			mt_compute_timestamp(td, field, value);

Just td->dev_time = value should work fine here.

> +			break;
>  		case HID_DG_CONTACTCOUNT:
>  			/*
>  			 * Includes multi-packet support where subsequent
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 6b4f322..0337e50 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

Was this missing this the old patch, or did it get moved here?

>  
>  /*
>   * HID report types --- Ouch! HID spec says 1 2 3!
> -- 
> 1.8.0

Thanks,
Henrik

Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices

[Benjamin Tissoires]

Hi Henrik,

thanks for the review of the patchset. I'll do my best for the next version :)

On Tue, Nov 13, 2012 at 5:42 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Wed, Nov 07, 2012 at 05:37:34PM +0100, Benjamin Tissoires wrote:
>> Win8 devices supporting hovering must provides InRange HID field.
>
> provide the
>
>> 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.
>
> In the absence of more detailed information, use ABS_MT_DISTANCE with
> a [0,1] interval to distinguish between presence (1) and touch (0).
>
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 24 ++++++++++++++++++++----
>>  1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index b393c6c..1f3d1e0 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -59,6 +59,7 @@ struct mt_slot {
>>       __s32 x, y, cx, cy, p, w, h;
>>       __s32 contactid;        /* the device ContactID assigned to this slot */
>>       bool touch_state;       /* is the touch valid? */
>> +     bool inrange_state;     /* is the finger in proximity of the sensor? */
>>  };
>>
>>  struct mt_class {
>> @@ -397,6 +398,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, -1, 1, 0, 0);
>
> Why [-1,1] here?

At first, I only used [0,1]. However, it's still the same problem with
the information being kept between the touches:
if you start an application after having touched your device, you
normally have to ask for the different per-touche states to get x, y,
distance, pressure, etc...
However, this is not much mandatory because the protocol in its
current form ensures that you will get the new states of the axes when
a new touch occurs.

ABS_MT_DISTANCE is a little bit different here because the protocol
guarantees that once you get the "not in range" state through
tracking_id == -1, distance should be 1.
When the new touch of the very same slot occurs, you also have the
guarantee that distance is at 1 too.

So basically, if you don't ask for the slot states, you will never get
that very first distance.

I know that it's a user-space problem, but honestly, I don't want to
fix several user-space problems when we could fix it through the
protocol.

I can see 2 solutions:
- set the range to: [0,1] and still sending -1 (or 2) for the invalid
distance value (if it's not clamped)
- set the range to: [0,2] and having: 0 for touch, 1 for hovering, and
2 for not in range

Does that make sense?

>
>> +                     }
>>                       mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>> @@ -516,6 +523,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>               if (slotnum < 0 || slotnum >= td->maxcontacts)
>>                       return;
>>
>> +             if (!test_bit(ABS_MT_DISTANCE, input->absbit))
>> +                     /* If InRange is not present, rely on TipSwitch */
>> +                     s->inrange_state = s->touch_state;
>> +
>
> This could be skipped, see below.
>
>>               if (td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES &&
>>                   input_mt_is_active(&input->mt->slots[slotnum]) &&
>>                   input_mt_is_used(input->mt, &input->mt->slots[slotnum]))
>> @@ -523,9 +534,9 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>
>>               input_mt_slot(input, slotnum);
>>               input_mt_report_slot_state(input, MT_TOOL_FINGER,
>> -                     s->touch_state);
>> -             if (s->touch_state) {
>> -                     /* this finger is on the screen */
>> +                     s->inrange_state);
>> +             if (s->inrange_state) {
>> +                     /* this finger is in proximity of the sensor */
>
> Using (s->touch_state || s->inrange_state) here seems simpler.

agree.

>
>>                       int wide = (s->w > s->h);
>>                       /* divided by two to match visual scale of touch */
>>                       int major = max(s->w, s->h) >> 1;
>> @@ -535,11 +546,14 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>                       input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
>>                       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_DISTANCE,
>> +                             !s->touch_state);
>>                       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);
>>                       input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
>> -             }
>> +             } else
>> +                     input_event(input, EV_ABS, ABS_MT_DISTANCE, -1);
>
> Just skip this, please.

see above.

Cheers,
Benjamin

>
>>       }
>>
>>       td->num_received++;
>> @@ -567,6 +581,8 @@ 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.inrange_state = value;
>>                       break;
>>               case HID_DG_TIPSWITCH:
>>                       if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik

Re: [PATCH v3 00/13] Win 8 support for digitizers

[Henrik Rydberg]

Hi Benjamin,

> > here is the third version of this patchset.
> > I think I included most of Henrik's comments.
> > 
> > Happy reviewing :)

thanks for the changes :-)

Jiri,

> I am fine with the HID-specific changes (and I have sent out my Acks to 
> the invidivual patches).

Looks reasonable to me too (although I have not looked through the exponent
code enough to want to formally ack it).

> For hid-multitouch changes, I'd like to have Reviewed-by: from Henrik as 
> well. Henrik, are you planning to review this patchset, please?

Yes, and finally done.

Cheers,
Henrik

Re: [PATCH v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME

[Benjamin Tissoires]

Hi Henrik,

On Tue, Nov 13, 2012 at 6:27 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> Date: Tue, 13 Nov 2012 15:12:17 +0100
>> Subject: [PATCH v4] HID: hid-multitouch: forwards MSC_TIMESTAMP
>>
>> Computes the device timestamp according to the specification.
>> It also ensures that if the time between two events is greater
>> than MAX_TIMESTAMP_INTERVAL, the timestamp will be reset.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 46 +++++++++++++++++++++++++++++++++++++++++---
>>  include/linux/hid.h          |  1 +
>>  2 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index caf0f0b..3f8432d 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/usb.h>
>>  #include <linux/input/mt.h>
>> +#include <linux/jiffies.h>
>
> Why?
>
>>  #include "usbhid/usbhid.h"
>>
>>
>> @@ -98,6 +99,9 @@ struct mt_device {
>>       bool serial_maybe;      /* need to check for serial protocol */
>>       bool curvalid;          /* is the current contact valid? */
>>       unsigned mt_flags;      /* flags to pass to input-mt */
>> +     __s32 dev_time;         /* the scan time provided by the device */
>> +     unsigned long jiffies;  /* the frame's jiffies */
>> +     unsigned timestamp;     /* the timestamp to be sent */
>
> Why not just dev_time here?

because max dev_time is at least 65535 according to the norm, and the
win 8 device I have has his max value of 65535.
Which means that every 6 seconds and a half the counter resets, and
the value is not properly reset in the way I understand the
specification. The device just forwards an internal clock that is
never reset.
So if you wait 653500 us + 8ms, everything happens as if the device
sent this frame right after the previous one (you get the same value).

I think that it's the job of the kernel to provide clean and coherent
values through evdev, which won't be the case if the jiffies thing is
not in place: every devices will have a different behavior, leading to
complicate things in the user-space.

>
>>  };
>>
>>  /* classes of device behavior */
>> @@ -126,6 +130,8 @@ struct mt_device {
>>  #define MT_DEFAULT_MAXCONTACT        10
>>  #define MT_MAX_MAXCONTACT    250
>>
>> +#define MAX_TIMESTAMP_INTERVAL       500000
>> +
>
> Needs to be done in userland anyways, so no need to check this in the kernel.
>
>>  #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)
>>
>> @@ -451,12 +457,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_MSC, MSC_TIMESTAMP);
>> +                     set_bit(MSC_TIMESTAMP, hi->input->mscbit);
>> +                     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;
>>               }
>> @@ -485,7 +498,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>>               struct hid_field *field, struct hid_usage *usage,
>>               unsigned long **bit, int *max)
>>  {
>> -     if (usage->type == EV_KEY || usage->type == EV_ABS)
>> +     if (usage->type == EV_KEY || usage->type == EV_ABS ||
>> +         usage->type == EV_MSC)
>>               set_bit(usage->type, hi->input->evbit);
>>
>>       return -1;
>> @@ -565,11 +579,34 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>   */
>>  static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
>>  {
>> +     input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp);
>
> I think this should go after the frame sync,
>
>>       input_mt_sync_frame(input);
>
> And the computation (100 * td->dev_time) should work fine. It will wrap
> properly.
>
>>       input_sync(input);
>>       td->num_received = 0;
>>  }
>>
>> +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
>> +             __s32 value)
>> +{
>> +     long delta = value - td->dev_time;
>> +     unsigned long jdelta = jiffies_to_usecs(jiffies - td->jiffies);
>> +
>> +     td->jiffies = jiffies;
>> +     td->dev_time = value;
>> +
>> +     if (delta < 0)
>> +             delta += field->logical_maximum;
>> +
>> +     /* HID_DG_SCANTIME is expressed in 100us, we want it in ms. */
>> +     delta *= 100;
>> +
>> +     if (abs(delta - jdelta) > MAX_TIMESTAMP_INTERVAL)
>> +             /* obviously wrong clock -> the device time has been reset */
>> +             td->timestamp = 0;
>> +     else
>> +             td->timestamp += delta;
>> +}
>> +
>
> Can be skipped entirely.
>
>>  static int mt_event(struct hid_device *hid, struct hid_field *field,
>>                               struct hid_usage *usage, __s32 value)
>>  {
>> @@ -617,6 +654,9 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>               case HID_DG_HEIGHT:
>>                       td->curdata.h = value;
>>                       break;
>> +             case HID_DG_SCANTIME:
>> +                     mt_compute_timestamp(td, field, value);
>
> Just td->dev_time = value should work fine here.
>
>> +                     break;
>>               case HID_DG_CONTACTCOUNT:
>>                       /*
>>                        * Includes multi-packet support where subsequent
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 6b4f322..0337e50 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
>
> Was this missing this the old patch, or did it get moved here?

Sorry for that. I moved it there because I cleaned a little the other
patch by removing the hid things.

Cheers,
Benjamin

>
>>
>>  /*
>>   * HID report types --- Ouch! HID spec says 1 2 3!
>> --
>> 1.8.0
>
> Thanks,
> Henrik

Re: [PATCH v3 00/13] Win 8 support for digitizers

[Benjamin Tissoires]

Hi Henrik,

On Tue, Nov 13, 2012 at 6:33 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> > here is the third version of this patchset.
>> > I think I included most of Henrik's comments.
>> >
>> > Happy reviewing :)
>
> thanks for the changes :-)

and thanks for the review.

>
> Jiri,
>
>> I am fine with the HID-specific changes (and I have sent out my Acks to
>> the invidivual patches).
>
> Looks reasonable to me too (although I have not looked through the exponent
> code enough to want to formally ack it).
>
>> For hid-multitouch changes, I'd like to have Reviewed-by: from Henrik as
>> well. Henrik, are you planning to review this patchset, please?
>
> Yes, and finally done.
>
> Cheers,
> Henrik

Jiri,

I'll send a new patchset tomorrow with the different acked, reviewed
and requested changes.

Some patches at the end may need an other review, so I'll make sure to
get the reviewed patches at the beginning of the set so that you can
pick them for 3.8.
It will also allow me not to send dozens of patches in a row :)

Is it good for you?

Cheers,
Benjamin

Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices

[Henrik Rydberg]

Hi Benjamin,

> > Why [-1,1] here?
> 
> At first, I only used [0,1]. However, it's still the same problem with
> the information being kept between the touches:
> if you start an application after having touched your device, you
> normally have to ask for the different per-touche states to get x, y,
> distance, pressure, etc...
> However, this is not much mandatory because the protocol in its
> current form ensures that you will get the new states of the axes when
> a new touch occurs.

Right, the stateful communication requires a full state read after
opening the deivce, although in most cases this is not really
necessary. This is no different for ABS_MT_DISTANCE, of course.

> ABS_MT_DISTANCE is a little bit different here because the protocol
> guarantees that once you get the "not in range" state through
> tracking_id == -1, distance should be 1.
> When the new touch of the very same slot occurs, you also have the
> guarantee that distance is at 1 too.

ABS_MT_DISTANCE is just another attribute of the slot, so it really is
no different.

> So basically, if you don't ask for the slot states, you will never get
> that very first distance.

Which will not be important either; as long as the slot is unused, it
does not matter what the attributes of that slot are.

> I know that it's a user-space problem, but honestly, I don't want to
> fix several user-space problems when we could fix it through the
> protocol.
> 
> I can see 2 solutions:
> - set the range to: [0,1] and still sending -1 (or 2) for the invalid
> distance value (if it's not clamped)
> - set the range to: [0,2] and having: 0 for touch, 1 for hovering, and
> 2 for not in range
> 
> Does that make sense?

I just do not see what problem you want to solve here.

Thanks,
Henrik

Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices

[Benjamin Tissoires]

On Tue, Nov 13, 2012 at 7:04 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> > Why [-1,1] here?
>>
>> At first, I only used [0,1]. However, it's still the same problem with
>> the information being kept between the touches:
>> if you start an application after having touched your device, you
>> normally have to ask for the different per-touche states to get x, y,
>> distance, pressure, etc...
>> However, this is not much mandatory because the protocol in its
>> current form ensures that you will get the new states of the axes when
>> a new touch occurs.
>
> Right, the stateful communication requires a full state read after
> opening the deivce, although in most cases this is not really
> necessary. This is no different for ABS_MT_DISTANCE, of course.
>
>> ABS_MT_DISTANCE is a little bit different here because the protocol
>> guarantees that once you get the "not in range" state through
>> tracking_id == -1, distance should be 1.
>> When the new touch of the very same slot occurs, you also have the
>> guarantee that distance is at 1 too.
>
> ABS_MT_DISTANCE is just another attribute of the slot, so it really is
> no different.
>
>> So basically, if you don't ask for the slot states, you will never get
>> that very first distance.
>
> Which will not be important either; as long as the slot is unused, it
> does not matter what the attributes of that slot are.

And if the slot is unused, but has been used since the plug of the
device, the state is kept between the touch.

>
>> I know that it's a user-space problem, but honestly, I don't want to
>> fix several user-space problems when we could fix it through the
>> protocol.
>>
>> I can see 2 solutions:
>> - set the range to: [0,1] and still sending -1 (or 2) for the invalid
>> distance value (if it's not clamped)
>> - set the range to: [0,2] and having: 0 for touch, 1 for hovering, and
>> 2 for not in range
>>
>> Does that make sense?
>
> I just do not see what problem you want to solve here.

I just intend to force the update of the distance value at the
beginning of the touch.
This is just to send a coherent state when the finger goes in range,
and to make sure that user-space application do not rely on the
undefined value (0) whereas the kernel thought it was set to 1.

Cheers,
Benjamin

>
> Thanks,
> Henrik

Re: [PATCH v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME

[Henrik Rydberg]

> >> @@ -98,6 +99,9 @@ struct mt_device {
> >>       bool serial_maybe;      /* need to check for serial protocol */
> >>       bool curvalid;          /* is the current contact valid? */
> >>       unsigned mt_flags;      /* flags to pass to input-mt */
> >> +     __s32 dev_time;         /* the scan time provided by the device */
> >> +     unsigned long jiffies;  /* the frame's jiffies */
> >> +     unsigned timestamp;     /* the timestamp to be sent */
> >
> > Why not just dev_time here?
> 
> because max dev_time is at least 65535 according to the norm, and the
> win 8 device I have has his max value of 65535.
> Which means that every 6 seconds and a half the counter resets, and
> the value is not properly reset in the way I understand the
> specification. The device just forwards an internal clock that is
> never reset.

Ok, I though it was a 32-bit value, and that it would wrap with a
longer period. It does not change the essence of the definition,
though. If we say "seconds" instead of "hours", we should still be
fine, no?

> So if you wait 653500 us + 8ms, everything happens as if the device
> sent this frame right after the previous one (you get the same value).

Yes, but we have this effect on a 32-bit counter as well.

> I think that it's the job of the kernel to provide clean and coherent
> values through evdev, which won't be the case if the jiffies thing is
> not in place: every devices will have a different behavior, leading to
> complicate things in the user-space.

The whole point is to provide the device clock to userland when it
exists, isn't it? Thus, the jiffies would never be used. If a future
device needs additions to work conformly, we just have to deal with it
at that point in time.

To conclude, we obviously have devices with a rather short wrap-around
time. However, since the normal inter-frame time is in the millisecond
range, it should not be overly restrictive to change the definition of
the minimum wraparound time from hours to seconds.

Thanks,
Henrik

Re: [PATCH v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME

[Henrik Rydberg]

> To conclude, we obviously have devices with a rather short wrap-around
> time. However, since the normal inter-frame time is in the millisecond
> range, it should not be overly restrictive to change the definition of
> the minimum wraparound time from hours to seconds.

Sorry, Benjamin, you are right about the counter. To be useful, we do
need to know the number of bits of the counter, so that differences
can be computed properly. In other words, either ABS_SCAN_TIME is
be the better choice, or we need to make sure that the counter wraps
on the full 32 bit boundary.

Henrik

Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices

[Henrik Rydberg]

> I just intend to force the update of the distance value at the
> beginning of the touch.
> This is just to send a coherent state when the finger goes in range,
> and to make sure that user-space application do not rely on the
> undefined value (0) whereas the kernel thought it was set to 1.

Right, so we use EVIOCGMTSLOTS. This "problem" occurs with
ABS_MT_ORIENTATION as well.

Thanks,
Henrik