diff mbox series

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

Message ID 20221014111533.908-4-nyanpasu256@gmail.com
State New
Headers
Series Input: Fix incorrectly halved touchpad range on ELAN v3 touchpads |

Commit Message

Eirin Nya Oct. 14, 2022, 11:15 a.m. UTC 
  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(-)

Comments

Mattijs Korpershoek Oct. 14, 2022, 1:26 p.m. UTC | #1 
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
Dmitry Torokhov Nov. 28, 2022, 5:57 p.m. UTC | #2 
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.
Dmitry Torokhov Nov. 28, 2022, 5:58 p.m. UTC | #3 
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
phoenix Nov. 29, 2022, 3:47 a.m. UTC | #4 
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
phoenix Nov. 30, 2022, 11:22 a.m. UTC | #5 
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
Eirin Nya Dec. 3, 2022, 7:42 a.m. UTC | #6 
> 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?
Mattijs Korpershoek Dec. 5, 2022, 1:28 p.m. UTC | #7 
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 mbox series

Patch

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: