[PATCH 0/3] synaptics: Change min/max quirk table to pnp-id matching

[PATCH 0/3] synaptics: Change min/max quirk table to pnp-id matching

[Hans de Goede]

Hi Dmitry,

After seeing how happy you were with the last quirk patch for the synaptics
driver I thought it was time to make things somewhat better, hence this
patch set.

Most of the affected models share pnp-ids for the touchpad. So switching
to pnp-ids give us 2 advantages:
1) It shrinks the quirk list
2) It will lower the new quirk addition frequency

Note that this will not get rid of the need to add a new quirk every now
and then, but it should significantly reduce the frequency with which we add
them.

This has been tested on a T440s.

Since I would also like to avoid the need to still add new quirks to stable,
and since the prereqs are also all in stable, I've also put a CC stable on
this series.

Regards,

Hans

[PATCH 1/3] synaptics: T540p: make the min/max identical to other LEN0034 models

[Hans de Goede]

The T540p has a touchpad with pnp-id LEN0034, all the models with this pnp-id
have the same min/max values, except the T540p where the values are slightly
off. Fix them to be identical.

This is a preparation patch for simplifying the quirk table.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.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 d4c05b1..898e8bb 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1603,7 +1603,7 @@ static const struct dmi_system_id min_max_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T540"),
 		},
-		.driver_data = (int []){1024, 5056, 2058, 4832},
+		.driver_data = (int []){1024, 5112, 2024, 4832},
 	},
 	{
 		/* Lenovo ThinkPad L540 */
-- 
1.9.0

[PATCH 2/3] synaptics: Add a matches_pnp_id helper function

[Hans de Goede]

This is a preparation patch for simplifying the min/max quirk table.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/mouse/synaptics.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 898e8bb..395ec9c 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -156,6 +156,18 @@ static const char * const topbuttonpad_pnp_ids[] = {
 	NULL
 };
 
+static bool matches_pnp_id(struct psmouse *psmouse, const char * const ids[])
+{
+	int i;
+
+	if (!strncmp(psmouse->ps2dev.serio->firmware_id, "PNP:", 4))
+		for (i = 0; ids[i]; i++)
+			if (strstr(psmouse->ps2dev.serio->firmware_id, ids[i]))
+				return true;
+
+	return false;
+}
+
 /*****************************************************************************
  *	Synaptics communications functions
  ****************************************************************************/
@@ -1365,17 +1377,8 @@ static void set_input_params(struct psmouse *psmouse,
 
 	if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
 		__set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
-		/* See if this buttonpad has a top button area */
-		if (!strncmp(psmouse->ps2dev.serio->firmware_id, "PNP:", 4)) {
-			for (i = 0; topbuttonpad_pnp_ids[i]; i++) {
-				if (strstr(psmouse->ps2dev.serio->firmware_id,
-					   topbuttonpad_pnp_ids[i])) {
-					__set_bit(INPUT_PROP_TOPBUTTONPAD,
-						  dev->propbit);
-					break;
-				}
-			}
-		}
+		if (matches_pnp_id(psmouse, topbuttonpad_pnp_ids))
+			__set_bit(INPUT_PROP_TOPBUTTONPAD, dev->propbit);
 		/* Clickpads report only left button */
 		__clear_bit(BTN_RIGHT, dev->keybit);
 		__clear_bit(BTN_MIDDLE, dev->keybit);
-- 
1.9.0

[PATCH 3/3] synaptics: Change min/max quirk table to pnp-id matching

[Hans de Goede]

Most of the affected models share pnp-ids for the touchpad. So switching
to pnp-ids give us 2 advantages:
1) It shrinks the quirk list
2) It will lower the new quirk addition frequency, ie the recently added W540
   quirk would not have been necessary since it uses the same LEN0034 pnp ids
   as other models already added before it

As an added bonus it actually puts the quirk on the actual psmouse, rather then
on the machine, which is technically more correct.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/mouse/synaptics.c | 149 ++++++++++------------------------------
 1 file changed, 36 insertions(+), 113 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 395ec9c..c5ec703 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -117,6 +117,31 @@ void synaptics_reset(struct psmouse *psmouse)
 }
 
 #ifdef CONFIG_MOUSE_PS2_SYNAPTICS
