[PATCH v3 0/5] Add support for Focaltech FTS Touchscreen

[PATCH v3 0/5] Add support for Focaltech FTS Touchscreen

[Joel Selvaraj]

Changes in v3:(Suggested by Krzysztof Kozlowski and Konrad Dybcio)
--------------
- dts: removed the invalid "input-enable" property
- dts: replace interrupts with interrupts-extended
- dts: removed redundant dma configuration
- dts: reorder pinctrl and pinctrl-names
- bindings: moved unevaluatedProperties after required
- bindings: make interrupts a required property (new change from my end)
- bindings: update example based on dts changes

I have made the interrupts a required property in the bindings as the driver
will not function without an interrupt. Because of this new change, I have not
picked up the reviewed by tag of Krzysztof Kozlowski for the bindings.

Changes in v2:
--------------
1. dt-bindings changes (Suggested by Krzysztof Kozlowski)
	- changed file name from focaltech,fts.yaml to focaltech,fts5452.yaml
	- removed focaltech,max-touch-number property, handled in driver now
	- removed touchscreen-* properties and used unevaluatedProperties: false
	instead of additionalProperties: false
	- fixed the example dts node name to be generic
	
2. FTS Touchscreen driver changes (Suggested by Markuss Broks and Jeff LaBundy)
	- removed repeated license terms since SPDX tag is used
	- includes are now sorted
	- added the missing input_mt_sync_frame when reporting touch
	- focaltech,max-touch-number is no longer read from dts and instead
	specified in the driver as compatible data.
	- removed redundant __set_bits
	- input_mt_init_slots is now called after the axes are defined
	- irq handler now returns IRQ_NONE when there is an i2c error
	- other minor fixes and refactoring as suggested
	- renamed driver filename from focaltech_fts.c to focaltech_fts5452.c
	(Suggested by Krzysztof Kozlowski)
	
3. dts changes (Suggested by Krzysztof Kozlowski)
	- use generic touchscreen nodes
	- removed focaltech,max-touch-number property
	- irq type was specified wrongly for Poco F1 in v1. Changed the irq
	type to IRQ_TYPE_EDGE_FALLING as that is correct.

Some Clarifications on v1 comments:
-----------------------------------
1. Jeff LaBundy suggested I could read chip id with the following:
	__be16 val;
	regmap_raw_read(data->regmap, FTS_REG_CHIP_ID_H, &val, sizeof(val));
But this is not possible because FTS_REG_CHIP_ID_H and FTS_REG_CHIP_ID_L
are not continuous register, therefore reading it together as 16-bit values
will not work. So I went with what Markuss Broks suggested:
	regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
        regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
        id |= val << 8;

2. As Markuss Broks suggested, I tried to cast the buffer to struct, but 
unfortunately was not able to successfully do it. The buffer layout is 
weirdly split into 4 bits and 12 bits at someplaces which makes it hard 
to cast into a struct. For example, we can note
	type = buf[base + 3] >> 6
	x = ((buf[base + 3] & 0x0F) << 8) + (buf[base + 4] & 0xFF);
Here at buffer index 3, the first two bits (>>6) are used for denoting
event type. The next two bits are not used. But the last 4 bits (&0x0F)
of buffer[3] are added with buffer index 4 to get the x position. 
I don't know how to handle these when casting to a struct. I tried
experimenting with bitfields in struct, but to no avail. So I am sticking
with my initial implementation for now.

Kindly let me know if any further improvements are needed. Thanks.

The Focaltech FTS driver supports several variants of focaltech
touchscreens found in ~2018 era smartphones including variants found on
the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
This driver is loosely based on the original driver from Focaltech and
the patches submitted by Caleb Connolly previously[1] but has been
simplified and largely reworked.

[1] https://patchwork.kernel.org/project/linux-input/patch/20220123173650.290349-3-caleb@connolly.tech/

Joel Selvaraj (5):
  dt-bindings: input: touchscreen: add focaltech,fts5452 touchscreen
  Input: add driver for Focaltech FTS touchscreen
  arm64: dts: qcom: sdm845-xiaomi-beryllium-common: add touchscreen
    related nodes
  arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for
    fts touchscreen
  arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen
    properties

 .../input/touchscreen/focaltech,fts5452.yaml  |  71 +++
 MAINTAINERS                                   |   8 +
 .../boot/dts/qcom/sdm845-shift-axolotl.dts    |  18 +-
 .../qcom/sdm845-xiaomi-beryllium-common.dtsi  |  37 ++
 .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts |  21 +
 drivers/input/touchscreen/Kconfig             |  12 +
 drivers/input/touchscreen/Makefile            |   1 +
 drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
 8 files changed, 590 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
 create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c

-- 
2.40.0

[PATCH v3 1/5] dt-bindings: input: touchscreen: add focaltech,fts5452 touchscreen

[Joel Selvaraj]

Document the Focaltech FTS touchscreen driver.

Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
---
 .../input/touchscreen/focaltech,fts5452.yaml  | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
new file mode 100644
index 000000000000..28270dc5ed67
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/focaltech,fts5452.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Focaltech FTS I2C Touchscreen Controller
+
+maintainers:
+  - Joel Selvaraj <joelselvaraj.oss@gmail.com>
+  - Caleb Connolly <caleb@connolly.tech>
+
+allOf:
+  - $ref: touchscreen.yaml#
+
+properties:
+  compatible:
+    enum:
+      - focaltech,fts5452
+      - focaltech,fts8719
+
+  reg:
+    const: 0x38
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  avdd-supply:
+    description: regulator supplying analog power (2.6V to 3.3V).
+
+  vddio-supply:
+    description: regulator supplying IO power (1.8V).
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@38 {
+        compatible = "focaltech,fts5452";
+        reg = <0x38>;
+
+        interrupts-extended = <&tlmm 125 IRQ_TYPE_EDGE_FALLING>;
+        reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+        avdd-supply = <&vreg_l28a_3p0>;
+        vddio-supply = <&vreg_l14a_1p88>;
+
+        pinctrl-0 = <&ts_int_active &ts_reset_active>;
+        pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
+        pinctrl-names = "default", "suspend";
+
+        touchscreen-size-x = <1080>;
+        touchscreen-size-y = <2160>;
+      };
+    };
-- 
2.40.0

[PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Joel Selvaraj]

The Focaltech FTS driver supports several variants of focaltech
touchscreens found in ~2018 era smartphones including variants found on
the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
This driver is loosely based on the original driver from Focaltech
but has been simplified and largely reworked.

Co-developed-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
 MAINTAINERS                                   |   8 +
 drivers/input/touchscreen/Kconfig             |  12 +
 drivers/input/touchscreen/Makefile            |   1 +
 drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
 4 files changed, 453 insertions(+)
 create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7ec4ce64f66d..1a3ea61e1f52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8028,6 +8028,14 @@ L:	linux-input@vger.kernel.org
 S:	Maintained
 F:	drivers/input/joystick/fsia6b.c
 
+FOCALTECH FTS5452 TOUCHSCREEN DRIVER
+M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
+M:	Caleb Connolly <caleb@connolly.tech>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
+F:	drivers/input/touchscreen/focaltech_fts5452.c
+
 FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
 M:	Geoffrey D. Bennett <g@b4.vu>
 L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1feecd7ed3cb..11af91504969 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
 	  To compile this driver as a module, choose M here: the
 	  module will be called exc3000.
 
+config TOUCHSCREEN_FOCALTECH_FTS5452
+	tristate "Focaltech FTS Touchscreen"
+	depends on I2C
+	help
+	  Say Y here to enable support for I2C connected Focaltech FTS
+	  based touch panels, including the 5452 and 8917 panels.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called focaltech_fts.
+
 config TOUCHSCREEN_FUJITSU
 	tristate "Fujitsu serial touchscreen"
 	select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 159cd5136fdb..47d78c9cff21 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
 obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
 obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
 obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
+obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452)	+= focaltech_fts5452.o
 obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
 obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
 obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
