linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Yunkang Tang <yunkang.tang@cn.alps.com>, linux-input@vger.kernel.org
Subject: Re: [PATCH 14/14] alps: Add support for v7 devices
Date: Sat, 26 Jul 2014 10:22:08 +0200	[thread overview]
Message-ID: <53D36530.9020408@redhat.com> (raw)
In-Reply-To: <20140726055811.GA31435@core.coreip.homeip.net>

Hi,

On 07/26/2014 07:59 AM, Dmitry Torokhov wrote:> Hi Hans,
>
> On Wed, Jul 09, 2014 at 05:24:05PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> This series started out as a single patch to add support for a new model
>> of alps touchpad, called PROTO_V7 in this patchset.
>>
>> While working on this I ended up doing some refactoring as preparation, which
>> I tested on a Rushmore alps touchpad, which lead to some bugfixes and more
>> cleanups, etc.
>>
>> The result is a 14 patch patch-set, which:
>>
>> 1) Significantly improves multi-touch support on V3 and V4 models (including
>>    the Rushmore V3 variant)
>> 2) Improves the code quality / readability quite a bit
>> 3) Adds support for PROTO_V7
>
> Excellent series, very easy to read. I have applied everything but v7
> support as I have questions about that patch.

I'm glad you like the series, that shows that my work to split it into
manageable bits was worth the extra effort, so that is good to hear.

On 07/26/2014 07:58 AM, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Wed, Jul 09, 2014 at 05:24:19PM +0200, Hans de Goede wrote:
>> From: Yunkang Tang <yunkang.tang@cn.alps.com>
>>
>> Such as found on the new Toshiba Portégé Z30-A and Z40-A.
>>
>> Signed-off-by: Yunkang Tang <yunkang.tang@cn.alps.com>
>> [hdegoede@redhat.com: Remove softbutton handling, this is done in userspace]
>> [hdegoede@redhat.com: Report INPUT_PROP_BUTTONPAD]
>> [hdegoede@redhat.com: Do not report fake PRESSURE, reporting BTN_TOUCH is
>>  enough]
>> [hdegoede@redhat.com: Various cleanups / refactoring]
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/input/mouse/alps.c | 257 ++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/input/mouse/alps.h |  18 ++++
>>  2 files changed, 272 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index ad3a708..8b9b4b0 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -100,6 +100,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
>>  #define ALPS_PS2_INTERLEAVED	0x80	/* 3-byte PS/2 packet interleaved with
>>  					   6-byte ALPS packet */
>>  #define ALPS_IS_RUSHMORE	0x100	/* device is a rushmore */
>> +#define ALPS_BUTTONPAD		0x200	/* device is a clickpad */
>>  
>>  static const struct alps_model_info alps_model_data[] = {
>>  	{ { 0x32, 0x02, 0x14 },	0x00, ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT },	/* Toshiba Salellite Pro M10 */
>> @@ -845,6 +846,177 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
>>  	alps_report_semi_mt_data(psmouse, f->fingers);
>>  }
>>  
>> +static bool alps_is_valid_package_v7(struct psmouse *psmouse)
>> +{
>> +	if ((psmouse->pktcnt == 3) && ((psmouse->packet[2] & 0x40) != 0x40))
>> +		return false;
>> +	if ((psmouse->pktcnt == 4) && ((psmouse->packet[3] & 0x48) != 0x48))
>> +		return false;
>> +	if ((psmouse->pktcnt == 6) && ((psmouse->packet[5] & 0x40) != 0x0))
>> +		return false;
>> +	return true;
> 
> Maybe:
> 
> 	switch (psmouse->pktcnt) {
> 	case 3:
> 		return (psmouse->packet[2] & 0x40) == 0x40;
> 
> 	case 4:
> 		return (psmouse->packet[3] & 0x48) == 0x48;
> 
> 	case 6:
> 		return (psmouse->packet[5] & 0x40) == 0x00;
> 	}
> 
> 	return true;
> 
> ?

Will fix.

> 
> 
>> +}
>> +
>> +static unsigned char alps_get_packet_id_v7(char *byte)
>> +{
>> +	unsigned char packet_id;
>> +
>> +	if (byte[4] & 0x40)
>> +		packet_id = V7_PACKET_ID_TWO;
>> +	else if (byte[4] & 0x01)
>> +		packet_id = V7_PACKET_ID_MULTI;
>> +	else if ((byte[0] & 0x10) && !(byte[4] & 0x43))
>> +		packet_id = V7_PACKET_ID_NEW;
>> +	else if (byte[1] == 0x00 && byte[4] == 0x00)
>> +		packet_id = V7_PACKET_ID_IDLE;
>> +	else
>> +		packet_id = V7_PACKET_ID_UNKNOWN;
>> +
>> +	return packet_id;
>> +}
>> +
>> +static void alps_get_finger_coordinate_v7(struct input_mt_pos *mt,
>> +					  unsigned char *pkt,
>> +					  unsigned char pkt_id)
>> +{
>> +	mt[0].x = ((pkt[2] & 0x80) << 4);
>> +	mt[0].x |= ((pkt[2] & 0x3F) << 5);
>> +	mt[0].x |= ((pkt[3] & 0x30) >> 1);
>> +	mt[0].x |= (pkt[3] & 0x07);
>> +	mt[0].y = (pkt[1] << 3) | (pkt[0] & 0x07);
>> +
>> +	mt[1].x = ((pkt[3] & 0x80) << 4);
>> +	mt[1].x |= ((pkt[4] & 0x80) << 3);
>> +	mt[1].x |= ((pkt[4] & 0x3F) << 4);
>> +	mt[1].y = ((pkt[5] & 0x80) << 3);
>> +	mt[1].y |= ((pkt[5] & 0x3F) << 4);
>> +
>> +	if (pkt_id == V7_PACKET_ID_TWO) {
>> +		mt[1].x &= ~0x000F;
>> +		mt[1].y |= 0x000F;
>> +	} else if (pkt_id == V7_PACKET_ID_MULTI) {
>> +		mt[1].x &= ~0x003F;
>> +		mt[1].y &= ~0x0020;
>> +		mt[1].y |= ((pkt[4] & 0x02) << 4);
>> +		mt[1].y |= 0x001F;
>> +	} else if (pkt_id == V7_PACKET_ID_NEW) {
>> +		mt[1].x &= ~0x003F;
>> +		mt[1].x |= (pkt[0] & 0x20);
>> +		mt[1].y |= 0x000F;
>> +	}
>> +
>> +	mt[0].y = 0x7FF - mt[0].y;
>> +	mt[1].y = 0x7FF - mt[1].y;
>> +}
>> +
>> +static int alps_get_mt_count(struct input_mt_pos *mt)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < MAX_TOUCHES && mt[i].x != 0 && mt[i].y != 0; i++)
>> +		;
> 
> 		/* empty */;
> 
> just to make sure...

