Re: [PATCH v4 2/4] input: Cypress PS/2 Trackpad psmouse driver
Hi Henrik-
Thanks yet again, for your review. I very much appreciate your time and
efforts to get the driver closer to acceptance. Dmitry and Dudley, my
thanks to you as well.
Henrik, the forthcoming PATCH v5 includes all of your change requests
from this round. See below for additional notes on those changes.
On Wed, 2012-12-05 at 21:07 +0100, Henrik Rydberg wrote:
> > +#undef CYTP_RELATIVE_SUPPORT /* define to enable unused EV_REL code */
>
> Code inside a local ifdef which is off by default is very rare in the
> kernel, and will likely bitrot. If you want to preserve the
> information, why not add it to the documentation, and remove the code
> here?
For PATCH v5, I have removed all that #ifdef'd relative-mode code.
> > + /*
> > +* send extension command 0xE8 or 0xF3,
> > +* if send extension command failed,
> > +* try to send recovery command to make
> > +* trackpad device return to ready wait command state.
> > +* It alwasy success based on this recovery commands.
> > +*/
>
> This still reads the same, please change the wording of the last sentence.
The comment has been changed to:
/*
* Send extension command byte (0xE8 or 0xF3).
* If sending the command fails, send recovery command
* to make the device return to the ready state.
*/
> > + if (cmd == CYTP_CMD_READ_VITAL_STATISTICS)
> > + psmouse->pktsize = 8;
>
> This still reads statistics, which is a misnomer.
All the "vital_stat{ist}ics" references have been changed to
"tp_metrics".
> > + if (cytp->tp_res_x && cytp->tp_res_y) {
> > + input_abs_set_res(input, ABS_X, cytp->tp_res_x);
> > + input_abs_set_res(input, ABS_Y, cytp->tp_res_y);
> > +
> > + input_abs_set_res(input, ABS_MT_POSITION_X,
> > + cytp->tp_res_x);
> > + input_abs_set_res(input, ABS_MT_POSITION_Y,
> > + cytp->tp_res_y);
> > +
> > + }
>
> If the above block is not executed, the device will not function
> properly. Please return error or check preconditions again.
I have fixed this check (it now checks at the top of the routine, and
returns an error if x or y are insane).
> > + /* Remove HSCROLL bit */
> > + if (report_data->contact_cnt == 4)
> > + header_byte &= ~(ABS_HSCROLL_BIT);
>
> Why should this not be removed when contact_cnt is 5 ?
You're right: That bit should be removed if (contact_cnt & 0x4) ...
But you later pointed out that the vscroll/hscroll code isn't actually
used. So having dropped that code, this clause can actually just be
dropped as well, which I've done.
> > +
> > + if (report_data->contact_cnt <= 0)
> > + return 0;
>
> How can this happen?
It can be == 0 in the palm detection case (never <0). I fixed and moved
that check up higher in the routine and commented it.
> > +#define CYTP_MAX_CONTACTS 2
> > +#define CYTP_MAX_MT_SLOTS 2
>
> Code seems to depend on both being the same, please skip one.
CYTP_MAX_CONTACTS has been dropped.
> > + signed char vscroll;
> > + signed char hscroll;
>
> The above two are only used for debug output.
They've been dropped.
-Kamal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 2/4] input: Cypress PS/2 Trackpad psmouse driver
Hi Kamal,
> From: Dudley Du
>
> Input/mouse driver for Cypress PS/2 Trackpad.
>
> Original code contributed by Dudley Du (Cypress Semiconductor Corporation),
> modified by Kamal Mostafa and Kyle Fazzari.
>
> BugLink: http://launchpad.net/bugs/978807
>
> Signed-off-by: Dudley Du
> Signed-off-by: Kamal Mostafa
> Signed-off-by: Kyle Fazzari
> Signed-off-by: Mario Limonciello
> Signed-off-by: Tim Gardner
> Acked-by: Herton Krzesinski
> ---
> drivers/input/mouse/cypress_ps2.c | 835
> +
> drivers/input/mouse/cypress_ps2.h | 219 ++
> 2 files changed, 1054 insertions(+)
> create mode 100644 drivers/input/mouse/cypress_ps2.c
> create mode 100644 drivers/input/mouse/cypress_ps2.h
>
> diff --git a/drivers/input/mouse/cypress_ps2.c
> b/drivers/input/mouse/cypress_ps2.c
> new file mode 100644
> index 000..fab4d18
> --- /dev/null
> +++ b/drivers/input/mouse/cypress_ps2.c
> @@ -0,0 +1,835 @@
> +/*
> + * Cypress Trackpad PS/2 mouse driver
> + *
> + * Copyright (c) 2012 Cypress Semiconductor Corporation.
> + *
> + * Author:
> + * Dudley Du
> + *
> + * Additional contributors include:
> + * Kamal Mostafa
> + * Kyle Fazzari
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> by
> + * the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "cypress_ps2.h"
> +
> +#undef CYTP_DEBUG_VERBOSE /* define this and DEBUG for more verbose dump */
> +
> +#undef CYTP_RELATIVE_SUPPORT /* define to enable unused EV_REL code */
Code inside a local ifdef which is off by default is very rare in the
kernel, and will likely bitrot. If you want to preserve the
information, why not add it to the documentation, and remove the code
here?
> +
> +#define cytp_dbg(fmt, ...) psmouse_dbg(psmouse, pr_fmt(fmt), ##__VA_ARGS__)
> +
> +
> +static int is_cypress_key(const unsigned char *param)
> +{
> + return (param[0] == CYPRESS_KEY_1) && (param[1] == CYPRESS_KEY_2);
> +}
> +
> +static void cypress_set_packet_size(struct psmouse *psmouse, unsigned int n)
> +{
> + struct cytp_data *cytp = psmouse->private;
> + psmouse->pktsize = cytp->pkt_size = n;
> +}
> +
> +static void cypress_set_abs_rel_mode(struct cytp_data *cytp,
> + unsigned int cytp_mode_bit)
> +{
> + cytp->mode = (cytp->mode & ~CYTP_BIT_ABS_REL_MASK) | cytp_mode_bit;
> +}
> +
> +static const unsigned char cytp_rate[] = {10, 20, 40, 60, 100, 200};
> +static const unsigned char cytp_resolution[] = {0x00, 0x01, 0x02, 0x03};
> +
> +static int cypress_ps2_sendbyte(struct psmouse *psmouse, int value)
> +{
> + struct ps2dev *ps2dev = &psmouse->ps2dev;
> +
> + if (ps2_sendbyte(ps2dev, value & 0xff, CYTP_CMD_TIMEOUT) < 0) {
> + cytp_dbg("send command 0x%02x failed, resp 0x%02x\n",
> + value & 0xff, ps2dev->nak);
> + if (ps2dev->nak == CYTP_PS2_RETRY)
> + return CYTP_PS2_RETRY;
> + else
> + return CYTP_PS2_ERROR;
> + }
> +
> + cytp_dbg("send command 0x%02x success, resp 0xfa\n", value & 0xff);
> +
> + return 0;
> +}
> +
> +static int cypress_ps2_ext_cmd(struct psmouse *psmouse, unsigned short cmd,
> +unsigned char data)
> +{
> + struct ps2dev *ps2dev = &psmouse->ps2dev;
> + int tries = CYTP_PS2_CMD_TRIES;
> + int rc;
> +
> + ps2_begin_command(ps2dev);
> +
> + do {
> + /*
> + * send extension command 0xE8 or 0xF3,
> + * if send extension command failed,
> + * try to send recovery command to make
> + * trackpad device return to ready wait command state.
> + * It alwasy success based on this recovery commands.
> + */
This still reads the same, please change the wording of the last sentence.
> + rc = cypress_ps2_sendbyte(psmouse, cmd & 0xff);
> + if (rc == CYTP_PS2_RETRY) {
> + rc = cypress_ps2_sendbyte(psmouse, 0x00);
> + if (rc == CYTP_PS2_RETRY)
> + rc = cypress_ps2_sendbyte(psmouse, 0x0a);
> + }
> + if (rc == CYTP_PS2_ERROR)
> + continue;
> +
> + rc = cypress_ps2_sendbyte(psmouse, data);
> + if (rc == CYTP_PS2_RETRY)
> + rc = cypress_ps2_sendbyte(psmouse, data);
> + if (rc == CYTP_PS2_ERROR)
> + continue;
> + else
> + break;
> + } while (--tries > 0);
> +
> + ps2_end_command(ps2dev);
> +
> + return rc;
> +}
> +
> +static int cypress_ps2_read_cmd_status(struct psmouse *psmouse,
> +unsigned char cmd,
> +
[PATCH v4 2/4] input: Cypress PS/2 Trackpad psmouse driver
From: Dudley Du
Input/mouse driver for Cypress PS/2 Trackpad.
Original code contributed by Dudley Du (Cypress Semiconductor Corporation),
modified by Kamal Mostafa and Kyle Fazzari.
BugLink: http://launchpad.net/bugs/978807
Signed-off-by: Dudley Du
Signed-off-by: Kamal Mostafa
Signed-off-by: Kyle Fazzari
Signed-off-by: Mario Limonciello
Signed-off-by: Tim Gardner
Acked-by: Herton Krzesinski
---
drivers/input/mouse/cypress_ps2.c | 835 +
drivers/input/mouse/cypress_ps2.h | 219 ++
2 files changed, 1054 insertions(+)
create mode 100644 drivers/input/mouse/cypress_ps2.c
create mode 100644 drivers/input/mouse/cypress_ps2.h
diff --git a/drivers/input/mouse/cypress_ps2.c
b/drivers/input/mouse/cypress_ps2.c
new file mode 100644
index 000..fab4d18
--- /dev/null
+++ b/drivers/input/mouse/cypress_ps2.c
@@ -0,0 +1,835 @@
+/*
+ * Cypress Trackpad PS/2 mouse driver
+ *
+ * Copyright (c) 2012 Cypress Semiconductor Corporation.
+ *
+ * Author:
+ * Dudley Du
+ *
+ * Additional contributors include:
+ * Kamal Mostafa
+ * Kyle Fazzari
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "cypress_ps2.h"
+
+#undef CYTP_DEBUG_VERBOSE /* define this and DEBUG for more verbose dump */
+
+#undef CYTP_RELATIVE_SUPPORT /* define to enable unused EV_REL code */
+
+#define cytp_dbg(fmt, ...) psmouse_dbg(psmouse, pr_fmt(fmt), ##__VA_ARGS__)
+
+
+static int is_cypress_key(const unsigned char *param)
+{
+ return (param[0] == CYPRESS_KEY_1) && (param[1] == CYPRESS_KEY_2);
+}
+
+static void cypress_set_packet_size(struct psmouse *psmouse, unsigned int n)
+{
+ struct cytp_data *cytp = psmouse->private;
+ psmouse->pktsize = cytp->pkt_size = n;
+}
+
+static void cypress_set_abs_rel_mode(struct cytp_data *cytp,
+ unsigned int cytp_mode_bit)
+{
+ cytp->mode = (cytp->mode & ~CYTP_BIT_ABS_REL_MASK) | cytp_mode_bit;
+}
+
+static const unsigned char cytp_rate[] = {10, 20, 40, 60, 100, 200};
+static const unsigned char cytp_resolution[] = {0x00, 0x01, 0x02, 0x03};
+
+static int cypress_ps2_sendbyte(struct psmouse *psmouse, int value)
+{
+ struct ps2dev *ps2dev = &psmouse->ps2dev;
+
+ if (ps2_sendbyte(ps2dev, value & 0xff, CYTP_CMD_TIMEOUT) < 0) {
+ cytp_dbg("send command 0x%02x failed, resp 0x%02x\n",
+value & 0xff, ps2dev->nak);
+ if (ps2dev->nak == CYTP_PS2_RETRY)
+ return CYTP_PS2_RETRY;
+ else
+ return CYTP_PS2_ERROR;
+ }
+
+ cytp_dbg("send command 0x%02x success, resp 0xfa\n", value & 0xff);
+
+ return 0;
+}
+
+static int cypress_ps2_ext_cmd(struct psmouse *psmouse, unsigned short cmd,
+ unsigned char data)
+{
+ struct ps2dev *ps2dev = &psmouse->ps2dev;
+ int tries = CYTP_PS2_CMD_TRIES;
+ int rc;
+
+ ps2_begin_command(ps2dev);
+
+ do {
+ /*
+* send extension command 0xE8 or 0xF3,
+* if send extension command failed,
+* try to send recovery command to make
+* trackpad device return to ready wait command state.
+* It alwasy success based on this recovery commands.
+*/
+ rc = cypress_ps2_sendbyte(psmouse, cmd & 0xff);
+ if (rc == CYTP_PS2_RETRY) {
+ rc = cypress_ps2_sendbyte(psmouse, 0x00);
+ if (rc == CYTP_PS2_RETRY)
+ rc = cypress_ps2_sendbyte(psmouse, 0x0a);
+ }
+ if (rc == CYTP_PS2_ERROR)
+ continue;
+
+ rc = cypress_ps2_sendbyte(psmouse, data);
+ if (rc == CYTP_PS2_RETRY)
+ rc = cypress_ps2_sendbyte(psmouse, data);
+ if (rc == CYTP_PS2_ERROR)
+ continue;
+ else
+ break;
+ } while (--tries > 0);
+
+ ps2_end_command(ps2dev);
+
+ return rc;
+}
+
+static int cypress_ps2_read_cmd_status(struct psmouse *psmouse,
+ unsigned char cmd,
+ unsigned char *param)
+{
+ int i;
+ int rc;
+ struct ps2dev *ps2dev = &psmouse->ps2dev;
+ enum psmouse_state old_state;
+ unsigned char old_pktsize;
+
+ ps2_begin_command(&psmouse->ps2dev);
+
+ old_state = psmouse->state;
+ psmouse->state = PSMOUSE_CMD_MODE;
+ psmouse->pktcnt = 0;
+ old_pktsize = psmouse->pktsize;
+ psmouse->pktsize = 3;
+ if (cmd == CYTP_CMD_READ_VITAL_STATISTICS)
+
