All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Input: synaptics - multitouch and multifinger support
@ 2010-10-08 14:57 Chase Douglas
  2010-10-08 14:57 ` [PATCH 1/3] Input: synaptics - add multitouch support Chase Douglas
  2010-10-08 16:37 ` [PATCH 0/3] Input: synaptics - multitouch and multifinger support Takashi Iwai
  0 siblings, 2 replies; 43+ messages in thread
From: Chase Douglas @ 2010-10-08 14:57 UTC (permalink / raw)
  To: linux-input, xorg-devel
  Cc: Dmitry Torokhov, Takashi Iwai, Chris Bagwell, Andy Whitcroft,
	Henrik Rydberg, linux-kernel, Peter Hutterer, Duncan McGreggor

Tobyn Bertram reverse engineered the multitouch protocol for Synaptics devices.
I've been able to take his work and produce a series of commits to enable MT
and multifinger (MF) support.

Unfortunately, there's a tricky issue with some Synaptics touchpads that have
integrated buttons. For example, the left and right buttons on the touchpad of
my Dell Mini 1012 consist of the lower ~20% of the touchpad surface. The
touchpad physically clicks under these areas.

The X synaptics input module now has a parameter to disable touches occuring
over the button area, but this solution still doesn't work perfectly. If you
click a button and drag with another finger near the clicking finger, the
touchpad gets confused.

Now that we have full MT support, we can try to handle this scenario better.
What I've found to work best is to make touches vanish if they occur over the
button area of the trackpad while any button is held. This works in conjunction
with the X synaptics driver to disable single touch control over the button
area. With full MT support, the touchpad doesn't seem to get confused when a
click and drag occurs with two fingers close to each other, and it enables MT
gestures and MF support across the entire trackpad when no buttons are held.

The first question is whether this seems appropriate to others, or if some
other method would work better. Secondarily, should the solution occur in the
kernel, like I have in the third patch of this series, or should it occur in
the X input module? Although we don't have this information today, we may be
able to query the touchpad in the future to know the area of the integrated
buttons. If that were possible, would the recommended location for the hack
change?

Thanks,

-- Chase

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 1/3] Input: synaptics - add multitouch support
  2010-10-08 14:57 [PATCH 0/3] Input: synaptics - multitouch and multifinger support Chase Douglas
@ 2010-10-08 14:57 ` Chase Douglas
  2010-10-08 14:57   ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chase Douglas
                     ` (2 more replies)
  2010-10-08 16:37 ` [PATCH 0/3] Input: synaptics - multitouch and multifinger support Takashi Iwai
  1 sibling, 3 replies; 43+ messages in thread
From: Chase Douglas @ 2010-10-08 14:57 UTC (permalink / raw)
  To: linux-input, xorg-devel
  Cc: Dmitry Torokhov, Takashi Iwai, Chris Bagwell, Andy Whitcroft,
	Henrik Rydberg, linux-kernel, Peter Hutterer, Duncan McGreggor

Newer Synaptics devices support multitouch. It appears there is no touch
tracking, so the non-slotted protocol is used.

Multitouch mode is disabled by default because it makes click-and-drag
on touchpads with integrated buttons even more difficult than it already
is. Maybe if/when the X synaptics input module works around this issue
we can enable it by default.

Credit goes to Tobyn Bertram for reverse engineering the protocol.

Reported-by: Tobyn Bertram
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/input/mouse/synaptics.c |   78 +++++++++++++++++++++++++++++++++++----
 drivers/input/mouse/synaptics.h |    3 +
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 96b70a4..990598f 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -44,6 +44,10 @@
 #define YMIN_NOMINAL 1408
 #define YMAX_NOMINAL 4448
 
+static bool synaptics_multitouch;
+module_param(synaptics_multitouch, bool, 0644);
+MODULE_PARM_DESC(synaptics_multitouch,
+		 "Enable multitouch mode on Synaptics devices");
 
 /*****************************************************************************
  *	Stuff we need even when we do not want native Synaptics support
@@ -279,6 +283,22 @@ static void synaptics_set_rate(struct psmouse *psmouse, unsigned int rate)
 	synaptics_mode_cmd(psmouse, priv->mode);
 }
 
+static void synaptics_set_multitouch_mode(struct psmouse *psmouse)
+{
+	static unsigned char param = 0xc8;
+	struct synaptics_data *priv = psmouse->private;
+
+	if (!SYN_CAP_MULTITOUCH(priv->ext_cap_0c) || !synaptics_multitouch)
+		return;
+	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))
+		return;
+	if (ps2_command(&psmouse->ps2dev, &param, PSMOUSE_CMD_SETRATE))
+		return;
+
+	priv->multitouch = 1;
+	printk(KERN_INFO "Synaptics: Multitouch mode enabled\n");
+}
+
 /*****************************************************************************
  *	Synaptics pass-through PS/2 port support
  ****************************************************************************/
@@ -362,18 +382,30 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
 	memset(hw, 0, sizeof(struct synaptics_hw_state));
 
 	if (SYN_MODEL_NEWABS(priv->model_id)) {
-		hw->x = (((buf[3] & 0x10) << 8) |
-			 ((buf[1] & 0x0f) << 8) |
-			 buf[4]);
-		hw->y = (((buf[3] & 0x20) << 7) |
-			 ((buf[1] & 0xf0) << 4) |
-			 buf[5]);
-
-		hw->z = buf[2];
 		hw->w = (((buf[0] & 0x30) >> 2) |
 			 ((buf[0] & 0x04) >> 1) |
 			 ((buf[3] & 0x04) >> 2));
 
+		if (SYN_MULTITOUCH(priv, hw)) {
+			/* Multitouch data is half normal resolution */
+			hw->x = (((buf[4] & 0x0f) << 8) |
+				 buf[1]) << 1;
+			hw->y = (((buf[4] & 0xf0) << 4) |
+				 buf[2]) << 1;
+
+			hw->z = ((buf[3] & 0x30) |
+				 (buf[5] & 0x0f)) << 1;
+		} else {
+			hw->x = (((buf[3] & 0x10) << 8) |
+				 ((buf[1] & 0x0f) << 8) |
+				 buf[4]);
+			hw->y = (((buf[3] & 0x20) << 7) |
+				 ((buf[1] & 0xf0) << 4) |
+				 buf[5]);
+
+			hw->z = buf[2];
+		}
+
 		hw->left  = (buf[0] & 0x01) ? 1 : 0;
 		hw->right = (buf[0] & 0x02) ? 1 : 0;
 
@@ -445,6 +477,18 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 
 	synaptics_parse_hw_state(psmouse->packet, priv, &hw);
 
+	if (SYN_MULTITOUCH(priv, &hw)) {
+		if (hw.z > 0) {
+			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
+			input_report_abs(dev, ABS_MT_POSITION_Y,
+					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
+		}
+
+		input_mt_sync(dev);
+		return;
+	}
+
 	if (hw.scroll) {
 		priv->scroll += hw.scroll;
 
@@ -499,6 +543,12 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	if (hw.z > 0) {
 		input_report_abs(dev, ABS_X, hw.x);
 		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+		if (priv->multitouch) {
+			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
+			input_report_abs(dev, ABS_MT_POSITION_Y,
+					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
+		}
 	}
 	input_report_abs(dev, ABS_PRESSURE, hw.z);
 
@@ -525,6 +575,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
 		input_report_key(dev, BTN_0 + i, hw.ext_buttons & (1 << i));
 
+	if (priv->multitouch)
+		input_mt_sync(dev);
 	input_sync(dev);
 }
 
@@ -605,6 +657,14 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 			     YMIN_NOMINAL, priv->y_max ?: YMAX_NOMINAL, 0, 0);
 	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
 
+	if (priv->multitouch) {
+		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
+				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
+				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+	}
+
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
 		input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
 
@@ -745,6 +805,8 @@ int synaptics_init(struct psmouse *psmouse)
 		goto init_fail;
 	}
 
+	synaptics_set_multitouch_mode(psmouse);
+
 	priv->pkt_type = SYN_MODEL_NEWABS(priv->model_id) ? SYN_NEWABS : SYN_OLDABS;
 
 	printk(KERN_INFO "Synaptics Touchpad, model: %ld, fw: %ld.%ld, id: %#lx, caps: %#lx/%#lx/%#lx\n",
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index b6aa7d2..5126c9c 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -53,6 +53,7 @@
 #define SYN_CAP_PRODUCT_ID(ec)		(((ec) & 0xff0000) >> 16)
 #define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100100)
 #define SYN_CAP_MAX_DIMENSIONS(ex0c)	((ex0c) & 0x020000)
+#define SYN_CAP_MULTITOUCH(ex0c)	((ex0c) & 0x080000)
 
 /* synaptics modes query bits */
 #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
@@ -78,6 +79,7 @@
 #define SYN_NEWABS_STRICT		1
 #define SYN_NEWABS_RELAXED		2
 #define SYN_OLDABS			3
+#define SYN_MULTITOUCH(priv, hw)	((priv)->multitouch && (hw)->w == 2)
 
 /*
  * A structure to describe the state of the touchpad hardware (buttons and pad)
@@ -110,6 +112,7 @@ struct synaptics_data {
 	unsigned char pkt_type;			/* packet type - old, new, etc */
 	unsigned char mode;			/* current mode byte */
 	int scroll;
+	int multitouch;				/* Whether device provides MT */
 };
 
 void synaptics_module_init(void);
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 2/3] Input: synaptics - add multitouch multifinger support
  2010-10-08 14:57 ` [PATCH 1/3] Input: synaptics - add multitouch support Chase Douglas
@ 2010-10-08 14:57   ` Chase Douglas
  2010-10-08 14:58     ` [PATCH 3/3] Input: synaptics - remove touches over button click area Chase Douglas
  2010-10-10 15:44     ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chris Bagwell
  2010-10-10 15:37   ` [PATCH 1/3] Input: synaptics - add multitouch support Chris Bagwell
  2010-10-10 15:41   ` Chris Bagwell
  2 siblings, 2 replies; 43+ messages in thread
From: Chase Douglas @ 2010-10-08 14:57 UTC (permalink / raw)
  To: linux-input, xorg-devel
  Cc: Dmitry Torokhov, Takashi Iwai, Chris Bagwell, Andy Whitcroft,
	Henrik Rydberg, linux-kernel, Peter Hutterer, Duncan McGreggor

Newer multitouch Synaptics trackpads do not advertise multifinger
support. Now that we have multitouch support, we can use the number of
touches to report multifinger functionality.

In conjunction with the X synaptics input module, this enables
functionality such as two finger scrolling.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/input/mouse/synaptics.c |   24 +++++++++++++-----------
 drivers/input/mouse/synaptics.h |    1 +
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 990598f..7289d88 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -471,7 +471,6 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct synaptics_data *priv = psmouse->private;
 	struct synaptics_hw_state hw;
-	int num_fingers;
 	int finger_width;
 	int i;
 
@@ -483,6 +482,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 			input_report_abs(dev, ABS_MT_POSITION_Y,
 					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
 			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
+			priv->num_fingers++;
 		}
 
 		input_mt_sync(dev);
@@ -510,13 +510,13 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	}
 
 	if (hw.z > 0) {
-		num_fingers = 1;
+		priv->num_fingers++;
 		finger_width = 5;
 		if (SYN_CAP_EXTENDED(priv->capabilities)) {
 			switch (hw.w) {
 			case 0 ... 1:
 				if (SYN_CAP_MULTIFINGER(priv->capabilities))
-					num_fingers = hw.w + 2;
+					priv->num_fingers = hw.w + 2;
 				break;
 			case 2:
 				if (SYN_MODEL_PEN(priv->model_id))
@@ -528,10 +528,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 				break;
 			}
 		}
-	} else {
-		num_fingers = 0;
+	} else
 		finger_width = 0;
-	}
 
 	/* Post events
 	 * BTN_TOUCH has to be first as mousedev relies on it when doing
@@ -555,15 +553,19 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
 		input_report_abs(dev, ABS_TOOL_WIDTH, finger_width);
 
-	input_report_key(dev, BTN_TOOL_FINGER, num_fingers == 1);
+	input_report_key(dev, BTN_TOOL_FINGER, priv->num_fingers == 1);
 	input_report_key(dev, BTN_LEFT, hw.left);
 	input_report_key(dev, BTN_RIGHT, hw.right);
 
-	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
-		input_report_key(dev, BTN_TOOL_DOUBLETAP, num_fingers == 2);
-		input_report_key(dev, BTN_TOOL_TRIPLETAP, num_fingers == 3);
+	if (SYN_CAP_MULTIFINGER(priv->capabilities) || priv->multitouch) {
+		input_report_key(dev, BTN_TOOL_DOUBLETAP,
+				 priv->num_fingers == 2);
+		input_report_key(dev, BTN_TOOL_TRIPLETAP,
+				 priv->num_fingers == 3);
 	}
 
+	priv->num_fingers = 0;
+
 	if (SYN_CAP_MIDDLE_BUTTON(priv->capabilities))
 		input_report_key(dev, BTN_MIDDLE, hw.middle);
 
@@ -674,7 +676,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 	__set_bit(BTN_LEFT, dev->keybit);
 	__set_bit(BTN_RIGHT, dev->keybit);
 
-	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
+	if (SYN_CAP_MULTIFINGER(priv->capabilities) || priv->multitouch) {
 		__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
 		__set_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
 	}
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 5126c9c..0989b8d 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -113,6 +113,7 @@ struct synaptics_data {
 	unsigned char mode;			/* current mode byte */
 	int scroll;
 	int multitouch;				/* Whether device provides MT */
+	unsigned int num_fingers;		/* Number of fingers touching */
 };
 
 void synaptics_module_init(void);
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-08 14:57   ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chase Douglas
@ 2010-10-08 14:58     ` Chase Douglas
  2010-10-10 15:58       ` Chris Bagwell
  2010-10-11 16:24         ` Chris Bagwell
  2010-10-10 15:44     ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chris Bagwell
  1 sibling, 2 replies; 43+ messages in thread
From: Chase Douglas @ 2010-10-08 14:58 UTC (permalink / raw)
  To: linux-input, xorg-devel
  Cc: Dmitry Torokhov, Takashi Iwai, Chris Bagwell, Andy Whitcroft,
	Henrik Rydberg, linux-kernel, Peter Hutterer, Duncan McGreggor

Now that we have proper multitouch support, we can handle integrated
buttons better. If we know the top of the buttons on the touchpad, we
can ignore any touches that occur within the touchpad area while a
button is clicked. It may be possible to get the button area by querying
the device, but for now allow the user to manually set it.

A note on why this works: the Synaptics touchpads have pseudo touch
tracking. When two touches are on the touchpad, an MT touch packet with
just the X, Y, and pressure values is sent before a normal Synaptics
touch packet. When one touch is obviously in motion and the other is
stationary, the touchpad controller sends the touch in motion in the
normal packet and the stationary touch in the MT packet. Single touch
emulation is provided by the normal packet, so an action like clicking
a button and dragging with another finger still works as expected.

Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
synaptics_button_thresh=4100.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 7289d88..e67778d 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
 MODULE_PARM_DESC(synaptics_multitouch,
 		 "Enable multitouch mode on Synaptics devices");
 
+static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
+module_param(synaptics_button_thresh, int, 0644);
+MODULE_PARM_DESC(synaptics_button_thres,
+		 "Y coordinate threshold of integrated buttons on Synaptics "
+		 "devices");
+
 /*****************************************************************************
  *	Stuff we need even when we do not want native Synaptics support
  ****************************************************************************/
@@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
 	}
 }
 
+#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
+			       (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
+				synaptics_button_thresh))
+
 /*
  *  called for each full received packet from the touchpad
  */
@@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	synaptics_parse_hw_state(psmouse->packet, priv, &hw);
 
 	if (SYN_MULTITOUCH(priv, &hw)) {
-		if (hw.z > 0) {
+		if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
 			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
 			input_report_abs(dev, ABS_MT_POSITION_Y,
 					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
@@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 		return;
 	}
 
+	/* If touch occurs over depressed button, ignore it */
+	if (TOUCH_OVER_BUTTON(hw))
+		hw.z = 0;
+
 	if (hw.z > 0) {
 		priv->num_fingers++;
 		finger_width = 5;
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 14:57 [PATCH 0/3] Input: synaptics - multitouch and multifinger support Chase Douglas
  2010-10-08 14:57 ` [PATCH 1/3] Input: synaptics - add multitouch support Chase Douglas
