[PATCH 0/5] Input: alps - cleanup

[PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

This patch series cleanup usage of alps_model_data table.

Pali Rohár (5):
  Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
  Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
  Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
  Input: alps - warn about unsupported ALPS V9 touchpad
  Input: alps - cleanup ALPS_PROTO_V2 detection

 drivers/input/mouse/alps.c |   99 ++++++++++++++++++++++++++------------------
 drivers/input/mouse/alps.h |   10 ++---
 2 files changed, 62 insertions(+), 47 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] Input: alps - move ALPS_PROTO_V6 out of alps_model_data table

[Pali Rohár]

Like for other protocols create alps_v6_protocol_data and use it in
alps_identify() function.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 936f07a..eb0c57b 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -133,7 +133,6 @@ static const struct alps_model_info alps_model_data[] = {
 	/* Dell Latitude E5500, E6400, E6500, Precision M4400 */
 	{ { 0x62, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf,
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },
-	{ { 0x73, 0x00, 0x14 }, 0x00, { ALPS_PROTO_V6, 0xff, 0xff, ALPS_DUALPOINT } },		/* Dell XT2 */
 	{ { 0x73, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },	/* Dell Vostro 1400 */
 	{ { 0x52, 0x01, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff,
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Toshiba Tecra A11-11L */
@@ -152,6 +151,10 @@ static const struct alps_protocol_info alps_v5_protocol_data = {
 	ALPS_PROTO_V5, 0xc8, 0xd8, 0
 };
 
+static const struct alps_protocol_info alps_v6_protocol_data = {
+	ALPS_PROTO_V6, 0xff, 0xff, ALPS_DUALPOINT
+};
+
 static const struct alps_protocol_info alps_v7_protocol_data = {
 	ALPS_PROTO_V7, 0x48, 0x48, ALPS_DUALPOINT
 };
@@ -2756,6 +2759,8 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 		if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
 			   ec[0] == 0x73 && (ec[1] == 0x01 || ec[1] == 0x02)) {
 			protocol = &alps_v5_protocol_data;
+		} else if (e7[0] == 0x73 && e7[1] == 0x00 && e7[2] == 0x14) {
+			protocol = &alps_v6_protocol_data;
 		} else if (ec[0] == 0x88 &&
 			   ((ec[1] & 0xf0) == 0xb0 || (ec[1] & 0xf0) == 0xc0)) {
 			protocol = &alps_v7_protocol_data;
-- 
1.7.9.5

[PATCH 2/5] Input: alps - move ALPS_PROTO_V4 out of alps_model_data table

[Pali Rohár]

Like for other protocols create alps_v4_protocol_data and use it in
alps_identify() function.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index eb0c57b..087fbe5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -136,7 +136,6 @@ static const struct alps_model_info alps_model_data[] = {
 	{ { 0x73, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },	/* Dell Vostro 1400 */
 	{ { 0x52, 0x01, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff,
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Toshiba Tecra A11-11L */
-	{ { 0x73, 0x02, 0x64 }, 0x8a, { ALPS_PROTO_V4, 0x8f, 0x8f, 0 } },
 };
 
 static const struct alps_protocol_info alps_v3_protocol_data = {
@@ -147,6 +146,10 @@ static const struct alps_protocol_info alps_v3_rushmore_data = {
 	ALPS_PROTO_V3_RUSHMORE, 0x8f, 0x8f, ALPS_DUALPOINT
 };
 
+static const struct alps_protocol_info alps_v4_protocol_data = {
+	ALPS_PROTO_V4, 0x8f, 0x8f, 0
+};
+
 static const struct alps_protocol_info alps_v5_protocol_data = {
 	ALPS_PROTO_V5, 0xc8, 0xd8, 0
 };
@@ -2756,7 +2759,10 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 
 	protocol = alps_match_table(e7, ec);
 	if (!protocol) {
-		if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
+		if (e7[0] == 0x73 && e7[1] == 0x02 && e7[2] == 0x64 &&
+			   ec[2] == 0x8a) {
+			protocol = &alps_v4_protocol_data;
+		} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
 			   ec[0] == 0x73 && (ec[1] == 0x01 || ec[1] == 0x02)) {
 			protocol = &alps_v5_protocol_data;
 		} else if (e7[0] == 0x73 && e7[1] == 0x00 && e7[2] == 0x14) {
-- 
1.7.9.5

[PATCH 3/5] Input: alps - move ALPS_PROTO_V1 out of alps_model_data table

[Pali Rohár]

Like for other protocols create alps_v1_protocol_data and use it in
alps_identify() function.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 087fbe5..4e6d76e 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -106,7 +106,6 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
 
 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 */
-	{ { 0x33, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V1, 0x88, 0xf8, 0 } },				/* UMAX-530T */
 	{ { 0x53, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
 	{ { 0x53, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
 	{ { 0x60, 0x03, 0xc8 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },				/* HP ze1115 */
@@ -138,6 +137,10 @@ static const struct alps_model_info alps_model_data[] = {
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Toshiba Tecra A11-11L */
 };
 
+static const struct alps_protocol_info alps_v1_protocol_data = {
+	ALPS_PROTO_V1, 0x88, 0xf8, 0
+};
+
 static const struct alps_protocol_info alps_v3_protocol_data = {
 	ALPS_PROTO_V3, 0x8f, 0x8f, ALPS_DUALPOINT
 };
@@ -2759,7 +2762,9 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 
 	protocol = alps_match_table(e7, ec);
 	if (!protocol) {
-		if (e7[0] == 0x73 && e7[1] == 0x02 && e7[2] == 0x64 &&
+		if (e7[0] == 0x33 && e7[1] == 0x02 && e7[2] == 0x0a) {
+			protocol = &alps_v1_protocol_data;
+		} else if (e7[0] == 0x73 && e7[1] == 0x02 && e7[2] == 0x64 &&
 			   ec[2] == 0x8a) {
 			protocol = &alps_v4_protocol_data;
 		} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
-- 
1.7.9.5

[PATCH 4/5] Input: alps - warn about unsupported ALPS V9 touchpad

[Pali Rohár]

Support for devices with ALPS_PROTO_V9 is not implemented yet but we can
detect these alps touchpads and warn users about it.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |    9 +++++++++
 drivers/input/mouse/alps.h |    1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 4e6d76e..3618320 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -169,6 +169,10 @@ static const struct alps_protocol_info alps_v8_protocol_data = {
 	ALPS_PROTO_V8, 0x18, 0x18, 0
 };
 
+static const struct alps_protocol_info alps_v9_protocol_data = {
+	ALPS_PROTO_V9, 0xc8, 0xc8, 0
+};
+
 /*
  * Some v2 models report the stick buttons in separate bits
  */
@@ -2783,6 +2787,11 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 		} else if (e7[0] == 0x73 && e7[1] == 0x03 &&
 			   e7[2] == 0x14 && ec[1] == 0x02) {
 			protocol = &alps_v8_protocol_data;
+		} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0xc8) {
+			protocol = &alps_v9_protocol_data;
+			psmouse_warn(psmouse,
+				     "Unsupported ALPS V9 touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
+			return -EINVAL;
 		} else {
 			psmouse_dbg(psmouse,
 				    "Likely not an ALPS touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index d37f814..fd0a359 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -23,6 +23,7 @@
 #define ALPS_PROTO_V6		0x600
 #define ALPS_PROTO_V7		0x700	/* t3btl t4s */
 #define ALPS_PROTO_V8		0x800	/* SS4btl SS4s */
+#define ALPS_PROTO_V9		0x900	/* ss3btl */
 
 #define MAX_TOUCHES	4
 
-- 
1.7.9.5

[PATCH 5/5] Input: alps - cleanup ALPS_PROTO_V2 detection

[Pali Rohár]

Table alps_model_data is now used only for ALPS_PROTO_V2, so add "v2"
indentifier to all relevant functions/variables/structures. Sort all
devices by signature and remove command_mode_resp which is not used
anymore.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |   66 ++++++++++++++++++++------------------------
 drivers/input/mouse/alps.h |    9 ++----
 2 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 3618320..7dc49c1e 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -104,37 +104,35 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
 #define ALPS_STICK_BITS		0x100	/* separate stick button bits */
 #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 */
-	{ { 0x53, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x53, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x60, 0x03, 0xc8 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },				/* HP ze1115 */
-	{ { 0x63, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x63, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x63, 0x02, 0x28 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },		/* Fujitsu Siemens S6010 */
-	{ { 0x63, 0x02, 0x3c }, 0x00, { ALPS_PROTO_V2, 0x8f, 0x8f, ALPS_WHEEL } },		/* Toshiba Satellite S2400-103 */
-	{ { 0x63, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xef, 0xef, ALPS_FW_BK_1 } },		/* NEC Versa L320 */
-	{ { 0x63, 0x02, 0x64 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x63, 0x03, 0xc8 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D800 */
-	{ { 0x73, 0x00, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_DUALPOINT } },		/* ThinkPad R61 8918-5QG */
-	{ { 0x73, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x73, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },		/* Ahtec Laptop */
-
+static const struct alps_v2_model_info alps_v2_model_data[] = {
 	/*
 	 * XXX This entry is suspicious. First byte has zero lower nibble,
 	 * which is what a normal mouse would report. Also, the value 0x0e
 	 * isn't valid per PS/2 spec.
 	 */
-	{ { 0x20, 0x02, 0x0e }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
-
-	{ { 0x22, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
-	{ { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D600 */
-	/* Dell Latitude E5500, E6400, E6500, Precision M4400 */
-	{ { 0x62, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf,
-		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },
-	{ { 0x73, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },	/* Dell Vostro 1400 */
-	{ { 0x52, 0x01, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff,
+	{ { 0x20, 0x02, 0x0e }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
+
+	{ { 0x22, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
+	{ { 0x22, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D600 */
+	{ { 0x32, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Toshiba Salellite Pro M10 */
+	{ { 0x52, 0x01, 0x14 }, { ALPS_PROTO_V2, 0xff, 0xff,
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Toshiba Tecra A11-11L */
+	{ { 0x53, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x53, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x60, 0x03, 0xc8 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },				/* HP ze1115 */
+	{ { 0x62, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xcf, 0xcf,
+		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Dell Latitude E5500, E6400, E6500, Precision M4400 */
+	{ { 0x63, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x63, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x63, 0x02, 0x28 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },			/* Fujitsu Siemens S6010 */
+	{ { 0x63, 0x02, 0x3c }, { ALPS_PROTO_V2, 0x8f, 0x8f, ALPS_WHEEL } },			/* Toshiba Satellite S2400-103 */
+	{ { 0x63, 0x02, 0x50 }, { ALPS_PROTO_V2, 0xef, 0xef, ALPS_FW_BK_1 } },			/* NEC Versa L320 */
+	{ { 0x63, 0x02, 0x64 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x63, 0x03, 0xc8 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D800 */
+	{ { 0x73, 0x00, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_DUALPOINT } },		/* ThinkPad R61 8918-5QG */
+	{ { 0x73, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x73, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },			/* Ahtec Laptop */
+	{ { 0x73, 0x02, 0x50 }, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },		/* Dell Vostro 1400 */
 };
 
 static const struct alps_protocol_info alps_v1_protocol_data = {
@@ -2714,21 +2712,17 @@ static int alps_set_protocol(struct psmouse *psmouse,
 	return 0;
 }
 
-static const struct alps_protocol_info *alps_match_table(unsigned char *e7,
-							 unsigned char *ec)
+static const struct alps_protocol_info *alps_v2_match_table(unsigned char *e7,
+							    unsigned char *ec)
 {
-	const struct alps_model_info *model;
+	const struct alps_v2_model_info *model;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(alps_model_data); i++) {
-		model = &alps_model_data[i];
-
-		if (!memcmp(e7, model->signature, sizeof(model->signature)) &&
-		    (!model->command_mode_resp ||
-		     model->command_mode_resp == ec[2])) {
+	for (i = 0; i < ARRAY_SIZE(alps_v2_model_data); i++) {
+		model = &alps_v2_model_data[i];
 
+		if (!memcmp(e7, model->signature, sizeof(model->signature)))
 			return &model->protocol_info;
-		}
 	}
 
 	return NULL;
@@ -2764,7 +2758,7 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 	    alps_exit_command_mode(psmouse))
 		return -EIO;
 
-	protocol = alps_match_table(e7, ec);
+	protocol = alps_v2_match_table(e7, ec);
 	if (!protocol) {
 		if (e7[0] == 0x33 && e7[1] == 0x02 && e7[2] == 0x0a) {
 			protocol = &alps_v1_protocol_data;
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index fd0a359..8b56169 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -139,21 +139,16 @@ struct alps_protocol_info {
 };
 
 /**
- * struct alps_model_info - touchpad ID table
+ * struct alps_v2_model_info - touchpad ID table
  * @signature: E7 response string to match.
- * @command_mode_resp: For V3/V4 touchpads, the final byte of the EC response
- *   (aka command mode response) identifies the firmware minor version.  This
- *   can be used to distinguish different hardware models which are not
- *   uniquely identifiable through their E7 responses.
  * @protocol_info: information about protcol used by the device.
  *
  * Many (but not all) ALPS touchpads can be identified by looking at the
  * values returned in the "E7 report" and/or the "EC report."  This table
  * lists a number of such touchpads.
  */
-struct alps_model_info {
+struct alps_v2_model_info {
 	u8 signature[3];
-	u8 command_mode_resp;
 	struct alps_protocol_info protocol_info;
 };
 
-- 
1.7.9.5

Re: [PATCH 0/5] Input: alps - cleanup

[Hans de Goede]

Hi,

On 06-06-16 13:23, Pali Rohár wrote:
> This patch series cleanup usage of alps_model_data table.
>
> Pali Rohár (5):
>   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
>   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
>   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
>   Input: alps - warn about unsupported ALPS V9 touchpad
>   Input: alps - cleanup ALPS_PROTO_V2 detection
>
>  drivers/input/mouse/alps.c |   99 ++++++++++++++++++++++++++------------------
>  drivers/input/mouse/alps.h |   10 ++---
>  2 files changed, 62 insertions(+), 47 deletions(-)

Series looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks & Regards,

Hans

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 944 bytes --]

On Monday 06 June 2016 13:32:31 Hans de Goede wrote:
> Hi,
> 
> On 06-06-16 13:23, Pali Rohár wrote:
> > This patch series cleanup usage of alps_model_data table.
> > 
> > Pali Rohár (5):
> >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
> >   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
> >   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
> >   Input: alps - warn about unsupported ALPS V9 touchpad
> >   Input: alps - cleanup ALPS_PROTO_V2 detection
> >  
> >  drivers/input/mouse/alps.c |   99
> >  ++++++++++++++++++++++++++------------------
> >  drivers/input/mouse/alps.h |   10 ++---
> >  2 files changed, 62 insertions(+), 47 deletions(-)
> 
> Series looks good to me:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks & Regards,
> 
> Hans

Dmitry, can you take this series? Or is there some issue?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 718 bytes --]

On Monday 06 June 2016 13:23:56 Pali Rohár wrote:
> This patch series cleanup usage of alps_model_data table.
> 
> Pali Rohár (5):
>   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
>   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
>   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
>   Input: alps - warn about unsupported ALPS V9 touchpad
>   Input: alps - cleanup ALPS_PROTO_V2 detection
> 
>  drivers/input/mouse/alps.c |   99
> ++++++++++++++++++++++++++------------------
> drivers/input/mouse/alps.h |   10 ++---
>  2 files changed, 62 insertions(+), 47 deletions(-)

Dmitry, can you look at this series?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/5] Input: alps - cleanup

[Dmitry Torokhov]

Hi Pali,

On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> This patch series cleanup usage of alps_model_data table.
> 
> Pali Rohár (5):
>   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
>   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
>   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
>   Input: alps - warn about unsupported ALPS V9 touchpad
>   Input: alps - cleanup ALPS_PROTO_V2 detection

Frankly, I do not quite like this series. The rule of thumb we had: if
we can use e7 data to identify the device it should go into table,
if we need to have more elaborate logic - then implement it in
__alps_indentify(). I would understand if we got rid of the table
completely, but we didn't.

I think the patch removing ALPS_PROTO_V4 and subsequent patch removing
command_mode_resp from alps_model_info are good, the rest are not so
much.

Thanks.

-- 
Dmitry

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> Hi Pali,
> 
> On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > This patch series cleanup usage of alps_model_data table.
> > 
> > Pali Rohár (5):
> >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
> >   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
> >   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
> >   Input: alps - warn about unsupported ALPS V9 touchpad
> >   Input: alps - cleanup ALPS_PROTO_V2 detection
> 
> Frankly, I do not quite like this series. The rule of thumb we had: if
> we can use e7 data to identify the device it should go into table,
> if we need to have more elaborate logic - then implement it in
> __alps_indentify(). I would understand if we got rid of the table
> completely, but we didn't.

Hans and me agreed that alps_model_data array is for old touchpads
defined as quirks table. So in this patch series I'm trying to eliminate
using that array. And it is possible for V1, V4 and V6 touchpads because
each protocol has only one entry in table. And last user is just V2
protocol which is I think better...

So this is my motivation for this patch series.

> I think the patch removing ALPS_PROTO_V4 and subsequent patch removing
> command_mode_resp from alps_model_info are good, the rest are not so
> much.
> 
> Thanks.
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > Hi Pali,
> > 
> > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > This patch series cleanup usage of alps_model_data table.
> > > 
> > > Pali Rohár (5):
> > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
> > >   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
> > >   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
> > >   Input: alps - warn about unsupported ALPS V9 touchpad
> > >   Input: alps - cleanup ALPS_PROTO_V2 detection
> > 
> > Frankly, I do not quite like this series. The rule of thumb we had: if
> > we can use e7 data to identify the device it should go into table,
> > if we need to have more elaborate logic - then implement it in
> > __alps_indentify(). I would understand if we got rid of the table
> > completely, but we didn't.
> 
> Hans and me agreed that alps_model_data array is for old touchpads
> defined as quirks table. So in this patch series I'm trying to eliminate
> using that array. And it is possible for V1, V4 and V6 touchpads because
> each protocol has only one entry in table. And last user is just V2
> protocol which is I think better...
> 
> So this is my motivation for this patch series.

Any suggestion how to rework it? And any agreement if we should remove
V1/V4/V6 from alps_model_date or let it stay here?

> > I think the patch removing ALPS_PROTO_V4 and subsequent patch removing
> > command_mode_resp from alps_model_info are good, the rest are not so
> > much.
> > 
> > Thanks.
> > 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/5] Input: alps - cleanup

[Dmitry Torokhov]

On Thu, Jul 07, 2016 at 01:41:01PM +0200, Pali Rohár wrote:
> On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> > On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > > Hi Pali,
> > > 
> > > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > > This patch series cleanup usage of alps_model_data table.
> > > > 
> > > > Pali Rohár (5):
> > > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data table
> > > >   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
> > > >   Input: alps - move ALPS_PROTO_V1 out of alps_model_data table
> > > >   Input: alps - warn about unsupported ALPS V9 touchpad
> > > >   Input: alps - cleanup ALPS_PROTO_V2 detection
> > > 
> > > Frankly, I do not quite like this series. The rule of thumb we had: if
> > > we can use e7 data to identify the device it should go into table,
> > > if we need to have more elaborate logic - then implement it in
> > > __alps_indentify(). I would understand if we got rid of the table
> > > completely, but we didn't.
> > 
> > Hans and me agreed that alps_model_data array is for old touchpads
> > defined as quirks table. So in this patch series I'm trying to eliminate
> > using that array. And it is possible for V1, V4 and V6 touchpads because
> > each protocol has only one entry in table. And last user is just V2
> > protocol which is I think better...
> > 
> > So this is my motivation for this patch series.
> 
> Any suggestion how to rework it? And any agreement if we should remove
> V1/V4/V6 from alps_model_date or let it stay here?

As I mentioned below I am happy with removing ALPS_PROTO_V4 and
subsequently command_mode_resp from alps_model_info, while leaving the
rest in the table.

Thanks.

> 
> > > I think the patch removing ALPS_PROTO_V4 and subsequent patch removing
> > > command_mode_resp from alps_model_info are good, the rest are not so
> > > much.
> > > 
> > > Thanks.
> > > 
> > 
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

-- 
Dmitry

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2668 bytes --]

On Friday 08 July 2016 23:37:54 Dmitry Torokhov wrote:
> On Thu, Jul 07, 2016 at 01:41:01PM +0200, Pali Rohár wrote:
> > On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> > > On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > > > Hi Pali,
> > > > 
> > > > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > > > This patch series cleanup usage of alps_model_data table.
> > > > > 
> > > > > Pali Rohár (5):
> > > > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data
> > > > >   table Input: alps - move ALPS_PROTO_V4 out of
> > > > >   alps_model_data table Input: alps - move ALPS_PROTO_V1 out
> > > > >   of alps_model_data table Input: alps - warn about
> > > > >   unsupported ALPS V9 touchpad Input: alps - cleanup
> > > > >   ALPS_PROTO_V2 detection
> > > > 
> > > > Frankly, I do not quite like this series. The rule of thumb we
> > > > had: if we can use e7 data to identify the device it should go
> > > > into table, if we need to have more elaborate logic - then
> > > > implement it in __alps_indentify(). I would understand if we
> > > > got rid of the table completely, but we didn't.
> > > 
> > > Hans and me agreed that alps_model_data array is for old
> > > touchpads defined as quirks table. So in this patch series I'm
> > > trying to eliminate using that array. And it is possible for V1,
> > > V4 and V6 touchpads because each protocol has only one entry in
> > > table. And last user is just V2 protocol which is I think
> > > better...
> > > 
> > > So this is my motivation for this patch series.
> > 
> > Any suggestion how to rework it? And any agreement if we should
> > remove V1/V4/V6 from alps_model_date or let it stay here?
> 
> As I mentioned below I am happy with removing ALPS_PROTO_V4 and
> subsequently command_mode_resp from alps_model_info, while leaving
> the rest in the table.

Now I'm not fully understand what you mean. This patch series does not 
remove ALPS_PROTO_V4 support. Just move ALPS_PROTO_V4 out of 
alps_model_info table structure (same as for V1 and V6). Field 
command_mode_resp is removed from alps_model_info, but that can be done 
only because command_mode_resp is used by ALPS_PROTO_V4 (which is moved 
out of alps_model_info).

So I do not understand why do you think moving ALPS_PROTO_V4 out of 
alps_model_info is good, but ALPS_PROTO_V6 or ALPS_PROTO_V1 not.

> Thanks.
> 
> > > > I think the patch removing ALPS_PROTO_V4 and subsequent patch
> > > > removing command_mode_resp from alps_model_info are good, the
> > > > rest are not so much.
> > > > 
> > > > Thanks.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

On Saturday 09 July 2016 11:58:03 Pali Rohár wrote:
> On Friday 08 July 2016 23:37:54 Dmitry Torokhov wrote:
> > On Thu, Jul 07, 2016 at 01:41:01PM +0200, Pali Rohár wrote:
> > > On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> > > > On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > > > > This patch series cleanup usage of alps_model_data table.
> > > > > > 
> > > > > > Pali Rohár (5):
> > > > > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data
> > > > > >   table Input: alps - move ALPS_PROTO_V4 out of
> > > > > >   alps_model_data table Input: alps - move ALPS_PROTO_V1 out
> > > > > >   of alps_model_data table Input: alps - warn about
> > > > > >   unsupported ALPS V9 touchpad Input: alps - cleanup
> > > > > >   ALPS_PROTO_V2 detection
> > > > > 
> > > > > Frankly, I do not quite like this series. The rule of thumb we
> > > > > had: if we can use e7 data to identify the device it should go
> > > > > into table, if we need to have more elaborate logic - then
> > > > > implement it in __alps_indentify(). I would understand if we
> > > > > got rid of the table completely, but we didn't.
> > > > 
> > > > Hans and me agreed that alps_model_data array is for old
> > > > touchpads defined as quirks table. So in this patch series I'm
> > > > trying to eliminate using that array. And it is possible for V1,
> > > > V4 and V6 touchpads because each protocol has only one entry in
> > > > table. And last user is just V2 protocol which is I think
> > > > better...
> > > > 
> > > > So this is my motivation for this patch series.
> > > 
> > > Any suggestion how to rework it? And any agreement if we should
> > > remove V1/V4/V6 from alps_model_date or let it stay here?
> > 
> > As I mentioned below I am happy with removing ALPS_PROTO_V4 and
> > subsequently command_mode_resp from alps_model_info, while leaving
> > the rest in the table.
> 
> Now I'm not fully understand what you mean. This patch series does not 
> remove ALPS_PROTO_V4 support. Just move ALPS_PROTO_V4 out of 
> alps_model_info table structure (same as for V1 and V6). Field 
> command_mode_resp is removed from alps_model_info, but that can be done 
> only because command_mode_resp is used by ALPS_PROTO_V4 (which is moved 
> out of alps_model_info).
> 
> So I do not understand why do you think moving ALPS_PROTO_V4 out of 
> alps_model_info is good, but ALPS_PROTO_V6 or ALPS_PROTO_V1 not.

Hi Dmitry! If you do not agree with my changes, please can you let me
know what is needed to rework or change? Thanks.

> > Thanks.
> > 
> > > > > I think the patch removing ALPS_PROTO_V4 and subsequent patch
> > > > > removing command_mode_resp from alps_model_info are good, the
> > > > > rest are not so much.
> > > > > 
> > > > > Thanks.
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3039 bytes --]

On Tuesday 09 August 2016 09:56:07 Pali Rohár wrote:
> On Saturday 09 July 2016 11:58:03 Pali Rohár wrote:
> > On Friday 08 July 2016 23:37:54 Dmitry Torokhov wrote:
> > > On Thu, Jul 07, 2016 at 01:41:01PM +0200, Pali Rohár wrote:
> > > > On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> > > > > On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > > > > > Hi Pali,
> > > > > > 
> > > > > > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > > > > > This patch series cleanup usage of alps_model_data table.
> > > > > > > 
> > > > > > > Pali Rohár (5):
> > > > > > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data
> > > > > > >   table Input: alps - move ALPS_PROTO_V4 out of
> > > > > > >   alps_model_data table Input: alps - move ALPS_PROTO_V1
> > > > > > >   out of alps_model_data table Input: alps - warn about
> > > > > > >   unsupported ALPS V9 touchpad Input: alps - cleanup
> > > > > > >   ALPS_PROTO_V2 detection
> > > > > > 
> > > > > > Frankly, I do not quite like this series. The rule of thumb
> > > > > > we had: if we can use e7 data to identify the device it
> > > > > > should go into table, if we need to have more elaborate
> > > > > > logic - then implement it in __alps_indentify(). I would
> > > > > > understand if we got rid of the table completely, but we
> > > > > > didn't.
> > > > > 
> > > > > Hans and me agreed that alps_model_data array is for old
> > > > > touchpads defined as quirks table. So in this patch series
> > > > > I'm trying to eliminate using that array. And it is possible
> > > > > for V1, V4 and V6 touchpads because each protocol has only
> > > > > one entry in table. And last user is just V2 protocol which
> > > > > is I think better...
> > > > > 
> > > > > So this is my motivation for this patch series.
> > > > 
> > > > Any suggestion how to rework it? And any agreement if we should
> > > > remove V1/V4/V6 from alps_model_date or let it stay here?
> > > 
> > > As I mentioned below I am happy with removing ALPS_PROTO_V4 and
> > > subsequently command_mode_resp from alps_model_info, while
> > > leaving the rest in the table.
> > 
> > Now I'm not fully understand what you mean. This patch series does
> > not remove ALPS_PROTO_V4 support. Just move ALPS_PROTO_V4 out of
> > alps_model_info table structure (same as for V1 and V6). Field
> > command_mode_resp is removed from alps_model_info, but that can be
> > done only because command_mode_resp is used by ALPS_PROTO_V4
> > (which is moved out of alps_model_info).
> > 
> > So I do not understand why do you think moving ALPS_PROTO_V4 out of
> > alps_model_info is good, but ALPS_PROTO_V6 or ALPS_PROTO_V1 not.
> 
> Hi Dmitry! If you do not agree with my changes, please can you let me
> know what is needed to rework or change? Thanks.

Hi! Can you clarify above? Or tell me what is wrong?

Hans already acked this series, but if there are some problems I can try 
to fix them...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/5] Input: alps - cleanup

[Pali Rohár]

On Wednesday 14 September 2016 23:02:08 Pali Rohár wrote:
> On Tuesday 09 August 2016 09:56:07 Pali Rohár wrote:
> > On Saturday 09 July 2016 11:58:03 Pali Rohár wrote:
> > > On Friday 08 July 2016 23:37:54 Dmitry Torokhov wrote:
> > > > On Thu, Jul 07, 2016 at 01:41:01PM +0200, Pali Rohár wrote:
> > > > > On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> > > > > > On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > > > > > > Hi Pali,
> > > > > > > 
> > > > > > > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > > > > > > This patch series cleanup usage of alps_model_data table.
> > > > > > > > 
> > > > > > > > Pali Rohár (5):
> > > > > > > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data
> > > > > > > >   table Input: alps - move ALPS_PROTO_V4 out of
> > > > > > > >   alps_model_data table Input: alps - move ALPS_PROTO_V1
> > > > > > > >   out of alps_model_data table Input: alps - warn about
> > > > > > > >   unsupported ALPS V9 touchpad Input: alps - cleanup
> > > > > > > >   ALPS_PROTO_V2 detection
> > > > > > > 
> > > > > > > Frankly, I do not quite like this series. The rule of thumb
> > > > > > > we had: if we can use e7 data to identify the device it
> > > > > > > should go into table, if we need to have more elaborate
> > > > > > > logic - then implement it in __alps_indentify(). I would
> > > > > > > understand if we got rid of the table completely, but we
> > > > > > > didn't.
> > > > > > 
> > > > > > Hans and me agreed that alps_model_data array is for old
> > > > > > touchpads defined as quirks table. So in this patch series
> > > > > > I'm trying to eliminate using that array. And it is possible
> > > > > > for V1, V4 and V6 touchpads because each protocol has only
> > > > > > one entry in table. And last user is just V2 protocol which
> > > > > > is I think better...
> > > > > > 
> > > > > > So this is my motivation for this patch series.
> > > > > 
> > > > > Any suggestion how to rework it? And any agreement if we should
> > > > > remove V1/V4/V6 from alps_model_date or let it stay here?
> > > > 
> > > > As I mentioned below I am happy with removing ALPS_PROTO_V4 and
> > > > subsequently command_mode_resp from alps_model_info, while
> > > > leaving the rest in the table.
> > > 
> > > Now I'm not fully understand what you mean. This patch series does
> > > not remove ALPS_PROTO_V4 support. Just move ALPS_PROTO_V4 out of
> > > alps_model_info table structure (same as for V1 and V6). Field
> > > command_mode_resp is removed from alps_model_info, but that can be
> > > done only because command_mode_resp is used by ALPS_PROTO_V4
> > > (which is moved out of alps_model_info).
> > > 
> > > So I do not understand why do you think moving ALPS_PROTO_V4 out of
> > > alps_model_info is good, but ALPS_PROTO_V6 or ALPS_PROTO_V1 not.
> > 
> > Hi Dmitry! If you do not agree with my changes, please can you let me
> > know what is needed to rework or change? Thanks.
> 
> Hi! Can you clarify above? Or tell me what is wrong?
> 
> Hans already acked this series, but if there are some problems I can try 
> to fix them...
> 

PING

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 0/5] Input: alps - cleanup

[Dmitry Torokhov]

On Sat, Jul 09, 2016 at 11:58:03AM +0200, Pali Rohár wrote:
> On Friday 08 July 2016 23:37:54 Dmitry Torokhov wrote:
> > On Thu, Jul 07, 2016 at 01:41:01PM +0200, Pali Rohár wrote:
> > > On Tuesday 21 June 2016 13:27:30 Pali Rohár wrote:
> > > > On Monday 20 June 2016 17:31:13 Dmitry Torokhov wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On Mon, Jun 06, 2016 at 01:23:56PM +0200, Pali Rohár wrote:
> > > > > > This patch series cleanup usage of alps_model_data table.
> > > > > > 
> > > > > > Pali Rohár (5):
> > > > > >   Input: alps - move ALPS_PROTO_V6 out of alps_model_data
> > > > > >   table Input: alps - move ALPS_PROTO_V4 out of
> > > > > >   alps_model_data table Input: alps - move ALPS_PROTO_V1 out
> > > > > >   of alps_model_data table Input: alps - warn about
> > > > > >   unsupported ALPS V9 touchpad Input: alps - cleanup
> > > > > >   ALPS_PROTO_V2 detection
> > > > > 
> > > > > Frankly, I do not quite like this series. The rule of thumb we
> > > > > had: if we can use e7 data to identify the device it should go
> > > > > into table, if we need to have more elaborate logic - then
> > > > > implement it in __alps_indentify(). I would understand if we
> > > > > got rid of the table completely, but we didn't.
> > > > 
> > > > Hans and me agreed that alps_model_data array is for old
> > > > touchpads defined as quirks table. So in this patch series I'm
> > > > trying to eliminate using that array. And it is possible for V1,
> > > > V4 and V6 touchpads because each protocol has only one entry in
> > > > table. And last user is just V2 protocol which is I think
> > > > better...
> > > > 
> > > > So this is my motivation for this patch series.
> > > 
> > > Any suggestion how to rework it? And any agreement if we should
> > > remove V1/V4/V6 from alps_model_date or let it stay here?
> > 
> > As I mentioned below I am happy with removing ALPS_PROTO_V4 and
> > subsequently command_mode_resp from alps_model_info, while leaving
> > the rest in the table.
> 
> Now I'm not fully understand what you mean. This patch series does not 
> remove ALPS_PROTO_V4 support. Just move ALPS_PROTO_V4 out of 
> alps_model_info table structure (same as for V1 and V6). Field 
> command_mode_resp is removed from alps_model_info, but that can be done 
> only because command_mode_resp is used by ALPS_PROTO_V4 (which is moved 
> out of alps_model_info).
> 
> So I do not understand why do you think moving ALPS_PROTO_V4 out of 
> alps_model_info is good, but ALPS_PROTO_V6 or ALPS_PROTO_V1 not.

Quoting from my earlier email (you can see it a few lines above as well:

> > > > > Frankly, I do not quite like this series. The rule of thumb we 
> > > > > had: if we can use e7 data to identify the device it should go 
> > > > > into table, if we need to have more elaborate logic - then
> > > > > implement it in __alps_indentify(). I would understand if we
> > > > > got rid of the table completely, but we didn't.

I am OK with moving ALPS_PROTO_V4 out of the alps_model_data table
because it needs not only e7 response data, but also ec data, so moving
this protocol out of alps_model_data and writing custom comparison also
allows us to drop command_mode_resp field from alps_model_info
structure.

ALPS_PROTO_V1 and ALPS_PROTO_V6, just like ALPS_PROTO_V2, only need e7
response data. That is why I do not see the benefit of removing them
form the alps_model_data table and open-coding the comparison.

I hope this clears my position.

Thanks.

-- 
Dmitry

[PATCH v2 0/3] Input: alps - cleanup

[Pali Rohár]

This patch series cleanup usage of alps_model_data table.

Changes since v1: Patches for ALPS_PROTO_V6 and ALPS_PROTO_V1 were
dropped and support for those touchpad are still in alps_model_data.

Pali Rohár (3):
  Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
  Input: alps - warn about unsupported ALPS V9 touchpad
  Input: alps - cleanup alps_model_data

 drivers/input/mouse/alps.c |   75 +++++++++++++++++++++++++-------------------
 drivers/input/mouse/alps.h |    6 +---
 2 files changed, 43 insertions(+), 38 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] Input: alps - move ALPS_PROTO_V4 out of alps_model_data table

[Pali Rohár]

Like for other protocols create alps_v4_protocol_data and use it in
alps_identify() function.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 72b28eb..f36de82 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -138,7 +138,6 @@
 	{ { 0x73, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },	/* Dell Vostro 1400 */
 	{ { 0x52, 0x01, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff,
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Toshiba Tecra A11-11L */
-	{ { 0x73, 0x02, 0x64 }, 0x8a, { ALPS_PROTO_V4, 0x8f, 0x8f, 0 } },
 };
 
 static const struct alps_protocol_info alps_v3_protocol_data = {
@@ -149,6 +148,10 @@
 	ALPS_PROTO_V3_RUSHMORE, 0x8f, 0x8f, ALPS_DUALPOINT
 };
 
+static const struct alps_protocol_info alps_v4_protocol_data = {
+	ALPS_PROTO_V4, 0x8f, 0x8f, 0
+};
+
 static const struct alps_protocol_info alps_v5_protocol_data = {
 	ALPS_PROTO_V5, 0xc8, 0xd8, 0
 };
@@ -2815,7 +2818,10 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 
 	protocol = alps_match_table(e7, ec);
 	if (!protocol) {
-		if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
+		if (e7[0] == 0x73 && e7[1] == 0x02 && e7[2] == 0x64 &&
+			   ec[2] == 0x8a) {
+			protocol = &alps_v4_protocol_data;
+		} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0x50 &&
 			   ec[0] == 0x73 && (ec[1] == 0x01 || ec[1] == 0x02)) {
 			protocol = &alps_v5_protocol_data;
 		} else if (ec[0] == 0x88 &&
-- 
1.7.9.5

[PATCH v2 2/3] Input: alps - warn about unsupported ALPS V9 touchpad

[Pali Rohár]

Support for devices with ALPS_PROTO_V9 is not implemented yet but we can
detect these alps touchpads and warn users about it.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |    9 +++++++++
 drivers/input/mouse/alps.h |    1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index f36de82..9908f18 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -164,6 +164,10 @@
 	ALPS_PROTO_V8, 0x18, 0x18, 0
 };
 
+static const struct alps_protocol_info alps_v9_protocol_data = {
+	ALPS_PROTO_V9, 0xc8, 0xc8, 0
+};
+
 /*
  * Some v2 models report the stick buttons in separate bits
  */
@@ -2838,6 +2842,11 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 		} else if (e7[0] == 0x73 && e7[1] == 0x03 &&
 			   e7[2] == 0x28 && ec[1] == 0x01) {
 			protocol = &alps_v8_protocol_data;
+		} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0xc8) {
+			protocol = &alps_v9_protocol_data;
+			psmouse_warn(psmouse,
+				     "Unsupported ALPS V9 touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
+			return -EINVAL;
 		} else {
 			psmouse_dbg(psmouse,
 				    "Likely not an ALPS touchpad: E7=%3ph, EC=%3ph\n", e7, ec);
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 6d279aa..54f9f44 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -23,6 +23,7 @@
 #define ALPS_PROTO_V6		0x600
 #define ALPS_PROTO_V7		0x700	/* t3btl t4s */
 #define ALPS_PROTO_V8		0x800	/* SS4btl SS4s */
+#define ALPS_PROTO_V9		0x900	/* ss3btl */
 
 #define MAX_TOUCHES	4
 
-- 
1.7.9.5

[PATCH v2 3/3] Input: alps - cleanup alps_model_data

[Pali Rohár]

Sort all devices in alps_model_data by signature and remove
command_mode_resp which is not used anymore.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |   56 ++++++++++++++++++++------------------------
 drivers/input/mouse/alps.h |    5 ----
 2 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 9908f18..6fc608d 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -106,38 +106,36 @@
 #define ALPS_DUALPOINT_WITH_PRESSURE	0x400	/* device can report trackpoint pressure */
 
 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 */
-	{ { 0x33, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V1, 0x88, 0xf8, 0 } },				/* UMAX-530T */
-	{ { 0x53, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x53, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x60, 0x03, 0xc8 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },				/* HP ze1115 */
-	{ { 0x63, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x63, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x63, 0x02, 0x28 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },		/* Fujitsu Siemens S6010 */
-	{ { 0x63, 0x02, 0x3c }, 0x00, { ALPS_PROTO_V2, 0x8f, 0x8f, ALPS_WHEEL } },		/* Toshiba Satellite S2400-103 */
-	{ { 0x63, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xef, 0xef, ALPS_FW_BK_1 } },		/* NEC Versa L320 */
-	{ { 0x63, 0x02, 0x64 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x63, 0x03, 0xc8 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D800 */
-	{ { 0x73, 0x00, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_DUALPOINT } },		/* ThinkPad R61 8918-5QG */
-	{ { 0x73, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
-	{ { 0x73, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },		/* Ahtec Laptop */
-
 	/*
 	 * XXX This entry is suspicious. First byte has zero lower nibble,
 	 * which is what a normal mouse would report. Also, the value 0x0e
 	 * isn't valid per PS/2 spec.
 	 */
-	{ { 0x20, 0x02, 0x0e }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
-
-	{ { 0x22, 0x02, 0x0a }, 0x00, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
-	{ { 0x22, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D600 */
-	/* Dell Latitude E5500, E6400, E6500, Precision M4400 */
-	{ { 0x62, 0x02, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf,
-		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },
-	{ { 0x73, 0x00, 0x14 }, 0x00, { ALPS_PROTO_V6, 0xff, 0xff, ALPS_DUALPOINT } },		/* Dell XT2 */
-	{ { 0x73, 0x02, 0x50 }, 0x00, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },	/* Dell Vostro 1400 */
-	{ { 0x52, 0x01, 0x14 }, 0x00, { ALPS_PROTO_V2, 0xff, 0xff,
+	{ { 0x20, 0x02, 0x0e }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
+
+	{ { 0x22, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },
+	{ { 0x22, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xff, 0xff, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D600 */
+	{ { 0x32, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Toshiba Salellite Pro M10 */
+	{ { 0x33, 0x02, 0x0a }, { ALPS_PROTO_V1, 0x88, 0xf8, 0 } },				/* UMAX-530T */
+	{ { 0x52, 0x01, 0x14 }, { ALPS_PROTO_V2, 0xff, 0xff,
 		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Toshiba Tecra A11-11L */
+	{ { 0x53, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x53, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x60, 0x03, 0xc8 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },				/* HP ze1115 */
+	{ { 0x62, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xcf, 0xcf,
+		ALPS_PASS | ALPS_DUALPOINT | ALPS_PS2_INTERLEAVED } },				/* Dell Latitude E5500, E6400, E6500, Precision M4400 */
+	{ { 0x63, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x63, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x63, 0x02, 0x28 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },			/* Fujitsu Siemens S6010 */
+	{ { 0x63, 0x02, 0x3c }, { ALPS_PROTO_V2, 0x8f, 0x8f, ALPS_WHEEL } },			/* Toshiba Satellite S2400-103 */
+	{ { 0x63, 0x02, 0x50 }, { ALPS_PROTO_V2, 0xef, 0xef, ALPS_FW_BK_1 } },			/* NEC Versa L320 */
+	{ { 0x63, 0x02, 0x64 }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x63, 0x03, 0xc8 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT } },	/* Dell Latitude D800 */
+	{ { 0x73, 0x00, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_DUALPOINT } },		/* ThinkPad R61 8918-5QG */
+	{ { 0x73, 0x00, 0x14 }, { ALPS_PROTO_V6, 0xff, 0xff, ALPS_DUALPOINT } },		/* Dell XT2 */
+	{ { 0x73, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
+	{ { 0x73, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } },			/* Ahtec Laptop */
+	{ { 0x73, 0x02, 0x50 }, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } },		/* Dell Vostro 1400 */
 };
 
 static const struct alps_protocol_info alps_v3_protocol_data = {
@@ -2779,12 +2777,8 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7,
 	for (i = 0; i < ARRAY_SIZE(alps_model_data); i++) {
 		model = &alps_model_data[i];
 
-		if (!memcmp(e7, model->signature, sizeof(model->signature)) &&
-		    (!model->command_mode_resp ||
-		     model->command_mode_resp == ec[2])) {
-
+		if (!memcmp(e7, model->signature, sizeof(model->signature)))
 			return &model->protocol_info;
-		}
 	}
 
 	return NULL;
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 54f9f44..dea3163 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -163,10 +163,6 @@ struct alps_protocol_info {
 /**
  * struct alps_model_info - touchpad ID table
  * @signature: E7 response string to match.
- * @command_mode_resp: For V3/V4 touchpads, the final byte of the EC response
- *   (aka command mode response) identifies the firmware minor version.  This
- *   can be used to distinguish different hardware models which are not
- *   uniquely identifiable through their E7 responses.
  * @protocol_info: information about protocol used by the device.
  *
  * Many (but not all) ALPS touchpads can be identified by looking at the
@@ -175,7 +171,6 @@ struct alps_protocol_info {
  */
 struct alps_model_info {
 	u8 signature[3];
-	u8 command_mode_resp;
 	struct alps_protocol_info protocol_info;
 };
 
-- 
1.7.9.5

Re: [PATCH v2 0/3] Input: alps - cleanup

[Dmitry Torokhov]

On Sat, Mar 04, 2017 at 12:24:23PM +0100, Pali Rohár wrote:
> This patch series cleanup usage of alps_model_data table.
> 
> Changes since v1: Patches for ALPS_PROTO_V6 and ALPS_PROTO_V1 were
> dropped and support for those touchpad are still in alps_model_data.
> 
> Pali Rohár (3):
>   Input: alps - move ALPS_PROTO_V4 out of alps_model_data table
>   Input: alps - warn about unsupported ALPS V9 touchpad
>   Input: alps - cleanup alps_model_data
> 
>  drivers/input/mouse/alps.c |   75 +++++++++++++++++++++++++-------------------
>  drivers/input/mouse/alps.h |    6 +---
>  2 files changed, 43 insertions(+), 38 deletions(-)

Applied the lot, thank you.

-- 
Dmitry