[PATCH 1/4] hid-multitouch: support for PixCir-based panels

[PATCH 1/4] hid-multitouch: support for PixCir-based panels

[Stephane Chatty]

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

Signed-off-by: Stephane Chatty <chatty@enac.fr>
Tested-by: Stephane Lajeunesse <slajeunesse@yahoo.com>
Tested-by: arjun kv <kvarjun@gmail.com>

diff -rupN a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
--- a/drivers/hid/hid-core.c	2010-10-14 00:30:43.992936922 +0200
+++ b/drivers/hid/hid-core.c	2010-10-14 01:29:28.009936912 +0200
@@ -1290,6 +1290,7 @@ static const struct hid_device_id hid_bl
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION) },
@@ -1312,6 +1313,7 @@ static const struct hid_device_id hid_bl
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
diff -rupN a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
--- a/drivers/hid/hid-ids.h	2010-10-14 00:27:48.288936922 +0200
+++ b/drivers/hid/hid-ids.h	2010-10-14 01:29:37.273937194 +0200
@@ -132,6 +132,7 @@
 #define USB_DEVICE_ID_BTC_EMPREX_REMOTE_2	0x5577
 
 #define USB_VENDOR_ID_CANDO		0x2087
+#define USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH 0x0703
 #define USB_DEVICE_ID_CANDO_MULTI_TOUCH	0x0a01
 #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6 0x0b03
 
@@ -295,6 +296,9 @@
 #define USB_DEVICE_ID_GYRATION_REMOTE_2 0x0003
 #define USB_DEVICE_ID_GYRATION_REMOTE_3 0x0008
 
+#define USB_VENDOR_ID_HANVON		0x20b3
+#define USB_DEVICE_ID_HANVON_MULTITOUCH	0x0a18
+
 #define USB_VENDOR_ID_HAPP		0x078b
 #define USB_DEVICE_ID_UGCI_DRIVING	0x0010
 #define USB_DEVICE_ID_UGCI_FLYING	0x0020
