[PATCH v2 00/10] Fix Elan I2C touchpads in latest generation from Lenovo

[PATCH v2 00/10] Fix Elan I2C touchpads in latest generation from Lenovo

[Benjamin Tissoires]

Hi,

This is the v2 from https://lkml.org/lkml/2018/10/12/633

So I initially thought it would be easy to integrate the suggested changes
in the v1, but it turns our that the changes to have the touchscreen-width
and height parameters were quite hard to do.

I finally postponed the issue by blacklisting the 2 laptops we knew were
not working and tried to devote more time to understand both drivers more.

But it s the time where Lenovo is preparing the new models, and guess what,
they suffer from the same symptoms.

So I took a few time to work on this and finally got my head around the
width and height problem. Once I got it, it was simple clear, but this also
means we can not really rely on a device tree property for that.

So in the elan* drivers, the "traces" are simply how many antennas there
are on each axis. Which means that if a trace of 4 is reported in the
events, it means it is simply seen by 4 antennas. So the computation of the
width/height is the following: we take how many antennas there are, we
subtract one to have the number of holes between the antennas, and we
divide the number of unit we have in the axis by the value we just
computed.
This gives a rough 4mm on the P52, in both directions.

And once you get that, you can just realize that the unit of the width and
height are just the same than the X and Y coordinates, so we can apply the
same resolution.

So, in the end, that means that elan_i2c needs the information, or it will
not be able to convert the number of crossed antennas into a size, but this
is something specific to this touchpad.

So here come, 7 months later the v2 on the subject.

Cheers,
Benjamin

Benjamin Tissoires (10):
  Input: elantech - query the min/max information beforehand too
  Input: elantech - add helper function elantech_is_buttonpad()
  Input: elantech - detect middle button based on firmware version
  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
  Input: elan_i2c - handle physical middle button
  Input: elan_i2c - export true width/height
  Input: elan_i2c - correct the width/size base value
  Input: elantech: remove P52 from SMBus blacklist

 .../devicetree/bindings/input/elan_i2c.txt    |  11 +
 drivers/input/mouse/elan_i2c_core.c           |  85 +++--
 drivers/input/mouse/elantech.c                | 318 ++++++++++--------
 drivers/input/mouse/elantech.h                |   8 +
 4 files changed, 251 insertions(+), 171 deletions(-)

-- 
2.21.0

[PATCH v2 01/10] 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.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

no changes in v2
---
 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 a7f8b1614559..5953c21774d7 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)
@@ -1200,10 +1118,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);
@@ -1687,6 +1604,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));
 
@@ -1755,6 +1673,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.21.0

[PATCH v2 02/10] 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.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

changes in v2:
- updated with latest upstream
---
 drivers/input/mouse/elantech.c | 93 ++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 44 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 5953c21774d7..34b96b96fc96 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -229,6 +229,52 @@ 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 CELSIUS H760    0x570f02        40, 14, 0c      3 hw buttons (**)
+ * Fujitsu CELSIUS H780    0x5d0f02        41, 16, 0d      3 hw buttons (**)
+ * 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 +572,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 +590,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,53 +1066,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 CELSIUS H760    0x570f02        40, 14, 0c      3 hw buttons (**)
- * Fujitsu CELSIUS H780    0x5d0f02        41, 16, 0d      3 hw buttons (**)
- * 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.21.0

[PATCH v2 03/10] Input: elantech - detect middle button based on firmware version

[Benjamin Tissoires]

Looks like the new generation of Lenovo machine also need to
be added to the PnPID whitelist. This is definitively not going
to scale, as there is nothing that tells us currently if a
touchpad supports a true physical middle button.

Consider that all new touchpads that are not clickpads
(so matching ETP_NEW_IC_SMBUS_HOST_NOTIFY) are handling 3 physical
buttons.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

new in v2
---
 drivers/input/mouse/elantech.c | 16 ++++++----------
 drivers/input/mouse/elantech.h |  1 +
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 34b96b96fc96..057a3cf01eec 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -1107,14 +1107,6 @@ static const struct dmi_system_id elantech_dmi_has_middle_button[] = {
 	{ }
 };
 
-static const char * const middle_button_pnp_ids[] = {
-	"LEN2131", /* ThinkPad P52 w/ NFC */
-	"LEN2132", /* ThinkPad P52 */
-	"LEN2133", /* ThinkPad P72 w/ NFC */
-	"LEN2134", /* ThinkPad P72 */
-	NULL
-};
-
 /*
  * Set the appropriate event bits for the input subsystem
  */
@@ -1133,8 +1125,7 @@ static int elantech_set_input_params(struct psmouse *psmouse)
 	__clear_bit(EV_REL, dev->evbit);
 
 	__set_bit(BTN_LEFT, dev->keybit);
-	if (dmi_check_system(elantech_dmi_has_middle_button) ||
-			psmouse_matches_pnp_id(psmouse, middle_button_pnp_ids))
+	if (info->has_middle_button)
 		__set_bit(BTN_MIDDLE, dev->keybit);
 	__set_bit(BTN_RIGHT, dev->keybit);
 
@@ -1748,6 +1739,11 @@ static int elantech_query_info(struct psmouse *psmouse,
 		break;
 	}
 
+	/* check for the middle button: DMI matching or new v4 firmwares */
+	info->has_middle_button = dmi_check_system(elantech_dmi_has_middle_button) ||
+				  (ETP_NEW_IC_SMBUS_HOST_NOTIFY(info->fw_version) &&
+				   !elantech_is_buttonpad(info));
+
 	return 0;
 }
 
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 194503ed59c5..16174b54ffc3 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -158,6 +158,7 @@ struct elantech_device_info {
 	bool crc_enabled;
 	bool set_hw_resolution;
 	bool has_trackpoint;
+	bool has_middle_button;
 	int (*send_cmd)(struct psmouse *psmouse, unsigned char c,
 			unsigned char *param);
 };
-- 
2.21.0

[PATCH v2 04/10] 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.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

changes in v2:
- Use of generic touchscreen properties for min/max and resolutions
- add elan,middle-button
- add elan,*_traces
---
 Documentation/devicetree/bindings/input/elan_i2c.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
