[v2,4/5] Input: synaptics - Skip quirks when post-2013 dimensions

Commit Message

Daniel Martin Jan. 27, 2015, 8:34 a.m. UTC

From: Daniel Martin <[e-mail]>

If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
have post-2013 model and don't need to apply any quirk.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <[e-mail]>
---
 drivers/input/mouse/synaptics.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Benjamin Tissoires Jan. 29, 2015, 7:02 p.m. UTC | #1

Hi Daniel,

On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
<[e-mail]> wrote:
> From: Daniel Martin <[e-mail]>
>
> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
> have post-2013 model and don't need to apply any quirk.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> Signed-off-by: Daniel Martin <[e-mail]>
> ---
>  drivers/input/mouse/synaptics.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 37d4dff..f6c43ff 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>         struct synaptics_data *priv = psmouse->private;
>         int i;
>
> +       /* Post-2013 models expose correct dimensions. */
> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
> +           priv->y_min == 1170 && priv->y_max == 4684)
> +               return 0;
> +

Well, this one, I don't like it either :(

At least, the test should be within the psmouse_matches_pnp_id() below
to ensure we are deciding with Lenovo devices only.

The other concern is hardcoding these values in the code directly.
What if Synaptics/Lenovo decides to ship a new released model with
proper min_max ranges but with a different offset?

Andrew told us that the board ID should be enough to discriminate old
and faulty touchpads from the new and valid touchpads.

My concern here is that we will have to backport these changes in the
various stable kernel and the various distributions. And if we do not
end up with the right solution right now, that means that we will have
to do the job over and over.

I am quite tempted to find a solution in the userspace for that fix.
Not sure I'll be able to find the right one right now, but it may
worth trying.

Cheers,
Benjamin

>         for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
>                 if (psmouse_matches_pnp_id(psmouse,
>                                            min_max_pnpid_table[i].pnp_ids)) {
> --
> 2.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benjamin Tissoires Jan. 29, 2015, 7:50 p.m. UTC | #2

On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
<[e-mail]> wrote:
> Hi Daniel,
>
> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
> <[e-mail]> wrote:
>> From: Daniel Martin <[e-mail]>
>>
>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>> have post-2013 model and don't need to apply any quirk.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>> Signed-off-by: Daniel Martin <[e-mail]>
>> ---
>>  drivers/input/mouse/synaptics.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 37d4dff..f6c43ff 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>         struct synaptics_data *priv = psmouse->private;
>>         int i;
>>
>> +       /* Post-2013 models expose correct dimensions. */
>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>> +           priv->y_min == 1170 && priv->y_max == 4684)
>> +               return 0;
>> +
>
> Well, this one, I don't like it either :(
>
> At least, the test should be within the psmouse_matches_pnp_id() below
> to ensure we are deciding with Lenovo devices only.
>
> The other concern is hardcoding these values in the code directly.
> What if Synaptics/Lenovo decides to ship a new released model with
> proper min_max ranges but with a different offset?
>
> Andrew told us that the board ID should be enough to discriminate old
> and faulty touchpads from the new and valid touchpads.
>
> My concern here is that we will have to backport these changes in the
> various stable kernel and the various distributions. And if we do not
> end up with the right solution right now, that means that we will have
> to do the job over and over.
>
> I am quite tempted to find a solution in the userspace for that fix.
> Not sure I'll be able to find the right one right now, but it may
> worth trying.
>

