[PATCH v4 00/14] Win 8 support for digitizers

[PATCH v4 00/14] Win 8 support for digitizers

[Benjamin Tissoires]

Hi Guys,

here is the forth version of this patchset.

* patches 1-9 has already been reviewed and are ready for inclusion I would say.
* Jiri, I kept your ack on patch 4 even if I changed the place of the comment in hid.h
* patch 10 is half new as it is splitted from a patch of the v3
* patch 11 has been changed according to Henrik's comments
* patches 12-13 have been splitted since v3 to introduce QUIRK_HOVERING and setup WIN 8
devices in a better way.
* patch 14 has been copied from the v3 as Dmitry wanted to use a MSC event for the timestamp.

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.

v4 changes:
* introduced QUIRK_HOVERING for hovering devices (not necessarily win 8 ones)
* removed QUIRK_WIN8_CERTIFIED as it's not relevant anymore
* made the change in input-mt.c
* add a test against input->mt != null

Benjamin Tissoires (14):
  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
  Input: introduce EV_MSC Timestamp
  Input: mt: add input_mt_is_used
  HID: hid-multitouch: add MT_QUIRK_IGNORE_DUPLICATES
  HID: hid-multitouch: support for hovering devices
  HID: hid-multitouch: fix Win 8 protocol
  HID: hid-multitouch: forwards MSC_TIMESTAMP

 Documentation/input/event-codes.txt |  11 +++
 drivers/hid/hid-core.c              |  20 ++++-
 drivers/hid/hid-input.c             |  24 ++++--
 drivers/hid/hid-multitouch.c        | 155 ++++++++++++++++++++++++++++++------
 drivers/input/input-mt.c            |   2 +-
 include/linux/hid.h                 |   4 +
 include/linux/input.h               |   1 +
 include/linux/input/mt.h            |   6 ++
 8 files changed, 191 insertions(+), 32 deletions(-)

-- 
1.8.0

[PATCH v4 01/14] 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>
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.8.0

[PATCH v4 02/14] 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>
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.8.0

[PATCH v4 03/14] 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>
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.8.0