diff -rupN a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
--- a/drivers/hid/hid-multitouch.c	1970-01-01 01:00:00.000000000 +0100
+++ b/drivers/hid/hid-multitouch.c	2010-10-14 01:30:59.009937031 +0200
@@ -0,0 +1,398 @@
+/*
+ *  HID driver for multitouch panels
+ *
+ *  Copyright (c) 2010 Stephane Chatty <chatty@enac.fr>
+ *
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include "usbhid/usbhid.h"
+
+
+MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");
+MODULE_DESCRIPTION("HID multitouch panels");
+MODULE_LICENSE("GPL");
+
+#include "hid-ids.h"
+
+#define MAX_TRKID	USHRT_MAX
+
+
+struct mt_slot {
+	__u16 x, y, p;
+	bool valid;	/* did we just get valid contact data for this slot? */
+	bool prev_valid;/* was this slot previously valid/active? */
+	__u16 trkid;	/* the tracking ID that was assigned to this slot */
+};
+
+struct mt_device {
+	struct mt_class *mtclass;/* our mt device class */
+	struct mt_slot *slots;	/* buffer with all slots */
+	__u8 optfeatures;	/* what optional mt features do we get? */
+	__u8 curcontact; 	/* index of the current contact */
+	__u8 maxcontact;	/* expected last contact index */ 
+	bool curvalid; 		/* is the current contact valid? */
+	__u16 curcontactid; 	/* ContactID of the current contact */
+	__u16 curx, cury, curp;	/* other attributes of the current contact */
+	__u16 lasttrkid;	/* the last tracking ID we assigned */
+};
+
+struct mt_class {
+	int (*compute_slot)(struct mt_device *);
+	__u8 maxcontacts;
+	__s8 inputmode;	/* InputMode HID feature number, -1 if non-existent */
+};
+
+/* classes of device behavior */
+#define DUAL1 0
+
+/* contact data that only some devices report */
+#define PRESSURE 	(1 << 0)
+#define SIZE		(1 << 1)
+
+/*
+ * these device-dependent functions determine what slot corresponds
+ * to a valid contact that was just read.
+ */
+
+static int slot_from_contactid(struct mt_device *td)
+{
+	return td->curcontactid;
+}
+
+struct mt_class mt_classes[] = {
+	/* DUAL1 */		{ slot_from_contactid, 2, -1 },
+};
+
+
+static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	switch (usage->hid & HID_USAGE_PAGE) {
+
+	case HID_UP_GENDESK:
+		switch (usage->hid) {
+		case HID_GD_X:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_POSITION_X);
+			input_set_abs_params(hi->input, ABS_MT_POSITION_X,
+						field->logical_minimum,
+						field->logical_maximum, 0, 0);
+			/* touchscreen emulation */
+			input_set_abs_params(hi->input, ABS_X,
+						field->logical_minimum,
+						field->logical_maximum, 0, 0);
+			return 1;
+		case HID_GD_Y:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_POSITION_Y);
+			input_set_abs_params(hi->input, ABS_MT_POSITION_Y,
+						field->logical_minimum,
+						field->logical_maximum, 0, 0);
+			/* touchscreen emulation */
+			input_set_abs_params(hi->input, ABS_Y,
+						field->logical_minimum,
+						field->logical_maximum, 0, 0);
+			return 1;
+		}
+		return 0;
+
+	case HID_UP_DIGITIZER:
+		switch (usage->hid) {
+		case HID_DG_INRANGE:
+		case HID_DG_CONFIDENCE:
+			return -1;
+		case HID_DG_TIPSWITCH:
+			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
+			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
+			return 1;
+		case HID_DG_CONTACTID:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_TRACKING_ID);
+			input_set_abs_params(hi->input, ABS_MT_TRACKING_ID,
+						0, MAX_TRKID, 0, 0);
+			if (!hi->input->mt)
+				input_mt_create_slots(hi->input,
+						td->mtclass->maxcontacts);
+
+		case HID_DG_TIPPRESSURE:
+			hid_map_usage(hi, usage, bit, max,
+					EV_ABS, ABS_MT_PRESSURE);
+			td->optfeatures |= PRESSURE;
+			return 1;
+		case HID_DG_CONTACTCOUNT:
+		case HID_DG_CONTACTMAX:
+			return -1;
+		}
+		/* let hid-input decide for the others */
+		return 0;
+
+	case 0xff000000:
+		/* we do not want to map these: no input-oriented meaning */
+		return -1;
+	}
+
+	return 0;
+}
+
+static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
+		struct hid_field *field, struct hid_usage *usage,
+		unsigned long **bit, int *max)
+{
+	if (usage->type == EV_KEY || usage->type == EV_ABS)
+		set_bit(usage->type, hi->input->evbit);
+
+	return -1;
+}
+
+/*
+ * this function is called when a whole contact has been processed,
+ * so that it can assign it to a slot and store the data there
+ */
+static void mt_complete_slot(struct mt_device *td)
+{
+	if (td->curvalid) {
+		struct mt_slot *slot;
+		int slotnum = td->mtclass->compute_slot(td);
+		if (slotnum >= 0 && slotnum <= td->mtclass->maxcontacts - 1) {
+			slot = td->slots + slotnum;
+
+			slot->valid = true;
+			slot->x = td->curx;
+			slot->y = td->cury;
+			slot->p = td->curp;
+		}
+	}
+	td->curcontact++;
+}
+
+
+/*
+ * this function is called when a whole packet has been received and processed,
+ * so that it can decide what to send to the input layer.
+ */
+static void mt_emit_event(struct mt_device *td, struct input_dev *input)
+{
+	struct mt_slot *oldest = 0; /* touchscreen emulation: oldest touch */
+	int i;
+
+	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
+		struct mt_slot *s = td->slots + i;
+		if (!s->valid) {
+			/*
+			 * this slot does not contain useful data,
+			 * notify its closure if necessary
+			 */
+			if (s->prev_valid) {
+				input_mt_slot(input, i);	
+				input_event(input, EV_ABS,
+						ABS_MT_TRACKING_ID, -1);
+				s->prev_valid = false;
+			}
+			continue;
+		}
+		if (!s->prev_valid)
+			s->trkid = td->lasttrkid++;
+		input_mt_slot(input, i);
+		input_event(input, EV_ABS, ABS_MT_TRACKING_ID, s->trkid);
+		input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
+		input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
+		if (td->optfeatures & PRESSURE)
+			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
+		s->prev_valid = true;
+		s->valid = false;
+
+		/* touchscreen emulation: is this the oldest contact? */
+		if (!oldest || ((s->trkid - oldest->trkid) & (SHRT_MAX + 1)))
+			oldest = s;
+	}
+
+	/* touchscreen emulation */
+	if (oldest) {
+		input_event(input, EV_KEY, BTN_TOUCH, 1);
+		input_event(input, EV_ABS, ABS_X, oldest->x);
+		input_event(input, EV_ABS, ABS_Y, oldest->y);
+	} else {
+		input_event(input, EV_KEY, BTN_TOUCH, 0);
+	}
+
+	input_sync(input);
+	td->curcontact = 0;
+}
+
+
+
+static int mt_event(struct hid_device *hid, struct hid_field *field,
+				struct hid_usage *usage, __s32 value)
+{
+	struct mt_device *td = hid_get_drvdata(hid);
+
+	if (hid->claimed & HID_CLAIMED_INPUT) {
+		struct input_dev *input = field->hidinput->input;
+		switch (usage->hid) {
+		case HID_DG_INRANGE:
+			break;	
+		case HID_DG_TIPSWITCH:
+			td->curvalid = value;
+			break;
+		case HID_DG_CONFIDENCE:
+			break;
+		case HID_DG_CONTACTID:
+			td->curcontactid = value;
+			break;	
+		case HID_DG_TIPPRESSURE:
+			td->curp = value;
+			break;
+		case HID_GD_X:
+			td->curx = value;
+			break;
+		case HID_GD_Y:
+			td->cury = value;
+			/* works for devices where Y is last in a contact */
+			mt_complete_slot(td);
+			break;
+		case HID_DG_CONTACTCOUNT:
+			/*
+			 * works for devices where contact count is
+			 * the last field in a message
+			 */
+			if (value)
+				td->maxcontact = value - 1;
+			if (td->curcontact > td->maxcontact)
+				mt_emit_event(td, input);
+			break;
+		case HID_DG_CONTACTMAX:
+			break;
+
+		default:
+			/* fallback to the generic hidinput handling */
+			return 0;
+		}
+	}
+
+	/* we have handled the hidinput part, now remains hiddev */
+	if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
+		hid->hiddev_hid_event(hid, field, usage, value);
+
+	return 1;
+}
+
+static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	struct mt_device *td;
+
+#if 0
+	/*
+         * todo: activate this as soon as the patch where the quirk below
+         * is defined is commited. This will allow the driver to correctly
+         * support devices that emit events over several HID messages.
+         */
+	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+#endif
+
+	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
+	if (!td) {
+		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
+		return -ENOMEM;
+	}
+	td->mtclass = mt_classes + id->driver_data;
+	td->slots = kzalloc(td->mtclass->maxcontacts * sizeof(struct mt_slot),
+				GFP_KERNEL);
+	if (!td->slots) {
+		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
+		ret = -ENOMEM;
+		goto fail;
+	}
+	hid_set_drvdata(hdev, td);
+
+	ret = hid_parse(hdev);
+	if (ret != 0)
+		goto fail;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret != 0)
+		goto fail;
+
+	if (td->mtclass->inputmode >= 0) {
+		struct hid_report *r;
+		struct hid_report_enum *re = hdev->report_enum
+						+ HID_FEATURE_REPORT;
+		r = re->report_id_hash[td->mtclass->inputmode];
+		if (r) {
+			r->field[0]->value[0] = 0x02;
+			usbhid_submit_report(hdev, r, USB_DIR_OUT);
+		}
+	}
+	return 0;
+
+fail:
+	kfree(td);
+	return ret;
+}
+
+static void mt_remove(struct hid_device *hdev)
+{
+	struct mt_device *td = hid_get_drvdata(hdev);
+	hid_hw_stop(hdev);
+	kfree(td->slots);
+	kfree(td);
+	hid_set_drvdata(hdev, NULL);
+}
+
+static const struct hid_device_id mt_devices[] = {
+
+	/* PixCir-based panels */
+	{ .driver_data = DUAL1,
+		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
+			USB_DEVICE_ID_HANVON_MULTITOUCH) },
+	{ .driver_data = DUAL1,
+		HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
+			USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
+
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, mt_devices);
+
+static const struct hid_usage_id mt_grabbed_usages[] = {
+	{ HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
+	{ HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
+};
+
+static struct hid_driver mt_driver = {
+	.name = "hid-multitouch",
+	.id_table = mt_devices,
+	.probe = mt_probe,
+	.remove = mt_remove,
+	.input_mapping = mt_input_mapping,
+	.input_mapped = mt_input_mapped,
+	.usage_table = mt_grabbed_usages,
+	.event = mt_event,
+};
+
+static int __init mt_init(void)
+{
+	return hid_register_driver(&mt_driver);
+}
+
+static void __exit mt_exit(void)
+{
+	hid_unregister_driver(&mt_driver);
+}
+
+module_init(mt_init);
+module_exit(mt_exit);
+
diff -rupN a/drivers/hid/Kconfig b/drivers/hid/Kconfig
--- a/drivers/hid/Kconfig	2010-10-14 01:39:17.263936923 +0200
+++ b/drivers/hid/Kconfig	2010-10-14 01:42:37.964936879 +0200
@@ -264,6 +264,12 @@ config HID_MONTEREY
 	---help---
 	Support for Monterey Genius KB29E.
 
+config HID_MULTITOUCH
+	tristate "HID Multitouch panels"
+	depends on USB_HID
+	---help---
+	Generic support for HID multitouch panels.
+
 config HID_NTRIG
 	tristate "NTrig"
 	depends on USB_HID
diff -rupN a/drivers/hid/Makefile b/drivers/hid/Makefile
--- a/drivers/hid/Makefile	2010-10-14 01:38:27.873936922 +0200
+++ b/drivers/hid/Makefile	2010-10-14 01:40:25.571936977 +0200
@@ -43,6 +43,7 @@ obj-$(CONFIG_HID_MAGICMOUSE)    += hid-m
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
 obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
 obj-$(CONFIG_HID_MOSART)	+= hid-mosart.o
+obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
 obj-$(CONFIG_HID_NTRIG)		+= hid-ntrig.o
 obj-$(CONFIG_HID_ORTEK)		+= hid-ortek.o
 obj-$(CONFIG_HID_PRODIKEYS)	+= hid-prodikeys.o

Re: [PATCH 1/4] hid-multitouch: support for PixCir-based panels

[Henrik Rydberg]

On 10/14/2010 12:30 AM, Stephane Chatty wrote:

> Created a driver for PixCir based dual-touch panels, including the one
> in the Hanvon tablet.  This is done in a code structure aimed at unifying
> support for several existing HID multitouch panels.
> 
> Signed-off-by: Stephane Chatty <chatty@enac.fr>
> Tested-by: Stephane Lajeunesse <slajeunesse@yahoo.com>
> Tested-by: arjun kv <kvarjun@gmail.com>
> 
> diff -rupN a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> --- a/drivers/hid/hid-core.c	2010-10-14 00:30:43.992936922 +0200
> +++ b/drivers/hid/hid-core.c	2010-10-14 01:29:28.009936912 +0200
> @@ -1290,6 +1290,7 @@ static const struct hid_device_id hid_bl
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BTC, USB_DEVICE_ID_BTC_EMPREX_REMOTE_2) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CANDO, USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CHERRY, USB_DEVICE_ID_CHERRY_CYMOTION) },
> @@ -1312,6 +1313,7 @@ static const struct hid_device_id hid_bl
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_HANVON, USB_DEVICE_ID_HANVON_MULTITOUCH) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
> diff -rupN a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> --- a/drivers/hid/hid-ids.h	2010-10-14 00:27:48.288936922 +0200
> +++ b/drivers/hid/hid-ids.h	2010-10-14 01:29:37.273937194 +0200
> @@ -132,6 +132,7 @@
>  #define USB_DEVICE_ID_BTC_EMPREX_REMOTE_2	0x5577
>  
>  #define USB_VENDOR_ID_CANDO		0x2087
> +#define USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH 0x0703
>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH	0x0a01
>  #define USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6 0x0b03
>  
> @@ -295,6 +296,9 @@
>  #define USB_DEVICE_ID_GYRATION_REMOTE_2 0x0003
>  #define USB_DEVICE_ID_GYRATION_REMOTE_3 0x0008
>  
> +#define USB_VENDOR_ID_HANVON		0x20b3
> +#define USB_DEVICE_ID_HANVON_MULTITOUCH	0x0a18
> +
>  #define USB_VENDOR_ID_HAPP		0x078b
>  #define USB_DEVICE_ID_UGCI_DRIVING	0x0010
>  #define USB_DEVICE_ID_UGCI_FLYING	0x0020
> diff -rupN a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> --- a/drivers/hid/hid-multitouch.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/drivers/hid/hid-multitouch.c	2010-10-14 01:30:59.009937031 +0200
> @@ -0,0 +1,398 @@
> +/*
> + *  HID driver for multitouch panels
> + *
> + *  Copyright (c) 2010 Stephane Chatty <chatty@enac.fr>
> + *
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include "usbhid/usbhid.h"
> +
> +
> +MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");
> +MODULE_DESCRIPTION("HID multitouch panels");
> +MODULE_LICENSE("GPL");
> +
> +#include "hid-ids.h"
> +
> +#define MAX_TRKID	USHRT_MAX
> +
> +
> +struct mt_slot {
> +	__u16 x, y, p;
> +	bool valid;	/* did we just get valid contact data for this slot? */
> +	bool prev_valid;/* was this slot previously valid/active? */