index 797607460735..9963247706f2 100644
--- a/Documentation/devicetree/bindings/input/elan_i2c.txt
+++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
@@ -13,9 +13,20 @@ 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,middle-button: touchpad has a physical middle button
+- elan,x_traces: number of antennas on the x axis
+- elan,y_traces: number of antennas on the y axis
+- some generic touchscreen properties [2]:
+  * touchscreen-size-x
+  * touchscreen-size-y
+  * touchscreen-x-mm
+  * touchscreen-y-mm
+
 
 [0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 [1]: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
 
 Example:
 	&i2c1 {
-- 
2.21.0

[PATCH v2 05/10] 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.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

changes in v2:
- updated accroding to previous patch
---
 drivers/input/mouse/elan_i2c_core.c | 56 ++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f9525d6f0bfe..53cac610ba33 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -366,27 +366,59 @@ 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;
+	u32 x_mm, y_mm;
 	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;
-
-	error = data->ops->get_num_traces(data->client, &x_traces, &y_traces);
-	if (error)
-		return error;
+	if (device_property_read_u32(&client->dev,
+				     "touchscreen-size-x", &data->max_x) ||
+	    device_property_read_u32(&client->dev,
+				     "touchscreen-size-y", &data->max_y)) {
+		error = data->ops->get_max(data->client,
+					   &data->max_x,
+					   &data->max_y);
+		if (error)
+			return error;
+	} else {
+		/* size is the maximum + 1 */
+		--data->max_x;
+		--data->max_y;
+	}
 
+	if (device_property_read_u32(&client->dev,
+				     "elan,x_traces",
+				     &x_traces) ||
+	    device_property_read_u32(&client->dev,
+				     "elan,y_traces",
+				     &y_traces)) {
+		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;
 
-	error = data->ops->get_resolution(data->client, &hw_x_res, &hw_y_res);
-	if (error)
-		return error;
+	if (device_property_read_u32(&client->dev,
+				     "touchscreen-x-mm", &x_mm) ||
+	    device_property_read_u32(&client->dev,
+				     "touchscreen-y-mm", &y_mm)) {
+		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);
+	} else {
+		data->x_res = (data->max_x + 1) / x_mm;
+		data->y_res = (data->max_y + 1) / y_mm;
+	}
 
-	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.21.0

[PATCH v2 06/10] 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 prepares this by exporting the info from PS/2.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

changes in v2:
- updated according to the 2 previous patches
---
 drivers/input/mouse/elantech.c | 47 ++++++++++++++++++++++++++++++----
 drivers/input/mouse/elantech.h |  2 ++
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 057a3cf01eec..ca10fd97d9d5 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -1736,6 +1736,15 @@ static int elantech_query_info(struct psmouse *psmouse,
 			return -EINVAL;
 
 		info->width = info->x_max / (traces - 1);
+
+		/* column number of traces */
+		info->x_traces = traces;
+
+		/* row number of traces */
+		traces = info->capabilities[2];
+		if ((traces >= 2) && (traces <= info->y_max))
+			info->y_traces = traces;
+
 		break;
 	}
 
@@ -1781,17 +1790,45 @@ 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[11] = {};
 	struct i2c_board_info smbus_board = {
 		I2C_BOARD_INFO("elan_i2c", 0x15),
 		.flags = I2C_CLIENT_HOST_NOTIFY,
 	};
+	unsigned int idx = 0;
+
+	smbus_board.properties = i2c_props;
+
+	i2c_props[idx++] = PROPERTY_ENTRY_U32("touchscreen-size-x",
+						   info->x_max + 1);
+	i2c_props[idx++] = PROPERTY_ENTRY_U32("touchscreen-size-y",
+						   info->y_max + 1);
+	i2c_props[idx++] = PROPERTY_ENTRY_U32("touchscreen-min-x",
+						   info->x_min);
+	i2c_props[idx++] = PROPERTY_ENTRY_U32("touchscreen-min-y",
+						   info->y_min);
+	if (info->x_res)
+		i2c_props[idx++] = PROPERTY_ENTRY_U32("touchscreen-x-mm",
+						      (info->x_max + 1) / info->x_res);
+	if (info->y_res)
+		i2c_props[idx++] = PROPERTY_ENTRY_U32("touchscreen-y-mm",
+						      (info->y_max + 1) / info->y_res);
 
 	if (info->has_trackpoint)
-		smbus_board.properties = i2c_properties;
+		i2c_props[idx++] = PROPERTY_ENTRY_BOOL("elan,trackpoint");
+
+	if (info->has_middle_button)
+		i2c_props[idx++] = PROPERTY_ENTRY_BOOL("elan,middle-button");
+
+	if (info->x_traces)
+		i2c_props[idx++] = PROPERTY_ENTRY_U32("elan,x_traces",
+						      info->x_traces);
+	if (info->y_traces)
+		i2c_props[idx++] = PROPERTY_ENTRY_U32("elan,y_traces",
+						      info->y_traces);
+
+	if (elantech_is_buttonpad(info))
+		i2c_props[idx++] = PROPERTY_ENTRY_BOOL("elan,clickpad");
 
 	return psmouse_smbus_init(psmouse, &smbus_board, NULL, 0, false,
 				  leave_breadcrumbs);
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 16174b54ffc3..a7eaa62af6a0 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -150,6 +150,8 @@ struct elantech_device_info {
 	unsigned int y_max;
 	unsigned int x_res;
 	unsigned int y_res;
+	unsigned int x_traces;
+	unsigned int y_traces;
 	unsigned int width;
 	unsigned int bus;
 	bool paritycheck;
-- 
2.21.0

[PATCH v2 07/10] Input: elan_i2c - handle physical middle button

[Benjamin Tissoires]

Some models have a middle button, we should export it as well.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

new in v2

Is it really worth having a separate property, or should we just expose a
middle button whatsoever?
---
 drivers/input/mouse/elan_i2c_core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 53cac610ba33..7ff044c6cd11 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -99,6 +99,7 @@ struct elan_tp_data {
 	u8			max_baseline;
 	bool			baseline_ready;
 	u8			clickpad;
+	bool			middle_button;
 };
 
 static int elan_get_fwinfo(u16 ic_type, u16 *validpage_count,
@@ -420,6 +421,9 @@ static int elan_query_device_parameters(struct elan_tp_data *data)
 	if (device_property_read_bool(&client->dev, "elan,clickpad"))
 		data->clickpad = 1;
 
+	if (device_property_read_bool(&client->dev, "elan,middle-button"))
+		data->middle_button = true;
+
 	return 0;
 }
 
@@ -958,8 +962,9 @@ static void elan_report_absolute(struct elan_tp_data *data, u8 *packet)
 			finger_data += ETP_FINGER_DATA_LEN;
 	}
 
-	input_report_key(input, BTN_LEFT, tp_info & 0x01);
-	input_report_key(input, BTN_RIGHT, tp_info & 0x02);
+	input_report_key(input, BTN_LEFT,   tp_info & BIT(0));
+	input_report_key(input, BTN_MIDDLE, tp_info & BIT(2));
+	input_report_key(input, BTN_RIGHT,  tp_info & BIT(1));
 	input_report_abs(input, ABS_DISTANCE, hover_event != 0);
 	input_mt_report_pointer_emulation(input, true);
 	input_sync(input);
@@ -1091,10 +1096,13 @@ static int elan_setup_input_device(struct elan_tp_data *data)
 
 	__set_bit(EV_ABS, input->evbit);
 	__set_bit(INPUT_PROP_POINTER, input->propbit);
-	if (data->clickpad)
+	if (data->clickpad) {
 		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
-	else
+	} else {
 		__set_bit(BTN_RIGHT, input->keybit);
+		if (data->middle_button)
+			__set_bit(BTN_MIDDLE, input->keybit);
+	}
 	__set_bit(BTN_LEFT, input->keybit);
 
 	/* Set up ST parameters */
-- 
2.21.0

[PATCH v2 08/10] Input: elan_i2c - export true width/height

[Benjamin Tissoires]

The width/height is actually in the same unit than X and Y. So we should
not tamper the data, but just set the proper resolution, so that userspace
can correctly detect which touch is a palm or a finger.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

new in v2
---
 drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 7ff044c6cd11..6f4feedb7765 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -45,7 +45,6 @@
 #define DRIVER_NAME		"elan_i2c"
 #define ELAN_VENDOR_ID		0x04f3
 #define ETP_MAX_PRESSURE	255
-#define ETP_FWIDTH_REDUCE	90
 #define ETP_FINGER_WIDTH	15
 #define ETP_RETRY_COUNT		3
 
@@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
 			return;
 		}
 
-		/*
-		 * To avoid treating large finger as palm, let's reduce the
-		 * width x and y per trace.
-		 */
-		area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
-		area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
+		area_x = mk_x * data->width_x;
+		area_y = mk_y * data->width_y;
 
 		major = max(area_x, area_y);
 		minor = min(area_x, area_y);
@@ -1123,8 +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data)
 			     ETP_MAX_PRESSURE, 0, 0);
 	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
 			     ETP_FINGER_WIDTH * max_width, 0, 0);