[PATCH v4 04/14] 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>
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..ce14c32 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -379,6 +379,7 @@ struct hid_collection {
 struct hid_usage {
 	unsigned  hid;			/* hid usage code */
 	unsigned  collection_index;	/* index into collection array */
+	unsigned  usage_index;		/* index into usage array */
 	/* hidinput data */
 	__u16     code;			/* input driver code */
 	__u8      type;			/* input driver type */
-- 
1.8.0

[PATCH v4 05/14] 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>
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
---
 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.8.0

[PATCH v4 06/14] 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.8.0

[PATCH v4 07/14] 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>
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
---
 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.8.0

[PATCH v4 08/14] 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>
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
---
 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.8.0

[PATCH v4 09/14] Input: introduce EV_MSC Timestamp

[Benjamin Tissoires]

Some devices provides the actual timestamp (hid_dg_scan_time in win8 ones)
computed by the hardware itself. This value is global to the frame and is
not specific to the multitouch protocol.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
---
 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..f1ea2c6 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 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 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

Re: [PATCH v4 09/14] Input: introduce EV_MSC Timestamp

[Dmitry Torokhov]

On Wed, Nov 14, 2012 at 04:59:21PM +0100, Benjamin Tissoires wrote:
> Some devices provides the actual timestamp (hid_dg_scan_time in win8 ones)
> computed by the hardware itself. This value is global to the frame and is
> not specific to the multitouch protocol.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Reviewed-by: Henrik Rydberg <rydberg@euromail.se>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I take it will go through Jiri's tree, right?

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

-- 
Dmitry

Re: [PATCH v4 09/14] Input: introduce EV_MSC Timestamp

[Jiri Kosina]

On Wed, 14 Nov 2012, Dmitry Torokhov wrote:

> On Wed, Nov 14, 2012 at 04:59:21PM +0100, Benjamin Tissoires wrote:
> > Some devices provides the actual timestamp (hid_dg_scan_time in win8 ones)
> > computed by the hardware itself. This value is global to the frame and is
> > not specific to the multitouch protocol.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> I take it will go through Jiri's tree, right?

Yup, I am taking it together with the rest of the series, and including 
your Ack. Thanks a lot.

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

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 09/14] Input: introduce EV_MSC Timestamp

[Henrik Rydberg]

On Wed, Nov 14, 2012 at 04:59:21PM +0100, Benjamin Tissoires wrote:
> Some devices provides the actual timestamp (hid_dg_scan_time in win8 ones)
> computed by the hardware itself. This value is global to the frame and is
> not specific to the multitouch protocol.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
> ---

Does not directly apply to 3.7 due to the uapi changes, but trivial enough.

Henrik

[PATCH v4 10/14] Input: mt: add input_mt_is_used

[Benjamin Tissoires]

This patch extracts the test (slot->frame == mt->frame) so that it can
be used in third party drivers.

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

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index c0ec7d4..475b9d4 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -247,7 +247,7 @@ void input_mt_sync_frame(struct input_dev *dev)
 
 	if (mt->flags & INPUT_MT_DROP_UNUSED) {
 		for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
-			if (s->frame == mt->frame)
+			if (input_mt_is_used(mt, s))
 				continue;
 			input_mt_slot(dev, s - mt->slots);
 			input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
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.8.0

Re: [PATCH v4 10/14] Input: mt: add input_mt_is_used

[Henrik Rydberg]

On Wed, Nov 14, 2012 at 04:59:22PM +0100, Benjamin Tissoires wrote:
> This patch extracts the test (slot->frame == mt->frame) so that it can
> be used in third party drivers.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/input/input-mt.c | 2 +-
>  include/linux/input/mt.h | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index c0ec7d4..475b9d4 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -247,7 +247,7 @@ void input_mt_sync_frame(struct input_dev *dev)
>  
>  	if (mt->flags & INPUT_MT_DROP_UNUSED) {
>  		for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
> -			if (s->frame == mt->frame)
> +			if (input_mt_is_used(mt, s))
>  				continue;
>  			input_mt_slot(dev, s - mt->slots);
>  			input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
> 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.8.0
> 

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

Thanks,
Henrik

[PATCH v4 11/14] HID: hid-multitouch: add MT_QUIRK_IGNORE_DUPLICATES

[Benjamin Tissoires]

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2352770..c5b81a7 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;
@@ -506,10 +507,18 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
+		struct input_mt *mt = input->mt;
 
 		if (slotnum < 0 || slotnum >= td->maxcontacts)
 			return;
 
+		if ((td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES) && mt) {
+			struct input_mt_slot *slot = &mt->slots[slotnum];
+			if (input_mt_is_active(slot) &&
+			    input_mt_is_used(mt, slot))
+				return;
+		}
+
 		input_mt_slot(input, slotnum);
 		input_mt_report_slot_state(input, MT_TOOL_FINGER,
 			s->touch_state);
-- 
1.8.0

Re: [PATCH v4 11/14] HID: hid-multitouch: add MT_QUIRK_IGNORE_DUPLICATES

[Henrik Rydberg]

On Wed, Nov 14, 2012 at 04:59:23PM +0100, Benjamin Tissoires wrote:
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 2352770..c5b81a7 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;
> @@ -506,10 +507,18 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>  		int slotnum = mt_compute_slot(td, input);
>  		struct mt_slot *s = &td->curdata;
> +		struct input_mt *mt = input->mt;
>  
>  		if (slotnum < 0 || slotnum >= td->maxcontacts)
>  			return;
>  
> +		if ((td->mtclass.quirks & MT_QUIRK_IGNORE_DUPLICATES) && mt) {
> +			struct input_mt_slot *slot = &mt->slots[slotnum];
> +			if (input_mt_is_active(slot) &&
> +			    input_mt_is_used(mt, slot))
> +				return;
> +		}
> +

Slightly ugly, but I cannot see how to make it any prettier. :-)

>  		input_mt_slot(input, slotnum);
>  		input_mt_report_slot_state(input, MT_TOOL_FINGER,
>  			s->touch_state);
> -- 
> 1.8.0
> 

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

Thanks,
Henrik

[PATCH v4 12/14] 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 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c5b81a7..5f26b2f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -53,11 +53,13 @@ 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_HOVERING		(1 << 11)
 
 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 {
@@ -392,6 +394,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
+			if (cls->quirks & MT_QUIRK_HOVERING) {
+				hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_DISTANCE);
+				input_set_abs_params(hi->input,
+					ABS_MT_DISTANCE, 0, 1, 0, 0);
+			}
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
@@ -521,9 +529,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->touch_state || s->inrange_state);
+		if (s->touch_state || 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;
@@ -533,6 +541,8 @@ 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);
@@ -565,6 +575,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_HOVERING)
+				td->curdata.inrange_state = value;
 			break;
 		case HID_DG_TIPSWITCH:
 			if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