I do think these should be named "touch" or "prox" rather than "valid". The
latter term is used in many drivers no represent "not null", and mixing the
semantics with proximity is best avoided.

> +	__u16 trkid;	/* the tracking ID that was assigned to this slot */

>From a struct-packing perspective, I think it is better to list variables in


size-descending order. In this particular case it does not matter.

> +};
> +
> +struct mt_device {
> +	struct mt_class *mtclass;/* our mt device class */
> +	struct mt_slot *slots;	/* buffer with all slots */
> +	__u8 optfeatures;	/* what optional mt features do we get? */
> +	__u8 curcontact; 	/* index of the current contact */

> +	__u8 maxcontact;	/* expected last contact index */ 
> +	bool curvalid; 		/* is the current contact valid? */
> +	__u16 curcontactid; 	/* ContactID of the current contact */
> +	__u16 curx, cury, curp;	/* other attributes of the current contact */


Could be a struct mt_slot instead.

> +	__u16 lasttrkid;	/* the last tracking ID we assigned */


Adding struct mt_slot slot_data[]; here at the end would be nice.

> +};
> +
> +struct mt_class {
> +	int (*compute_slot)(struct mt_device *);
> +	__u8 maxcontacts;
> +	__s8 inputmode;	/* InputMode HID feature number, -1 if non-existent */


It would be nice to have additional information here, like signal-to-noise ratio.

> +};
> +
> +/* classes of device behavior */
> +#define DUAL1 0
> +
> +/* contact data that only some devices report */
> +#define PRESSURE 	(1 << 0)
> +#define SIZE		(1 << 1)


The names here are a bit too general, IMO.

> +
> +/*
> + * these device-dependent functions determine what slot corresponds
> + * to a valid contact that was just read.
> + */
> +
> +static int slot_from_contactid(struct mt_device *td)
> +{
> +	return td->curcontactid;
> +}


I suppose this one is meant to loop over available slots for some implementations.

> +
> +struct mt_class mt_classes[] = {
> +	/* DUAL1 */		{ slot_from_contactid, 2, -1 },
> +};


Off placement of comment.

> +
> +
> +static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> +		struct hid_field *field, struct hid_usage *usage,
> +		unsigned long **bit, int *max)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	switch (usage->hid & HID_USAGE_PAGE) {
> +
> +	case HID_UP_GENDESK:
> +		switch (usage->hid) {
> +		case HID_GD_X:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_POSITION_X);
> +			input_set_abs_params(hi->input, ABS_MT_POSITION_X,
> +						field->logical_minimum,
> +						field->logical_maximum, 0, 0);


A helper function that simplifies the repeated usage of input_set_abs_params()
would be nice.

> +			/* touchscreen emulation */
> +			input_set_abs_params(hi->input, ABS_X,
> +						field->logical_minimum,
> +						field->logical_maximum, 0, 0);
> +			return 1;
> +		case HID_GD_Y:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_POSITION_Y);
> +			input_set_abs_params(hi->input, ABS_MT_POSITION_Y,
> +						field->logical_minimum,
> +						field->logical_maximum, 0, 0);
> +			/* touchscreen emulation */
> +			input_set_abs_params(hi->input, ABS_Y,
> +						field->logical_minimum,
> +						field->logical_maximum, 0, 0);
> +			return 1;
> +		}
> +		return 0;
> +
> +	case HID_UP_DIGITIZER:
> +		switch (usage->hid) {
> +		case HID_DG_INRANGE:
> +		case HID_DG_CONFIDENCE:
> +			return -1;
> +		case HID_DG_TIPSWITCH:
> +			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
> +			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
> +			return 1;
> +		case HID_DG_CONTACTID:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_TRACKING_ID);
> +			input_set_abs_params(hi->input, ABS_MT_TRACKING_ID,
> +						0, MAX_TRKID, 0, 0);
> +			if (!hi->input->mt)
> +				input_mt_create_slots(hi->input,
> +						td->mtclass->maxcontacts);


