[PATCH 0/5] Fix Elan I2C touchpads in latest generation from Lenovo

[PATCH 0/5] Fix Elan I2C touchpads in latest generation from Lenovo

[Benjamin Tissoires]

Since v4.18, we unconditionally switch the I2C capable touchpads over I2C.
In the model I had (a pre-prod t480s I guess), the touchpad was behaving
fine.
However, it occurs that later production models don't expose the clickpad
information from I2C. The Windows driver gets all the information from PS/2
so we should do the same.

The situation is even worse for the P52. Once of the query parameter function
fails, which means the touchpad doesn't even probe. This effectively kills
the touchpad, which is less than ideal.

Dmitry, I am not sure if we should take those for stable in v4.18+.
I'd like to, but given the series is 5 patches, I don't know if this
will be acceptable.
We could revert in stable df077237cf55928f5 but that would mean
distributions will have to revert the revert if they want to provide
the I2C behavior.

So, regarding stable: your call :)

Cheers,
Benjamin

Benjamin Tissoires (5):
  Input: elantech - query the min/max information beforehand too
  Input: elantech - add helper function elantech_is_buttonpad()
  dt-bindings: add more optional properties for elan_i2c touchpads
  Input: elan_i2c - do not query the info if they are provided
  Input: elantech/SMBus - export all capabilities from the PS/2 node

 .../devicetree/bindings/input/elan_i2c.txt         |   8 +
 drivers/input/mouse/elan_i2c_core.c                |  49 +++-
 drivers/input/mouse/elantech.c                     | 272 +++++++++++----------
 drivers/input/mouse/elantech.h                     |   5 +
 4 files changed, 188 insertions(+), 146 deletions(-)

-- 
2.14.3

[PATCH 1/5] Input: elantech - query the min/max information beforehand too

[Benjamin Tissoires]

For the latest generation of Elantech touchpads, we need to forward
the min/max information from PS/2 to SMBus. Prepare this work
by fetching the information before creating the SMBus companion
device.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/elantech.c | 160 +++++++++++++++++++----------------------
 drivers/input/mouse/elantech.h |   5 ++
 2 files changed, 79 insertions(+), 86 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 2d95e8d93cc7..577c795cbe82 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -994,88 +994,6 @@ static int elantech_set_absolute_mode(struct psmouse *psmouse)
 	return rc;
 }
 