+	input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
 	input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
 			     ETP_FINGER_WIDTH * min_width, 0, 0);
+	input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
 
 	data->input = input;
 
-- 
2.21.0

[PATCH v2 09/10] Input: elan_i2c - correct the width/size base value

[Benjamin Tissoires]

*_traces are the number of antennas. width_* is thus the space between 2
antennas. Which means, we should subtract 1 to the number of antennas
to divide the touchpad by the number of holes between each antenna.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

new in v2
---
 drivers/input/mouse/elan_i2c_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 6f4feedb7765..3375eaa9a72e 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -398,8 +398,8 @@ static int elan_query_device_parameters(struct elan_tp_data *data)
 		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 - 1);
+	data->width_y = data->max_y / (y_traces - 1);
 
 	if (device_property_read_u32(&client->dev,
 				     "touchscreen-x-mm", &x_mm) ||
-- 
2.21.0

[PATCH v2 10/10] Input: elantech: remove P52 from SMBus blacklist

[Benjamin Tissoires]

The P52 now works correctly over SMBus. Let's use this driver so we can
update all five fingers every 8ms.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

new in v2
---
 drivers/input/mouse/elantech.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index ca10fd97d9d5..4a41e5f3074e 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -1779,8 +1779,6 @@ static const char * const i2c_blacklist_pnp_ids[] = {
 	 * These are known to not be working properly as bits are missing
 	 * in elan_i2c.
 	 */
-	"LEN2131", /* ThinkPad P52 w/ NFC */
-	"LEN2132", /* ThinkPad P52 */
 	"LEN2133", /* ThinkPad P72 w/ NFC */
 	"LEN2134", /* ThinkPad P72 */
 	NULL
-- 
2.21.0

Re: [PATCH v2 00/10] Fix Elan I2C touchpads in latest generation from Lenovo

[Benjamin Tissoires]

Hi,

On Tue, May 21, 2019 at 3:27 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> This is the v2 from https://lkml.org/lkml/2018/10/12/633
>
> So I initially thought it would be easy to integrate the suggested changes
> in the v1, but it turns our that the changes to have the touchscreen-width
> and height parameters were quite hard to do.
>
> I finally postponed the issue by blacklisting the 2 laptops we knew were
> not working and tried to devote more time to understand both drivers more.
>
> But it s the time where Lenovo is preparing the new models, and guess what,
> they suffer from the same symptoms.

I just received the confirmation from Lenovo that this series makes
the new laptops working.

Cheers,
Benjamin

>
> So I took a few time to work on this and finally got my head around the
> width and height problem. Once I got it, it was simple clear, but this also
> means we can not really rely on a device tree property for that.
>
> So in the elan* drivers, the "traces" are simply how many antennas there
> are on each axis. Which means that if a trace of 4 is reported in the
> events, it means it is simply seen by 4 antennas. So the computation of the
> width/height is the following: we take how many antennas there are, we
> subtract one to have the number of holes between the antennas, and we
> divide the number of unit we have in the axis by the value we just
> computed.
> This gives a rough 4mm on the P52, in both directions.
>
> And once you get that, you can just realize that the unit of the width and
> height are just the same than the X and Y coordinates, so we can apply the
> same resolution.
>
> So, in the end, that means that elan_i2c needs the information, or it will
> not be able to convert the number of crossed antennas into a size, but this
> is something specific to this touchpad.
>
> So here come, 7 months later the v2 on the subject.
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (10):
>   Input: elantech - query the min/max information beforehand too
>   Input: elantech - add helper function elantech_is_buttonpad()
>   Input: elantech - detect middle button based on firmware version
>   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
>   Input: elan_i2c - handle physical middle button
>   Input: elan_i2c - export true width/height
>   Input: elan_i2c - correct the width/size base value
>   Input: elantech: remove P52 from SMBus blacklist
>
>  .../devicetree/bindings/input/elan_i2c.txt    |  11 +
>  drivers/input/mouse/elan_i2c_core.c           |  85 +++--
>  drivers/input/mouse/elantech.c                | 318 ++++++++++--------
>  drivers/input/mouse/elantech.h                |   8 +
>  4 files changed, 251 insertions(+), 171 deletions(-)
>
> --
> 2.21.0
>

RE: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value

[廖崇榮]

Hi Benjamin,

Thanks so much for all you do for Elan touchpad.

For the width_*, I have a question for it.
Our antenna sensors fully occupied the whole touchpad PCB.

The Gap between 2 sensors are 7.5 mil (0.19mm).
That's why we did not minus one trace.


Thanks
KT
-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
Sent: Tuesday, May 21, 2019 9:27 PM
To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; Benjamin Tissoires
Subject: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base
value

*_traces are the number of antennas. width_* is thus the space between 2
antennas. Which means, we should subtract 1 to the number of antennas to
divide the touchpad by the number of holes between each antenna.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

--

new in v2
---
 drivers/input/mouse/elan_i2c_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c
b/drivers/input/mouse/elan_i2c_core.c
index 6f4feedb7765..3375eaa9a72e 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -398,8 +398,8 @@ static int elan_query_device_parameters(struct
elan_tp_data *data)
 		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 - 1);
+	data->width_y = data->max_y / (y_traces - 1);
 
 	if (device_property_read_u32(&client->dev,
 				     "touchscreen-x-mm", &x_mm) ||
--
2.21.0

Re: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value

[Benjamin Tissoires]

On Fri, May 24, 2019 at 5:13 AM 廖崇榮 <kt.liao@emc.com.tw> wrote:
>
> Hi Benjamin,
>
> Thanks so much for all you do for Elan touchpad.
>
> For the width_*, I have a question for it.
> Our antenna sensors fully occupied the whole touchpad PCB.
>
> The Gap between 2 sensors are 7.5 mil (0.19mm).
> That's why we did not minus one trace.

So, with the P52 I have:
[  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
[  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
[  +0.000002] size:   (98,55) drivers/input/mouse/elan_i2c_core.c:430
[  +0.000001] res:    (31,31) drivers/input/mouse/elan_i2c_core.c:431

calculated size (max/res): 98 x 56 mm
true size, as measured: 101 x 60 mm

Which gives (without the minus 1):
width_x = max_x / x_traces = 3045 / 24 = 126.875 -> 3.9885 mm
width_y = max_y / y_traces = 1731 / 14 = 123.643 -> 4.0927 mm

-> this gives a total size of the touchpad of: 96 x 57 mm (width_x *
24, width_y * 14)

With the minus 1:
width_x = max_x / x_traces = 3045 / 23 = 132.391 -> 4.2707 mm
width_y = max_y / y_traces = 1731 / 14 = 133.154 -> 4.2953 mm

-> this gives a total size of the touchpad of: 102 x 60 mm (width_x *
24, width_y * 14)
and considering traces-1: 98 x 56 mm

Removing 1 to the number of traces gave a squarer values in rows and
columns, and this is what is done in the PS/2 driver.
Also, going back to the size of the touchpad gives a better value when
removing 1 on the *traces.
So maybe when forwarding the properties we should remove one there in
the PS/2 driver?

Cheers,
Benjamin

>
>
> Thanks
> KT
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Tuesday, May 21, 2019 9:27 PM
> To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Benjamin Tissoires
> Subject: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base
> value
>
> *_traces are the number of antennas. width_* is thus the space between 2
> antennas. Which means, we should subtract 1 to the number of antennas to
> divide the touchpad by the number of holes between each antenna.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> --
>
> new in v2
> ---
>  drivers/input/mouse/elan_i2c_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c
> b/drivers/input/mouse/elan_i2c_core.c
> index 6f4feedb7765..3375eaa9a72e 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -398,8 +398,8 @@ static int elan_query_device_parameters(struct
> elan_tp_data *data)
>                 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 - 1);
> +       data->width_y = data->max_y / (y_traces - 1);
>
>         if (device_property_read_u32(&client->dev,
>                                      "touchscreen-x-mm", &x_mm) ||
> --
> 2.21.0
>

RE: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value

[廖崇榮]

Hi

-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
Sent: Friday, May 24, 2019 3:06 PM
To: 廖崇榮
Cc: Dmitry Torokhov; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
Subject: Re: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value

On Fri, May 24, 2019 at 5:13 AM 廖崇榮 <kt.liao@emc.com.tw> wrote:
>
> Hi Benjamin,
>
> Thanks so much for all you do for Elan touchpad.
>
> For the width_*, I have a question for it.
> Our antenna sensors fully occupied the whole touchpad PCB.
>
> The Gap between 2 sensors are 7.5 mil (0.19mm).
> That's why we did not minus one trace.

So, with the P52 I have:
[  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
[  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
[  +0.000002] size:   (98,55) drivers/input/mouse/elan_i2c_core.c:430
[  +0.000001] res:    (31,31) drivers/input/mouse/elan_i2c_core.c:431

calculated size (max/res): 98 x 56 mm
true size, as measured: 101 x 60 mm

I list layout information of P52 touchpad as below.
Physical size : 99 x 58 mm
Active Area size : ~ 97 * 56 mm, (boarding is 1.008mm for each side)

Sensor layout:
X Pitch : 4.0286 mm
Y Pitch : 4.0147 mm

Which gives (without the minus 1):
width_x = max_x / x_traces = 3045 / 24 = 126.875 -> 3.9885 mm width_y = max_y / y_traces = 1731 / 14 = 123.643 -> 4.0927 mm

-> this gives a total size of the touchpad of: 96 x 57 mm (width_x *
24, width_y * 14)

With the minus 1:
width_x = max_x / x_traces = 3045 / 23 = 132.391 -> 4.2707 mm width_y = max_y / y_traces = 1731 / 14 = 133.154 -> 4.2953 mm

-> this gives a total size of the touchpad of: 102 x 60 mm (width_x *
24, width_y * 14)
and considering traces-1: 98 x 56 mm

Removing 1 to the number of traces gave a squarer values in rows and columns, and this is what is done in the PS/2 driver.
Also, going back to the size of the touchpad gives a better value when removing 1 on the *traces.
So maybe when forwarding the properties we should remove one there in the PS/2 driver?

Removing 1 trace may be better for some of previous touchpad. (depending on sensor pattern)
mk_* indicate the number of trace which is touched, and it's not a precise value.
I think the usability won't change too much whether removing one trace.
PS/2 have supported plenty of touchpad. It's better to remain the same.

Thanks
KT

Cheers,
Benjamin

>
>
> Thanks
> KT
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Tuesday, May 21, 2019 9:27 PM
> To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; 
> devicetree@vger.kernel.org; Benjamin Tissoires
> Subject: [PATCH v2 09/10] Input: elan_i2c - correct the width/size 
> base value
>
> *_traces are the number of antennas. width_* is thus the space between 
> 2 antennas. Which means, we should subtract 1 to the number of 
> antennas to divide the touchpad by the number of holes between each antenna.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> --
>
> new in v2
> ---
>  drivers/input/mouse/elan_i2c_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c
> b/drivers/input/mouse/elan_i2c_core.c
> index 6f4feedb7765..3375eaa9a72e 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -398,8 +398,8 @@ static int elan_query_device_parameters(struct
> elan_tp_data *data)
>                 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 - 1);
> +       data->width_y = data->max_y / (y_traces - 1);
>
>         if (device_property_read_u32(&client->dev,
>                                      "touchscreen-x-mm", &x_mm) ||
> --
> 2.21.0
>

Re: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value

[Benjamin Tissoires]

On Fri, May 24, 2019 at 11:00 AM 廖崇榮 <kt.liao@emc.com.tw> wrote:
>
> Hi
>
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Friday, May 24, 2019 3:06 PM
> To: 廖崇榮
> Cc: Dmitry Torokhov; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 09/10] Input: elan_i2c - correct the width/size base value
>
> On Fri, May 24, 2019 at 5:13 AM 廖崇榮 <kt.liao@emc.com.tw> wrote:
> >
> > Hi Benjamin,
> >
> > Thanks so much for all you do for Elan touchpad.
> >
> > For the width_*, I have a question for it.
> > Our antenna sensors fully occupied the whole touchpad PCB.
> >
> > The Gap between 2 sensors are 7.5 mil (0.19mm).
> > That's why we did not minus one trace.
>
> So, with the P52 I have:
> [  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
> [  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
> [  +0.000002] size:   (98,55) drivers/input/mouse/elan_i2c_core.c:430
> [  +0.000001] res:    (31,31) drivers/input/mouse/elan_i2c_core.c:431
>
> calculated size (max/res): 98 x 56 mm
> true size, as measured: 101 x 60 mm
>
> I list layout information of P52 touchpad as below.
> Physical size : 99 x 58 mm
> Active Area size : ~ 97 * 56 mm, (boarding is 1.008mm for each side)
>
> Sensor layout:
> X Pitch : 4.0286 mm
> Y Pitch : 4.0147 mm
>
> Which gives (without the minus 1):
> width_x = max_x / x_traces = 3045 / 24 = 126.875 -> 3.9885 mm width_y = max_y / y_traces = 1731 / 14 = 123.643 -> 4.0927 mm
>
> -> this gives a total size of the touchpad of: 96 x 57 mm (width_x *
> 24, width_y * 14)
>
> With the minus 1:
> width_x = max_x / x_traces = 3045 / 23 = 132.391 -> 4.2707 mm width_y = max_y / y_traces = 1731 / 14 = 133.154 -> 4.2953 mm
>
> -> this gives a total size of the touchpad of: 102 x 60 mm (width_x *
> 24, width_y * 14)
> and considering traces-1: 98 x 56 mm
>
> Removing 1 to the number of traces gave a squarer values in rows and columns, and this is what is done in the PS/2 driver.
> Also, going back to the size of the touchpad gives a better value when removing 1 on the *traces.
> So maybe when forwarding the properties we should remove one there in the PS/2 driver?
>
> Removing 1 trace may be better for some of previous touchpad. (depending on sensor pattern)
> mk_* indicate the number of trace which is touched, and it's not a precise value.
> I think the usability won't change too much whether removing one trace.
> PS/2 have supported plenty of touchpad. It's better to remain the same.
>

OK, so I guess I should just drop this patch from the series then.

Cheers,
Benjamin

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[Benjamin Tissoires]

On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> The width/height is actually in the same unit than X and Y. So we should
> not tamper the data, but just set the proper resolution, so that userspace
> can correctly detect which touch is a palm or a finger.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> --
>
> new in v2
> ---
>  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index 7ff044c6cd11..6f4feedb7765 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -45,7 +45,6 @@
>  #define DRIVER_NAME            "elan_i2c"
>  #define ELAN_VENDOR_ID         0x04f3
>  #define ETP_MAX_PRESSURE       255
> -#define ETP_FWIDTH_REDUCE      90
>  #define ETP_FINGER_WIDTH       15
>  #define ETP_RETRY_COUNT                3
>
> @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
>                         return;
>                 }
>
> -               /*
> -                * To avoid treating large finger as palm, let's reduce the
> -                * width x and y per trace.
> -                */
> -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> +               area_x = mk_x * data->width_x;
> +               area_y = mk_y * data->width_y;
>
>                 major = max(area_x, area_y);
>                 minor = min(area_x, area_y);
> @@ -1123,8 +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data)
>                              ETP_MAX_PRESSURE, 0, 0);
>         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
>                              ETP_FINGER_WIDTH * max_width, 0, 0);
> +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
>         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
>                              ETP_FINGER_WIDTH * min_width, 0, 0);
> +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);

I had a chat with Peter on Wednesday, and he mentioned that this is
dangerous as Major/Minor are max/min of the width and height. And
given that we might have 2 different resolutions, we would need to do
some computation in the kernel to ensure the data is correct with
respect to the resolution.

TL;DR: I don't think we should export the resolution there :(

KT, should I drop the patch entirely, or is there a strong argument
for keeping the ETP_FWIDTH_REDUCE around?

Cheers,
Benjamin


>
>         data->input = input;
>
> --
> 2.21.0
>

RE: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[廖崇榮]

Hi

-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
Sent: Friday, May 24, 2019 5:37 PM
To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
> The width/height is actually in the same unit than X and Y. So we 
> should not tamper the data, but just set the proper resolution, so 
> that userspace can correctly detect which touch is a palm or a finger.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> --
>
> new in v2
> ---
>  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c 
> b/drivers/input/mouse/elan_i2c_core.c
> index 7ff044c6cd11..6f4feedb7765 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -45,7 +45,6 @@
>  #define DRIVER_NAME            "elan_i2c"
>  #define ELAN_VENDOR_ID         0x04f3
>  #define ETP_MAX_PRESSURE       255
> -#define ETP_FWIDTH_REDUCE      90
>  #define ETP_FINGER_WIDTH       15
>  #define ETP_RETRY_COUNT                3
>
> @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
>                         return;
>                 }
>
> -               /*
> -                * To avoid treating large finger as palm, let's reduce the
> -                * width x and y per trace.
> -                */
> -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> +               area_x = mk_x * data->width_x;
> +               area_y = mk_y * data->width_y;
>
>                 major = max(area_x, area_y);
>                 minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ 
> static int elan_setup_input_device(struct elan_tp_data *data)
>                              ETP_MAX_PRESSURE, 0, 0);
>         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
>                              ETP_FINGER_WIDTH * max_width, 0, 0);
> +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
>         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
>                              ETP_FINGER_WIDTH * min_width, 0, 0);
> +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);

