[PATCH v3 0/7] Subject: [PATCH v3 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers

[PATCH v3 0/7] Subject: [PATCH v3 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers

[David Heidelberg via B4 Relay]

With the growing popularity of running upstream Linux on mobile devices,
we're beginning to run into more and more edgecases. The OnePlus 6 is a
fairly well supported 2018 era smartphone, selling over a million units
in it's first 22 days. With this level of popularity, it's almost
inevitable that we get third party replacement displays, and as a
result, replacement touchscreen controllers.

The OnePlus 6 shipped with an extremely usecase specific touchscreen
driver, it implemented only the bare minimum parts of the highly generic
rmi4 protocol, instead hardcoding most of the register addresses.
  
As a result, the third party touchscreen controllers that are often
found in replacement screens, implement only the registers that the 
downstream driver reads from. They additionally have other restrictions
such as heavy penalties on unaligned reads.
 
This series attempts to implement the necessary workaround to support  
some of these chips with the rmi4 driver. Although it's worth noting
that at the time of writing there are other unofficial controllers in
the wild that don't work even with these patches.
 
We have been shipping these patches in postmarketOS for the last several
months, and they are known to not cause any regressions on the OnePlus
6/6T (with the official Synaptics controller), however I don't own any
other rmi4 hardware to further validate this.

---
Changes since v2:
- reworded dt-bindings property description
- fixed the rmi_driver_of_probe definition for non device-tree builds.
- fixed some indentation issues reported by checkpatch
- change rmi_pdt_entry_is_valid() variable to unsigned 
- Link to v2: https://patchwork.kernel.org/project/linux-input/cover/20230929-caleb-rmi4-quirks-v2-0-b227ac498d88@linaro.org/

Changes since v1:
- Improve dt-bindings patch (thanks Rob)
- Add missing cast in patch 5 to fix the pointer arithmetic
- Link to v1: https://lore.kernel.org/r/20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org

---
Caleb Connolly (2):
      dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
      Input: synaptics-rmi4 - handle duplicate/unknown PDT entries

methanal (5):
      Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
      Input: synaptics-rmi4 - f55: handle zero electrode count
      Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
      Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
      Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes

 .../devicetree/bindings/input/syna,rmi4.yaml       |  18 +++
 drivers/input/rmi4/rmi_driver.c                    | 140 +++++++++++++++++----
 drivers/input/rmi4/rmi_driver.h                    |   8 ++
 drivers/input/rmi4/rmi_f01.c                       |  14 +++
 drivers/input/rmi4/rmi_f12.c                       | 117 +++++++++++++----
 drivers/input/rmi4/rmi_f55.c                       |   5 +
 include/linux/rmi.h                                |   3 +
 7 files changed, 258 insertions(+), 47 deletions(-)
---
base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
change-id: 20250308-synaptics-rmi4-c832b2f73ceb

Best regards,
-- 
David Heidelberg <david@ixit.cz>

[PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[David Heidelberg via B4 Relay]

From: Caleb Connolly <caleb.connolly@linaro.org>

This new property allows devices to specify some register values which
are missing on units with third party replacement displays. These
displays use unofficial touch ICs which only implement a subset of the
RMI4 specification.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 Documentation/devicetree/bindings/input/syna,rmi4.yaml | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
index b522c8d3ce0db719ff379f2fefbdca79e73d027c..a80ec0c052cb1b7278f0832dd18ebd3256bc0874 100644
--- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
+++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
@@ -49,6 +49,24 @@ properties:
     description:
       Delay to wait after powering on the device.
 
+  syna,pdt-fallback-desc:
+    $ref: /schemas/types.yaml#/definitions/uint8-matrix
+    description:
+      This property provides fallback values for certain register fields that
+      are missing on devices using third-party replacement displays.
+      These unofficial displays contain touch controllers that claim RMI4
+      compliance but fail to populate the function_number and function_version
+      registers of their Page Descriptor Table (PDT) entries.
+
+      Since the number of required fallback entries depends on the number of
+      Page Descriptor Tables supported by a given device, this property
+      should be provided on a best-effort basis.
+
+    items:
+      items:
+        - description: The 5th byte of the PDT entry (function number)
+        - description: The 4th byte of the PDT entry (version value)
+
   vdd-supply: true
   vio-supply: true
 

-- 
2.47.2

[PATCH v3 2/7] Input: synaptics-rmi4 - handle duplicate/unknown PDT entries

[David Heidelberg via B4 Relay]

From: Caleb Connolly <caleb.connolly@linaro.org>

Some third party rmi4-compatible ICs don't expose their PDT entries
very well. Add a few checks to skip duplicate entries as well as entries
for unsupported functions.

This is required to support some phones with third party displays.

Validated on a stock OnePlus 6T (original parts):
manufacturer: Synaptics, product: S3706B, fw id: 2852315

Co-developed-by: methanal <baclofen@tuta.io>
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------
 drivers/input/rmi4/rmi_driver.h |  6 ++++++
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
 	fd->function_version = pdt->function_version;
 }
 
+static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev,
+				   struct pdt_scan_state *state, u8 fn)
+{
+	unsigned int i;
+
+	switch (fn) {
+	case 0x01:
+	case 0x03:
+	case 0x11:
+	case 0x12:
+	case 0x30:
+	case 0x34:
+	case 0x3a:
+	case 0x54:
+	case 0x55:
+		break;
+
+	default:
+		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
+			"PDT has unknown function number %#02x\n", fn);
+		return false;
+	}
+
+	for (i = 0; i < state->pdt_count; i++) {
+		if (state->pdts[i] == fn)
+			return false;
+	}
+
+	state->pdts[state->pdt_count++] = fn;
+	return true;
+}
+
 #define RMI_SCAN_CONTINUE	0
 #define RMI_SCAN_DONE		1
 
 static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 			     int page,
-			     int *empty_pages,
+			     struct pdt_scan_state *state,
 			     void *ctx,
 			     int (*callback)(struct rmi_device *rmi_dev,
 					     void *ctx,
@@ -521,6 +553,9 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 		if (RMI4_END_OF_PDT(pdt_entry.function_number))
 			break;
 
+		if (!rmi_pdt_entry_is_valid(rmi_dev, state, pdt_entry.function_number))
+			continue;
+
 		retval = callback(rmi_dev, ctx, &pdt_entry);
 		if (retval != RMI_SCAN_CONTINUE)
 			return retval;
@@ -531,11 +566,11 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 	 * or more is found, stop scanning.
 	 */
 	if (addr == pdt_start)
-		++*empty_pages;
+		++state->empty_pages;
 	else
-		*empty_pages = 0;
+		state->empty_pages = 0;
 
-	return (data->bootloader_mode || *empty_pages >= 2) ?
+	return (data->bootloader_mode || state->empty_pages >= 2) ?
 					RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
 }
 