-static int elantech_set_range(struct psmouse *psmouse,
-			      unsigned int *x_min, unsigned int *y_min,
-			      unsigned int *x_max, unsigned int *y_max,
-			      unsigned int *width)
-{
-	struct elantech_data *etd = psmouse->private;
-	struct elantech_device_info *info = &etd->info;
-	unsigned char param[3];
-	unsigned char traces;
-
-	switch (info->hw_version) {
-	case 1:
-		*x_min = ETP_XMIN_V1;
-		*y_min = ETP_YMIN_V1;
-		*x_max = ETP_XMAX_V1;
-		*y_max = ETP_YMAX_V1;
-		break;
-
-	case 2:
-		if (info->fw_version == 0x020800 ||
-		    info->fw_version == 0x020b00 ||
-		    info->fw_version == 0x020030) {
-			*x_min = ETP_XMIN_V2;
-			*y_min = ETP_YMIN_V2;
-			*x_max = ETP_XMAX_V2;
-			*y_max = ETP_YMAX_V2;
-		} else {
-			int i;
-			int fixed_dpi;
-
-			i = (info->fw_version > 0x020800 &&
-			     info->fw_version < 0x020900) ? 1 : 2;
-
-			if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
-				return -1;
-
-			fixed_dpi = param[1] & 0x10;
-
-			if (((info->fw_version >> 16) == 0x14) && fixed_dpi) {
-				if (info->send_cmd(psmouse, ETP_SAMPLE_QUERY, param))
-					return -1;
-
-				*x_max = (info->capabilities[1] - i) * param[1] / 2;
-				*y_max = (info->capabilities[2] - i) * param[2] / 2;
-			} else if (info->fw_version == 0x040216) {
-				*x_max = 819;
-				*y_max = 405;
-			} else if (info->fw_version == 0x040219 || info->fw_version == 0x040215) {
-				*x_max = 900;
-				*y_max = 500;
-			} else {
-				*x_max = (info->capabilities[1] - i) * 64;
-				*y_max = (info->capabilities[2] - i) * 64;
-			}
-		}
-		break;
-
-	case 3:
-		if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
-			return -1;
-
-		*x_max = (0x0f & param[0]) << 8 | param[1];
-		*y_max = (0xf0 & param[0]) << 4 | param[2];
-		break;
-
-	case 4:
-		if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
-			return -1;
-
-		*x_max = (0x0f & param[0]) << 8 | param[1];
-		*y_max = (0xf0 & param[0]) << 4 | param[2];
-		traces = info->capabilities[1];
-		if ((traces < 2) || (traces > *x_max))
-			return -1;
-
-		*width = *x_max / (traces - 1);
-		break;
-	}
-
-	return 0;
-}
-
 /*
  * (value from firmware) * 10 + 790 = dpi
  * we also have to convert dpi to dots/mm (*10/254 to avoid floating point)
@@ -1191,10 +1109,9 @@ static int elantech_set_input_params(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct elantech_data *etd = psmouse->private;
 	struct elantech_device_info *info = &etd->info;
-	unsigned int x_min = 0, y_min = 0, x_max = 0, y_max = 0, width = 0;
-
-	if (elantech_set_range(psmouse, &x_min, &y_min, &x_max, &y_max, &width))
-		return -1;
+	unsigned int x_min = info->x_min, y_min = info->y_min,
+		     x_max = info->x_max, y_max = info->y_max,
+		     width = info->width;
 
 	__set_bit(INPUT_PROP_POINTER, dev->propbit);
 	__set_bit(EV_KEY, dev->evbit);
@@ -1678,6 +1595,7 @@ static int elantech_query_info(struct psmouse *psmouse,
 			       struct elantech_device_info *info)
 {
 	unsigned char param[3];
+	unsigned char traces;
 
 	memset(info, 0, sizeof(*info));
 
@@ -1746,6 +1664,76 @@ static int elantech_query_info(struct psmouse *psmouse,
 		}
 	}
 
+	/* query range information */
+	switch (info->hw_version) {
+	case 1:
+		info->x_min = ETP_XMIN_V1;
+		info->y_min = ETP_YMIN_V1;
+		info->x_max = ETP_XMAX_V1;
+		info->y_max = ETP_YMAX_V1;
+		break;
+
+	case 2:
+		if (info->fw_version == 0x020800 ||
+		    info->fw_version == 0x020b00 ||
+		    info->fw_version == 0x020030) {
+			info->x_min = ETP_XMIN_V2;
+			info->y_min = ETP_YMIN_V2;
+			info->x_max = ETP_XMAX_V2;
+			info->y_max = ETP_YMAX_V2;
+		} else {
+			int i;
+			int fixed_dpi;
+
+			i = (info->fw_version > 0x020800 &&
+			     info->fw_version < 0x020900) ? 1 : 2;
+
+			if (info->send_cmd(psmouse, ETP_FW_ID_QUERY, param))
+				return -EINVAL;
+
+			fixed_dpi = param[1] & 0x10;
+
+			if (((info->fw_version >> 16) == 0x14) && fixed_dpi) {
+				if (info->send_cmd(psmouse, ETP_SAMPLE_QUERY, param))
+					return -EINVAL;
+
+				info->x_max = (info->capabilities[1] - i) * param[1] / 2;
+				info->y_max = (info->capabilities[2] - i) * param[2] / 2;
+			} else if (info->fw_version == 0x040216) {
+				info->x_max = 819;
+				info->y_max = 405;
+			} else if (info->fw_version == 0x040219 || info->fw_version == 0x040215) {
+				info->x_max = 900;
+				info->y_max = 500;
+			} else {
+				info->x_max = (info->capabilities[1] - i) * 64;
+				info->y_max = (info->capabilities[2] - i) * 64;
+			}
+		}
+		break;
+
+	case 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];
+		break;
+
+	case 4:
+		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];
+		traces = info->capabilities[1];
+		if ((traces < 2) || (traces > info->x_max))
+			return -EINVAL;
+
+		info->width = info->x_max / (traces - 1);
+		break;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 119727085a60..194503ed59c5 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -144,8 +144,13 @@ struct elantech_device_info {
 	unsigned char debug;
 	unsigned char hw_version;
 	unsigned int fw_version;
+	unsigned int x_min;
+	unsigned int y_min;
+	unsigned int x_max;
+	unsigned int y_max;
 	unsigned int x_res;
 	unsigned int y_res;
+	unsigned int width;
 	unsigned int bus;
 	bool paritycheck;
 	bool jumpy_cursor;
-- 
2.14.3

[PATCH 2/5] Input: elantech - add helper function elantech_is_buttonpad()

[Benjamin Tissoires]

We check for this bit all over the code, better have it defined once
for all.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/elantech.c | 89 ++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 577c795cbe82..78ac732e8188 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -229,6 +229,50 @@ static void elantech_packet_dump(struct psmouse *psmouse)
 		       psmouse->pktsize, psmouse->packet);
 }
 
