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@xxxxxxxxxxx>
>>
>> Such as found on the new Toshiba Portégé Z30-A and Z40-A.
>>
>> Signed-off-by: Yunkang Tang <yunkang.tang@xxxxxxxxxxx>
>> [hdegoede@xxxxxxxxxx: Remove softbutton handling, this is done in userspace]
>> [hdegoede@xxxxxxxxxx: Report INPUT_PROP_BUTTONPAD]
>> [hdegoede@xxxxxxxxxx: Do not report fake PRESSURE, reporting BTN_TOUCH is
>> enough]
>> [hdegoede@xxxxxxxxxx: Various cleanups / refactoring]
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