@@ -544,11 +579,11 @@ int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
 		 void *ctx, const struct pdt_entry *entry))
 {
 	int page;
-	int empty_pages = 0;
+	struct pdt_scan_state state = {0, 0, {0}};
 	int retval = RMI_SCAN_DONE;
 
 	for (page = 0; page <= RMI4_MAX_PAGE; page++) {
-		retval = rmi_scan_pdt_page(rmi_dev, page, &empty_pages,
+		retval = rmi_scan_pdt_page(rmi_dev, page, &state,
 					   ctx, callback);
 		if (retval != RMI_SCAN_CONTINUE)
 			break;
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 3bfe9013043ef3dff46249095a5b3116c8f7d9a6..0ecbd19cefffe00ce7c8001db6479a2e213ac466 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -46,6 +46,12 @@ struct pdt_entry {
 	u8 function_number;
 };
 
+struct pdt_scan_state {
+	u8 empty_pages;
+	u8 pdt_count;
+	u8 pdts[254];
+};
+
 #define RMI_REG_DESC_PRESENSE_BITS	(32 * BITS_PER_BYTE)
 #define RMI_REG_DESC_SUBPACKET_BITS	(37 * BITS_PER_BYTE)
 

-- 
2.47.2

[PATCH v3 3/7] Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs

[David Heidelberg via B4 Relay]

From: methanal <baclofen@tuta.io>

Some replacement displays include third-party touch ICs which are
devoid of register descriptors. Create a fake data register descriptor
for such ICs and provide hardcoded default values.

It isn't possible to reliably determine if the touch IC is original or
not, so these fallback values are offered as an alternative to the error
path when register descriptors aren't available.

Signed-off-by: methanal <baclofen@tuta.io>
[changes for readability / codeflow, checkpatch fixes]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/input/rmi4/rmi_f12.c | 117 +++++++++++++++++++++++++++++++++----------
 1 file changed, 91 insertions(+), 26 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index 8246fe77114bbd8b795ba35d5a37ede8727fc7cb..1a103cc5f2235a6eafd7a3f5b89cbfc9e53203d2 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -218,6 +218,41 @@ static void rmi_f12_process_objects(struct f12_data *f12, u8 *data1, int size)
 		rmi_2d_sensor_abs_report(sensor, &sensor->objs[i], i);
 }
 
+static void rmi_f12_set_hardcoded_desc(struct rmi_function *fn, struct f12_data *f12)
+{
+	struct rmi_2d_sensor *sensor = &f12->sensor;
+	struct rmi_register_desc_item *reg_desc;
+
+	/* We have no f12->data_reg_desc, so the pkt_size is 0, override it with
+	 * a somewhat sensible default (this corresponds to 10 fingers).
+	 */
+	sensor->pkt_size = 88;
+
+	/*
+	 * There are no register descriptors to get these values from.
+	 * We set them to high values to either be overwritten by the clip
+	 * properties from devicetree, or to just not get in the way.
+	 */
+	sensor->max_x = 65535;
+	sensor->max_y = 65535;
+
+	/*
+	 * Create the Data1 register descriptor so that touch events
+	 * can work properly.
+	 */
+	reg_desc = devm_kcalloc(&fn->dev, 1,
+			sizeof(struct rmi_register_desc_item), GFP_KERNEL);
+	reg_desc->reg = 1;
+	reg_desc->reg_size = 80;
+	reg_desc->num_subpackets = 10;
+
+	f12->data1 = reg_desc;
+	f12->data1_offset = 0;
+	sensor->nbr_fingers = reg_desc->num_subpackets;
+	sensor->report_abs = 1;
+	sensor->attn_size += reg_desc->reg_size;
+}
+
 static irqreturn_t rmi_f12_attention(int irq, void *ctx)
 {
 	int retval;
@@ -338,6 +373,40 @@ static int rmi_f12_config(struct rmi_function *fn)
 	return 0;
 }
 
+static int rmi_f12_sensor_init(struct rmi_function *fn, struct f12_data *f12)
+{
+	struct rmi_2d_sensor *sensor = &f12->sensor;
+
+	sensor->fn = fn;
+	f12->data_addr = fn->fd.data_base_addr;
+
+	/* On quirky devices that don't have a data_reg_desc we hardcode the packet
+	 * in rmi_f12_set_hardcoded_desc(). Make sure not to set it to 0 here.
+	 */
+	if (!sensor->pkt_size)
+		sensor->pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
+
+	sensor->axis_align =
+		f12->sensor_pdata.axis_align;
+
+	sensor->x_mm = f12->sensor_pdata.x_mm;
+	sensor->y_mm = f12->sensor_pdata.y_mm;
+	sensor->dribble = f12->sensor_pdata.dribble;
+
+	if (sensor->sensor_type == rmi_sensor_default)
+		sensor->sensor_type =
+			f12->sensor_pdata.sensor_type;
+
+	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %d\n", __func__,
+		sensor->pkt_size);
+
+	sensor->data_pkt = devm_kzalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
+	if (!sensor->data_pkt)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int rmi_f12_probe(struct rmi_function *fn)
 {
 	struct f12_data *f12;
@@ -351,6 +420,7 @@ static int rmi_f12_probe(struct rmi_function *fn)
 	struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
 	u16 data_offset = 0;
 	int mask_size;
+	bool hardcoded_desc_quirk = false;
 
 	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s\n", __func__);
 
@@ -365,9 +435,9 @@ static int rmi_f12_probe(struct rmi_function *fn)
 	++query_addr;
 
 	if (!(buf & BIT(0))) {
-		dev_err(&fn->dev,
-			"Behavior of F12 without register descriptors is undefined.\n");
-		return -ENODEV;
+		rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+			"No register descriptors defined for F12, using fallback\n");
+		hardcoded_desc_quirk = true;
 	}
 
 	f12 = devm_kzalloc(&fn->dev, sizeof(struct f12_data) + mask_size * 2,
@@ -375,6 +445,8 @@ static int rmi_f12_probe(struct rmi_function *fn)
 	if (!f12)
 		return -ENOMEM;
 
+	dev_set_drvdata(&fn->dev, f12);
+
 	f12->abs_mask = (unsigned long *)((char *)f12
 			+ sizeof(struct f12_data));
 	f12->rel_mask = (unsigned long *)((char *)f12
@@ -393,6 +465,18 @@ static int rmi_f12_probe(struct rmi_function *fn)
 		f12->sensor_pdata = pdata->sensor_pdata;
 	}
 
+	sensor = &f12->sensor;
+
+	if (hardcoded_desc_quirk) {
+		rmi_f12_set_hardcoded_desc(fn, f12);
+
+		ret = rmi_f12_sensor_init(fn, f12);
+		if (ret)
+			return ret;
+
+		goto skip_register_desc;
+	}
+
 	ret = rmi_read_register_desc(rmi_dev, query_addr,
 					&f12->query_reg_desc);
 	if (ret) {
@@ -423,29 +507,9 @@ static int rmi_f12_probe(struct rmi_function *fn)
 	}
 	query_addr += 3;
 
-	sensor = &f12->sensor;
-	sensor->fn = fn;
-	f12->data_addr = fn->fd.data_base_addr;
-	sensor->pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
-
-	sensor->axis_align =
-		f12->sensor_pdata.axis_align;
-
-	sensor->x_mm = f12->sensor_pdata.x_mm;
-	sensor->y_mm = f12->sensor_pdata.y_mm;
-	sensor->dribble = f12->sensor_pdata.dribble;
-
-	if (sensor->sensor_type == rmi_sensor_default)
-		sensor->sensor_type =
-			f12->sensor_pdata.sensor_type;
-
-	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %d\n", __func__,
-		sensor->pkt_size);
-	sensor->data_pkt = devm_kzalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
-	if (!sensor->data_pkt)
-		return -ENOMEM;
-
-	dev_set_drvdata(&fn->dev, f12);
+	ret = rmi_f12_sensor_init(fn, f12);
+	if (ret)
+		return ret;
 
 	ret = rmi_f12_read_sensor_tuning(f12);
 	if (ret)
@@ -543,6 +607,7 @@ static int rmi_f12_probe(struct rmi_function *fn)
 		data_offset += item->reg_size;
 	}
 
+skip_register_desc:
 	/* allocate the in-kernel tracking buffers */
 	sensor->tracking_pos = devm_kcalloc(&fn->dev,
 			sensor->nbr_fingers, sizeof(struct input_mt_pos),

-- 
2.47.2

[PATCH v3 4/7] Input: synaptics-rmi4 - f55: handle zero electrode count

[David Heidelberg via B4 Relay]

From: methanal <baclofen@tuta.io>

Some third party ICs claim to support f55 but report an electrode count
of 0. Catch this and bail out early so that we don't confuse the i2c bus
with 0 sized reads.

Signed-off-by: methanal <baclofen@tuta.io>
[simplify code, adjust wording]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/input/rmi4/rmi_f55.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
index 488adaca4dd00482cd1106d813b32871092c83a0..ad2ef14ae9f4e897473db43334792cc3de966d52 100644
--- a/drivers/input/rmi4/rmi_f55.c
+++ b/drivers/input/rmi4/rmi_f55.c
@@ -52,6 +52,11 @@ static int rmi_f55_detect(struct rmi_function *fn)
 
 	f55->num_rx_electrodes = f55->qry[F55_NUM_RX_OFFSET];
 	f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
+	if (!f55->num_rx_electrodes || !f55->num_tx_electrodes) {
+		rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+			"F55 query returned no electrodes, giving up\n");
+		return 0;
+	}
 
 	f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
 	f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;

-- 
2.47.2

[PATCH v3 5/7] Input: synaptics-rmi4 - don't do unaligned reads in IRQ context

[David Heidelberg via B4 Relay]

From: methanal <baclofen@tuta.io>

Some replacement displays include third-party touch ICs which incur a
significant penalty (1-2 seconds) when doing certain unaligned reads.
This is enough to break functionality when it happens in the hot path,
so adjust the interrupt handler to not read from an unaligned address.

Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/input/rmi4/rmi_driver.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 51c23a407b2731d5b6eaefe9cae6288f97316e34..2790f00a58bc66c04656d0cd1b4afe9f843ee093 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -136,9 +136,14 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
 		return 0;
 
 	if (!data->attn_data.data) {
+		/*
+		 * Read the device status register as well and ignore it.
+		 * Some aftermarket ICs have issues with interrupt requests
+		 * otherwise.
+		 */
 		error = rmi_read_block(rmi_dev,
-				data->f01_container->fd.data_base_addr + 1,
-				data->irq_status, data->num_of_irq_regs);
+				data->f01_container->fd.data_base_addr,
+				(u8 *)data->irq_status - 1, data->num_of_irq_regs + 1);
 		if (error < 0) {
 			dev_err(dev, "Failed to read irqs, code=%d\n", error);
 			return error;
@@ -1083,16 +1088,17 @@ int rmi_probe_interrupts(struct rmi_driver_data *data)
 	data->num_of_irq_regs = (data->irq_count + 7) / 8;
 
 	size = BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long);
-	data->irq_memory = devm_kcalloc(dev, size, 4, GFP_KERNEL);
+	data->irq_memory = devm_kzalloc(dev, size * 4 + 1, GFP_KERNEL);
 	if (!data->irq_memory) {
 		dev_err(dev, "Failed to allocate memory for irq masks.\n");
 		return -ENOMEM;
 	}
 
-	data->irq_status	= data->irq_memory + size * 0;
-	data->fn_irq_bits	= data->irq_memory + size * 1;
-	data->current_irq_mask	= data->irq_memory + size * 2;
-	data->new_irq_mask	= data->irq_memory + size * 3;
+	/* The first byte is reserved for the device status register */
+	data->irq_status	= data->irq_memory + size * 0 + 1;
+	data->fn_irq_bits	= data->irq_memory + size * 1 + 1;
+	data->current_irq_mask	= data->irq_memory + size * 2 + 1;
+	data->new_irq_mask	= data->irq_memory + size * 3 + 1;
 
 	return retval;
 }