@ 2010-10-08 16:37 ` Takashi Iwai
  2010-10-08 16:38   ` Takashi Iwai
                     ` (2 more replies)
  1 sibling, 3 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-08 16:37 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Fri,  8 Oct 2010 10:57:57 -0400,
Chase Douglas wrote:
> 
> Tobyn Bertram reverse engineered the multitouch protocol for Synaptics devices.
> I've been able to take his work and produce a series of commits to enable MT
> and multifinger (MF) support.
> 
> Unfortunately, there's a tricky issue with some Synaptics touchpads that have
> integrated buttons. For example, the left and right buttons on the touchpad of
> my Dell Mini 1012 consist of the lower ~20% of the touchpad surface. The
> touchpad physically clicks under these areas.
> 
> The X synaptics input module now has a parameter to disable touches occuring
> over the button area, but this solution still doesn't work perfectly. If you
> click a button and drag with another finger near the clicking finger, the
> touchpad gets confused.
> 
> Now that we have full MT support, we can try to handle this scenario better.
> What I've found to work best is to make touches vanish if they occur over the
> button area of the trackpad while any button is held. This works in conjunction
> with the X synaptics driver to disable single touch control over the button
> area. With full MT support, the touchpad doesn't seem to get confused when a
> click and drag occurs with two fingers close to each other, and it enables MT
> gestures and MF support across the entire trackpad when no buttons are held.
> 
> The first question is whether this seems appropriate to others, or if some
> other method would work better. Secondarily, should the solution occur in the
> kernel, like I have in the third patch of this series, or should it occur in
> the X input module? Although we don't have this information today, we may be
> able to query the touchpad in the future to know the area of the integrated
> buttons. If that were possible, would the recommended location for the hack
> change?

Great!  Finally someone found it out!
I found this and made a series of patches in 4 months ago.  Since
then, Novell legal prohibited me to send the patches to the upstream
due to "possible patent infringing".  Now you cracked out.  Yay.

FWIW, my corresponding patch is below.  It really looks similar in the
end ;)  I added a kconfig just to be safer.

Regarding the "clickpad" support: in my case, I implemented almost
everything about it in xorg driver.  I'm going to submit xorg
patches.


thanks,

Takashi

---
>From 4c88fb69bdfb20a6ad030c7a19eed1e76beb0442 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Mon, 14 Jun 2010 12:46:48 +0200
Subject: [PATCH] input: Add multi-touch protocol support to Synaptics

This patch adds the experimental support of the multi-touch protocol
for recent Synaptics devices.  Note that the protocol was analyzed just
by a pure guess work by watching device outputs, so it might be incorrect.
Also, the check of the multi-touch capability based on the 0x0c caps
might be also wrong.

In the multi-touch mode, the driver gives MT_POSITION_X, MT_POSITION_Y
and MT_PRESSURE abs events to follow the type A in
Documentation/input/multi-touch-protocol.txt.
The device supports up to two finger points, as far as I've tested.

As the driver now gives the MT_* events, this extension might result in
the incompatible event outputs.  Thus make sure that the user-space side
(e.g. X11 synaptics driver) is also updated to support MT_* events.

Signed-off-by: Takashi Iwai <tiwai@suse.de>

---
 drivers/input/mouse/Kconfig     |   11 ++++
 drivers/input/mouse/synaptics.c |  113 +++++++++++++++++++++++++++++++++-----
 drivers/input/mouse/synaptics.h |    1 +
 3 files changed, 110 insertions(+), 15 deletions(-)

diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
index 91d3517..6dd0d29 100644
--- a/drivers/input/mouse/Kconfig
+++ b/drivers/input/mouse/Kconfig
@@ -76,6 +76,17 @@ config MOUSE_PS2_SYNAPTICS_LED
 	  Say Y here if you have a Synaptics device with an embedded LED.
 	  This will enable LED class driver to control the LED device.
 
+config MOUSE_PS2_SYNAPTICS_MULTI_TOUCH
+	bool "Support multi-touch protocol of Synaptics devices"
+	depends on MOUSE_PS2_SYNAPTICS
+	help
+	  Say Y here for enabling the multi-touch protocol of recent
+	  Syanptics devices.  This may result in incompatible input
+	  events, thus make sure that you update X11 synaptics driver
+	  beforehand with the multi-protocol touch.
+
+	  If unsure, say N.
+
 config MOUSE_PS2_LIFEBOOK
 	bool "Fujitsu Lifebook PS/2 mouse protocol extension" if EMBEDDED
 	default y
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 00799bc..79d9463 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -181,6 +181,12 @@ static int synaptics_capability(struct psmouse *psmouse)
 		}
 	}
 
+#ifdef CONFIG_MOUSE_PS2_SYNAPTICS_MULTI_TOUCH
+	/* FIXME: is this the right guess? */
+	if (priv->ext_cap_0c & (0x08 << 16))
+		priv->can_multi_touch = 1;
+#endif
+
 	return 0;
 }
 
@@ -458,6 +464,32 @@ static void synaptics_free_led(struct psmouse *psmouse)
 #define synaptics_sync_led(ps)	do {} while (0)
 #endif
 
+/* change to the multi-touch mode;
+ * this is done by sending SYN_QUE_MODEL cmd but setting a parameter
+ * by SETRATE instead of querying via GETINFO.
+ * 0xc8 seems to be the multi-touch mode.
+ */
+static int synaptics_init_multi_touch(struct psmouse *psmouse)
+{
+	unsigned char param[1];
+
+	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))
+		return -1;
+	param[0] = 0xc8;
+	if (ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_SETRATE))
+		return -1;
+	return 0;
+}
+
+#ifdef CONFIG_MOUSE_PS2_SYNAPTICS_MULTI_TOUCH
+#define is_multi_touch(priv)	(priv)->can_multi_touch
+#else
+#define is_multi_touch(priv)	0
+#endif
+/* the multi-touch packet contains w=2 (like pen) */
+#define is_multi_touch_packet(priv, hw) \
+	(is_multi_touch(priv) && (hw)->w == 2)
+
 /*****************************************************************************
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
@@ -467,17 +499,27 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
 	memset(hw, 0, sizeof(struct synaptics_hw_state));
 
 	if (SYN_MODEL_NEWABS(priv->model_id)) {
-		hw->x = (((buf[3] & 0x10) << 8) |
-			 ((buf[1] & 0x0f) << 8) |
-			 buf[4]);
-		hw->y = (((buf[3] & 0x20) << 7) |
-			 ((buf[1] & 0xf0) << 4) |
-			 buf[5]);
-
-		hw->z = buf[2];
 		hw->w = (((buf[0] & 0x30) >> 2) |
 			 ((buf[0] & 0x04) >> 1) |
 			 ((buf[3] & 0x04) >> 2));
+		if (is_multi_touch_packet(priv, hw)) {
+			/* a multi-touch packet is encoded differently;
+			 * it appears to have half-resolutions.  I might
+			 * have missed the lowest bits, but it's hard
+			 * to recognize.
+			 */
+			hw->x = ((buf[4] & 0x0f) << 8 | buf[1]) << 1;
+			hw->y = ((buf[4] & 0xf0) << 4 | buf[2]) << 1;
+			hw->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+		} else {
+			hw->x = (((buf[3] & 0x10) << 8) |
+				 ((buf[1] & 0x0f) << 8) |
+				 buf[4]);
+			hw->y = (((buf[3] & 0x20) << 7) |
+				 ((buf[1] & 0xf0) << 4) |
+				 buf[5]);
+			hw->z = buf[2];
+		}
 
 		hw->left  = (buf[0] & 0x01) ? 1 : 0;
 		hw->right = (buf[0] & 0x02) ? 1 : 0;
@@ -492,7 +534,7 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
 
 		} else if (SYN_CAP_MIDDLE_BUTTON(priv->capabilities)) {
 			hw->middle = ((buf[0] ^ buf[3]) & 0x01) ? 1 : 0;
-			if (hw->w == 2)
+			if (!is_multi_touch(priv) && hw->w == 2)
 				hw->scroll = (signed char)(buf[1]);
 		}
 
@@ -550,6 +592,18 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 
 	synaptics_parse_hw_state(psmouse->packet, priv, &hw);
 
+	if (is_multi_touch_packet(priv, &hw)) {
+		/* multi-touching */
+		if (hw.z > 0) {
+			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
+			input_report_abs(dev, ABS_MT_POSITION_Y,
+					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+		}
+		input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
+		input_mt_sync(dev);
+		return;
+	}
+
 	if (hw.scroll) {
 		priv->scroll += hw.scroll;
 
@@ -576,7 +630,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 		if (SYN_CAP_EXTENDED(priv->capabilities)) {
 			switch (hw.w) {
 			case 0 ... 1:
-				if (SYN_CAP_MULTIFINGER(priv->capabilities))
+				if (SYN_CAP_MULTIFINGER(priv->capabilities) ||
+				    is_multi_touch(priv))
 					num_fingers = hw.w + 2;
 				break;
 			case 2:
@@ -602,10 +657,15 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	if (hw.z < 25) input_report_key(dev, BTN_TOUCH, 0);
 
 	if (hw.z > 0) {
-		input_report_abs(dev, ABS_X, hw.x);
-		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+		int key;
+		key = is_multi_touch(priv) ? ABS_MT_POSITION_X : ABS_X;
+		input_report_abs(dev, key, hw.x);
+		key = is_multi_touch(priv) ? ABS_MT_POSITION_Y : ABS_Y;
+		input_report_abs(dev, key, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
 	}
-	input_report_abs(dev, ABS_PRESSURE, hw.z);
+	input_report_abs(dev,
+			 is_multi_touch(priv) ? ABS_MT_PRESSURE : ABS_PRESSURE,
+			 hw.z);
 
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
 		input_report_abs(dev, ABS_TOOL_WIDTH, finger_width);
@@ -614,7 +674,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	input_report_key(dev, BTN_LEFT, hw.left);
 	input_report_key(dev, BTN_RIGHT, hw.right);
 
-	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
+	if (SYN_CAP_MULTIFINGER(priv->capabilities) || is_multi_touch(priv)) {
 		input_report_key(dev, BTN_TOOL_DOUBLETAP, num_fingers == 2);
 		input_report_key(dev, BTN_TOOL_TRIPLETAP, num_fingers == 3);
 	}
@@ -630,6 +690,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
 		input_report_key(dev, BTN_0 + i, hw.ext_buttons & (1 << i));
 
+	if (is_multi_touch(priv))
+		input_mt_sync(dev);
 	input_sync(dev);
 }
 
@@ -719,7 +781,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 	__set_bit(BTN_LEFT, dev->keybit);
 	__set_bit(BTN_RIGHT, dev->keybit);
 
-	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
+	if (SYN_CAP_MULTIFINGER(priv->capabilities) || is_multi_touch(priv)) {
 		__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
 		__set_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
 	}
@@ -748,6 +810,16 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 		__clear_bit(BTN_RIGHT, dev->keybit);
 		__clear_bit(BTN_MIDDLE, dev->keybit);
 	}
+
+	if (is_multi_touch(priv)) {
+		input_set_abs_params(dev, ABS_MT_POSITION_X,
+				     XMIN_NOMINAL, XMAX_NOMINAL, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y,
+				     YMIN_NOMINAL, YMAX_NOMINAL, 0, 0);
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
+		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
+	}
 }
 
 static void synaptics_disconnect(struct psmouse *psmouse)
@@ -785,6 +857,8 @@ static int synaptics_reconnect(struct psmouse *psmouse)
 	}
 
 	synaptics_sync_led(psmouse);
+	if (is_multi_touch(priv))
+		synaptics_init_multi_touch(psmouse);
 
 	return 0;
 }
@@ -863,6 +937,15 @@ int synaptics_init(struct psmouse *psmouse)
 	if (synaptics_init_led(psmouse) < 0)
 		goto init_fail;
 
+	if (priv->can_multi_touch) {
+		if (synaptics_init_multi_touch(psmouse)) {
+			printk(KERN_WARNING "Synaptics: "
+			       "unable to initialize multi-touch\n");
+			priv->can_multi_touch = 0;
+		} else
+			printk(KERN_INFO "Synaptics: multi-touch enabled\n");
+	}
+
 	set_input_params(psmouse->dev, priv);
 
 	/*
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index e1a9033..b586087 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -111,6 +111,7 @@ struct synaptics_data {
 
 	unsigned char pkt_type;			/* packet type - old, new, etc */
 	unsigned char mode;			/* current mode byte */
+	unsigned char can_multi_touch;		/* multi-touch support */
 	int scroll;
 	struct synaptics_led *led;
 };
-- 
1.7.2.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 16:37 ` [PATCH 0/3] Input: synaptics - multitouch and multifinger support Takashi Iwai
@ 2010-10-08 16:38   ` Takashi Iwai
  2010-10-08 17:48     ` Takashi Iwai
  2010-10-08 17:15   ` Chase Douglas
  2010-10-10  7:49   ` Henrik Rydberg
  2 siblings, 1 reply; 43+ messages in thread
From: Takashi Iwai @ 2010-10-08 16:38 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Fri, 08 Oct 2010 18:37:22 +0200,
Takashi Iwai wrote:
> 
> At Fri,  8 Oct 2010 10:57:57 -0400,
> Chase Douglas wrote:
> > 
> > Tobyn Bertram reverse engineered the multitouch protocol for Synaptics devices.
> > I've been able to take his work and produce a series of commits to enable MT
> > and multifinger (MF) support.
> > 
> > Unfortunately, there's a tricky issue with some Synaptics touchpads that have
> > integrated buttons. For example, the left and right buttons on the touchpad of
> > my Dell Mini 1012 consist of the lower ~20% of the touchpad surface. The
> > touchpad physically clicks under these areas.
> > 
> > The X synaptics input module now has a parameter to disable touches occuring
> > over the button area, but this solution still doesn't work perfectly. If you
> > click a button and drag with another finger near the clicking finger, the
> > touchpad gets confused.
> > 
> > Now that we have full MT support, we can try to handle this scenario better.
> > What I've found to work best is to make touches vanish if they occur over the
> > button area of the trackpad while any button is held. This works in conjunction
> > with the X synaptics driver to disable single touch control over the button
> > area. With full MT support, the touchpad doesn't seem to get confused when a
> > click and drag occurs with two fingers close to each other, and it enables MT
> > gestures and MF support across the entire trackpad when no buttons are held.
> > 
> > The first question is whether this seems appropriate to others, or if some
> > other method would work better. Secondarily, should the solution occur in the
> > kernel, like I have in the third patch of this series, or should it occur in
> > the X input module? Although we don't have this information today, we may be
> > able to query the touchpad in the future to know the area of the integrated
> > buttons. If that were possible, would the recommended location for the hack
> > change?
> 
> Great!  Finally someone found it out!
> I found this and made a series of patches in 4 months ago.  Since
> then, Novell legal prohibited me to send the patches to the upstream
> due to "possible patent infringing".  Now you cracked out.  Yay.
> 
> FWIW, my corresponding patch is below.  It really looks similar in the
> end ;)  I added a kconfig just to be safer.
> 
> Regarding the "clickpad" support: in my case, I implemented almost
> everything about it in xorg driver.  I'm going to submit xorg
> patches.

BTW, yet another kernel patch is missing; the support of embedded LED.
I've posted this once, but it seems forgotten since then.  Reposted
below.


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH 2/2] input: Add LED support to Synaptics device

The new Synaptics devices have an LED on the top-left corner.
This patch adds a new LED class device to control it.  It's created
dynamically upon synaptics device probing.

The LED is controlled via the command 0x0a with parameters 0x88 or 0x10.
This seems only on/off control although other value might be accepted.

The detection of the LED isn't clear yet.  It should have been the new
capability bits that indicate the presence, but on real machines, it
doesn't fit.  So, for the time being, the driver checks the product id
in the ext capability bits and assumes that LED exists on the known
devices.

Signed-off-by: Takashi Iwai <tiwai@suse.de>

---
 drivers/input/mouse/Kconfig     |    9 +++
 drivers/input/mouse/synaptics.c |  111 ++++++++++++++++++++++++++++++++++++++++
 drivers/input/mouse/synaptics.h |    3 +
 3 files changed, 123 insertions(+)

--- a/drivers/input/mouse/Kconfig
+++ b/drivers/input/mouse/Kconfig
@@ -19,6 +19,7 @@ config MOUSE_PS2
 	select SERIO_LIBPS2
 	select SERIO_I8042 if X86
 	select SERIO_GSCPS2 if GSC
+	select LEDS_CLASS if MOUSE_PS2_SYNAPICS_LED
 	help
 	  Say Y here if you have a PS/2 mouse connected to your system. This
 	  includes the standard 2 or 3-button PS/2 mouse, as well as PS/2
@@ -67,6 +68,14 @@ config MOUSE_PS2_SYNAPTICS
 
 	  If unsure, say Y.
 
+config MOUSE_PS2_SYNAPTICS_LED
+	bool "Support embedded LED on Synaptics devices"
+	depends on MOUSE_PS2_SYNAPTICS
+	select NEW_LEDS
+	help
+	  Say Y here if you have a Synaptics device with an embedded LED.
+	  This will enable LED class driver to control the LED device.
+
 config MOUSE_PS2_LIFEBOOK
 	bool "Fujitsu Lifebook PS/2 mouse protocol extension" if EMBEDDED
 	default y
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -28,6 +28,7 @@
 #include <linux/input.h>
 #include <linux/serio.h>
 #include <linux/libps2.h>
+#include <linux/leds.h>
 #include <linux/slab.h>
 #include "psmouse.h"
 #include "synaptics.h"
@@ -353,6 +354,110 @@ static void synaptics_pt_create(struct p
 	serio_register_port(serio);
 }
 
+#ifdef CONFIG_MOUSE_PS2_SYNAPTICS_LED
+/*
+ * LED handling:
+ * Some Synaptics devices have an embeded LED at the top-left corner.
+ */
+
+struct synaptics_led {
+	struct psmouse *psmouse;
+	struct work_struct work;
+	struct led_classdev cdev;
+};
+
+static void synaptics_set_led(struct psmouse *psmouse, int on)
+{
+	int i;
+	unsigned char cmd = on ? 0x88 : 0x10;
+
+	ps2_begin_command(&psmouse->ps2dev);
+	if (__ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11))
+		goto out;
+	for (i = 6; i >= 0; i -= 2) {
+		unsigned char d = (cmd >> i) & 3;
+		if (__ps2_command(&psmouse->ps2dev, &d, PSMOUSE_CMD_SETRES))
+			goto out;
+	}
+	cmd = 0x0a;
+	__ps2_command(&psmouse->ps2dev, &cmd, PSMOUSE_CMD_SETRATE);
+ out:
+	ps2_end_command(&psmouse->ps2dev);
+}
+
+static void synaptics_led_work(struct work_struct *work)
+{
+	struct synaptics_led *led;
+
+	led = container_of(work, struct synaptics_led, work);
+	synaptics_set_led(led->psmouse, led->cdev.brightness);
+}
+
+static void synaptics_led_cdev_brightness_set(struct led_classdev *cdev,
+					      enum led_brightness value)
+{
+	struct synaptics_led *led;
+
+	led = container_of(cdev, struct synaptics_led, cdev);
+	schedule_work(&led->work);
+}
+
+static void synaptics_sync_led(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+
+	if (priv->led)
+		synaptics_set_led(psmouse, priv->led->cdev.brightness);
+}
+
+static int synaptics_init_led(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	struct synaptics_led *led;
+	int err;
+
+	/* FIXME: LED is supposedly detectable in cap0c[1] 0x20, but it seems
+	 * not working on real machines.
+	 * So we check the product id to be sure.
+	 */
+	if (!priv->ext_cap_0c || SYN_CAP_PRODUCT_ID(priv->ext_cap) != 0xe4)
+		return 0;
+
+	printk(KERN_INFO "synaptics: support LED control\n");
+	led = kzalloc(sizeof(struct synaptics_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+	led->psmouse = psmouse;
+	INIT_WORK(&led->work, synaptics_led_work);
+	led->cdev.name = "psmouse::synaptics";
+	led->cdev.brightness_set = synaptics_led_cdev_brightness_set;
+	led->cdev.flags = LED_CORE_SUSPENDRESUME;
+	err = led_classdev_register(NULL, &led->cdev);
+	if (err < 0) {
+		kfree(led);
+		return err;
+	}
+	priv->led = led;
+	return 0;
+}
+
+static void synaptics_free_led(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+
+	if (!priv->led)
+		return;
+	cancel_work_sync(&priv->led->work);
+	synaptics_set_led(psmouse, 0);
+	led_classdev_unregister(&priv->led->cdev);
+	kfree(priv->led);
+}
+#else
+#define synaptics_init_led(ps)	0
+#define synaptics_free_led(ps)	do {} while (0)
+#define synaptics_sync_led(ps)	do {} while (0)
+#endif
+
 /*****************************************************************************
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
@@ -647,6 +752,7 @@ static void set_input_params(struct inpu
 
 static void synaptics_disconnect(struct psmouse *psmouse)
 {
+	synaptics_free_led(psmouse);
 	synaptics_reset(psmouse);
 	kfree(psmouse->private);
 	psmouse->private = NULL;
@@ -678,6 +784,8 @@ static int synaptics_reconnect(struct ps
 		return -1;
 	}
 
+	synaptics_sync_led(psmouse);
+
 	return 0;
 }
 
@@ -752,6 +860,9 @@ int synaptics_init(struct psmouse *psmou
 		SYN_ID_MAJOR(priv->identity), SYN_ID_MINOR(priv->identity),
 		priv->model_id, priv->capabilities, priv->ext_cap, priv->ext_cap_0c);
 
+	if (synaptics_init_led(psmouse) < 0)
+		goto init_fail;
+
 	set_input_params(psmouse->dev, priv);
 
 	/*
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -97,6 +97,8 @@ struct synaptics_hw_state {
 	signed char scroll;
 };
 
+struct synaptics_led;
+
 struct synaptics_data {
 	/* Data read from the touchpad */
 	unsigned long int model_id;		/* Model-ID */
@@ -110,6 +112,7 @@ struct synaptics_data {
 	unsigned char pkt_type;			/* packet type - old, new, etc */
 	unsigned char mode;			/* current mode byte */
 	int scroll;
+	struct synaptics_led *led;
 };
 
 void synaptics_module_init(void);

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 16:37 ` [PATCH 0/3] Input: synaptics - multitouch and multifinger support Takashi Iwai
  2010-10-08 16:38   ` Takashi Iwai
@ 2010-10-08 17:15   ` Chase Douglas
  2010-10-08 17:46     ` Takashi Iwai
  2010-10-08 18:04     ` Dmitry Torokhov
  2010-10-10  7:49   ` Henrik Rydberg
  2 siblings, 2 replies; 43+ messages in thread
From: Chase Douglas @ 2010-10-08 17:15 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Fri, 2010-10-08 at 18:37 +0200, Takashi Iwai wrote:
> At Fri,  8 Oct 2010 10:57:57 -0400,
> Chase Douglas wrote:
> > 
> > Tobyn Bertram reverse engineered the multitouch protocol for Synaptics devices.
> > I've been able to take his work and produce a series of commits to enable MT
> > and multifinger (MF) support.
> > 
> > Unfortunately, there's a tricky issue with some Synaptics touchpads that have
> > integrated buttons. For example, the left and right buttons on the touchpad of
> > my Dell Mini 1012 consist of the lower ~20% of the touchpad surface. The
> > touchpad physically clicks under these areas.
> > 
> > The X synaptics input module now has a parameter to disable touches occuring
> > over the button area, but this solution still doesn't work perfectly. If you
> > click a button and drag with another finger near the clicking finger, the
> > touchpad gets confused.
> > 
> > Now that we have full MT support, we can try to handle this scenario better.
> > What I've found to work best is to make touches vanish if they occur over the
> > button area of the trackpad while any button is held. This works in conjunction
> > with the X synaptics driver to disable single touch control over the button
> > area. With full MT support, the touchpad doesn't seem to get confused when a
> > click and drag occurs with two fingers close to each other, and it enables MT
> > gestures and MF support across the entire trackpad when no buttons are held.
> > 
> > The first question is whether this seems appropriate to others, or if some
> > other method would work better. Secondarily, should the solution occur in the
> > kernel, like I have in the third patch of this series, or should it occur in
> > the X input module? Although we don't have this information today, we may be
> > able to query the touchpad in the future to know the area of the integrated
> > buttons. If that were possible, would the recommended location for the hack
> > change?
> 
> Great!  Finally someone found it out!
> I found this and made a series of patches in 4 months ago.  Since
> then, Novell legal prohibited me to send the patches to the upstream
> due to "possible patent infringing".  Now you cracked out.  Yay.
> 
> FWIW, my corresponding patch is below.  It really looks similar in the
> end ;)  I added a kconfig just to be safer.
> 
> Regarding the "clickpad" support: in my case, I implemented almost
> everything about it in xorg driver.  I'm going to submit xorg
> patches.

So I'm confused. I was working off of source code posted to:

https://bugs.launchpad.net/utouch/+bug/633225

I was under the impression that someone else had reverse engineered the
protocol and written patches. But the code is exactly the same as what
you've posted here. If you're the originator of the work, and my patch
is accepted, I think we'll need your SOB on it. Of course, if your patch
is accepted then the point is moot :).