A local variable for hi->input would be nice.

> +
> +		case HID_DG_TIPPRESSURE:
> +			hid_map_usage(hi, usage, bit, max,
> +					EV_ABS, ABS_MT_PRESSURE);
> +			td->optfeatures |= PRESSURE;

> +			return 1;
> +		case HID_DG_CONTACTCOUNT:
> +		case HID_DG_CONTACTMAX:
> +			return -1;
> +		}
> +		/* let hid-input decide for the others */
> +		return 0;
> +
> +	case 0xff000000:
> +		/* we do not want to map these: no input-oriented meaning */
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
> +		struct hid_field *field, struct hid_usage *usage,
> +		unsigned long **bit, int *max)
> +{
> +	if (usage->type == EV_KEY || usage->type == EV_ABS)
> +		set_bit(usage->type, hi->input->evbit);
> +
> +	return -1;
> +}


If we want to support fuzz, and do not want to add it to the hid structure
(because it is not available in the reports), we should consider setting up the
input_dev completely within this driver, as done in the 3M and egalax drivers.

> +
> +/*
> + * this function is called when a whole contact has been processed,
> + * so that it can assign it to a slot and store the data there
> + */
> +static void mt_complete_slot(struct mt_device *td)
> +{
> +	if (td->curvalid) {
> +		struct mt_slot *slot;
> +		int slotnum = td->mtclass->compute_slot(td);
> +		if (slotnum >= 0 && slotnum <= td->mtclass->maxcontacts - 1) {


The returned slotnum should always be valid, so this test is redundant.

> +			slot = td->slots + slotnum;
> +
> +			slot->valid = true;


Rather that setting valid here, it should simply be the proximity (touch) state.
All slot data in our buffer is actually valid at all times.

> +			slot->x = td->curx;
> +			slot->y = td->cury;
> +			slot->p = td->curp;


Using a temporary slot struct for curx etc would be nice.

> +		}
> +	}
> +	td->curcontact++;


I think it is unnecessary to have both a slot number and a curcontact value.
Either the firmware reports the slot itself, or it reports a contact id that
needs to be looked up from the list of slots. In neither case does anything but
slot number and contact id enter the equation.

> +}
> +
> +
> +/*
> + * this function is called when a whole packet has been received and processed,
> + * so that it can decide what to send to the input layer.
> + */
> +static void mt_emit_event(struct mt_device *td, struct input_dev *input)
> +{
> +	struct mt_slot *oldest = 0; /* touchscreen emulation: oldest touch */
> +	int i;
> +
> +	for (i = 0; i < td->mtclass->maxcontacts; ++i) {
> +		struct mt_slot *s = td->slots + i;


Stating as &td->slots[i] is common practice.

> +		if (!s->valid) {
> +			/*
> +			 * this slot does not contain useful data,
> +			 * notify its closure if necessary
> +			 */
> +			if (s->prev_valid) {
> +				input_mt_slot(input, i);	


The logic is a bit easier if input_mt_slot() appears once before all the
branching. If nothing else is emitted, the input core does not emit the slot event.

> +				input_event(input, EV_ABS,
> +						ABS_MT_TRACKING_ID, -1);
> +				s->prev_valid = false;
> +			}
> +			continue;
> +		}
> +		if (!s->prev_valid)
> +			s->trkid = td->lasttrkid++;
> +		input_mt_slot(input, i);


So this one goes to the top.

> +		input_event(input, EV_ABS, ABS_MT_TRACKING_ID, s->trkid);
> +		input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
> +		input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
> +		if (td->optfeatures & PRESSURE)
> +			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);


How about touch_major, touch_minor and orientation here?

> +		s->prev_valid = true;
> +		s->valid = false;


Changing the touch state to false here seems wrong.

> +
> +		/* touchscreen emulation: is this the oldest contact? */
> +		if (!oldest || ((s->trkid - oldest->trkid) & (SHRT_MAX + 1)))
> +			oldest = s;
> +	}
> +
> +	/* touchscreen emulation */
> +	if (oldest) {
> +		input_event(input, EV_KEY, BTN_TOUCH, 1);
> +		input_event(input, EV_ABS, ABS_X, oldest->x);
> +		input_event(input, EV_ABS, ABS_Y, oldest->y);


Pressure here as well.

> +	} else {
> +		input_event(input, EV_KEY, BTN_TOUCH, 0);
> +	}
> +
> +	input_sync(input);
> +	td->curcontact = 0;


The curcontact semantics could be clearer, as noted elsewhere.

> +}
> +
> +
> +
> +static int mt_event(struct hid_device *hid, struct hid_field *field,
> +				struct hid_usage *usage, __s32 value)
> +{
> +	struct mt_device *td = hid_get_drvdata(hid);
> +
> +	if (hid->claimed & HID_CLAIMED_INPUT) {
> +		struct input_dev *input = field->hidinput->input;
> +		switch (usage->hid) {
> +		case HID_DG_INRANGE:


This one is used to report the validity of the current reported contact on many
devices. Maybe all, even?

> +			break;	

> +		case HID_DG_TIPSWITCH:
> +			td->curvalid = value;


This is really the touch state, the proximity.

> +			break;
> +		case HID_DG_CONFIDENCE:
> +			break;
> +		case HID_DG_CONTACTID:
> +			td->curcontactid = value;


Here is where I would but the conversion to slot id. And it should definitely be
guarded so that it is always a valid index.

> +			break;	
> +		case HID_DG_TIPPRESSURE:
> +			td->curp = value;


Using a mt_slot struct for this data would make sense.

> +			break;
> +		case HID_GD_X:
> +			td->curx = value;
> +			break;
> +		case HID_GD_Y:
> +			td->cury = value;
> +			/* works for devices where Y is last in a contact */
> +			mt_complete_slot(td);


Relying on a particular field to come last seems non-generic. Since there is
already a bit field started for various features of the device, why not complete
that list with all data reported per contact, and use it know when a contact is
complete? Something like "recorded_mask == expected_mask".

> +			break;
> +		case HID_DG_CONTACTCOUNT:
> +			/*
> +			 * works for devices where contact count is
> +			 * the last field in a message
> +			 */
> +			if (value)
> +				td->maxcontact = value - 1;


The usage of maxcontact is a bit odd. Perhaps there should be a "maxslot" which
never changes, and a "contactcount" which starts at zero.

> +			if (td->curcontact > td->maxcontact)
> +				mt_emit_event(td, input);


So curcontact is here a counter, which implements the logic found in the 3M,
with partial reports combining into a single one. Maybe we need a counter, or
maybe we can get by using the CONTACTID field.

> +			break;
> +		case HID_DG_CONTACTMAX:
> +			break;
> +
> +		default:
> +			/* fallback to the generic hidinput handling */
> +			return 0;
> +		}
> +	}
> +
> +	/* we have handled the hidinput part, now remains hiddev */
> +	if (hid->claimed & HID_CLAIMED_HIDDEV && hid->hiddev_hid_event)
> +		hid->hiddev_hid_event(hid, field, usage, value);
> +
> +	return 1;
> +}
> +
> +static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int ret;
> +	struct mt_device *td;
> +
> +#if 0
> +	/*
> +         * todo: activate this as soon as the patch where the quirk below
> +         * is defined is commited. This will allow the driver to correctly
> +         * support devices that emit events over several HID messages.
> +         */
> +	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
> +#endif


