diff mbox

[01/01] Input multitouch: fix horizontal two-finger-scrolling on Sentelic touchpads

Message ID 20121212012135.GA9043@sherka.no-ip.biz
State New, archived
Headers show

Commit Message

Christophe TORDEUX Dec. 12, 2012, 1:21 a.m. UTC
From: Christophe TORDEUX <christophe@tordeux.net>

Fix horizontal two-finger scrolling on Sentelic touchpads. Currently
horizontal two-fingers scrolling does not work with these touchpads.
The patch also makes vertical two-finger scrolling smoother in some
applications e.g. Firefox.

Signed-off-by: Christophe TORDEUX <christophe@tordeux.net>
----

This patch was inspired by the reading of
drivers/input/mouse/synaptics.c. It is known to work (tested) on
Sentelic touchpads version STL3888_C0. Very probably works on all later
versions, and possibly works on some earlier versions of the hardware in
question.

Comments

Christophe TORDEUX Dec. 12, 2012, 1:27 a.m. UTC | #1
I am very sorry I forgot to mention that my patch was done against 
kernel version 3.7-rc8. I can re-send the original patch with this 
information included if required, just ask me.
Henrik Rydberg Dec. 19, 2012, 8:41 a.m. UTC | #2
Hi Christophe,

> Fix horizontal two-finger scrolling on Sentelic touchpads. Currently
> horizontal two-fingers scrolling does not work with these touchpads.
> The patch also makes vertical two-finger scrolling smoother in some
> applications e.g. Firefox.
> 
> Signed-off-by: Christophe TORDEUX <christophe@tordeux.net>
> ----
> 
> This patch was inspired by the reading of
> drivers/input/mouse/synaptics.c. It is known to work (tested) on
> Sentelic touchpads version STL3888_C0. Very probably works on all later
> versions, and possibly works on some earlier versions of the hardware in
> question.

Thanks for the patch. I would prefer it if this driver was converted
to use input_mt_assign_slots() instead.