My patch essentially is a rework of yours, incorporating changes that
allow for the driver to work in single touch and multitouch mode
simultaneously. As is done in other MT drivers, one touch is picked to
act as the single touch emulation.

However, as I sit here using the touchpad and thinking some more, I'm
not sure how to best handle single touch emulation for synaptics. Single
touch emulation only really works when touches are tracked. The touches
from the synaptics driver are swapped whenever one touch moves and the
other stays stationary. I think I'm noticing some issues with the
following sequence of events:

1. Two touches begin, triggering a DOUBLETAP key event
2. X synaptics treats DOUBLETAP as scroll events
3. One touch moves up, it's sent as ABS_{X,Y}, scroll performed
4. The touch in motion stops
5. Other touch moves up, it's now sent as ABS_{X,Y}
6. X synaptics scroll behaves poorly because this new touch's X,Y are in
a different place from the first touch's X,Y. Scrolling seems to snap
back to original location

Certainly it's not hard to perform touch tracking when only two touches
are active. However, Henrik, as the MT input guru, has resisted doing in
kernel tracking, at least in a hackish, per-driver manner. It's why he
wrote mtdev in user space, for example. However, unless mtdev is
extended to support single touch emulation, we're kind of stuck without
in kernel tracking.

-- Chase


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 17:15   ` Chase Douglas
@ 2010-10-08 17:46     ` Takashi Iwai
  2010-10-08 18:04     ` Dmitry Torokhov
  1 sibling, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-08 17:46 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Fri, 08 Oct 2010 13:15:35 -0400,
Chase Douglas wrote:
> 
> On Fri, 2010-10-08 at 18:37 +0200, Takashi Iwai wrote:
> > At Fri,  8 Oct 2010 10:57:57 -0400,
> > Chase Douglas wrote:
> > > 
> > > Tobyn Bertram reverse engineered the multitouch protocol for Synaptics devices.
> > > I've been able to take his work and produce a series of commits to enable MT
> > > and multifinger (MF) support.
> > > 
> > > Unfortunately, there's a tricky issue with some Synaptics touchpads that have
> > > integrated buttons. For example, the left and right buttons on the touchpad of
> > > my Dell Mini 1012 consist of the lower ~20% of the touchpad surface. The
> > > touchpad physically clicks under these areas.
> > > 
> > > The X synaptics input module now has a parameter to disable touches occuring
> > > over the button area, but this solution still doesn't work perfectly. If you
> > > click a button and drag with another finger near the clicking finger, the
> > > touchpad gets confused.
> > > 
> > > Now that we have full MT support, we can try to handle this scenario better.
> > > What I've found to work best is to make touches vanish if they occur over the
> > > button area of the trackpad while any button is held. This works in conjunction
> > > with the X synaptics driver to disable single touch control over the button
> > > area. With full MT support, the touchpad doesn't seem to get confused when a
> > > click and drag occurs with two fingers close to each other, and it enables MT
> > > gestures and MF support across the entire trackpad when no buttons are held.
> > > 
> > > The first question is whether this seems appropriate to others, or if some
> > > other method would work better. Secondarily, should the solution occur in the
> > > kernel, like I have in the third patch of this series, or should it occur in
> > > the X input module? Although we don't have this information today, we may be
> > > able to query the touchpad in the future to know the area of the integrated
> > > buttons. If that were possible, would the recommended location for the hack
> > > change?
> > 
> > Great!  Finally someone found it out!
> > I found this and made a series of patches in 4 months ago.  Since
> > then, Novell legal prohibited me to send the patches to the upstream
> > due to "possible patent infringing".  Now you cracked out.  Yay.
> > 
> > FWIW, my corresponding patch is below.  It really looks similar in the
> > end ;)  I added a kconfig just to be safer.
> > 
> > Regarding the "clickpad" support: in my case, I implemented almost
> > everything about it in xorg driver.  I'm going to submit xorg
> > patches.
> 
> So I'm confused. I was working off of source code posted to:
> 
> https://bugs.launchpad.net/utouch/+bug/633225
> 
> I was under the impression that someone else had reverse engineered the
> protocol and written patches. But the code is exactly the same as what
> you've posted here. If you're the originator of the work, and my patch
> is accepted, I think we'll need your SOB on it. Of course, if your patch
> is accepted then the point is moot :).

Hm, then this was a leak, likely taken from Novell bugzilla or build
service.  The patch was written and published once before I was
prohibited to send to upstream, so basically it was fine to go outside
by itself ;)

> My patch essentially is a rework of yours, incorporating changes that
> allow for the driver to work in single touch and multitouch mode
> simultaneously. As is done in other MT drivers, one touch is picked to
> act as the single touch emulation.
> 
> However, as I sit here using the touchpad and thinking some more, I'm
> not sure how to best handle single touch emulation for synaptics. Single
> touch emulation only really works when touches are tracked. The touches
> from the synaptics driver are swapped whenever one touch moves and the
> other stays stationary. I think I'm noticing some issues with the
> following sequence of events:
> 
> 1. Two touches begin, triggering a DOUBLETAP key event
> 2. X synaptics treats DOUBLETAP as scroll events
> 3. One touch moves up, it's sent as ABS_{X,Y}, scroll performed
> 4. The touch in motion stops
> 5. Other touch moves up, it's now sent as ABS_{X,Y}
> 6. X synaptics scroll behaves poorly because this new touch's X,Y are in
> a different place from the first touch's X,Y. Scrolling seems to snap
> back to original location
> 
> Certainly it's not hard to perform touch tracking when only two touches
> are active. However, Henrik, as the MT input guru, has resisted doing in
> kernel tracking, at least in a hackish, per-driver manner. It's why he
> wrote mtdev in user space, for example. However, unless mtdev is
> extended to support single touch emulation, we're kind of stuck without
> in kernel tracking.

Yeah, there are lots of rooms regarding the handling of multi-touch
touchpad.

I feel that handling the touchpad in the same way as touch-screen
isn't always wise even though they are both multi-touch, since they
are completely different devices; the movement of the former is
relative while the latter takes always the absolute coordinate.

Anyway, I'd love to help improving things once when I'm back to work.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 16:38   ` Takashi Iwai
@ 2010-10-08 17:48     ` Takashi Iwai
  0 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-08 17:48 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Fri, 08 Oct 2010 18:38:38 +0200,
Takashi Iwai wrote:
> 
> At Fri, 08 Oct 2010 18:37:22 +0200,
> Takashi Iwai wrote:
> > 
> > At Fri,  8 Oct 2010 10:57:57 -0400,
> > Chase Douglas wrote:
> > > 
> > > Tobyn Bertram reverse engineered the multitouch protocol for Synaptics devices.
> > > I've been able to take his work and produce a series of commits to enable MT
> > > and multifinger (MF) support.
> > > 
> > > Unfortunately, there's a tricky issue with some Synaptics touchpads that have
> > > integrated buttons. For example, the left and right buttons on the touchpad of
> > > my Dell Mini 1012 consist of the lower ~20% of the touchpad surface. The
> > > touchpad physically clicks under these areas.
> > > 
> > > The X synaptics input module now has a parameter to disable touches occuring
> > > over the button area, but this solution still doesn't work perfectly. If you
> > > click a button and drag with another finger near the clicking finger, the
> > > touchpad gets confused.
> > > 
> > > Now that we have full MT support, we can try to handle this scenario better.
> > > What I've found to work best is to make touches vanish if they occur over the
> > > button area of the trackpad while any button is held. This works in conjunction
> > > with the X synaptics driver to disable single touch control over the button
> > > area. With full MT support, the touchpad doesn't seem to get confused when a
> > > click and drag occurs with two fingers close to each other, and it enables MT
> > > gestures and MF support across the entire trackpad when no buttons are held.
> > > 
> > > The first question is whether this seems appropriate to others, or if some
> > > other method would work better. Secondarily, should the solution occur in the
> > > kernel, like I have in the third patch of this series, or should it occur in
> > > the X input module? Although we don't have this information today, we may be
> > > able to query the touchpad in the future to know the area of the integrated
> > > buttons. If that were possible, would the recommended location for the hack
> > > change?
> > 
> > Great!  Finally someone found it out!
> > I found this and made a series of patches in 4 months ago.  Since
> > then, Novell legal prohibited me to send the patches to the upstream
> > due to "possible patent infringing".  Now you cracked out.  Yay.
> > 
> > FWIW, my corresponding patch is below.  It really looks similar in the
> > end ;)  I added a kconfig just to be safer.
> > 
> > Regarding the "clickpad" support: in my case, I implemented almost
> > everything about it in xorg driver.  I'm going to submit xorg
> > patches.
> 
> BTW, yet another kernel patch is missing; the support of embedded LED.
> I've posted this once, but it seems forgotten since then.  Reposted
> below.

Oh, any yet another patch, which enables multi-touch mode forcibly.
I see a similar option in your patch, so this might be useless.

But, I found that some old laptops have a little MT-support although
they have no such capability bit.  They can detect multi-fingers but
can't track the positions, it seems.  We'd need to add some whitelist
for such devices.


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: Add multi_touch parameter to psmouse driver

The multi-touch feature of Synaptics device is disabled unless this option
is set.  Setting to 2 forces the multi-touch mode no matter whether the
feature is detected or not.

Signed-off-by: Takashi Iwai <tiwai@suse.de>

---
 drivers/input/mouse/synaptics.c |  106 +++++++++++++++++++++++++++++++---------
 1 file changed, 83 insertions(+), 23 deletions(-)

--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -482,10 +482,88 @@
 }
 
 #ifdef CONFIG_MOUSE_PS2_SYNAPTICS_MULTI_TOUCH
-#define is_multi_touch(priv)	(priv)->can_multi_touch
+static int multi_touch_flag;
+
+static void setup_multi_touch(struct psmouse *psmouse, int verbose);
+
+static struct psmouse *_psmouse;
+
+static int param_set_multi_touch(const char *val, const struct kernel_param *kp)
+{
+	int mode, mode_changed;
+
+	if (!val)
+		return -EINVAL;
+	mode = simple_strtol(val, NULL, 0);
+	if (mode < 0 || mode > 2)
+		return -EINVAL;
+	mode_changed = mode != multi_touch_flag;
+	multi_touch_flag = mode;
+	if (mode_changed)
+		setup_multi_touch(_psmouse, 1);
+	return 0;
+}
+
+#define param_check_multi_touch(name, p) __param_check(name, p, int)
+
+static struct kernel_param_ops param_ops_multi_touch = {
+	.set = param_set_multi_touch,
+	.get = param_get_int,
+};
+
+module_param_named(multi_touch, multi_touch_flag, multi_touch, 0644);
+
+static inline int is_multi_touch(struct synaptics_data *priv)
+{
+	return (multi_touch_flag == 2 ||
+		(priv->can_multi_touch && multi_touch_flag));
+}
+
+static void setup_multi_touch(struct psmouse *psmouse, int verbose)
+{
+	struct input_dev *dev;
+	struct synaptics_data *priv;
+
+	_psmouse = psmouse;
+	if (!psmouse)
+		return;
+	dev = psmouse->dev;
+	priv = psmouse->private;
+	if (!dev || !priv)
+		return;
+	if (is_multi_touch(priv) &&
+	    !synaptics_init_multi_touch(psmouse)) {
+		if (verbose)
+			printk(KERN_INFO "Synaptics: enabling multi-touch\n");
+		if (!SYN_CAP_MULTIFINGER(priv->capabilities)) {
+			__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
+			__set_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
+		}
+		input_set_abs_params(dev, ABS_MT_POSITION_X,
+				     XMIN_NOMINAL, XMAX_NOMINAL, 0, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y,
+				     YMIN_NOMINAL, YMAX_NOMINAL, 0, 0);
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
+		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
+	} else {
+		if (verbose)
+			printk(KERN_INFO "Synaptics: disabling multi-touch\n");
+		if (!SYN_CAP_MULTIFINGER(priv->capabilities)) {
+			__clear_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
+			__clear_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
+		}
+		__clear_bit(ABS_MT_POSITION_X, dev->absbit);
+		__clear_bit(ABS_MT_POSITION_Y, dev->absbit);
+		__clear_bit(ABS_MT_PRESSURE, dev->absbit);
+	}
+}
+
 #else
 #define is_multi_touch(priv)	0
+#define setup_multi_touch(ps, v) do { } while (0)
 #endif
+
 /* the multi-touch packet contains w=2 (like pen) */
 #define is_multi_touch_packet(priv, hw) \
 	(is_multi_touch(priv) && (hw)->w == 2)
@@ -781,7 +859,7 @@
 	__set_bit(BTN_LEFT, dev->keybit);
 	__set_bit(BTN_RIGHT, dev->keybit);
 
-	if (SYN_CAP_MULTIFINGER(priv->capabilities) || is_multi_touch(priv)) {
+	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
 		__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
 		__set_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
 	}
@@ -810,20 +888,11 @@
 		__clear_bit(BTN_RIGHT, dev->keybit);
 		__clear_bit(BTN_MIDDLE, dev->keybit);
 	}
-
-	if (is_multi_touch(priv)) {
-		input_set_abs_params(dev, ABS_MT_POSITION_X,
-				     XMIN_NOMINAL, XMAX_NOMINAL, 0, 0);
-		input_set_abs_params(dev, ABS_MT_POSITION_Y,
-				     YMIN_NOMINAL, YMAX_NOMINAL, 0, 0);
-		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
-		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
-		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
-	}
 }
 
 static void synaptics_disconnect(struct psmouse *psmouse)
 {
+	setup_multi_touch(NULL, 0);
 	synaptics_free_led(psmouse);
 	synaptics_reset(psmouse);
 	kfree(psmouse->private);
@@ -857,8 +926,7 @@
 	}
 
 	synaptics_sync_led(psmouse);
-	if (is_multi_touch(priv))
-		synaptics_init_multi_touch(psmouse);
+	setup_multi_touch(psmouse, 0);
 
 	return 0;
 }
@@ -937,16 +1005,8 @@
 	if (synaptics_init_led(psmouse) < 0)
 		goto init_fail;
 
-	if (priv->can_multi_touch) {
-		if (synaptics_init_multi_touch(psmouse)) {
-			printk(KERN_WARNING "Synaptics: "
-			       "unable to initialize multi-touch\n");
-			priv->can_multi_touch = 0;
-		} else
-			printk(KERN_INFO "Synaptics: multi-touch enabled\n");
-	}
-
 	set_input_params(psmouse->dev, priv);
+	setup_multi_touch(psmouse, 0);
 
 	/*
 	 * Encode touchpad model so that it can be used to set

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 17:15   ` Chase Douglas
  2010-10-08 17:46     ` Takashi Iwai
@ 2010-10-08 18:04     ` Dmitry Torokhov
  2010-10-08 19:31       ` Takashi Iwai
  1 sibling, 1 reply; 43+ messages in thread
From: Dmitry Torokhov @ 2010-10-08 18:04 UTC (permalink / raw)
  To: Chase Douglas
  Cc: Takashi Iwai, linux-input, xorg-devel, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Friday, October 08, 2010 10:15:35 am Chase Douglas wrote:
> On Fri, 2010-10-08 at 18:37 +0200, Takashi Iwai wrote:
> > At Fri,  8 Oct 2010 10:57:57 -0400,
> > 
> > Chase Douglas wrote:
> > > 
> > >
> > > Tobyn Bertram reverse engineered the multitouch protocol for Synaptics
> > > devices. I've been able to take his work and produce a series of
> > > commits to enable MT and multifinger (MF) support.
> > >
> > > 
> > >
> > > Unfortunately, there's a tricky issue with some Synaptics touchpads
> > > that have integrated buttons. For example, the left and right buttons
> > > on the touchpad of my Dell Mini 1012 consist of the lower ~20% of the
> > > touchpad surface. The touchpad physically clicks under these areas.
> > >
> > > 
> > >
> > > The X synaptics input module now has a parameter to disable touches
> > > occuring over the button area, but this solution still doesn't work
> > > perfectly. If you click a button and drag with another finger near the
> > > clicking finger, the touchpad gets confused.
> > >
> > > 
> > >
> > > Now that we have full MT support, we can try to handle this scenario
> > > better. What I've found to work best is to make touches vanish if they
> > > occur over the button area of the trackpad while any button is held.
> > > This works in conjunction with the X synaptics driver to disable
> > > single touch control over the button area. With full MT support, the
> > > touchpad doesn't seem to get confused when a click and drag occurs
> > > with two fingers close to each other, and it enables MT gestures and
> > > MF support across the entire trackpad when no buttons are held.
> > >
> > > 
> > >
> > > The first question is whether this seems appropriate to others, or if
> > > some other method would work better. Secondarily, should the solution
> > > occur in the kernel, like I have in the third patch of this series, or
> > > should it occur in the X input module? Although we don't have this
> > > information today, we may be able to query the touchpad in the future
> > > to know the area of the integrated buttons. If that were possible,
> > > would the recommended location for the hack change?
> >
> > 
> >
> > Great!  Finally someone found it out!
> > I found this and made a series of patches in 4 months ago.  Since
> > then, Novell legal prohibited me to send the patches to the upstream
> > due to "possible patent infringing".  Now you cracked out.  Yay.
> >
> > 
> >
> > FWIW, my corresponding patch is below.  It really looks similar in the
> > end ;)  I added a kconfig just to be safer.
> >
> > 
> >
> > Regarding the "clickpad" support: in my case, I implemented almost
> > everything about it in xorg driver.  I'm going to submit xorg
> > patches.
> 
> So I'm confused. I was working off of source code posted to:
> 
> https://bugs.launchpad.net/utouch/+bug/633225
> 
> I was under the impression that someone else had reverse engineered the
> protocol and written patches. But the code is exactly the same as what
> you've posted here. If you're the originator of the work, and my patch
> is accepted, I think we'll need your SOB on it.

Comment #6 is quite clear on this matter:

> Takashi Iwai from OpenSuse has done quite a bit of work for the Synaptics
> Clickpad including some experimental multitouch support, his repo is here:
> http://download.opensuse.org/repositories/home:/tiwai:/clickpad:/openSUSE_
> 11.3/openSUSE_11.3/src/
> 
> I have played around with the synaptics.c code in the kernel to add
> multitouch events (ABS_MT_POSITION_X, ABS_MT_POSITION_Y, ABS_MT_PRESSURE)
> using Takashi's work as a model.

So I do believe we need to have Takashi's SOB at the very least and maybe
credit him as the author of the patches.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 18:04     ` Dmitry Torokhov
@ 2010-10-08 19:31       ` Takashi Iwai
  2010-10-10 21:04         ` Dmitry Torokhov
  0 siblings, 1 reply; 43+ messages in thread
From: Takashi Iwai @ 2010-10-08 19:31 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Chase Douglas, linux-input, xorg-devel, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Fri, 8 Oct 2010 11:04:01 -0700,
Dmitry Torokhov wrote:
> 
> On Friday, October 08, 2010 10:15:35 am Chase Douglas wrote:
> > On Fri, 2010-10-08 at 18:37 +0200, Takashi Iwai wrote:
> > > At Fri,  8 Oct 2010 10:57:57 -0400,
> > > 
> > > Chase Douglas wrote:
> > > > 
> > > >
> > > > Tobyn Bertram reverse engineered the multitouch protocol for Synaptics
> > > > devices. I've been able to take his work and produce a series of
> > > > commits to enable MT and multifinger (MF) support.
> > > >
> > > > 
> > > >
> > > > Unfortunately, there's a tricky issue with some Synaptics touchpads
> > > > that have integrated buttons. For example, the left and right buttons
> > > > on the touchpad of my Dell Mini 1012 consist of the lower ~20% of the
> > > > touchpad surface. The touchpad physically clicks under these areas.
> > > >
> > > > 
> > > >
> > > > The X synaptics input module now has a parameter to disable touches
> > > > occuring over the button area, but this solution still doesn't work
> > > > perfectly. If you click a button and drag with another finger near the
> > > > clicking finger, the touchpad gets confused.
> > > >
> > > > 
> > > >
> > > > Now that we have full MT support, we can try to handle this scenario
> > > > better. What I've found to work best is to make touches vanish if they
> > > > occur over the button area of the trackpad while any button is held.
> > > > This works in conjunction with the X synaptics driver to disable
> > > > single touch control over the button area. With full MT support, the
> > > > touchpad doesn't seem to get confused when a click and drag occurs
> > > > with two fingers close to each other, and it enables MT gestures and
> > > > MF support across the entire trackpad when no buttons are held.
> > > >
> > > > 
> > > >
> > > > The first question is whether this seems appropriate to others, or if
> > > > some other method would work better. Secondarily, should the solution
> > > > occur in the kernel, like I have in the third patch of this series, or
> > > > should it occur in the X input module? Although we don't have this
> > > > information today, we may be able to query the touchpad in the future
> > > > to know the area of the integrated buttons. If that were possible,
> > > > would the recommended location for the hack change?
> > >
> > > 
> > >
> > > Great!  Finally someone found it out!
> > > I found this and made a series of patches in 4 months ago.  Since
> > > then, Novell legal prohibited me to send the patches to the upstream
> > > due to "possible patent infringing".  Now you cracked out.  Yay.
> > >
> > > 
> > >
> > > FWIW, my corresponding patch is below.  It really looks similar in the
> > > end ;)  I added a kconfig just to be safer.
> > >
> > > 
> > >
> > > Regarding the "clickpad" support: in my case, I implemented almost
> > > everything about it in xorg driver.  I'm going to submit xorg
> > > patches.
> > 
> > So I'm confused. I was working off of source code posted to:
> > 
> > https://bugs.launchpad.net/utouch/+bug/633225
> > 
> > I was under the impression that someone else had reverse engineered the
> > protocol and written patches. But the code is exactly the same as what
> > you've posted here. If you're the originator of the work, and my patch
> > is accepted, I think we'll need your SOB on it.
> 
> Comment #6 is quite clear on this matter:
> 
> > Takashi Iwai from OpenSuse has done quite a bit of work for the Synaptics
> > Clickpad including some experimental multitouch support, his repo is here:
> > http://download.opensuse.org/repositories/home:/tiwai:/clickpad:/openSUSE_
> > 11.3/openSUSE_11.3/src/
> > 
> > I have played around with the synaptics.c code in the kernel to add
> > multitouch events (ABS_MT_POSITION_X, ABS_MT_POSITION_Y, ABS_MT_PRESSURE)
> > using Takashi's work as a model.
> 
> So I do believe we need to have Takashi's SOB at the very least and maybe
> credit him as the author of the patches.