I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.

TL;DR: I don't think we should export the resolution there :(

KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. 
Our FW team know nothing about ETP_FWIDTH_REDUCE ether.

The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
Chrome's finger/palm threshold may be different from other Linux distribution.
We will discuss it with Google once the patch picked by chrome and cause something wrong.

Cheers,
Benjamin


>
>         data->input = input;
>
> --
> 2.21.0
>

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

['Dmitry Torokhov']

Hi Benjamin, KT,

On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> Hi
> 
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
> Sent: Friday, May 24, 2019 5:37 PM
> To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> 
> On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >
> > The width/height is actually in the same unit than X and Y. So we 
> > should not tamper the data, but just set the proper resolution, so 
> > that userspace can correctly detect which touch is a palm or a finger.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > --
> >
> > new in v2
> > ---
> >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c 
> > b/drivers/input/mouse/elan_i2c_core.c
> > index 7ff044c6cd11..6f4feedb7765 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -45,7 +45,6 @@
> >  #define DRIVER_NAME            "elan_i2c"
> >  #define ELAN_VENDOR_ID         0x04f3
> >  #define ETP_MAX_PRESSURE       255
> > -#define ETP_FWIDTH_REDUCE      90
> >  #define ETP_FINGER_WIDTH       15
> >  #define ETP_RETRY_COUNT                3
> >
> > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> >                         return;
> >                 }
> >
> > -               /*
> > -                * To avoid treating large finger as palm, let's reduce the
> > -                * width x and y per trace.
> > -                */
> > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > +               area_x = mk_x * data->width_x;
> > +               area_y = mk_y * data->width_y;
> >
> >                 major = max(area_x, area_y);
> >                 minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@ 
> > static int elan_setup_input_device(struct elan_tp_data *data)
> >                              ETP_MAX_PRESSURE, 0, 0);
> >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> >                              ETP_FINGER_WIDTH * max_width, 0, 0);
> > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> >                              ETP_FINGER_WIDTH * min_width, 0, 0);
> > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> 
> I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> 
> TL;DR: I don't think we should export the resolution there :(
> 
> KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed. 
> Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> 
> The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> Chrome's finger/palm threshold may be different from other Linux distribution.
> We will discuss it with Google once the patch picked by chrome and cause something wrong.