+struct min_max_quirk {
+	const char * const *pnp_ids;
+	int x_min, x_max, y_min, y_max;
+};
+
+static const struct min_max_quirk min_max_pnpid_table[] = {
+	{
+		(const char * const []){"LEN0033", NULL},
+		1024, 5052, 2258, 4832
+	},
+	{
+		(const char * const []){"LEN0035", "LEN0042", NULL},
+		1232, 5710, 1156, 4696
+	},
+	{
+		(const char * const []){"LEN0034", "LEN0036", "LEN2004", NULL},
+		1024, 5112, 2024, 4832
+	},
+	{
+		(const char * const []){"LEN2001", NULL},
+		1024, 5022, 2508, 4832
+	},
+	{ }
+};
+
 /* This list has been kindly provided by Synaptics. */
 static const char * const topbuttonpad_pnp_ids[] = {
 	"LEN0017",
@@ -129,7 +154,7 @@ static const char * const topbuttonpad_pnp_ids[] = {
 	"LEN002D",
 	"LEN002E",
 	"LEN0033", /* Helix */
-	"LEN0034", /* T431s, T540, X1 Carbon 2nd */
+	"LEN0034", /* T431s, L440, L540, T540, W540, X1 Carbon 2nd */
 	"LEN0035", /* X240 */
 	"LEN0036", /* T440 */
 	"LEN0037",
@@ -142,7 +167,7 @@ static const char * const topbuttonpad_pnp_ids[] = {
 	"LEN0048",
 	"LEN0049",
 	"LEN2000",
-	"LEN2001",
+	"LEN2001", /* Edge E431 */
 	"LEN2002",
 	"LEN2003",
 	"LEN2004", /* L440 */
@@ -316,20 +341,20 @@ static int synaptics_identify(struct psmouse *psmouse)
  * Resolution is left zero if touchpad does not support the query
  */
 
-static const int *quirk_min_max;
-
 static int synaptics_resolution(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
 	unsigned char resp[3];
+	int i;
 
-	if (quirk_min_max) {
-		priv->x_min = quirk_min_max[0];
-		priv->x_max = quirk_min_max[1];
-		priv->y_min = quirk_min_max[2];
-		priv->y_max = quirk_min_max[3];
-		return 0;
-	}
+	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++)
+		if (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_ID_MAJOR(priv->identity) < 4)
 		return 0;
@@ -1550,112 +1575,10 @@ static const struct dmi_system_id olpc_dmi_table[] __initconst = {
 	{ }
 };
 
-static const struct dmi_system_id min_max_dmi_table[] __initconst = {
-#if defined(CONFIG_DMI)
-	{
-		/* Lenovo ThinkPad Helix */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad Helix"),
-		},
-		.driver_data = (int []){1024, 5052, 2258, 4832},
-	},
-	{
-		/* Lenovo ThinkPad X240 */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X240"),
-		},
-		.driver_data = (int []){1232, 5710, 1156, 4696},
-	},
-	{
-		/* Lenovo ThinkPad Edge E431 */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad Edge E431"),
-		},
-		.driver_data = (int []){1024, 5022, 2508, 4832},
-	},
-	{
-		/* Lenovo ThinkPad T431s */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T431"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-	{
-		/* Lenovo ThinkPad T440s */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T440"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-	{
-		/* Lenovo ThinkPad L440 */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad L440"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-	{
-		/* Lenovo ThinkPad T540p */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T540"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-	{
-		/* Lenovo ThinkPad L540 */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad L540"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-	{
-		/* Lenovo ThinkPad W540 */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W540"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-	{
-		/* Lenovo Yoga S1 */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION,
-					"ThinkPad S1 Yoga"),
-		},
-		.driver_data = (int []){1232, 5710, 1156, 4696},
-	},
-	{
-		/* Lenovo ThinkPad X1 Carbon Haswell (3rd generation) */
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-			DMI_MATCH(DMI_PRODUCT_VERSION,
-					"ThinkPad X1 Carbon 2nd"),
-		},
-		.driver_data = (int []){1024, 5112, 2024, 4832},
-	},
-#endif
-	{ }
-};
-
 void __init synaptics_module_init(void)
 {
-	const struct dmi_system_id *min_max_dmi;
-
 	impaired_toshiba_kbc = dmi_check_system(toshiba_dmi_table);
 	broken_olpc_ec = dmi_check_system(olpc_dmi_table);
-
-	min_max_dmi = dmi_first_match(min_max_dmi_table);
-	if (min_max_dmi)
-		quirk_min_max = min_max_dmi->driver_data;
 }
 
 static int __synaptics_init(struct psmouse *psmouse, bool absolute_mode)