Will fix.

> 
>> +
>> +	return i;
>> +}
>> +
>> +static int alps_decode_packet_v7(struct alps_fields *f,
>> +				  unsigned char *p,
>> +				  struct psmouse *psmouse)
>> +{
>> +	unsigned char pkt_id;
>> +
>> +	pkt_id = alps_get_packet_id_v7(p);
>> +	if (pkt_id == V7_PACKET_ID_IDLE)
>> +		return 0;
>> +	if (pkt_id == V7_PACKET_ID_UNKNOWN)
>> +		return -1;
>> +
>> +	alps_get_finger_coordinate_v7(f->mt, p, pkt_id);
>> +
>> +	if (pkt_id == V7_PACKET_ID_TWO || pkt_id == V7_PACKET_ID_MULTI) {
>> +		f->left = (p[0] & 0x80) >> 7;
>> +		f->right = (p[0] & 0x20) >> 5;
>> +		f->middle = (p[0] & 0x10) >> 4;
>> +	}
>> +
>> +	if (pkt_id == V7_PACKET_ID_TWO)
>> +		f->fingers = alps_get_mt_count(f->mt);
>> +	else if (pkt_id == V7_PACKET_ID_MULTI)
>> +		f->fingers = 3 + (p[5] & 0x03);
>> +
>> +	return 0;
>> +}
>> +
>> +static void alps_process_trackstick_packet_v7(struct psmouse *psmouse)
>> +{
>> +	struct alps_data *priv = psmouse->private;
>> +	unsigned char *packet = psmouse->packet;
>> +	struct input_dev *dev2 = priv->dev2;
>> +	int x, y, z, left, right, middle;
>> +
>> +	/*
>> +	 *        b7 b6 b5 b4 b3 b2 b1 b0
>> +	 * Byte0   0  1  0  0  1  0  0  0
>> +	 * Byte1   1  1  *  *  1  M  R  L
>> +	 * Byte2  X7  1 X5 X4 X3 X2 X1 X0
>> +	 * Byte3  Z6  1 Y6 X6  1 Y2 Y1 Y0
>> +	 * Byte4  Y7  0 Y5 Y4 Y3  1  1  0
>> +	 * Byte5 T&P  0 Z5 Z4 Z3 Z2 Z1 Z0
>> +	 * M / R / L: Middle / Right / Left button
>> +	 */
>> +
>> +	x = ((packet[2] & 0xbf)) | ((packet[3] & 0x10) << 2);
>> +	y = (packet[3] & 0x07) | (packet[4] & 0xb8) |
>> +	    ((packet[3] & 0x20) << 1);
>> +	z = (packet[5] & 0x3f) | ((packet[3] & 0x80) >> 1);
>> +
>> +	left = (packet[1] & 0x01);
>> +	right = (packet[1] & 0x02) >> 1;
>> +	middle = (packet[1] & 0x04) >> 2;
>> +
>> +	/* Divide 2 since trackpoint's speed is too fast */
>> +	input_report_rel(dev2, REL_X, (char)x / 2);
>> +	input_report_rel(dev2, REL_Y, -((char)y / 2));
>> +
>> +	input_report_key(dev2, BTN_LEFT, left);
>> +	input_report_key(dev2, BTN_RIGHT, right);
>> +	input_report_key(dev2, BTN_MIDDLE, middle);
>> +
>> +	input_sync(dev2);
>> +}
>> +
>> +static void alps_process_touchpad_packet_v7(struct psmouse *psmouse)
>> +{
>> +	struct alps_data *priv = psmouse->private;
>> +	struct input_dev *dev = psmouse->dev;
>> +	struct alps_fields *f = &priv->f;
>> +
>> +	memset(f, 0, sizeof(*f));
>> +
>> +	if (priv->decode_fields(f, psmouse->packet, psmouse))
>> +		return;
>> +
>> +	alps_report_mt_data(psmouse, alps_get_mt_count(f->mt));
>> +
>> +	input_mt_report_finger_count(dev, f->fingers);
>> +
>> +	input_report_key(dev, BTN_LEFT, f->left);
>> +	input_report_key(dev, BTN_RIGHT, f->right);
>> +	input_report_key(dev, BTN_MIDDLE, f->middle);
>> +
>> +	input_sync(dev);
>> +}
>> +
>> +static void alps_process_packet_v7(struct psmouse *psmouse)
>> +{
>> +	unsigned char *packet = psmouse->packet;
>> +
>> +	if ((packet[0] == 0x48) && ((packet[4] & 0x47) == 0x06))
>> +		alps_process_trackstick_packet_v7(psmouse);
>> +	else
>> +		alps_process_touchpad_packet_v7(psmouse);
>> +}
>> +
>>  static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>>  					unsigned char packet[],
>>  					bool report_buttons)
>> @@ -1009,6 +1181,14 @@ static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
>>  		return PSMOUSE_BAD_DATA;
>>  	}
>>  
>> +	if (priv->proto_version == ALPS_PROTO_V7 &&
>> +			!alps_is_valid_package_v7(psmouse)) {
>> +		psmouse_dbg(psmouse, "refusing packet[%i] = %x\n",
>> +			    psmouse->pktcnt - 1,
>> +			    psmouse->packet[psmouse->pktcnt - 1]);
>> +		return PSMOUSE_BAD_DATA;
>> +	}
>> +
>>  	if (psmouse->pktcnt == psmouse->pktsize) {
>>  		priv->process_packet(psmouse);
>>  		return PSMOUSE_FULL_PACKET;
>> @@ -1121,6 +1301,22 @@ static int alps_rpt_cmd(struct psmouse *psmouse, int init_command,
>>  	return 0;
>>  }
>>  
>> +static int alps_check_valid_firmware_id(unsigned char id[])
> 
> bool
> 
>> +{
>> +	int valid = 1;
> 
> bool; true
> 
>> +
>> +	if (id[0] == 0x73)
>> +		valid = 1;
> 
> true
> 
>> +	else if (id[0] == 0x88) {
>> +		if ((id[1] == 0x07) ||
>> +		    (id[1] == 0x08) ||
>> +		    ((id[1] & 0xf0) == 0xB0))
>> +			valid = 1;
> 
> true
> 
>> +	}
>> +
>> +	return valid;
> 
> Hmmm, does not make sense - it is never false...

Right, if you look at the code it factors out (below)
valid should clearly be initialized to false, good catch.

Or even better, don't have valid at all simply use
"return true" in the if blocks and "return false"
at the end, that's what I'll do for v2.




> 
>> +}
>> +
>>  static int alps_enter_command_mode(struct psmouse *psmouse)
>>  {
>>  	unsigned char param[4];
>> @@ -1130,8 +1326,7 @@ static int alps_enter_command_mode(struct psmouse *psmouse)
>>  		return -1;
>>  	}
>>  
>> -	if ((param[0] != 0x88 || (param[1] != 0x07 && param[1] != 0x08)) &&
>> -	    param[0] != 0x73) {
>> +	if (!alps_check_valid_firmware_id(param)) {
>>  		psmouse_dbg(psmouse,
>>  			    "unknown response while entering command mode\n");
>>  		return -1;
>> @@ -1785,6 +1980,32 @@ static int alps_hw_init_dolphin_v1(struct psmouse *psmouse)
>>  	return 0;
>>  }


Regards,

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

  reply	other threads:[~2014-07-26  8:22 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-09 15:24 [PATCH 00/14] alps: bugfixes, cleanups and new hardware support Hans de Goede
2014-07-09 15:24 ` [PATCH 01/14] alps: Fix rushmore packet decoding Hans de Goede
2014-07-09 15:24 ` [PATCH 02/14] alps: Always report 2 fingers (or more) when receiving mt data on v3 models Hans de Goede
2014-07-09 15:24 ` [PATCH 03/14] alps: process_bitmap: Don't invert the Y-axis on Rushmore Hans de Goede
2014-07-09 15:24 ` [PATCH 04/14] alps: process_bitmap: Add alps_get_bitmap_points() helper function Hans de Goede
2014-07-09 15:24 ` [PATCH 05/14] alps: process_bitmap: Fix counting of high point bits Hans de Goede
2014-07-09 15:24 ` [PATCH 06/14] alps: process_bitmap: Round down when spreading adjescent fingers over 2 points Hans de Goede
2014-07-09 15:24 ` [PATCH 07/14] alps: Use struct input_mt_pos to track coordinates Hans de Goede
2014-07-09 15:24 ` [PATCH 08/14] alps: Use input_mt_assign_slots && input_mt_sync_frame instead of DIY Hans de Goede
2014-07-09 15:24 ` [PATCH 09/14] alps: Use single touch data when v3 mt data contains only one finger Hans de Goede
2014-07-09 15:24 ` [PATCH 10/14] alps: Add an alps_report_semi_mt_data function Hans de Goede
2014-07-09 15:24 ` [PATCH 11/14] alps: Report 2 touches when we've > 2 fingers Hans de Goede
2014-07-09 15:24 ` [PATCH 12/14] alps: Change decode function prototype to return an int Hans de Goede
2014-07-09 15:24 ` [PATCH 13/14] alps: Cache firmware version Hans de Goede
2014-07-09 15:24 ` [PATCH 14/14] alps: Add support for v7 devices Hans de Goede
2014-07-26  5:58   ` Dmitry Torokhov
2014-07-26  8:22     ` Hans de Goede [this message]
2014-07-26  5:59 ` [PATCH 00/14] alps: bugfixes, cleanups and new hardware support Dmitry Torokhov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53D36530.9020408@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=linux-input@vger.kernel.org \
    --cc=yunkang.tang@cn.alps.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).