Chrome has logic that contact with maximum major/minor is treated as a
palm, so here the driver (which originally came from Chrome OS)
artificially reduces the contact size to ensure that palm rejection
logic does not trigger.

I'm adding Harry to confirm whether we are still using this logic and to
see if we can adjust it to be something else.

Thanks.

-- 
Dmitry

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[Harry Cutts]

On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin, KT,
>
> On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > Hi
> >
> > -----Original Message-----
> > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > Sent: Friday, May 24, 2019 5:37 PM
> > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> >
> > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > >
> > > The width/height is actually in the same unit than X and Y. So we
> > > should not tamper the data, but just set the proper resolution, so
> > > that userspace can correctly detect which touch is a palm or a finger.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > --
> > >
> > > new in v2
> > > ---
> > >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > b/drivers/input/mouse/elan_i2c_core.c
> > > index 7ff044c6cd11..6f4feedb7765 100644
> > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > @@ -45,7 +45,6 @@
> > >  #define DRIVER_NAME            "elan_i2c"
> > >  #define ELAN_VENDOR_ID         0x04f3
> > >  #define ETP_MAX_PRESSURE       255
> > > -#define ETP_FWIDTH_REDUCE      90
> > >  #define ETP_FINGER_WIDTH       15
> > >  #define ETP_RETRY_COUNT                3
> > >
> > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > >                         return;
> > >                 }
> > >
> > > -               /*
> > > -                * To avoid treating large finger as palm, let's reduce the
> > > -                * width x and y per trace.
> > > -                */
> > > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > +               area_x = mk_x * data->width_x;
> > > +               area_y = mk_y * data->width_y;
> > >
> > >                 major = max(area_x, area_y);
> > >                 minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > static int elan_setup_input_device(struct elan_tp_data *data)
> > >                              ETP_MAX_PRESSURE, 0, 0);
> > >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > >                              ETP_FINGER_WIDTH * max_width, 0, 0);
> > > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > >                              ETP_FINGER_WIDTH * min_width, 0, 0);
> > > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> >
> > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> >
> > TL;DR: I don't think we should export the resolution there :(
> >
> > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> >
> > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > Chrome's finger/palm threshold may be different from other Linux distribution.
> > We will discuss it with Google once the patch picked by chrome and cause something wrong.
>
> Chrome has logic that contact with maximum major/minor is treated as a
> palm, so here the driver (which originally came from Chrome OS)
> artificially reduces the contact size to ensure that palm rejection
> logic does not trigger.
>
> I'm adding Harry to confirm whether we are still using this logic and to
> see if we can adjust it to be something else.

