Re: [PATCH 1/2] elantech: Properly differentiate between clickpads and normal touchpads

[Peter Hutterer <peter.hutterer@who-t.net>]

On Mon, Dec 09, 2013 at 02:21:19PM -0500, Benjamin Tissoires wrote:
> On Mon, Dec 9, 2013 at 2:14 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi,
> >
> >
> > On 12/09/2013 07:02 PM, Benjamin Tissoires wrote:
> >>
> >> Hi Hans,
> >>
> >> adding in CC Duson, who seems to be working on the same driver
> >> currently, and Dmitry, the input maintainer.
> >>
> >> On Mon, Dec 9, 2013 at 9:32 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> The current assumption in the elantech driver that hw version 3 touchpads
> >>> are
> >>> never clickpads and hw version 4 touchpads are always clickpads is wrong.
> >>>
> >>> There are several bug reports for this, ie:
> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1030802
> >>>
> >>> http://superuser.com/questions/619582/right-elantech-touchpad-button-not-working-in-linux
> >>>
> >>> I've spend a couple of hours wading through various bugzillas,
> >>> launchpads and forum posts to create a list of fw-versions and
> >>> capabilities
> >>> for different laptop models to find a good method to differentiate
> >>> between
> >>> clickpads and versions with separate hardware buttons.
> >>>
> >>> Which shows that a device being a clickpad is reliable indicated by bit
> >>> 12
> >>> being set in the fw_version. I've included the gathered list inside the
> >>> driver,
> >>> so that we've this info at hand if we need to revisit this later.
> >>
> >>
> >> Duson, can you confirm this? It's not that I don't trust Hans, but if
> >> we could have the hardware maker validating this part, this would be
> >> great.
> >>
> >>>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>>   drivers/input/mouse/elantech.c | 43
> >>> +++++++++++++++++++++++++++++++++++++++---
> >>>   1 file changed, 40 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/input/mouse/elantech.c
> >>> b/drivers/input/mouse/elantech.c
> >>> index 8551dca..f7baa32 100644
> >>> --- a/drivers/input/mouse/elantech.c
> >>> +++ b/drivers/input/mouse/elantech.c
> >>> @@ -486,6 +486,7 @@ static void elantech_input_sync_v4(struct psmouse
> >>> *psmouse)
> >>>          unsigned char *packet = psmouse->packet;
> >>>
> >>>          input_report_key(dev, BTN_LEFT, packet[0] & 0x01);
> >>> +       input_report_key(dev, BTN_RIGHT, packet[0] & 0x02);
> >>>          input_mt_report_pointer_emulation(dev, true);
> >>>          input_sync(dev);
> >>>   }
> >>> @@ -984,6 +985,42 @@ static int elantech_get_resolution_v4(struct psmouse
> >>> *psmouse,
> >>>   }
> >>>
> >>>   /*
> >>> + * Advertise INPUT_PROP_BUTTONPAD for clickpads. The testing of bit 12
> >>> in
> >>> + * fw_version for this is based on the following fw_version & caps
> >>> table:
> >>> + *
> >>> + * Laptop-model:           fw_version:     caps:           buttons:
> >>> + * Acer S3                 0x461f00        10, 13, 0e      clickpad
> >>> + * Acer S7-392             0x581f01        50, 17, 0d      clickpad
> >>> + * Acer V5-131             0x461f02        01, 16, 0c      clickpad
> >>> + * Acer V5-551             0x461f00        ?               clickpad
> >>> + * Asus K53SV              0x450f01        78, 15, 0c      2 hw buttons
> >>> + * Asus G46VW              0x460f02        00, 18, 0c      2 hw buttons
> >>> + * Asus G750JX             0x360f00        00, 16, 0c      2 hw buttons
> >>> + * Asus UX31               0x361f00        20, 15, 0e      clickpad
> >>> + * Asus UX32VD             0x361f02        00, 15, 0e      clickpad
> >>> + * Avatar AVIU-145A2       0x361f00        ?               clickpad
> >>> + * Gigabyte U2442          0x450f01        58, 17, 0c      2 hw buttons
> >>> + * Lenovo L430             0x350f02        b9, 15, 0c      2 hw buttons
> >>> (*)
> >>> + * Samsung NF210           0x150b00        78, 14, 0a      2 hw buttons
> >>> + * Samsung NP770Z5E        0x575f01        10, 15, 0f      clickpad
> >>> + * Samsung NP700Z5B        0x361f06        21, 15, 0f      clickpad
> >>> + * Samsung NP900X3E-A02    0x575f03        ?               clickpad
> >>> + * Samsung NP-QX410        0x851b00        19, 14, 0c      clickpad
> >>> + * Samsung RC512           0x450f00        08, 15, 0c      2 hw buttons
> >>> + * Samsung RF710           0x450f00        ?               2 hw buttons
> >>> + * System76 Pangolin       0x250f01        ?               2 hw buttons
> >>> + * (*) + 3 trackpoint buttons
> >>> + */
> >>> +static void elantech_set_buttonpad_prop(struct psmouse *psmouse)
> >>> +{
> >>> +       struct input_dev *dev = psmouse->dev;
> >>> +       struct elantech_data *etd = psmouse->private;
> >>> +
> >>> +       if (etd->fw_version & 0x001000)
> >>> +               __set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
> >>
> >>
> >> Small question here: if the touchpad is a clickpad, should'nt we also
> >> remove the BTN_RIGHT bit too?
> >
> >
> > We could, but I don't see much value in that, and it would also require
> > if statements in the sync methods to ensure that we don't tree to send
> > a button event for a button we don't advertise.
> 
> We don't need this test in the sync method. The test is already
> implemented in input_event. So now, it's just a matter of taste
> regarding upper layers. Peter, any thoughts on this?

sorry, a bit late to the party. as Dmitry said, please don't advertise
buttons that don't exist, just makes it harder to guess on top of what we're
already guessing anyway. So removing the BTN_RIGHT bit is the right thing to
do here.

Cheers,
   Peter

> 
> Anyway, other than that:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Cheers,
> Benjamin
> 
> >
> > Regards,
> >
> > Hans
> --
> 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
>