-- 
1.8.0

Re: [PATCH v4 12/14] HID: hid-multitouch: support for hovering devices

[Henrik Rydberg]

On Wed, Nov 14, 2012 at 04:59:24PM +0100, Benjamin Tissoires wrote:
> 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 | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c5b81a7..5f26b2f 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -53,11 +53,13 @@ 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_HOVERING		(1 << 11)
>  
>  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 {
> @@ -392,6 +394,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	case HID_UP_DIGITIZER:
>  		switch (usage->hid) {
>  		case HID_DG_INRANGE:
> +			if (cls->quirks & MT_QUIRK_HOVERING) {
> +				hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_DISTANCE);
> +				input_set_abs_params(hi->input,
> +					ABS_MT_DISTANCE, 0, 1, 0, 0);
> +			}
>  			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
> @@ -521,9 +529,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->touch_state || s->inrange_state);
> +		if (s->touch_state || 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;
> @@ -533,6 +541,8 @@ 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);
> @@ -565,6 +575,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_HOVERING)
> +				td->curdata.inrange_state = value;
>  			break;
>  		case HID_DG_TIPSWITCH:
>  			if (quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
> -- 
> 1.8.0
> 

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

Thanks,
Henrik

[PATCH v4 13/14] HID: hid-multitouch: fix Win 8 protocol

[Benjamin Tissoires]

The Win 8 protocol specify the fact that each valid touch must be reported
within a frame until it is released.
We can therefore use the always_valid quirk and dismiss reports when we see
duplicate contacts 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, 12 insertions(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5f26b2f..efae60c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -295,6 +295,18 @@ 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) {
+			/* Win 8 devices need special quirks */
+			__s32 *quirks = &td->mtclass.quirks;
+			*quirks |= MT_QUIRK_ALWAYS_VALID;
+			*quirks |= MT_QUIRK_IGNORE_DUPLICATES;
+			*quirks |= MT_QUIRK_HOVERING;
+			*quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
+			*quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
+			*quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
+		}
+		break;
 	}
 }
 
-- 
1.8.0

Re: [PATCH v4 13/14] HID: hid-multitouch: fix Win 8 protocol

[Henrik Rydberg]

On Wed, Nov 14, 2012 at 04:59:25PM +0100, Benjamin Tissoires wrote:
> The Win 8 protocol specify the fact that each valid touch must be reported
> within a frame until it is released.
> We can therefore use the always_valid quirk and dismiss reports when we see
> duplicate contacts 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, 12 insertions(+)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 5f26b2f..efae60c 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -295,6 +295,18 @@ 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) {
> +			/* Win 8 devices need special quirks */
> +			__s32 *quirks = &td->mtclass.quirks;
> +			*quirks |= MT_QUIRK_ALWAYS_VALID;
> +			*quirks |= MT_QUIRK_IGNORE_DUPLICATES;
> +			*quirks |= MT_QUIRK_HOVERING;
> +			*quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
> +			*quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
> +			*quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
> +		}
> +		break;
>  	}
>  }
>  
> -- 
> 1.8.0
> 

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

Thanks,
Henrik

[PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Benjamin Tissoires]

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 efae60c..47ccf3a 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)
 
@@ -459,12 +465,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;
 		}
@@ -493,7 +506,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;
@@ -572,10 +586,33 @@ 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_mt_sync_frame(input);
+	input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp);
 	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)
 {
@@ -623,6 +660,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 ce14c32..aaba0e7 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

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Henrik Rydberg]

Hi Benjamin,

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

This was not what I envisioned from the discussion yesterday, I was
probably too vague. Firstly, the absolute time interval checked seems
to be 500 ms, which is arbitrary. Second, the wrap should probably use
(logical_maximum + 1). Third, we are still not sure whether we should
take the 'time = 0 means reset' logic literally, I think it needs to
be tested.