I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[Sean O'Brien]

We do still use a maxed out major axis as a signal for a palm in the touchscreen
logic, but I'm not too concerned because if that axis is maxed out, the contact
should probably be treated as a palm anyway...

I'm more concerned with this affecting our gesture detection for
touchpad. It looks
like this change would cause all contacts to reported as some percentage bigger
than they are currently. Can you give me an idea of how big that percentage is?

On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
>
> On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Benjamin, KT,
> >
> > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > Hi
> > >
> > > -----Original Message-----
> > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > Sent: Friday, May 24, 2019 5:37 PM
> > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> > >
> > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > The width/height is actually in the same unit than X and Y. So we
> > > > should not tamper the data, but just set the proper resolution, so
> > > > that userspace can correctly detect which touch is a palm or a finger.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > --
> > > >
> > > > new in v2
> > > > ---
> > > >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > @@ -45,7 +45,6 @@
> > > >  #define DRIVER_NAME            "elan_i2c"
> > > >  #define ELAN_VENDOR_ID         0x04f3
> > > >  #define ETP_MAX_PRESSURE       255
> > > > -#define ETP_FWIDTH_REDUCE      90
> > > >  #define ETP_FINGER_WIDTH       15
> > > >  #define ETP_RETRY_COUNT                3
> > > >
> > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > >                         return;
> > > >                 }
> > > >
> > > > -               /*
> > > > -                * To avoid treating large finger as palm, let's reduce the
> > > > -                * width x and y per trace.
> > > > -                */
> > > > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > +               area_x = mk_x * data->width_x;
> > > > +               area_y = mk_y * data->width_y;
> > > >
> > > >                 major = max(area_x, area_y);
> > > >                 minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > >                              ETP_MAX_PRESSURE, 0, 0);
> > > >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > >                              ETP_FINGER_WIDTH * max_width, 0, 0);
> > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > >                              ETP_FINGER_WIDTH * min_width, 0, 0);
> > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> > >
> > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > >
> > > TL;DR: I don't think we should export the resolution there :(
> > >
> > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > >
> > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> >
> > Chrome has logic that contact with maximum major/minor is treated as a
> > palm, so here the driver (which originally came from Chrome OS)
> > artificially reduces the contact size to ensure that palm rejection
> > logic does not trigger.
> >
> > I'm adding Harry to confirm whether we are still using this logic and to
> > see if we can adjust it to be something else.
>
> I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[Benjamin Tissoires]

On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
>
> We do still use a maxed out major axis as a signal for a palm in the touchscreen
> logic, but I'm not too concerned because if that axis is maxed out, the contact
> should probably be treated as a palm anyway...
>
> I'm more concerned with this affecting our gesture detection for
> touchpad. It looks
> like this change would cause all contacts to reported as some percentage bigger
> than they are currently. Can you give me an idea of how big that percentage is?

On the P52, I currently have:
[  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
[  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429

-> with the computation done in the kernel:
width_ratio: 126
height_ratio: 123

For my average finger, the reported traces are between 4 and 6:
With the ETP_FWIDTH_REDUCE:
Major between 144 to 216
Minor between 132 to 198

Without:
Major between 504 to 756
Minor between 492 to 738

So a rough augmentation of 350%

For the Synaptics devices (over SMBus), they send the raw value of the
traces, so you will get a major/minor between 2 to 5. Max on these
axes is 15, so we should get the same percentage of value comparing to
the range.
Which is why libinput has a database of which device reports which
pressure/major/minor ranges as otherwise the values are just
impossible to understand.

Cheers,
Benjamin



>
> On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> >
> > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Benjamin, KT,
> > >
> > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > Hi
> > > >
> > > > -----Original Message-----
> > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> > > >
> > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > >
> > > > > The width/height is actually in the same unit than X and Y. So we
> > > > > should not tamper the data, but just set the proper resolution, so
> > > > > that userspace can correctly detect which touch is a palm or a finger.
> > > > >
> > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > >
> > > > > --
> > > > >
> > > > > new in v2
> > > > > ---
> > > > >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > @@ -45,7 +45,6 @@
> > > > >  #define DRIVER_NAME            "elan_i2c"
> > > > >  #define ELAN_VENDOR_ID         0x04f3
> > > > >  #define ETP_MAX_PRESSURE       255
> > > > > -#define ETP_FWIDTH_REDUCE      90
> > > > >  #define ETP_FINGER_WIDTH       15
> > > > >  #define ETP_RETRY_COUNT                3
> > > > >
> > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > >                         return;
> > > > >                 }
> > > > >
> > > > > -               /*
> > > > > -                * To avoid treating large finger as palm, let's reduce the
> > > > > -                * width x and y per trace.
> > > > > -                */
> > > > > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > +               area_x = mk_x * data->width_x;
> > > > > +               area_y = mk_y * data->width_y;
> > > > >
> > > > >                 major = max(area_x, area_y);
> > > > >                 minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > > >                              ETP_MAX_PRESSURE, 0, 0);
> > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > >                              ETP_FINGER_WIDTH * max_width, 0, 0);
> > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > >                              ETP_FINGER_WIDTH * min_width, 0, 0);
> > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> > > >
> > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > >
> > > > TL;DR: I don't think we should export the resolution there :(
> > > >
> > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > >
> > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > >
> > > Chrome has logic that contact with maximum major/minor is treated as a
> > > palm, so here the driver (which originally came from Chrome OS)
> > > artificially reduces the contact size to ensure that palm rejection
> > > logic does not trigger.
> > >
> > > I'm adding Harry to confirm whether we are still using this logic and to
> > > see if we can adjust it to be something else.
> >
> > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.