+/*
+ * 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 TP500LN            0x381f17        10, 14, 0e      clickpad
+ * Asus X750JN             0x381f17        10, 14, 0e      clickpad
+ * Asus UX31               0x361f00        20, 15, 0e      clickpad
+ * Asus UX32VD             0x361f02        00, 15, 0e      clickpad
+ * Avatar AVIU-145A2       0x361f00        ?               clickpad
+ * Fujitsu LIFEBOOK E544   0x470f00        d0, 12, 09      2 hw buttons
+ * Fujitsu LIFEBOOK E546   0x470f00        50, 12, 09      2 hw buttons
+ * Fujitsu LIFEBOOK E547   0x470f00        50, 12, 09      2 hw buttons
+ * Fujitsu LIFEBOOK E554   0x570f01        40, 14, 0c      2 hw buttons
+ * Fujitsu LIFEBOOK E557   0x570f01        40, 14, 0c      2 hw buttons
+ * Fujitsu T725            0x470f01        05, 12, 09      2 hw buttons
+ * Fujitsu H730            0x570f00        c0, 14, 0c      3 hw buttons (**)
+ * Gigabyte U2442          0x450f01        58, 17, 0c      2 hw buttons
+ * Lenovo L430             0x350f02        b9, 15, 0c      2 hw buttons (*)
+ * Lenovo L530             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
+ * (**) + 0 trackpoint buttons
+ * Note: Lenovo L430 and Lenovo L530 have the same fw_version/caps
+ */
+static inline int elantech_is_buttonpad(struct elantech_device_info *info)
+{
+	return info->fw_version & 0x001000;
+}
+
 /*
  * Interpret complete data packets and report absolute mode input events for
  * hardware version 1. (4 byte packets)
@@ -526,7 +570,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
 	input_report_key(dev, BTN_TOOL_TRIPLETAP, fingers == 3);
 
 	/* For clickpads map both buttons to BTN_LEFT */
-	if (etd->info.fw_version & 0x001000)
+	if (elantech_is_buttonpad(&etd->info))
 		input_report_key(dev, BTN_LEFT, packet[0] & 0x03);
 	else
 		psmouse_report_standard_buttons(dev, packet[0]);
@@ -544,7 +588,7 @@ static void elantech_input_sync_v4(struct psmouse *psmouse)
 	unsigned char *packet = psmouse->packet;
 
 	/* For clickpads map both buttons to BTN_LEFT */
-	if (etd->info.fw_version & 0x001000)
+	if (elantech_is_buttonpad(&etd->info))
 		input_report_key(dev, BTN_LEFT, packet[0] & 0x03);
 	else
 		psmouse_report_standard_buttons(dev, packet[0]);
@@ -1020,51 +1064,12 @@ static int elantech_get_resolution_v4(struct psmouse *psmouse,
 	return 0;
 }
 