In light of this, I would like to suggest the patch below instead. It
should follow the current interpretation of the definition to the
letter. I imagine very few devices will actually do anything but wrap
the counter, but that remains to be seen. In that case, we could
simplify the code further, since we will have no hope of detecting
large intervals properly anyways.

Thanks,
Henrik

>From 448808dd988e96d58b79148292fde147935305ab Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Wed, 14 Nov 2012 20:53:17 +0100
Subject: [PATCH] HID: hid-multitouch: send the device timestamp when present

Compute and relay the device timestamp when present. The counter
value range varies between devices, but the MSC_TIMESTAMP event
is always 32 bits long. A value of zero means the time since the
previous event is unknown.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-multitouch.c | 36 +++++++++++++++++++++++++++++++++---
 include/linux/hid.h          |  1 +
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 61543c0..586f9c6 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -98,6 +98,8 @@ 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 */
+	unsigned dev_time;	/* device scan time in units of 100 us */
+	unsigned timestamp;	/* the timestamp to be sent */
 };
 
 /* classes of device behavior */
@@ -458,12 +460,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;
 		case HID_DG_TOUCH:
@@ -492,7 +501,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;
@@ -571,10 +581,27 @@ 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_mt_sync_frame(input);
+	input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp);
 	input_sync(input);
 	td->num_received = 0;
 }
 
+static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
+				 unsigned value)
+{
+	unsigned dt;
+
+	if (value) {
+		dt = (value - td->dev_time) % (field->logical_maximum + 1);
+		td->timestamp += 100 * dt;
+		td->timestamp += !td->timestamp;
+	} else {
+		td->timestamp = 0;
+	}
+
+	td->dev_time = value;
+}
+
 static int mt_event(struct hid_device *hid, struct hid_field *field,
 				struct hid_usage *usage, __s32 value)
 {
@@ -622,6 +649,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 d2c42dd..eb7ec6c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -242,6 +242,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

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Benjamin Tissoires]

Hi Henrik,

On 11/14/2012 08:58 PM, Henrik Rydberg wrote:
> Hi Benjamin,
> 
>> 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>
>> ---
> 
> This was not what I envisioned from the discussion yesterday, I was
> probably too vague. Firstly, the absolute time interval checked seems
> to be 500 ms, which is arbitrary. Second, the wrap should probably use

Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s).

> (logical_maximum + 1). Third, we are still not sure whether we should
> take the 'time = 0 means reset' logic literally, I think it needs to
> be tested.

Again, the device I have never does any reset at the first touch, it just wraps the counter.
The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds.