This code should be active.

> +
> +	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
> +	if (!td) {
> +		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
> +		return -ENOMEM;
> +	}
> +	td->mtclass = mt_classes + id->driver_data;
> +	td->slots = kzalloc(td->mtclass->maxcontacts * sizeof(struct mt_slot),
> +				GFP_KERNEL);


Allocating slots in the same range as mt_device would be nice.

> +	if (!td->slots) {
> +		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +	hid_set_drvdata(hdev, td);
> +
> +	ret = hid_parse(hdev);
> +	if (ret != 0)
> +		goto fail;
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret != 0)
> +		goto fail;
> +
> +	if (td->mtclass->inputmode >= 0) {
> +		struct hid_report *r;
> +		struct hid_report_enum *re = hdev->report_enum
> +						+ HID_FEATURE_REPORT;


&hdev->report_enum[HID_FEATURE_REPORT]

> +		r = re->report_id_hash[td->mtclass->inputmode];
> +		if (r) {
> +			r->field[0]->value[0] = 0x02;
> +			usbhid_submit_report(hdev, r, USB_DIR_OUT);
> +		}
> +	}
> +	return 0;
> +
> +fail:
> +	kfree(td);
> +	return ret;
> +}
> +
> +static void mt_remove(struct hid_device *hdev)
> +{
> +	struct mt_device *td = hid_get_drvdata(hdev);
> +	hid_hw_stop(hdev);
> +	kfree(td->slots);
> +	kfree(td);
> +	hid_set_drvdata(hdev, NULL);
> +}
> +
> +static const struct hid_device_id mt_devices[] = {
> +
> +	/* PixCir-based panels */
> +	{ .driver_data = DUAL1,
> +		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
> +			USB_DEVICE_ID_HANVON_MULTITOUCH) },
> +	{ .driver_data = DUAL1,
> +		HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
> +			USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, mt_devices);
> +
> +static const struct hid_usage_id mt_grabbed_usages[] = {
> +	{ HID_ANY_ID, HID_ANY_ID, HID_ANY_ID },
> +	{ HID_ANY_ID - 1, HID_ANY_ID - 1, HID_ANY_ID - 1}
> +};
> +
> +static struct hid_driver mt_driver = {
> +	.name = "hid-multitouch",
> +	.id_table = mt_devices,
> +	.probe = mt_probe,
> +	.remove = mt_remove,
> +	.input_mapping = mt_input_mapping,
> +	.input_mapped = mt_input_mapped,
> +	.usage_table = mt_grabbed_usages,
> +	.event = mt_event,
> +};
> +
> +static int __init mt_init(void)
> +{
> +	return hid_register_driver(&mt_driver);
> +}
> +
> +static void __exit mt_exit(void)
> +{
> +	hid_unregister_driver(&mt_driver);
> +}
> +
> +module_init(mt_init);
> +module_exit(mt_exit);
> +
> diff -rupN a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> --- a/drivers/hid/Kconfig	2010-10-14 01:39:17.263936923 +0200
> +++ b/drivers/hid/Kconfig	2010-10-14 01:42:37.964936879 +0200
> @@ -264,6 +264,12 @@ config HID_MONTEREY
>  	---help---
>  	Support for Monterey Genius KB29E.
>  
> +config HID_MULTITOUCH
> +	tristate "HID Multitouch panels"
> +	depends on USB_HID
> +	---help---
> +	Generic support for HID multitouch panels.
> +
>  config HID_NTRIG
>  	tristate "NTrig"
>  	depends on USB_HID
> diff -rupN a/drivers/hid/Makefile b/drivers/hid/Makefile
> --- a/drivers/hid/Makefile	2010-10-14 01:38:27.873936922 +0200
> +++ b/drivers/hid/Makefile	2010-10-14 01:40:25.571936977 +0200
> @@ -43,6 +43,7 @@ obj-$(CONFIG_HID_MAGICMOUSE)    += hid-m
>  obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
>  obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
>  obj-$(CONFIG_HID_MOSART)	+= hid-mosart.o
> +obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
>  obj-$(CONFIG_HID_NTRIG)		+= hid-ntrig.o
>  obj-$(CONFIG_HID_ORTEK)		+= hid-ortek.o
>  obj-$(CONFIG_HID_PRODIKEYS)	+= hid-prodikeys.o


That's it for now :-)

Cheers,
Henrik

Re: [PATCH 1/4] hid-multitouch: support for PixCir-based panels

[Stéphane Chatty]

Le 14 oct. 10 à 11:38, Henrik Rydberg a écrit :
>>
>> +	bool valid;	/* did we just get valid contact data for this slot? */
>> +	bool prev_valid;/* was this slot previously valid/active? */
>
>
> I do think these should be named "touch" or "prox" rather than  
> "valid". The
> latter term is used in many drivers no represent "not null", and  
> mixing the
> semantics with proximity is best avoided.
>

Actually, the mix of semantics exists because of the mismatch between  
the (static) HID protocol and the (dynamic) data it transports. Most  
of the time, TipSwitch=0 occurs when we get placeholder data: there  
is nothing to report at all, but the message format imposes that  
something is sent. And sometimes (rarely), we get TipSwitch=0 because  
a finger has been released. We are lucky that we can handle the two  
situations in the same way.

See below for a discussion of how we can solve this.


>> +	bool curvalid; 		/* is the current contact valid? */
>> +	__u16 curcontactid; 	/* ContactID of the current contact */
>> +	__u16 curx, cury, curp;	/* other attributes of the current  
>> contact */
>
>
> Could be a struct mt_slot instead.

Well, it's not *exactly* the same data. Especially if, as I'm  
beginning to understand thanks to one of your later comments, there  
is a difference in the semantics of 'valid' between slots and the  
current contact as read in the HID message:
  - my above discussion applies to the current contact
  - and in slots, this information gets filtered to become actual  
proximity information.

This would give:
  - curvalid in mt_data
  - touch in mt_slot (and not prox, because prox will mean something  
else in devices that have hover detection)


>
>> +};
>> +
>> +struct mt_class {
>> +	int (*compute_slot)(struct mt_device *);
>> +	__u8 maxcontacts;
>> +	__s8 inputmode;	/* InputMode HID feature number, -1 if non- 
>> existent */
>
>
> It would be nice to have additional information here, like signal- 
> to-noise ratio.

I thought you would say that. My answer was ready: I left it to you  
to add it afterwards :-)

>>
>> +/* contact data that only some devices report */
>> +#define PRESSURE 	(1 << 0)
>> +#define SIZE		(1 << 1)
>
>
> The names here are a bit too general, IMO.

I thought so too, but was too lazy to imagine something else and  
decided that what the heck, these are private names. Any suggestion?