-/*
- * 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 TP500LN            0x381f17        10, 14, 0e      clickpad
- * Asus X750JN             0x381f17        10, 14, 0e      clickpad
- * Asus UX31               0x361f00        20, 15, 0e      clickpad
- * Asus UX32VD             0x361f02        00, 15, 0e      clickpad
- * Avatar AVIU-145A2       0x361f00        ?               clickpad
- * Fujitsu LIFEBOOK E544   0x470f00        d0, 12, 09      2 hw buttons
- * Fujitsu LIFEBOOK E546   0x470f00        50, 12, 09      2 hw buttons
- * Fujitsu LIFEBOOK E547   0x470f00        50, 12, 09      2 hw buttons
- * Fujitsu LIFEBOOK E554   0x570f01        40, 14, 0c      2 hw buttons
- * Fujitsu LIFEBOOK E557   0x570f01        40, 14, 0c      2 hw buttons
- * Fujitsu T725            0x470f01        05, 12, 09      2 hw buttons
- * Fujitsu H730            0x570f00        c0, 14, 0c      3 hw buttons (**)
- * Gigabyte U2442          0x450f01        58, 17, 0c      2 hw buttons
- * Lenovo L430             0x350f02        b9, 15, 0c      2 hw buttons (*)
- * Lenovo L530             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
- * (**) + 0 trackpoint buttons
- * Note: Lenovo L430 and Lenovo L530 have the same fw_version/caps
- */
 static void elantech_set_buttonpad_prop(struct psmouse *psmouse)
 {
 	struct input_dev *dev = psmouse->dev;
 	struct elantech_data *etd = psmouse->private;
 
-	if (etd->info.fw_version & 0x001000) {
+	if (elantech_is_buttonpad(&etd->info)) {
 		__set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
 		__clear_bit(BTN_RIGHT, dev->keybit);
 	}
-- 
2.14.3

[PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Benjamin Tissoires]

Some new touchpads IC are connected through PS/2 and I2C. On some of these
new IC, the I2C part doesn't have all of the information available.
We need to be able to forward the touchpad parameters from PS/2 and
thus, we need those new optional properties.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
index 797607460735..ace6bcb0b4eb 100644
--- a/Documentation/devicetree/bindings/input/elan_i2c.txt
+++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
@@ -13,6 +13,14 @@ Optional properties:
   pinctrl binding [1]).
 - vcc-supply: a phandle for the regulator supplying 3.3V power.
 - elan,trackpoint: touchpad can support a trackpoint (boolean)
+- elan,clickpad: touchpad is a clickpad (the entire surface is a button)
+- elan,max_x: the maximum reported value on the X axis
+- elan,max_y: the maximum reported value on the Y axis
+- elan,min_x: the minimum reported value on the X axis
+- elan,min_y: the minimum reported value on the Y axis
+- elan,x_res: the resolution of the X axis (in units per mm)
+- elan,y_res: the resolution of the Y axis (in units per mm)
+- elan,width: max reported width of a blob
 
 [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 [1]: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
-- 
2.14.3

Re: [PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Rob Herring]

On Fri, Oct 12, 2018 at 04:24:11PM +0200, Benjamin Tissoires wrote:
> Some new touchpads IC are connected through PS/2 and I2C. On some of these
> new IC, the I2C part doesn't have all of the information available.
> We need to be able to forward the touchpad parameters from PS/2 and
> thus, we need those new optional properties.
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
> index 797607460735..ace6bcb0b4eb 100644
> --- a/Documentation/devicetree/bindings/input/elan_i2c.txt
> +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
> @@ -13,6 +13,14 @@ Optional properties:
>    pinctrl binding [1]).
>  - vcc-supply: a phandle for the regulator supplying 3.3V power.
>  - elan,trackpoint: touchpad can support a trackpoint (boolean)
> +- elan,clickpad: touchpad is a clickpad (the entire surface is a button)

> +- elan,max_x: the maximum reported value on the X axis
> +- elan,max_y: the maximum reported value on the Y axis
> +- elan,min_x: the minimum reported value on the X axis
> +- elan,min_y: the minimum reported value on the Y axis
> +- elan,x_res: the resolution of the X axis (in units per mm)
> +- elan,y_res: the resolution of the Y axis (in units per mm)
> +- elan,width: max reported width of a blob

Can't we use standard touchscreen properties here? (Yes, I get this is a 
touchpad, not touchscreen).

Rob

Re: [PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Benjamin Tissoires]

On Wed, Oct 17, 2018 at 10:15 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 12, 2018 at 04:24:11PM +0200, Benjamin Tissoires wrote:
> > Some new touchpads IC are connected through PS/2 and I2C. On some of these
> > new IC, the I2C part doesn't have all of the information available.
> > We need to be able to forward the touchpad parameters from PS/2 and
> > thus, we need those new optional properties.
> >
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
> > index 797607460735..ace6bcb0b4eb 100644
> > --- a/Documentation/devicetree/bindings/input/elan_i2c.txt
> > +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
> > @@ -13,6 +13,14 @@ Optional properties:
> >    pinctrl binding [1]).
> >  - vcc-supply: a phandle for the regulator supplying 3.3V power.
> >  - elan,trackpoint: touchpad can support a trackpoint (boolean)
> > +- elan,clickpad: touchpad is a clickpad (the entire surface is a button)
>
> > +- elan,max_x: the maximum reported value on the X axis
> > +- elan,max_y: the maximum reported value on the Y axis
> > +- elan,min_x: the minimum reported value on the X axis
> > +- elan,min_y: the minimum reported value on the Y axis
> > +- elan,x_res: the resolution of the X axis (in units per mm)
> > +- elan,y_res: the resolution of the Y axis (in units per mm)
> > +- elan,width: max reported width of a blob
>
> Can't we use standard touchscreen properties here? (Yes, I get this is a
> touchpad, not touchscreen).

Hey Rob,