new file mode 100644
index 000000000000..abf8a2f271ca
--- /dev/null
+++ b/drivers/input/touchscreen/focaltech_fts5452.c
@@ -0,0 +1,432 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * FocalTech touchscreen driver.
+ *
+ * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
+ * Copyright (C) 2018 XiaoMi, Inc.
+ * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
+ * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#define FTS_REG_CHIP_ID_H		0xA3
+#define FTS_REG_CHIP_ID_L		0x9F
+
+#define FTS_MAX_POINTS_SUPPORT		10
+#define FTS_ONE_TOUCH_LEN		6
+
+#define FTS_TOUCH_X_H_OFFSET		3
+#define FTS_TOUCH_X_L_OFFSET		4
+#define FTS_TOUCH_Y_H_OFFSET		5
+#define FTS_TOUCH_Y_L_OFFSET		6
+#define FTS_TOUCH_PRESSURE_OFFSET	7
+#define FTS_TOUCH_AREA_OFFSET		8
+#define FTS_TOUCH_TYPE_OFFSET		3
+#define FTS_TOUCH_ID_OFFSET		5
+
+#define FTS_TOUCH_DOWN			0
+#define FTS_TOUCH_UP			1
+#define FTS_TOUCH_CONTACT		2
+
+#define FTS_INTERVAL_READ_REG_MS	100
+#define FTS_TIMEOUT_READ_REG_MS		2000
+
+#define FTS_DRIVER_NAME			"fts-i2c"
+
+static const u16 fts_chip_types[] = {
+	0x5452,
+	0x8719,
+};
+
+struct fts_ts_data {
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	struct regmap *regmap;
+	int irq;
+	struct regulator_bulk_data regulators[2];
+	u8 max_touch_points;
+	u8 *point_buf;
+	int point_buf_size;
+	struct gpio_desc *reset_gpio;
+	struct touchscreen_properties prop;
+};
+
+struct fts_i2c_chip_data {
+	int max_touch_points;
+};
+
+int fts_check_status(struct fts_ts_data *data)
+{
+	int error, i = 0, count = 0;
+	unsigned int val, id;
+
+	do {
+		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
+		if (error)
+			dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+
+		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
+		if (error)
+			dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+
+		id |= val << 8;
+
+		for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
+			if (id == fts_chip_types[i])
+				return 0;
+
+		count++;
+		msleep(FTS_INTERVAL_READ_REG_MS);
+	} while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
+
+	return -EIO;
+}
+
+static int fts_report_touch(struct fts_ts_data *data)
+{
+	struct input_dev *input_dev = data->input_dev;
+	int base;
+	unsigned int x, y, z, maj;
+	u8 slot, type;
+	int error, i = 0;
+
+	u8 *buf = data->point_buf;
+
+	memset(buf, 0, data->point_buf_size);
+
+	error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
+	if (error) {
+		dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+		return error;
+	}
+
+	for (i = 0; i < data->max_touch_points; i++) {
+		base = FTS_ONE_TOUCH_LEN * i;
+
+		slot = buf[base + FTS_TOUCH_ID_OFFSET] >> 4;
+		if (slot >= data->max_touch_points)
+			break;
+
+		x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
+		    (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
+		y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
+		    (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
+
+		z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
+		if (z == 0)
+			z = 0x3f;
+
+		maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
+		if (maj == 0)
+			maj = 0x09;
+
+		type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
+
+		input_mt_slot(input_dev, slot);
+		if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
+			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
+			touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
+			input_report_abs(input_dev, ABS_MT_PRESSURE, z);
+			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
+			input_report_key(data->input_dev, BTN_TOUCH, 1);
+		} else {
+			input_report_key(data->input_dev, BTN_TOUCH, 0);
+			input_mt_report_slot_inactive(input_dev);
+		}
+	}
+	input_mt_sync_frame(input_dev);
+	input_sync(input_dev);
+
+	return 0;
+}
+
+static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
+{
+	struct fts_ts_data *data = dev_id;
+
+	return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
+}
+
+static void fts_power_off(void *d)
+{
+	struct fts_ts_data *data = d;
+
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
+static int fts_start(struct fts_ts_data *data)
+{
+	int error;
+
+	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				      data->regulators);
+	if (error) {
+		dev_err(&data->client->dev, "failed to enable regulators\n");
+		return error;
+	}
+
+	gpiod_set_value_cansleep(data->reset_gpio, 0);
+	msleep(200);
+
+	enable_irq(data->irq);
+
+	return 0;
+}
+
+static int fts_stop(struct fts_ts_data *data)
+{
+	disable_irq(data->irq);
+	gpiod_set_value_cansleep(data->reset_gpio, 1);
+	fts_power_off(data);
+
+	return 0;
+}
+
+static int fts_input_open(struct input_dev *dev)
+{
+	struct fts_ts_data *data = input_get_drvdata(dev);
+	int error;
+
+	error = fts_start(data);
+	if (error)
+		return error;
+
+	error = fts_check_status(data);
+	if (error) {
+		dev_err(&data->client->dev, "Failed to start or unsupported chip");
+		return error;
+	}
+
+	return 0;
+}
+
+static void fts_input_close(struct input_dev *dev)
+{
+	struct fts_ts_data *data = input_get_drvdata(dev);
+
+	fts_stop(data);
+}
+
+static int fts_input_init(struct fts_ts_data *data)
+{
+	struct device *dev = &data->client->dev;
+	struct input_dev *input_dev;
+	int error = 0;
+
+	input_dev = devm_input_allocate_device(dev);
+	if (!input_dev)
+		return -ENOMEM;
+
+	data->input_dev = input_dev;
+
+	input_dev->name = FTS_DRIVER_NAME;
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = dev;
+	input_dev->open = fts_input_open;
+	input_dev->close = fts_input_close;
+	input_set_drvdata(input_dev, data);
+
+	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
+	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+
+	touchscreen_parse_properties(input_dev, true, &data->prop);
+	if (!data->prop.max_x || !data->prop.max_y) {
+		dev_err(dev,
+			"touchscreen-size-x and/or touchscreen-size-y not set in device properties\n");
+		return -EINVAL;
+	}
+
+	error = input_mt_init_slots(input_dev, data->max_touch_points,
+				    INPUT_MT_DIRECT);
+	if (error)
+		return error;
+
+	data->point_buf_size = (data->max_touch_points * FTS_ONE_TOUCH_LEN) + 3;
+	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
+	if (!data->point_buf)
+		return -ENOMEM;
+
+	error = input_register_device(input_dev);
+	if (error) {
+		dev_err(dev, "Failed to register input device\n");
+		return error;
+	}
+
+	return 0;
+}
+
+static const struct regmap_config fts_ts_i2c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int fts_ts_probe(struct i2c_client *client)
+{
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+	const struct fts_i2c_chip_data *chip_data;
+	struct fts_ts_data *data;
+	int error = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "I2C not supported");
+		return -ENODEV;
+	}
+
+	if (!client->irq) {
+		dev_err(&client->dev, "No irq specified\n");
+		return -EINVAL;
+	}
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	chip_data = device_get_match_data(&client->dev);
+	if (!chip_data)
+		chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
+	if (!chip_data || !chip_data->max_touch_points) {
+		dev_err(&client->dev, "invalid or missing chip data\n");
+		return -EINVAL;
+	}
+	if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
+		dev_err(&client->dev,
+			"invalid chip data, max_touch_points should be less than or equal to %d\n",
+			FTS_MAX_POINTS_SUPPORT);
+		return -EINVAL;
+	}
+	data->max_touch_points = chip_data->max_touch_points;
+
+	data->client = client;
+	i2c_set_clientdata(client, data);
+
+	data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpio)) {
+		error = PTR_ERR(data->reset_gpio);
+		dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
+		return error;
+	}
+
+	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		error = PTR_ERR(data->regmap);
+		dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
+		return error;
+	}
+
+	/*
+	 * AVDD is the analog voltage supply (2.6V to 3.3V)
+	 * VDDIO is the digital voltage supply (1.8V)
+	 */
+	data->regulators[0].supply = "avdd";
+	data->regulators[1].supply = "vddio";
+	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
+					data->regulators);
+	if (error) {
+		dev_err(&client->dev, "Failed to get regulators %d\n", error);
+		return error;
+	}
+
+	error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
+	if (error) {
+		dev_err(&client->dev, "failed to install power off handler\n");
+		return error;
+	}
+
+	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					  fts_ts_interrupt, IRQF_ONESHOT,
+					  client->name, data);
+	if (error) {
+		dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
+		return error;
+	}
+
+	error = fts_input_init(data);
+	if (error)
+		return error;
+
+	return 0;
+}
+
+static int fts_pm_suspend(struct device *dev)
+{
+	struct fts_ts_data *data = dev_get_drvdata(dev);
+
+	mutex_lock(&data->input_dev->mutex);
+
+	if (input_device_enabled(data->input_dev))
+		fts_stop(data);
+
+	mutex_unlock(&data->input_dev->mutex);
+
+	return 0;
+}
+
+static int fts_pm_resume(struct device *dev)
+{
+	struct fts_ts_data *data = dev_get_drvdata(dev);
+	int error = 0;
+
+	mutex_lock(&data->input_dev->mutex);
+
+	if (input_device_enabled(data->input_dev))
+		error = fts_start(data);
+
+	mutex_unlock(&data->input_dev->mutex);
+
+	return error;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(fts_dev_pm_ops, fts_pm_suspend, fts_pm_resume);
+
+static const struct fts_i2c_chip_data fts5452_chip_data = {
+	.max_touch_points = 5,
+};
+
+static const struct fts_i2c_chip_data fts8719_chip_data = {
+	.max_touch_points = 10,
+};
+
+static const struct i2c_device_id fts_i2c_id[] = {
+	{ .name = "fts5452", .driver_data = (long)&fts5452_chip_data },
+	{ .name = "fts8719", .driver_data = (long)&fts8719_chip_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, fts_i2c_id);
+
+static const struct of_device_id fts_of_match[] = {
+	{ .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
+	{ .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, fts_of_match);
+
+static struct i2c_driver fts_ts_driver = {
+	.probe_new = fts_ts_probe,
+	.id_table = fts_i2c_id,
+	.driver = {
+		.name = FTS_DRIVER_NAME,
+		.pm = pm_sleep_ptr(&fts_dev_pm_ops),
+		.of_match_table = fts_of_match,
+	},
+};
+module_i2c_driver(fts_ts_driver);
+
+MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
+MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
+MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
+MODULE_LICENSE("GPL");
-- 
2.40.0

[PATCH v3 3/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: add touchscreen related nodes

[Joel Selvaraj]

Enable qupv3_id_1 and gpi_dma1 as they are required for configuring
touchscreen. Also add pinctrl configurations needed for touchscreen.
These are common for both the tianma and ebbg touchscreen variant.
In the subsequent patch, we will initially enable support for the focaltech
touchscreen used in the EBBG variant. This is done in preparation for that.

Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
 .../qcom/sdm845-xiaomi-beryllium-common.dtsi  | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
index 5ed975cc6ecb..b34ba46080ce 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
@@ -268,6 +268,10 @@ &gmu {
 	status = "okay";
 };
 
+&gpi_dma1 {
+	status = "okay";
+};
+
 &gpu {
 	status = "okay";
 
@@ -376,6 +380,10 @@ &qupv3_id_0 {
 	status = "okay";
 };
 
+&qupv3_id_1 {
+	status = "okay";
+};
+
 &sdhc_2 {
 	status = "okay";
 
@@ -481,6 +489,35 @@ sdc2_card_det_n: sd-card-det-n-state {
 		function = "gpio";
 		bias-pull-up;
 	};
+
+	ts_int_default: ts-int-default-state {
+		pins = "gpio31";
+		function = "gpio";
+		drive-strength = <16>;
+		bias-pull-down;
+	};
+
+	ts_reset_default: ts-reset-default-state {
+		pins = "gpio32";
+		function = "gpio";
+		drive-strength = <16>;
+		output-high;
+	};
+
+	ts_int_sleep: ts-int-sleep-state {
+		pins = "gpio31";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	ts_reset_sleep: ts-reset-sleep-state {
+		pins = "gpio32";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+		output-low;
+	};
 };
 
 &uart6 {
-- 
2.40.0

[PATCH v3 4/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for fts touchscreen

[Joel Selvaraj]

The Poco F1 EBBG variant uses Focaltech FTS touchscreen. Introduce
support for it.

Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
 .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
index 76931ebad065..26e77979cdab 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
@@ -13,3 +13,24 @@ &display_panel {
 	compatible = "ebbg,ft8719";
 	status = "okay";
 };
+
+&i2c14 {
+	status = "okay";
+
+	touchscreen@38 {
+		compatible = "focaltech,fts8719";
+		reg = <0x38>;
+
+		interrupts-extended = <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
+
+		vddio-supply = <&vreg_l14a_1p8>;
+
+		pinctrl-0 = <&ts_int_default &ts_reset_default>;
+		pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
+		pinctrl-names = "default", "sleep";
+
+		touchscreen-size-x = <1080>;
+		touchscreen-size-y = <2246>;
+	};
+};
-- 
2.40.0

[PATCH v3 5/5] arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen properties

[Joel Selvaraj]

The touchscreen nodes were added before the driver patches were merged.
Update the focaltech touchscreen properties to match with the upstreamed
focaltech driver. Also, the touchscreen used is in axolotl is fts5452
and not fts8719.

Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
 .../boot/dts/qcom/sdm845-shift-axolotl.dts     | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
index b54e304abf71..70286e53e000 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
@@ -474,23 +474,21 @@ &i2c5 {
 	status = "okay";
 
 	touchscreen@38 {
-		compatible = "focaltech,fts8719";
+		compatible = "focaltech,fts5452";
 		reg = <0x38>;
-		wakeup-source;
-		interrupt-parent = <&tlmm>;
-		interrupts = <125 0x2>;
-		vdd-supply = <&vreg_l28a_3p0>;
-		vcc-i2c-supply = <&vreg_l14a_1p88>;
 
-		pinctrl-names = "default", "suspend";
+		interrupts-extended = <&tlmm 125 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+		avdd-supply = <&vreg_l28a_3p0>;
+		vddio-supply = <&vreg_l14a_1p88>;
+
 		pinctrl-0 = <&ts_int_active &ts_reset_active>;
 		pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
+		pinctrl-names = "default", "suspend";
 
-		reset-gpio = <&tlmm 99 GPIO_ACTIVE_HIGH>;
-		irq-gpio = <&tlmm 125 GPIO_TRANSITORY>;
 		touchscreen-size-x = <1080>;
 		touchscreen-size-y = <2160>;
-		focaltech,max-touch-number = <5>;
 	};
 };
 
-- 
2.40.0

Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Christophe JAILLET]

Le 15/04/2023 à 04:02, Joel Selvaraj a écrit :
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
> 
> Co-developed-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> Signed-off-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   MAINTAINERS                                   |   8 +
>   drivers/input/touchscreen/Kconfig             |  12 +
>   drivers/input/touchscreen/Makefile            |   1 +
>   drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
>   4 files changed, 453 insertions(+)
>   create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7ec4ce64f66d..1a3ea61e1f52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8028,6 +8028,14 @@ L:	linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>   S:	Maintained
>   F:	drivers/input/joystick/fsia6b.c
>   
> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> +M:	Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> +M:	Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> +L:	linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
> +F:	drivers/input/touchscreen/focaltech_fts5452.c
> +
>   FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>   M:	Geoffrey D. Bennett <g@b4.vu>
>   L:	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1feecd7ed3cb..11af91504969 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called exc3000.
>   
> +config TOUCHSCREEN_FOCALTECH_FTS5452
> +	tristate "Focaltech FTS Touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here to enable support for I2C connected Focaltech FTS
> +	  based touch panels, including the 5452 and 8917 panels.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called focaltech_fts.
focaltech_fts5452?
Or should modifications be done elsewhere so that is does not look too 
5452 specific?

> +
>   config TOUCHSCREEN_FUJITSU
>   	tristate "Fujitsu serial touchscreen"
>   	select SERIO

[...]

> +struct fts_i2c_chip_data {
> +	int max_touch_points;
> +};
> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> +	int error, i = 0, count = 0;

No need to init "i".

> +	unsigned int val, id;
> +
> +	do {
> +		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
> +		if (error)
> +			dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> +
> +		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> +		if (error)
> +			dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> +
> +		id |= val << 8;
> +
> +		for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
> +			if (id == fts_chip_types[i])
> +				return 0;
> +
> +		count++;
> +		msleep(FTS_INTERVAL_READ_REG_MS);
> +	} while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
> +
> +	return -EIO;
> +}
> +
> +static int fts_report_touch(struct fts_ts_data *data)
> +{
> +	struct input_dev *input_dev = data->input_dev;
> +	int base;
> +	unsigned int x, y, z, maj;
> +	u8 slot, type;
> +	int error, i = 0;

No need to init "i".

> +
> +	u8 *buf = data->point_buf;
> +
> +	memset(buf, 0, data->point_buf_size);
> +
> +	error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> +	if (error) {
> +		dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> +		return error;
> +	}
> +
> +	for (i = 0; i < data->max_touch_points; i++) {
> +		base = FTS_ONE_TOUCH_LEN * i;

[...]

> +static int fts_ts_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const struct fts_i2c_chip_data *chip_data;
> +	struct fts_ts_data *data;
> +	int error = 0;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "I2C not supported");
> +		return -ENODEV;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev, "No irq specified\n");
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	chip_data = device_get_match_data(&client->dev);
> +	if (!chip_data)
> +		chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
> +	if (!chip_data || !chip_data->max_touch_points) {
> +		dev_err(&client->dev, "invalid or missing chip data\n");
> +		return -EINVAL;
> +	}
> +	if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
> +		dev_err(&client->dev,
> +			"invalid chip data, max_touch_points should be less than or equal to %d\n",
> +			FTS_MAX_POINTS_SUPPORT);
> +		return -EINVAL;
> +	}
> +	data->max_touch_points = chip_data->max_touch_points;
> +
> +	data->client = client;
> +	i2c_set_clientdata(client, data);
> +
> +	data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
> +		return error;
> +	}
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		error = PTR_ERR(data->regmap);
> +		dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
> +		return error;
> +	}
> +
> +	/*
> +	 * AVDD is the analog voltage supply (2.6V to 3.3V)
> +	 * VDDIO is the digital voltage supply (1.8V)
> +	 */
> +	data->regulators[0].supply = "avdd";
> +	data->regulators[1].supply = "vddio";
> +	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> +					data->regulators);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to get regulators %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> +	if (error) {
> +		dev_err(&client->dev, "failed to install power off handler\n");
> +		return error;
> +	}

Is it really needed?
This could lead to disable something that is not enabled. This looks 
harmless, but I wonder if it can occur?

I don't know the pm and input_dev frameworks enough to figure it myself, 
so this question is just about curiousity.

CJ

> +
> +	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					  fts_ts_interrupt, IRQF_ONESHOT,
> +					  client->name, data);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> +		return error;
> +	}
> +
> +	error = fts_input_init(data);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
[...]

Re: [PATCH v3 3/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: add touchscreen related nodes

[Konrad Dybcio]

On 15.04.2023 04:02, Joel Selvaraj wrote:
> Enable qupv3_id_1 and gpi_dma1 as they are required for configuring
> touchscreen. Also add pinctrl configurations needed for touchscreen.
> These are common for both the tianma and ebbg touchscreen variant.
> In the subsequent patch, we will initially enable support for the focaltech
> touchscreen used in the EBBG variant. This is done in preparation for that.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
Bit weird to add everything except the touchscreen, but okay..

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  .../qcom/sdm845-xiaomi-beryllium-common.dtsi  | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> index 5ed975cc6ecb..b34ba46080ce 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> @@ -268,6 +268,10 @@ &gmu {
>  	status = "okay";
>  };
>  
> +&gpi_dma1 {
> +	status = "okay";
> +};
> +
>  &gpu {
>  	status = "okay";
>  
> @@ -376,6 +380,10 @@ &qupv3_id_0 {
>  	status = "okay";
>  };
>  
> +&qupv3_id_1 {
> +	status = "okay";
> +};
> +
>  &sdhc_2 {
>  	status = "okay";
>  
> @@ -481,6 +489,35 @@ sdc2_card_det_n: sd-card-det-n-state {
>  		function = "gpio";
>  		bias-pull-up;
>  	};
> +
> +	ts_int_default: ts-int-default-state {
> +		pins = "gpio31";
> +		function = "gpio";
> +		drive-strength = <16>;
> +		bias-pull-down;
> +	};
> +
> +	ts_reset_default: ts-reset-default-state {
> +		pins = "gpio32";
> +		function = "gpio";
> +		drive-strength = <16>;
> +		output-high;
> +	};
> +
> +	ts_int_sleep: ts-int-sleep-state {
> +		pins = "gpio31";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	ts_reset_sleep: ts-reset-sleep-state {
> +		pins = "gpio32";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};
>  };
>  
>  &uart6 {

Re: [PATCH v3 4/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for fts touchscreen

[Konrad Dybcio]

On 15.04.2023 04:02, Joel Selvaraj wrote:
> The Poco F1 EBBG variant uses Focaltech FTS touchscreen. Introduce
> support for it.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> index 76931ebad065..26e77979cdab 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> @@ -13,3 +13,24 @@ &display_panel {
>  	compatible = "ebbg,ft8719";
>  	status = "okay";
>  };
> +
> +&i2c14 {
> +	status = "okay";
> +
> +	touchscreen@38 {
> +		compatible = "focaltech,fts8719";
> +		reg = <0x38>;
> +
> +		interrupts-extended = <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
> +		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
> +
> +		vddio-supply = <&vreg_l14a_1p8>;
> +
> +		pinctrl-0 = <&ts_int_default &ts_reset_default>;
> +		pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
> +		pinctrl-names = "default", "sleep";
> +
> +		touchscreen-size-x = <1080>;
> +		touchscreen-size-y = <2246>;
> +	};
> +};

Re: [PATCH v3 0/5] Add support for Focaltech FTS Touchscreen

[Krzysztof Kozlowski]

On 15/04/2023 04:02, Joel Selvaraj wrote:
> Changes in v3:(Suggested by Krzysztof Kozlowski and Konrad Dybcio)
> --------------
> - dts: removed the invalid "input-enable" property
> - dts: replace interrupts with interrupts-extended
> - dts: removed redundant dma configuration
> - dts: reorder pinctrl and pinctrl-names
> - bindings: moved unevaluatedProperties after required
> - bindings: make interrupts a required property (new change from my end)
> - bindings: update example based on dts changes
> 
> I have made the interrupts a required property in the bindings as the driver
> will not function without an interrupt. Because of this new change, I have not

Driver does not really matter for the bindings. The BSD driver for
example might function without an interrupt, so why requiring it? The
reason for requiring or not is in the hardware and how it works.

Best regards,
Krzysztof

Re: [PATCH v3 1/5] dt-bindings: input: touchscreen: add focaltech,fts5452 touchscreen

[Krzysztof Kozlowski]

On 15/04/2023 04:02, Joel Selvaraj wrote:
> Document the Focaltech FTS touchscreen driver.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../input/touchscreen/focaltech,fts5452.yaml  | 71 +++++++++++++++++++


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 5/5] arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen properties

[Caleb Connolly]

On 15/04/2023 03:02, Joel Selvaraj wrote:
> The touchscreen nodes were added before the driver patches were merged.
> Update the focaltech touchscreen properties to match with the upstreamed
> focaltech driver. Also, the touchscreen used is in axolotl is fts5452
> and not fts8719.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>

Reviewed-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../boot/dts/qcom/sdm845-shift-axolotl.dts     | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> index b54e304abf71..70286e53e000 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> @@ -474,23 +474,21 @@ &i2c5 {
>  	status = "okay";
>
>  	touchscreen@38 {
> -		compatible = "focaltech,fts8719";
> +		compatible = "focaltech,fts5452";
>  		reg = <0x38>;
> -		wakeup-source;
> -		interrupt-parent = <&tlmm>;
> -		interrupts = <125 0x2>;
> -		vdd-supply = <&vreg_l28a_3p0>;
> -		vcc-i2c-supply = <&vreg_l14a_1p88>;
>
> -		pinctrl-names = "default", "suspend";
> +		interrupts-extended = <&tlmm 125 IRQ_TYPE_EDGE_FALLING>;
> +		reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> +
> +		avdd-supply = <&vreg_l28a_3p0>;
> +		vddio-supply = <&vreg_l14a_1p88>;
> +
>  		pinctrl-0 = <&ts_int_active &ts_reset_active>;
>  		pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
> +		pinctrl-names = "default", "suspend";
>
> -		reset-gpio = <&tlmm 99 GPIO_ACTIVE_HIGH>;
> -		irq-gpio = <&tlmm 125 GPIO_TRANSITORY>;
>  		touchscreen-size-x = <1080>;
>  		touchscreen-size-y = <2160>;
> -		focaltech,max-touch-number = <5>;
>  	};
>  };
>
> --
> 2.40.0
>

--
Kind Regards,
Caleb

Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Jeff LaBundy]

Hi Joel,

Great work so far! It's coming along nicely. Please find my latest
feedback below.

On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
> 
> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
>  MAINTAINERS                                   |   8 +
>  drivers/input/touchscreen/Kconfig             |  12 +
>  drivers/input/touchscreen/Makefile            |   1 +
>  drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
>  4 files changed, 453 insertions(+)
>  create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7ec4ce64f66d..1a3ea61e1f52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8028,6 +8028,14 @@ L:	linux-input@vger.kernel.org
>  S:	Maintained
>  F:	drivers/input/joystick/fsia6b.c
>  
> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +M:	Caleb Connolly <caleb@connolly.tech>
> +L:	linux-input@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
> +F:	drivers/input/touchscreen/focaltech_fts5452.c
> +
>  FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>  M:	Geoffrey D. Bennett <g@b4.vu>
>  L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1feecd7ed3cb..11af91504969 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called exc3000.
>  
> +config TOUCHSCREEN_FOCALTECH_FTS5452
> +	tristate "Focaltech FTS Touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here to enable support for I2C connected Focaltech FTS
> +	  based touch panels, including the 5452 and 8917 panels.

This language is a bit misleading, as it seems to suggest three or more
models are supported. It seems the title should simply be "FocalTech
FTS5452 touchscreen controller" with the description as "...FocalTech
FTS5452 and compatible touchscreen controllers."

As more are found to be compatible (e.g. FTS8917), the compatible strings
can simply be appended.

> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called focaltech_fts.
> +
>  config TOUCHSCREEN_FUJITSU
>  	tristate "Fujitsu serial touchscreen"
>  	select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 159cd5136fdb..47d78c9cff21 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
>  obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
>  obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452)	+= focaltech_fts5452.o
>  obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
> diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
> new file mode 100644
> index 000000000000..abf8a2f271ca
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts5452.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * FocalTech touchscreen driver.
> + *
> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> + * Copyright (C) 2018 XiaoMi, Inc.
> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> + */

Nit: inconsistent copyright capitalization.

> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#define FTS_REG_CHIP_ID_H		0xA3
> +#define FTS_REG_CHIP_ID_L		0x9F
> +
> +#define FTS_MAX_POINTS_SUPPORT		10
> +#define FTS_ONE_TOUCH_LEN		6
> +
> +#define FTS_TOUCH_X_H_OFFSET		3
> +#define FTS_TOUCH_X_L_OFFSET		4
> +#define FTS_TOUCH_Y_H_OFFSET		5
> +#define FTS_TOUCH_Y_L_OFFSET		6
> +#define FTS_TOUCH_PRESSURE_OFFSET	7
> +#define FTS_TOUCH_AREA_OFFSET		8
> +#define FTS_TOUCH_TYPE_OFFSET		3
> +#define FTS_TOUCH_ID_OFFSET		5
> +
> +#define FTS_TOUCH_DOWN			0
> +#define FTS_TOUCH_UP			1
> +#define FTS_TOUCH_CONTACT		2
> +
> +#define FTS_INTERVAL_READ_REG_MS	100
> +#define FTS_TIMEOUT_READ_REG_MS		2000
> +
> +#define FTS_DRIVER_NAME			"fts-i2c"
> +
> +static const u16 fts_chip_types[] = {
> +	0x5452,
> +	0x8719,
> +};
> +
> +struct fts_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct regmap *regmap;
> +	int irq;
> +	struct regulator_bulk_data regulators[2];
> +	u8 max_touch_points;
> +	u8 *point_buf;
> +	int point_buf_size;
> +	struct gpio_desc *reset_gpio;
> +	struct touchscreen_properties prop;
> +};
> +
> +struct fts_i2c_chip_data {
> +	int max_touch_points;
> +};

There is no reason to wrap a single member in a struct; just define an array
and point each driver_data member to the appropriate element.

An even better solution, however, would be to merge the device ID into this.
Then you would have a single array of structs with very clear association
between device ID and number of points.

> +
> +int fts_check_status(struct fts_ts_data *data)

This function can be static. It also seems to be inappropriately named. Here
we are checking the device's ID, not its status.

> +{
> +	int error, i = 0, count = 0;
> +	unsigned int val, id;
> +
> +	do {
> +		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
> +		if (error)
> +			dev_err(&data->client->dev, "I2C read failed: %d\n", error);

If this read fails, there is no point in continuing further in this loop. Most
likely the second read would fail as well but if it doesn't, you are computing
the id using an uninitialized variable.

Can you also explain, and possibly add comments, as to why the device ID must
be checked in a retry loop? Is it because the device may be in a deep sleep and
must be hit with I2C traffic a couple of times?

If so, then you likely want to briefly sleep and then start over (i.e. continue)
in the event of an error.

> +
> +		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> +		if (error)
> +			dev_err(&data->client->dev, "I2C read failed: %d\n", error);

Same problem here.

> +
> +		id |= val << 8;
> +
> +		for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
> +			if (id == fts_chip_types[i])
> +				return 0;

This retry loop in general seems a bit non-optimal. If for example the driver
is simply communicating with an incompatible device, there is no need to go
through all N loops.

> +
> +		count++;
> +		msleep(FTS_INTERVAL_READ_REG_MS);
> +	} while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);

This multiplication seems unnecessarily complicated; can we not simply have
FTS_MAX_RETRIES or similar?

> +
> +	return -EIO;
> +}
> +
> +static int fts_report_touch(struct fts_ts_data *data)
> +{
> +	struct input_dev *input_dev = data->input_dev;
> +	int base;
> +	unsigned int x, y, z, maj;
> +	u8 slot, type;
> +	int error, i = 0;
> +
> +	u8 *buf = data->point_buf;
> +
> +	memset(buf, 0, data->point_buf_size);
> +
> +	error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> +	if (error) {
> +		dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> +		return error;
> +	}
> +
> +	for (i = 0; i < data->max_touch_points; i++) {
> +		base = FTS_ONE_TOUCH_LEN * i;
> +
> +		slot = buf[base + FTS_TOUCH_ID_OFFSET] >> 4;
> +		if (slot >= data->max_touch_points)
> +			break;
> +
> +		x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
> +		    (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
> +		y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
> +		    (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);

Sorry, I did not quite follow the image that was shared in an earlier thread.
It is unclear to me why we cannot represent the interrupt status registers
as an array of __be16 values and then do something like the following:

		x = be16_to_cpu(buf[FTS_TOUCH_X_OFFSET]) & GENMASK(11, 0);

I would be surprised if the mask is even necessary; you would need to refer
to a datasheet however. Perhaps the vendor would be willing to give one to
you if it means they get an upstream driver?

> +
> +		z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> +		if (z == 0)
> +			z = 0x3f;
> +
> +		maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
> +		if (maj == 0)
> +			maj = 0x09;

I think we need some comments and possibly some #defines to explain what is
happening here.

> +
> +		type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
> +
> +		input_mt_slot(input_dev, slot);
> +		if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
> +			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
> +			touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, z);
> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
> +			input_report_key(data->input_dev, BTN_TOUCH, 1);
> +		} else {
> +			input_report_key(data->input_dev, BTN_TOUCH, 0);
> +			input_mt_report_slot_inactive(input_dev);
> +		}
> +	}
> +	input_mt_sync_frame(input_dev);
> +	input_sync(input_dev);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct fts_ts_data *data = dev_id;
> +
> +	return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
> +}
> +
> +static void fts_power_off(void *d)
> +{
> +	struct fts_ts_data *data = d;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> +}
> +
> +static int fts_start(struct fts_ts_data *data)
> +{
> +	int error;
> +
> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (error) {
> +		dev_err(&data->client->dev, "failed to enable regulators\n");
> +		return error;
> +	}
> +
> +	gpiod_set_value_cansleep(data->reset_gpio, 0);
> +	msleep(200);

Same here with respect to comments; what happens during these first 200 ms after
reset is released? Does the interrupt pin toggle several times? 200 ms is also
quite a while to wait each time the input handler opens the device; is it really
necessary?

> +
> +	enable_irq(data->irq);
> +
> +	return 0;
> +}
> +
> +static int fts_stop(struct fts_ts_data *data)
> +{
> +	disable_irq(data->irq);
> +	gpiod_set_value_cansleep(data->reset_gpio, 1);
> +	fts_power_off(data);
> +
> +	return 0;
> +}
> +
> +static int fts_input_open(struct input_dev *dev)
> +{
> +	struct fts_ts_data *data = input_get_drvdata(dev);
> +	int error;
> +
> +	error = fts_start(data);
> +	if (error)
> +		return error;
> +
> +	error = fts_check_status(data);
> +	if (error) {
> +		dev_err(&data->client->dev, "Failed to start or unsupported chip");
> +		return error;
> +	}

It seems unnecessary and wasteful to check the device ID every time the input
handler opens the device. We also don't want to go through all the trouble of
registering the device, only to find out later it wasn't even the right part.

Instead, you should power up the device during probe, validate its ID and then
power it back down.

> +
> +	return 0;
> +}
> +
> +static void fts_input_close(struct input_dev *dev)
> +{
> +	struct fts_ts_data *data = input_get_drvdata(dev);
> +
> +	fts_stop(data);
> +}
> +
> +static int fts_input_init(struct fts_ts_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct input_dev *input_dev;
> +	int error = 0;

No need to initialize this, only for it to get overwritten later.

> +
> +	input_dev = devm_input_allocate_device(dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	data->input_dev = input_dev;
> +
> +	input_dev->name = FTS_DRIVER_NAME;
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = dev;
> +	input_dev->open = fts_input_open;
> +	input_dev->close = fts_input_close;
> +	input_set_drvdata(input_dev, data);
> +
> +	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
> +	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
> +	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> +	touchscreen_parse_properties(input_dev, true, &data->prop);
> +	if (!data->prop.max_x || !data->prop.max_y) {
> +		dev_err(dev,
> +			"touchscreen-size-x and/or touchscreen-size-y not set in device properties\n");

"Device properties" is vague; one could interpret it to mean the controller's
embedded FW. Just cut the message off at "...not set".

> +		return -EINVAL;
> +	}
> +
> +	error = input_mt_init_slots(input_dev, data->max_touch_points,
> +				    INPUT_MT_DIRECT);
> +	if (error)
> +		return error;
> +
> +	data->point_buf_size = (data->max_touch_points * FTS_ONE_TOUCH_LEN) + 3;
> +	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> +	if (!data->point_buf)
> +		return -ENOMEM;
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const struct fts_i2c_chip_data *chip_data;
> +	struct fts_ts_data *data;
> +	int error = 0;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "I2C not supported");
> +		return -ENODEV;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev, "No irq specified\n");
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	chip_data = device_get_match_data(&client->dev);
> +	if (!chip_data)
> +		chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
> +	if (!chip_data || !chip_data->max_touch_points) {
> +		dev_err(&client->dev, "invalid or missing chip data\n");
> +		return -EINVAL;
> +	}
> +	if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
> +		dev_err(&client->dev,
> +			"invalid chip data, max_touch_points should be less than or equal to %d\n",
> +			FTS_MAX_POINTS_SUPPORT);
> +		return -EINVAL;
> +	}