I sent my original one, so this should suffice, right?


thanks,

Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 16:37 ` [PATCH 0/3] Input: synaptics - multitouch and multifinger support Takashi Iwai
  2010-10-08 16:38   ` Takashi Iwai
  2010-10-08 17:15   ` Chase Douglas
@ 2010-10-10  7:49   ` Henrik Rydberg
  2010-10-10 20:59     ` Dmitry Torokhov
  2 siblings, 1 reply; 43+ messages in thread
From: Henrik Rydberg @ 2010-10-10  7:49 UTC (permalink / raw)
  To: Takashi Iwai, Chase Douglas; +Cc: linux-input, Dmitry Torokhov, linux-kernel

Takashi, Chase,

talk about Heinz effect. :-)

Rather than taking any of the patches, I was wondering if we could get a single
patch including all the later findings and considerations on what devices should
have the multitouch mode. Also, unless there is a really really good reason for
it, without kernel parameters. After all, these patches should only add new
functionality without regressions.

Regarding the clickpad functionality, it is similar to the macbook pads with
integrated buttons, which has been implemented in userspace.

Regarding the tracking aspect of pointer emulation, Chase is completely right,
and this is a generic problem for several drivers. Tracking a single point by
picking the closest contact is a linear problem and could easily be performed in
interrupt context. I suggest we add it directly to the synaptics driver and then
revisit the question of adding generic tracking support to the kernel. Dmitry,
how does that sound to you?

Cheers,
Henrik

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 1/3] Input: synaptics - add multitouch support
  2010-10-08 14:57 ` [PATCH 1/3] Input: synaptics - add multitouch support Chase Douglas
  2010-10-08 14:57   ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chase Douglas
@ 2010-10-10 15:37   ` Chris Bagwell
  2010-10-10 15:41   ` Chris Bagwell
  2 siblings, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-10 15:37 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Takashi Iwai,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/08/2010 09:57 AM, Chase Douglas wrote:
> Newer Synaptics devices support multitouch. It appears there is no touch
> tracking, so the non-slotted protocol is used.
>
> Multitouch mode is disabled by default because it makes click-and-drag
> on touchpads with integrated buttons even more difficult than it already
> is. Maybe if/when the X synaptics input module works around this issue
> we can enable it by default.

I don't have access to a clickpad and I'm trying to understand its 
unique issues better.  Can you give a little more information on how X 
synaptics driver behaves differently with MT enabled compared to how it 
behaves with MT disabled?

On non-clickpad's, the X/Y will always track close to first finger 
touch.  If clickpad's continue this behaviour in non-MT mode then I'd 
assume click-and-drag will only work if you touch the drag finger before 
the click finger.  If you click first then at best I'd expect extremely 
slow movement since it tracks close but not exactly to first finger.

Does MT mode change behaviour?  Your patch #3 description sounds like 
the non-MT packet tracks moving finger always and so it constantly 
swapping its finger meaning.  Off hand, I'd think that helps 
click-and-drag issue although it creates others.

As example of what issues it creates, I'd expect xf86-input-synaptics to 
go crazy with cursor jumps when its 2 finger gestures are turned off and 
you randomly touch an extra finger to touchpad since the meaning of 
ABS_X/ABS_Y is changing without warning to it (and it doesn't understand 
MT right now).

I agree with the other comments that we want to avoid options as much as 
possible.

>
> Credit goes to Tobyn Bertram for reverse engineering the protocol.
>
> Reported-by: Tobyn Bertram
> Signed-off-by: Chase Douglas<chase.douglas@canonical.com>
> ---
>   drivers/input/mouse/synaptics.c |   78 +++++++++++++++++++++++++++++++++++----
>   drivers/input/mouse/synaptics.h |    3 +
>   2 files changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 96b70a4..990598f 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -44,6 +44,10 @@
>   #define YMIN_NOMINAL 1408
>   #define YMAX_NOMINAL 4448
>
> +static bool synaptics_multitouch;
> +module_param(synaptics_multitouch, bool, 0644);
> +MODULE_PARM_DESC(synaptics_multitouch,
> +		 "Enable multitouch mode on Synaptics devices");
>
>   /*****************************************************************************
>    *	Stuff we need even when we do not want native Synaptics support
> @@ -279,6 +283,22 @@ static void synaptics_set_rate(struct psmouse *psmouse, unsigned int rate)
>   	synaptics_mode_cmd(psmouse, priv->mode);
>   }
>
> +static void synaptics_set_multitouch_mode(struct psmouse *psmouse)
> +{
> +	static unsigned char param = 0xc8;
> +	struct synaptics_data *priv = psmouse->private;
> +
> +	if (!SYN_CAP_MULTITOUCH(priv->ext_cap_0c) || !synaptics_multitouch)
> +		return;
> +	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))
> +		return;
> +	if (ps2_command(&psmouse->ps2dev,&param, PSMOUSE_CMD_SETRATE))
> +		return;
> +
> +	priv->multitouch = 1;
> +	printk(KERN_INFO "Synaptics: Multitouch mode enabled\n");
> +}
> +
>   /*****************************************************************************
>    *	Synaptics pass-through PS/2 port support
>    ****************************************************************************/
> @@ -362,18 +382,30 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
>   	memset(hw, 0, sizeof(struct synaptics_hw_state));
>
>   	if (SYN_MODEL_NEWABS(priv->model_id)) {
> -		hw->x = (((buf[3]&  0x10)<<  8) |
> -			 ((buf[1]&  0x0f)<<  8) |
> -			 buf[4]);
> -		hw->y = (((buf[3]&  0x20)<<  7) |
> -			 ((buf[1]&  0xf0)<<  4) |
> -			 buf[5]);
> -
> -		hw->z = buf[2];
>   		hw->w = (((buf[0]&  0x30)>>  2) |
>   			 ((buf[0]&  0x04)>>  1) |
>   			 ((buf[3]&  0x04)>>  2));
>
> +		if (SYN_MULTITOUCH(priv, hw)) {
> +			/* Multitouch data is half normal resolution */
> +			hw->x = (((buf[4]&  0x0f)<<  8) |
> +				 buf[1])<<  1;
> +			hw->y = (((buf[4]&  0xf0)<<  4) |
> +				 buf[2])<<  1;
> +
> +			hw->z = ((buf[3]&  0x30) |
> +				 (buf[5]&  0x0f))<<  1;
> +		} else {
> +			hw->x = (((buf[3]&  0x10)<<  8) |
> +				 ((buf[1]&  0x0f)<<  8) |
> +				 buf[4]);
> +			hw->y = (((buf[3]&  0x20)<<  7) |
> +				 ((buf[1]&  0xf0)<<  4) |
> +				 buf[5]);
> +
> +			hw->z = buf[2];
> +		}
> +
>   		hw->left  = (buf[0]&  0x01) ? 1 : 0;
>   		hw->right = (buf[0]&  0x02) ? 1 : 0;
>
> @@ -445,6 +477,18 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>
>   	synaptics_parse_hw_state(psmouse->packet, priv,&hw);
>
> +	if (SYN_MULTITOUCH(priv,&hw)) {
> +		if (hw.z>  0) {
> +			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> +			input_report_abs(dev, ABS_MT_POSITION_Y,
> +					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
> +		}
> +
> +		input_mt_sync(dev);
> +		return;
> +	}
> +
>   	if (hw.scroll) {
>   		priv->scroll += hw.scroll;
>
> @@ -499,6 +543,12 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	if (hw.z>  0) {
>   		input_report_abs(dev, ABS_X, hw.x);
>   		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +		if (priv->multitouch) {
> +			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> +			input_report_abs(dev, ABS_MT_POSITION_Y,
> +					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
> +		}
>   	}
>   	input_report_abs(dev, ABS_PRESSURE, hw.z);
>
> @@ -525,6 +575,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	for (i = 0; i<  SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
>   		input_report_key(dev, BTN_0 + i, hw.ext_buttons&  (1<<  i));
>
> +	if (priv->multitouch)
> +		input_mt_sync(dev);

This mt_sync would seem more nature to be sent after ABS_MT_PRESSURE to 
match MT packet processing.

Chris

>   	input_sync(dev);
>   }
>
> @@ -605,6 +657,14 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>   			     YMIN_NOMINAL, priv->y_max ?: YMAX_NOMINAL, 0, 0);
>   	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>
> +	if (priv->multitouch) {
> +		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
> +				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
> +		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
> +				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
> +		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +	}
> +
>   	if (SYN_CAP_PALMDETECT(priv->capabilities))
>   		input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
>
> @@ -745,6 +805,8 @@ int synaptics_init(struct psmouse *psmouse)
>   		goto init_fail;
>   	}
>
> +	synaptics_set_multitouch_mode(psmouse);
> +
>   	priv->pkt_type = SYN_MODEL_NEWABS(priv->model_id) ? SYN_NEWABS : SYN_OLDABS;
>
>   	printk(KERN_INFO "Synaptics Touchpad, model: %ld, fw: %ld.%ld, id: %#lx, caps: %#lx/%#lx/%#lx\n",
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index b6aa7d2..5126c9c 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -53,6 +53,7 @@
>   #define SYN_CAP_PRODUCT_ID(ec)		(((ec)&  0xff0000)>>  16)
>   #define SYN_CAP_CLICKPAD(ex0c)		((ex0c)&  0x100100)
>   #define SYN_CAP_MAX_DIMENSIONS(ex0c)	((ex0c)&  0x020000)
> +#define SYN_CAP_MULTITOUCH(ex0c)	((ex0c)&  0x080000)
>
>   /* synaptics modes query bits */
>   #define SYN_MODE_ABSOLUTE(m)		((m)&  (1<<  7))
> @@ -78,6 +79,7 @@
>   #define SYN_NEWABS_STRICT		1
>   #define SYN_NEWABS_RELAXED		2
>   #define SYN_OLDABS			3
> +#define SYN_MULTITOUCH(priv, hw)	((priv)->multitouch&&  (hw)->w == 2)
>
>   /*
>    * A structure to describe the state of the touchpad hardware (buttons and pad)
> @@ -110,6 +112,7 @@ struct synaptics_data {
>   	unsigned char pkt_type;			/* packet type - old, new, etc */
>   	unsigned char mode;			/* current mode byte */
>   	int scroll;
> +	int multitouch;				/* Whether device provides MT */
>   };
>
>   void synaptics_module_init(void);


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 1/3] Input: synaptics - add multitouch support
  2010-10-08 14:57 ` [PATCH 1/3] Input: synaptics - add multitouch support Chase Douglas
  2010-10-08 14:57   ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chase Douglas
  2010-10-10 15:37   ` [PATCH 1/3] Input: synaptics - add multitouch support Chris Bagwell
@ 2010-10-10 15:41   ` Chris Bagwell
  2 siblings, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-10 15:41 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Takashi Iwai,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/08/2010 09:57 AM, Chase Douglas wrote:
> Newer Synaptics devices support multitouch. It appears there is no touch
> tracking, so the non-slotted protocol is used.
>
> Multitouch mode is disabled by default because it makes click-and-drag
> on touchpads with integrated buttons even more difficult than it already
> is. Maybe if/when the X synaptics input module works around this issue
> we can enable it by default.

I don't have access to a clickpad and I'm trying to understand its 
unique issues better.  Can you give a little more information on how X 
synaptics driver behaves differently with MT enabled compared to how it 
behaves with MT disabled?

On non-clickpad's, the X/Y will always track close to first finger 
touch.  If clickpad's continue this behaviour in non-MT mode then I'd 
assume click-and-drag will only work if you touch the drag finger before 
the click finger.  If you click first then at best I'd expect extremely 
slow movement since it tracks close but not exactly to first finger.

Does MT mode change behaviour?  Your patch #3 description sounds like 
the non-MT packet tracks moving finger always and so it constantly 
swapping its finger meaning.  Off hand, I'd think that helps 
click-and-drag issue although it creates others.

As example of what issues it creates, I'd expect xf86-input-synaptics to 
go crazy with cursor jumps when its 2 finger gestures are turned off and 
you randomly touch an extra finger to touchpad since the meaning of 
ABS_X/ABS_Y is changing without warning to it (and it doesn't understand 
MT right now).

I agree with the other comments that we want to avoid options as much as 
possible.

>
> Credit goes to Tobyn Bertram for reverse engineering the protocol.
>
> Reported-by: Tobyn Bertram
> Signed-off-by: Chase Douglas<chase.douglas@canonical.com>
> ---
>   drivers/input/mouse/synaptics.c |   78 +++++++++++++++++++++++++++++++++++----
>   drivers/input/mouse/synaptics.h |    3 +
>   2 files changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 96b70a4..990598f 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -44,6 +44,10 @@
>   #define YMIN_NOMINAL 1408
>   #define YMAX_NOMINAL 4448
>
> +static bool synaptics_multitouch;
> +module_param(synaptics_multitouch, bool, 0644);
> +MODULE_PARM_DESC(synaptics_multitouch,
> +		 "Enable multitouch mode on Synaptics devices");
>
>   /*****************************************************************************
>    *	Stuff we need even when we do not want native Synaptics support
> @@ -279,6 +283,22 @@ static void synaptics_set_rate(struct psmouse *psmouse, unsigned int rate)
>   	synaptics_mode_cmd(psmouse, priv->mode);
>   }
>
> +static void synaptics_set_multitouch_mode(struct psmouse *psmouse)
> +{
> +	static unsigned char param = 0xc8;
> +	struct synaptics_data *priv = psmouse->private;
> +
> +	if (!SYN_CAP_MULTITOUCH(priv->ext_cap_0c) || !synaptics_multitouch)
> +		return;
> +	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))
> +		return;
> +	if (ps2_command(&psmouse->ps2dev,&param, PSMOUSE_CMD_SETRATE))
> +		return;
> +
> +	priv->multitouch = 1;
> +	printk(KERN_INFO "Synaptics: Multitouch mode enabled\n");
> +}
> +
>   /*****************************************************************************
>    *	Synaptics pass-through PS/2 port support
>    ****************************************************************************/
> @@ -362,18 +382,30 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
>   	memset(hw, 0, sizeof(struct synaptics_hw_state));
>
>   	if (SYN_MODEL_NEWABS(priv->model_id)) {
> -		hw->x = (((buf[3]&  0x10)<<  8) |
> -			 ((buf[1]&  0x0f)<<  8) |
> -			 buf[4]);
> -		hw->y = (((buf[3]&  0x20)<<  7) |
> -			 ((buf[1]&  0xf0)<<  4) |
> -			 buf[5]);
> -
> -		hw->z = buf[2];
>   		hw->w = (((buf[0]&  0x30)>>  2) |
>   			 ((buf[0]&  0x04)>>  1) |
>   			 ((buf[3]&  0x04)>>  2));
>
> +		if (SYN_MULTITOUCH(priv, hw)) {
> +			/* Multitouch data is half normal resolution */
> +			hw->x = (((buf[4]&  0x0f)<<  8) |
> +				 buf[1])<<  1;
> +			hw->y = (((buf[4]&  0xf0)<<  4) |
> +				 buf[2])<<  1;
> +
> +			hw->z = ((buf[3]&  0x30) |
> +				 (buf[5]&  0x0f))<<  1;
> +		} else {
> +			hw->x = (((buf[3]&  0x10)<<  8) |
> +				 ((buf[1]&  0x0f)<<  8) |
> +				 buf[4]);
> +			hw->y = (((buf[3]&  0x20)<<  7) |
> +				 ((buf[1]&  0xf0)<<  4) |
> +				 buf[5]);
> +
> +			hw->z = buf[2];
> +		}
> +
>   		hw->left  = (buf[0]&  0x01) ? 1 : 0;
>   		hw->right = (buf[0]&  0x02) ? 1 : 0;
>
> @@ -445,6 +477,18 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>
>   	synaptics_parse_hw_state(psmouse->packet, priv,&hw);
>
> +	if (SYN_MULTITOUCH(priv,&hw)) {
> +		if (hw.z>  0) {
> +			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> +			input_report_abs(dev, ABS_MT_POSITION_Y,
> +					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
> +		}
> +
> +		input_mt_sync(dev);
> +		return;
> +	}
> +
>   	if (hw.scroll) {
>   		priv->scroll += hw.scroll;
>
> @@ -499,6 +543,12 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	if (hw.z>  0) {
>   		input_report_abs(dev, ABS_X, hw.x);
>   		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +		if (priv->multitouch) {
> +			input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> +			input_report_abs(dev, ABS_MT_POSITION_Y,
> +					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
> +		}
>   	}
>   	input_report_abs(dev, ABS_PRESSURE, hw.z);
>
> @@ -525,6 +575,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	for (i = 0; i<  SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
>   		input_report_key(dev, BTN_0 + i, hw.ext_buttons&  (1<<  i));
>
> +	if (priv->multitouch)
> +		input_mt_sync(dev);

This mt_sync would seem more nature to be sent after ABS_MT_PRESSURE to 
match MT packet processing.

Chris

>   	input_sync(dev);
>   }
>
> @@ -605,6 +657,14 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>   			     YMIN_NOMINAL, priv->y_max ?: YMAX_NOMINAL, 0, 0);
>   	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>
> +	if (priv->multitouch) {
> +		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
> +				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
> +		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
> +				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
> +		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +	}
> +
>   	if (SYN_CAP_PALMDETECT(priv->capabilities))
>   		input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
>
> @@ -745,6 +805,8 @@ int synaptics_init(struct psmouse *psmouse)
>   		goto init_fail;
>   	}
>
> +	synaptics_set_multitouch_mode(psmouse);
> +
>   	priv->pkt_type = SYN_MODEL_NEWABS(priv->model_id) ? SYN_NEWABS : SYN_OLDABS;
>
>   	printk(KERN_INFO "Synaptics Touchpad, model: %ld, fw: %ld.%ld, id: %#lx, caps: %#lx/%#lx/%#lx\n",
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index b6aa7d2..5126c9c 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -53,6 +53,7 @@
>   #define SYN_CAP_PRODUCT_ID(ec)		(((ec)&  0xff0000)>>  16)
>   #define SYN_CAP_CLICKPAD(ex0c)		((ex0c)&  0x100100)
>   #define SYN_CAP_MAX_DIMENSIONS(ex0c)	((ex0c)&  0x020000)
> +#define SYN_CAP_MULTITOUCH(ex0c)	((ex0c)&  0x080000)
>
>   /* synaptics modes query bits */
>   #define SYN_MODE_ABSOLUTE(m)		((m)&  (1<<  7))
> @@ -78,6 +79,7 @@
>   #define SYN_NEWABS_STRICT		1
>   #define SYN_NEWABS_RELAXED		2
>   #define SYN_OLDABS			3
> +#define SYN_MULTITOUCH(priv, hw)	((priv)->multitouch&&  (hw)->w == 2)
>
>   /*
>    * A structure to describe the state of the touchpad hardware (buttons and pad)
> @@ -110,6 +112,7 @@ struct synaptics_data {
>   	unsigned char pkt_type;			/* packet type - old, new, etc */
>   	unsigned char mode;			/* current mode byte */
>   	int scroll;
> +	int multitouch;				/* Whether device provides MT */
>   };
>
>   void synaptics_module_init(void);


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 2/3] Input: synaptics - add multitouch multifinger support
  2010-10-08 14:57   ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chase Douglas
  2010-10-08 14:58     ` [PATCH 3/3] Input: synaptics - remove touches over button click area Chase Douglas
@ 2010-10-10 15:44     ` Chris Bagwell
  1 sibling, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-10 15:44 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Takashi Iwai,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/08/2010 09:57 AM, Chase Douglas wrote:
> Newer multitouch Synaptics trackpads do not advertise multifinger
> support. Now that we have multitouch support, we can use the number of
> touches to report multifinger functionality.
>
> In conjunction with the X synaptics input module, this enables
> functionality such as two finger scrolling.
>
> Signed-off-by: Chase Douglas<chase.douglas@canonical.com>
> ---
>   drivers/input/mouse/synaptics.c |   24 +++++++++++++-----------
>   drivers/input/mouse/synaptics.h |    1 +
>   2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 990598f..7289d88 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -471,7 +471,6 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	struct input_dev *dev = psmouse->dev;
>   	struct synaptics_data *priv = psmouse->private;
>   	struct synaptics_hw_state hw;
> -	int num_fingers;
>   	int finger_width;
>   	int i;
>
> @@ -483,6 +482,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   			input_report_abs(dev, ABS_MT_POSITION_Y,
>   					 YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
>   			input_report_abs(dev, ABS_MT_PRESSURE, hw.z);
> +			priv->num_fingers++;
>   		}
>
>   		input_mt_sync(dev);
> @@ -510,13 +510,13 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	}
>
>   	if (hw.z>  0) {
> -		num_fingers = 1;
> +		priv->num_fingers++;

In this area of code, its not as obvious your relying on MT packets to 
always come before standard packets.  I think its worth a comment here 
or below on why your initialising priv->num_fingers at bottom of 
processing instead of at top of processing.

It will also help explain to reader why mt_sync events work out as expected.

>   		finger_width = 5;
>   		if (SYN_CAP_EXTENDED(priv->capabilities)) {
>   			switch (hw.w) {
>   			case 0 ... 1:
>   				if (SYN_CAP_MULTIFINGER(priv->capabilities))
> -					num_fingers = hw.w + 2;
> +					priv->num_fingers = hw.w + 2;
>   				break;
>   			case 2:
>   				if (SYN_MODEL_PEN(priv->model_id))
> @@ -528,10 +528,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   				break;
>   			}
>   		}
> -	} else {
> -		num_fingers = 0;
> +	} else
>   		finger_width = 0;
> -	}
>
>   	/* Post events
>   	 * BTN_TOUCH has to be first as mousedev relies on it when doing
> @@ -555,15 +553,19 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>   	if (SYN_CAP_PALMDETECT(priv->capabilities))
>   		input_report_abs(dev, ABS_TOOL_WIDTH, finger_width);
>
> -	input_report_key(dev, BTN_TOOL_FINGER, num_fingers == 1);
> +	input_report_key(dev, BTN_TOOL_FINGER, priv->num_fingers == 1);
>   	input_report_key(dev, BTN_LEFT, hw.left);
>   	input_report_key(dev, BTN_RIGHT, hw.right);
>
> -	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
> -		input_report_key(dev, BTN_TOOL_DOUBLETAP, num_fingers == 2);
> -		input_report_key(dev, BTN_TOOL_TRIPLETAP, num_fingers == 3);
> +	if (SYN_CAP_MULTIFINGER(priv->capabilities) || priv->multitouch) {
> +		input_report_key(dev, BTN_TOOL_DOUBLETAP,
> +				 priv->num_fingers == 2);
> +		input_report_key(dev, BTN_TOOL_TRIPLETAP,
> +				 priv->num_fingers == 3);
>   	}
>
> +	priv->num_fingers = 0;
> +
>   	if (SYN_CAP_MIDDLE_BUTTON(priv->capabilities))
>   		input_report_key(dev, BTN_MIDDLE, hw.middle);
>
> @@ -674,7 +676,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>   	__set_bit(BTN_LEFT, dev->keybit);
>   	__set_bit(BTN_RIGHT, dev->keybit);
>
> -	if (SYN_CAP_MULTIFINGER(priv->capabilities)) {
> +	if (SYN_CAP_MULTIFINGER(priv->capabilities) || priv->multitouch) {
>   		__set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
>   		__set_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
>   	}
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 5126c9c..0989b8d 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -113,6 +113,7 @@ struct synaptics_data {
>   	unsigned char mode;			/* current mode byte */
>   	int scroll;
>   	int multitouch;				/* Whether device provides MT */
> +	unsigned int num_fingers;		/* Number of fingers touching */
>   };
>
>   void synaptics_module_init(void);


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-08 14:58     ` [PATCH 3/3] Input: synaptics - remove touches over button click area Chase Douglas
@ 2010-10-10 15:58       ` Chris Bagwell
  2010-10-11 16:24         ` Chris Bagwell
  1 sibling, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-10 15:58 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Takashi Iwai,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/08/2010 09:58 AM, Chase Douglas wrote:
> Now that we have proper multitouch support, we can handle integrated
> buttons better. If we know the top of the buttons on the touchpad, we
> can ignore any touches that occur within the touchpad area while a
> button is clicked. It may be possible to get the button area by querying
> the device, but for now allow the user to manually set it.
>
> A note on why this works: the Synaptics touchpads have pseudo touch
> tracking. When two touches are on the touchpad, an MT touch packet with
> just the X, Y, and pressure values is sent before a normal Synaptics
> touch packet. When one touch is obviously in motion and the other is
> stationary, the touchpad controller sends the touch in motion in the
> normal packet and the stationary touch in the MT packet. Single touch
> emulation is provided by the normal packet, so an action like clicking
> a button and dragging with another finger still works as expected.
>
> Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> synaptics_button_thresh=4100.
>

Even if we did not submit the MT logic, I'd go a totally different 
direction and move clickpad button press support fully to 
xf86-input-synaptics and I'd remove the logic from kernel side that maps 
HW's middle button to left button.  It seems just limping a long with 
single button support anyways.

I haven't had time to review Takashi's xf86-input-synaptics patches just 
sent yet but seems along this line of thinking as well.

Chris

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-10  7:49   ` Henrik Rydberg
@ 2010-10-10 20:59     ` Dmitry Torokhov
  2010-10-11  7:28       ` Takashi Iwai
  0 siblings, 1 reply; 43+ messages in thread
From: Dmitry Torokhov @ 2010-10-10 20:59 UTC (permalink / raw)
  To: Henrik Rydberg; +Cc: Takashi Iwai, Chase Douglas, linux-input, linux-kernel

On Sun, Oct 10, 2010 at 09:49:45AM +0200, Henrik Rydberg wrote:
> Takashi, Chase,
> 
> talk about Heinz effect. :-)
> 
> Rather than taking any of the patches, I was wondering if we could get a single
> patch including all the later findings and considerations on what devices should
> have the multitouch mode. Also, unless there is a really really good reason for
> it, without kernel parameters. After all, these patches should only add new
> functionality without regressions.

Yes, I think we should limit number of module options (and even more so
Kconfig options). I believe that either MT support is ready for prime-time
and then it should be enabled unconditionally, or it is not ready and
should be kept as patches. his also means that we should not introduce
regressions in ST (even when emulated in MT mode).

> 
> Regarding the clickpad functionality, it is similar to the macbook pads with
> integrated buttons, which has been implemented in userspace.

Right now we map the "click" to BTN_LEFT leaving it to the synaptics X
driver to work our the "click zone" support.

> 
> Regarding the tracking aspect of pointer emulation, Chase is completely right,
> and this is a generic problem for several drivers. Tracking a single point by
> picking the closest contact is a linear problem and could easily be performed in
> interrupt context. I suggest we add it directly to the synaptics driver and then
> revisit the question of adding generic tracking support to the kernel. Dmitry,
> how does that sound to you?

Sounds good to me. Hopefully it could be made it a library[ish] function
that several MT drivers could use to produce ST-compliant events.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-08 19:31       ` Takashi Iwai
@ 2010-10-10 21:04         ` Dmitry Torokhov
  2010-10-11  7:35           ` Takashi Iwai
  0 siblings, 1 reply; 43+ messages in thread
From: Dmitry Torokhov @ 2010-10-10 21:04 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chase Douglas, linux-input, xorg-devel, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Fri, Oct 08, 2010 at 09:31:30PM +0200, Takashi Iwai wrote:
> At Fri, 8 Oct 2010 11:04:01 -0700,
> Dmitry Torokhov wrote:
> > 
> > So I do believe we need to have Takashi's SOB at the very least and maybe
> > credit him as the author of the patches.
> 
> I sent my original one, so this should suffice, right?
> 

Takashi,

Generally I think people should not add or remove any SOB lines except
for their own as the work on the patches, so original patches should
retain their original SOBs... In absence of that I'd like to have
explicit confirmations that I can add SOBs statements; otherwise it just
dilutes the value of SOBs significantly.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-10 20:59     ` Dmitry Torokhov
@ 2010-10-11  7:28       ` Takashi Iwai
  2010-10-11  7:40         ` Henrik Rydberg
  0 siblings, 1 reply; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11  7:28 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Henrik Rydberg, Chase Douglas, linux-input, linux-kernel

At Sun, 10 Oct 2010 13:59:32 -0700,
Dmitry Torokhov wrote:
> 
> On Sun, Oct 10, 2010 at 09:49:45AM +0200, Henrik Rydberg wrote:
> > Takashi, Chase,
> > 
> > talk about Heinz effect. :-)
> > 
> > Rather than taking any of the patches, I was wondering if we could get a single
> > patch including all the later findings and considerations on what devices should
> > have the multitouch mode. Also, unless there is a really really good reason for
> > it, without kernel parameters. After all, these patches should only add new
> > functionality without regressions.
> 
> Yes, I think we should limit number of module options (and even more so
> Kconfig options). I believe that either MT support is ready for prime-time
> and then it should be enabled unconditionally, or it is not ready and
> should be kept as patches. his also means that we should not introduce
> regressions in ST (even when emulated in MT mode).

Right, but it's pretty hard to know whether it works really for all
machines, simply because the current MT-detection is nothing but a
pure guess work.  As mentioned in another mail, there are devices that
have no this bit but reports multi-finger detections without
tracking.  So, there are definitely other things to be checked.

Thus, I'm inclined to add a module option, at least, for *disabling*
MT for non-working machines.  Also, another question is whether it's
safe to enable MT even for user-space that don't understand MT.


Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-10 21:04         ` Dmitry Torokhov
@ 2010-10-11  7:35           ` Takashi Iwai
  2010-10-11  7:48             ` Henrik Rydberg
  0 siblings, 1 reply; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11  7:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Chase Douglas, linux-input, xorg-devel, Chris Bagwell,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Sun, 10 Oct 2010 14:04:34 -0700,
Dmitry Torokhov wrote:
> 
> On Fri, Oct 08, 2010 at 09:31:30PM +0200, Takashi Iwai wrote:
> > At Fri, 8 Oct 2010 11:04:01 -0700,
> > Dmitry Torokhov wrote:
> > > 
> > > So I do believe we need to have Takashi's SOB at the very least and maybe
> > > credit him as the author of the patches.
> > 
> > I sent my original one, so this should suffice, right?
> > 
> 
> Takashi,
> 
> Generally I think people should not add or remove any SOB lines except
> for their own as the work on the patches, so original patches should
> retain their original SOBs... In absence of that I'd like to have
> explicit confirmations that I can add SOBs statements; otherwise it just
> dilutes the value of SOBs significantly.

Yeah, in general, sign-offs should be retained.  In this particular case,
it's also my (well more exactly my employer's) fault, that blocked the
submission of the original patch.

