[PATCH 1/2] hid-multitouch: work on default classes and renaming of classes

The safest quirk for a device (the one that works out of the box for
most of them) is the MT_QUIRK_NOT_SEEN_MEANS_UP. Indeed, it does not
make any assumption on the device. When adding a new device, we can
easily test it against MT_CLS_DEFAULT, and then optimize it with other
quirks: that's why no device use MT_CLS_DEFAULT right now.

This patch introduce also MT_CLS_DUAL_DEFAULT which has the same purpose
than MT_CLS_DEFAULT, but for dual touch panels.

Finally, the patch renames MT_CLS_DUAL1 to MT_CLS_DUAL_INRANGE_CONTACTID
and MT_CLS_DUAL2 to MT_CLS_DUAL_INRANGE_CONTACTNUMBER for better
readability.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 07d3183..ffe2a76 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -24,6 +24,7 @@
 
 
 MODULE_AUTHOR("Stephane Chatty <chatty@enac.fr>");
+MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
 MODULE_DESCRIPTION("HID multitouch panels");
 MODULE_LICENSE("GPL");
 

@@ -65,10 +66,11 @@ struct mt_class {
 };
 
 /* classes of device behavior */
-#define MT_CLS_DEFAULT	1
-#define MT_CLS_DUAL1	2
-#define MT_CLS_DUAL2	3
-#define MT_CLS_CYPRESS	4
+#define MT_CLS_DEFAULT				1
+#define MT_CLS_DUAL_DEFAULT			2
+#define MT_CLS_DUAL_INRANGE_CONTACTID		3
+#define MT_CLS_DUAL_INRANGE_CONTACTNUMBER	4
+#define MT_CLS_CYPRESS				10
 
 /*
  * these device-dependent functions determine what slot corresponds

@@ -104,13 +106,16 @@ static int find_slot_from_contactid(struct mt_device *td)
 
 struct mt_class mt_classes[] = {
 	{ .name = MT_CLS_DEFAULT,
-		.quirks = MT_QUIRK_VALID_IS_INRANGE,
+		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP,
 		.maxcontacts = 10 },
-	{ .name = MT_CLS_DUAL1,
+	{ .name = MT_CLS_DUAL_DEFAULT,
+		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP,
+		.maxcontacts = 2 },
+	{ .name = MT_CLS_DUAL_INRANGE_CONTACTID,
 		.quirks = MT_QUIRK_VALID_IS_INRANGE |
 			MT_QUIRK_SLOT_IS_CONTACTID,
 		.maxcontacts = 2 },
-	{ .name = MT_CLS_DUAL2,
+	{ .name = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
 		.quirks = MT_QUIRK_VALID_IS_INRANGE |
 			MT_QUIRK_SLOT_IS_CONTACTNUMBER,
 		.maxcontacts = 2 },

@@ -466,15 +471,15 @@ static const struct hid_device_id mt_devices[] = {
 			USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
 
 	/* GeneralTouch panel */
-	{ .driver_data = MT_CLS_DUAL2,
+	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
 		HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
 			USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
 
 	/* PixCir-based panels */
-	{ .driver_data = MT_CLS_DUAL1,
+	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
 		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
 			USB_DEVICE_ID_HANVON_MULTITOUCH) },