RE: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[廖崇榮]

-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] 
Sent: Wednesday, May 29, 2019 3:17 PM
To: Sean O'Brien; Peter Hutterer
Cc: Harry Cutts; Dmitry Torokhov; 廖崇榮; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
>
> We do still use a maxed out major axis as a signal for a palm in the 
> touchscreen logic, but I'm not too concerned because if that axis is 
> maxed out, the contact should probably be treated as a palm anyway...
>
> I'm more concerned with this affecting our gesture detection for 
> touchpad. It looks like this change would cause all contacts to 
> reported as some percentage bigger than they are currently. Can you 
> give me an idea of how big that percentage is?

On the P52, I currently have:
[  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
[  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429

-> with the computation done in the kernel:
width_ratio: 126
height_ratio: 123

For my average finger, the reported traces are between 4 and 6:
With the ETP_FWIDTH_REDUCE:
Major between 144 to 216
Minor between 132 to 198

Without:
Major between 504 to 756
Minor between 492 to 738

So a rough augmentation of 350%

For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range.

Elan's vendor report contains such information, which indicate how many trace are touched by finger/palm
		mk_x = (finger_data[3] & 0x0f); 
		mk_y = (finger_data[3] >> 4);
Do we need to use mk_* for major/minor for keeping it consistent with other vendor?
But this modification will impact Chromebook's usability and Chrome test suite.



Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand.

Cheers,
Benjamin



>
> On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> >
> > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Benjamin, KT,
> > >
> > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > Hi
> > > >
> > > > -----Original Message-----
> > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de 
> > > > Goede
> > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true 
> > > > width/height
> > > >
> > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > >
> > > > > The width/height is actually in the same unit than X and Y. So 
> > > > > we should not tamper the data, but just set the proper 
> > > > > resolution, so that userspace can correctly detect which touch is a palm or a finger.
> > > > >
> > > > > Signed-off-by: Benjamin Tissoires 
> > > > > <benjamin.tissoires@redhat.com>
> > > > >
> > > > > --
> > > > >
> > > > > new in v2
> > > > > ---
> > > > >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > @@ -45,7 +45,6 @@
> > > > >  #define DRIVER_NAME            "elan_i2c"
> > > > >  #define ELAN_VENDOR_ID         0x04f3
> > > > >  #define ETP_MAX_PRESSURE       255
> > > > > -#define ETP_FWIDTH_REDUCE      90
> > > > >  #define ETP_FINGER_WIDTH       15
> > > > >  #define ETP_RETRY_COUNT                3
> > > > >
> > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > >                         return;
> > > > >                 }
> > > > >
> > > > > -               /*
> > > > > -                * To avoid treating large finger as palm, let's reduce the
> > > > > -                * width x and y per trace.
> > > > > -                */
> > > > > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > +               area_x = mk_x * data->width_x;
> > > > > +               area_y = mk_y * data->width_y;
> > > > >
> > > > >                 major = max(area_x, area_y);
> > > > >                 minor = min(area_x, area_y); @@ -1123,8 
> > > > > +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data)
> > > > >                              ETP_MAX_PRESSURE, 0, 0);
> > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > >                              ETP_FINGER_WIDTH * max_width, 0, 
> > > > > 0);
> > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, 
> > > > > + data->x_res);
> > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > >                              ETP_FINGER_WIDTH * min_width, 0, 
> > > > > 0);
> > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, 
> > > > > + data->y_res);
> > > >
> > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > >
> > > > TL;DR: I don't think we should export the resolution there :(
> > > >
> > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > >
> > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > >
> > > Chrome has logic that contact with maximum major/minor is treated 
> > > as a palm, so here the driver (which originally came from Chrome 
> > > OS) artificially reduces the contact size to ensure that palm 
> > > rejection logic does not trigger.
> > >
> > > I'm adding Harry to confirm whether we are still using this logic 
> > > and to see if we can adjust it to be something else.
> >
> > I'm not very familiar with our touchpad code, so adding Sean 
> > O'Brien, who is.

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[Benjamin Tissoires]