-- 
1.9.0

Re: [PATCH 3/3] synaptics: Change min/max quirk table to pnp-id matching

[Dmitry Torokhov]

On Fri, May 16, 2014 at 08:46:49PM +0200, Hans de Goede wrote:
> Most of the affected models share pnp-ids for the touchpad. So switching
> to pnp-ids give us 2 advantages:
> 1) It shrinks the quirk list
> 2) It will lower the new quirk addition frequency, ie the recently added W540
>    quirk would not have been necessary since it uses the same LEN0034 pnp ids
>    as other models already added before it
> 
> As an added bonus it actually puts the quirk on the actual psmouse, rather then
> on the machine, which is technically more correct.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/mouse/synaptics.c | 149 ++++++++++------------------------------
>  1 file changed, 36 insertions(+), 113 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 395ec9c..c5ec703 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -117,6 +117,31 @@ void synaptics_reset(struct psmouse *psmouse)
>  }
>  
>  #ifdef CONFIG_MOUSE_PS2_SYNAPTICS
> +struct min_max_quirk {
> +	const char * const *pnp_ids;
> +	int x_min, x_max, y_min, y_max;
> +};


Why don't we define this as 1 quirk per PNP id?

struct min_max_quirk {
	const char *pnp_id;
	int x_min, x_max, y_min, y_max;
};

?

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] synaptics: Change min/max quirk table to pnp-id matching

[Hans de Goede]

Hi,

On 05/18/2014 10:35 PM, Dmitry Torokhov wrote:
> On Fri, May 16, 2014 at 08:46:49PM +0200, Hans de Goede wrote:
>> Most of the affected models share pnp-ids for the touchpad. So switching
>> to pnp-ids give us 2 advantages:
>> 1) It shrinks the quirk list
>> 2) It will lower the new quirk addition frequency, ie the recently added W540
>>    quirk would not have been necessary since it uses the same LEN0034 pnp ids
>>    as other models already added before it
>>
>> As an added bonus it actually puts the quirk on the actual psmouse, rather then
>> on the machine, which is technically more correct.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/input/mouse/synaptics.c | 149 ++++++++++------------------------------
>>  1 file changed, 36 insertions(+), 113 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 395ec9c..c5ec703 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -117,6 +117,31 @@ void synaptics_reset(struct psmouse *psmouse)
>>  }
>>  
>>  #ifdef CONFIG_MOUSE_PS2_SYNAPTICS
>> +struct min_max_quirk {
>> +	const char * const *pnp_ids;
>> +	int x_min, x_max, y_min, y_max;
>> +};
> 
> 
> Why don't we define this as 1 quirk per PNP id?
> 
> struct min_max_quirk {
> 	const char *pnp_id;
> 	int x_min, x_max, y_min, y_max;
> };
> 
> ?

1) I thought it would be better to allow multiple ids for one min/max quad,
since there seem to only be a few types of touchpads out there, which are
sometimes referenced to by multiple ids. IE LEN0034 and LEN2004 refer to the
exact same touchpad (exact same firmware and board id). Also this way we avoid
people adding entries with values which are slightly off since determining
the min/max range on a single model will give some noise.

2) This way we can use one helper function for the matching for both the
INPUT_PROP_TOPBUTTONPAD quirks and for the min/max quirks.

Regards,

Hans

Re: [PATCH 3/3] synaptics: Change min/max quirk table to pnp-id matching

[Hans de Goede]

Hi Dmitry,