Well, there is that (it's a touchpad driver) and we can't also really
use the of_touchscreen.c implementation.
If both concerns are not an issue, we can then move the [min/max/res]
properties to the touchscreen ones.

Regarding 'elan,width', this is something missing from the standard ts
properties, and AFAICT, this controls the maximum reported
width/height of a touch.
I should probably rename them to max_width, max_height.

Hans, do you think we should add such properties to of_touchscreen.c
too? (the width/height ones)

Cheers,
Benjamin

Re: [PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Hans de Goede]

Hi,

On 18-10-18 10:10, Benjamin Tissoires wrote:
> On Wed, Oct 17, 2018 at 10:15 PM Rob Herring <robh@kernel.org> wrote:
>>
>> On Fri, Oct 12, 2018 at 04:24:11PM +0200, Benjamin Tissoires wrote:
>>> Some new touchpads IC are connected through PS/2 and I2C. On some of these
>>> new IC, the I2C part doesn't have all of the information available.
>>> We need to be able to forward the touchpad parameters from PS/2 and
>>> thus, we need those new optional properties.
>>>
>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>>   Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
>>> index 797607460735..ace6bcb0b4eb 100644
>>> --- a/Documentation/devicetree/bindings/input/elan_i2c.txt
>>> +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
>>> @@ -13,6 +13,14 @@ Optional properties:
>>>     pinctrl binding [1]).
>>>   - vcc-supply: a phandle for the regulator supplying 3.3V power.
>>>   - elan,trackpoint: touchpad can support a trackpoint (boolean)
>>> +- elan,clickpad: touchpad is a clickpad (the entire surface is a button)
>>
>>> +- elan,max_x: the maximum reported value on the X axis
>>> +- elan,max_y: the maximum reported value on the Y axis
>>> +- elan,min_x: the minimum reported value on the X axis
>>> +- elan,min_y: the minimum reported value on the Y axis
>>> +- elan,x_res: the resolution of the X axis (in units per mm)
>>> +- elan,y_res: the resolution of the Y axis (in units per mm)
>>> +- elan,width: max reported width of a blob
>>
>> Can't we use standard touchscreen properties here? (Yes, I get this is a
>> touchpad, not touchscreen).
> 
> Hey Rob,
> 
> Well, there is that (it's a touchpad driver) and we can't also really
> use the of_touchscreen.c implementation.
> If both concerns are not an issue, we can then move the [min/max/res]
> properties to the touchscreen ones.
> 
> Regarding 'elan,width', this is something missing from the standard ts
> properties, and AFAICT, this controls the maximum reported
> width/height of a touch.
> I should probably rename them to max_width, max_height.
> 
> Hans, do you think we should add such properties to of_touchscreen.c
> too? (the width/height ones)

Are there touchscreens which report finger/touch width / height ? if so
then it probably does make sense. Note that for historical reasons
Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Also the touchscreen bindings have: touchscreen-x-mm and touchscreen-y-mm
rather then res, which can then be used to calculate the resolution.

Regards,

Hans

Re: [PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Benjamin Tissoires]

On Thu, Oct 18, 2018 at 10:39 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 18-10-18 10:10, Benjamin Tissoires wrote:
> > On Wed, Oct 17, 2018 at 10:15 PM Rob Herring <robh@kernel.org> wrote:
> >>
> >> On Fri, Oct 12, 2018 at 04:24:11PM +0200, Benjamin Tissoires wrote:
> >>> Some new touchpads IC are connected through PS/2 and I2C. On some of these
> >>> new IC, the I2C part doesn't have all of the information available.
> >>> We need to be able to forward the touchpad parameters from PS/2 and
> >>> thus, we need those new optional properties.
> >>>
> >>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
> >>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >>> ---
> >>>   Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
> >>>   1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
> >>> index 797607460735..ace6bcb0b4eb 100644
> >>> --- a/Documentation/devicetree/bindings/input/elan_i2c.txt
> >>> +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
> >>> @@ -13,6 +13,14 @@ Optional properties:
> >>>     pinctrl binding [1]).
> >>>   - vcc-supply: a phandle for the regulator supplying 3.3V power.
> >>>   - elan,trackpoint: touchpad can support a trackpoint (boolean)
> >>> +- elan,clickpad: touchpad is a clickpad (the entire surface is a button)
> >>
> >>> +- elan,max_x: the maximum reported value on the X axis
> >>> +- elan,max_y: the maximum reported value on the Y axis
> >>> +- elan,min_x: the minimum reported value on the X axis
> >>> +- elan,min_y: the minimum reported value on the Y axis
> >>> +- elan,x_res: the resolution of the X axis (in units per mm)
> >>> +- elan,y_res: the resolution of the Y axis (in units per mm)
> >>> +- elan,width: max reported width of a blob
> >>
> >> Can't we use standard touchscreen properties here? (Yes, I get this is a
> >> touchpad, not touchscreen).
> >
> > Hey Rob,
> >
> > Well, there is that (it's a touchpad driver) and we can't also really
> > use the of_touchscreen.c implementation.
> > If both concerns are not an issue, we can then move the [min/max/res]
> > properties to the touchscreen ones.
> >
> > Regarding 'elan,width', this is something missing from the standard ts
> > properties, and AFAICT, this controls the maximum reported
> > width/height of a touch.
> > I should probably rename them to max_width, max_height.
> >
> > Hans, do you think we should add such properties to of_touchscreen.c
> > too? (the width/height ones)
>
> Are there touchscreens which report finger/touch width / height ? if so
> then it probably does make sense.

Well, it's pretty common for hid-multitouch touchscreens to report
such properties (it's a way to indicate the palm). Don't know about
the touchscreens that rely on of_touchscreen though.

> Note that for historical reasons
> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

Looks like your sentence is not finished here :)

>
> Also the touchscreen bindings have: touchscreen-x-mm and touchscreen-y-mm
> rather then res, which can then be used to calculate the resolution.
>

yeah, that's fine, I would need to convert to mm, then go back to res.
Extra effort, but that's the price to pay.

Cheers,
Benjamin

Re: [PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Hans de Goede]

Hi,

On 18-10-18 10:44, Benjamin Tissoires wrote:
> On Thu, Oct 18, 2018 at 10:39 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 18-10-18 10:10, Benjamin Tissoires wrote:
>>> On Wed, Oct 17, 2018 at 10:15 PM Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Fri, Oct 12, 2018 at 04:24:11PM +0200, Benjamin Tissoires wrote:
>>>>> Some new touchpads IC are connected through PS/2 and I2C. On some of these
>>>>> new IC, the I2C part doesn't have all of the information available.
>>>>> We need to be able to forward the touchpad parameters from PS/2 and
>>>>> thus, we need those new optional properties.
>>>>>
>>>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
>>>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
>>>>> index 797607460735..ace6bcb0b4eb 100644
>>>>> --- a/Documentation/devicetree/bindings/input/elan_i2c.txt
>>>>> +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
>>>>> @@ -13,6 +13,14 @@ Optional properties:
>>>>>      pinctrl binding [1]).
>>>>>    - vcc-supply: a phandle for the regulator supplying 3.3V power.
>>>>>    - elan,trackpoint: touchpad can support a trackpoint (boolean)
>>>>> +- elan,clickpad: touchpad is a clickpad (the entire surface is a button)
>>>>
>>>>> +- elan,max_x: the maximum reported value on the X axis
>>>>> +- elan,max_y: the maximum reported value on the Y axis
>>>>> +- elan,min_x: the minimum reported value on the X axis
>>>>> +- elan,min_y: the minimum reported value on the Y axis
>>>>> +- elan,x_res: the resolution of the X axis (in units per mm)
>>>>> +- elan,y_res: the resolution of the Y axis (in units per mm)
>>>>> +- elan,width: max reported width of a blob
>>>>
>>>> Can't we use standard touchscreen properties here? (Yes, I get this is a
>>>> touchpad, not touchscreen).
>>>
>>> Hey Rob,
>>>
>>> Well, there is that (it's a touchpad driver) and we can't also really
>>> use the of_touchscreen.c implementation.
>>> If both concerns are not an issue, we can then move the [min/max/res]
>>> properties to the touchscreen ones.
>>>
>>> Regarding 'elan,width', this is something missing from the standard ts
>>> properties, and AFAICT, this controls the maximum reported
>>> width/height of a touch.
>>> I should probably rename them to max_width, max_height.
>>>
>>> Hans, do you think we should add such properties to of_touchscreen.c
>>> too? (the width/height ones)
>>
>> Are there touchscreens which report finger/touch width / height ? if so
>> then it probably does make sense.
> 
> Well, it's pretty common for hid-multitouch touchscreens to report
> such properties (it's a way to indicate the palm). Don't know about
> the touchscreens that rely on of_touchscreen though.

Now that you mention it I think some may also have some sort of
pressure/weight value, but at least for the ones I wrote I do not
think we do anything with it, since the actual meaning of the
field is somewhat vague. This is all IIRC.

>> Note that for historical reasons
>> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> 
> Looks like your sentence is not finished here :)

I actually deleted it, but not those 2 lines, what I had written there
was that for historical reasons it uses touchscreen-size-x rather then
max-x and that that might be a bit confusing vs max-width, but I could
not come up with something better then max-width, so I deleted my
rambling, but ended up not deleting all of it :)

