Re: [PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 01/29/2015 07:48 PM, Benjamin Tissoires wrote:
> Hi Daniel,
> 
> in one hand I would like to see that upstream ASAP. But on the other
> hand, I have some concerns here and there regarding this series.
> 
> On Tue, Jan 27, 2015 at 3:34 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 is safe. Atm. this is
>> version 8.1 only.
>>
>> With that we don't need quirks for post-2013 models anymore as they
>> expose correct min and max dimensions.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>> ---
>>  drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 7a78d75..37d4dff 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
>>         return -1;
>>  }
>>
>> +struct fw_version {
>> +       unsigned char major;
>> +       unsigned char minor;
>> +};
>> +
>> +static const struct fw_version safe_fw_list[] = {
>> +       {8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
>> +       { }
>> +};
>> +
>> +static bool synaptics_is_safe_fw(struct psmouse *psmouse)
>> +{
>> +       struct synaptics_data *priv = psmouse->private;
>> +       int i;
>> +
>> +       for (i = 0; safe_fw_list[i].major; i++) {
>> +               if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
>> +                   safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
>> +                       return true;
>> +       }
>> +
>> +       return false;
>> +}
> 
> This seems a little bit too much for just one firmware ID.
> Plus the name safe_fw_list gives hope that this list can be used for
> something else later, but then it may conflict with it current
> purpose.
> 
> How about we just rely on the current list of PNPIDs we already have
> for this generation?
> This might not be a good idea either, so an other solution would be to
> directly check for firmware 8.1 in the test below.
> 
> I'd like to hear other opinions on this however before we commit to a decision.

I think that checking the firmware version, rather then relying on pnp-ids, is
the right thing to do, this e.g. also gives us more accurate min/max values
on the x230 / t430 series.

As for using the list, rather then just the one id, I'm fine either way, the list
is more future proof, which is why I acked this patch as is. But we could go
with just a check for the single version now and extend this later if needed.

Regards,

Hans


> 
> 
> Cheers,
> Benjamin
> 
> PS: I think Hans already mentioned, but if you want your patches to
> land in Dmitry's tree, it's better to CC him directly when submitting
> a patch.
> 
>> +
>>  /*
>>   * Read touchpad resolution and maximum reported coordinates
>>   * Resolution is left zero if touchpad does not support the query
>> @@ -368,7 +392,8 @@ static int synaptics_resolution(struct psmouse *psmouse)
>>                 }
>>         }
>>
>> -       if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
>> +       if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 ||
>> +            synaptics_is_safe_fw(psmouse)) &&
>>             SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
>>                 if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
>>                         psmouse_warn(psmouse,
>> --
>> 2.2.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html