[v3] synaptics: match PNP-Id is not sufficient for min/max quirks

[v3] synaptics: match PNP-Id is not sufficient for min/max quirks

[Daniel Martin]

Hi folks,

this is the third version of my patches to address the bug:
    https://bugzilla.kernel.org/show_bug.cgi?id=91541

One task on top of patch 5 remains: Find suitable board id ranges.
I.e. in the first comment of the bug, the board id 2962 is the lowest
one that I know of for a post-2013 model. So, the quirk with PnP id
LEN0036 could have a max board id of 2961.

Changes to v2 are:

[PATCH 3/5] Input: synaptics - Query min dimensions for fw v8.1
- Here I have replaced the "safe fw" list with a simple check for
  version 8.1

[PATCH 4/5] Input: synaptics - Remove obsolete min/max quirk for X240
- This patch has been moved from the patch set tail here.

[PATCH 5/5] Input: synaptics - Support min/max board id in
- With that we can restrict min/max quirks by board id ranges. It
  replaces the previous patch:
      Input: synaptics - Skip quirks when post-2013 dimensions


Cheers,
    Daniel Martin

[PATCH 1/5] Input: synaptics - Split synaptics_resolution(), query first

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Split the function synaptics_resolution() into
    synaptics_resolution() and synaptics_quirks().

synaptics_resolution() will be called before synaptics_quirks() to query
dimensions and resolutions before overwriting them with quirks.

v2: Removed SYN_ID_MAJOR() check from synaptics_quirks().

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f947292..89b408b 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -342,7 +342,6 @@ static int synaptics_resolution(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
 	unsigned char resp[3];
-	int i;
 
 	if (SYN_ID_MAJOR(priv->identity) < 4)
 		return 0;
@@ -354,17 +353,6 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		}
 	}
 
-	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
-		if (psmouse_matches_pnp_id(psmouse,
-					   min_max_pnpid_table[i].pnp_ids)) {
-			priv->x_min = min_max_pnpid_table[i].x_min;
-			priv->x_max = min_max_pnpid_table[i].x_max;
-			priv->y_min = min_max_pnpid_table[i].y_min;
-			priv->y_max = min_max_pnpid_table[i].y_max;
-			return 0;
-		}
-	}
-
 	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 5 &&
 	    SYN_CAP_MAX_DIMENSIONS(priv->ext_cap_0c)) {
 		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MAX_COORDS, resp)) {
@@ -390,6 +378,29 @@ static int synaptics_resolution(struct psmouse *psmouse)
 	return 0;
 }
 
+/*
+ * Apply quirk(s) if the hardware matches
+ */
+
+static int synaptics_quirks(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int i;
+
+	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
+		if (psmouse_matches_pnp_id(psmouse,
+					   min_max_pnpid_table[i].pnp_ids)) {
+			priv->x_min = min_max_pnpid_table[i].x_min;
+			priv->x_max = min_max_pnpid_table[i].x_max;
+			priv->y_min = min_max_pnpid_table[i].y_min;
+			priv->y_max = min_max_pnpid_table[i].y_max;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static int synaptics_query_hardware(struct psmouse *psmouse)
 {
 	if (synaptics_identify(psmouse))
@@ -404,6 +415,8 @@ static int synaptics_query_hardware(struct psmouse *psmouse)
 		return -1;
 	if (synaptics_resolution(psmouse))
 		return -1;
+	if (synaptics_quirks(psmouse))
+		return -1;
 
 	return 0;
 }
-- 
2.2.2

[PATCH 2/5] Input: synaptics - Log queried and quirked dimension values

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Logging the dimension values we queried (info) and the values we use
from a quirk to overwrite (warn) can be helpful for debugging.

This partly relates to bug:
    https://bugzilla.kernel.org/show_bug.cgi?id=91541

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 89b408b..7a78d75 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -361,6 +361,10 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		} else {
 			priv->x_max = (resp[0] << 5) | ((resp[1] & 0x0f) << 1);
 			priv->y_max = (resp[2] << 5) | ((resp[1] & 0xf0) >> 3);
+			psmouse_info(psmouse,
+				     "queried max coordinates: "
+				     "x [..%d], y [..%d]\n",
+				     priv->x_max, priv->y_max);
 		}
 	}
 
@@ -372,6 +376,10 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		} else {
 			priv->x_min = (resp[0] << 5) | ((resp[1] & 0x0f) << 1);
 			priv->y_min = (resp[2] << 5) | ((resp[1] & 0xf0) >> 3);
+			psmouse_info(psmouse,
+				     "queried min coordinates: "
+				     "x [%d..], y [%d..]\n",
+				     priv->x_min, priv->y_min);
 		}
 	}
 