In anyway, feel free to add my sign-off there since I already posted
my own one as a reference.

But, I have an open issue with Chase's patch.  Maybe it's rather a
question to Henrik:

Shouldn't all points be reported as ABS_MT_* events?  So far, only the
second touch-point is reported via ABS_MT_* while the first  point is
still reported as ABX_[X|Y].

I corrected this in my patch I posted, but I wasn't sure, too.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11  7:28       ` Takashi Iwai
@ 2010-10-11  7:40         ` Henrik Rydberg
  0 siblings, 0 replies; 43+ messages in thread
From: Henrik Rydberg @ 2010-10-11  7:40 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Dmitry Torokhov, Chase Douglas, linux-input, linux-kernel

On 10/11/2010 09:28 AM, Takashi Iwai wrote:
[...]

> Right, but it's pretty hard to know whether it works really for all
> machines, simply because the current MT-detection is nothing but a
> pure guess work.  As mentioned in another mail, there are devices that
> have no this bit but reports multi-finger detections without
> tracking.  So, there are definitely other things to be checked.
> 
> Thus, I'm inclined to add a module option, at least, for *disabling*
> MT for non-working machines.  Also, another question is whether it's
> safe to enable MT even for user-space that don't understand MT.


Yes, you can add MT support on top of the ST protocol of an existing driver.

Henrik

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11  7:35           ` Takashi Iwai
@ 2010-10-11  7:48             ` Henrik Rydberg
  2010-10-11  7:59               ` Takashi Iwai
  2010-10-11 13:41                 ` Chris Bagwell
  0 siblings, 2 replies; 43+ messages in thread
From: Henrik Rydberg @ 2010-10-11  7:48 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Dmitry Torokhov, Chase Douglas, linux-input, xorg-devel,
	Chris Bagwell, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/11/2010 09:35 AM, Takashi Iwai wrote:
[...]

> In anyway, feel free to add my sign-off there since I already posted
> my own one as a reference.
> 
> But, I have an open issue with Chase's patch.  Maybe it's rather a
> question to Henrik:
> 
> Shouldn't all points be reported as ABS_MT_* events?  So far, only the
> second touch-point is reported via ABS_MT_* while the first  point is
> still reported as ABX_[X|Y].
> 
> I corrected this in my patch I posted, but I wasn't sure, too.


I have issues with all submitted patches, but did not give explicit reasons
since there were overlapping submissions. Perhaps Chase and yourself can work
out how you want to submit the new patches? And yes, all points should be
reported as ABS_MT events.

Thanks,
Henrik

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11  7:48             ` Henrik Rydberg
@ 2010-10-11  7:59               ` Takashi Iwai
  2010-10-11 13:41                 ` Chris Bagwell
  1 sibling, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11  7:59 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Dmitry Torokhov, Chase Douglas, linux-input, xorg-devel,
	Chris Bagwell, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 09:48:36 +0200,
Henrik Rydberg wrote:
> 
> On 10/11/2010 09:35 AM, Takashi Iwai wrote:
> [...]
> 
> > In anyway, feel free to add my sign-off there since I already posted
> > my own one as a reference.
> > 
> > But, I have an open issue with Chase's patch.  Maybe it's rather a
> > question to Henrik:
> > 
> > Shouldn't all points be reported as ABS_MT_* events?  So far, only the
> > second touch-point is reported via ABS_MT_* while the first  point is
> > still reported as ABX_[X|Y].
> > 
> > I corrected this in my patch I posted, but I wasn't sure, too.
> 
> 
> I have issues with all submitted patches, but did not give explicit reasons
> since there were overlapping submissions. Perhaps Chase and yourself can work
> out how you want to submit the new patches?

Yeah, we definitely work on patches and sort out issues.

> And yes, all points should be reported as ABS_MT events.

OK, thanks for clarification!


Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11  7:48             ` Henrik Rydberg
@ 2010-10-11 13:41                 ` Chris Bagwell
  2010-10-11 13:41                 ` Chris Bagwell
  1 sibling, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-11 13:41 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Takashi Iwai, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Mon, Oct 11, 2010 at 2:48 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On 10/11/2010 09:35 AM, Takashi Iwai wrote:
> [...]
>
>> In anyway, feel free to add my sign-off there since I already posted
>> my own one as a reference.
>>
>> But, I have an open issue with Chase's patch.  Maybe it's rather a
>> question to Henrik:
>>
>> Shouldn't all points be reported as ABS_MT_* events?  So far, only the
>> second touch-point is reported via ABS_MT_* while the first  point is
>> still reported as ABX_[X|Y].
>>
>> I corrected this in my patch I posted, but I wasn't sure, too.
>
>
> I have issues with all submitted patches, but did not give explicit reasons
> since there were overlapping submissions. Perhaps Chase and yourself can work
> out how you want to submit the new patches? And yes, all points should be
> reported as ABS_MT events.
>
> Thanks,
> Henrik
>

And is it also safe to say that we need to continue to report
ABS_X/ABS_Y *and* those values need to always track 1st finger touch
for backwards compatibility?

It was brought up in thread but not stated as strong requirement.

BTW, there are patches in last couple months to x86-input-synaptics
that will allow it to ignore jumps in values of ABS_X/ABS_Y when
transition of multi-touch occur (both adding or removing fingers via
BTN_TOOL_*TAP).  So one new-ish option is for ABS_X/ABS_Y to not track
1st finger but become average of 2 fingers.

Chris

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
@ 2010-10-11 13:41                 ` Chris Bagwell
  0 siblings, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-11 13:41 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Takashi Iwai, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Mon, Oct 11, 2010 at 2:48 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On 10/11/2010 09:35 AM, Takashi Iwai wrote:
> [...]
>
>> In anyway, feel free to add my sign-off there since I already posted
>> my own one as a reference.
>>
>> But, I have an open issue with Chase's patch.  Maybe it's rather a
>> question to Henrik:
>>
>> Shouldn't all points be reported as ABS_MT_* events?  So far, only the
>> second touch-point is reported via ABS_MT_* while the first  point is
>> still reported as ABX_[X|Y].
>>
>> I corrected this in my patch I posted, but I wasn't sure, too.
>
>
> I have issues with all submitted patches, but did not give explicit reasons
> since there were overlapping submissions. Perhaps Chase and yourself can work
> out how you want to submit the new patches? And yes, all points should be
> reported as ABS_MT events.
>
> Thanks,
> Henrik
>

And is it also safe to say that we need to continue to report
ABS_X/ABS_Y *and* those values need to always track 1st finger touch
for backwards compatibility?

It was brought up in thread but not stated as strong requirement.

BTW, there are patches in last couple months to x86-input-synaptics
that will allow it to ignore jumps in values of ABS_X/ABS_Y when
transition of multi-touch occur (both adding or removing fingers via
BTN_TOOL_*TAP).  So one new-ish option is for ABS_X/ABS_Y to not track
1st finger but become average of 2 fingers.

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11 13:41                 ` Chris Bagwell
@ 2010-10-11 14:01                   ` Takashi Iwai
  -1 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 14:01 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Henrik Rydberg, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 08:41:44 -0500,
Chris Bagwell wrote:
> 
> On Mon, Oct 11, 2010 at 2:48 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> > On 10/11/2010 09:35 AM, Takashi Iwai wrote:
> > [...]
> >
> >> In anyway, feel free to add my sign-off there since I already posted
> >> my own one as a reference.
> >>
> >> But, I have an open issue with Chase's patch.  Maybe it's rather a
> >> question to Henrik:
> >>
> >> Shouldn't all points be reported as ABS_MT_* events?  So far, only the
> >> second touch-point is reported via ABS_MT_* while the first  point is
> >> still reported as ABX_[X|Y].
> >>
> >> I corrected this in my patch I posted, but I wasn't sure, too.
> >
> >
> > I have issues with all submitted patches, but did not give explicit reasons
> > since there were overlapping submissions. Perhaps Chase and yourself can work
> > out how you want to submit the new patches? And yes, all points should be
> > reported as ABS_MT events.
> >
> > Thanks,
> > Henrik
> >
> 
> And is it also safe to say that we need to continue to report
> ABS_X/ABS_Y *and* those values need to always track 1st finger touch
> for backwards compatibility?

Indeed this was an implicit question of my previous inquiry.
I suppose mtdev tracks only ABS_MT_*?

> It was brought up in thread but not stated as strong requirement.
> 
> BTW, there are patches in last couple months to x86-input-synaptics
> that will allow it to ignore jumps in values of ABS_X/ABS_Y when
> transition of multi-touch occur (both adding or removing fingers via
> BTN_TOOL_*TAP).  So one new-ish option is for ABS_X/ABS_Y to not track
> 1st finger but become average of 2 fingers.

The tracking of multi-touch is inevitably needed for clickpad devices.

But, I'm reluctant for merging the clickpad support code into mtdev.
Its behavior is too messy, and it's only for synaptics touchpads, not
for touch-screens.  For mtdev maintenance POV, I guess, it'd be
cleaner to keep this away from it.