>
>> +
>> +/*
>> + * these device-dependent functions determine what slot corresponds
>> + * to a valid contact that was just read.
>> + */
>> +
>> +static int slot_from_contactid(struct mt_device *td)
>> +{
>> +	return td->curcontactid;
>> +}
>
>
> I suppose this one is meant to loop over available slots for some  
> implementations.

See the following patches for examples. So far, looping was not  
necessary.

>
>> +
>> +struct mt_class mt_classes[] = {
>> +	/* DUAL1 */		{ slot_from_contactid, 2, -1 },
>> +};
>
>
> Off placement of comment.
>>

Please, please don't force me to add a useless field to the struct so  
as to make sure that the type is read before the data :-) Oh well...


>
> A helper function that simplifies the repeated usage of  
> input_set_abs_params()
> would be nice.
>
> A local variable for hi->input would be nice.
>
> If we want to support fuzz, and do not want to add it to the hid  
> structure
> (because it is not available in the reports), we should consider  
> setting up the
> input_dev completely within this driver, as done in the 3M and  
> egalax drivers.

Yes, my idea was that you'd like to take over and introduce the kind  
of work you are doing for 3M and eGalax. As for the helper function  
and hi->input, hey, I just copied your code :-)


>> +		if (slotnum >= 0 && slotnum <= td->mtclass->maxcontacts - 1) {
>
>
> The returned slotnum should always be valid, so this test is  
> redundant.

Nope. See Cypress devices.

>
>> +			slot = td->slots + slotnum;
>> +
>> +			slot->valid = true;
>
>
> Rather that setting valid here, it should simply be the proximity  
> (touch) state.
> All slot data in our buffer is actually valid at all times.
>
>> +			slot->x = td->curx;
>> +			slot->y = td->cury;
>> +			slot->p = td->curp;
>
>
> Using a temporary slot struct for curx etc would be nice.
>
>> +		}
>> +	}
>> +	td->curcontact++;

See my previous discussion on curvalid vs valid/prox/touch.

>
> I think it is unnecessary to have both a slot number and a  
> curcontact value.
> Either the firmware reports the slot itself, or it reports a  
> contact id that
> needs to be looked up from the list of slots. In neither case does  
> anything but
> slot number and contact id enter the equation.

See the following patches for examples of this being wrong.


>>
>> +			if (s->prev_valid) {
>> +				input_mt_slot(input, i);	
>
>
> The logic is a bit easier if input_mt_slot() appears once before  
> all the
> branching. If nothing else is emitted, the input core does not emit  
> the slot event.

I'm not a big fan of this kind of 'blind pass'. Jiri?


>
>
> How about touch_major, touch_minor and orientation here?

I have no device available to test this at the moment. What I had in  
mind was to add them when we add support for a device that has them.

>
>> +		s->prev_valid = true;
>> +		s->valid = false;
>
>
> Changing the touch state to false here seems wrong.

But it's necessary, believe me. There are devices for which we don't  
get a chance to change it to false when the finger is released.  
Setting it to false every time and resetting it to true when we get  
confirmation that the contact is still alive is the failsafe way of  
doing things.


>> +		input_event(input, EV_KEY, BTN_TOUCH, 1);
>> +		input_event(input, EV_ABS, ABS_X, oldest->x);
>> +		input_event(input, EV_ABS, ABS_Y, oldest->y);
>
>
> Pressure here as well.

Oops.

>
>> +	} else {
>> +		input_event(input, EV_KEY, BTN_TOUCH, 0);
>> +	}
>> +
>> +	input_sync(input);
>> +	td->curcontact = 0;
>
>
> The curcontact semantics could be clearer, as noted elsewhere.

Another name, perhaps? The bottom line is that we need to keep trace  
of the rank of the contact in the current message.

>
>> +}
>> +
>> +
>> +
>> +static int mt_event(struct hid_device *hid, struct hid_field *field,
>> +				struct hid_usage *usage, __s32 value)
>> +{
>> +	struct mt_device *td = hid_get_drvdata(hid);
>> +
>> +	if (hid->claimed & HID_CLAIMED_INPUT) {
>> +		struct input_dev *input = field->hidinput->input;
>> +		switch (usage->hid) {
>> +		case HID_DG_INRANGE:
>
>
> This one is used to report the validity of the current reported  
> contact on many
> devices. Maybe all, even?

Actually, only on a minority of device last time I checked.


>> +		case HID_DG_TIPSWITCH:
>> +			td->curvalid = value;
>
>
> This is really the touch state, the proximity.

I wish.

>
>> +			break;
>> +		case HID_DG_CONFIDENCE:
>> +			break;
>> +		case HID_DG_CONTACTID:
>> +			td->curcontactid = value;
>
>
> Here is where I would but the conversion to slot id. And it should  
> definitely be
> guarded so that it is always a valid index.

I'll pay you a beer if you find how to make this work for all  
devices :-)

>
>> +		case HID_GD_Y:
>> +			td->cury = value;
>> +			/* works for devices where Y is last in a contact */
>> +			mt_complete_slot(td);
>
>
> Relying on a particular field to come last seems non-generic. Since  
> there is
> already a bit field started for various features of the device, why  
> not complete
> that list with all data reported per contact, and use it know when  
> a contact is
> complete? Something like "recorded_mask == expected_mask".

What I had in mind was that the information is available in lower  
level code, and that we could handle this when switching to a raw hid  
device. Otherwise, we'd just be reinventing the wheel, that is  
rebuilding information that's already there. Same for  
HID_CONTACTCOUNT below: I use it because it marks the end of messages  
on most devices, but there should be a better way of knowing that we  
are at the end of a message.


>
>> +			break;
>> +		case HID_DG_CONTACTCOUNT:
>> +			/*
>> +			 * works for devices where contact count is
>> +			 * the last field in a message
>> +			 */
>> +			if (value)
>> +				td->maxcontact = value - 1;
>
>
> The usage of maxcontact is a bit odd. Perhaps there should be a  
> "maxslot" which
> never changes, and a "contactcount" which starts at zero.

maxcontact works with curcontact. I found the name 'contactcount'  
confusing, because in HID it means something else. Still looking for  
a better name for 'curcontact'.

>
>> +			if (td->curcontact > td->maxcontact)
>> +				mt_emit_event(td, input);
>
>
> So curcontact is here a counter, which implements the logic found  
> in the 3M,
> with partial reports combining into a single one. Maybe we need a  
> counter, or
> maybe we can get by using the CONTACTID field.
>
>>

Yes, curcontact is a counter. And we also need it for computing the  
slot id sometimes.


>> +
>> +#if 0
>> +	/*
>> +         * todo: activate this as soon as the patch where the  
>> quirk below
>> +         * is defined is commited. This will allow the driver to  
>> correctly
>> +         * support devices that emit events over several HID  
>> messages.
>> +         */
>> +	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
>> +#endif
>
>
> This code should be active.

Then it won't compile in 2.6.36-rc7.

>
>> +
>> +	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
>> +	if (!td) {
>> +		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
>> +		return -ENOMEM;
>> +	}
>> +	td->mtclass = mt_classes + id->driver_data;
>> +	td->slots = kzalloc(td->mtclass->maxcontacts * sizeof(struct  
>> mt_slot),
>> +				GFP_KERNEL);
>
>
> Allocating slots in the same range as mt_device would be nice.

Agreed.

>
>> +		struct hid_report_enum *re = hdev->report_enum
>> +						+ HID_FEATURE_REPORT;
>
>
> &hdev->report_enum[HID_FEATURE_REPORT]

Won't fit on one line, then. Visually, it gets worse. Someone else,  
an opinion?

>
>
> That's it for now :-)
>

Then, on to patches 2, 3 and 4 :-)

St.

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

Re: [PATCH 1/4] hid-multitouch: support for PixCir-based panels

[Henrik Rydberg]

On 10/14/2010 12:28 PM, Stéphane Chatty wrote:

> 
> Le 14 oct. 10 à 11:38, Henrik Rydberg a écrit :
>>>
>>> +    bool valid;    /* did we just get valid contact data for this slot? */
>>> +    bool prev_valid;/* was this slot previously valid/active? */
>>
>>
>> I do think these should be named "touch" or "prox" rather than "valid". The
>> latter term is used in many drivers no represent "not null", and mixing the
>> semantics with proximity is best avoided.
>>
> 
> Actually, the mix of semantics exists because of the mismatch between the
> (static) HID protocol and the (dynamic) data it transports. Most of the time,
> TipSwitch=0 occurs when we get placeholder data: there is nothing to report at
> all, but the message format imposes that something is sent. And sometimes
> (rarely), we get TipSwitch=0 because a finger has been released. We are lucky
> that we can handle the two situations in the same way.


It seems to me an individual packet from the device is either valid or invalid,
and there are a couple of different mechanisms used so far; HID_DG_INRANGE
and/or HID_DG_CONTACTCOUNT seem to care of all but one (cypress) which also uses
HID_DG_CONTACTID.

In each valid packet, there is a notion of touch/enable/press/proximity, and it
seems to be either true for all valid packets, or goverened by HID_DG_TIPSWITCH.

> 
> See below for a discussion of how we can solve this.
> 
> 
>>> +    bool curvalid;         /* is the current contact valid? */
>>> +    __u16 curcontactid;     /* ContactID of the current contact */
>>> +    __u16 curx, cury, curp;    /* other attributes of the current contact */
>>
>>
>> Could be a struct mt_slot instead.
> 
> Well, it's not *exactly* the same data. Especially if, as I'm beginning to
> understand thanks to one of your later comments, there is a difference in the
> semantics of 'valid' between slots and the current contact as read in the HID
> message:
>  - my above discussion applies to the current contact
>  - and in slots, this information gets filtered to become actual proximity
> information.


In the recent egalax driver submission, the data collected during a hid report
round is actually exactly the same as the data stored per slot.

> This would give:
>  - curvalid in mt_data
>  - touch in mt_slot (and not prox, because prox will mean something else in
> devices that have hover detection)


I do think it is fruitful to formulate this as mt_data holding both curvalid and
the touch state of the current slot.

>>
>>> +};
>>> +
>>> +struct mt_class {
>>> +    int (*compute_slot)(struct mt_device *);
>>> +    __u8 maxcontacts;
>>> +    __s8 inputmode;    /* InputMode HID feature number, -1 if non-existent */
>>
>>
>> It would be nice to have additional information here, like signal-to-noise ratio.
> 
> I thought you would say that. My answer was ready: I left it to you to add it
> afterwards :-)