-- 
2.47.2

[PATCH v3 6/7] Input: synaptics-rmi4 - read product ID on aftermarket touch ICs

[David Heidelberg via B4 Relay]

From: methanal <baclofen@tuta.io>

Some replacement displays include third-party touch ICs which do not
report the product ID correctly unless we read directly from the
product ID register. Add a check and a fallback read to handle this.

Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/input/rmi4/rmi_f01.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 47be64284b25ede8103ada86d6b58fd3a26976bb..2278e9b6a920774b07ec9dd3e452cedc69469be8 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -250,6 +250,20 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
 		}
 	}
 
+	/*
+	 * Some aftermarket ICs put garbage into the product id field unless
+	 * we read directly from the product id register.
+	 */
+	if (props->product_id[0] < 0x20) {
+		ret = rmi_read_block(rmi_dev, query_base_addr + 11,
+				       props->product_id, RMI_PRODUCT_ID_LENGTH);
+		if (ret) {
+			dev_err(&rmi_dev->dev,
+				"Failed to read product id: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 

-- 
2.47.2

[PATCH v3 7/7] Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes

[David Heidelberg via B4 Relay]

From: methanal <baclofen@tuta.io>

Some replacement displays include third-party touch ICs which do not
expose the function number and the interrupt status in its PDT entries.

OnePlus 6 (original touch IC)
  rmi4_i2c 12-0020: read 6 bytes at 0x00e3: 0 (2b 22 0d 06 01 01)

OnePlus 6 (aftermarket touch IC)
  rmi4_i2c 12-0020: read 6 bytes at 0x00e3: 0 (2c 23 0d 06 00 00)

Introduce a new devicetree property `syna,pdt-desc` which can be used to
provide platform-specific fallback values for users with a replacement
display with an aftermarket touch IC.

Signed-off-by: methanal <baclofen@tuta.io>
[codeflow adjustments, checkpatch fixes, wording]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/input/rmi4/rmi_driver.c | 73 ++++++++++++++++++++++++++++++++++++-----
 drivers/input/rmi4/rmi_driver.h |  2 ++
 include/linux/rmi.h             |  3 ++
 3 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 2790f00a58bc66c04656d0cd1b4afe9f843ee093..f233c0b1f8e6a936089fd81344c46b6741d3d82d 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -461,9 +461,10 @@ static int rmi_driver_reset_handler(struct rmi_device *rmi_dev)
 	return 0;
 }
 
-static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
-			      struct pdt_entry *entry, u16 pdt_address)
+static int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
+			      struct pdt_scan_state *state, u16 pdt_address)
 {
+	const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
 	u8 buf[RMI_PDT_ENTRY_SIZE];
 	int error;
 
@@ -474,6 +475,21 @@ static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
 		return error;
 	}
 