> 
> In light of this, I would like to suggest the patch below instead. It
> should follow the current interpretation of the definition to the
> letter. I imagine very few devices will actually do anything but wrap
> the counter, but that remains to be seen. In that case, we could
> simplify the code further, since we will have no hope of detecting
> large intervals properly anyways.
> 
> Thanks,
> Henrik
> 
>  From 448808dd988e96d58b79148292fde147935305ab Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Wed, 14 Nov 2012 20:53:17 +0100
> Subject: [PATCH] HID: hid-multitouch: send the device timestamp when present
> 
> Compute and relay the device timestamp when present. The counter
> value range varies between devices, but the MSC_TIMESTAMP event
> is always 32 bits long. A value of zero means the time since the
> previous event is unknown.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>   drivers/hid/hid-multitouch.c | 36 +++++++++++++++++++++++++++++++++---
>   include/linux/hid.h          |  1 +
>   2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 61543c0..586f9c6 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -98,6 +98,8 @@ 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 */
> +	unsigned dev_time;	/* device scan time in units of 100 us */
> +	unsigned timestamp;	/* the timestamp to be sent */
>   };
>   
>   /* classes of device behavior */
> @@ -458,12 +460,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;
>   		case HID_DG_TOUCH:
> @@ -492,7 +501,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;
> @@ -571,10 +581,27 @@ 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_mt_sync_frame(input);
> +	input_event(input, EV_MSC, MSC_TIMESTAMP, td->timestamp);
>   	input_sync(input);
>   	td->num_received = 0;
>   }
>   
> +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
> +				 unsigned value)
> +{
> +	unsigned dt;
> +
> +	if (value) {
> +		dt = (value - td->dev_time) % (field->logical_maximum + 1);

The curious thing is that this is not working on my kernel:

this is the output of the following printk:
 printk(KERN_ERR "%s timestamp=%d value=%d dt=%d field->logical_maximum=%d !td->timestamp=%d\n", __func__, td->timestamp, value, dt, field->logical_maximum, !td->timestamp);

Nov 14 22:09:55 localhost kernel: [ 1288.821899] mt_compute_timestamp timestamp=6535900 value=65359 dt=93 field->logical_maximum=65535 !td->timestamp=0
Nov 14 22:09:55 localhost kernel: [ 1288.831938] mt_compute_timestamp timestamp=6545200 value=65452 dt=93 field->logical_maximum=65535 !td->timestamp=0
Nov 14 22:09:55 localhost kernel: [ 1288.841852] mt_compute_timestamp timestamp=900 value=9 dt=-65443 field->logical_maximum=65535 !td->timestamp=0
Nov 14 22:09:55 localhost kernel: [ 1288.850852] mt_compute_timestamp timestamp=10300 value=103 dt=94 field->logical_maximum=65535 !td->timestamp=0
Nov 14 22:09:55 localhost kernel: [ 1288.860839] mt_compute_timestamp timestamp=19600 value=196 dt=93 field->logical_maximum=65535 !td->timestamp=0

I know I cheated when I put "dt=%d" for an unsigned, but the fact is that the counter rolls back...

However, his construction works:

static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
		__s32 value)
{
	__s32 dt;

	if (value) {
		dt = value - td->dev_time;
		if (dt < 0)
			dt += field->logical_maximum + 1;
		td->timestamp += 100 * dt;
	} else {
		td->timestamp = 0;
	}

	td->dev_time = value;
}


> +		td->timestamp += 100 * dt;
> +		td->timestamp += !td->timestamp;

I don't understand the meaning of adding !td->timestamp. :/

> +	} else {
> +		td->timestamp = 0;

My particular device never falls into this case... So I never reset the timestamp.
This is problematic because we compute the timestamp on an unsigned, and the struct input_absinfo sends __s32... I'm afraid we will have some troubles after a long use of the device.

Anyway, many thanks for the review of this whole patchset!

Cheers,
Benjamin


> +	}
> +
> +	td->dev_time = value;
> +}
> +
>   static int mt_event(struct hid_device *hid, struct hid_field *field,
>   				struct hid_usage *usage, __s32 value)
>   {
> @@ -622,6 +649,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 d2c42dd..eb7ec6c 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -242,6 +242,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!
> 

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Henrik Rydberg]

Hi Benjamin,

> > This was not what I envisioned from the discussion yesterday, I was
> > probably too vague. Firstly, the absolute time interval checked seems
> > to be 500 ms, which is arbitrary. Second, the wrap should probably use
> 
> Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s).

Sorry about that, but the argument remains; it should depend on logical_maximum.

> > (logical_maximum + 1). Third, we are still not sure whether we should
> > take the 'time = 0 means reset' logic literally, I think it needs to
> > be tested.
> 
> Again, the device I have never does any reset at the first touch, it just wraps the counter.
> The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds.

So the very first win8 device we test already diverges from the win8
specification, how nice.

Ok, you have conviced me that comparing to a proper time makes
sense. However, I am not happy about using a separate time for this,
given that we will fill in the event time later in the
chain.

In addition, it would make perfect sense to extend the validity of the
hardware time with the event time for longer intervals. The relative
error only makes a difference on milisecond intervals.

A patch that seamlessly extends the validity of the hardware time,
ideally using the event time, seems like a viable solution.

> > +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
> > +				 unsigned value)
> > +{
> > +	unsigned dt;
> > +
> > +	if (value) {
> > +		dt = (value - td->dev_time) % (field->logical_maximum + 1);
> 
> The curious thing is that this is not working on my kernel:

Oups, I suspect (value - td->dev_time) needs to be explicitly converted to unsigned.

> I don't understand the meaning of adding !td->timestamp. :/

With the definition that timestamp == 0 means an unknown interval, we
do not want to send that value by accident.

Thanks,
Henrik

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Benjamin Tissoires]

Hi Henrik,

On Fri, Nov 16, 2012 at 9:09 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> > This was not what I envisioned from the discussion yesterday, I was
>> > probably too vague. Firstly, the absolute time interval checked seems
>> > to be 500 ms, which is arbitrary. Second, the wrap should probably use
>>
>> Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s).
>
> Sorry about that, but the argument remains; it should depend on logical_maximum.

