| Message ID | 20221014111533.908-4-nyanpasu256@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: Fix incorrectly halved touchpad range on ELAN v3 touchpads | expand |
On Fri, Oct 14, 2022 at 04:15, Eirin Nya <nyanpasu256@gmail.com> 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. > > Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/ > Link: https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-void.nyanpasu256.gmail.com.beta.tailscale.net/ > Fixes: 37548659bb22 ("Input: elantech - query the min/max information beforehand too") > Signed-off-by: Eirin Nya <nyanpasu256@gmail.com> > --- > > 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-void.nyanpasu256.gmail.com.beta.tailscale.net/ I don't think it's a problem to query twice. To me the patch looks fine as is. Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > > 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
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? > > Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/ > Link: https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-void.nyanpasu256.gmail.com.beta.tailscale.net/ > Fixes: 37548659bb22 ("Input: elantech - query the min/max information beforehand too") > Signed-off-by: Eirin Nya <nyanpasu256@gmail.com> > --- > > 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-void.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.
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-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/ > > Link: https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-void.nyanpasu256.gmail.com.beta.tailscale.net/ > > Fixes: 37548659bb22 ("Input: elantech - query the min/max information beforehand too") > > Signed-off-by: Eirin Nya <nyanpasu256@gmail.com> > > --- > > > > 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-void.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
Loop Josh -----Original Message----- From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] Sent: Tuesday, November 29, 2022 1:59 AM To: Eirin Nya <nyanpasu256@gmail.com>; Phoenix Huang <phoenix@emc.com.tw> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org 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@mail.gmail.com/ > > 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@gmail.com> > > --- > > > > 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
Consulted with FW team, we suggest re-querying x/y resolution after setting absolute mode. -----Original Message----- From: phoenix [mailto:phoenix@emc.com.tw] Sent: Tuesday, November 29, 2022 11:47 AM To: 'Dmitry Torokhov' <dmitry.torokhov@gmail.com>; 'Eirin Nya' <nyanpasu256@gmail.com> Cc: 'linux-input@vger.kernel.org' <linux-input@vger.kernel.org>; 'linux-kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>; 'Josh.Chen' <josh.chen@emc.com.tw> Subject: RE: [PATCH V2 3/3] Input: elantech - Fix incorrectly halved touchpad range on ELAN v3 touchpads Loop Josh -----Original Message----- From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] Sent: Tuesday, November 29, 2022 1:59 AM To: Eirin Nya <nyanpasu256@gmail.com>; Phoenix Huang <phoenix@emc.com.tw> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org 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@mail.gmail.com/ > > 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@gmail.com> > > --- > > > > 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
> If the setting is indeed double resolution, can we simply multiply > x_max and y_max by 2 instead of re-querying it? Perhaps in some laptops, x_max or y_max is odd when resolution is doubled, and rounded up or down in half-resolution mode, so multiplying the half-resolution size by 2 results in a slightly incorrect full size. I don't know if that's the case in other touchpads, but my laptop's touchpad has even size in full-resolution (doubled) mode, meaning it doesn't round its size in half-resolution mode (so multiplying both values by 2 would work on my machine). On Wed, 30 Nov 2022 19:22:25 +0800 "phoenix" <phoenix@emc.com.tw> wrote: > Consulted with FW team, we suggest re-querying x/y resolution after > setting absolute mode. My current patch re-queries x/y resolution after setting absolute mode. If this is the process that Elan's FW team recommends, should the patch be kept as-is?
Hi Eirin, On Fri, Dec 02, 2022 at 23:42, Eirin Nya <nyanpasu256@gmail.com> wrote: >> If the setting is indeed double resolution, can we simply multiply >> x_max and y_max by 2 instead of re-querying it? > > Perhaps in some laptops, x_max or y_max is odd when resolution is > doubled, and rounded up or down in half-resolution mode, so multiplying > the half-resolution size by 2 results in a slightly incorrect full > size. I don't know if that's the case in other touchpads, but my > laptop's touchpad has even size in full-resolution (doubled) mode, > meaning it doesn't round its size in half-resolution mode (so > multiplying both values by 2 would work on my machine). > > On Wed, 30 Nov 2022 19:22:25 +0800 > "phoenix" <phoenix@emc.com.tw> wrote: > >> Consulted with FW team, we suggest re-querying x/y resolution after >> setting absolute mode. > > My current patch re-queries x/y resolution after setting absolute mode. > If this is the process that Elan's FW team recommends, should the patch > be kept as-is? Yes, I think you can keep it the way it is. Dmitry will either pick it up or suggest additional changes.
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:
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. Link: https://lore.kernel.org/linux-input/CAL57YxZNutUVxBtvbVWKMw-V2kqeVB5kTQ5BFdJmN=mdPq8Q8Q@mail.gmail.com/ Link: https://lore.kernel.org/linux-input/20221008093437.72d0f6b0@dell-void.nyanpasu256.gmail.com.beta.tailscale.net/ Fixes: 37548659bb22 ("Input: elantech - query the min/max information beforehand too") Signed-off-by: Eirin Nya <nyanpasu256@gmail.com> --- 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-void.nyanpasu256.gmail.com.beta.tailscale.net/ drivers/input/mouse/elantech.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)