As an example of mess of Clickpad: if you keep your finger on a button
area and another finger on the normal area, you shouldn't trigger the
multi-touch mode, no matter whether it's clicked or not.  People tend
to keep the finger on the button before actually dragging.

But, if you put both fingers in the button area and sliding together,
it should be handled as two-finger scrolling.  Also, if you move one
finger on a button area, it should be tracked as a normal pointer
movement.

Another mess is that, as the default setup, the pointer movement is
too sensitive, and when user pushes down the touchpad for clicking,
the pointer moves a few or more pixels.  This eventually misses the
target.  So, some trick to drag the pointer is necessary for click
action.  One of my patches does it by introducing some "move
threshold" value.


Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
@ 2010-10-11 14:01                   ` Takashi Iwai
  0 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 14:01 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Henrik Rydberg, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 08:41:44 -0500,
Chris Bagwell wrote:
> 
> On Mon, Oct 11, 2010 at 2:48 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> > On 10/11/2010 09:35 AM, Takashi Iwai wrote:
> > [...]
> >
> >> In anyway, feel free to add my sign-off there since I already posted
> >> my own one as a reference.
> >>
> >> But, I have an open issue with Chase's patch.  Maybe it's rather a
> >> question to Henrik:
> >>
> >> Shouldn't all points be reported as ABS_MT_* events?  So far, only the
> >> second touch-point is reported via ABS_MT_* while the first  point is
> >> still reported as ABX_[X|Y].
> >>
> >> I corrected this in my patch I posted, but I wasn't sure, too.
> >
> >
> > I have issues with all submitted patches, but did not give explicit reasons
> > since there were overlapping submissions. Perhaps Chase and yourself can work
> > out how you want to submit the new patches? And yes, all points should be
> > reported as ABS_MT events.
> >
> > Thanks,
> > Henrik
> >
> 
> And is it also safe to say that we need to continue to report
> ABS_X/ABS_Y *and* those values need to always track 1st finger touch
> for backwards compatibility?

Indeed this was an implicit question of my previous inquiry.
I suppose mtdev tracks only ABS_MT_*?

> It was brought up in thread but not stated as strong requirement.
> 
> BTW, there are patches in last couple months to x86-input-synaptics
> that will allow it to ignore jumps in values of ABS_X/ABS_Y when
> transition of multi-touch occur (both adding or removing fingers via
> BTN_TOOL_*TAP).  So one new-ish option is for ABS_X/ABS_Y to not track
> 1st finger but become average of 2 fingers.

The tracking of multi-touch is inevitably needed for clickpad devices.

But, I'm reluctant for merging the clickpad support code into mtdev.
Its behavior is too messy, and it's only for synaptics touchpads, not
for touch-screens.  For mtdev maintenance POV, I guess, it'd be
cleaner to keep this away from it.

As an example of mess of Clickpad: if you keep your finger on a button
area and another finger on the normal area, you shouldn't trigger the
multi-touch mode, no matter whether it's clicked or not.  People tend
to keep the finger on the button before actually dragging.

But, if you put both fingers in the button area and sliding together,
it should be handled as two-finger scrolling.  Also, if you move one
finger on a button area, it should be tracked as a normal pointer
movement.

Another mess is that, as the default setup, the pointer movement is
too sensitive, and when user pushes down the touchpad for clicking,
the pointer moves a few or more pixels.  This eventually misses the
target.  So, some trick to drag the pointer is necessary for click
action.  One of my patches does it by introducing some "move
threshold" value.


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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11 14:01                   ` Takashi Iwai
  (?)
@ 2010-10-11 14:24                   ` Henrik Rydberg
  2010-10-11 14:49                     ` Takashi Iwai
  -1 siblings, 1 reply; 43+ messages in thread
From: Henrik Rydberg @ 2010-10-11 14:24 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chris Bagwell, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/11/2010 04:01 PM, Takashi Iwai wrote:
[...]

> As an example of mess of Clickpad: if you keep your finger on a button
> area and another finger on the normal area, you shouldn't trigger the
> multi-touch mode, no matter whether it's clicked or not.  People tend
> to keep the finger on the button before actually dragging.
> 
> But, if you put both fingers in the button area and sliding together,
> it should be handled as two-finger scrolling.  Also, if you move one
> finger on a button area, it should be tracked as a normal pointer
> movement.


Are you aware of http://bitmath.org/code/multitouch/? The driver was developed
to address very similar issues on the macbook trackpads.

> Another mess is that, as the default setup, the pointer movement is
> too sensitive, and when user pushes down the touchpad for clicking,
> the pointer moves a few or more pixels.  This eventually misses the
> target.  So, some trick to drag the pointer is necessary for click
> action.  One of my patches does it by introducing some "move
> threshold" value.


Also addressed in that project. What I am hoping to see, eventually, is code
similar to the core of the multitouch project in something like ginn. This would
exercise the whole utouch stack in an extensible way, and finally make
multitouch trackpads work the way they are supposed to.

Henrik

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11 14:24                   ` Henrik Rydberg
@ 2010-10-11 14:49                     ` Takashi Iwai
  2010-10-11 15:31                       ` Henrik Rydberg
  0 siblings, 1 reply; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 14:49 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Chris Bagwell, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 16:24:41 +0200,
Henrik Rydberg wrote:
> 
> On 10/11/2010 04:01 PM, Takashi Iwai wrote:
> [...]
> 
> > As an example of mess of Clickpad: if you keep your finger on a button
> > area and another finger on the normal area, you shouldn't trigger the
> > multi-touch mode, no matter whether it's clicked or not.  People tend
> > to keep the finger on the button before actually dragging.
> > 
> > But, if you put both fingers in the button area and sliding together,
> > it should be handled as two-finger scrolling.  Also, if you move one
> > finger on a button area, it should be tracked as a normal pointer
> > movement.
> 
> 
> Are you aware of http://bitmath.org/code/multitouch/? The driver was developed
> to address very similar issues on the macbook trackpads.

Well, I thought Macs don't use the certain areas as virtual buttons
but determines the button by the number of fingers.  Or was it
changed?  If yes, could you point out which source file does it?

I'm looking through the git tree
http://bitmath.org/git/multitouch.git, but couldn't find out yet...

> > Another mess is that, as the default setup, the pointer movement is
> > too sensitive, and when user pushes down the touchpad for clicking,
> > the pointer moves a few or more pixels.  This eventually misses the
> > target.  So, some trick to drag the pointer is necessary for click
> > action.  One of my patches does it by introducing some "move
> > threshold" value.
> 
> 
> Also addressed in that project. What I am hoping to see, eventually, is code
> similar to the core of the multitouch project in something like ginn. This would
> exercise the whole utouch stack in an extensible way, and finally make
> multitouch trackpads work the way they are supposed to.

If the messy behavior of clickpad suits with the multi-touch library,
I have nothing against it, sure.  It was just my concern that the
clickpad-style handling might pollute the code.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11 14:49                     ` Takashi Iwai
@ 2010-10-11 15:31                       ` Henrik Rydberg
  2010-10-11 15:58                         ` Takashi Iwai
  0 siblings, 1 reply; 43+ messages in thread
From: Henrik Rydberg @ 2010-10-11 15:31 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chris Bagwell, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/11/2010 04:49 PM, Takashi Iwai wrote:
[...]
>>> As an example of mess of Clickpad: if you keep your finger on a button

>>> area and another finger on the normal area, you shouldn't trigger the
>>> multi-touch mode, no matter whether it's clicked or not.  People tend
>>> to keep the finger on the button before actually dragging.
>>>
>>> But, if you put both fingers in the button area and sliding together,
>>> it should be handled as two-finger scrolling.  Also, if you move one
>>> finger on a button area, it should be tracked as a normal pointer
>>> movement.
>>
>>
>> Are you aware of http://bitmath.org/code/multitouch/? The driver was developed
>> to address very similar issues on the macbook trackpads.
> 
> Well, I thought Macs don't use the certain areas as virtual buttons
> but determines the button by the number of fingers.  Or was it
> changed?  If yes, could you point out which source file does it?


The trackpads with integrated button are like clickpads with one button. It
would certainly be interesting to use the clickpad pattern on those as well. For
both types of pads, the bottom area is special. Two-finger scroll works as
normal, while a clicking finger may move around without affecting anything, nor
the number of fingers on the pad.

> I'm looking through the git tree
> http://bitmath.org/git/multitouch.git, but couldn't find out yet...


src/memory.c might be what you are looking for. There are also two unpublished
branches around - one which uses the mtdev library, and one which uses
utouch-grail directly (but with limited functionality).

>>> Another mess is that, as the default setup, the pointer movement is
>>> too sensitive, and when user pushes down the touchpad for clicking,
>>> the pointer moves a few or more pixels.  This eventually misses the
>>> target.  So, some trick to drag the pointer is necessary for click
>>> action.  One of my patches does it by introducing some "move
>>> threshold" value.
>>
>>
>> Also addressed in that project. What I am hoping to see, eventually, is code
>> similar to the core of the multitouch project in something like ginn. This would
>> exercise the whole utouch stack in an extensible way, and finally make
>> multitouch trackpads work the way they are supposed to.
> 
> If the messy behavior of clickpad suits with the multi-touch library,
> I have nothing against it, sure.  It was just my concern that the
> clickpad-style handling might pollute the code.


I am not sure it has to be messy at all, and it has to reside *somewhere*. I
think something like ginn, or parallel to ginn, is the best long-run place for
it. Submitting clickpad functionality to multitouch for starters would
definitely work in my book.

Cheers,
Henrik

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 0/3] Input: synaptics - multitouch and multifinger support
  2010-10-11 15:31                       ` Henrik Rydberg
@ 2010-10-11 15:58                         ` Takashi Iwai
  0 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 15:58 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Chris Bagwell, Dmitry Torokhov, Chase Douglas, linux-input,
	xorg-devel, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 17:31:22 +0200,
Henrik Rydberg wrote:
> 
> On 10/11/2010 04:49 PM, Takashi Iwai wrote:
> [...]
> >>> As an example of mess of Clickpad: if you keep your finger on a button
> 
> >>> area and another finger on the normal area, you shouldn't trigger the
> >>> multi-touch mode, no matter whether it's clicked or not.  People tend
> >>> to keep the finger on the button before actually dragging.
> >>>
> >>> But, if you put both fingers in the button area and sliding together,
> >>> it should be handled as two-finger scrolling.  Also, if you move one
> >>> finger on a button area, it should be tracked as a normal pointer
> >>> movement.
> >>
> >>
> >> Are you aware of http://bitmath.org/code/multitouch/? The driver was developed
> >> to address very similar issues on the macbook trackpads.
> > 
> > Well, I thought Macs don't use the certain areas as virtual buttons
> > but determines the button by the number of fingers.  Or was it
> > changed?  If yes, could you point out which source file does it?
> 
> 
> The trackpads with integrated button are like clickpads with one button. It
> would certainly be interesting to use the clickpad pattern on those as well. For
> both types of pads, the bottom area is special. Two-finger scroll works as
> normal, while a clicking finger may move around without affecting anything, nor
> the number of fingers on the pad.
> 
> > I'm looking through the git tree
> > http://bitmath.org/git/multitouch.git, but couldn't find out yet...
> 
> 
> src/memory.c might be what you are looking for. There are also two unpublished
> branches around - one which uses the mtdev library, and one which uses
> utouch-grail directly (but with limited functionality).

Thanks, I found what it does now.
Indeed, the clickpad behavior would be to add left/right areas (and
the middle area if wanted) instead of a single button area.

> >>> Another mess is that, as the default setup, the pointer movement is
> >>> too sensitive, and when user pushes down the touchpad for clicking,
> >>> the pointer moves a few or more pixels.  This eventually misses the
> >>> target.  So, some trick to drag the pointer is necessary for click
> >>> action.  One of my patches does it by introducing some "move
> >>> threshold" value.
> >>
> >>
> >> Also addressed in that project. What I am hoping to see, eventually, is code
> >> similar to the core of the multitouch project in something like ginn. This would
> >> exercise the whole utouch stack in an extensible way, and finally make
> >> multitouch trackpads work the way they are supposed to.
> > 
> > If the messy behavior of clickpad suits with the multi-touch library,
> > I have nothing against it, sure.  It was just my concern that the
> > clickpad-style handling might pollute the code.
> 
> 
> I am not sure it has to be messy at all, and it has to reside *somewhere*. I
> think something like ginn, or parallel to ginn, is the best long-run place for
> it. Submitting clickpad functionality to multitouch for starters would
> definitely work in my book.

OK, that sounds good.

What I was concerned is that the choice of sensitivity and
acceleration functions for pointer movement aren't easy for such
devices.  In the button area, especially insensitive movement is
required because of pressing.  The default function of synaptics
driver was too sensitive, and thus a special handling was needed.


Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-08 14:58     ` [PATCH 3/3] Input: synaptics - remove touches over button click area Chase Douglas
@ 2010-10-11 16:24         ` Chris Bagwell
  2010-10-11 16:24         ` Chris Bagwell
  1 sibling, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-11 16:24 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Takashi Iwai,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> Now that we have proper multitouch support, we can handle integrated
> buttons better. If we know the top of the buttons on the touchpad, we
> can ignore any touches that occur within the touchpad area while a
> button is clicked. It may be possible to get the button area by querying
> the device, but for now allow the user to manually set it.
>
> A note on why this works: the Synaptics touchpads have pseudo touch
> tracking. When two touches are on the touchpad, an MT touch packet with
> just the X, Y, and pressure values is sent before a normal Synaptics
> touch packet. When one touch is obviously in motion and the other is
> stationary, the touchpad controller sends the touch in motion in the
> normal packet and the stationary touch in the MT packet. Single touch
> emulation is provided by the normal packet, so an action like clicking
> a button and dragging with another finger still works as expected.
>
> Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> synaptics_button_thresh=4100.
>
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 7289d88..e67778d 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
>  MODULE_PARM_DESC(synaptics_multitouch,
>                 "Enable multitouch mode on Synaptics devices");
>
> +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> +module_param(synaptics_button_thresh, int, 0644);
> +MODULE_PARM_DESC(synaptics_button_thres,
> +                "Y coordinate threshold of integrated buttons on Synaptics "
> +                "devices");
> +
>  /*****************************************************************************
>  *     Stuff we need even when we do not want native Synaptics support
>  ****************************************************************************/
> @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
>        }
>  }
>
> +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> +                               synaptics_button_thresh))
> +
>  /*
>  *  called for each full received packet from the touchpad
>  */
> @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
>
>        if (SYN_MULTITOUCH(priv, &hw)) {
> -               if (hw.z > 0) {
> +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
>                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
>                        input_report_abs(dev, ABS_MT_POSITION_Y,
>                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>                return;
>        }
>
> +       /* If touch occurs over depressed button, ignore it */
> +       if (TOUCH_OVER_BUTTON(hw))
> +               hw.z = 0;
> +
>        if (hw.z > 0) {
>                priv->num_fingers++;
>                finger_width = 5;
> --
> 1.7.1
>
>

I'm convinced now that clickpad style touchpads can't work without
multi-touch and something like logic in xf86-input-multitouch.  So now
I'd like to just consider how the MT-enabled touchpad interface can
best work with non-multitouch aware applications since thats what
users will need to deal with on fresh installs for a while.  I believe
the above approach of setting hw.z to zero would cause havoc on
non-multi-touch aware applications.

I see three main choices:

1) Do not report any button presses when in click area and report
ABS_X/ABS_Y based on first finger touch always.  Something like
xf86-input-synaptics RBCornerButton feature would be responsible for
button presses and can support left/middle/right concepts easily.

The downside is a mis-configured box will not be able to use GUI since
no button presses will work.  Also, there is no clear way to
auto-enable RBCornerButton-like features in user land in the same way
is being done in some patches that consider single button touchpads as
clickpads.

2.1) Send BTN_LEFT when in click area and ABS_X/ABS_Y tracks 1st
finger during 1 touch and 2nd finger during multi-touch.
xf86-input-synaptics needs change to detect left/middle/right based on
ABS_X/ABS_Y values right at report of BTN_LEFT for clickpads.
Touching drag finger before click finger breaks click-and-drag.

2.2) Send BTN_LEFT when in click area and ABS_X/ABS_Y tracks 1st
finger during 1 touch and middle point of 2 fingers during
multi-touch.  Touching drag finger before click finger breaks
click-and-drag and left/middle/right detection.

2.3) Send BTN_LEFT when in click area and ABS_X/ABS_Y indicates first
finger always. I'm sure this is behavior of synaptics touchpad before
we enable MT-packets.  Touching drag finger first will break
left/middle/right detection.  Touching click finger first breaks
click-and-drag.

3) A version of finger tracking in #2.1 could be done by detecting
which touch of two touches is in click area (as patch does) and
preferring non-click area data when it exists.  This has same issues
as #2.1 and has down side that we need to query hw or hard code click
areas.

Nothing ideal.  I'd probably pick #2.1 (or #3 if we can auto-detect
click area) since it at least allows consistent detecting of
left/middle/right.  I believe #2.1 is approach that bcm 5974 takes but
not sure from only quick review.

Chris

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
@ 2010-10-11 16:24         ` Chris Bagwell
  0 siblings, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-11 16:24 UTC (permalink / raw)
  To: Chase Douglas
  Cc: linux-input, xorg-devel, Dmitry Torokhov, Takashi Iwai,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> Now that we have proper multitouch support, we can handle integrated
> buttons better. If we know the top of the buttons on the touchpad, we
> can ignore any touches that occur within the touchpad area while a
> button is clicked. It may be possible to get the button area by querying
> the device, but for now allow the user to manually set it.
>
> A note on why this works: the Synaptics touchpads have pseudo touch
> tracking. When two touches are on the touchpad, an MT touch packet with
> just the X, Y, and pressure values is sent before a normal Synaptics
> touch packet. When one touch is obviously in motion and the other is
> stationary, the touchpad controller sends the touch in motion in the
> normal packet and the stationary touch in the MT packet. Single touch
> emulation is provided by the normal packet, so an action like clicking
> a button and dragging with another finger still works as expected.
>
> Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> synaptics_button_thresh=4100.
>
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 7289d88..e67778d 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
>  MODULE_PARM_DESC(synaptics_multitouch,
>                 "Enable multitouch mode on Synaptics devices");
>
> +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> +module_param(synaptics_button_thresh, int, 0644);
> +MODULE_PARM_DESC(synaptics_button_thres,
> +                "Y coordinate threshold of integrated buttons on Synaptics "
> +                "devices");
> +
>  /*****************************************************************************
>  *     Stuff we need even when we do not want native Synaptics support
>  ****************************************************************************/
> @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
>        }
>  }
>
> +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> +                               synaptics_button_thresh))
> +
>  /*
>  *  called for each full received packet from the touchpad
>  */
> @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
>
>        if (SYN_MULTITOUCH(priv, &hw)) {
> -               if (hw.z > 0) {
> +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
>                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
>                        input_report_abs(dev, ABS_MT_POSITION_Y,
>                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>                return;
>        }
>
> +       /* If touch occurs over depressed button, ignore it */
> +       if (TOUCH_OVER_BUTTON(hw))
> +               hw.z = 0;
> +
>        if (hw.z > 0) {
>                priv->num_fingers++;
>                finger_width = 5;
> --
> 1.7.1
>
>

I'm convinced now that clickpad style touchpads can't work without
multi-touch and something like logic in xf86-input-multitouch.  So now
I'd like to just consider how the MT-enabled touchpad interface can
best work with non-multitouch aware applications since thats what
users will need to deal with on fresh installs for a while.  I believe
the above approach of setting hw.z to zero would cause havoc on
non-multi-touch aware applications.

I see three main choices:

1) Do not report any button presses when in click area and report
ABS_X/ABS_Y based on first finger touch always.  Something like
xf86-input-synaptics RBCornerButton feature would be responsible for
button presses and can support left/middle/right concepts easily.

The downside is a mis-configured box will not be able to use GUI since
no button presses will work.  Also, there is no clear way to
auto-enable RBCornerButton-like features in user land in the same way
is being done in some patches that consider single button touchpads as
clickpads.

2.1) Send BTN_LEFT when in click area and ABS_X/ABS_Y tracks 1st
finger during 1 touch and 2nd finger during multi-touch.
xf86-input-synaptics needs change to detect left/middle/right based on
ABS_X/ABS_Y values right at report of BTN_LEFT for clickpads.
Touching drag finger before click finger breaks click-and-drag.

2.2) Send BTN_LEFT when in click area and ABS_X/ABS_Y tracks 1st
finger during 1 touch and middle point of 2 fingers during
multi-touch.  Touching drag finger before click finger breaks
click-and-drag and left/middle/right detection.

2.3) Send BTN_LEFT when in click area and ABS_X/ABS_Y indicates first
finger always. I'm sure this is behavior of synaptics touchpad before
we enable MT-packets.  Touching drag finger first will break
left/middle/right detection.  Touching click finger first breaks
click-and-drag.