So, the user space solution seems difficult because we do not export
either the board_id or the firmware_id. So that would required to
update the kernel anyway, a bunch of user space tools and a hwdb... :(

How about we just add an extra min/max in struct min_max_quirk,
compare the current min/max with the 2 possible values and if there is
a match, we do not override the values.
This way, we keep the crap of wrong/correct min max in the small list
of device we know are problematic, and if the new batch of E540 has a
different correct min/max range, then we will be able to adjust it
without breaking the other we fixed.

Dmitry, Hans, any comments on this?

Cheers,
Benjamin

>>         for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
>>                 if (psmouse_matches_pnp_id(psmouse,
>>                                            min_max_pnpid_table[i].pnp_ids)) {
>> --
>> 2.2.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hans de Goede Jan. 30, 2015, 9:59 a.m. UTC | #3

Hi,

On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
> <[e-mail]> wrote:
>> Hi Daniel,
>>
>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>> <[e-mail]> wrote:
>>> From: Daniel Martin <[e-mail]>
>>>
>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>> have post-2013 model and don't need to apply any quirk.
>>>
>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>> Signed-off-by: Daniel Martin <[e-mail]>
>>> ---
>>>  drivers/input/mouse/synaptics.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>> index 37d4dff..f6c43ff 100644
>>> --- a/drivers/input/mouse/synaptics.c
>>> +++ b/drivers/input/mouse/synaptics.c
>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>         struct synaptics_data *priv = psmouse->private;
>>>         int i;
>>>
>>> +       /* Post-2013 models expose correct dimensions. */
>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>> +               return 0;
>>> +
>>
>> Well, this one, I don't like it either :(
>>
>> At least, the test should be within the psmouse_matches_pnp_id() below
>> to ensure we are deciding with Lenovo devices only.
>>
>> The other concern is hardcoding these values in the code directly.
>> What if Synaptics/Lenovo decides to ship a new released model with
>> proper min_max ranges but with a different offset?
>>
>> Andrew told us that the board ID should be enough to discriminate old
>> and faulty touchpads from the new and valid touchpads.
>>
>> My concern here is that we will have to backport these changes in the
>> various stable kernel and the various distributions. And if we do not
>> end up with the right solution right now, that means that we will have
>> to do the job over and over.
>>
>> I am quite tempted to find a solution in the userspace for that fix.
>> Not sure I'll be able to find the right one right now, but it may
>> worth trying.
>>
> 
> So, the user space solution seems difficult because we do not export
> either the board_id or the firmware_id. So that would required to
> update the kernel anyway, a bunch of user space tools and a hwdb... :(
> 
> How about we just add an extra min/max in struct min_max_quirk,
> compare the current min/max with the 2 possible values and if there is
> a match, we do not override the values.
> This way, we keep the crap of wrong/correct min max in the small list
> of device we know are problematic, and if the new batch of E540 has a
> different correct min/max range, then we will be able to adjust it
> without breaking the other we fixed.
> 
> Dmitry, Hans, any comments on this?

I'm thinking more along the lines of adding a max_broken_board_id field
to the quirks, and if the touchpad board_id is larger then the
max_broken_board_id not use the quirk.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benjamin Tissoires Jan. 30, 2015, 3:34 p.m. UTC | #4

On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <[e-mail]> wrote:
> Hi,
>
> On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>> <[e-mail]> wrote:
>>> Hi Daniel,
>>>
>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>> <[e-mail]> wrote:
>>>> From: Daniel Martin <[e-mail]>
>>>>
>>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>>> have post-2013 model and don't need to apply any quirk.
>>>>
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>> Signed-off-by: Daniel Martin <[e-mail]>
>>>> ---
>>>>  drivers/input/mouse/synaptics.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>> index 37d4dff..f6c43ff 100644
>>>> --- a/drivers/input/mouse/synaptics.c
>>>> +++ b/drivers/input/mouse/synaptics.c
>>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>>         struct synaptics_data *priv = psmouse->private;
>>>>         int i;
>>>>
>>>> +       /* Post-2013 models expose correct dimensions. */
>>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>>> +               return 0;
>>>> +
>>>
>>> Well, this one, I don't like it either :(
>>>
>>> At least, the test should be within the psmouse_matches_pnp_id() below
>>> to ensure we are deciding with Lenovo devices only.
>>>
>>> The other concern is hardcoding these values in the code directly.
>>> What if Synaptics/Lenovo decides to ship a new released model with
>>> proper min_max ranges but with a different offset?
>>>
>>> Andrew told us that the board ID should be enough to discriminate old
>>> and faulty touchpads from the new and valid touchpads.
>>>
>>> My concern here is that we will have to backport these changes in the
>>> various stable kernel and the various distributions. And if we do not
>>> end up with the right solution right now, that means that we will have
>>> to do the job over and over.
>>>
>>> I am quite tempted to find a solution in the userspace for that fix.
>>> Not sure I'll be able to find the right one right now, but it may
>>> worth trying.
>>>
>>
>> So, the user space solution seems difficult because we do not export
>> either the board_id or the firmware_id. So that would required to
>> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>>
>> How about we just add an extra min/max in struct min_max_quirk,
>> compare the current min/max with the 2 possible values and if there is
>> a match, we do not override the values.
>> This way, we keep the crap of wrong/correct min max in the small list
>> of device we know are problematic, and if the new batch of E540 has a
>> different correct min/max range, then we will be able to adjust it
>> without breaking the other we fixed.
>>
>> Dmitry, Hans, any comments on this?
>
> I'm thinking more along the lines of adding a max_broken_board_id field
> to the quirks, and if the touchpad board_id is larger then the
> max_broken_board_id not use the quirk.
>

Yep, this was confirmed by Synaptics that the board id will be
incremented only at each new board revision. So it should be safe to
only check for that.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Duggan Jan. 30, 2015, 10:47 p.m. UTC | #5

On 01/30/2015 07:34 AM, Benjamin Tissoires wrote:
> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <[e-mail]> wrote:
>> Hi,
>>
>> On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>>> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>>> <[e-mail]> wrote:
>>>> Hi Daniel,
>>>>
>>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>>> <[e-mail]> wrote:
>>>>> From: Daniel Martin <[e-mail]>
>>>>>
>>>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>>>> have post-2013 model and don't need to apply any quirk.
>>>>>
>>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>>> Signed-off-by: Daniel Martin <[e-mail]>
>>>>> ---
>>>>>   drivers/input/mouse/synaptics.c | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>>> index 37d4dff..f6c43ff 100644
>>>>> --- a/drivers/input/mouse/synaptics.c
>>>>> +++ b/drivers/input/mouse/synaptics.c
>>>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>>>          struct synaptics_data *priv = psmouse->private;
>>>>>          int i;
>>>>>
>>>>> +       /* Post-2013 models expose correct dimensions. */
>>>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>>>> +               return 0;
>>>>> +
>>>> Well, this one, I don't like it either :(
>>>>
>>>> At least, the test should be within the psmouse_matches_pnp_id() below
>>>> to ensure we are deciding with Lenovo devices only.
>>>>
>>>> The other concern is hardcoding these values in the code directly.
>>>> What if Synaptics/Lenovo decides to ship a new released model with
>>>> proper min_max ranges but with a different offset?
>>>>
>>>> Andrew told us that the board ID should be enough to discriminate old
>>>> and faulty touchpads from the new and valid touchpads.
>>>>
>>>> My concern here is that we will have to backport these changes in the
>>>> various stable kernel and the various distributions. And if we do not
>>>> end up with the right solution right now, that means that we will have
>>>> to do the job over and over.
>>>>
>>>> I am quite tempted to find a solution in the userspace for that fix.
>>>> Not sure I'll be able to find the right one right now, but it may
>>>> worth trying.
>>>>
>>> So, the user space solution seems difficult because we do not export
>>> either the board_id or the firmware_id. So that would required to
>>> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>>>
>>> How about we just add an extra min/max in struct min_max_quirk,
>>> compare the current min/max with the 2 possible values and if there is
>>> a match, we do not override the values.
>>> This way, we keep the crap of wrong/correct min max in the small list
>>> of device we know are problematic, and if the new batch of E540 has a
>>> different correct min/max range, then we will be able to adjust it
>>> without breaking the other we fixed.
>>>
>>> Dmitry, Hans, any comments on this?
>> I'm thinking more along the lines of adding a max_broken_board_id field
>> to the quirks, and if the touchpad board_id is larger then the
>> max_broken_board_id not use the quirk.
>>
> Yep, this was confirmed by Synaptics that the board id will be
> incremented only at each new board revision. So it should be safe to
> only check for that.

It is worth noting that the format of the modes query changed in 
firmware revision 7.5. If you are going to use board ids you should 
probably check to make sure they have firmware which is 7.5 or later 
before doing the check.

Andrew

> Cheers,
> Benjamin

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Martin Jan. 31, 2015, 8:18 a.m. UTC | #6

On Fri, Jan 30, 2015 at 10:34:22AM -0500, Benjamin Tissoires wrote:
> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <[e-mail]> wrote:
> > Hi,
> >
> > On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
> >> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
> >> <[e-mail]> wrote:
> >>> Hi Daniel,
> >>>
> >>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
> >>> <[e-mail]> wrote:
> >>>> From: Daniel Martin <[e-mail]>
> >>>>
> >>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
> >>>> have post-2013 model and don't need to apply any quirk.
> >>>>
> >>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> >>>> Signed-off-by: Daniel Martin <[e-mail]>
> >>>> ---
> >>>>  drivers/input/mouse/synaptics.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >>>> index 37d4dff..f6c43ff 100644
> >>>> --- a/drivers/input/mouse/synaptics.c
> >>>> +++ b/drivers/input/mouse/synaptics.c
> >>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
> >>>>         struct synaptics_data *priv = psmouse->private;
> >>>>         int i;
> >>>>
> >>>> +       /* Post-2013 models expose correct dimensions. */
> >>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
> >>>> +           priv->y_min == 1170 && priv->y_max == 4684)
> >>>> +               return 0;
> >>>> +
> >>>
> >>> Well, this one, I don't like it either :(
> >>>
> >>> At least, the test should be within the psmouse_matches_pnp_id() below
> >>> to ensure we are deciding with Lenovo devices only.
> >>>
> >>> The other concern is hardcoding these values in the code directly.
> >>> What if Synaptics/Lenovo decides to ship a new released model with
> >>> proper min_max ranges but with a different offset?
> >>>
> >>> Andrew told us that the board ID should be enough to discriminate old
> >>> and faulty touchpads from the new and valid touchpads.
> >>>
> >>> My concern here is that we will have to backport these changes in the
> >>> various stable kernel and the various distributions. And if we do not
> >>> end up with the right solution right now, that means that we will have
> >>> to do the job over and over.
> >>>
> >>> I am quite tempted to find a solution in the userspace for that fix.
> >>> Not sure I'll be able to find the right one right now, but it may
> >>> worth trying.
> >>>
> >>
> >> So, the user space solution seems difficult because we do not export
> >> either the board_id or the firmware_id. So that would required to
> >> update the kernel anyway, a bunch of user space tools and a hwdb... :(
> >>
> >> How about we just add an extra min/max in struct min_max_quirk,
> >> compare the current min/max with the 2 possible values and if there is
> >> a match, we do not override the values.
> >> This way, we keep the crap of wrong/correct min max in the small list
> >> of device we know are problematic, and if the new batch of E540 has a
> >> different correct min/max range, then we will be able to adjust it
> >> without breaking the other we fixed.
> >>
> >> Dmitry, Hans, any comments on this?
> >
> > I'm thinking more along the lines of adding a max_broken_board_id field
> > to the quirks, and if the touchpad board_id is larger then the
> > max_broken_board_id not use the quirk.
> >
> 
> Yep, this was confirmed by Synaptics that the board id will be
> incremented only at each new board revision. So it should be safe to
> only check for that.

Could you ask your contact for an exact board id, since when the ranges
have been fixed? From the data I can look at it seems to be <= 2962.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benjamin Tissoires Feb. 5, 2015, 9:22 p.m. UTC | #7

On Sat, Jan 31, 2015 at 3:18 AM, Daniel Martin <[e-mail]> wrote:
> On Fri, Jan 30, 2015 at 10:34:22AM -0500, Benjamin Tissoires wrote:
>> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <[e-mail]> wrote:
>> > Hi,
>> >
>> > On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>> >> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>> >> <[e-mail]> wrote:
>> >>> Hi Daniel,
>> >>>
>> >>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>> >>> <[e-mail]> wrote:
>> >>>> From: Daniel Martin <[e-mail]>
>> >>>>
>> >>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>> >>>> have post-2013 model and don't need to apply any quirk.
>> >>>>
>> >>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>> >>>> Signed-off-by: Daniel Martin <[e-mail]>
>> >>>> ---
>> >>>>  drivers/input/mouse/synaptics.c | 5 +++++
>> >>>>  1 file changed, 5 insertions(+)
>> >>>>
>> >>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> >>>> index 37d4dff..f6c43ff 100644
>> >>>> --- a/drivers/input/mouse/synaptics.c
>> >>>> +++ b/drivers/input/mouse/synaptics.c
>> >>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>> >>>>         struct synaptics_data *priv = psmouse->private;
>> >>>>         int i;
>> >>>>
>> >>>> +       /* Post-2013 models expose correct dimensions. */
>> >>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>> >>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>> >>>> +               return 0;
>> >>>> +
>> >>>
>> >>> Well, this one, I don't like it either :(
>> >>>
>> >>> At least, the test should be within the psmouse_matches_pnp_id() below
>> >>> to ensure we are deciding with Lenovo devices only.
>> >>>
>> >>> The other concern is hardcoding these values in the code directly.
>> >>> What if Synaptics/Lenovo decides to ship a new released model with
>> >>> proper min_max ranges but with a different offset?
>> >>>
>> >>> Andrew told us that the board ID should be enough to discriminate old
>> >>> and faulty touchpads from the new and valid touchpads.
>> >>>
>> >>> My concern here is that we will have to backport these changes in the
>> >>> various stable kernel and the various distributions. And if we do not
>> >>> end up with the right solution right now, that means that we will have
>> >>> to do the job over and over.
>> >>>
>> >>> I am quite tempted to find a solution in the userspace for that fix.
>> >>> Not sure I'll be able to find the right one right now, but it may
>> >>> worth trying.
>> >>>
>> >>
>> >> So, the user space solution seems difficult because we do not export
>> >> either the board_id or the firmware_id. So that would required to
>> >> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>> >>
>> >> How about we just add an extra min/max in struct min_max_quirk,
>> >> compare the current min/max with the 2 possible values and if there is
>> >> a match, we do not override the values.
>> >> This way, we keep the crap of wrong/correct min max in the small list
>> >> of device we know are problematic, and if the new batch of E540 has a
>> >> different correct min/max range, then we will be able to adjust it
>> >> without breaking the other we fixed.
>> >>
>> >> Dmitry, Hans, any comments on this?
>> >
>> > I'm thinking more along the lines of adding a max_broken_board_id field
>> > to the quirks, and if the touchpad board_id is larger then the
>> > max_broken_board_id not use the quirk.
>> >
>>
>> Yep, this was confirmed by Synaptics that the board id will be
>> incremented only at each new board revision. So it should be safe to
>> only check for that.
>
> Could you ask your contact for an exact board id, since when the ranges
> have been fixed? From the data I can look at it seems to be <= 2962.

IIRC, Andrew said in a private mail that extracting this kind of
information is quite tricky.

I would say that we should add a per-pnp_id board limit with the data we know.

I think you could add this in the struct min_max_quirk, and if the
max_board_id is > 0, we check against it.
This way, we could have a finer grain when dealing with the hardware refreshes.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hans de Goede Feb. 7, 2015, 8:02 a.m. UTC | #8

Hi,

On 02/05/2015 10:22 PM, Benjamin Tissoires wrote:
> On Sat, Jan 31, 2015 at 3:18 AM, Daniel Martin <[e-mail]> wrote:
>> On Fri, Jan 30, 2015 at 10:34:22AM -0500, Benjamin Tissoires wrote:
>>> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <[e-mail]> wrote:
>>>> Hi,
>>>>
>>>> On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>>>>> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>>>>> <[e-mail]> wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>>>>> <[e-mail]> wrote:
>>>>>>> From: Daniel Martin <[e-mail]>
>>>>>>>
>>>>>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>>>>>> have post-2013 model and don't need to apply any quirk.
>>>>>>>
>>>>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>>>>> Signed-off-by: Daniel Martin <[e-mail]>
>>>>>>> ---
>>>>>>>  drivers/input/mouse/synaptics.c | 5 +++++
>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>>>>> index 37d4dff..f6c43ff 100644
>>>>>>> --- a/drivers/input/mouse/synaptics.c
>>>>>>> +++ b/drivers/input/mouse/synaptics.c
>>>>>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>>>>>         struct synaptics_data *priv = psmouse->private;
>>>>>>>         int i;
>>>>>>>
>>>>>>> +       /* Post-2013 models expose correct dimensions. */
>>>>>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>>>>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>>>>>> +               return 0;
>>>>>>> +
>>>>>>
>>>>>> Well, this one, I don't like it either :(
>>>>>>
>>>>>> At least, the test should be within the psmouse_matches_pnp_id() below
>>>>>> to ensure we are deciding with Lenovo devices only.
>>>>>>
>>>>>> The other concern is hardcoding these values in the code directly.
>>>>>> What if Synaptics/Lenovo decides to ship a new released model with
>>>>>> proper min_max ranges but with a different offset?
>>>>>>
>>>>>> Andrew told us that the board ID should be enough to discriminate old
>>>>>> and faulty touchpads from the new and valid touchpads.
>>>>>>
>>>>>> My concern here is that we will have to backport these changes in the
>>>>>> various stable kernel and the various distributions. And if we do not
>>>>>> end up with the right solution right now, that means that we will have
>>>>>> to do the job over and over.
>>>>>>
>>>>>> I am quite tempted to find a solution in the userspace for that fix.
>>>>>> Not sure I'll be able to find the right one right now, but it may
>>>>>> worth trying.
>>>>>>
>>>>>
>>>>> So, the user space solution seems difficult because we do not export
>>>>> either the board_id or the firmware_id. So that would required to
>>>>> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>>>>>
>>>>> How about we just add an extra min/max in struct min_max_quirk,
>>>>> compare the current min/max with the 2 possible values and if there is
>>>>> a match, we do not override the values.
>>>>> This way, we keep the crap of wrong/correct min max in the small list
>>>>> of device we know are problematic, and if the new batch of E540 has a
>>>>> different correct min/max range, then we will be able to adjust it
>>>>> without breaking the other we fixed.
>>>>>
>>>>> Dmitry, Hans, any comments on this?
>>>>
>>>> I'm thinking more along the lines of adding a max_broken_board_id field
>>>> to the quirks, and if the touchpad board_id is larger then the
>>>> max_broken_board_id not use the quirk.
>>>>
>>>
>>> Yep, this was confirmed by Synaptics that the board id will be
>>> incremented only at each new board revision. So it should be safe to
>>> only check for that.
>>
>> Could you ask your contact for an exact board id, since when the ranges
>> have been fixed? From the data I can look at it seems to be <= 2962.
> 
> IIRC, Andrew said in a private mail that extracting this kind of
> information is quite tricky.
> 
> I would say that we should add a per-pnp_id board limit with the data we know.
> 
> I think you could add this in the struct min_max_quirk, and if the
> max_board_id is > 0, we check against it.
> This way, we could have a finer grain when dealing with the hardware refreshes.

ACK, I agree that this is the best way forward with this.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 37d4dff..f6c43ff 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -420,6 +420,11 @@  static int synaptics_quirks(struct psmouse *psmouse)
 	struct synaptics_data *priv = psmouse->private;
 	int i;
 
+	/* Post-2013 models expose correct dimensions. */
+	if (priv->x_min == 1266 && priv->x_max == 5674 &&
+	    priv->y_min == 1170 && priv->y_max == 4684)
+		return 0;
+
 	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
 		if (psmouse_matches_pnp_id(psmouse,
 					   min_max_pnpid_table[i].pnp_ids)) {