I must be tired when I read your mail, I only understand now what you
meant by logical_maximum in your first mail... sorry.

So, totally agree, we could make this kind of thing depending on logical_max.

>
>> > (logical_maximum + 1). Third, we are still not sure whether we should
>> > take the 'time = 0 means reset' logic literally, I think it needs to
>> > be tested.
>>
>> Again, the device I have never does any reset at the first touch, it just wraps the counter.
>> The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds.
>
> So the very first win8 device we test already diverges from the win8
> specification, how nice.

hehe... well, the scan time is not as critical as can be the other
fields. Anyway, it's fun :)

>
> Ok, you have conviced me that comparing to a proper time makes
> sense. However, I am not happy about using a separate time for this,
> given that we will fill in the event time later in the
> chain.

that may explains the delta I observed from the kernel time and the device time.

>
> In addition, it would make perfect sense to extend the validity of the
> hardware time with the event time for longer intervals. The relative
> error only makes a difference on milisecond intervals.
>
> A patch that seamlessly extends the validity of the hardware time,
> ideally using the event time, seems like a viable solution.

Just to be sure:
When I receive scan_time, I increment timestamp with the device value.
When not, I find out the number of counter wrap I missed with the
kernel timer (jiffies) to get the actual device time.

Is that ok for you?

>
>> > +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
>> > +                            unsigned value)
>> > +{
>> > +   unsigned dt;
>> > +
>> > +   if (value) {
>> > +           dt = (value - td->dev_time) % (field->logical_maximum + 1);
>>
>> The curious thing is that this is not working on my kernel:
>
> Oups, I suspect (value - td->dev_time) needs to be explicitly converted to unsigned.

I can't remember if I tried this one, but I'll try to make it one line
in the next version.

>
>> I don't understand the meaning of adding !td->timestamp. :/
>
> With the definition that timestamp == 0 means an unknown interval, we
> do not want to send that value by accident.

ok, makes sense.

Cheers,
Benjamin

>
> Thanks,
> Henrik

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Henrik Rydberg]

Hi Benjamin,

> > In addition, it would make perfect sense to extend the validity of the
> > hardware time with the event time for longer intervals. The relative
> > error only makes a difference on milisecond intervals.
> >
> > A patch that seamlessly extends the validity of the hardware time,
> > ideally using the event time, seems like a viable solution.
> 
> Just to be sure:
> When I receive scan_time, I increment timestamp with the device value.
> When not, I find out the number of counter wrap I missed with the
> kernel timer (jiffies) to get the actual device time.
> 
> Is that ok for you?

If the device has no scan time, then no scan time is reported, of
course.  If the scan time delta _and_ the actual time delta are both
below logical_max (say), then increase the counter with the scan time
delta. Else, if the actual time delta is below 2^31, increase the
counter with the actual time delta. Else set the counter to zero.

This ought to give us the sought-after accuracy for small times, the
sought-after extension for intermediate times, and a clear definition
of unknown time.

Cheers,
Henrik

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Henrik Rydberg]

> below logical_max (say),

That was meant to read (logical_max / 2).

Henrik

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Benjamin Tissoires]

Hi Guys,

well, I'm not very satisfied with this patch. I first thought it was a
good idea but I can see now several cons:
1. Henrik would like to partially base the time spent between two
events when the device wraps on the *event* time. This is a great idea
for consistency, but I'm afraid I won't be able to implement it
because this time is computed *after* we call input_event and is only
used by evdev. Thus, I still need to add an other clock and some
differences may occur.
2. the user space (at least X) will not use it before a long time:
they already have the time of the event and it will not add that much
consistency.
3. it will wake up the whole input chain when fingers are present but
no moves occurs on the digitizer: the only events we get are
MSC_TIMESTAMP and EV_SYN and the user-space will be awaken just for
that.
4. MSC_TIMESTAMP does not have an abs_max value, thus we are forced to
compute sth consistent in the kernel that can be forwarded to the user
space.

So, I propose not to include this feature, and eventually reverting
the patch that introduced MSC_TIMESTAMP as it's useless if we don't
use it right now.

Jiri, Dmitry, Henrik, are ok with that?

