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

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

[Benjamin Tissoires]

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 <benjamin.tissoires@enac.fr>
---
 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

[PATCH 2/2] hid-multitouch: introduce IrTouch Infrared USB device

[Benjamin Tissoires]

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 <v.zhuk@acs-ltd.ru>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 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

Re: [2/2] hid-multitouch: introduce IrTouch Infrared USB device

[Florian Echtler]

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

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

Re: [2/2] hid-multitouch: introduce IrTouch Infrared USB device

[Benjamin Tissoires]

Hi Florian,


On Fri, Jan 28, 2011 at 17:24, Florian Echtler <floe@butterbrot.org> wrote:
> 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?

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

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

Re: [PATCH 2/2] hid-multitouch: introduce IrTouch Infrared USB device

[Henrik Rydberg]

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 <v.zhuk@acs-ltd.ru>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---

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

>  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

Re: [PATCH 2/2] hid-multitouch: introduce IrTouch Infrared USB device

[Benjamin Tissoires]

On Fri, Jan 28, 2011 at 18:21, Henrik Rydberg <rydberg@euromail.se> wrote:
> 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 <v.zhuk@acs-ltd.ru>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>
> 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

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

Re: [PATCH 2/2] hid-multitouch: introduce IrTouch Infrared USB device

[Henrik Rydberg]

On Fri, Jan 28, 2011 at 06:59:13PM +0100, Benjamin Tissoires wrote:
> On Fri, Jan 28, 2011 at 18:21, Henrik Rydberg <rydberg@euromail.se> wrote:
> > 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 <v.zhuk@acs-ltd.ru>
> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> >> ---
> >
> > 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

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

[Henrik Rydberg]

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 <benjamin.tissoires@enac.fr>
> ---

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.

>  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>");

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

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

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

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

[Benjamin Tissoires]

On Fri, Jan 28, 2011 at 18:18, Henrik Rydberg <rydberg@euromail.se> wrote:
> 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?

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.

>
>> 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 <benjamin.tissoires@enac.fr>
>> ---
>
> 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

>
>>  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>");
>
> Adding this is fine, based on your prior contributions. Perhaps it
> should be motivated as such in a separate patch instead.

ok

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


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

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

[Henrik Rydberg]

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

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