>> Also the touchscreen bindings have: touchscreen-x-mm and touchscreen-y-mm
>> rather then res, which can then be used to calculate the resolution.
>>
> 
> yeah, that's fine, I would need to convert to mm, then go back to res.
> Extra effort, but that's the price to pay.

Ack.

Regards,

Hans

Re: [PATCH 3/5] dt-bindings: add more optional properties for elan_i2c touchpads

[Rob Herring]

On Thu, Oct 18, 2018 at 3:10 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Oct 17, 2018 at 10:15 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Oct 12, 2018 at 04:24:11PM +0200, Benjamin Tissoires wrote:
> > > Some new touchpads IC are connected through PS/2 and I2C. On some of these
> > > new IC, the I2C part doesn't have all of the information available.
> > > We need to be able to forward the touchpad parameters from PS/2 and
> > > thus, we need those new optional properties.
> > >
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >  Documentation/devicetree/bindings/input/elan_i2c.txt | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
> > > index 797607460735..ace6bcb0b4eb 100644
> > > --- a/Documentation/devicetree/bindings/input/elan_i2c.txt
> > > +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
> > > @@ -13,6 +13,14 @@ Optional properties:
> > >    pinctrl binding [1]).
> > >  - vcc-supply: a phandle for the regulator supplying 3.3V power.
> > >  - elan,trackpoint: touchpad can support a trackpoint (boolean)
> > > +- elan,clickpad: touchpad is a clickpad (the entire surface is a button)
> >
> > > +- elan,max_x: the maximum reported value on the X axis
> > > +- elan,max_y: the maximum reported value on the Y axis
> > > +- elan,min_x: the minimum reported value on the X axis
> > > +- elan,min_y: the minimum reported value on the Y axis
> > > +- elan,x_res: the resolution of the X axis (in units per mm)
> > > +- elan,y_res: the resolution of the Y axis (in units per mm)
> > > +- elan,width: max reported width of a blob
> >
> > Can't we use standard touchscreen properties here? (Yes, I get this is a
> > touchpad, not touchscreen).
>
> Hey Rob,
>
> Well, there is that (it's a touchpad driver) and we can't also really
> use the of_touchscreen.c implementation.
> If both concerns are not an issue, we can then move the [min/max/res]
> properties to the touchscreen ones.

Neither is problem for me. Bindings don't have to map 1:1 to kernel code.

> Regarding 'elan,width', this is something missing from the standard ts
> properties, and AFAICT, this controls the maximum reported
> width/height of a touch.
> I should probably rename them to max_width, max_height.

Sure, just use '-' rather than '_' in property names.

> Hans, do you think we should add such properties to of_touchscreen.c
> too? (the width/height ones)
>
> Cheers,
> Benjamin

[PATCH 4/5] Input: elan_i2c - do not query the info if they are provided

[Benjamin Tissoires]

See the previous patch for a long explanation.

TL;DR: the P52 and the t480s from Lenovo can't rely on I2C to fetch
the information, so we need it from PS/2.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/elan_i2c_core.c | 49 +++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f5ae24865355..5e7ba383f95c 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -366,27 +366,50 @@ static unsigned int elan_convert_resolution(u8 val)
 
 static int elan_query_device_parameters(struct elan_tp_data *data)
 {
+	struct i2c_client *client = data->client;
 	unsigned int x_traces, y_traces;
 	u8 hw_x_res, hw_y_res;
 	int error;
 
-	error = data->ops->get_max(data->client, &data->max_x, &data->max_y);
-	if (error)
-		return error;
+	if (device_property_read_u32(&client->dev,
+				     "elan,max_x", &data->max_x) ||
+	    device_property_read_u32(&client->dev,
+				     "elan,max_y", &data->max_y)) {
+		error = data->ops->get_max(data->client,
+					   &data->max_x,
+					   &data->max_y);
+		if (error)
+			return error;
+	}
 
-	error = data->ops->get_num_traces(data->client, &x_traces, &y_traces);
-	if (error)
-		return error;
+	if (device_property_read_u32(&client->dev,
+				     "elan,width", &data->width_x)) {
+		error = data->ops->get_num_traces(data->client,
+						  &x_traces, &y_traces);
+		if (error)
+			return error;
 
-	data->width_x = data->max_x / x_traces;
-	data->width_y = data->max_y / y_traces;
+		data->width_x = data->max_x / x_traces;
+		data->width_y = data->max_y / y_traces;
+	} else {
+		data->width_y = data->width_x;
+	}
 
-	error = data->ops->get_resolution(data->client, &hw_x_res, &hw_y_res);
-	if (error)
-		return error;
+	if (device_property_read_u32(&client->dev,
+				     "elan,x_res", &data->x_res) ||
+	    device_property_read_u32(&client->dev,
+				     "elan,y_res", &data->y_res)) {
+		error = data->ops->get_resolution(data->client,
+						  &hw_x_res, &hw_y_res);
+		if (error)
+			return error;
+
+		data->x_res = elan_convert_resolution(hw_x_res);
+		data->y_res = elan_convert_resolution(hw_y_res);
+	}
 
-	data->x_res = elan_convert_resolution(hw_x_res);
-	data->y_res = elan_convert_resolution(hw_y_res);
+	if (device_property_read_bool(&client->dev, "elan,clickpad"))
+		data->clickpad = 1;
 
 	return 0;
 }
-- 
2.14.3

[PATCH 5/5] Input: elantech/SMBus - export all capabilities from the PS/2 node

[Benjamin Tissoires]

The recent touchpads might not have all the information regarding the
characteristics through the I2C port.
On some Lenovo t480s, this results in the touchpad not being detected
as a clickpad, and on the Lenovo P52, this results in a failure while
fetching the resolution through I2C.

We need to imitate the Windows behavior: fetch the data under PS/2, and
limit the querries under I2C.

This patch exports the needed info from PS/2.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1628715
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/elantech.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 78ac732e8188..f597b5d14de8 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -1764,17 +1764,30 @@ static int elantech_create_smbus(struct psmouse *psmouse,
 				 struct elantech_device_info *info,
 				 bool leave_breadcrumbs)
 {
-	const struct property_entry i2c_properties[] = {
-		PROPERTY_ENTRY_BOOL("elan,trackpoint"),
-		{ },
-	};
+	struct property_entry i2c_props[10] = {};
 	struct i2c_board_info smbus_board = {
 		I2C_BOARD_INFO("elan_i2c", 0x15),
 		.flags = I2C_CLIENT_HOST_NOTIFY,
 	};
+	unsigned int prop_idx = 0;
+
+	smbus_board.properties = i2c_props;
+
+	i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,max_x", info->x_max);
+	i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,max_y", info->y_max);
+	i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,min_x", info->x_min);
+	i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,min_y", info->y_min);
+	i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,x_res", info->x_res);
+	i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,y_res", info->y_res);
 
 	if (info->has_trackpoint)
-		smbus_board.properties = i2c_properties;
+		i2c_props[prop_idx++] = PROPERTY_ENTRY_BOOL("elan,trackpoint");
+
+	if (info->width)
+		i2c_props[prop_idx++] = PROPERTY_ENTRY_U32("elan,width", info->width);
+
+	if (elantech_is_buttonpad(info))
+		i2c_props[prop_idx++] = PROPERTY_ENTRY_BOOL("elan,clickpad");
 
 	return psmouse_smbus_init(psmouse, &smbus_board, NULL, 0, false,
 				  leave_breadcrumbs);
-- 
2.14.3

Re: [PATCH 0/5] Fix Elan I2C touchpads in latest generation from Lenovo

[Dmitry Torokhov]

On Fri, Oct 12, 2018 at 04:24:08PM +0200, Benjamin Tissoires wrote:
> Since v4.18, we unconditionally switch the I2C capable touchpads over I2C.
> In the model I had (a pre-prod t480s I guess), the touchpad was behaving
> fine.
> However, it occurs that later production models don't expose the clickpad
> information from I2C. The Windows driver gets all the information from PS/2
> so we should do the same.
> 
> The situation is even worse for the P52. Once of the query parameter function
> fails, which means the touchpad doesn't even probe. This effectively kills
> the touchpad, which is less than ideal.
> 
> Dmitry, I am not sure if we should take those for stable in v4.18+.
> I'd like to, but given the series is 5 patches, I don't know if this
> will be acceptable.
> We could revert in stable df077237cf55928f5 but that would mean
> distributions will have to revert the revert if they want to provide
> the I2C behavior.
> 
> So, regarding stable: your call :)

Heh ;) I think we have to fix it at least in 4.19 stable train. How
about we merge it normally into 4.20, and let it cook there for a while.
If there is no issues, then we can send it to 4.19 stable manually. How
does this sound?