On 05/19/2014 10:51 AM, Hans de Goede wrote:
> Hi,
> 
> On 05/18/2014 10:35 PM, Dmitry Torokhov wrote:
>> On Fri, May 16, 2014 at 08:46:49PM +0200, Hans de Goede wrote:
>>> Most of the affected models share pnp-ids for the touchpad. So switching
>>> to pnp-ids give us 2 advantages:
>>> 1) It shrinks the quirk list
>>> 2) It will lower the new quirk addition frequency, ie the recently added W540
>>>    quirk would not have been necessary since it uses the same LEN0034 pnp ids
>>>    as other models already added before it
>>>
>>> As an added bonus it actually puts the quirk on the actual psmouse, rather then
>>> on the machine, which is technically more correct.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/input/mouse/synaptics.c | 149 ++++++++++------------------------------
>>>  1 file changed, 36 insertions(+), 113 deletions(-)
>>>
>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>> index 395ec9c..c5ec703 100644
>>> --- a/drivers/input/mouse/synaptics.c
>>> +++ b/drivers/input/mouse/synaptics.c
>>> @@ -117,6 +117,31 @@ void synaptics_reset(struct psmouse *psmouse)
>>>  }
>>>  
>>>  #ifdef CONFIG_MOUSE_PS2_SYNAPTICS
>>> +struct min_max_quirk {
>>> +	const char * const *pnp_ids;
>>> +	int x_min, x_max, y_min, y_max;
>>> +};
>>
>>
>> Why don't we define this as 1 quirk per PNP id?
>>
>> struct min_max_quirk {
>> 	const char *pnp_id;
>> 	int x_min, x_max, y_min, y_max;
>> };
>>
>> ?
> 
> 1) I thought it would be better to allow multiple ids for one min/max quad,
> since there seem to only be a few types of touchpads out there, which are
> sometimes referenced to by multiple ids. IE LEN0034 and LEN2004 refer to the
> exact same touchpad (exact same firmware and board id). Also this way we avoid
> people adding entries with values which are slightly off since determining
> the min/max range on a single model will give some noise.
> 
> 2) This way we can use one helper function for the matching for both the
> INPUT_PROP_TOPBUTTONPAD quirks and for the min/max quirks.

I've not heard back from you on this, does that mean that you are ok with
taking this patch-set as is ?

Regards,

Hans

Re: [PATCH 3/3] synaptics: Change min/max quirk table to pnp-id matching

[Dmitry Torokhov]

On Mon, May 26, 2014 at 01:47:31PM +0200, Hans de Goede wrote:
> Hi Dmitry,
> 
> On 05/19/2014 10:51 AM, Hans de Goede wrote:
> > Hi,
> > 
> > On 05/18/2014 10:35 PM, Dmitry Torokhov wrote:
> >> On Fri, May 16, 2014 at 08:46:49PM +0200, Hans de Goede wrote:
> >>> Most of the affected models share pnp-ids for the touchpad. So switching
> >>> to pnp-ids give us 2 advantages:
> >>> 1) It shrinks the quirk list
> >>> 2) It will lower the new quirk addition frequency, ie the recently added W540
> >>>    quirk would not have been necessary since it uses the same LEN0034 pnp ids
> >>>    as other models already added before it
> >>>
> >>> As an added bonus it actually puts the quirk on the actual psmouse, rather then
> >>> on the machine, which is technically more correct.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>>  drivers/input/mouse/synaptics.c | 149 ++++++++++------------------------------
> >>>  1 file changed, 36 insertions(+), 113 deletions(-)
> >>>
> >>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >>> index 395ec9c..c5ec703 100644
> >>> --- a/drivers/input/mouse/synaptics.c
> >>> +++ b/drivers/input/mouse/synaptics.c
> >>> @@ -117,6 +117,31 @@ void synaptics_reset(struct psmouse *psmouse)
> >>>  }
> >>>  
> >>>  #ifdef CONFIG_MOUSE_PS2_SYNAPTICS
> >>> +struct min_max_quirk {
> >>> +	const char * const *pnp_ids;
> >>> +	int x_min, x_max, y_min, y_max;
> >>> +};
> >>
> >>
> >> Why don't we define this as 1 quirk per PNP id?
> >>
> >> struct min_max_quirk {
> >> 	const char *pnp_id;
> >> 	int x_min, x_max, y_min, y_max;
> >> };
> >>
> >> ?
> > 
> > 1) I thought it would be better to allow multiple ids for one min/max quad,
> > since there seem to only be a few types of touchpads out there, which are
> > sometimes referenced to by multiple ids. IE LEN0034 and LEN2004 refer to the
> > exact same touchpad (exact same firmware and board id). Also this way we avoid
> > people adding entries with values which are slightly off since determining
> > the min/max range on a single model will give some noise.
> > 
> > 2) This way we can use one helper function for the matching for both the
> > INPUT_PROP_TOPBUTTONPAD quirks and for the min/max quirks.
> 
> I've not heard back from you on this, does that mean that you are ok with
> taking this patch-set as is ?

Hans,

Yes, I applied it.

Thanks.

-- 
Dmitry