Cheers,
Benjamin

On Tue, Nov 20, 2012 at 9:54 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> below logical_max (say),
>
> That was meant to read (logical_max / 2).
>
> Henrik

Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP

[Henrik Rydberg]

Hi Benjamin,

> well, I'm not very satisfied with this patch. I first thought it was a
> good idea but I can see now several cons:
> 1. Henrik would like to partially base the time spent between two
> events when the device wraps on the *event* time. This is a great idea
> for consistency, but I'm afraid I won't be able to implement it
> because this time is computed *after* we call input_event and is only
> used by evdev. Thus, I still need to add an other clock and some
> differences may occur.

Alternatively, device times need to become part of input core.

> 2. the user space (at least X) will not use it before a long time:
> they already have the time of the event and it will not add that much
> consistency.

Ok.

> 3. it will wake up the whole input chain when fingers are present but
> no moves occurs on the digitizer: the only events we get are
> MSC_TIMESTAMP and EV_SYN and the user-space will be awaken just for
> that.

Good point, and a second argument for including this in the input core.

> 4. MSC_TIMESTAMP does not have an abs_max value, thus we are forced to
> compute sth consistent in the kernel that can be forwarded to the user
> space.

Granted, but we do not really lose anything by doing so.

> So, I propose not to include this feature, and eventually reverting
> the patch that introduced MSC_TIMESTAMP as it's useless if we don't
> use it right now.
> 
> Jiri, Dmitry, Henrik, are ok with that?

I think it is fine to postpone the patch, but based on the comments
above, I do not think we need to revert the input definition.

Thanks,
Henrik

Re: [PATCH v4 00/14] Win 8 support for digitizers

[Henrik Rydberg]

Hi Benjamin,

> * patches 1-9 has already been reviewed and are ready for inclusion I would say.
> * Jiri, I kept your ack on patch 4 even if I changed the place of the comment in hid.h
> * patch 10 is half new as it is splitted from a patch of the v3
> * patch 11 has been changed according to Henrik's comments
> * patches 12-13 have been splitted since v3 to introduce QUIRK_HOVERING and setup WIN 8
> devices in a better way.
> * patch 14 has been copied from the v3 as Dmitry wanted to use a MSC event for the timestamp.

Everything but the last patch looks good now, me thinks. Thanks!

Henrik

Re: [PATCH v4 00/14] Win 8 support for digitizers

[Jiri Kosina]

On Wed, 14 Nov 2012, Henrik Rydberg wrote:

> > * patches 1-9 has already been reviewed and are ready for inclusion I would say.
> > * Jiri, I kept your ack on patch 4 even if I changed the place of the comment in hid.h
> > * patch 10 is half new as it is splitted from a patch of the v3
> > * patch 11 has been changed according to Henrik's comments
> > * patches 12-13 have been splitted since v3 to introduce QUIRK_HOVERING and setup WIN 8
> > devices in a better way.
> > * patch 14 has been copied from the v3 as Dmitry wanted to use a MSC event for the timestamp.
> 
> Everything but the last patch looks good now, me thinks. Thanks!

I have now applied everything (with all the gathered Acks/Reviewed-by 
incorporated) but the patch #14.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4 00/14] Win 8 support for digitizers

[Benjamin Tissoires]

On Thu, Nov 15, 2012 at 10:33 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 14 Nov 2012, Henrik Rydberg wrote:
>
>> > * patches 1-9 has already been reviewed and are ready for inclusion I would say.
>> > * Jiri, I kept your ack on patch 4 even if I changed the place of the comment in hid.h
>> > * patch 10 is half new as it is splitted from a patch of the v3
>> > * patch 11 has been changed according to Henrik's comments
>> > * patches 12-13 have been splitted since v3 to introduce QUIRK_HOVERING and setup WIN 8
>> > devices in a better way.
>> > * patch 14 has been copied from the v3 as Dmitry wanted to use a MSC event for the timestamp.
>>
>> Everything but the last patch looks good now, me thinks. Thanks!

Hi Henrik,

Thanks for the review!

>
> I have now applied everything (with all the gathered Acks/Reviewed-by
> incorporated) but the patch #14.
>

Hi Jiri,

many thanks for applying it. Patch #14 is less critical, so we can
take more time to include it.

Cheers,
Benjamin

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