This check is not necessary; if someone adds an invalid max_touch_points, then the
driver was updated incorrectly. There is no need to check it at every runtime.

> +	data->max_touch_points = chip_data->max_touch_points;
> +
> +	data->client = client;
> +	i2c_set_clientdata(client, data);
> +
> +	data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
> +		return error;
> +	}
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		error = PTR_ERR(data->regmap);
> +		dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
> +		return error;
> +	}
> +
> +	/*
> +	 * AVDD is the analog voltage supply (2.6V to 3.3V)
> +	 * VDDIO is the digital voltage supply (1.8V)
> +	 */
> +	data->regulators[0].supply = "avdd";
> +	data->regulators[1].supply = "vddio";
> +	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> +					data->regulators);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to get regulators %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> +	if (error) {
> +		dev_err(&client->dev, "failed to install power off handler\n");
> +		return error;
> +	}

Christophe makes a great point. If this or any other call throughout the rest of
probe as you have written it fails, you will try to disable a disabled regulator.

The same will happen when the driver is torn down, as the input handler should
have already powered down the device by way of the close callback. Did you build
this driver as a module and test removal? I suspect you will get a stack trace.

I think the call needs to go away altogether.

> +
> +	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					  fts_ts_interrupt, IRQF_ONESHOT,
> +					  client->name, data);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> +		return error;
> +	}
> +
> +	error = fts_input_init(data);
> +	if (error)
> +		return error;
> +
> +	return 0;