+	if (pdata->pdt_fallback_size > state->pdt_count * RMI_OF_PDT_DESC_CELLS + 1) {
+		/* Use the description bytes from DT */
+		buf[5] = pdata->pdt_fallback_desc[state->pdt_count * RMI_OF_PDT_DESC_CELLS];
+		buf[4] = pdata->pdt_fallback_desc[state->pdt_count * RMI_OF_PDT_DESC_CELLS + 1];
+
+		error = rmi_read_block(rmi_dev, pdt_address, buf,
+				RMI_PDT_ENTRY_SIZE - 2);
+		if (error) {
+			dev_err(&rmi_dev->dev,
+					"Read PDT entry at %#06x failed, code: %d.\n",
+					pdt_address, error);
+			return error;
+		}
+	}
+
 	entry->page_start = pdt_address & RMI4_PAGE_MASK;
 	entry->query_base_addr = buf[0];
 	entry->command_base_addr = buf[1];
@@ -551,7 +567,7 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 	int retval;
 
 	for (addr = pdt_start; addr >= pdt_end; addr -= RMI_PDT_ENTRY_SIZE) {
-		error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, addr);
+		error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, state, addr);
 		if (error)
 			return error;
 
@@ -1028,9 +1044,11 @@ static int rmi_driver_remove(struct device *dev)
 }
 
 #ifdef CONFIG_OF