Ok, no problem :-)

> 
>>>
>>> +/* contact data that only some devices report */
>>> +#define PRESSURE     (1 << 0)
>>> +#define SIZE        (1 << 1)
>>
>>
>> The names here are a bit too general, IMO.
> 
> I thought so too, but was too lazy to imagine something else and decided that
> what the heck, these are private names. Any suggestion?


True, private, but how about a HID_ or MT_ prefix, or something like that.


>>> +
>>> +/*
>>> + * these device-dependent functions determine what slot corresponds
>>> + * to a valid contact that was just read.
>>> + */
>>> +
>>> +static int slot_from_contactid(struct mt_device *td)
>>> +{
>>> +    return td->curcontactid;
>>> +}
>>
>>
>> I suppose this one is meant to loop over available slots for some
>> implementations.
> 
> See the following patches for examples. So far, looping was not necessary.


Right, those devices are yet to come. As suggested in another patch as well,
perhaps we can get by without actually using different function pointers here. A
switch statement does weigh fairly light in lieu of the processing done per hid
report. :-)

>>> +
>>> +struct mt_class mt_classes[] = {
>>> +    /* DUAL1 */        { slot_from_contactid, 2, -1 },
>>> +};
>>
>>
>> Off placement of comment.
>>>
> 
> Please, please don't force me to add a useless field to the struct so as to make
> sure that the type is read before the data :-) Oh well...


I really don't care how the comments are placed, but somebody else might.
Standard is your friend. ;-)


>> A helper function that simplifies the repeated usage of input_set_abs_params()
>> would be nice.
>>
>> A local variable for hi->input would be nice.
>>
>> If we want to support fuzz, and do not want to add it to the hid structure
>> (because it is not available in the reports), we should consider setting up the
>> input_dev completely within this driver, as done in the 3M and egalax drivers.
> 
> Yes, my idea was that you'd like to take over and introduce the kind of work you
> are doing for 3M and eGalax. As for the helper function and hi->input, hey, I
> just copied your code :-)


Fine with me, although since we are introducing a new driver, it seems slightly
backwards. Perhaps we can fold some patches jointly in the next round? And yes,
guilty, apparently my preferences regarding these drivers is changing with time,
too. :-)

>>> +        if (slotnum >= 0 && slotnum <= td->mtclass->maxcontacts - 1) {
>>
>>
>> The returned slotnum should always be valid, so this test is redundant.
> 
> Nope. See Cypress devices.


I believe we have covered this in a separate thread now.


>>
>>> +            slot = td->slots + slotnum;
>>> +
>>> +            slot->valid = true;
>>
>>
>> Rather that setting valid here, it should simply be the proximity (touch) state.
>> All slot data in our buffer is actually valid at all times.
>>
>>> +            slot->x = td->curx;
>>> +            slot->y = td->cury;
>>> +            slot->p = td->curp;
>>
>>
>> Using a temporary slot struct for curx etc would be nice.
>>
>>> +        }
>>> +    }
>>> +    td->curcontact++;
> 
> See my previous discussion on curvalid vs valid/prox/touch.


Ditto.


>> I think it is unnecessary to have both a slot number and a curcontact value.
>> Either the firmware reports the slot itself, or it reports a contact id that
>> needs to be looked up from the list of slots. In neither case does anything but
>> slot number and contact id enter the equation.
> 
> See the following patches for examples of this being wrong.


The enumeration used in some of the drivers makes sense, but I never got around
to test on a broader basis whether HID_DG_CONTACTID would accomplish the same
thing. I am fine with using a counter for some drivers.

