Hi Henrik-

Thanks again for your review.  The forthcoming PATCH v4 includes the
majority of your change requests, except where noted below (and
considering my email "Subject: SEMI_MT vs. simulated mt")...


On Mon, 2012-12-03 at 08:45 +0100, Henrik Rydberg wrote:

quoted

+		/*
+		 * 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.

-EPARSE

I don't understand the meaning of that comment either.  Perhaps the
original author Dudley Du can explain it.

quoted

+static int cypress_read_vital_statistics(struct psmouse *psmouse)

Why do you call this statistics?

Perhaps "cypress_read_tp_metrics" would be a better name.  Any thoughts
on this, Dudley?

quoted

+/*
+ * reset trackpad device to standard relative mode.
+ * This is also the defalut mode when trackpad powered on.
+ */
+static void cypress_reset(struct psmouse *psmouse)
+{
+	struct cytp_data *cytp = psmouse->private;
+
+	psmouse_reset(psmouse);
+
+	CYTP_SET_MODE_BIT(CYTP_BIT_STANDARD_REL);
+	CYTP_SET_PACKET_SIZE(3);
+
+	cytp->prev_contact_cnt = 0;
+}

I suppose it is, but is it necessary to reset to default mode?

I left this unchanged, as I think you and Dmitry agreed.

quoted

+
+static int cypress_set_input_params(struct input_dev *input,
+				    struct cytp_data *cytp)
+{
+	int ret;
+
+	if (cytp->mode & CYTP_BIT_ABS_MASK) {

It seems the device will always operate in this mode, so please simplify.

I #ifdef'd out the unused relative-mode code, as opposed to deleting it.
Will that be acceptable?

quoted

+		/* Remove HSCROLL bit */
+		if (report_data->contact_cnt == 4)
+			header_byte &= ~(ABS_HSCROLL_BIT);

Why conditionally?

Dudley, can you answer this?  I left this (and all the scroll code)
unchanged.

quoted

+#if 0
+	/* FIXME: cypress_reconnect() never works...
+	 * just let psmouse re-init() us for now.
+	 */
+	psmouse->reconnect = cypress_reconnect;
+#endif

This needs to be removed or fixed, of course.

Per Dudley, this does work in his disconnect/reconnect scenario, so I
re-enabled it.  We will continue to investigate why it fails
(harmlessly) in my suspend/resume scenario.

quoted

+	int tp_max_abs_x;  /* Max X absolution units can be reported. */
+	int tp_max_abs_y;  /* Max Y absolution units can be reported. */

I do not think we are seeking absolution here. :-)

Ha!  Speak for yourself, Henrik!  ;-)  I'm starting to feel like maybe I
should be seeking absolution here!


 -Kamal