@@ -394,6 +402,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
 			priv->x_max = min_max_pnpid_table[i].x_max;
 			priv->y_min = min_max_pnpid_table[i].y_min;
 			priv->y_max = min_max_pnpid_table[i].y_max;
+			psmouse_warn(psmouse,
+				     "quirked min/max coordinates: "
+				     "x [%d..%d], y [%d..%d]\n",
+				     priv->x_min, priv->x_max,
+				     priv->y_min, priv->y_max);
 			break;
 		}
 	}
-- 
2.2.2

[PATCH 3/5] Input: synaptics - Query min dimensions for fw v8.1

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Query the min dimensions even if the check
    SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
fails, but we know that the firmware version 8.1 is safe.

With that we don't need quirks for post-2013 models anymore as they
expose correct min and max dimensions.

v2: Don't use a list for safe firmwares.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 7a78d75..0b7805c 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -368,8 +368,13 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		}
 	}
 
-	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
-	    SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
+	if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
+	     SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) ||
+	    /* Firmware v8.1 doesn't stand the previous checks, though has
+	     * been proven to report correct min coordinates.
+	     *     https://bugzilla.kernel.org/show_bug.cgi?id=91541 */
+	    (SYN_ID_MAJOR(priv->identity) == 8 &&
+	     SYN_ID_MINOR(priv->identity) == 1)) {
 		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
 			psmouse_warn(psmouse,
 				     "device claims to have min coordinates query, but I'm not able to read it.\n");
-- 
2.2.2

Re: [PATCH 3/5] Input: synaptics - Query min dimensions for fw v8.1

[Benjamin Tissoires]

Thanks Daniel for the respin.

On Fri, Feb 6, 2015 at 8:25 AM, Daniel Martin <daniel.martin@secunet.com> wrote:
> From: Daniel Martin <consume.noise@gmail.com>
>
> Query the min dimensions even if the check
>     SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
> fails, but we know that the firmware version 8.1 is safe.
>
> With that we don't need quirks for post-2013 models anymore as they
> expose correct min and max dimensions.
>
> v2: Don't use a list for safe firmwares.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 7a78d75..0b7805c 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -368,8 +368,13 @@ static int synaptics_resolution(struct psmouse *psmouse)
>                 }
>         }
>
> -       if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
> -           SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
> +       if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
> +            SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) ||
> +           /* Firmware v8.1 doesn't stand the previous checks, though has
> +            * been proven to report correct min coordinates.
> +            *     https://bugzilla.kernel.org/show_bug.cgi?id=91541 */
> +           (SYN_ID_MAJOR(priv->identity) == 8 &&
> +            SYN_ID_MINOR(priv->identity) == 1)) {

I have one minor comment here. I think it will be safer to ask for:

if (SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c) &&
   (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 ||
    /* Firmware v8.1 doesn't stand the previous checks, though has
     * been proven to report correct min coordinates.
     *     https://bugzilla.kernel.org/show_bug.cgi?id=91541 */
     (SYN_ID_MAJOR(priv->identity) == 8 &&
      SYN_ID_MINOR(priv->identity) == 1)))

IIRC, the min_dimension bit was properly set, but not the capability >= 7.

I am not sure if all the boards with the 8.1 FW are able to be queried
on the min coordinate.

Cheers,
Benjamin

>                 if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
>                         psmouse_warn(psmouse,
>                                      "device claims to have min coordinates query, but I'm not able to read it.\n");
> --
> 2.2.2
>

[PATCH 4/5] Input: synaptics - Remove obsolete min/max quirk for X240

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

The firmware of the X240 (LEN0035, 2013/12) exposes the same values
    x [1232..5710], y [1156..4696]