>>>
>>> +            if (s->prev_valid) {
>>> +                input_mt_slot(input, i);   
>>
>>
>> The logic is a bit easier if input_mt_slot() appears once before all the
>> branching. If nothing else is emitted, the input core does not emit the slot
>> event.
> 
> I'm not a big fan of this kind of 'blind pass'. Jiri?


In this particular case, I think it makes the logic clearer.

>> How about touch_major, touch_minor and orientation here?
> 
> I have no device available to test this at the moment. What I had in mind was to
> add them when we add support for a device that has them.


OK, in conjunction with the 3M adaption, then.

>>> +        s->prev_valid = true;
>>> +        s->valid = false;
>>
>>
>> Changing the touch state to false here seems wrong.
> 
> But it's necessary, believe me. There are devices for which we don't get a
> chance to change it to false when the finger is released. Setting it to false
> every time and resetting it to true when we get confirmation that the contact is
> still alive is the failsafe way of doing things.


Different assumptions here, I think. I am assuming that the set of kept slots
are always valid, and contain a touch state. If a set of touches limited by
HID_DG_CONTACTCOUNT comes in, one would reset the touch state of the remaining
touches. This would simplify adaption of the devices that send partial or
sequential states, IMO.

> 
>>> +        input_event(input, EV_KEY, BTN_TOUCH, 1);
>>> +        input_event(input, EV_ABS, ABS_X, oldest->x);
>>> +        input_event(input, EV_ABS, ABS_Y, oldest->y);
>>
>>
>> Pressure here as well.
> 
> Oops.
> 
>>
>>> +    } else {
>>> +        input_event(input, EV_KEY, BTN_TOUCH, 0);
>>> +    }
>>> +
>>> +    input_sync(input);
>>> +    td->curcontact = 0;
>>
>>
>> The curcontact semantics could be clearer, as noted elsewhere.
> 
> Another name, perhaps? The bottom line is that we need to keep trace of the rank
> of the contact in the current message.


Yeah, different name. And something that makes clear whether we are talking
about an index (count - 1) or the count itself.

>>> +}
>>> +
>>> +
>>> +
>>> +static int mt_event(struct hid_device *hid, struct hid_field *field,
>>> +                struct hid_usage *usage, __s32 value)
>>> +{
>>> +    struct mt_device *td = hid_get_drvdata(hid);
>>> +
>>> +    if (hid->claimed & HID_CLAIMED_INPUT) {
>>> +        struct input_dev *input = field->hidinput->input;
>>> +        switch (usage->hid) {
>>> +        case HID_DG_INRANGE:
>>
>>
>> This one is used to report the validity of the current reported contact on many
>> devices. Maybe all, even?
> 
> Actually, only on a minority of device last time I checked.


Adding "if sequence number less than number of sent contacts" to the list, I
think we might cover all of them.


> 
>>> +        case HID_DG_TIPSWITCH:
>>> +            td->curvalid = value;
>>
>>
>> This is really the touch state, the proximity.
> 
> I wish.


Same comment as above. :-)

> 
>>
>>> +            break;
>>> +        case HID_DG_CONFIDENCE:
>>> +            break;
>>> +        case HID_DG_CONTACTID:
>>> +            td->curcontactid = value;
>>
>>
>> Here is where I would but the conversion to slot id. And it should definitely be
>> guarded so that it is always a valid index.
> 
> I'll pay you a beer if you find how to make this work for all devices :-)


It will keep you to your promise - and vice versa if it does not work out. :-)

>>
>>> +        case HID_GD_Y:
>>> +            td->cury = value;
>>> +            /* works for devices where Y is last in a contact */
>>> +            mt_complete_slot(td);
>>
>>
>> Relying on a particular field to come last seems non-generic. Since there is
>> already a bit field started for various features of the device, why not complete
>> that list with all data reported per contact, and use it know when a contact is
>> complete? Something like "recorded_mask == expected_mask".
> 
> What I had in mind was that the information is available in lower level code,
> and that we could handle this when switching to a raw hid device. Otherwise,
> we'd just be reinventing the wheel, that is rebuilding information that's
> already there. Same for HID_CONTACTCOUNT below: I use it because it marks the
> end of messages on most devices, but there should be a better way of knowing
> that we are at the end of a message.


Yeah. I was looking for a means in the hid usage code. One option could be to
add a "serve me all usages at once" method in the HID layer. Regardless, I doubt
the current code will actually work for all devices?

>>
>>> +            break;
>>> +        case HID_DG_CONTACTCOUNT:
>>> +            /*
>>> +             * works for devices where contact count is
>>> +             * the last field in a message
>>> +             */
>>> +            if (value)
>>> +                td->maxcontact = value - 1;
>>
>>
>> The usage of maxcontact is a bit odd. Perhaps there should be a "maxslot" which
>> never changes, and a "contactcount" which starts at zero.
> 
> maxcontact works with curcontact. I found the name 'contactcount' confusing,
> because in HID it means something else. Still looking for a better name for
> 'curcontact'.
> 
>>
>>> +            if (td->curcontact > td->maxcontact)
>>> +                mt_emit_event(td, input);
>>
>>
>> So curcontact is here a counter, which implements the logic found in the 3M,
>> with partial reports combining into a single one. Maybe we need a counter, or
>> maybe we can get by using the CONTACTID field.
>>
>>>
> 
> Yes, curcontact is a counter. And we also need it for computing the slot id
> sometimes.


Ok. How about num_received?

>>> +
>>> +#if 0
>>> +    /*
>>> +         * todo: activate this as soon as the patch where the quirk below
>>> +         * is defined is commited. This will allow the driver to correctly
>>> +         * support devices that emit events over several HID messages.
>>> +         */
>>> +    hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
>>> +#endif
>>
>>
>> This code should be active.
> 
> Then it won't compile in 2.6.36-rc7.


But to test it, it will have to compile :-)

>>> +
>>> +    td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
>>> +    if (!td) {
>>> +        dev_err(&hdev->dev, "cannot allocate multitouch data\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +    td->mtclass = mt_classes + id->driver_data;
>>> +    td->slots = kzalloc(td->mtclass->maxcontacts * sizeof(struct mt_slot),
>>> +                GFP_KERNEL);
>>
>>
>> Allocating slots in the same range as mt_device would be nice.
> 
> Agreed.
> 
>>
>>> +        struct hid_report_enum *re = hdev->report_enum
>>> +                        + HID_FEATURE_REPORT;
>>
>>
>> &hdev->report_enum[HID_FEATURE_REPORT]
> 
> Won't fit on one line, then. Visually, it gets worse. Someone else, an opinion?
> 
>>
>>
>> That's it for now :-)
>>
> 
> Then, on to patches 2, 3 and 4 :-)


And the next round :-)

Cheers,
Henrik

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

Re: [PATCH 1/4] hid-multitouch: support for PixCir-based panels

[Jiri Kosina]

On Thu, 14 Oct 2010, Stephane Chatty wrote:

> Created a driver for PixCir based dual-touch panels, including the one
> in the Hanvon tablet.  This is done in a code structure aimed at unifying
> support for several existing HID multitouch panels.
> 
> Signed-off-by: Stephane Chatty <chatty@enac.fr>
> Tested-by: Stephane Lajeunesse <slajeunesse@yahoo.com>
> Tested-by: arjun kv <kvarjun@gmail.com>

For some reason this series never reached my mailbox. But patchwork 
revealed those patches to me now :)

So sorry for the delay guys, I'll be looking into this when I come back 
from Kernel Summit/LPC.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.