3) A version of finger tracking in #2.1 could be done by detecting
which touch of two touches is in click area (as patch does) and
preferring non-click area data when it exists.  This has same issues
as #2.1 and has down side that we need to query hw or hard code click
areas.

Nothing ideal.  I'd probably pick #2.1 (or #3 if we can auto-detect
click area) since it at least allows consistent detecting of
left/middle/right.  I believe #2.1 is approach that bcm 5974 takes but
not sure from only quick review.

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-11 16:24         ` Chris Bagwell
@ 2010-10-11 17:10           ` Takashi Iwai
  -1 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 17:10 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Chase Douglas, linux-input, xorg-devel, Dmitry Torokhov,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 11:24:04 -0500,
Chris Bagwell wrote:
> 
> On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
> > Now that we have proper multitouch support, we can handle integrated
> > buttons better. If we know the top of the buttons on the touchpad, we
> > can ignore any touches that occur within the touchpad area while a
> > button is clicked. It may be possible to get the button area by querying
> > the device, but for now allow the user to manually set it.
> >
> > A note on why this works: the Synaptics touchpads have pseudo touch
> > tracking. When two touches are on the touchpad, an MT touch packet with
> > just the X, Y, and pressure values is sent before a normal Synaptics
> > touch packet. When one touch is obviously in motion and the other is
> > stationary, the touchpad controller sends the touch in motion in the
> > normal packet and the stationary touch in the MT packet. Single touch
> > emulation is provided by the normal packet, so an action like clicking
> > a button and dragging with another finger still works as expected.
> >
> > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> > synaptics_button_thresh=4100.
> >
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > ---
> >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> >  1 files changed, 15 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index 7289d88..e67778d 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> >  MODULE_PARM_DESC(synaptics_multitouch,
> >                 "Enable multitouch mode on Synaptics devices");
> >
> > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> > +module_param(synaptics_button_thresh, int, 0644);
> > +MODULE_PARM_DESC(synaptics_button_thres,
> > +                "Y coordinate threshold of integrated buttons on Synaptics "
> > +                "devices");
> > +
> >  /*****************************************************************************
> >  *     Stuff we need even when we do not want native Synaptics support
> >  ****************************************************************************/
> > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> >        }
> >  }
> >
> > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> > +                               synaptics_button_thresh))
> > +
> >  /*
> >  *  called for each full received packet from the touchpad
> >  */
> > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> >
> >        if (SYN_MULTITOUCH(priv, &hw)) {
> > -               if (hw.z > 0) {
> > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >                return;
> >        }
> >
> > +       /* If touch occurs over depressed button, ignore it */
> > +       if (TOUCH_OVER_BUTTON(hw))
> > +               hw.z = 0;
> > +
> >        if (hw.z > 0) {
> >                priv->num_fingers++;
> >                finger_width = 5;
> > --
> > 1.7.1
> >
> >
> 
> I'm convinced now that clickpad style touchpads can't work without
> multi-touch and something like logic in xf86-input-multitouch.

Actually Clickpad works without multi-touch patch.  With my patches to
synaptics, it worked in some level.  There are many restrictions (e.g.
pushing the button first then drag), though.


Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
@ 2010-10-11 17:10           ` Takashi Iwai
  0 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 17:10 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Chase Douglas, linux-input, xorg-devel, Dmitry Torokhov,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 11:24:04 -0500,
Chris Bagwell wrote:
> 
> On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
> > Now that we have proper multitouch support, we can handle integrated
> > buttons better. If we know the top of the buttons on the touchpad, we
> > can ignore any touches that occur within the touchpad area while a
> > button is clicked. It may be possible to get the button area by querying
> > the device, but for now allow the user to manually set it.
> >
> > A note on why this works: the Synaptics touchpads have pseudo touch
> > tracking. When two touches are on the touchpad, an MT touch packet with
> > just the X, Y, and pressure values is sent before a normal Synaptics
> > touch packet. When one touch is obviously in motion and the other is
> > stationary, the touchpad controller sends the touch in motion in the
> > normal packet and the stationary touch in the MT packet. Single touch
> > emulation is provided by the normal packet, so an action like clicking
> > a button and dragging with another finger still works as expected.
> >
> > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> > synaptics_button_thresh=4100.
> >
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > ---
> >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> >  1 files changed, 15 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index 7289d88..e67778d 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> >  MODULE_PARM_DESC(synaptics_multitouch,
> >                 "Enable multitouch mode on Synaptics devices");
> >
> > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> > +module_param(synaptics_button_thresh, int, 0644);
> > +MODULE_PARM_DESC(synaptics_button_thres,
> > +                "Y coordinate threshold of integrated buttons on Synaptics "
> > +                "devices");
> > +
> >  /*****************************************************************************
> >  *     Stuff we need even when we do not want native Synaptics support
> >  ****************************************************************************/
> > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> >        }
> >  }
> >
> > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> > +                               synaptics_button_thresh))
> > +
> >  /*
> >  *  called for each full received packet from the touchpad
> >  */
> > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> >
> >        if (SYN_MULTITOUCH(priv, &hw)) {
> > -               if (hw.z > 0) {
> > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >                return;
> >        }
> >
> > +       /* If touch occurs over depressed button, ignore it */
> > +       if (TOUCH_OVER_BUTTON(hw))
> > +               hw.z = 0;
> > +
> >        if (hw.z > 0) {
> >                priv->num_fingers++;
> >                finger_width = 5;
> > --
> > 1.7.1
> >
> >
> 
> I'm convinced now that clickpad style touchpads can't work without
> multi-touch and something like logic in xf86-input-multitouch.

Actually Clickpad works without multi-touch patch.  With my patches to
synaptics, it worked in some level.  There are many restrictions (e.g.
pushing the button first then drag), though.


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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-11 17:10           ` Takashi Iwai
@ 2010-10-11 17:30             ` Dmitry Torokhov
  -1 siblings, 0 replies; 43+ messages in thread
From: Dmitry Torokhov @ 2010-10-11 17:30 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chris Bagwell, Chase Douglas, linux-input, xorg-devel,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Mon, Oct 11, 2010 at 07:10:33PM +0200, Takashi Iwai wrote:
> At Mon, 11 Oct 2010 11:24:04 -0500,
> Chris Bagwell wrote:
> > 
> > On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> > <chase.douglas@canonical.com> wrote:
> > > Now that we have proper multitouch support, we can handle integrated
> > > buttons better. If we know the top of the buttons on the touchpad, we
> > > can ignore any touches that occur within the touchpad area while a
> > > button is clicked. It may be possible to get the button area by querying
> > > the device, but for now allow the user to manually set it.
> > >
> > > A note on why this works: the Synaptics touchpads have pseudo touch
> > > tracking. When two touches are on the touchpad, an MT touch packet with
> > > just the X, Y, and pressure values is sent before a normal Synaptics
> > > touch packet. When one touch is obviously in motion and the other is
> > > stationary, the touchpad controller sends the touch in motion in the
> > > normal packet and the stationary touch in the MT packet. Single touch
> > > emulation is provided by the normal packet, so an action like clicking
> > > a button and dragging with another finger still works as expected.
> > >
> > > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> > > synaptics_button_thresh=4100.
> > >
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > ---
> > >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> > >  1 files changed, 15 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > > index 7289d88..e67778d 100644
> > > --- a/drivers/input/mouse/synaptics.c
> > > +++ b/drivers/input/mouse/synaptics.c
> > > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> > >  MODULE_PARM_DESC(synaptics_multitouch,
> > >                 "Enable multitouch mode on Synaptics devices");
> > >
> > > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> > > +module_param(synaptics_button_thresh, int, 0644);
> > > +MODULE_PARM_DESC(synaptics_button_thres,
> > > +                "Y coordinate threshold of integrated buttons on Synaptics "
> > > +                "devices");
> > > +
> > >  /*****************************************************************************
> > >  *     Stuff we need even when we do not want native Synaptics support
> > >  ****************************************************************************/
> > > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> > >        }
> > >  }
> > >
> > > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> > > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> > > +                               synaptics_button_thresh))
> > > +
> > >  /*
> > >  *  called for each full received packet from the touchpad
> > >  */
> > > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> > >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> > >
> > >        if (SYN_MULTITOUCH(priv, &hw)) {
> > > -               if (hw.z > 0) {
> > > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> > >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> > >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> > >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> > >                return;
> > >        }
> > >
> > > +       /* If touch occurs over depressed button, ignore it */
> > > +       if (TOUCH_OVER_BUTTON(hw))
> > > +               hw.z = 0;
> > > +
> > >        if (hw.z > 0) {
> > >                priv->num_fingers++;
> > >                finger_width = 5;
> > > --
> > > 1.7.1
> > >
> > >
> > 
> > I'm convinced now that clickpad style touchpads can't work without
> > multi-touch and something like logic in xf86-input-multitouch.
> 
> Actually Clickpad works without multi-touch patch.  With my patches to
> synaptics, it worked in some level.  There are many restrictions (e.g.
> pushing the button first then drag), though.
> 

I am OK with devices not working perfectly with default/existing
drivers, but we should allow enough functionality (movement, primary
- left - click and so on) so that user can go through install and/or
upgrade of  userpsace component.

This also mean that we need to have userspace component available before
changing the behavior that will cause adverse effect for older setups.
Of course if we could avoid degrading older setups that would be even
better.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
@ 2010-10-11 17:30             ` Dmitry Torokhov
  0 siblings, 0 replies; 43+ messages in thread
From: Dmitry Torokhov @ 2010-10-11 17:30 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chris Bagwell, Chase Douglas, linux-input, xorg-devel,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Mon, Oct 11, 2010 at 07:10:33PM +0200, Takashi Iwai wrote:
> At Mon, 11 Oct 2010 11:24:04 -0500,
> Chris Bagwell wrote:
> > 
> > On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> > <chase.douglas@canonical.com> wrote:
> > > Now that we have proper multitouch support, we can handle integrated
> > > buttons better. If we know the top of the buttons on the touchpad, we
> > > can ignore any touches that occur within the touchpad area while a
> > > button is clicked. It may be possible to get the button area by querying
> > > the device, but for now allow the user to manually set it.
> > >
> > > A note on why this works: the Synaptics touchpads have pseudo touch
> > > tracking. When two touches are on the touchpad, an MT touch packet with
> > > just the X, Y, and pressure values is sent before a normal Synaptics
> > > touch packet. When one touch is obviously in motion and the other is
> > > stationary, the touchpad controller sends the touch in motion in the
> > > normal packet and the stationary touch in the MT packet. Single touch
> > > emulation is provided by the normal packet, so an action like clicking
> > > a button and dragging with another finger still works as expected.
> > >
> > > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> > > synaptics_button_thresh=4100.
> > >
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > ---
> > >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> > >  1 files changed, 15 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > > index 7289d88..e67778d 100644
> > > --- a/drivers/input/mouse/synaptics.c
> > > +++ b/drivers/input/mouse/synaptics.c
> > > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> > >  MODULE_PARM_DESC(synaptics_multitouch,
> > >                 "Enable multitouch mode on Synaptics devices");
> > >
> > > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> > > +module_param(synaptics_button_thresh, int, 0644);
> > > +MODULE_PARM_DESC(synaptics_button_thres,
> > > +                "Y coordinate threshold of integrated buttons on Synaptics "
> > > +                "devices");
> > > +
> > >  /*****************************************************************************
> > >  *     Stuff we need even when we do not want native Synaptics support
> > >  ****************************************************************************/
> > > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> > >        }
> > >  }
> > >
> > > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> > > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> > > +                               synaptics_button_thresh))
> > > +
> > >  /*
> > >  *  called for each full received packet from the touchpad
> > >  */
> > > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> > >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> > >
> > >        if (SYN_MULTITOUCH(priv, &hw)) {
> > > -               if (hw.z > 0) {
> > > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> > >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> > >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> > >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> > >                return;
> > >        }
> > >
> > > +       /* If touch occurs over depressed button, ignore it */
> > > +       if (TOUCH_OVER_BUTTON(hw))
> > > +               hw.z = 0;
> > > +
> > >        if (hw.z > 0) {
> > >                priv->num_fingers++;
> > >                finger_width = 5;
> > > --
> > > 1.7.1
> > >
> > >
> > 
> > I'm convinced now that clickpad style touchpads can't work without
> > multi-touch and something like logic in xf86-input-multitouch.
> 
> Actually Clickpad works without multi-touch patch.  With my patches to
> synaptics, it worked in some level.  There are many restrictions (e.g.
> pushing the button first then drag), though.
> 

I am OK with devices not working perfectly with default/existing
drivers, but we should allow enough functionality (movement, primary
- left - click and so on) so that user can go through install and/or
upgrade of  userpsace component.

This also mean that we need to have userspace component available before
changing the behavior that will cause adverse effect for older setups.
Of course if we could avoid degrading older setups that would be even
better.

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-11 17:30             ` Dmitry Torokhov
  (?)
@ 2010-10-11 17:40             ` Takashi Iwai
  -1 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 17:40 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Chris Bagwell, Chase Douglas, linux-input, xorg-devel,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 10:30:16 -0700,
Dmitry Torokhov wrote:
> 
> On Mon, Oct 11, 2010 at 07:10:33PM +0200, Takashi Iwai wrote:
> > At Mon, 11 Oct 2010 11:24:04 -0500,
> > Chris Bagwell wrote:
> > > 
> > > On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> > > <chase.douglas@canonical.com> wrote:
> > > > Now that we have proper multitouch support, we can handle integrated
> > > > buttons better. If we know the top of the buttons on the touchpad, we
> > > > can ignore any touches that occur within the touchpad area while a
> > > > button is clicked. It may be possible to get the button area by querying
> > > > the device, but for now allow the user to manually set it.
> > > >
> > > > A note on why this works: the Synaptics touchpads have pseudo touch
> > > > tracking. When two touches are on the touchpad, an MT touch packet with
> > > > just the X, Y, and pressure values is sent before a normal Synaptics
> > > > touch packet. When one touch is obviously in motion and the other is
> > > > stationary, the touchpad controller sends the touch in motion in the
> > > > normal packet and the stationary touch in the MT packet. Single touch
> > > > emulation is provided by the normal packet, so an action like clicking
> > > > a button and dragging with another finger still works as expected.
> > > >
> > > > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> > > > synaptics_button_thresh=4100.
> > > >
> > > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > > > ---
> > > >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> > > >  1 files changed, 15 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > > > index 7289d88..e67778d 100644
> > > > --- a/drivers/input/mouse/synaptics.c
> > > > +++ b/drivers/input/mouse/synaptics.c
> > > > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> > > >  MODULE_PARM_DESC(synaptics_multitouch,
> > > >                 "Enable multitouch mode on Synaptics devices");
> > > >
> > > > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> > > > +module_param(synaptics_button_thresh, int, 0644);
> > > > +MODULE_PARM_DESC(synaptics_button_thres,
> > > > +                "Y coordinate threshold of integrated buttons on Synaptics "
> > > > +                "devices");
> > > > +
> > > >  /*****************************************************************************
> > > >  *     Stuff we need even when we do not want native Synaptics support
> > > >  ****************************************************************************/
> > > > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> > > >        }
> > > >  }
> > > >
> > > > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> > > > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> > > > +                               synaptics_button_thresh))
> > > > +
> > > >  /*
> > > >  *  called for each full received packet from the touchpad
> > > >  */
> > > > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> > > >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> > > >
> > > >        if (SYN_MULTITOUCH(priv, &hw)) {
> > > > -               if (hw.z > 0) {
> > > > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> > > >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> > > >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> > > >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > > > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> > > >                return;
> > > >        }
> > > >
> > > > +       /* If touch occurs over depressed button, ignore it */
> > > > +       if (TOUCH_OVER_BUTTON(hw))
> > > > +               hw.z = 0;
> > > > +
> > > >        if (hw.z > 0) {
> > > >                priv->num_fingers++;
> > > >                finger_width = 5;
> > > > --
> > > > 1.7.1
> > > >
> > > >
> > > 
> > > I'm convinced now that clickpad style touchpads can't work without
> > > multi-touch and something like logic in xf86-input-multitouch.
> > 
> > Actually Clickpad works without multi-touch patch.  With my patches to
> > synaptics, it worked in some level.  There are many restrictions (e.g.
> > pushing the button first then drag), though.
> > 
> 
> I am OK with devices not working perfectly with default/existing
> drivers, but we should allow enough functionality (movement, primary
> - left - click and so on) so that user can go through install and/or
> upgrade of  userpsace component.
> 
> This also mean that we need to have userspace component available before
> changing the behavior that will cause adverse effect for older setups.
> Of course if we could avoid degrading older setups that would be even
> better.

The xorg synaptics patches work with 2.6.34 or later kernel that contains
SYN_CAP_CLICKPAD() in kernel driver.

The question from now on is about the multi-touch patch, I'd say.
One concern is the compatibility.  The multi-touch is definitely
benefit for non-clickpad devices.  I guess integrating mtdev into xorg
synaptics would be a good way to go, while picking up the minimal
support for MT (like my patches) might be a quicker at this moment.

In anyway, I see no merit for implementing clickpad support in the
kernel driver for the time being, especially since the multi-touch can
be better supported in the user-space.


Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-11 17:10           ` Takashi Iwai
@ 2010-10-11 17:46             ` Chris Bagwell
  -1 siblings, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-11 17:46 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chase Douglas, linux-input, xorg-devel, Dmitry Torokhov,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Mon, Oct 11, 2010 at 12:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 11 Oct 2010 11:24:04 -0500,
> Chris Bagwell wrote:
>>
>> On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>> > Now that we have proper multitouch support, we can handle integrated
>> > buttons better. If we know the top of the buttons on the touchpad, we
>> > can ignore any touches that occur within the touchpad area while a
>> > button is clicked. It may be possible to get the button area by querying
>> > the device, but for now allow the user to manually set it.
>> >
>> > A note on why this works: the Synaptics touchpads have pseudo touch
>> > tracking. When two touches are on the touchpad, an MT touch packet with
>> > just the X, Y, and pressure values is sent before a normal Synaptics
>> > touch packet. When one touch is obviously in motion and the other is
>> > stationary, the touchpad controller sends the touch in motion in the
>> > normal packet and the stationary touch in the MT packet. Single touch
>> > emulation is provided by the normal packet, so an action like clicking
>> > a button and dragging with another finger still works as expected.
>> >
>> > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
>> > synaptics_button_thresh=4100.
>> >
>> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> > ---
>> >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
>> >  1 files changed, 15 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> > index 7289d88..e67778d 100644
>> > --- a/drivers/input/mouse/synaptics.c
>> > +++ b/drivers/input/mouse/synaptics.c
>> > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
>> >  MODULE_PARM_DESC(synaptics_multitouch,
>> >                 "Enable multitouch mode on Synaptics devices");
>> >
>> > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
>> > +module_param(synaptics_button_thresh, int, 0644);
>> > +MODULE_PARM_DESC(synaptics_button_thres,
>> > +                "Y coordinate threshold of integrated buttons on Synaptics "
>> > +                "devices");
>> > +
>> >  /*****************************************************************************
>> >  *     Stuff we need even when we do not want native Synaptics support
>> >  ****************************************************************************/
>> > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
>> >        }
>> >  }
>> >
>> > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
>> > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
>> > +                               synaptics_button_thresh))
>> > +
>> >  /*
>> >  *  called for each full received packet from the touchpad
>> >  */
>> > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>> >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
>> >
>> >        if (SYN_MULTITOUCH(priv, &hw)) {
>> > -               if (hw.z > 0) {
>> > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
>> >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
>> >                        input_report_abs(dev, ABS_MT_POSITION_Y,
>> >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
>> > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>> >                return;
>> >        }
>> >
>> > +       /* If touch occurs over depressed button, ignore it */
>> > +       if (TOUCH_OVER_BUTTON(hw))
>> > +               hw.z = 0;
>> > +
>> >        if (hw.z > 0) {
>> >                priv->num_fingers++;
>> >                finger_width = 5;
>> > --
>> > 1.7.1
>> >
>> >
>>
>> I'm convinced now that clickpad style touchpads can't work without
>> multi-touch and something like logic in xf86-input-multitouch.
>
> Actually Clickpad works without multi-touch patch.  With my patches to
> synaptics, it worked in some level.  There are many restrictions (e.g.
> pushing the button first then drag), though.
>

True, but if I understand synaptic hardware MT behavior (sends
actively moving finger in higher resolution packet regardless of
original finger touch) then your patch will result in jumpy cursor on
X side and that side would need patches to attempt to guess invalid
data and discard.  I've worked on a few similar patches to various
xf86-input-* and generally they've failed to detect difference between
invalid packets vs. fast user movements.

The main point of my 3 options was to address jumpy cursor in
xf86-input-* that are not MT aware.  I think ABS_X/ABS_Y should only
allow its meaning to change at detectable time periods so user can
account for it and, specifically, that time period is best at
transition of BTN_TOOL_DOUBLETAP.

Assuming its easy enough to support exact rules for ABS_X/ABS_Y
changing meanings on kernel side (which I think it probably is pretty
easy), I think we should do it so that applications don't have to
become MT-aware as the official solution for jumpy cursors.

Chris

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
@ 2010-10-11 17:46             ` Chris Bagwell
  0 siblings, 0 replies; 43+ messages in thread
From: Chris Bagwell @ 2010-10-11 17:46 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Chase Douglas, linux-input, xorg-devel, Dmitry Torokhov,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On Mon, Oct 11, 2010 at 12:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 11 Oct 2010 11:24:04 -0500,
> Chris Bagwell wrote:
>>
>> On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>> > Now that we have proper multitouch support, we can handle integrated
>> > buttons better. If we know the top of the buttons on the touchpad, we
>> > can ignore any touches that occur within the touchpad area while a
>> > button is clicked. It may be possible to get the button area by querying
>> > the device, but for now allow the user to manually set it.
>> >
>> > A note on why this works: the Synaptics touchpads have pseudo touch
>> > tracking. When two touches are on the touchpad, an MT touch packet with
>> > just the X, Y, and pressure values is sent before a normal Synaptics
>> > touch packet. When one touch is obviously in motion and the other is
>> > stationary, the touchpad controller sends the touch in motion in the
>> > normal packet and the stationary touch in the MT packet. Single touch
>> > emulation is provided by the normal packet, so an action like clicking
>> > a button and dragging with another finger still works as expected.
>> >
>> > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
>> > synaptics_button_thresh=4100.
>> >
>> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> > ---
>> >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
>> >  1 files changed, 15 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> > index 7289d88..e67778d 100644
>> > --- a/drivers/input/mouse/synaptics.c
>> > +++ b/drivers/input/mouse/synaptics.c
>> > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
>> >  MODULE_PARM_DESC(synaptics_multitouch,
>> >                 "Enable multitouch mode on Synaptics devices");
>> >
>> > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
>> > +module_param(synaptics_button_thresh, int, 0644);
>> > +MODULE_PARM_DESC(synaptics_button_thres,
>> > +                "Y coordinate threshold of integrated buttons on Synaptics "
>> > +                "devices");
>> > +
>> >  /*****************************************************************************
>> >  *     Stuff we need even when we do not want native Synaptics support
>> >  ****************************************************************************/
>> > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
>> >        }
>> >  }
>> >
>> > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
>> > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
>> > +                               synaptics_button_thresh))
>> > +
>> >  /*
>> >  *  called for each full received packet from the touchpad
>> >  */
>> > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>> >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
>> >
>> >        if (SYN_MULTITOUCH(priv, &hw)) {
>> > -               if (hw.z > 0) {
>> > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
>> >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
>> >                        input_report_abs(dev, ABS_MT_POSITION_Y,
>> >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
>> > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>> >                return;
>> >        }
>> >
>> > +       /* If touch occurs over depressed button, ignore it */
>> > +       if (TOUCH_OVER_BUTTON(hw))
>> > +               hw.z = 0;
>> > +
>> >        if (hw.z > 0) {
>> >                priv->num_fingers++;
>> >                finger_width = 5;
>> > --
>> > 1.7.1
>> >
>> >
>>
>> I'm convinced now that clickpad style touchpads can't work without
>> multi-touch and something like logic in xf86-input-multitouch.
>
> Actually Clickpad works without multi-touch patch.  With my patches to
> synaptics, it worked in some level.  There are many restrictions (e.g.
> pushing the button first then drag), though.
>

True, but if I understand synaptic hardware MT behavior (sends
actively moving finger in higher resolution packet regardless of
original finger touch) then your patch will result in jumpy cursor on
X side and that side would need patches to attempt to guess invalid
data and discard.  I've worked on a few similar patches to various
xf86-input-* and generally they've failed to detect difference between
invalid packets vs. fast user movements.

The main point of my 3 options was to address jumpy cursor in
xf86-input-* that are not MT aware.  I think ABS_X/ABS_Y should only
allow its meaning to change at detectable time periods so user can
account for it and, specifically, that time period is best at
transition of BTN_TOOL_DOUBLETAP.

Assuming its easy enough to support exact rules for ABS_X/ABS_Y
changing meanings on kernel side (which I think it probably is pretty
easy), I think we should do it so that applications don't have to
become MT-aware as the official solution for jumpy cursors.

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-11 17:46             ` Chris Bagwell
  (?)
@ 2010-10-11 17:54             ` Henrik Rydberg
  -1 siblings, 0 replies; 43+ messages in thread
From: Henrik Rydberg @ 2010-10-11 17:54 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Takashi Iwai, Chase Douglas, linux-input, xorg-devel,
	Dmitry Torokhov, Andy Whitcroft, linux-kernel, Peter Hutterer,
	Duncan McGreggor

On 10/11/2010 07:46 PM, Chris Bagwell wrote:
[...]

>>> I'm convinced now that clickpad style touchpads can't work without
>>> multi-touch and something like logic in xf86-input-multitouch.
>>
>> Actually Clickpad works without multi-touch patch.  With my patches to
>> synaptics, it worked in some level.  There are many restrictions (e.g.
>> pushing the button first then drag), though.
>>
> 
> True, but if I understand synaptic hardware MT behavior (sends
> actively moving finger in higher resolution packet regardless of
> original finger touch) then your patch will result in jumpy cursor on
> X side and that side would need patches to attempt to guess invalid
> data and discard.  I've worked on a few similar patches to various
> xf86-input-* and generally they've failed to detect difference between
> invalid packets vs. fast user movements.
> 
> The main point of my 3 options was to address jumpy cursor in
> xf86-input-* that are not MT aware.  I think ABS_X/ABS_Y should only
> allow its meaning to change at detectable time periods so user can
> account for it and, specifically, that time period is best at
> transition of BTN_TOOL_DOUBLETAP.
> 
> Assuming its easy enough to support exact rules for ABS_X/ABS_Y
> changing meanings on kernel side (which I think it probably is pretty
> easy), I think we should do it so that applications don't have to
> become MT-aware as the official solution for jumpy cursors.


A solution like the one you describe was actually proposed and agreed upon in
the beginning of this thread, so I think the only thing left to do is actually
write the patch. :-) I can only assume that Takashi or Chase will be back in a
bit with an update.

Cheers,
Henrik

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
  2010-10-11 17:46             ` Chris Bagwell
@ 2010-10-11 18:29               ` Takashi Iwai
  -1 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 18:29 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Chase Douglas, linux-input, xorg-devel, Dmitry Torokhov,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 12:46:40 -0500,
Chris Bagwell wrote:
> 
> On Mon, Oct 11, 2010 at 12:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Mon, 11 Oct 2010 11:24:04 -0500,
> > Chris Bagwell wrote:
> >>
> >> On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> >> <chase.douglas@canonical.com> wrote:
> >> > Now that we have proper multitouch support, we can handle integrated
> >> > buttons better. If we know the top of the buttons on the touchpad, we
> >> > can ignore any touches that occur within the touchpad area while a
> >> > button is clicked. It may be possible to get the button area by querying
> >> > the device, but for now allow the user to manually set it.
> >> >
> >> > A note on why this works: the Synaptics touchpads have pseudo touch
> >> > tracking. When two touches are on the touchpad, an MT touch packet with
> >> > just the X, Y, and pressure values is sent before a normal Synaptics
> >> > touch packet. When one touch is obviously in motion and the other is
> >> > stationary, the touchpad controller sends the touch in motion in the
> >> > normal packet and the stationary touch in the MT packet. Single touch
> >> > emulation is provided by the normal packet, so an action like clicking
> >> > a button and dragging with another finger still works as expected.
> >> >
> >> > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> >> > synaptics_button_thresh=4100.
> >> >
> >> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >> > ---
> >> >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> >> >  1 files changed, 15 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >> > index 7289d88..e67778d 100644
> >> > --- a/drivers/input/mouse/synaptics.c
> >> > +++ b/drivers/input/mouse/synaptics.c
> >> > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> >> >  MODULE_PARM_DESC(synaptics_multitouch,
> >> >                 "Enable multitouch mode on Synaptics devices");
> >> >
> >> > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> >> > +module_param(synaptics_button_thresh, int, 0644);
> >> > +MODULE_PARM_DESC(synaptics_button_thres,
> >> > +                "Y coordinate threshold of integrated buttons on Synaptics "
> >> > +                "devices");
> >> > +
> >> >  /*****************************************************************************
> >> >  *     Stuff we need even when we do not want native Synaptics support
> >> >  ****************************************************************************/
> >> > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> >> >        }
> >> >  }
> >> >
> >> > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> >> > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> >> > +                               synaptics_button_thresh))
> >> > +
> >> >  /*
> >> >  *  called for each full received packet from the touchpad
> >> >  */
> >> > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >> >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> >> >
> >> >        if (SYN_MULTITOUCH(priv, &hw)) {
> >> > -               if (hw.z > 0) {
> >> > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> >> >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> >> >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> >> >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> >> > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >> >                return;
> >> >        }
> >> >
> >> > +       /* If touch occurs over depressed button, ignore it */
> >> > +       if (TOUCH_OVER_BUTTON(hw))
> >> > +               hw.z = 0;
> >> > +
> >> >        if (hw.z > 0) {
> >> >                priv->num_fingers++;
> >> >                finger_width = 5;
> >> > --
> >> > 1.7.1
> >> >
> >> >
> >>
> >> I'm convinced now that clickpad style touchpads can't work without
> >> multi-touch and something like logic in xf86-input-multitouch.
> >
> > Actually Clickpad works without multi-touch patch.  With my patches to
> > synaptics, it worked in some level.  There are many restrictions (e.g.
> > pushing the button first then drag), though.
> >
> 
> True, but if I understand synaptic hardware MT behavior (sends
> actively moving finger in higher resolution packet regardless of
> original finger touch) then your patch will result in jumpy cursor on
> X side and that side would need patches to attempt to guess invalid
> data and discard.  I've worked on a few similar patches to various
> xf86-input-* and generally they've failed to detect difference between
> invalid packets vs. fast user movements.

Right.  I also tackled to this once.

> The main point of my 3 options was to address jumpy cursor in
> xf86-input-* that are not MT aware.  I think ABS_X/ABS_Y should only
> allow its meaning to change at detectable time periods so user can
> account for it and, specifically, that time period is best at
> transition of BTN_TOOL_DOUBLETAP.
> 
> Assuming its easy enough to support exact rules for ABS_X/ABS_Y
> changing meanings on kernel side (which I think it probably is pretty
> easy), I think we should do it so that applications don't have to
> become MT-aware as the official solution for jumpy cursors.

Hm, so are you suggesting that we need to transform MT events to
non-MT events in the kernel side with some clever filtering?
I don't know whether this is really needed...  If the patch is minimal
amount, it might be worth to get in, though.

However, I feel that we should forget about clickpad support in
kernel; as you mentioned, Clickpad works properly only with the
multi-touch, after all...

Rather I'm concerned how to decide the kernel driver's behavior,
whether behaving in MT or non-MT mode.  As Henrik suggested, in MT
mode, all events should be ABS_MT_* type.  But non-MT-aware user-space
apps won't understand all these events.  So, we'll need some fallback
mechanism for non-MT apps, anyway.

I've implemented multi_touch module option for that purpose, but it's
hacikish.  Dmitry, do you have any suggestion?


thanks,

Takashi

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 3/3] Input: synaptics - remove touches over button click area
@ 2010-10-11 18:29               ` Takashi Iwai
  0 siblings, 0 replies; 43+ messages in thread
From: Takashi Iwai @ 2010-10-11 18:29 UTC (permalink / raw)
  To: Chris Bagwell
  Cc: Chase Douglas, linux-input, xorg-devel, Dmitry Torokhov,
	Andy Whitcroft, Henrik Rydberg, linux-kernel, Peter Hutterer,
	Duncan McGreggor

At Mon, 11 Oct 2010 12:46:40 -0500,
Chris Bagwell wrote:
> 
> On Mon, Oct 11, 2010 at 12:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Mon, 11 Oct 2010 11:24:04 -0500,
> > Chris Bagwell wrote:
> >>
> >> On Fri, Oct 8, 2010 at 9:58 AM, Chase Douglas
> >> <chase.douglas@canonical.com> wrote:
> >> > Now that we have proper multitouch support, we can handle integrated
> >> > buttons better. If we know the top of the buttons on the touchpad, we
> >> > can ignore any touches that occur within the touchpad area while a
> >> > button is clicked. It may be possible to get the button area by querying
> >> > the device, but for now allow the user to manually set it.
> >> >
> >> > A note on why this works: the Synaptics touchpads have pseudo touch
> >> > tracking. When two touches are on the touchpad, an MT touch packet with
> >> > just the X, Y, and pressure values is sent before a normal Synaptics
> >> > touch packet. When one touch is obviously in motion and the other is
> >> > stationary, the touchpad controller sends the touch in motion in the
> >> > normal packet and the stationary touch in the MT packet. Single touch
> >> > emulation is provided by the normal packet, so an action like clicking
> >> > a button and dragging with another finger still works as expected.
> >> >
> >> > Tested on a Dell Mini 1012 with synaptics_multitouch=1 and
> >> > synaptics_button_thresh=4100.
> >> >
> >> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >> > ---
> >> >  drivers/input/mouse/synaptics.c |   16 +++++++++++++++-
> >> >  1 files changed, 15 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >> > index 7289d88..e67778d 100644
> >> > --- a/drivers/input/mouse/synaptics.c
> >> > +++ b/drivers/input/mouse/synaptics.c
> >> > @@ -49,6 +49,12 @@ module_param(synaptics_multitouch, bool, 0644);
> >> >  MODULE_PARM_DESC(synaptics_multitouch,
> >> >                 "Enable multitouch mode on Synaptics devices");
> >> >
> >> > +static int synaptics_button_thresh = YMIN_NOMINAL + YMAX_NOMINAL;
> >> > +module_param(synaptics_button_thresh, int, 0644);
> >> > +MODULE_PARM_DESC(synaptics_button_thres,
> >> > +                "Y coordinate threshold of integrated buttons on Synaptics "
> >> > +                "devices");
> >> > +
> >> >  /*****************************************************************************
> >> >  *     Stuff we need even when we do not want native Synaptics support
> >> >  ****************************************************************************/
> >> > @@ -463,6 +469,10 @@ static void synaptics_parse_hw_state(unsigned char buf[], struct synaptics_data
> >> >        }
> >> >  }
> >> >
> >> > +#define TOUCH_OVER_BUTTON(hw) (((hw).left || (hw).middle || (hw).right) && \
> >> > +                              (YMAX_NOMINAL + YMIN_NOMINAL - (hw).y > \
> >> > +                               synaptics_button_thresh))
> >> > +
> >> >  /*
> >> >  *  called for each full received packet from the touchpad
> >> >  */
> >> > @@ -477,7 +487,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >> >        synaptics_parse_hw_state(psmouse->packet, priv, &hw);
> >> >
> >> >        if (SYN_MULTITOUCH(priv, &hw)) {
> >> > -               if (hw.z > 0) {
> >> > +               if (hw.z > 0 && !TOUCH_OVER_BUTTON(hw)) {
> >> >                        input_report_abs(dev, ABS_MT_POSITION_X, hw.x);
> >> >                        input_report_abs(dev, ABS_MT_POSITION_Y,
> >> >                                         YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> >> > @@ -509,6 +519,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >> >                return;
> >> >        }
> >> >
> >> > +       /* If touch occurs over depressed button, ignore it */
> >> > +       if (TOUCH_OVER_BUTTON(hw))
> >> > +               hw.z = 0;
> >> > +
> >> >        if (hw.z > 0) {
> >> >                priv->num_fingers++;
> >> >                finger_width = 5;
> >> > --
> >> > 1.7.1
> >> >
> >> >
> >>
> >> I'm convinced now that clickpad style touchpads can't work without
> >> multi-touch and something like logic in xf86-input-multitouch.
> >
> > Actually Clickpad works without multi-touch patch.  With my patches to
> > synaptics, it worked in some level.  There are many restrictions (e.g.
> > pushing the button first then drag), though.
> >
> 
> True, but if I understand synaptic hardware MT behavior (sends
> actively moving finger in higher resolution packet regardless of
> original finger touch) then your patch will result in jumpy cursor on
> X side and that side would need patches to attempt to guess invalid
> data and discard.  I've worked on a few similar patches to various
> xf86-input-* and generally they've failed to detect difference between
> invalid packets vs. fast user movements.

Right.  I also tackled to this once.

> The main point of my 3 options was to address jumpy cursor in
> xf86-input-* that are not MT aware.  I think ABS_X/ABS_Y should only
> allow its meaning to change at detectable time periods so user can
> account for it and, specifically, that time period is best at
> transition of BTN_TOOL_DOUBLETAP.
> 
> Assuming its easy enough to support exact rules for ABS_X/ABS_Y
> changing meanings on kernel side (which I think it probably is pretty
> easy), I think we should do it so that applications don't have to
> become MT-aware as the official solution for jumpy cursors.

Hm, so are you suggesting that we need to transform MT events to
non-MT events in the kernel side with some clever filtering?
I don't know whether this is really needed...  If the patch is minimal
amount, it might be worth to get in, though.

However, I feel that we should forget about clickpad support in
kernel; as you mentioned, Clickpad works properly only with the
multi-touch, after all...

Rather I'm concerned how to decide the kernel driver's behavior,
whether behaving in MT or non-MT mode.  As Henrik suggested, in MT
mode, all events should be ABS_MT_* type.  But non-MT-aware user-space
apps won't understand all these events.  So, we'll need some fallback
mechanism for non-MT apps, anyway.

I've implemented multi_touch module option for that purpose, but it's
hacikish.  Dmitry, do you have any suggestion?


thanks,

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, other threads:[~2010-10-11 18:30 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-08 14:57 [PATCH 0/3] Input: synaptics - multitouch and multifinger support Chase Douglas
2010-10-08 14:57 ` [PATCH 1/3] Input: synaptics - add multitouch support Chase Douglas
2010-10-08 14:57   ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chase Douglas
2010-10-08 14:58     ` [PATCH 3/3] Input: synaptics - remove touches over button click area Chase Douglas
2010-10-10 15:58       ` Chris Bagwell
2010-10-11 16:24       ` Chris Bagwell
2010-10-11 16:24         ` Chris Bagwell
2010-10-11 17:10         ` Takashi Iwai
2010-10-11 17:10           ` Takashi Iwai
2010-10-11 17:30           ` Dmitry Torokhov
2010-10-11 17:30             ` Dmitry Torokhov
2010-10-11 17:40             ` Takashi Iwai
2010-10-11 17:46           ` Chris Bagwell
2010-10-11 17:46             ` Chris Bagwell
2010-10-11 17:54             ` Henrik Rydberg
2010-10-11 18:29             ` Takashi Iwai
2010-10-11 18:29               ` Takashi Iwai
2010-10-10 15:44     ` [PATCH 2/3] Input: synaptics - add multitouch multifinger support Chris Bagwell
2010-10-10 15:37   ` [PATCH 1/3] Input: synaptics - add multitouch support Chris Bagwell
2010-10-10 15:41   ` Chris Bagwell
2010-10-08 16:37 ` [PATCH 0/3] Input: synaptics - multitouch and multifinger support Takashi Iwai
2010-10-08 16:38   ` Takashi Iwai
2010-10-08 17:48     ` Takashi Iwai
2010-10-08 17:15   ` Chase Douglas
2010-10-08 17:46     ` Takashi Iwai
2010-10-08 18:04     ` Dmitry Torokhov
2010-10-08 19:31       ` Takashi Iwai
2010-10-10 21:04         ` Dmitry Torokhov
2010-10-11  7:35           ` Takashi Iwai
2010-10-11  7:48             ` Henrik Rydberg
2010-10-11  7:59               ` Takashi Iwai
2010-10-11 13:41               ` Chris Bagwell
2010-10-11 13:41                 ` Chris Bagwell
2010-10-11 14:01                 ` Takashi Iwai
2010-10-11 14:01                   ` Takashi Iwai
2010-10-11 14:24                   ` Henrik Rydberg
2010-10-11 14:49                     ` Takashi Iwai
2010-10-11 15:31                       ` Henrik Rydberg
2010-10-11 15:58                         ` Takashi Iwai
2010-10-10  7:49   ` Henrik Rydberg
2010-10-10 20:59     ` Dmitry Torokhov
2010-10-11  7:28       ` Takashi Iwai
2010-10-11  7:40         ` Henrik Rydberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.