-static int rmi_driver_of_probe(struct device *dev,
-				struct rmi_device_platform_data *pdata)
+static int rmi_driver_of_probe(struct rmi_device *rmi_dev,
+			       struct rmi_device_platform_data *pdata)
 {
+	struct device *dev = rmi_dev->xport->dev;
+	u8 buf[RMI_PDT_ENTRY_SIZE];
 	int retval;
 
 	retval = rmi_of_property_read_u32(dev, &pdata->reset_delay_ms,
@@ -1038,11 +1056,50 @@ static int rmi_driver_of_probe(struct device *dev,
 	if (retval)
 		return retval;
 
+	/*
+	 * In some aftermerket touch ICs, the first PDT entry is empty and
+	 * the function number register is 0. If so, the platform
+	 * may have provided backup PDT entries in the device tree.
+	 */
+
+	retval = rmi_read_block(rmi_dev, PDT_START_SCAN_LOCATION,
+			buf, RMI_PDT_ENTRY_SIZE);
+	if (retval) {
+		dev_err(dev, "Read PDT entry at %#06x failed, code: %d.\n",
+			PDT_START_SCAN_LOCATION, retval);
+		return retval;
+	}
+
+	if (!RMI4_END_OF_PDT(buf[5]))
+		return 0;
+
+	pdata->pdt_fallback_size = of_property_count_u8_elems(dev->of_node,
+						  "syna,pdt-fallback-desc");
+
+	/* The rmi4 driver would fail later anyway, so just error out now. */
+	if (pdata->pdt_fallback_size == -EINVAL) {
+		dev_err(dev, "First PDT entry is empty and no backup values provided\n");
+		return -EINVAL;
+	}
+
+	if (pdata->pdt_fallback_size < 0) {
+		dev_err(dev, "syna,pdt-fallback-desc property was not specified properly: %d\n",
+			pdata->pdt_fallback_size);
+		return pdata->pdt_fallback_size;
+	}
+
+	pdata->pdt_fallback_desc = devm_kzalloc(dev, pdata->pdt_fallback_size, GFP_KERNEL);
+
+	retval = of_property_read_u8_array(dev->of_node, "syna,pdt-fallback-desc",
+		pdata->pdt_fallback_desc, pdata->pdt_fallback_size);
+	if (retval < 0)
+		return retval;
+
 	return 0;
 }
 #else
-static inline int rmi_driver_of_probe(struct device *dev,
-					struct rmi_device_platform_data *pdata)
+static inline int rmi_driver_of_probe(struct rmi_device *rmi_dev,
+				      struct rmi_device_platform_data *pdata)
 {
 	return -ENODEV;
 }
@@ -1163,7 +1220,7 @@ static int rmi_driver_probe(struct device *dev)
 	pdata = rmi_get_platform_data(rmi_dev);
 
 	if (rmi_dev->xport->dev->of_node) {
-		retval = rmi_driver_of_probe(rmi_dev->xport->dev, pdata);
+		retval = rmi_driver_of_probe(rmi_dev, pdata);
 		if (retval)
 			return retval;
 	}
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 0ecbd19cefffe00ce7c8001db6479a2e213ac466..67a04133a4d85c56d80d0eafee65797319c75c54 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -31,6 +31,8 @@
 #define RMI_PDT_FUNCTION_VERSION_MASK   0x60
 #define RMI_PDT_INT_SOURCE_COUNT_MASK   0x07
 
+#define RMI_OF_PDT_DESC_CELLS 2
+
 #define PDT_START_SCAN_LOCATION 0x00e9
 #define PDT_END_SCAN_LOCATION	0x0005
 #define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index ab7eea01ab4274bfc9efcefcdb0cced6ec34966f..974597960b5ee61b670ab55a78c317710022d7cc 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -214,6 +214,9 @@ struct rmi_device_platform_data {
 	int reset_delay_ms;
 	int irq;
 
+	u8 *pdt_fallback_desc;
+	int pdt_fallback_size;
+
 	struct rmi_device_platform_data_spi spi_data;
 
 	/* function handler pdata */

-- 
2.47.2

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[Krzysztof Kozlowski]

On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
> From: Caleb Connolly <caleb.connolly@linaro.org>
> 
> This new property allows devices to specify some register values which
> are missing on units with third party replacement displays. These
> displays use unofficial touch ICs which only implement a subset of the
> RMI4 specification.

These are different ICs, so they have their own compatibles. Why this
cannot be deduced from the compatible?

> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  Documentation/devicetree/bindings/input/syna,rmi4.yaml | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> index b522c8d3ce0db719ff379f2fefbdca79e73d027c..a80ec0c052cb1b7278f0832dd18ebd3256bc0874 100644
> --- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> +++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
> @@ -49,6 +49,24 @@ properties:
>      description:
>        Delay to wait after powering on the device.
>  
> +  syna,pdt-fallback-desc:
> +    $ref: /schemas/types.yaml#/definitions/uint8-matrix
> +    description:
> +      This property provides fallback values for certain register fields that
> +      are missing on devices using third-party replacement displays.
> +      These unofficial displays contain touch controllers that claim RMI4
> +      compliance but fail to populate the function_number and function_version
> +      registers of their Page Descriptor Table (PDT) entries.
> +
> +      Since the number of required fallback entries depends on the number of
> +      Page Descriptor Tables supported by a given device, this property
> +      should be provided on a best-effort basis.
> +
> +    items:

min/maxItems here

> +      items:
> +        - description: The 5th byte of the PDT entry (function number)
> +        - description: The 4th byte of the PDT entry (version value)

Best regards,
Krzysztof

Re: [PATCH v3 0/7] Subject: [PATCH v3 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers

[Caleb Connolly]

Hi David,

Please at least give me a heads up if you're going to resend a patch 
series of mine. I understand it's an old series but I don't think that 
courtesy is too much to ask.

On 3/8/25 14:08, David Heidelberg via B4 Relay wrote:
> With the growing popularity of running upstream Linux on mobile devices,
> we're beginning to run into more and more edgecases. The OnePlus 6 is a
> fairly well supported 2018 era smartphone, selling over a million units
> in it's first 22 days. With this level of popularity, it's almost
> inevitable that we get third party replacement displays, and as a
> result, replacement touchscreen controllers.
> 
> The OnePlus 6 shipped with an extremely usecase specific touchscreen
> driver, it implemented only the bare minimum parts of the highly generic
> rmi4 protocol, instead hardcoding most of the register addresses.
>    
> As a result, the third party touchscreen controllers that are often
> found in replacement screens, implement only the registers that the
> downstream driver reads from. They additionally have other restrictions
> such as heavy penalties on unaligned reads.
>   
> This series attempts to implement the necessary workaround to support
> some of these chips with the rmi4 driver. Although it's worth noting
> that at the time of writing there are other unofficial controllers in
> the wild that don't work even with these patches.
>   
> We have been shipping these patches in postmarketOS for the last several
> months, and they are known to not cause any regressions on the OnePlus
> 6/6T (with the official Synaptics controller), however I don't own any
> other rmi4 hardware to further validate this.
> 
> ---
> Changes since v2:
> - reworded dt-bindings property description
> - fixed the rmi_driver_of_probe definition for non device-tree builds.
> - fixed some indentation issues reported by checkpatch
> - change rmi_pdt_entry_is_valid() variable to unsigned
> - Link to v2: https://patchwork.kernel.org/project/linux-input/cover/20230929-caleb-rmi4-quirks-v2-0-b227ac498d88@linaro.org/

Please use lore links
> 
> Changes since v1:
> - Improve dt-bindings patch (thanks Rob)
> - Add missing cast in patch 5 to fix the pointer arithmetic
> - Link to v1: https://lore.kernel.org/r/20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org
> 
> ---
> Caleb Connolly (2):
>        dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
>        Input: synaptics-rmi4 - handle duplicate/unknown PDT entries
> 
> methanal (5):
>        Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
>        Input: synaptics-rmi4 - f55: handle zero electrode count
>        Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
>        Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
>        Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes
> 
>   .../devicetree/bindings/input/syna,rmi4.yaml       |  18 +++
>   drivers/input/rmi4/rmi_driver.c                    | 140 +++++++++++++++++----
>   drivers/input/rmi4/rmi_driver.h                    |   8 ++
>   drivers/input/rmi4/rmi_f01.c                       |  14 +++
>   drivers/input/rmi4/rmi_f12.c                       | 117 +++++++++++++----
>   drivers/input/rmi4/rmi_f55.c                       |   5 +
>   include/linux/rmi.h                                |   3 +
>   7 files changed, 258 insertions(+), 47 deletions(-)
> ---
> base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
> change-id: 20250308-synaptics-rmi4-c832b2f73ceb
> 
> Best regards,

-- 
Caleb (they/them)

Re: [PATCH v3 0/7] Subject: [PATCH v3 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers

[David Heidelberg]

Hello Caleb,

I'm very sorry about that. Next time I include your patches in the 
series I'll definitely send you heads up.

David

On 10/03/2025 11:04, Caleb Connolly wrote:
> Hi David,
> 
> Please at least give me a heads up if you're going to resend a patch 
> series of mine. I understand it's an old series but I don't think that 
> courtesy is too much to ask.
> 
> On 3/8/25 14:08, David Heidelberg via B4 Relay wrote:
>> With the growing popularity of running upstream Linux on mobile devices,
>> we're beginning to run into more and more edgecases. The OnePlus 6 is a
>> fairly well supported 2018 era smartphone, selling over a million units
>> in it's first 22 days. With this level of popularity, it's almost
>> inevitable that we get third party replacement displays, and as a
>> result, replacement touchscreen controllers.
>>
>> The OnePlus 6 shipped with an extremely usecase specific touchscreen
>> driver, it implemented only the bare minimum parts of the highly generic
>> rmi4 protocol, instead hardcoding most of the register addresses.
>> As a result, the third party touchscreen controllers that are often
>> found in replacement screens, implement only the registers that the
>> downstream driver reads from. They additionally have other restrictions
>> such as heavy penalties on unaligned reads.
>> This series attempts to implement the necessary workaround to support
>> some of these chips with the rmi4 driver. Although it's worth noting
>> that at the time of writing there are other unofficial controllers in
>> the wild that don't work even with these patches.
>> We have been shipping these patches in postmarketOS for the last several
>> months, and they are known to not cause any regressions on the OnePlus
>> 6/6T (with the official Synaptics controller), however I don't own any
>> other rmi4 hardware to further validate this.
>>
>> ---
>> Changes since v2:
>> - reworded dt-bindings property description
>> - fixed the rmi_driver_of_probe definition for non device-tree builds.
>> - fixed some indentation issues reported by checkpatch
>> - change rmi_pdt_entry_is_valid() variable to unsigned
>> - Link to v2: https://patchwork.kernel.org/project/linux-input/ 
>> cover/20230929-caleb-rmi4-quirks-v2-0-b227ac498d88@linaro.org/
> 
> Please use lore links
>>
>> Changes since v1:
>> - Improve dt-bindings patch (thanks Rob)
>> - Add missing cast in patch 5 to fix the pointer arithmetic
>> - Link to v1: https://lore.kernel.org/r/20230929-caleb-rmi4-quirks- 
>> v1-0-cc3c703f022d@linaro.org
>>
>> ---
>> Caleb Connolly (2):
>>        dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
>>        Input: synaptics-rmi4 - handle duplicate/unknown PDT entries
>>
>> methanal (5):
>>        Input: synaptics-rmi4 - f12: use hardcoded values for 
>> aftermarket touch ICs
>>        Input: synaptics-rmi4 - f55: handle zero electrode count
>>        Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
>>        Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
>>        Input: synaptics-rmi4 - support fallback values for PDT 
>> descriptor bytes
>>
>>   .../devicetree/bindings/input/syna,rmi4.yaml       |  18 +++
>>   drivers/input/rmi4/rmi_driver.c                    | 140 +++++++++++ 
>> ++++++----
>>   drivers/input/rmi4/rmi_driver.h                    |   8 ++
>>   drivers/input/rmi4/rmi_f01.c                       |  14 +++
>>   drivers/input/rmi4/rmi_f12.c                       | 117 +++++++++++ 
>> ++----
>>   drivers/input/rmi4/rmi_f55.c                       |   5 +
>>   include/linux/rmi.h                                |   3 +
>>   7 files changed, 258 insertions(+), 47 deletions(-)
>> ---
>> base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
>> change-id: 20250308-synaptics-rmi4-c832b2f73ceb
>>
>> Best regards,
> 

-- 
David Heidelberg

Re: [PATCH v3 4/7] Input: synaptics-rmi4 - f55: handle zero electrode count

[Dmitry Torokhov]

On Sat, Mar 08, 2025 at 03:08:40PM +0100, David Heidelberg via B4 Relay wrote:
> From: methanal <baclofen@tuta.io>
> 
> Some third party ICs claim to support f55 but report an electrode count
> of 0. Catch this and bail out early so that we don't confuse the i2c bus
> with 0 sized reads.
> 
> Signed-off-by: methanal <baclofen@tuta.io>
> [simplify code, adjust wording]
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/input/rmi4/rmi_f55.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
> index 488adaca4dd00482cd1106d813b32871092c83a0..ad2ef14ae9f4e897473db43334792cc3de966d52 100644
> --- a/drivers/input/rmi4/rmi_f55.c
> +++ b/drivers/input/rmi4/rmi_f55.c
> @@ -52,6 +52,11 @@ static int rmi_f55_detect(struct rmi_function *fn)
>  
>  	f55->num_rx_electrodes = f55->qry[F55_NUM_RX_OFFSET];
>  	f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
> +	if (!f55->num_rx_electrodes || !f55->num_tx_electrodes) {
> +		rmi_dbg(RMI_DEBUG_FN, &fn->dev,
> +			"F55 query returned no electrodes, giving up\n");
> +		return 0;

0 here means "successfully detected" and will result in F55 probe
succeeding. I expect you want -EINVAL or -ENODEV here.

Thanks.

-- 
Dmitry

Re: [PATCH v3 2/7] Input: synaptics-rmi4 - handle duplicate/unknown PDT entries

[Dmitry Torokhov]

Hi David,

On Sat, Mar 08, 2025 at 03:08:38PM +0100, David Heidelberg via B4 Relay wrote:
> From: Caleb Connolly <caleb.connolly@linaro.org>
> 
> Some third party rmi4-compatible ICs don't expose their PDT entries
> very well. Add a few checks to skip duplicate entries as well as entries
> for unsupported functions.
> 
> This is required to support some phones with third party displays.
> 
> Validated on a stock OnePlus 6T (original parts):
> manufacturer: Synaptics, product: S3706B, fw id: 2852315
> 
> Co-developed-by: methanal <baclofen@tuta.io>
> Signed-off-by: methanal <baclofen@tuta.io>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------
>  drivers/input/rmi4/rmi_driver.h |  6 ++++++
>  2 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
>  	fd->function_version = pdt->function_version;
>  }
>  
> +static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev,
> +				   struct pdt_scan_state *state, u8 fn)
> +{
> +	unsigned int i;
> +
> +	switch (fn) {
> +	case 0x01:
> +	case 0x03:
> +	case 0x11:
> +	case 0x12:
> +	case 0x30:
> +	case 0x34:
> +	case 0x3a:
> +	case 0x54:
> +	case 0x55:

This mean that we need to update this code any time there is new
function introduced. I'd rather we did not do that. The driver should be
able to handle unknown functions.

> +		break;
> +
> +	default:
> +		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
> +			"PDT has unknown function number %#02x\n", fn);
> +		return false;
> +	}
> +
> +	for (i = 0; i < state->pdt_count; i++) {
> +		if (state->pdts[i] == fn)
> +			return false;
> +	}
> +
> +	state->pdts[state->pdt_count++] = fn;

Duplicate detection could be handled thorough a bitmap.

Thanks.

-- 
Dmitry

Re: [PATCH v3 2/7] Input: synaptics-rmi4 - handle duplicate/unknown PDT entries

[Caleb Connolly]

Hi Dmitry,

On 3/10/25 19:10, Dmitry Torokhov wrote:
> Hi David,
> 
> On Sat, Mar 08, 2025 at 03:08:38PM +0100, David Heidelberg via B4 Relay wrote:
>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>
>> Some third party rmi4-compatible ICs don't expose their PDT entries
>> very well. Add a few checks to skip duplicate entries as well as entries
>> for unsupported functions.
>>
>> This is required to support some phones with third party displays.
>>
>> Validated on a stock OnePlus 6T (original parts):
>> manufacturer: Synaptics, product: S3706B, fw id: 2852315
>>
>> Co-developed-by: methanal <baclofen@tuta.io>
>> Signed-off-by: methanal <baclofen@tuta.io>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> Signed-off-by: David Heidelberg <david@ixit.cz>
>> ---
>>   drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------
>>   drivers/input/rmi4/rmi_driver.h |  6 ++++++
>>   2 files changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
>>   	fd->function_version = pdt->function_version;
>>   }
>>   
>> +static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev,
>> +				   struct pdt_scan_state *state, u8 fn)
>> +{
>> +	unsigned int i;
>> +
>> +	switch (fn) {
>> +	case 0x01:
>> +	case 0x03:
>> +	case 0x11:
>> +	case 0x12:
>> +	case 0x30:
>> +	case 0x34:
>> +	case 0x3a:
>> +	case 0x54:
>> +	case 0x55:
> 
> This mean that we need to update this code any time there is new
> function introduced. I'd rather we did not do that. The driver should be
> able to handle unknown functions.

Synaptics don't produce new RMI4 based tech anymore afaik, they have 
moved on. So I don't think there will be new functions being added here.

Unfortunately in my experience the fake touch ICs report completely 
random values here, so there really isn't a good way to handle this 
otherwise.

What if this list rather than being hardcoded here was just using the 
fn_handlers[] array from rmi_bus.c?

Kind regards,
> 
>> +		break;
>> +
>> +	default:
>> +		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
>> +			"PDT has unknown function number %#02x\n", fn);
>> +		return false;
>> +	}
>> +
>> +	for (i = 0; i < state->pdt_count; i++) {
>> +		if (state->pdts[i] == fn)
>> +			return false;
>> +	}
>> +
>> +	state->pdts[state->pdt_count++] = fn;
> 
> Duplicate detection could be handled thorough a bitmap.
> 
> Thanks.
> 

-- 
Caleb (they/them)

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[David Heidelberg]

On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>
>> This new property allows devices to specify some register values which
>> are missing on units with third party replacement displays. These
>> displays use unofficial touch ICs which only implement a subset of the
>> RMI4 specification.
> 
> These are different ICs, so they have their own compatibles. Why this
> cannot be deduced from the compatible?

Yes, but these identify as the originals.

Some of them can be detected at runtime which is what this patchset 
tries to do.

> 
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> Signed-off-by: David Heidelberg <david@ixit.cz>
>> ---
>>   Documentation/devicetree/bindings/input/syna,rmi4.yaml | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
>> index b522c8d3ce0db719ff379f2fefbdca79e73d027c..a80ec0c052cb1b7278f0832dd18ebd3256bc0874 100644
>> --- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
>> +++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
>> @@ -49,6 +49,24 @@ properties:
>>       description:
>>         Delay to wait after powering on the device.
>>   
>> +  syna,pdt-fallback-desc:
>> +    $ref: /schemas/types.yaml#/definitions/uint8-matrix
>> +    description:
>> +      This property provides fallback values for certain register fields that
>> +      are missing on devices using third-party replacement displays.
>> +      These unofficial displays contain touch controllers that claim RMI4
>> +      compliance but fail to populate the function_number and function_version
>> +      registers of their Page Descriptor Table (PDT) entries.
>> +
>> +      Since the number of required fallback entries depends on the number of
>> +      Page Descriptor Tables supported by a given device, this property
>> +      should be provided on a best-effort basis.
>> +
>> +    items:
> 
> min/maxItems here

There is no hardcoded limit how many of these items will be provided, 
should I put there some huge range as 1..1024?

Thank you
David

> 
>> +      items:
>> +        - description: The 5th byte of the PDT entry (function number)
>> +        - description: The 4th byte of the PDT entry (version value)
> 
> Best regards,
> Krzysztof
> 

-- 
David Heidelberg

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[Krzysztof Kozlowski]

On 24/03/2025 19:00, David Heidelberg wrote:
> On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
>> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>>
>>> This new property allows devices to specify some register values which
>>> are missing on units with third party replacement displays. These
>>> displays use unofficial touch ICs which only implement a subset of the
>>> RMI4 specification.
>>
>> These are different ICs, so they have their own compatibles. Why this
>> cannot be deduced from the compatible?
> 
> Yes, but these identify as the originals.


It does not matter how they identify. You have the compatible for them.
If you cannot add compatible for them, how can you add dedicated
property for them?

Best regards,
Krzysztof

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[Caleb Connolly]

On 3/25/25 08:36, Krzysztof Kozlowski wrote:
> On 24/03/2025 19:00, David Heidelberg wrote:
>> On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
>>> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>>>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>>>
>>>> This new property allows devices to specify some register values which
>>>> are missing on units with third party replacement displays. These
>>>> displays use unofficial touch ICs which only implement a subset of the
>>>> RMI4 specification.
>>>
>>> These are different ICs, so they have their own compatibles. Why this
>>> cannot be deduced from the compatible?
>>
>> Yes, but these identify as the originals.
> 
> 
> It does not matter how they identify. You have the compatible for them.
> If you cannot add compatible for them, how can you add dedicated
> property for them?

Hi Krzysztof,

There are an unknown number of knock-off RMI4 chips which are sold in 
cheap replacement display panels from multiple vendors. We suspect 
there's more than one implementation.

A new compatible string wouldn't help us, since we use the same DTB on 
fully original hardware as on hardware with replacement parts.

The proposed new property describes configuration registers which are 
present on original RMI4 chips but missing on the third party ones, the 
contents of the registers is static.

The third party chips were designed to work with a specific downstream 
driver which doesn't implement the self-describing features of RMI4 and 
just hardcoded the functionality they expect.

Kind regards,
> 
> Best regards,
> Krzysztof

-- 
Caleb (they/them)

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[Krzysztof Kozlowski]

On 25/03/2025 14:23, Caleb Connolly wrote:
> 
> 
> On 3/25/25 08:36, Krzysztof Kozlowski wrote:
>> On 24/03/2025 19:00, David Heidelberg wrote:
>>> On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
>>>> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>>>>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>>>>
>>>>> This new property allows devices to specify some register values which
>>>>> are missing on units with third party replacement displays. These
>>>>> displays use unofficial touch ICs which only implement a subset of the
>>>>> RMI4 specification.
>>>>
>>>> These are different ICs, so they have their own compatibles. Why this
>>>> cannot be deduced from the compatible?
>>>
>>> Yes, but these identify as the originals.
>>
>>
>> It does not matter how they identify. You have the compatible for them.
>> If you cannot add compatible for them, how can you add dedicated
>> property for them?
> 
> Hi Krzysztof,
> 
> There are an unknown number of knock-off RMI4 chips which are sold in 
> cheap replacement display panels from multiple vendors. We suspect 
> there's more than one implementation.
> 
> A new compatible string wouldn't help us, since we use the same DTB on 
> fully original hardware as on hardware with replacement parts.
> 
> The proposed new property describes configuration registers which are 
> present on original RMI4 chips but missing on the third party ones, the 
> contents of the registers is static.


So you want to add redundant information for existing compatible, while
claiming you cannot deduce it from that existing compatible... Well,
no.. you cannot be sure that only chosen boards will have touchscreens
replaced, thus you will have to add this property to every board using
this compatible making it equal to the compatible and we are back at my
original comment. This is deducible from the compatible. If not the new
one, then from old one.

Best regards,
Krzysztof

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[Caleb Connolly]

On 3/26/25 07:57, Krzysztof Kozlowski wrote:
> On 25/03/2025 14:23, Caleb Connolly wrote:
>>
>>
>> On 3/25/25 08:36, Krzysztof Kozlowski wrote:
>>> On 24/03/2025 19:00, David Heidelberg wrote:
>>>> On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
>>>>> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>>>>>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>>>>>
>>>>>> This new property allows devices to specify some register values which
>>>>>> are missing on units with third party replacement displays. These
>>>>>> displays use unofficial touch ICs which only implement a subset of the
>>>>>> RMI4 specification.
>>>>>
>>>>> These are different ICs, so they have their own compatibles. Why this
>>>>> cannot be deduced from the compatible?
>>>>
>>>> Yes, but these identify as the originals.
>>>
>>>
>>> It does not matter how they identify. You have the compatible for them.
>>> If you cannot add compatible for them, how can you add dedicated
>>> property for them?
>>
>> Hi Krzysztof,
>>
>> There are an unknown number of knock-off RMI4 chips which are sold in
>> cheap replacement display panels from multiple vendors. We suspect
>> there's more than one implementation.
>>
>> A new compatible string wouldn't help us, since we use the same DTB on
>> fully original hardware as on hardware with replacement parts.
>>
>> The proposed new property describes configuration registers which are
>> present on original RMI4 chips but missing on the third party ones, the
>> contents of the registers is static.
> 
> 
> So you want to add redundant information for existing compatible, while
> claiming you cannot deduce it from that existing compatible... Well,
> no.. you cannot be sure that only chosen boards will have touchscreens
> replaced, thus you will have to add this property to every board using
> this compatible making it equal to the compatible and we are back at my
> original comment. This is deducible from the compatible. If not the new
> one, then from old one.

hmm I see, so instead we should add a compatible for the specific 
variant (S3320 or something) of RMI4 in this device and handle this in 
the driver? I think that makes sense.

> 
> Best regards,
> Krzysztof

-- 
Caleb (they/them)

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[David Heidelberg]

On 26/03/2025 11:26, Caleb Connolly wrote:
> 
> 
> On 3/26/25 07:57, Krzysztof Kozlowski wrote:
>> On 25/03/2025 14:23, Caleb Connolly wrote:
>>>
>>>
>>> On 3/25/25 08:36, Krzysztof Kozlowski wrote:
>>>> On 24/03/2025 19:00, David Heidelberg wrote:
>>>>> On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
>>>>>> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>>>>>>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>>>>>>
>>>>>>> This new property allows devices to specify some register values 
>>>>>>> which
>>>>>>> are missing on units with third party replacement displays. These
>>>>>>> displays use unofficial touch ICs which only implement a subset 
>>>>>>> of the
>>>>>>> RMI4 specification.
>>>>>>
>>>>>> These are different ICs, so they have their own compatibles. Why this
>>>>>> cannot be deduced from the compatible?
>>>>>
>>>>> Yes, but these identify as the originals.
>>>>
>>>>
>>>> It does not matter how they identify. You have the compatible for them.
>>>> If you cannot add compatible for them, how can you add dedicated
>>>> property for them?
>>>
>>> Hi Krzysztof,
>>>
>>> There are an unknown number of knock-off RMI4 chips which are sold in
>>> cheap replacement display panels from multiple vendors. We suspect
>>> there's more than one implementation.
>>>
>>> A new compatible string wouldn't help us, since we use the same DTB on
>>> fully original hardware as on hardware with replacement parts.
>>>
>>> The proposed new property describes configuration registers which are
>>> present on original RMI4 chips but missing on the third party ones, the
>>> contents of the registers is static.
>>
>>
>> So you want to add redundant information for existing compatible, while
>> claiming you cannot deduce it from that existing compatible... Well,
>> no.. you cannot be sure that only chosen boards will have touchscreens
>> replaced, thus you will have to add this property to every board using
>> this compatible making it equal to the compatible and we are back at my
>> original comment. This is deducible from the compatible. If not the new
>> one, then from old one.
> 
> hmm I see, so instead we should add a compatible for the specific 
> variant (S3320 or something) of RMI4 in this device and handle this in 
> the driver? I think that makes sense.

Agree, preparing it for v4. So far proposing `compatible = 
"syna,rmi4-s3706b-i2c", "syna,rmi4-i2c"` (as S3706B is written in the 
commit and search confirms it for OP6/6T).

David>
>>
>> Best regards,
>> Krzysztof
> 

-- 
David Heidelberg

Re: [PATCH v3 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc

[Caleb Connolly]

On 3/28/25 23:45, David Heidelberg wrote:
> On 26/03/2025 11:26, Caleb Connolly wrote:
>>
>>
>> On 3/26/25 07:57, Krzysztof Kozlowski wrote:
>>> On 25/03/2025 14:23, Caleb Connolly wrote:
>>>>
>>>>
>>>> On 3/25/25 08:36, Krzysztof Kozlowski wrote:
>>>>> On 24/03/2025 19:00, David Heidelberg wrote:
>>>>>> On 10/03/2025 10:45, Krzysztof Kozlowski wrote:
>>>>>>> On Sat, Mar 08, 2025 at 03:08:37PM +0100, David Heidelberg wrote:
>>>>>>>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>>>>>>>
>>>>>>>> This new property allows devices to specify some register values 
>>>>>>>> which
>>>>>>>> are missing on units with third party replacement displays. These
>>>>>>>> displays use unofficial touch ICs which only implement a subset 
>>>>>>>> of the
>>>>>>>> RMI4 specification.
>>>>>>>
>>>>>>> These are different ICs, so they have their own compatibles. Why 
>>>>>>> this
>>>>>>> cannot be deduced from the compatible?
>>>>>>
>>>>>> Yes, but these identify as the originals.
>>>>>
>>>>>
>>>>> It does not matter how they identify. You have the compatible for 
>>>>> them.
>>>>> If you cannot add compatible for them, how can you add dedicated
>>>>> property for them?
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> There are an unknown number of knock-off RMI4 chips which are sold in
>>>> cheap replacement display panels from multiple vendors. We suspect
>>>> there's more than one implementation.
>>>>
>>>> A new compatible string wouldn't help us, since we use the same DTB on
>>>> fully original hardware as on hardware with replacement parts.
>>>>
>>>> The proposed new property describes configuration registers which are
>>>> present on original RMI4 chips but missing on the third party ones, the
>>>> contents of the registers is static.
>>>
>>>
>>> So you want to add redundant information for existing compatible, while
>>> claiming you cannot deduce it from that existing compatible... Well,
>>> no.. you cannot be sure that only chosen boards will have touchscreens
>>> replaced, thus you will have to add this property to every board using
>>> this compatible making it equal to the compatible and we are back at my
>>> original comment. This is deducible from the compatible. If not the new
>>> one, then from old one.
>>
>> hmm I see, so instead we should add a compatible for the specific 
>> variant (S3320 or something) of RMI4 in this device and handle this in 
>> the driver? I think that makes sense.
> 
> Agree, preparing it for v4. So far proposing `compatible = "syna,rmi4- 
> s3706b-i2c", "syna,rmi4-i2c"` (as S3706B is written in the commit and 
> search confirms it for OP6/6T).

ack, sounds good!
> 
> David>
>>>
>>> Best regards,
>>> Krzysztof
>>
> 

-- 
Caleb (they/them)

Re: [PATCH v3 2/7] Input: synaptics-rmi4 - handle duplicate/unknown PDT entries

[David Heidelberg]

On 10/03/2025 20:10, Dmitry Torokhov wrote:
> Hi David,
> 
> On Sat, Mar 08, 2025 at 03:08:38PM +0100, David Heidelberg via B4 Relay wrote:
>> From: Caleb Connolly <caleb.connolly@linaro.org>
>>
>> Some third party rmi4-compatible ICs don't expose their PDT entries
>> very well. Add a few checks to skip duplicate entries as well as entries
>> for unsupported functions.
>>
>> This is required to support some phones with third party displays.
>>
>> Validated on a stock OnePlus 6T (original parts):
>> manufacturer: Synaptics, product: S3706B, fw id: 2852315
>>
>> Co-developed-by: methanal <baclofen@tuta.io>
>> Signed-off-by: methanal <baclofen@tuta.io>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> Signed-off-by: David Heidelberg <david@ixit.cz>
>> ---
>>   drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------
>>   drivers/input/rmi4/rmi_driver.h |  6 ++++++
>>   2 files changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
>> index 2168b6cd7167334d44553c9c566f870a4e034179..51c23a407b2731d5b6eaefe9cae6288f97316e34 100644
>> --- a/drivers/input/rmi4/rmi_driver.c
>> +++ b/drivers/input/rmi4/rmi_driver.c
>> @@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
>>   	fd->function_version = pdt->function_version;
>>   }
>>   
>> +static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev,
>> +				   struct pdt_scan_state *state, u8 fn)
>> +{
>> +	unsigned int i;
>> +
>> +	switch (fn) {
>> +	case 0x01:
>> +	case 0x03:
>> +	case 0x11:
>> +	case 0x12:
>> +	case 0x30:
>> +	case 0x34:
>> +	case 0x3a:
>> +	case 0x54:
>> +	case 0x55:
> 
> This mean that we need to update this code any time there is new
> function introduced. I'd rather we did not do that. The driver should be
> able to handle unknown functions.

Hello Dmitry,

I hope the final state of Synaptics RMI4 described by Caleb was 
convincing for you, I sent v4, if you insist on re-doing this part, I'll 
do in v5 :)

> 
>> +		break;
>> +
>> +	default:
>> +		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
>> +			"PDT has unknown function number %#02x\n", fn);
>> +		return false;
>> +	}
>> +
>> +	for (i = 0; i < state->pdt_count; i++) {
>> +		if (state->pdts[i] == fn)
>> +			return false;
>> +	}
>> +
>> +	state->pdts[state->pdt_count++] = fn;
> 
> Duplicate detection could be handled thorough a bitmap.

Done

Thank you
David>
> Thanks.
> 

-- 
David Heidelberg