-	{ .driver_data = MT_CLS_DUAL1,
+	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
 		HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
 

-- 
1.7.3.4

The product id is the 42 inches one. I don't know if they have
other product id for the other size, or if it is a per-size product id.

Tested-by: Victor Zhuk <redacted>
Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/Kconfig          |    1 +
 drivers/hid/hid-ids.h        |    3 +++
 drivers/hid/hid-multitouch.c |    5 +++++
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 24cca2f..a0b117d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig

@@ -304,6 +304,7 @@ config HID_MULTITOUCH
 	  Say Y here if you have one of the following devices:
 	  - Cypress TrueTouch panels
 	  - Hanvon dual touch panels
+	  - IrTouch Infrared USB panels
 	  - Pixcir dual touch panels
 	  - 'Sensing Win7-TwoFinger' panel by GeneralTouch
 

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 92a0d61..b1dd7ad 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h

@@ -333,6 +333,9 @@
 #define USB_VENDOR_ID_IMATION		0x0718
 #define USB_DEVICE_ID_DISC_STAKKA	0xd000
 
+#define USB_VENDOR_ID_IRTOUCHSYSTEMS	0x6615
+#define USB_DEVICE_ID_IRTOUCH_INFRARED_USB	0x0070
+
 #define USB_VENDOR_ID_JESS		0x0c45
 #define USB_DEVICE_ID_JESS_YUREX	0x1010
 

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index ffe2a76..1629296 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -475,6 +475,11 @@ static const struct hid_device_id mt_devices[] = {
 		HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
 			USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },
 
+	/* IRTOUCH panels */
+	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
+		HID_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS,
+			USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
+
 	/* PixCir-based panels */
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
 		HID_USB_DEVICE(USB_VENDOR_ID_HANVON,

-- 
1.7.3.4

Hello everyone,

The product id is the 42 inches one. I don't know if they have
other product id for the other size, or if it is a per-size product id.

This is very interesting, I have access to a 65" IrTouch system which
has VID 0x6615 and PID 0x0C20. I wasn't aware that there are already
kernel drivers for their systems, I had been playing around with their
awful binary driver and had done some experiments with libusb. Is there
a documentation of some kind available?
 

I will test this on our screen next week.
I assume that I should use the tree from:
http://git.kernel.org/?p=linux/kernel/git/rydberg/input-mt.git;a=summary

Note: after briefly browsing through hid-multitouch.c, it seems to me
that there might be issues; the USB interface on our screen doesn't
report as HID device, but as vendor class (0xFF).

Florian

Hi Florian,


On Fri, Jan 28, 2011 at 17:24, Florian Echtler [off-list ref] wrote:

Hello everyone,

quoted

The product id is the 42 inches one. I don't know if they have
other product id for the other size, or if it is a per-size product id.

This is very interesting, I have access to a 65" IrTouch system which
has VID 0x6615 and PID 0x0C20. I wasn't aware that there are already
kernel drivers for their systems, I had been playing around with their
awful binary driver and had done some experiments with libusb. Is there
a documentation of some kind available?

We just made the analysis of the hid reports that are send. We have no
documentation for this particular device.

quoted

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 92a0d61..b1dd7ad 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h

@@ -333,6 +333,9 @@

 #define USB_VENDOR_ID_IMATION                0x0718
 #define USB_DEVICE_ID_DISC_STAKKA    0xd000

+#define USB_VENDOR_ID_IRTOUCHSYSTEMS 0x6615
+#define USB_DEVICE_ID_IRTOUCH_INFRARED_USB   0x0070

I will test this on our screen next week.
I assume that I should use the tree from:
http://git.kernel.org/?p=linux/kernel/git/rydberg/input-mt.git;a=summary

hid-multitouch is schedulded for 2.6.38. So you can also use the stock
2.6.38-rc2:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=summary

Note: after briefly browsing through hid-multitouch.c, it seems to me
that there might be issues; the USB interface on our screen doesn't
report as HID device, but as vendor class (0xFF).

This is more problematic. Hardware makers are forced to push hid aware
firmware to be win7 certified. Maybe they have a new firmware for your
device.

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

Hi Benjamin,

The safest quirk for a device (the one that works out of the box for
most of them) is the MT_QUIRK_NOT_SEEN_MEANS_UP. Indeed, it does not
make any assumption on the device. When adding a new device, we can
easily test it against MT_CLS_DEFAULT, and then optimize it with other
quirks: that's why no device use MT_CLS_DEFAULT right now.

This patch introduce also MT_CLS_DUAL_DEFAULT which has the same purpose
than MT_CLS_DEFAULT, but for dual touch panels.

But it is used anywhere?
 

Finally, the patch renames MT_CLS_DUAL1 to MT_CLS_DUAL_INRANGE_CONTACTID
and MT_CLS_DUAL2 to MT_CLS_DUAL_INRANGE_CONTACTNUMBER for better
readability.

Signed-off-by: Benjamin Tissoires <redacted>
---

The patch description and the content lacks a certain
distinctness. Please single out janitory actions into a separate
patch, or, if possible, skip it altogether.

Adding this is fine, based on your prior contributions. Perhaps it
should be motivated as such in a separate patch instead.

There is no need to change the numbering for unchanged names.

Thanks,
Henrik

On Fri, Jan 28, 2011 at 05:04:40PM +0100, Benjamin Tissoires wrote:

The product id is the 42 inches one. I don't know if they have
other product id for the other size, or if it is a per-size product id.

Tested-by: Victor Zhuk <redacted>
Signed-off-by: Benjamin Tissoires <redacted>
---

The canonical patch description presents the reason for the change,
and what the patch does (not necessarily how).

Thanks,
Henrik

On Fri, Jan 28, 2011 at 18:18, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

quoted

The safest quirk for a device (the one that works out of the box for
most of them) is the MT_QUIRK_NOT_SEEN_MEANS_UP. Indeed, it does not
make any assumption on the device. When adding a new device, we can
easily test it against MT_CLS_DEFAULT, and then optimize it with other
quirks: that's why no device use MT_CLS_DEFAULT right now.

This patch introduce also MT_CLS_DUAL_DEFAULT which has the same purpose
than MT_CLS_DEFAULT, but for dual touch panels.

But it is used anywhere?

Hi Henrik,

In it's current form, no. I often rely on it to make a quick patch
(just adding the ids in hid-ids, hid-core and hid-multitouch) to test
a new (dual touch) device. I thought it would be useful to be
mainstream.

quoted

Finally, the patch renames MT_CLS_DUAL1 to MT_CLS_DUAL_INRANGE_CONTACTID
and MT_CLS_DUAL2 to MT_CLS_DUAL_INRANGE_CONTACTNUMBER for better
readability.

Signed-off-by: Benjamin Tissoires <redacted>
---

The patch description and the content lacks a certain
distinctness. Please single out janitory actions into a separate
patch, or, if possible, skip it altogether.

ok, will do

quoted

 drivers/hid/hid-multitouch.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 07d3183..ffe2a76 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -24,6 +24,7 @@

 MODULE_AUTHOR("Stephane Chatty [off-list ref]");
+MODULE_AUTHOR("Benjamin Tissoires [off-list ref]");

Adding this is fine, based on your prior contributions. Perhaps it
should be motivated as such in a separate patch instead.

ok

quoted

 MODULE_DESCRIPTION("HID multitouch panels");
 MODULE_LICENSE("GPL");

@@ -65,10 +66,11 @@ struct mt_class {

 };

 /* classes of device behavior */
-#define MT_CLS_DEFAULT       1
-#define MT_CLS_DUAL1 2
-#define MT_CLS_DUAL2 3
-#define MT_CLS_CYPRESS       4
+#define MT_CLS_DEFAULT                               1
+#define MT_CLS_DUAL_DEFAULT                  2
+#define MT_CLS_DUAL_INRANGE_CONTACTID                3
+#define MT_CLS_DUAL_INRANGE_CONTACTNUMBER    4
+#define MT_CLS_CYPRESS                               10

There is no need to change the numbering for unchanged names.

I just wanted to keep device-specific at the end of the list. Hence
the 10 to keep a little space between generic and devices:
MT_CLS_DUAL_CONFIDENCE_CONTACT* are coming...

quoted

 /*
  * these device-dependent functions determine what slot corresponds

@@ -104,13 +106,16 @@ static int find_slot_from_contactid(struct mt_device *td)

 struct mt_class mt_classes[] = {
      { .name = MT_CLS_DEFAULT,
-             .quirks = MT_QUIRK_VALID_IS_INRANGE,
+             .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP,
              .maxcontacts = 10 },
-     { .name = MT_CLS_DUAL1,
+     { .name = MT_CLS_DUAL_DEFAULT,
+             .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP,
+             .maxcontacts = 2 },
+     { .name = MT_CLS_DUAL_INRANGE_CONTACTID,
              .quirks = MT_QUIRK_VALID_IS_INRANGE |
                      MT_QUIRK_SLOT_IS_CONTACTID,
              .maxcontacts = 2 },
-     { .name = MT_CLS_DUAL2,
+     { .name = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
              .quirks = MT_QUIRK_VALID_IS_INRANGE |
                      MT_QUIRK_SLOT_IS_CONTACTNUMBER,
              .maxcontacts = 2 },

@@ -466,15 +471,15 @@ static const struct hid_device_id mt_devices[] = {

                      USB_DEVICE_ID_CYPRESS_TRUETOUCH) },

      /* GeneralTouch panel */
-     { .driver_data = MT_CLS_DUAL2,
+     { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
              HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
                      USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },

      /* PixCir-based panels */
-     { .driver_data = MT_CLS_DUAL1,
+     { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
              HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
                      USB_DEVICE_ID_HANVON_MULTITOUCH) },
-     { .driver_data = MT_CLS_DUAL1,
+     { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
              HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
                      USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },

--
1.7.3.4

Thanks,
Henrik

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

On Fri, Jan 28, 2011 at 18:21, Henrik Rydberg [off-list ref] wrote:

On Fri, Jan 28, 2011 at 05:04:40PM +0100, Benjamin Tissoires wrote:

quoted

The product id is the 42 inches one. I don't know if they have
other product id for the other size, or if it is a per-size product id.

Tested-by: Victor Zhuk <redacted>
Signed-off-by: Benjamin Tissoires <redacted>
---

The canonical patch description presents the reason for the change,
and what the patch does (not necessarily how).

It was just to keep trace of the kind of display it was. As Florian
told the 63" does not have the same PID but not also the same
protocol.
Otherwise, we can also change the name of the define, but I'm not sure
on the rule to apply here.

Thanks,
Benjamin

quoted

 drivers/hid/Kconfig          |    1 +
 drivers/hid/hid-ids.h        |    3 +++
 drivers/hid/hid-multitouch.c |    5 +++++
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 24cca2f..a0b117d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig

@@ -304,6 +304,7 @@ config HID_MULTITOUCH

        Say Y here if you have one of the following devices:
        - Cypress TrueTouch panels
        - Hanvon dual touch panels
+       - IrTouch Infrared USB panels
        - Pixcir dual touch panels
        - 'Sensing Win7-TwoFinger' panel by GeneralTouch

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 92a0d61..b1dd7ad 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h

@@ -333,6 +333,9 @@

 #define USB_VENDOR_ID_IMATION                0x0718
 #define USB_DEVICE_ID_DISC_STAKKA    0xd000

+#define USB_VENDOR_ID_IRTOUCHSYSTEMS 0x6615
+#define USB_DEVICE_ID_IRTOUCH_INFRARED_USB   0x0070
+
 #define USB_VENDOR_ID_JESS           0x0c45
 #define USB_DEVICE_ID_JESS_YUREX     0x1010

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index ffe2a76..1629296 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -475,6 +475,11 @@ static const struct hid_device_id mt_devices[] = {

              HID_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
                      USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS) },

+     /* IRTOUCH panels */
+     { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
+             HID_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS,
+                     USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
+
      /* PixCir-based panels */
      { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
              HID_USB_DEVICE(USB_VENDOR_ID_HANVON,
--
1.7.3.4

Thanks,
Henrik

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

quoted

quoted

This patch introduce also MT_CLS_DUAL_DEFAULT which has the same purpose
than MT_CLS_DEFAULT, but for dual touch panels.

But it is used anywhere?

Hi Henrik,

In it's current form, no. I often rely on it to make a quick patch
(just adding the ids in hid-ids, hid-core and hid-multitouch) to test
a new (dual touch) device. I thought it would be useful to be
mainstream.

I see. However, it is just as easy to keep as a local patch. Quite
generally, there really is a difference between what we do in our
computers and what ends up in mainline, and it is almost always for a
good reason.

quoted

quoted

@@ -65,10 +66,11 @@ struct mt_class {

 };

 /* classes of device behavior */
-#define MT_CLS_DEFAULT       1
-#define MT_CLS_DUAL1 2
-#define MT_CLS_DUAL2 3
-#define MT_CLS_CYPRESS       4
+#define MT_CLS_DEFAULT                               1
+#define MT_CLS_DUAL_DEFAULT                  2
+#define MT_CLS_DUAL_INRANGE_CONTACTID                3
+#define MT_CLS_DUAL_INRANGE_CONTACTNUMBER    4
+#define MT_CLS_CYPRESS                               10

There is no need to change the numbering for unchanged names.

I just wanted to keep device-specific at the end of the list. Hence
the 10 to keep a little space between generic and devices:
MT_CLS_DUAL_CONFIDENCE_CONTACT* are coming...

It is very often the case that one would like to modify something to
keep to a certain aesthetic classification or idea, but there is a
very good reason not to do it. In order to maintain more than one
branch, for instance a stable tree and several distributions, having
simple patches makes a huge difference in maintenance burden. Not to
mention how hard it can be to merge trees with unrelated changes in
them. So please, keep patches short, sweet and to the point.

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

On Fri, Jan 28, 2011 at 06:59:13PM +0100, Benjamin Tissoires wrote:

On Fri, Jan 28, 2011 at 18:21, Henrik Rydberg [off-list ref] wrote:

quoted

On Fri, Jan 28, 2011 at 05:04:40PM +0100, Benjamin Tissoires wrote:

quoted

The product id is the 42 inches one. I don't know if they have
other product id for the other size, or if it is a per-size product id.

Tested-by: Victor Zhuk <redacted>
Signed-off-by: Benjamin Tissoires <redacted>
---

The canonical patch description presents the reason for the change,
and what the patch does (not necessarily how).

It was just to keep trace of the kind of display it was. As Florian
told the 63" does not have the same PID but not also the same
protocol.
Otherwise, we can also change the name of the define, but I'm not sure
on the rule to apply here.

Oh, I meant the patch description simply needs to be a bit more
self-contained. Keeping special information as what you write is
great, it just needs to in addition tell things like: "The IrTouch
infrared panels currently lack kernel support", and/or "This patch
adds support for IrTouch 42''".

Thanks,
Henrik