> 
> diff -uprN -X vanilla/linux-3.7-rc8/Documentation/dontdiff 
> vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.c 
> linux-3.7-rc8/drivers/input/mouse/sentelic.c
> --- vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.c	2012-12-11 04:32:44.476930522 +0100
> +++ linux-3.7-rc8/drivers/input/mouse/sentelic.c	2012-12-12 01:16:41.766018017 +0100
> @@ -706,8 +706,10 @@ static psmouse_ret_t fsp_process_byte(st
>  	struct input_dev *dev = psmouse->dev;
>  	struct fsp_data *ad = psmouse->private;
>  	unsigned char *packet = psmouse->packet;
> +	unsigned char *last_packet = ad->last_pkt;
>  	unsigned char button_status = 0, lscroll = 0, rscroll = 0;
>  	unsigned short abs_x, abs_y, fgrs = 0;
> +	unsigned short last_abs_x, last_abs_y;
>  	int rel_x, rel_y;
>  
>  	if (psmouse->pktcnt < 4)
> @@ -734,6 +736,9 @@ static psmouse_ret_t fsp_process_byte(st
>  
>  		abs_x = GET_ABS_X(packet);
>  		abs_y = GET_ABS_Y(packet);
> +		last_abs_x = GET_ABS_X(last_packet);
> +		last_abs_y = GET_ABS_Y(last_packet);
> +		memcpy(ad->last_pkt, packet, psmouse->pktcnt);

Why not save the variables instead of the array?

>  
>  		if (packet[0] & FSP_PB0_MFMC) {
>  			/*
> @@ -753,10 +758,19 @@ static psmouse_ret_t fsp_process_byte(st
>  					 */
>  					fgrs = 1;
>  					fsp_set_slot(dev, 0, false, 0, 0);
> +
> +					/* We don't need to re-order
> +					 * coordinates if last finger
> +					 * was same finger
> +					 */
> +					last_abs_x = abs_x;
> +					last_abs_y = abs_y;
>  				}
>  				ad->last_mt_fgr = 2;
>  
> -				fsp_set_slot(dev, 1, fgrs == 2, abs_x, abs_y);
> +				fsp_set_slot(dev, 1, fgrs == 2,
> +						max(abs_x, last_abs_x),
> +						max(abs_y, last_abs_y));

Relying on the box (x1, y1, max(x1, x2), max(y1, y2)) seems to assume
a lot about the behavior of the device.

>  			} else {
>  				/* 1st finger */
>  				if (ad->last_mt_fgr == 1) {
> @@ -767,9 +781,18 @@ static psmouse_ret_t fsp_process_byte(st
>  					 */
>  					fgrs = 1;
>  					fsp_set_slot(dev, 1, false, 0, 0);
> +
> +					/* We don't need to re-order
> +					 * coordinates if last finger
> +					 * was same finger
> +					 */
> +					last_abs_x = abs_x;
> +					last_abs_y = abs_y;
>  				}
>  				ad->last_mt_fgr = 1;
> -				fsp_set_slot(dev, 0, fgrs != 0, abs_x, abs_y);
> +				fsp_set_slot(dev, 0, fgrs != 0,
> +						min(abs_x, last_abs_x),
> +						min(abs_y, last_abs_y));
>  			}
>  		} else {
>  			/* SFAC packet */
> @@ -788,12 +811,18 @@ static psmouse_ret_t fsp_process_byte(st
>  			if (abs_x != 0 && abs_y != 0)
>  				fgrs = 1;
>  
> +			/* We don't need to re-order
> +			 * coordinates of SFAC packets
> +			 */
> +			last_abs_x = abs_x;
> +			last_abs_y = abs_y;
> +
>  			fsp_set_slot(dev, 0, fgrs > 0, abs_x, abs_y);
>  			fsp_set_slot(dev, 1, false, 0, 0);
>  		}
>  		if (fgrs > 0) {
> -			input_report_abs(dev, ABS_X, abs_x);
> -			input_report_abs(dev, ABS_Y, abs_y);
> +			input_report_abs(dev, ABS_X, min(abs_x, last_abs_x));
> +			input_report_abs(dev, ABS_Y, min(abs_y, last_abs_y));

Looks like this point could differ from the reported MT positions.

>  		}
>  		input_report_key(dev, BTN_LEFT, packet[0] & 0x01);
>  		input_report_key(dev, BTN_RIGHT, packet[0] & 0x02);
> diff -uprN -X vanilla/linux-3.7-rc8/Documentation/dontdiff vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.h linux-3.7-rc8/drivers/input/mouse/sentelic.h
> --- vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.h	2012-12-11 04:32:44.476930522 +0100
> +++ linux-3.7-rc8/drivers/input/mouse/sentelic.h	2012-12-12 01:16:41.954018011 +0100
> @@ -117,6 +117,7 @@ struct fsp_data {
>  	unsigned char	last_reg;	/* Last register we requested read from */
>  	unsigned char	last_val;
>  	unsigned int	last_mt_fgr;	/* Last seen finger(multitouch) */
> +	unsigned char	last_pkt[8];    /* Last packet we processed from fsp */
>  };
>  
>  #ifdef CONFIG_MOUSE_PS2_SENTELIC

Thanks.
Henrik

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christophe TORDEUX Dec. 19, 2012, 9:42 p.m. UTC | #3
On Wednesday 19 December 2012 at 09:41:13AM, Henrik Rydberg has written:
> Hi Christophe,
> 
> > Fix horizontal two-finger scrolling on Sentelic touchpads. Currently
> > horizontal two-fingers scrolling does not work with these touchpads.
> > The patch also makes vertical two-finger scrolling smoother in some
> > applications e.g. Firefox.
> > 
> > Signed-off-by: Christophe TORDEUX <christophe@tordeux.net>
> > ----
> > 
> > This patch was inspired by the reading of
> > drivers/input/mouse/synaptics.c. It is known to work (tested) on
> > Sentelic touchpads version STL3888_C0. Very probably works on all later
> > versions, and possibly works on some earlier versions of the hardware in
> > question.
> 
> Thanks for the patch. I would prefer it if this driver was converted
> to use input_mt_assign_slots() instead.
 
 Thanks for you feedback. 
 
 
 As for converting the driver to use input_mt_assign_slot, I can try, 
 and I have the device, give me just a couple of weeks.
 
 I guess a measure of complete success would be to have the device able 
 to function in userspace purely based on multitouch events, with 
 non-multitouch absolute events being kept simply as a legacy feature, 
 in case one needs to use input userspace drivers like evdev which are 
 not specifically touchpad/touchscreen drivers.

 However I'm not sure the synaptics xorg driver is fit to achieve this.  
 With this userspace driver as it stands today, a few more or less 
 functional multitouch features are achieved because the synaptics xorg 
 driver interprets a mixture of MT and non-MT absolute events. The 
 sequence in which MT and non-MT events arrive matters. 

> Why not save the variables instead of the array?

> Relying on the box (x1, y1, max(x1, x2), max(y1, y2)) seems to assume
> a lot about the behavior of the device.

 Well, as for your other objections, I guess I would agree today we 
 don't have to report touches which are (potentially) not located where 
 the physical fingers were detected, like I was doing in this early 
 attempt. Besides, saving data from the device's previous packet is not 
 an absolute necessity.

 I'm now going to post a much simpler patch, and probably cleaner. The 
 way I see it this patch will have a double purpose:
 
 1) fix horizontal and improve vertical scrolling, though maybe in a 
 coincidental way and 2) work towards making the non-MT absolute events 
 a purely legacy single finger fallback mode, without breaking anything.

 I'll post my new patch in something like an hour.