This is idiomatic, but I find "return fts_input_init(data);" to be simpler.

> +}
> +
> +static int fts_pm_suspend(struct device *dev)
> +{
> +	struct fts_ts_data *data = dev_get_drvdata(dev);
> +
> +	mutex_lock(&data->input_dev->mutex);
> +
> +	if (input_device_enabled(data->input_dev))
> +		fts_stop(data);
> +
> +	mutex_unlock(&data->input_dev->mutex);
> +
> +	return 0;
> +}
> +
> +static int fts_pm_resume(struct device *dev)
> +{
> +	struct fts_ts_data *data = dev_get_drvdata(dev);
> +	int error = 0;

Same here, there is no point in initializating this.

> +
> +	mutex_lock(&data->input_dev->mutex);
> +
> +	if (input_device_enabled(data->input_dev))
> +		error = fts_start(data);
> +
> +	mutex_unlock(&data->input_dev->mutex);
> +
> +	return error;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(fts_dev_pm_ops, fts_pm_suspend, fts_pm_resume);
> +
> +static const struct fts_i2c_chip_data fts5452_chip_data = {
> +	.max_touch_points = 5,
> +};
> +
> +static const struct fts_i2c_chip_data fts8719_chip_data = {
> +	.max_touch_points = 10,
> +};
> +
> +static const struct i2c_device_id fts_i2c_id[] = {
> +	{ .name = "fts5452", .driver_data = (long)&fts5452_chip_data },
> +	{ .name = "fts8719", .driver_data = (long)&fts8719_chip_data },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, fts_i2c_id);
> +
> +static const struct of_device_id fts_of_match[] = {
> +	{ .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
> +	{ .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_of_match);
> +
> +static struct i2c_driver fts_ts_driver = {
> +	.probe_new = fts_ts_probe,
> +	.id_table = fts_i2c_id,
> +	.driver = {
> +		.name = FTS_DRIVER_NAME,
> +		.pm = pm_sleep_ptr(&fts_dev_pm_ops),
> +		.of_match_table = fts_of_match,
> +	},
> +};
> +module_i2c_driver(fts_ts_driver);
> +
> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");

Nit: mixing 'FocalTech' and 'Focaltech' throughout.

> +MODULE_LICENSE("GPL");
> -- 
> 2.40.0
> 

Kind regards,
Jeff LaBundy

Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Hans de Goede]

Hi,

On 4/24/23 04:39, Jeff LaBundy wrote:
> Hi Joel,
> 
> Great work so far! It's coming along nicely. Please find my latest
> feedback below.
> 
> On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
>> The Focaltech FTS driver supports several variants of focaltech
>> touchscreens found in ~2018 era smartphones including variants found on
>> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
>> This driver is loosely based on the original driver from Focaltech
>> but has been simplified and largely reworked.
>>
>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>

Sorry for jumping into this thread a bit late.

I've been reading the archived discussion, but AFAICT the following question is not answered there:

Why do a new driver at all ? I have a couple of devices (Nextbook Ares 8, Nextbook Ares 8A) with focaltech FT5416 touchscreens and they both work fine with the existing drivers/input/touchscreen/edt-ft5x06.c driver.

Is there any reason we need a whole new driver for the ft5452 instead of
using (with maybe some tweaks?) the existing edt-ft5x06 driver ?

Note that despite the name the edt-ft5x06 is a generic Focaltect touchscreen driver.

Regards,

Hans



>> ---
>>  MAINTAINERS                                   |   8 +
>>  drivers/input/touchscreen/Kconfig             |  12 +
>>  drivers/input/touchscreen/Makefile            |   1 +
>>  drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
>>  4 files changed, 453 insertions(+)
>>  create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7ec4ce64f66d..1a3ea61e1f52 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8028,6 +8028,14 @@ L:	linux-input@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/input/joystick/fsia6b.c
>>  
>> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
>> +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> +M:	Caleb Connolly <caleb@connolly.tech>
>> +L:	linux-input@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
>> +F:	drivers/input/touchscreen/focaltech_fts5452.c
>> +
>>  FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>>  M:	Geoffrey D. Bennett <g@b4.vu>
>>  L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 1feecd7ed3cb..11af91504969 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called exc3000.
>>  
>> +config TOUCHSCREEN_FOCALTECH_FTS5452
>> +	tristate "Focaltech FTS Touchscreen"
>> +	depends on I2C
>> +	help
>> +	  Say Y here to enable support for I2C connected Focaltech FTS
>> +	  based touch panels, including the 5452 and 8917 panels.
> 
> This language is a bit misleading, as it seems to suggest three or more
> models are supported. It seems the title should simply be "FocalTech
> FTS5452 touchscreen controller" with the description as "...FocalTech
> FTS5452 and compatible touchscreen controllers."
> 
> As more are found to be compatible (e.g. FTS8917), the compatible strings
> can simply be appended.
> 
>> +
>> +	  If unsure, say N.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called focaltech_fts.
>> +
>>  config TOUCHSCREEN_FUJITSU
>>  	tristate "Fujitsu serial touchscreen"
>>  	select SERIO
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..47d78c9cff21 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
>>  obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
>>  obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
>>  obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
>> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452)	+= focaltech_fts5452.o
>>  obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>>  obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
>>  obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
>> diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
>> new file mode 100644
>> index 000000000000..abf8a2f271ca
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/focaltech_fts5452.c
>> @@ -0,0 +1,432 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * FocalTech touchscreen driver.
>> + *
>> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
>> + * Copyright (C) 2018 XiaoMi, Inc.
>> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
>> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> + */
> 
> Nit: inconsistent copyright capitalization.
> 
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#define FTS_REG_CHIP_ID_H		0xA3
>> +#define FTS_REG_CHIP_ID_L		0x9F
>> +
>> +#define FTS_MAX_POINTS_SUPPORT		10
>> +#define FTS_ONE_TOUCH_LEN		6
>> +
>> +#define FTS_TOUCH_X_H_OFFSET		3
>> +#define FTS_TOUCH_X_L_OFFSET		4
>> +#define FTS_TOUCH_Y_H_OFFSET		5
>> +#define FTS_TOUCH_Y_L_OFFSET		6
>> +#define FTS_TOUCH_PRESSURE_OFFSET	7
>> +#define FTS_TOUCH_AREA_OFFSET		8
>> +#define FTS_TOUCH_TYPE_OFFSET		3
>> +#define FTS_TOUCH_ID_OFFSET		5
>> +
>> +#define FTS_TOUCH_DOWN			0
>> +#define FTS_TOUCH_UP			1
>> +#define FTS_TOUCH_CONTACT		2
>> +
>> +#define FTS_INTERVAL_READ_REG_MS	100
>> +#define FTS_TIMEOUT_READ_REG_MS		2000
>> +
>> +#define FTS_DRIVER_NAME			"fts-i2c"
>> +
>> +static const u16 fts_chip_types[] = {
>> +	0x5452,
>> +	0x8719,
>> +};
>> +
>> +struct fts_ts_data {
>> +	struct i2c_client *client;
>> +	struct input_dev *input_dev;
>> +	struct regmap *regmap;
>> +	int irq;
>> +	struct regulator_bulk_data regulators[2];
>> +	u8 max_touch_points;
>> +	u8 *point_buf;
>> +	int point_buf_size;
>> +	struct gpio_desc *reset_gpio;
>> +	struct touchscreen_properties prop;
>> +};
>> +
>> +struct fts_i2c_chip_data {
>> +	int max_touch_points;
>> +};
> 
> There is no reason to wrap a single member in a struct; just define an array
> and point each driver_data member to the appropriate element.
> 
> An even better solution, however, would be to merge the device ID into this.
> Then you would have a single array of structs with very clear association
> between device ID and number of points.
> 
>> +
>> +int fts_check_status(struct fts_ts_data *data)
> 
> This function can be static. It also seems to be inappropriately named. Here
> we are checking the device's ID, not its status.
> 
>> +{
>> +	int error, i = 0, count = 0;
>> +	unsigned int val, id;
>> +
>> +	do {
>> +		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
>> +		if (error)
>> +			dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> 
> If this read fails, there is no point in continuing further in this loop. Most
> likely the second read would fail as well but if it doesn't, you are computing
> the id using an uninitialized variable.
> 
> Can you also explain, and possibly add comments, as to why the device ID must
> be checked in a retry loop? Is it because the device may be in a deep sleep and
> must be hit with I2C traffic a couple of times?
> 
> If so, then you likely want to briefly sleep and then start over (i.e. continue)
> in the event of an error.
> 
>> +
>> +		error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
>> +		if (error)
>> +			dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> 
> Same problem here.
> 
>> +
>> +		id |= val << 8;
>> +
>> +		for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
>> +			if (id == fts_chip_types[i])
>> +				return 0;
> 
> This retry loop in general seems a bit non-optimal. If for example the driver
> is simply communicating with an incompatible device, there is no need to go
> through all N loops.
> 
>> +
>> +		count++;
>> +		msleep(FTS_INTERVAL_READ_REG_MS);
>> +	} while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
> 
> This multiplication seems unnecessarily complicated; can we not simply have
> FTS_MAX_RETRIES or similar?
> 
>> +
>> +	return -EIO;
>> +}
>> +
>> +static int fts_report_touch(struct fts_ts_data *data)
>> +{
>> +	struct input_dev *input_dev = data->input_dev;
>> +	int base;
>> +	unsigned int x, y, z, maj;
>> +	u8 slot, type;
>> +	int error, i = 0;
>> +
>> +	u8 *buf = data->point_buf;
>> +
>> +	memset(buf, 0, data->point_buf_size);
>> +
>> +	error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
>> +	if (error) {
>> +		dev_err(&data->client->dev, "I2C read failed: %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	for (i = 0; i < data->max_touch_points; i++) {
>> +		base = FTS_ONE_TOUCH_LEN * i;
>> +
>> +		slot = buf[base + FTS_TOUCH_ID_OFFSET] >> 4;
>> +		if (slot >= data->max_touch_points)
>> +			break;
>> +
>> +		x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
>> +		    (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
>> +		y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
>> +		    (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
> 
> Sorry, I did not quite follow the image that was shared in an earlier thread.
> It is unclear to me why we cannot represent the interrupt status registers
> as an array of __be16 values and then do something like the following:
> 
> 		x = be16_to_cpu(buf[FTS_TOUCH_X_OFFSET]) & GENMASK(11, 0);
> 
> I would be surprised if the mask is even necessary; you would need to refer
> to a datasheet however. Perhaps the vendor would be willing to give one to
> you if it means they get an upstream driver?
> 
>> +
>> +		z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
>> +		if (z == 0)
>> +			z = 0x3f;
>> +
>> +		maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
>> +		if (maj == 0)
>> +			maj = 0x09;
> 
> I think we need some comments and possibly some #defines to explain what is
> happening here.
> 
>> +
>> +		type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
>> +
>> +		input_mt_slot(input_dev, slot);
>> +		if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
>> +			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
>> +			touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
>> +			input_report_abs(input_dev, ABS_MT_PRESSURE, z);
>> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
>> +			input_report_key(data->input_dev, BTN_TOUCH, 1);
>> +		} else {
>> +			input_report_key(data->input_dev, BTN_TOUCH, 0);
>> +			input_mt_report_slot_inactive(input_dev);
>> +		}
>> +	}
>> +	input_mt_sync_frame(input_dev);
>> +	input_sync(input_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
>> +{
>> +	struct fts_ts_data *data = dev_id;
>> +
>> +	return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
>> +}
>> +
>> +static void fts_power_off(void *d)
>> +{
>> +	struct fts_ts_data *data = d;
>> +
>> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>> +}
>> +
>> +static int fts_start(struct fts_ts_data *data)
>> +{
>> +	int error;
>> +
>> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
>> +				      data->regulators);
>> +	if (error) {
>> +		dev_err(&data->client->dev, "failed to enable regulators\n");
>> +		return error;
>> +	}
>> +
>> +	gpiod_set_value_cansleep(data->reset_gpio, 0);
>> +	msleep(200);
> 
> Same here with respect to comments; what happens during these first 200 ms after
> reset is released? Does the interrupt pin toggle several times? 200 ms is also
> quite a while to wait each time the input handler opens the device; is it really
> necessary?
> 
>> +
>> +	enable_irq(data->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int fts_stop(struct fts_ts_data *data)
>> +{
>> +	disable_irq(data->irq);
>> +	gpiod_set_value_cansleep(data->reset_gpio, 1);
>> +	fts_power_off(data);
>> +
>> +	return 0;
>> +}
>> +
>> +static int fts_input_open(struct input_dev *dev)
>> +{
>> +	struct fts_ts_data *data = input_get_drvdata(dev);
>> +	int error;
>> +
>> +	error = fts_start(data);
>> +	if (error)
>> +		return error;
>> +
>> +	error = fts_check_status(data);
>> +	if (error) {
>> +		dev_err(&data->client->dev, "Failed to start or unsupported chip");
>> +		return error;
>> +	}
> 
> It seems unnecessary and wasteful to check the device ID every time the input
> handler opens the device. We also don't want to go through all the trouble of
> registering the device, only to find out later it wasn't even the right part.
> 
> Instead, you should power up the device during probe, validate its ID and then
> power it back down.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static void fts_input_close(struct input_dev *dev)
>> +{
>> +	struct fts_ts_data *data = input_get_drvdata(dev);
>> +
>> +	fts_stop(data);
>> +}
>> +
>> +static int fts_input_init(struct fts_ts_data *data)
>> +{
>> +	struct device *dev = &data->client->dev;
>> +	struct input_dev *input_dev;
>> +	int error = 0;
> 
> No need to initialize this, only for it to get overwritten later.
> 
>> +
>> +	input_dev = devm_input_allocate_device(dev);
>> +	if (!input_dev)
>> +		return -ENOMEM;
>> +
>> +	data->input_dev = input_dev;
>> +
>> +	input_dev->name = FTS_DRIVER_NAME;
>> +	input_dev->id.bustype = BUS_I2C;
>> +	input_dev->dev.parent = dev;
>> +	input_dev->open = fts_input_open;
>> +	input_dev->close = fts_input_close;
>> +	input_set_drvdata(input_dev, data);
>> +
>> +	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
>> +	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
>> +	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
>> +
>> +	touchscreen_parse_properties(input_dev, true, &data->prop);
>> +	if (!data->prop.max_x || !data->prop.max_y) {
>> +		dev_err(dev,
>> +			"touchscreen-size-x and/or touchscreen-size-y not set in device properties\n");
> 
> "Device properties" is vague; one could interpret it to mean the controller's
> embedded FW. Just cut the message off at "...not set".
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	error = input_mt_init_slots(input_dev, data->max_touch_points,
>> +				    INPUT_MT_DIRECT);
>> +	if (error)
>> +		return error;
>> +
>> +	data->point_buf_size = (data->max_touch_points * FTS_ONE_TOUCH_LEN) + 3;
>> +	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
>> +	if (!data->point_buf)
>> +		return -ENOMEM;
>> +
>> +	error = input_register_device(input_dev);
>> +	if (error) {
>> +		dev_err(dev, "Failed to register input device\n");
>> +		return error;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct regmap_config fts_ts_i2c_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +static int fts_ts_probe(struct i2c_client *client)
>> +{
>> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>> +	const struct fts_i2c_chip_data *chip_data;
>> +	struct fts_ts_data *data;
>> +	int error = 0;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +		dev_err(&client->dev, "I2C not supported");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!client->irq) {
>> +		dev_err(&client->dev, "No irq specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	chip_data = device_get_match_data(&client->dev);
>> +	if (!chip_data)
>> +		chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
>> +	if (!chip_data || !chip_data->max_touch_points) {
>> +		dev_err(&client->dev, "invalid or missing chip data\n");
>> +		return -EINVAL;
>> +	}
>> +	if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
>> +		dev_err(&client->dev,
>> +			"invalid chip data, max_touch_points should be less than or equal to %d\n",
>> +			FTS_MAX_POINTS_SUPPORT);
>> +		return -EINVAL;
>> +	}
> 
> This check is not necessary; if someone adds an invalid max_touch_points, then the
> driver was updated incorrectly. There is no need to check it at every runtime.
> 
>> +	data->max_touch_points = chip_data->max_touch_points;
>> +
>> +	data->client = client;
>> +	i2c_set_clientdata(client, data);
>> +
>> +	data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(data->reset_gpio)) {
>> +		error = PTR_ERR(data->reset_gpio);
>> +		dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
>> +	if (IS_ERR(data->regmap)) {
>> +		error = PTR_ERR(data->regmap);
>> +		dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	/*
>> +	 * AVDD is the analog voltage supply (2.6V to 3.3V)
>> +	 * VDDIO is the digital voltage supply (1.8V)
>> +	 */
>> +	data->regulators[0].supply = "avdd";
>> +	data->regulators[1].supply = "vddio";
>> +	error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
>> +					data->regulators);
>> +	if (error) {
>> +		dev_err(&client->dev, "Failed to get regulators %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
>> +	if (error) {
>> +		dev_err(&client->dev, "failed to install power off handler\n");
>> +		return error;
>> +	}
> 
> Christophe makes a great point. If this or any other call throughout the rest of
> probe as you have written it fails, you will try to disable a disabled regulator.
> 
> The same will happen when the driver is torn down, as the input handler should
> have already powered down the device by way of the close callback. Did you build
> this driver as a module and test removal? I suspect you will get a stack trace.
> 
> I think the call needs to go away altogether.
> 
>> +
>> +	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +					  fts_ts_interrupt, IRQF_ONESHOT,
>> +					  client->name, data);
>> +	if (error) {
>> +		dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	error = fts_input_init(data);
>> +	if (error)
>> +		return error;
>> +
>> +	return 0;
> 
> This is idiomatic, but I find "return fts_input_init(data);" to be simpler.
> 
>> +}
>> +
>> +static int fts_pm_suspend(struct device *dev)
>> +{
>> +	struct fts_ts_data *data = dev_get_drvdata(dev);
>> +
>> +	mutex_lock(&data->input_dev->mutex);
>> +
>> +	if (input_device_enabled(data->input_dev))
>> +		fts_stop(data);
>> +
>> +	mutex_unlock(&data->input_dev->mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +static int fts_pm_resume(struct device *dev)
>> +{
>> +	struct fts_ts_data *data = dev_get_drvdata(dev);
>> +	int error = 0;
> 
> Same here, there is no point in initializating this.
> 
>> +
>> +	mutex_lock(&data->input_dev->mutex);
>> +
>> +	if (input_device_enabled(data->input_dev))
>> +		error = fts_start(data);
>> +
>> +	mutex_unlock(&data->input_dev->mutex);
>> +
>> +	return error;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(fts_dev_pm_ops, fts_pm_suspend, fts_pm_resume);
>> +
>> +static const struct fts_i2c_chip_data fts5452_chip_data = {
>> +	.max_touch_points = 5,
>> +};
>> +
>> +static const struct fts_i2c_chip_data fts8719_chip_data = {
>> +	.max_touch_points = 10,
>> +};
>> +
>> +static const struct i2c_device_id fts_i2c_id[] = {
>> +	{ .name = "fts5452", .driver_data = (long)&fts5452_chip_data },
>> +	{ .name = "fts8719", .driver_data = (long)&fts8719_chip_data },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, fts_i2c_id);
>> +
>> +static const struct of_device_id fts_of_match[] = {
>> +	{ .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
>> +	{ .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
>> +	{ /* sentinel */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, fts_of_match);
>> +
>> +static struct i2c_driver fts_ts_driver = {
>> +	.probe_new = fts_ts_probe,
>> +	.id_table = fts_i2c_id,
>> +	.driver = {
>> +		.name = FTS_DRIVER_NAME,
>> +		.pm = pm_sleep_ptr(&fts_dev_pm_ops),
>> +		.of_match_table = fts_of_match,
>> +	},
>> +};
>> +module_i2c_driver(fts_ts_driver);
>> +
>> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
>> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
>> +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
> 
> Nit: mixing 'FocalTech' and 'Focaltech' throughout.
> 
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.40.0
>>
> 
> Kind regards,
> Jeff LaBundy
> 

Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Joel Selvaraj]

Hi Jeff LaBundy,

On 4/23/23 21:39, Jeff LaBundy wrote:
> Hi Joel,
> 
> Great work so far! It's coming along nicely. Please find my latest
> feedback below.

Sorry for the late reply, university semester end got me hooked up.
I have a sad kind of good news... As pointed out by Hans de Goede, the 
edt-ft5x06 driver works out of the box for the fts8719 touchscreen in my 
device. So I think this patch series is no longer needed. I did have a 
look at the driver once when working on this patch series, but it looked 
different/not compatible at that time. After mentioned by Hans de Goede, 
I had a more closer look and the main touch buffer handling seems to be 
the same.

I am sorry as we put some considerable time in this patch series. I 
should have noted more carefully. Thank you though as I learnt things 
working on this patch series.

I guess I will send a different patch to add the compatible data to the 
existing edt-ft5x06 driver and dts changes to include that driver to my 
device.

> On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
>> The Focaltech FTS driver supports several variants of focaltech
>> touchscreens found in ~2018 era smartphones including variants found on
>> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
>> This driver is loosely based on the original driver from Focaltech
>> but has been simplified and largely reworked.
>>
>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> ---
>>   MAINTAINERS                                   |   8 +
>>   drivers/input/touchscreen/Kconfig             |  12 +
>>   drivers/input/touchscreen/Makefile            |   1 +
>>   drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
>>   4 files changed, 453 insertions(+)
>>   create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7ec4ce64f66d..1a3ea61e1f52 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8028,6 +8028,14 @@ L:	linux-input@vger.kernel.org
>>   S:	Maintained
>>   F:	drivers/input/joystick/fsia6b.c
>>   
>> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
>> +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> +M:	Caleb Connolly <caleb@connolly.tech>
>> +L:	linux-input@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
>> +F:	drivers/input/touchscreen/focaltech_fts5452.c
>> +
>>   FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>>   M:	Geoffrey D. Bennett <g@b4.vu>
>>   L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 1feecd7ed3cb..11af91504969 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called exc3000.
>>   
>> +config TOUCHSCREEN_FOCALTECH_FTS5452
>> +	tristate "Focaltech FTS Touchscreen"
>> +	depends on I2C
>> +	help
>> +	  Say Y here to enable support for I2C connected Focaltech FTS
>> +	  based touch panels, including the 5452 and 8917 panels.
> 
> This language is a bit misleading, as it seems to suggest three or more
> models are supported. It seems the title should simply be "FocalTech
> FTS5452 touchscreen controller" with the description as "...FocalTech
> FTS5452 and compatible touchscreen controllers."
> 
> As more are found to be compatible (e.g. FTS8917), the compatible strings
> can simply be appended.
> 
>> +
>> +	  If unsure, say N.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called focaltech_fts.
>> +
>>   config TOUCHSCREEN_FUJITSU
>>   	tristate "Fujitsu serial touchscreen"
>>   	select SERIO
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..47d78c9cff21 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
>>   obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
>>   obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
>>   obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
>> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452)	+= focaltech_fts5452.o
>>   obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>>   obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
>>   obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
>> diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
>> new file mode 100644
>> index 000000000000..abf8a2f271ca
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/focaltech_fts5452.c
>> @@ -0,0 +1,432 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * FocalTech touchscreen driver.
>> + *
>> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
>> + * Copyright (C) 2018 XiaoMi, Inc.
>> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
>> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> + */

[skip]

>> +static const struct of_device_id fts_of_match[] = {
>> +	{ .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
>> +	{ .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
>> +	{ /* sentinel */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, fts_of_match);
>> +
>> +static struct i2c_driver fts_ts_driver = {
>> +	.probe_new = fts_ts_probe,
>> +	.id_table = fts_i2c_id,
>> +	.driver = {
>> +		.name = FTS_DRIVER_NAME,
>> +		.pm = pm_sleep_ptr(&fts_dev_pm_ops),
>> +		.of_match_table = fts_of_match,
>> +	},
>> +};
>> +module_i2c_driver(fts_ts_driver);
>> +
>> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
>> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
>> +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
> 
> Nit: mixing 'FocalTech' and 'Focaltech' throughout.
> 
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.40.0
>>
> 
> Kind regards,
> Jeff LaBundy

Thank you,
Joel Selvaraj

Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen

[Jeff LaBundy]

Hi Joel,

On Thu, May 11, 2023 at 07:43:28PM -0500, Joel Selvaraj wrote:
> Hi Jeff LaBundy,
> 
> On 4/23/23 21:39, Jeff LaBundy wrote:
> > Hi Joel,
> > 
> > Great work so far! It's coming along nicely. Please find my latest
> > feedback below.
> 
> Sorry for the late reply, university semester end got me hooked up.
> I have a sad kind of good news... As pointed out by Hans de Goede, the
> edt-ft5x06 driver works out of the box for the fts8719 touchscreen in my
> device. So I think this patch series is no longer needed. I did have a look
> at the driver once when working on this patch series, but it looked
> different/not compatible at that time. After mentioned by Hans de Goede, I
> had a more closer look and the main touch buffer handling seems to be the
> same.
> 
> I am sorry as we put some considerable time in this patch series. I should
> have noted more carefully. Thank you though as I learnt things working on
> this patch series.

There is absolutely no need to apologize; the review process is just as
informative for the reviewer as it is the reviewee. The important thing
is that the most optimal solution lands. "Less code is the best code" :)

> 
> I guess I will send a different patch to add the compatible data to the
> existing edt-ft5x06 driver and dts changes to include that driver to my
> device.

Sounds great.

> 
> > On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
> > > The Focaltech FTS driver supports several variants of focaltech
> > > touchscreens found in ~2018 era smartphones including variants found on
> > > the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> > > This driver is loosely based on the original driver from Focaltech
> > > but has been simplified and largely reworked.
> > > 
> > > Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> > > Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> > > Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> > > ---
> > >   MAINTAINERS                                   |   8 +
> > >   drivers/input/touchscreen/Kconfig             |  12 +
> > >   drivers/input/touchscreen/Makefile            |   1 +
> > >   drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
> > >   4 files changed, 453 insertions(+)
> > >   create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 7ec4ce64f66d..1a3ea61e1f52 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -8028,6 +8028,14 @@ L:	linux-input@vger.kernel.org
> > >   S:	Maintained
> > >   F:	drivers/input/joystick/fsia6b.c
> > > +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> > > +M:	Joel Selvaraj <joelselvaraj.oss@gmail.com>
> > > +M:	Caleb Connolly <caleb@connolly.tech>
> > > +L:	linux-input@vger.kernel.org
> > > +S:	Maintained
> > > +F:	Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
> > > +F:	drivers/input/touchscreen/focaltech_fts5452.c
> > > +
> > >   FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
> > >   M:	Geoffrey D. Bennett <g@b4.vu>
> > >   L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index 1feecd7ed3cb..11af91504969 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
> > >   	  To compile this driver as a module, choose M here: the
> > >   	  module will be called exc3000.
> > > +config TOUCHSCREEN_FOCALTECH_FTS5452
> > > +	tristate "Focaltech FTS Touchscreen"
> > > +	depends on I2C
> > > +	help
> > > +	  Say Y here to enable support for I2C connected Focaltech FTS
> > > +	  based touch panels, including the 5452 and 8917 panels.
> > 
> > This language is a bit misleading, as it seems to suggest three or more
> > models are supported. It seems the title should simply be "FocalTech
> > FTS5452 touchscreen controller" with the description as "...FocalTech
> > FTS5452 and compatible touchscreen controllers."
> > 
> > As more are found to be compatible (e.g. FTS8917), the compatible strings
> > can simply be appended.
> > 
> > > +
> > > +	  If unsure, say N.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called focaltech_fts.
> > > +
> > >   config TOUCHSCREEN_FUJITSU
> > >   	tristate "Fujitsu serial touchscreen"
> > >   	select SERIO
> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > > index 159cd5136fdb..47d78c9cff21 100644
> > > --- a/drivers/input/touchscreen/Makefile
> > > +++ b/drivers/input/touchscreen/Makefile
> > > @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
> > >   obj-$(CONFIG_TOUCHSCREEN_EGALAX)	+= egalax_ts.o
> > >   obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL)	+= egalax_ts_serial.o
> > >   obj-$(CONFIG_TOUCHSCREEN_EXC3000)	+= exc3000.o
> > > +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452)	+= focaltech_fts5452.o
> > >   obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
> > >   obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix_ts.o
> > >   obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
> > > diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
> > > new file mode 100644
> > > index 000000000000..abf8a2f271ca
> > > --- /dev/null
> > > +++ b/drivers/input/touchscreen/focaltech_fts5452.c
> > > @@ -0,0 +1,432 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * FocalTech touchscreen driver.
> > > + *
> > > + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> > > + * Copyright (C) 2018 XiaoMi, Inc.
> > > + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> > > + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> > > + */
> 
> [skip]
> 
> > > +static const struct of_device_id fts_of_match[] = {
> > > +	{ .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
> > > +	{ .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, fts_of_match);
> > > +
> > > +static struct i2c_driver fts_ts_driver = {
> > > +	.probe_new = fts_ts_probe,
> > > +	.id_table = fts_i2c_id,
> > > +	.driver = {
> > > +		.name = FTS_DRIVER_NAME,
> > > +		.pm = pm_sleep_ptr(&fts_dev_pm_ops),
> > > +		.of_match_table = fts_of_match,
> > > +	},
> > > +};
> > > +module_i2c_driver(fts_ts_driver);
> > > +
> > > +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> > > +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> > > +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
> > 
> > Nit: mixing 'FocalTech' and 'Focaltech' throughout.
> > 
> > > +MODULE_LICENSE("GPL");
> > > -- 
> > > 2.40.0
> > > 
> > 
> > Kind regards,
> > Jeff LaBundy
> 
> Thank you,
> Joel Selvaraj

Kind regards,
Jeff LaBundy