Re: [PATCH v2 2/3] input: Cypress PS/2 Trackpad psmouse driver

Hi Henrik-

Thanks very much for your detailed review...


On Wed, 2012-11-21 at 20:20 +0100, Henrik Rydberg wrote:

Reading the patch, it looks very much like a typical semi-mt device to
me. Any good reason not to handle it that way?

Well, the device does properly report contact-counts up to 5 fingers
(though only two actual contact positions).  If I understand SEMI_MT
correctly (and I might not) using it restricts us to reporting only up
to 2 fingers, which seems undesirable.

That said, my forthcoming PATCH v3 does implement the SEMI_MT /
2-contacts-only method as you requested (I think) -- and includes one
more proposed patch to re-enable >2 contacts.

PATCH v3 also implements the majority of your other suggestions,
including replacing all the custom slot tracking code with
input_mt_assign_slots.  Large swaths of code have been eliminated
(thanks!).

Below are notes regarding the few suggestions which I have not
addressed.

Thanks again for your help, and in advance for your review of PATCH v3.

 -Kamal

quoted

+static int cypress_read_vital_statistics(struct psmouse *psmouse)
+{
[...]
+
+	if (!cytp->tp_max_pressure ||
+	    (cytp->tp_max_pressure < cytp->tp_min_pressure) ||
+	    (!cytp->tp_width || !cytp->tp_high) ||
+	    (!cytp->tp_max_abs_x) ||
+	    (cytp->tp_max_abs_x < cytp->tp_width) ||
+	    (!cytp->tp_max_abs_y) ||
+	    (cytp->tp_max_abs_y < cytp->tp_high))
+		return -1;

Odd parenthesis. Can tp_min_pressure be zero? Don't we trust the
values coming from the device?

I do not know, so I have left this as is.

quoted

+static psmouse_ret_t cypress_validate_byte(struct psmouse *psmouse)
+{
+	int contact_cnt;
+	int index = psmouse->pktcnt - 1;
+	unsigned char *packet = psmouse->packet;
+	struct cytp_data *cytp = psmouse->private;
+
+	if (index < 0 || index > cytp->pkt_size)
+		return PSMOUSE_BAD_DATA;
+
+	if ((index == 0) && ((packet[0] & 0xfc) == 0)) {
+		/* call packet process for reporting finger leave. */
+		cypress_process_packet(psmouse, 1);
+		return PSMOUSE_FULL_PACKET;
+	}

Odd placement given the function's name?

Do you mean the call to cypress_process_packet?  I agree that it is odd.
Left it as is.  :-)

quoted

diff --git a/drivers/input/mouse/cypress_ps2.h b/drivers/input/mouse/cypress_ps2.h

[...]
+
+/* bits of first byte response of E9h-Status Request command. */
+#define RESP_BTN_RIGHT_BIT  0x01
+#define RESP_BTN_MIDDLE_BIT 0x02
+#define RESP_BTN_LEFT_BIT   0x04
+#define RESP_SCALING_BIT    0x10
+#define RESP_ENABLE_BIT     0x20
+#define RESP_REMOTE_BIT     0x40
+#define RESP_SMBUS_BIT      0x80
+
+#define CYTP_MAX_CONTACTS 5
+#define CYTP_MAX_MT_SLOTS 16

Are all these values used?

Some of the #define's are not used, but I'm reluctant to remove them as
they might be useful for later versions of the driver.  I left them as
is, pending your advice.