> > 
> > diff -uprN -X vanilla/linux-3.7-rc8/Documentation/dontdiff 
> > vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.c 
> > linux-3.7-rc8/drivers/input/mouse/sentelic.c
> > --- vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.c	2012-12-11 04:32:44.476930522 +0100
> > +++ linux-3.7-rc8/drivers/input/mouse/sentelic.c	2012-12-12 01:16:41.766018017 +0100
> > @@ -706,8 +706,10 @@ static psmouse_ret_t fsp_process_byte(st
> >  	struct input_dev *dev = psmouse->dev;
> >  	struct fsp_data *ad = psmouse->private;
> >  	unsigned char *packet = psmouse->packet;
> > +	unsigned char *last_packet = ad->last_pkt;
> >  	unsigned char button_status = 0, lscroll = 0, rscroll = 0;
> >  	unsigned short abs_x, abs_y, fgrs = 0;
> > +	unsigned short last_abs_x, last_abs_y;
> >  	int rel_x, rel_y;
> >  
> >  	if (psmouse->pktcnt < 4)
> > @@ -734,6 +736,9 @@ static psmouse_ret_t fsp_process_byte(st
> >  
> >  		abs_x = GET_ABS_X(packet);
> >  		abs_y = GET_ABS_Y(packet);
> > +		last_abs_x = GET_ABS_X(last_packet);
> > +		last_abs_y = GET_ABS_Y(last_packet);
> > +		memcpy(ad->last_pkt, packet, psmouse->pktcnt);
> 
> >  
> >  		if (packet[0] & FSP_PB0_MFMC) {
> >  			/*
> > @@ -753,10 +758,19 @@ static psmouse_ret_t fsp_process_byte(st
> >  					 */
> >  					fgrs = 1;
> >  					fsp_set_slot(dev, 0, false, 0, 0);
> > +
> > +					/* We don't need to re-order
> > +					 * coordinates if last finger
> > +					 * was same finger
> > +					 */
> > +					last_abs_x = abs_x;
> > +					last_abs_y = abs_y;
> >  				}
> >  				ad->last_mt_fgr = 2;
> >  
> > -				fsp_set_slot(dev, 1, fgrs == 2, abs_x, abs_y);
> > +				fsp_set_slot(dev, 1, fgrs == 2,
> > +						max(abs_x, last_abs_x),
> > +						max(abs_y, last_abs_y));
> 
> 
> >  			} else {
> >  				/* 1st finger */
> >  				if (ad->last_mt_fgr == 1) {
> > @@ -767,9 +781,18 @@ static psmouse_ret_t fsp_process_byte(st
> >  					 */
> >  					fgrs = 1;
> >  					fsp_set_slot(dev, 1, false, 0, 0);
> > +
> > +					/* We don't need to re-order
> > +					 * coordinates if last finger
> > +					 * was same finger
> > +					 */
> > +					last_abs_x = abs_x;
> > +					last_abs_y = abs_y;
> >  				}
> >  				ad->last_mt_fgr = 1;
> > -				fsp_set_slot(dev, 0, fgrs != 0, abs_x, abs_y);
> > +				fsp_set_slot(dev, 0, fgrs != 0,
> > +						min(abs_x, last_abs_x),
> > +						min(abs_y, last_abs_y));
> >  			}
> >  		} else {
> >  			/* SFAC packet */
> > @@ -788,12 +811,18 @@ static psmouse_ret_t fsp_process_byte(st
> >  			if (abs_x != 0 && abs_y != 0)
> >  				fgrs = 1;
> >  
> > +			/* We don't need to re-order
> > +			 * coordinates of SFAC packets
> > +			 */
> > +			last_abs_x = abs_x;
> > +			last_abs_y = abs_y;
> > +
> >  			fsp_set_slot(dev, 0, fgrs > 0, abs_x, abs_y);
> >  			fsp_set_slot(dev, 1, false, 0, 0);
> >  		}
> >  		if (fgrs > 0) {
> > -			input_report_abs(dev, ABS_X, abs_x);
> > -			input_report_abs(dev, ABS_Y, abs_y);
> > +			input_report_abs(dev, ABS_X, min(abs_x, last_abs_x));
> > +			input_report_abs(dev, ABS_Y, min(abs_y, last_abs_y));
> 
> Looks like this point could differ from the reported MT positions.
> 
> >  		}
> >  		input_report_key(dev, BTN_LEFT, packet[0] & 0x01);
> >  		input_report_key(dev, BTN_RIGHT, packet[0] & 0x02);
> > diff -uprN -X vanilla/linux-3.7-rc8/Documentation/dontdiff vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.h linux-3.7-rc8/drivers/input/mouse/sentelic.h
> > --- vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.h	2012-12-11 04:32:44.476930522 +0100
> > +++ linux-3.7-rc8/drivers/input/mouse/sentelic.h	2012-12-12 01:16:41.954018011 +0100
> > @@ -117,6 +117,7 @@ struct fsp_data {
> >  	unsigned char	last_reg;	/* Last register we requested read from */
> >  	unsigned char	last_val;
> >  	unsigned int	last_mt_fgr;	/* Last seen finger(multitouch) */
> > +	unsigned char	last_pkt[8];    /* Last packet we processed from fsp */
> >  };
> >  
> >  #ifdef CONFIG_MOUSE_PS2_SENTELIC
> 
> Thanks.
> Henrik
> 
>
diff mbox

Patch

diff -uprN -X vanilla/linux-3.7-rc8/Documentation/dontdiff 
vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.c 
linux-3.7-rc8/drivers/input/mouse/sentelic.c
--- vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.c	2012-12-11 04:32:44.476930522 +0100
+++ linux-3.7-rc8/drivers/input/mouse/sentelic.c	2012-12-12 01:16:41.766018017 +0100
@@ -706,8 +706,10 @@  static psmouse_ret_t fsp_process_byte(st
 	struct input_dev *dev = psmouse->dev;
 	struct fsp_data *ad = psmouse->private;
 	unsigned char *packet = psmouse->packet;
+	unsigned char *last_packet = ad->last_pkt;
 	unsigned char button_status = 0, lscroll = 0, rscroll = 0;
 	unsigned short abs_x, abs_y, fgrs = 0;
+	unsigned short last_abs_x, last_abs_y;
 	int rel_x, rel_y;
 
 	if (psmouse->pktcnt < 4)
@@ -734,6 +736,9 @@  static psmouse_ret_t fsp_process_byte(st
 
 		abs_x = GET_ABS_X(packet);
 		abs_y = GET_ABS_Y(packet);
+		last_abs_x = GET_ABS_X(last_packet);
+		last_abs_y = GET_ABS_Y(last_packet);
+		memcpy(ad->last_pkt, packet, psmouse->pktcnt);
 
 		if (packet[0] & FSP_PB0_MFMC) {
 			/*
@@ -753,10 +758,19 @@  static psmouse_ret_t fsp_process_byte(st
 					 */
 					fgrs = 1;
 					fsp_set_slot(dev, 0, false, 0, 0);
+
+					/* We don't need to re-order
+					 * coordinates if last finger
+					 * was same finger
+					 */
+					last_abs_x = abs_x;
+					last_abs_y = abs_y;
 				}
 				ad->last_mt_fgr = 2;
 
-				fsp_set_slot(dev, 1, fgrs == 2, abs_x, abs_y);
+				fsp_set_slot(dev, 1, fgrs == 2,
+						max(abs_x, last_abs_x),
+						max(abs_y, last_abs_y));
 			} else {
 				/* 1st finger */
 				if (ad->last_mt_fgr == 1) {
@@ -767,9 +781,18 @@  static psmouse_ret_t fsp_process_byte(st
 					 */
 					fgrs = 1;
 					fsp_set_slot(dev, 1, false, 0, 0);
+
+					/* We don't need to re-order
+					 * coordinates if last finger
+					 * was same finger
+					 */
+					last_abs_x = abs_x;
+					last_abs_y = abs_y;
 				}
 				ad->last_mt_fgr = 1;
-				fsp_set_slot(dev, 0, fgrs != 0, abs_x, abs_y);
+				fsp_set_slot(dev, 0, fgrs != 0,
+						min(abs_x, last_abs_x),
+						min(abs_y, last_abs_y));
 			}
 		} else {
 			/* SFAC packet */
@@ -788,12 +811,18 @@  static psmouse_ret_t fsp_process_byte(st
 			if (abs_x != 0 && abs_y != 0)
 				fgrs = 1;
 
+			/* We don't need to re-order
+			 * coordinates of SFAC packets
+			 */
+			last_abs_x = abs_x;
+			last_abs_y = abs_y;
+
 			fsp_set_slot(dev, 0, fgrs > 0, abs_x, abs_y);
 			fsp_set_slot(dev, 1, false, 0, 0);
 		}
 		if (fgrs > 0) {
-			input_report_abs(dev, ABS_X, abs_x);
-			input_report_abs(dev, ABS_Y, abs_y);
+			input_report_abs(dev, ABS_X, min(abs_x, last_abs_x));
+			input_report_abs(dev, ABS_Y, min(abs_y, last_abs_y));
 		}
 		input_report_key(dev, BTN_LEFT, packet[0] & 0x01);
 		input_report_key(dev, BTN_RIGHT, packet[0] & 0x02);
diff -uprN -X vanilla/linux-3.7-rc8/Documentation/dontdiff vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.h linux-3.7-rc8/drivers/input/mouse/sentelic.h
--- vanilla/linux-3.7-rc8/drivers/input/mouse/sentelic.h	2012-12-11 04:32:44.476930522 +0100
+++ linux-3.7-rc8/drivers/input/mouse/sentelic.h	2012-12-12 01:16:41.954018011 +0100
@@ -117,6 +117,7 @@  struct fsp_data {
 	unsigned char	last_reg;	/* Last register we requested read from */
 	unsigned char	last_val;
 	unsigned int	last_mt_fgr;	/* Last seen finger(multitouch) */
+	unsigned char	last_pkt[8];    /* Last packet we processed from fsp */
 };
 
 #ifdef CONFIG_MOUSE_PS2_SENTELIC