RE: [PATCH V2 3/3] Input: elantech - Fix incorrectly halved touchpad range on ELAN v3 touchpads

From: phoenix
Date: Mon Nov 28 2022 - 22:49:08 EST


Loop Josh

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
Sent: Tuesday, November 29, 2022 1:59 AM
To: Eirin Nya <nyanpasu256@xxxxxxxxx>; Phoenix Huang <phoenix@xxxxxxxxxx>
Cc: linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH V2 3/3] Input: elantech - Fix incorrectly halved
touchpad range on ELAN v3 touchpads

On Mon, Nov 28, 2022 at 09:57:51AM -0800, Dmitry Torokhov wrote:
> On Fri, Oct 14, 2022 at 04:15:33AM -0700, Eirin Nya wrote:
> > On Linux 5.19.10, on my laptop (Dell Inspiron 15R SE 7520) with an
> > Elan
> > v3 touchpad (dmesg says "with firmware version 0x450f02"), the
> > reported size of my touchpad (in userspace by calling
> > mtdev_configure() and libevdev_get_abs_maximum(), in kernel space
> > elantech_device_info::x_max/y_max, either way 1470 by 700) is half
> > that of the actual touch range (2940 by 1400), and the upper half of
> > my touchpad reports negative values. As a result, with the Synaptics
> > or libinput X11 driver set to edge scrolling mode, the entire right
> > half of my touchpad has x-values past evdev's reported maximum size,
> > and acts as a giant scrollbar!
> >
> > The problem is that elantech_setup_ps2() ->
> > elantech_set_absolute_mode() sets up absolute mode and doubles the
> > hardware resolution (doubling the hardware's maximum reported x/y
> > coordinates and its response to ETP_FW_ID_QUERY), *after*
> > elantech_query_info() fetches the touchpad coordinate system size
> > using ETP_FW_ID_QUERY, which gets cached and reported to userspace
> > through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the touchpad size
> > reported to userspace (and used to subtract vertical coordinates from)
is half the maximum position of actual touches.
> >
> > This patch splits out a function elantech_query_range_v3() which
> > fetches
> > *only* ETP_FW_ID_QUERY (touchpad size), and calls it a second time
> > if
> > elantech_set_absolute_mode() enables double-size mode. This means
> > the first call is redundant and wasted if a second call occurs, but
> > this minimizes the need to restructure the driver.
>
> If the setting is indeed double resolution, can we simply multiply
> x_max and y_max by 2 instead of re-querying it?
>
> Also let's try adding one of Elan engineers for their take in this.
> Phoenix, do you have any suggestions please?

Argh, adding Phoenix for real now.

>
> >
> > Link:
> > https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5
> > kTQ5BFdJmN=mdPq8Q8Q@xxxxxxxxxxxxxx/
> > Link:
> > https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-voi
> > d.nyanpasu256.gmail.com.beta.tailscale.net/
> > Fixes: 37548659bb22 ("Input: elantech - query the min/max
> > information beforehand too")
> > Signed-off-by: Eirin Nya <nyanpasu256@xxxxxxxxx>
> > ---
> >
> > Notes:
> > Should we move (elantech_set_absolute_mode ->
> > elantech_write_reg(...0x0b or 0x01)) *earlier* into
elantech_query_info()
> > before "query range information"? See discussion at
> >
> > https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-voi
> > d.nyanpasu256.gmail.com.beta.tailscale.net/
> >
> > drivers/input/mouse/elantech.c | 30 ++++++++++++++++++++++++++----
> > 1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elantech.c
> > b/drivers/input/mouse/elantech.c index 263779c031..a2176f0fd3 100644
> > --- a/drivers/input/mouse/elantech.c
> > +++ b/drivers/input/mouse/elantech.c
> > @@ -1006,6 +1006,9 @@ static void
elantech_set_rate_restore_reg_07(struct psmouse *psmouse,
> > psmouse_err(psmouse, "restoring reg_07 failed\n"); }
> >
> > +static int elantech_query_range_v3(struct psmouse *psmouse,
> > + struct elantech_device_info *info);
> > +
> > /*
> > * Put the touchpad into absolute mode
> > */
> > @@ -1047,6 +1050,14 @@ static int elantech_set_absolute_mode(struct
psmouse *psmouse)
> > if (elantech_write_reg(psmouse, 0x10, etd->reg_10))
> > rc = -1;
> >
> > + /*
> > + * If we boost hardware resolution, we have to re-query
> > + * info->x_max and y_max.
> > + */
> > + if (etd->info.set_hw_resolution)
> > + if (elantech_query_range_v3(psmouse, &etd->info))
> > + rc = -1;
> > +
> > break;
> >
> > case 4:
> > @@ -1671,6 +1682,20 @@ static int elantech_set_properties(struct
elantech_device_info *info)
> > return 0;
> > }
> >
> > +static int elantech_query_range_v3(struct psmouse *psmouse,
> > + struct elantech_device_info *info) {
> > + unsigned char param[3];
> > +
> > + if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
> > + return -EINVAL;
> > +
> > + info->x_max = (0x0f & param[0]) << 8 | param[1];
> > + info->y_max = (0xf0 & param[0]) << 4 | param[2];
> > +
> > + return 0;
> > +}
> > +
> > static int elantech_query_info(struct psmouse *psmouse,
> > struct elantech_device_info *info) { @@
-1826,11 +1851,8
> > @@ static int elantech_query_info(struct psmouse *psmouse,
> > break;
> >
> > case 3:
> > - if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
> > + if (elantech_query_range_v3(psmouse, info))
> > return -EINVAL;
> > -
> > - info->x_max = (0x0f & param[0]) << 8 | param[1];
> > - info->y_max = (0xf0 & param[0]) << 4 | param[2];
> > break;
> >
> > case 4:
> > --
> > 2.38.0
> >
>
> Thanks.
>
> --
> Dmitry

--
Dmitry