On Fri, Jan 30, 2015 at 4:03 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > 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@xxxxxxxxxxx> wrote: >>> From: Daniel Martin <consume.noise@xxxxxxxxx> >>> >>> 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@xxxxxxxxx> >>> --- >>> 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. > If you are fine either way, I would lean to have only one check. I am not confident in the "future proof" way of having a list, because the list is too tight to the current bug. But I think I already made my point clear enough. Also, we already reported this to Synaptics, and I think (hope) they will fix this in their future FW. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