Thanks.

-- 
Dmitry

Re: [PATCH 0/5] Fix Elan I2C touchpads in latest generation from Lenovo

[Benjamin Tissoires]

On Fri, Oct 12, 2018 at 8:53 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Oct 12, 2018 at 04:24:08PM +0200, Benjamin Tissoires wrote:
> > Since v4.18, we unconditionally switch the I2C capable touchpads over I2C.
> > In the model I had (a pre-prod t480s I guess), the touchpad was behaving
> > fine.
> > However, it occurs that later production models don't expose the clickpad
> > information from I2C. The Windows driver gets all the information from PS/2
> > so we should do the same.
> >
> > The situation is even worse for the P52. Once of the query parameter function
> > fails, which means the touchpad doesn't even probe. This effectively kills
> > the touchpad, which is less than ideal.
> >
> > Dmitry, I am not sure if we should take those for stable in v4.18+.
> > I'd like to, but given the series is 5 patches, I don't know if this
> > will be acceptable.
> > We could revert in stable df077237cf55928f5 but that would mean
> > distributions will have to revert the revert if they want to provide
> > the I2C behavior.
> >
> > So, regarding stable: your call :)
>
> Heh ;) I think we have to fix it at least in 4.19 stable train. How
> about we merge it normally into 4.20, and let it cook there for a while.
> If there is no issues, then we can send it to 4.19 stable manually. How
> does this sound?
>

Sounds like a plan :)

Cheers,
Benjamin