On Wed, May 29, 2019 at 2:56 PM 廖崇榮 <kt.liao@emc.com.tw> wrote:
>
>
>
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Wednesday, May 29, 2019 3:17 PM
> To: Sean O'Brien; Peter Hutterer
> Cc: Harry Cutts; Dmitry Torokhov; 廖崇榮; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
>
> On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
> >
> > We do still use a maxed out major axis as a signal for a palm in the
> > touchscreen logic, but I'm not too concerned because if that axis is
> > maxed out, the contact should probably be treated as a palm anyway...
> >
> > I'm more concerned with this affecting our gesture detection for
> > touchpad. It looks like this change would cause all contacts to
> > reported as some percentage bigger than they are currently. Can you
> > give me an idea of how big that percentage is?
>
> On the P52, I currently have:
> [  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
> [  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
>
> -> with the computation done in the kernel:
> width_ratio: 126
> height_ratio: 123
>
> For my average finger, the reported traces are between 4 and 6:
> With the ETP_FWIDTH_REDUCE:
> Major between 144 to 216
> Minor between 132 to 198
>
> Without:
> Major between 504 to 756
> Minor between 492 to 738
>
> So a rough augmentation of 350%
>
> For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range.
>
> Elan's vendor report contains such information, which indicate how many trace are touched by finger/palm
>                 mk_x = (finger_data[3] & 0x0f);
>                 mk_y = (finger_data[3] >> 4);
> Do we need to use mk_* for major/minor for keeping it consistent with other vendor?

IMO, no. It is better to send something closer to an actual unit
instead of 12,5th of mm.
However, the problem here is that major/minor can be swapped depending
on how the finger is placed (horizontally or vertically), so
unfortunately, if the axes and resolutions are not the same, then we
are screwed, this would just be a value without unit.

> But this modification will impact Chromebook's usability and Chrome test suite.

Yeah, there is no point breaking things just for the fun of it.

Cheers,
Benjamin

>
>
>
> Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand.
>
> Cheers,
> Benjamin
>
>
>
> >
> > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> > >
> > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Benjamin, KT,
> > > >
> > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > > Hi
> > > > >
> > > > > -----Original Message-----
> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de
> > > > > Goede
> > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true
> > > > > width/height
> > > > >
> > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > > >
> > > > > > The width/height is actually in the same unit than X and Y. So
> > > > > > we should not tamper the data, but just set the proper
> > > > > > resolution, so that userspace can correctly detect which touch is a palm or a finger.
> > > > > >
> > > > > > Signed-off-by: Benjamin Tissoires
> > > > > > <benjamin.tissoires@redhat.com>
> > > > > >
> > > > > > --
> > > > > >
> > > > > > new in v2
> > > > > > ---
> > > > > >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > > @@ -45,7 +45,6 @@
> > > > > >  #define DRIVER_NAME            "elan_i2c"
> > > > > >  #define ELAN_VENDOR_ID         0x04f3
> > > > > >  #define ETP_MAX_PRESSURE       255
> > > > > > -#define ETP_FWIDTH_REDUCE      90
> > > > > >  #define ETP_FINGER_WIDTH       15
> > > > > >  #define ETP_RETRY_COUNT                3
> > > > > >
> > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > > >                         return;
> > > > > >                 }
> > > > > >
> > > > > > -               /*
> > > > > > -                * To avoid treating large finger as palm, let's reduce the
> > > > > > -                * width x and y per trace.
> > > > > > -                */
> > > > > > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > > +               area_x = mk_x * data->width_x;
> > > > > > +               area_y = mk_y * data->width_y;
> > > > > >
> > > > > >                 major = max(area_x, area_y);
> > > > > >                 minor = min(area_x, area_y); @@ -1123,8
> > > > > > +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data)
> > > > > >                              ETP_MAX_PRESSURE, 0, 0);
> > > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > > >                              ETP_FINGER_WIDTH * max_width, 0,
> > > > > > 0);
> > > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR,
> > > > > > + data->x_res);
> > > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > > >                              ETP_FINGER_WIDTH * min_width, 0,
> > > > > > 0);
> > > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR,
> > > > > > + data->y_res);
> > > > >
> > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > > >
> > > > > TL;DR: I don't think we should export the resolution there :(
> > > > >
> > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > > >
> > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > > >
> > > > Chrome has logic that contact with maximum major/minor is treated
> > > > as a palm, so here the driver (which originally came from Chrome
> > > > OS) artificially reduces the contact size to ensure that palm
> > > > rejection logic does not trigger.
> > > >
> > > > I'm adding Harry to confirm whether we are still using this logic
> > > > and to see if we can adjust it to be something else.
> > >
> > > I'm not very familiar with our touchpad code, so adding Sean
> > > O'Brien, who is.
>

Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height

[Peter Hutterer]

On Wed, May 29, 2019 at 09:16:38AM +0200, Benjamin Tissoires wrote:
> On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
> >
> > We do still use a maxed out major axis as a signal for a palm in the touchscreen
> > logic, but I'm not too concerned because if that axis is maxed out, the contact
> > should probably be treated as a palm anyway...
> >
> > I'm more concerned with this affecting our gesture detection for
> > touchpad. It looks
> > like this change would cause all contacts to reported as some percentage bigger
> > than they are currently. Can you give me an idea of how big that percentage is?
> 
> On the P52, I currently have:
> [  +0.000009] max:    (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
> [  +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
> 
> -> with the computation done in the kernel:
> width_ratio: 126
> height_ratio: 123
> 
> For my average finger, the reported traces are between 4 and 6:
> With the ETP_FWIDTH_REDUCE:
> Major between 144 to 216
> Minor between 132 to 198
> 
> Without:
> Major between 504 to 756
> Minor between 492 to 738
> 
> So a rough augmentation of 350%
> 
> For the Synaptics devices (over SMBus), they send the raw value of the
> traces, so you will get a major/minor between 2 to 5. Max on these
> axes is 15, so we should get the same percentage of value comparing to
> the range.
> Which is why libinput has a database of which device reports which
> pressure/major/minor ranges as otherwise the values are just
> impossible to understand.

yeah, I've given up on trying to guess finger/thumb/palm sizes.
git grep for these quirk names in libinput for the ranges:
    AttrTouchSizeRange
    AttrThumbSizeThreshold
    AttrPalmSizeThreshold

There are also matching s/Size/Pressure/ entries for touchpads without
major/minor. Looking at the database now, the palm size thresholds range
entries are 5 (Wacom) and a set of 800-1600 for apple touchpads. So yeah,
all this is really a bit random :) 

Feel free to steal those entries though and/or add to them where applicable.

Cheers,
   Peter
 
> 
> >
> > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> > >
> > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Benjamin, KT,
> > > >
> > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > > Hi
> > > > >
> > > > > -----Original Message-----
> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> > > > >
> > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > > >
> > > > > > The width/height is actually in the same unit than X and Y. So we
> > > > > > should not tamper the data, but just set the proper resolution, so
> > > > > > that userspace can correctly detect which touch is a palm or a finger.
> > > > > >
> > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > > >
> > > > > > --
> > > > > >
> > > > > > new in v2
> > > > > > ---
> > > > > >  drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > > @@ -45,7 +45,6 @@
> > > > > >  #define DRIVER_NAME            "elan_i2c"
> > > > > >  #define ELAN_VENDOR_ID         0x04f3
> > > > > >  #define ETP_MAX_PRESSURE       255
> > > > > > -#define ETP_FWIDTH_REDUCE      90
> > > > > >  #define ETP_FINGER_WIDTH       15
> > > > > >  #define ETP_RETRY_COUNT                3
> > > > > >
> > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > > >                         return;
> > > > > >                 }
> > > > > >
> > > > > > -               /*
> > > > > > -                * To avoid treating large finger as palm, let's reduce the
> > > > > > -                * width x and y per trace.
> > > > > > -                */
> > > > > > -               area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > > -               area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > > +               area_x = mk_x * data->width_x;
> > > > > > +               area_y = mk_y * data->width_y;
> > > > > >
> > > > > >                 major = max(area_x, area_y);
> > > > > >                 minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > > > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > > > >                              ETP_MAX_PRESSURE, 0, 0);
> > > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > > >                              ETP_FINGER_WIDTH * max_width, 0, 0);
> > > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > > > >         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > > >                              ETP_FINGER_WIDTH * min_width, 0, 0);
> > > > > > +       input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> > > > >
> > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > > >
> > > > > TL;DR: I don't think we should export the resolution there :(
> > > > >
> > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > > >
> > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > > >
> > > > Chrome has logic that contact with maximum major/minor is treated as a
> > > > palm, so here the driver (which originally came from Chrome OS)
> > > > artificially reduces the contact size to ensure that palm rejection
> > > > logic does not trigger.
> > > >
> > > > I'm adding Harry to confirm whether we are still using this logic and to
> > > > see if we can adjust it to be something else.
> > >
> > > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.