as the quirk applies.

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 0b7805c..0ea36ae 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -131,7 +131,7 @@ static const struct min_max_quirk min_max_pnpid_table[] = {
 		1024, 5052, 2258, 4832
 	},
 	{
-		(const char * const []){"LEN0035", "LEN0042", NULL},
+		(const char * const []){"LEN0042", NULL},
 		1232, 5710, 1156, 4696
 	},
 	{
-- 
2.2.2

[PATCH 5/5] Input: synaptics - Support min/max board id in min_max_pnpid_table

[Daniel Martin]

Add a min/max range for board ids to the min/max coordinates quirk. This
makes it possible to restrict quirks to specific models based upon their
board id. The define ANY_BOARD_ID (0) serves as a wildcard.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <daniel.martin@secunet.com>
---
 drivers/input/mouse/synaptics.c | 44 +++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 0ea36ae..601ed39 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -120,31 +120,40 @@ void synaptics_reset(struct psmouse *psmouse)
 
 static bool cr48_profile_sensor;
 
+#define ANY_BOARD_ID 0
 struct min_max_quirk {
 	const char * const *pnp_ids;
+	struct {
+		unsigned long int min, max;
+	} board_id;
 	int x_min, x_max, y_min, y_max;
 };
 
 static const struct min_max_quirk min_max_pnpid_table[] = {
 	{
 		(const char * const []){"LEN0033", NULL},
+		{ANY_BOARD_ID, ANY_BOARD_ID},
 		1024, 5052, 2258, 4832
 	},
 	{
 		(const char * const []){"LEN0042", NULL},
+		{ANY_BOARD_ID, ANY_BOARD_ID},
 		1232, 5710, 1156, 4696
 	},
 	{
 		(const char * const []){"LEN0034", "LEN0036", "LEN0039",
 					"LEN2002", "LEN2004", NULL},
+		{ANY_BOARD_ID, ANY_BOARD_ID},
 		1024, 5112, 2024, 4832
 	},
 	{
 		(const char * const []){"LEN2001", NULL},
+		{ANY_BOARD_ID, ANY_BOARD_ID},
 		1024, 5022, 2508, 4832
 	},
 	{
 		(const char * const []){"LEN2006", NULL},
+		{ANY_BOARD_ID, ANY_BOARD_ID},
 		1264, 5675, 1171, 4688
 	},
 	{ }
@@ -401,19 +410,28 @@ static int synaptics_quirks(struct psmouse *psmouse)
 	int i;
 
 	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
-		if (psmouse_matches_pnp_id(psmouse,
-					   min_max_pnpid_table[i].pnp_ids)) {
-			priv->x_min = min_max_pnpid_table[i].x_min;
-			priv->x_max = min_max_pnpid_table[i].x_max;
-			priv->y_min = min_max_pnpid_table[i].y_min;
-			priv->y_max = min_max_pnpid_table[i].y_max;
-			psmouse_warn(psmouse,
-				     "quirked min/max coordinates: "
-				     "x [%d..%d], y [%d..%d]\n",
-				     priv->x_min, priv->x_max,
-				     priv->y_min, priv->y_max);
-			break;
-		}
+		if (!psmouse_matches_pnp_id(psmouse,
+					    min_max_pnpid_table[i].pnp_ids))
+			continue;
+
+		if (min_max_pnpid_table[i].board_id.min != ANY_BOARD_ID &&
+		    priv->board_id < min_max_pnpid_table[i].board_id.min)
+			continue;
+
+		if (min_max_pnpid_table[i].board_id.max != ANY_BOARD_ID &&
+		    priv->board_id > min_max_pnpid_table[i].board_id.max)
+			continue;
+
+		priv->x_min = min_max_pnpid_table[i].x_min;
+		priv->x_max = min_max_pnpid_table[i].x_max;
+		priv->y_min = min_max_pnpid_table[i].y_min;
+		priv->y_max = min_max_pnpid_table[i].y_max;
+		psmouse_warn(psmouse,
+			     "quirked min/max coordinates: "
+			     "x [%d..%d], y [%d..%d]\n",
+			     priv->x_min, priv->x_max,
+			     priv->y_min, priv->y_max);
+		break;
 	}
 
 	return 0;
-- 
2.2.2

Re: [v3] synaptics: match PNP-Id is not sufficient for min/max quirks

[Benjamin Tissoires]

Again, Thanks for the respin.

On Fri, Feb 6, 2015 at 8:25 AM, Daniel Martin <daniel.martin@secunet.com> wrote:
> Hi folks,
>
> this is the third version of my patches to address the bug:
>     https://bugzilla.kernel.org/show_bug.cgi?id=91541
>
> One task on top of patch 5 remains: Find suitable board id ranges.
> I.e. in the first comment of the bug, the board id 2962 is the lowest
> one that I know of for a post-2013 model. So, the quirk with PnP id
> LEN0036 could have a max board id of 2961.

I think that is sensible. We can always update it later if we find a
new board id lower, but I doubt there will be much different board ids
between the refreshes.

On a private mail you told me that I could take over this work, so
I'll send the 6/5 patch with the board id sets and a v4 of the 3/5
with the small fix in the test.

Cheers,
Benjamin

>
> Changes to v2 are:
>
> [PATCH 3/5] Input: synaptics - Query min dimensions for fw v8.1
> - Here I have replaced the "safe fw" list with a simple check for
>   version 8.1
>
> [PATCH 4/5] Input: synaptics - Remove obsolete min/max quirk for X240
> - This patch has been moved from the patch set tail here.
>
> [PATCH 5/5] Input: synaptics - Support min/max board id in
> - With that we can restrict min/max quirks by board id ranges. It
>   replaces the previous patch:
>       Input: synaptics - Skip quirks when post-2013 dimensions
>
>
> Cheers,
>     Daniel Martin