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

[PATCH 0/5] Add support 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 and
the patches submitted by Caleb Connolly previously[1] but has been
simplified and largely reworked.

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

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

Joel Selvaraj (5):
  dt-bindings: input: touchscreen: add bindings for focaltech,fts
  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,fts.yaml      |  81 ++++
 MAINTAINERS                                   |   8 +
 .../boot/dts/qcom/sdm845-shift-axolotl.dts    |  17 +-
 .../qcom/sdm845-xiaomi-beryllium-common.dtsi  |  39 ++
 .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts |  27 ++
 drivers/input/touchscreen/Kconfig             |  12 +
 drivers/input/touchscreen/Makefile            |   1 +
 drivers/input/touchscreen/focaltech_fts.c     | 448 ++++++++++++++++++
 8 files changed, 625 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
 create mode 100644 drivers/input/touchscreen/focaltech_fts.c

-- 
2.39.2

[PATCH 1/5] dt-bindings: input: touchscreen: add bindings for focaltech,fts

[Joel Selvaraj]

Add devicetree bindings for the Focaltech FTS touchscreen drivers.

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

diff --git a/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
new file mode 100644
index 000000000000..07fe595cc9ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/focaltech,fts.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: a phandle for the regulator supplying analog power (2.6V to 3.3V).
+
+  vddio-supply:
+    description: a phandle for the regulator supplying IO power (1.8V).
+
+  focaltech,max-touch-number:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: max number of fingers supported
+    minimum: 2
+    maximum: 10
+
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - focaltech,max-touch-number
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    &i2c5 {
+      status="okay";
+
+      touchscreen: focaltech@38 {
+        compatible = "focaltech,fts8719";
+        reg = <0x38>;
+        interrupt-parent = <&tlmm>;
+        interrupts = <31 IRQ_TYPE_EDGE_RISING>;
+
+        avdd-supply = <&vreg_l28a_3p0>;
+        vddio-supply = <&vreg_l14a_1p8>;
+
+        pinctrl-names = "default", "sleep";
+        pinctrl-0 = <&ts_int_default &ts_reset_default>;
+        pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
+
+        reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
+
+        touchscreen-size-x = <1080>;
+        touchscreen-size-y = <2246>;
+        focaltech,max-touch-number = <10>;
+      };
+    };
-- 
2.39.2

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

[Krzysztof Kozlowski]

On 12/03/2023 10:32, Joel Selvaraj wrote:
> Add devicetree bindings for the Focaltech FTS touchscreen drivers.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../input/touchscreen/focaltech,fts.yaml      | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> new file mode 100644
> index 000000000000..07fe595cc9ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml

I have doubts you will cover here all possible FTS controllers, so
filename should be more specific, e.g. choose the oldest device compatible.

> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/focaltech,fts.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

Missing blank line

> +  reg:
> +    const: 0x38
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  avdd-supply:
> +    description: a phandle for the regulator supplying analog power (2.6V to 3.3V).

Drop "a phandle for the"

> +
> +  vddio-supply:
> +    description: a phandle for the regulator supplying IO power (1.8V).

Ditto

> +
> +  focaltech,max-touch-number:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: max number of fingers supported

Why this is not implied from compatible? IOW, why this differs between
boards?

If this property stays, then anyway "focaltech,max-touch", not number.
There is no such unit suffix as number.

> +    minimum: 2
> +    maximum: 10
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true

Drop these two

> +
> +additionalProperties: false

and then use unevaluatedProperties: false
so all properties from common schema apply. Unless these are not really
valid for the *device*?

> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - focaltech,max-touch-number
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    &i2c5 {

i2c

> +      status="okay";

Drop status

Anyway this should pop warnings... Please run `make dt_binding_check`
(see Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +
> +      touchscreen: focaltech@38 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, drop label touchscreen.

> +        compatible = "focaltech,fts8719";
> +        reg = <0x38>;
> +        interrupt-parent = <&tlmm>;
> +        interrupts = <31 IRQ_TYPE_EDGE_RISING>;
> +


Best regards,
Krzysztof

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

[Joel Selvaraj]

Hi Krzysztof,

Thanks for the review! I agree with most of your comments and will
fix them in v2. I have a few doubts as discussed below.

On 12/03/23 15:47, Krzysztof Kozlowski wrote:
> I have doubts you will cover here all possible FTS controllers, so
> filename should be more specific, e.g. choose the oldest device compatible.

The driver is kind of widely used and can actually support 49 touch
panel variants as per the downstream code [1]. With some slight
modifications, the other touch panels can be supported too. However, in
real world, we have only tested the driver against the two panel we have
access to (FT8719 - Poco F1 Phone and FT5452 - Shiftmq6 Phone).

Although its very generic and widely used, I agree we don't know that
will be the case forever. So I am ok with changing it to more specific
one. But I don't think the panel chip number denote which is older and
which newer. Shall I just go with focaltech,fts5452, as that's the
lowest number panel that we have tested so far and is supported?

Or do I just keep it generic as it can potentially support a lot of
variants?

>> +  focaltech,max-touch-number:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: max number of fingers supported
> 
> Why this is not implied from compatible? IOW, why this differs between
> boards?

Without proper datasheet it is kind of hard to say if this is the
maximum supported touch points by hardware or just a vendor specified
one. Because, downstream has it as devicetree property and we only know
what's set in that from each vendor tree. The FT8719 used in Poco F1
specifies 10 touch points in downstrean devicetree. But, if I specify it
as 2, it will still work fine. The FT5452 used in shiftmq6 specifies 5
touch points in downstream devicetree, but we won't know if that is the
maximum possible, unless we try to increase it upto 10 and confirm.

So, yeah without the datasheet, we will be just kind of assuming that is
is the maximum possible number of touch points by the hardware. I am not
sure if we wanna hard code that in the driver. Is it okay if we let this
configurable? Boards/Phones can use the max touch number their vendor
driver points too or if they have a datasheet, they can specify maximum
supported one too.

Kindly let me know your thoughts on this.

[1]
https://github.com/LineageOS/android_kernel_xiaomi_sdm845/blob/f1977d9edd01cff3fc9a12e09cd6a5a8052fc8ca/drivers/input/touchscreen/focaltech_touch/focaltech_config.h#L37

> Best regards,
> Krzysztof

Thanks,
Joel

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

[Krzysztof Kozlowski]

On 13/03/2023 01:21, Joel Selvaraj wrote:
> Hi Krzysztof,
> 
> Thanks for the review! I agree with most of your comments and will
> fix them in v2. I have a few doubts as discussed below.
> 
> On 12/03/23 15:47, Krzysztof Kozlowski wrote:
>> I have doubts you will cover here all possible FTS controllers, so
>> filename should be more specific, e.g. choose the oldest device compatible.
> 
> The driver is kind of widely used and can actually support 49 touch
> panel variants as per the downstream code [1]. With some slight
> modifications, the other touch panels can be supported too. However, in
> real world, we have only tested the driver against the two panel we have
> access to (FT8719 - Poco F1 Phone and FT5452 - Shiftmq6 Phone).
> 
> Although its very generic and widely used, I agree we don't know that
> will be the case forever. So I am ok with changing it to more specific
> one. But I don't think the panel chip number denote which is older and
> which newer. Shall I just go with focaltech,fts5452, as that's the
> lowest number panel that we have tested so far and is supported?
> 
> Or do I just keep it generic as it can potentially support a lot of
> variants?
> 
>>> +  focaltech,max-touch-number:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: max number of fingers supported
>>
>> Why this is not implied from compatible? IOW, why this differs between
>> boards?
> 
> Without proper datasheet it is kind of hard to say if this is the
> maximum supported touch points by hardware or just a vendor specified
> one. Because, downstream has it as devicetree property and we only know
> what's set in that from each vendor tree. The FT8719 used in Poco F1
> specifies 10 touch points in downstrean devicetree. But, if I specify it
> as 2, it will still work fine. The FT5452 used in shiftmq6 specifies 5
> touch points in downstream devicetree, but we won't know if that is the
> maximum possible, unless we try to increase it upto 10 and confirm.
> 
> So, yeah without the datasheet, we will be just kind of assuming that is
> is the maximum possible number of touch points by the hardware. I am not
> sure if we wanna hard code that in the driver. Is it okay if we let this
> configurable? Boards/Phones can use the max touch number their vendor
> driver points too or if they have a datasheet, they can specify maximum
> supported one too.

Downstream DTS is never a guideline on design of upstream bindings. They
violate DT binding rules so many times so much, that I don't treat it as
argument.

The property does not look board but device specific, so you should
infer it from compatible.


Best regards,
Krzysztof

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

[Joel Selvaraj]

Hi Krzysztof,

On 13/03/23 01:42, Krzysztof Kozlowski wrote:
> Downstream DTS is never a guideline on design of upstream bindings. They
> violate DT binding rules so many times so much, that I don't treat it as
> argument.
> 
> The property does not look board but device specific, so you should
> infer it from compatible.

Understood. Will do so in v2.

Thanks,
Joel

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

[Rob Herring]

On Sun, 12 Mar 2023 04:32:45 -0500, Joel Selvaraj wrote:
> Add devicetree bindings for the Focaltech FTS touchscreen drivers.
> 
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
>  .../input/touchscreen/focaltech,fts.yaml      | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.example.dts:23.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230312093249.1846993-2-joelselvaraj.oss@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 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_fts.c | 448 ++++++++++++++++++++++
 4 files changed, 469 insertions(+)
 create mode 100644 drivers/input/touchscreen/focaltech_fts.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2892858cb040..deb561c356f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7993,6 +7993,14 @@ L:	linux-input@vger.kernel.org
 S:	Maintained
 F:	drivers/input/joystick/fsia6b.c
 
+FOCALTECH FTS 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,fts.yaml
+F:	drivers/input/touchscreen/focaltech_fts.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 1a2049b336a6..320925bac3a1 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_FTS
+	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 f2fd28cc34a6..83ea2e3ce754 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_FTS)	+= focaltech_fts.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_fts.c b/drivers/input/touchscreen/focaltech_fts.c
new file mode 100644
index 000000000000..d389c8b88944
--- /dev/null
+++ b/drivers/input/touchscreen/focaltech_fts.c
@@ -0,0 +1,448 @@
+// 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>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/mutex.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_DRIVER_NAME "fts-i2c"
+#define INTERVAL_READ_REG 100 /* unit:ms */
+#define TIMEOUT_READ_REG 2000 /* unit:ms */
+
+#define CHIP_TYPE_5452 0x5452
+#define CHIP_TYPE_8719 0x8719
+
+struct fts_ts_data {
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	struct touchscreen_properties prop;
+
+	struct regmap *regmap;
+	int irq;
+
+	struct regulator_bulk_data regulators[2];
+
+	/* Touch data */
+	u8 max_touch_number;
+	u8 *point_buf;
+	int point_buf_size;
+
+	/* DT data */
+	struct gpio_desc *reset_gpio;
+};
+
+static const struct regmap_config fts_ts_i2c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
+{
+	if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
+		return false;
+
+	return true;
+}
+
+int fts_check_status(struct fts_ts_data *data)
+{
+	int count = 0;
+	unsigned int val, id;
+
+	do {
+		regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
+		id = val << 8;
+		regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
+		id |= val;
+
+		if (fts_chip_is_valid(data, id)) {
+			dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
+			return 0;
+		}
+
+		count++;
+		msleep(INTERVAL_READ_REG);
+	} while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
+
+	return -EIO;
+}
+
+static void 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_ratelimited(&data->client->dev, "I2C read failed: %d\n",
+				    error);
+		return;
+	}
+
+	for (i = 0; i < data->max_touch_number; i++) {
+		base = FTS_ONE_TOUCH_LEN * i;
+
+		slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
+		if (slot >= data->max_touch_number)
+			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_sync(input_dev);
+}
+
+static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
+{
+	struct fts_ts_data *data = dev_id;
+
+	fts_report_touch(data);
+
+	return 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;
+
+	/* Init and register Input device */
+	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);
+
+	__set_bit(EV_SYN, input_dev->evbit);
+	__set_bit(EV_ABS, input_dev->evbit);
+	__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+
+	input_mt_init_slots(input_dev, data->max_touch_number,
+			    INPUT_MT_DIRECT);
+	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 dts\n");
+		return -EINVAL;
+	}
+
+	data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
+	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
+	if (!data->point_buf) {
+		dev_err(dev, "Failed to alloc memory for point buffer\n");
+		return -ENOMEM;
+	}
+
+	error = input_register_device(input_dev);
+	if (error) {
+		dev_err(dev, "Failed to register input device\n");
+		return error;
+	}
+
+	return 0;
+}
+
+static int fts_parse_dt(struct fts_ts_data *data)
+{
+	int error = 0;
+	struct device *dev = &data->client->dev;
+	struct device_node *np = dev->of_node;
+	u32 val;
+
+	error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
+	if (error) {
+		dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
+		return -EINVAL;
+	}
+	if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
+		dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
+			FTS_MAX_POINTS_SUPPORT);
+		return -EINVAL;
+	}
+	data->max_touch_number = val;
+
+	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->reset_gpio)) {
+		error = PTR_ERR(data->reset_gpio);
+		dev_err(dev, "Failed to request reset gpio, error %d\n", error);
+		return error;
+	}
+
+	return 0;
+}
+
+static int fts_ts_probe(struct i2c_client *client)
+{
+	int error = 0;
+	struct fts_ts_data *data;
+
+	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;
+
+	data->client = client;
+
+	error = fts_parse_dt(data);
+	if (error)
+		return error;
+
+	i2c_set_clientdata(client, data);
+
+	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&client->dev, "regmap allocation failed\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	/*
+	 * 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 const struct dev_pm_ops fts_dev_pm_ops = {
+	.suspend = fts_pm_suspend,
+	.resume = fts_pm_resume,
+};
+
+static const struct of_device_id fts_match_table[] = {
+	{ .compatible = "focaltech,fts5452", },
+	{ .compatible = "focaltech,fts8719", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, fts_match_table);
+
+static struct i2c_driver fts_ts_driver = {
+	.probe_new = fts_ts_probe,
+	.driver = {
+		.name = FTS_DRIVER_NAME,
+		.pm = &fts_dev_pm_ops,
+		.of_match_table = fts_match_table,
+	},
+};
+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.39.2

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

[Markuss Broks]

Hi Joel,

Some comments about the driver below,

On 3/12/23 11:32, 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_fts.c | 448 ++++++++++++++++++++++
>   4 files changed, 469 insertions(+)
>   create mode 100644 drivers/input/touchscreen/focaltech_fts.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2892858cb040..deb561c356f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7993,6 +7993,14 @@ L:	linux-input@vger.kernel.org
>   S:	Maintained
>   F:	drivers/input/joystick/fsia6b.c
>   
> +FOCALTECH FTS 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,fts.yaml
> +F:	drivers/input/touchscreen/focaltech_fts.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 1a2049b336a6..320925bac3a1 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_FTS
> +	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 f2fd28cc34a6..83ea2e3ce754 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_FTS)	+= focaltech_fts.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_fts.c b/drivers/input/touchscreen/focaltech_fts.c
> new file mode 100644
> index 000000000000..d389c8b88944
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts.c
> @@ -0,0 +1,448 @@
> +// 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>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
Includes must be sorted alphabetically.
> +
> +#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_DRIVER_NAME "fts-i2c"
> +#define INTERVAL_READ_REG 100 /* unit:ms */
> +#define TIMEOUT_READ_REG 2000 /* unit:ms */
Instead of using those comments, it's better to use the _MS suffix.
> +
> +#define CHIP_TYPE_5452 0x5452
> +#define CHIP_TYPE_8719 0x8719
> +
> +struct fts_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct touchscreen_properties prop;
> +
> +	struct regmap *regmap;
> +	int irq;
> +
> +	struct regulator_bulk_data regulators[2];
> +
> +	/* Touch data */
> +	u8 max_touch_number;
> +	u8 *point_buf;
> +	int point_buf_size;
> +
> +	/* DT data */
> +	struct gpio_desc *reset_gpio;
> +};
> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
> +{
> +	if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
> +		return false;
> +
> +	return true;
> +}
> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> +	int count = 0;
> +	unsigned int val, id;
> +
> +	do {
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> +		id = val << 8;
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
> +		id |= val;
That register layout is surely... weird. Anyway, I'd probably do
```
         regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
         regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
         id |= val << 8;
```
to make it less actions. Also, this lacks error checking for the regmap 
calls.
> +
> +		if (fts_chip_is_valid(data, id)) {
> +			dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
> +			return 0;
> +		}
> +
> +		count++;
> +		msleep(INTERVAL_READ_REG);
> +	} while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
> +
> +	return -EIO;
> +}
> +
> +static void 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_ratelimited(&data->client->dev, "I2C read failed: %d\n",
> +				    error);
Why is the _ratelimited variant necessary?
> +		return;
> +	}
> +
> +	for (i = 0; i < data->max_touch_number; i++) {
> +		base = FTS_ONE_TOUCH_LEN * i;
> +
> +		slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
I believe parentheses aren't needed?
> +		if (slot >= data->max_touch_number)
> +			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);
> +		}
> +	}
Overall, I think it's better to cast the data type to a struct, which 
would make this seem with less random.
> +	input_sync(input_dev);
> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct fts_ts_data *data = dev_id;
> +
> +	fts_report_touch(data);
> +
> +	return 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;
> +
> +	/* Init and register Input device */
> +	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);
> +
> +	__set_bit(EV_SYN, input_dev->evbit);
> +	__set_bit(EV_ABS, input_dev->evbit);
> +	__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +
> +	input_mt_init_slots(input_dev, data->max_touch_number,
> +			    INPUT_MT_DIRECT);
> +	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 dts\n");
> +		return -EINVAL;
> +	}
> +
> +	data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
> +	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> +	if (!data->point_buf) {
> +		dev_err(dev, "Failed to alloc memory for point buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fts_parse_dt(struct fts_ts_data *data)
> +{
> +	int error = 0;
> +	struct device *dev = &data->client->dev;
> +	struct device_node *np = dev->of_node;
> +	u32 val;
> +
> +	error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
> +	if (error) {
> +		dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
> +		return -EINVAL;
> +	}
> +	if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
> +		dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
> +			FTS_MAX_POINTS_SUPPORT);
> +		return -EINVAL;
> +	}
> +	data->max_touch_number = val;
> +
> +	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		dev_err(dev, "Failed to request reset gpio, error %d\n", error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> +	int error = 0;
> +	struct fts_ts_data *data;
> +
> +	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;
> +
> +	data->client = client;
> +
> +	error = fts_parse_dt(data);
> +	if (error)
> +		return error;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap allocation failed\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	/*
> +	 * 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 const struct dev_pm_ops fts_dev_pm_ops = {
> +	.suspend = fts_pm_suspend,
> +	.resume = fts_pm_resume,
> +};
> +
> +static const struct of_device_id fts_match_table[] = {
> +	{ .compatible = "focaltech,fts5452", },
> +	{ .compatible = "focaltech,fts8719", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_match_table);
> +
> +static struct i2c_driver fts_ts_driver = {
> +	.probe_new = fts_ts_probe,
> +	.driver = {
> +		.name = FTS_DRIVER_NAME,
> +		.pm = &fts_dev_pm_ops,
> +		.of_match_table = fts_match_table,
> +	},
> +};
> +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");
- Markuss

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

[Joel Selvaraj]

Hi Markuss,

Thanks for the quick review! I agree with most of your comments and will
fix them in a v2 soon. I have a few doubts as discussed below.

On 12/03/23 15:40, Markuss Broks wrote:

> Why is the _ratelimited variant necessary?

I assumed in case of the interrupt working, but i2c reads fail for some
reason, it would spam a lot of error messages if the user touches the
screen continuously, like a swipe up gesture or something.

I referred to ad7879 touchscreen's irq handling code [1] and thought
it's probably best to do this, to be on the safe side. I will remove
this if it's not needed in v2.

> Overall, I think it's better to cast the data type to a struct, which
> would make this seem with less random.

Sorry, I am not sure I got this right. Do you mean I create an array of
struct called say "fts_point" that stores the x, y, type, etc. info of
all the points, then report it separately. Like similar to something
done by the auo-pixcir touchscreen driver [2]?

If I didn't get this correctly, can you show me some code in mainline,
that does it? It would be very helpful.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/ad7879.c?h=v6.3-rc1#n250
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/auo-pixcir-ts.c?h=v6.3-rc1#n162

> - Markuss
Thanks,
Joel

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

[Markuss Broks]

Hi Joel,

Sorry for responding late:

On 3/13/23 00:21, Joel Selvaraj wrote:
> Hi Markuss,
>
> Thanks for the quick review! I agree with most of your comments and will
> fix them in a v2 soon. I have a few doubts as discussed below.
>
> On 12/03/23 15:40, Markuss Broks wrote:
>
>> Why is the _ratelimited variant necessary?
> I assumed in case of the interrupt working, but i2c reads fail for some
> reason, it would spam a lot of error messages if the user touches the
> screen continuously, like a swipe up gesture or something.
>
> I referred to ad7879 touchscreen's irq handling code [1] and thought
> it's probably best to do this, to be on the safe side. I will remove
> this if it's not needed in v2.
>
>> Overall, I think it's better to cast the data type to a struct, which
>> would make this seem with less random.
> Sorry, I am not sure I got this right. Do you mean I create an array of
> struct called say "fts_point" that stores the x, y, type, etc. info of
> all the points, then report it separately. Like similar to something
> done by the auo-pixcir touchscreen driver [2]?

By that I meant doing something like the Zinitix driver[1] does. It has 
a struct data type for whatever you read from hardware, e.g.

struct point_coord {
     __le16    x;
     __le16    y;
...
};

from that driver. That way you can cast the data read to that struct and 
have it look a bit nicer.

This is just a suggestion though, you have the final choice in what 
design you choose for your code :)

>
> If I didn't get this correctly, can you show me some code in mainline,
> that does it? It would be very helpful.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/ad7879.c?h=v6.3-rc1#n250
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/auo-pixcir-ts.c?h=v6.3-rc1#n162
>
>> - Markuss
> Thanks,
> Joel
[1]
https://elixir.free-electrons.com/linux/v6.3-rc5/source/drivers/input/touchscreen/zinitix.c


- Markuss

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

[Joel Selvaraj]

Hi Markuss,

On 09/04/23 08:32, Markuss Broks wrote:
> By that I meant doing something like the Zinitix driver[1] does. It has
> a struct data type for whatever you read from hardware, e.g.
> 
> struct point_coord {
>     __le16    x;
>     __le16    y;
> ...
> };
> 
> from that driver. That way you can cast the data read to that struct and
> have it look a bit nicer.

Understood. Thanks for the clarification. Jeff LaBundy explained it a
bit too. I have addressed all the other comments in this patch series in
my WIP v2. However, I am having quite the trouble casting the buffer to
a struct. The register layout is bit weird. It would have been easier if
they are sets of 8bits or 16bits values. Instead it seems to be split in
terms 4bits and 12bits or I don't know. I am a bit new to this and
having trouble handling endianess with these weird splits in the buffer.

Here is a hand drawn image of the buffer layout [1]. Let me know if you
or anyone have any thoughts on this. Wonder if it's worth the trouble :)

[1] https://imgur.com/a/4RYrB1G

> This is just a suggestion though, you have the final choice in what
> design you choose for your code :)

I am gonna try it a few more days and if I can't make it work in a
sensible way, I will probably go with the existing approach.

> - Markuss

Thanks,
Joel

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

[Jeff LaBundy]

Hi Joel (and Caleb),

Great work! It's nice to see these devices get one step closer to
upstream. In addition to Markuss' valuable feedback, I have left a
few comments throughout.

On Sun, Mar 12, 2023 at 04:32:46AM -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_fts.c | 448 ++++++++++++++++++++++
>  4 files changed, 469 insertions(+)
>  create mode 100644 drivers/input/touchscreen/focaltech_fts.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2892858cb040..deb561c356f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7993,6 +7993,14 @@ L:	linux-input@vger.kernel.org
>  S:	Maintained
>  F:	drivers/input/joystick/fsia6b.c
>  
> +FOCALTECH FTS 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,fts.yaml
> +F:	drivers/input/touchscreen/focaltech_fts.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 1a2049b336a6..320925bac3a1 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_FTS
> +	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 f2fd28cc34a6..83ea2e3ce754 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_FTS)	+= focaltech_fts.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_fts.c b/drivers/input/touchscreen/focaltech_fts.c
> new file mode 100644
> index 000000000000..d389c8b88944
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts.c
> @@ -0,0 +1,448 @@
> +// 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>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

The SPDX tag relieves you of the need to repeat any license terms. These
two paragraphs can be dropped.

> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.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_DRIVER_NAME "fts-i2c"
> +#define INTERVAL_READ_REG 100 /* unit:ms */
> +#define TIMEOUT_READ_REG 2000 /* unit:ms */

In addition to what Markuss has mentioned, please namespace all #defines
(i.e. FTS_xxx). I also find these easier to read if they are all aligned
horizontally.

> +
> +#define CHIP_TYPE_5452 0x5452
> +#define CHIP_TYPE_8719 0x8719

A #define with the name equal to the value is a bit unnecessary; below I
will suggest an alternative.

> +
> +struct fts_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct touchscreen_properties prop;
> +
> +	struct regmap *regmap;
> +	int irq;
> +
> +	struct regulator_bulk_data regulators[2];
> +
> +	/* Touch data */
> +	u8 max_touch_number;
> +	u8 *point_buf;
> +	int point_buf_size;
> +
> +	/* DT data */
> +	struct gpio_desc *reset_gpio;
> +};

Technically the touchscreen_properties can be DT data too. I think the
comments and newlines in the struct definition are unnecessary.

> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};

I personally find the driver easier to read if this definition is closer
to probe, where it is used.

> +
> +static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
> +{
> +	if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
> +		return false;
> +
> +	return true;
> +}

This isn't particularly scalable; we would end up with a massive if block
as we add compatible devices. Instead, define an array toward the beginning
of the driver as follows:

static const u16 fts_chip_types[] = {
        0x5452,
        0x8719,
};

And then this function could turn into something like this:

        for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
                if (id = fts_chip_types[i])
                        return true;

        return false;

This would also get rid of those obvious #defines.

> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> +	int count = 0;
> +	unsigned int val, id;
> +
> +	do {
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> +		id = val << 8;
> +		regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
> +		id |= val;

There are cleaner alternatives for this, such as:

        __be16 val;

        do {
                error = regmap_raw_read(data->regmap, FTS_REG_CHIP_ID_H,
                                        &val, sizeof(val));
                if (error)
                        return error;

                if (fts_chip_is_valid(data, be16_to_cpu(id)) {
                        dev_dbg(...);
                        return 0;
                }

                /* ... */
        }

Please also note that there is no reason for fts_chip_is_valid() to
accept 'data' as an argument. One could also argue this function is
not doing enough work or being re-used enough to warrant being its
own function.

> +
> +		if (fts_chip_is_valid(data, id)) {
> +			dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
> +			return 0;
> +		}
> +
> +		count++;
> +		msleep(INTERVAL_READ_REG);
> +	} while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
> +
> +	return -EIO;
> +}
> +
> +static void 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_ratelimited(&data->client->dev, "I2C read failed: %d\n",
> +				    error);
> +		return;
> +	}
> +
> +	for (i = 0; i < data->max_touch_number; i++) {
> +		base = FTS_ONE_TOUCH_LEN * i;
> +
> +		slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
> +		if (slot >= data->max_touch_number)
> +			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);

It seems that the interrupt status registers can be represented with a packed
struct and then unpacked with be16_to_cpu().

> +
> +		z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> +		if (z <= 0)
> +			z = 0x3f;

z is unsigned; how can this happen?

> +
> +		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_sync(input_dev);

We need to call input_mt_sync_frame() as well.

> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct fts_ts_data *data = dev_id;
> +
> +	fts_report_touch(data);

Assuming the act of reading the interrupt status registers is what prompts the
device to deassert its interrupt pin, I recommend promoting fts_report_touch()
to int type and evaluating its return value as follows:

        return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;

This way if the register read fails and the interrupt is in theory not serviced,
the IRQ subsystem can mute the device and prevent an interrupt storm.

> +
> +	return 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;
> +
> +	/* Init and register Input device */

This comment is obvious and not entirely true, as we're not actually registering
the device until farther down.

> +	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);
> +
> +	__set_bit(EV_SYN, input_dev->evbit);

No need to set EV_SYN; input_register_device() does that for us.

> +	__set_bit(EV_ABS, input_dev->evbit);

No need to set EV_ABS; input_set_capability(..., EV_ABS, ...) does that for us.

> +	__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);

Same here; input_mt_init_slots() is already doing this.

> +
> +	input_mt_init_slots(input_dev, data->max_touch_number,
> +			    INPUT_MT_DIRECT);

Please check the return value of input_mt_init_slots(), and move this call
_after_ your axes have been defined. Otherwise, this function cannot copy
the multitouch axes to single-touch axes, and drivers such as libinput will
ignore this device.

> +	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 dts\n");

Note that dts may not always be the source of these (see comments below).

> +		return -EINVAL;
> +	}
> +
> +	data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
> +	data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> +	if (!data->point_buf) {
> +		dev_err(dev, "Failed to alloc memory for point buffer\n");

Did checkpatch not gripe about this? No need to print a warning for failed
memory allocation.

> +		return -ENOMEM;
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fts_parse_dt(struct fts_ts_data *data)
> +{
> +	int error = 0;
> +	struct device *dev = &data->client->dev;
> +	struct device_node *np = dev->of_node;
> +	u32 val;
> +
> +	error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
> +	if (error) {
> +		dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
> +		return -EINVAL;

Just return 'error' rather than papering over the actual error code.

> +	}

Please switch to the device_property API to allow easy adoption of ACPI
at a later time. To that end, it is not necessarily appropriate to use
the term 'dt' in the name of this function.

That being said, I do agree with Krzysztof's point in the binding review;
it seems you are relying on the user to know the hardware limitation. In
my opinion this is the driver's responsibility.

I think it is OK if you want the user to have the option to artificially
limit the number of points, but in that case the property should be optional.

> +	if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
> +		dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
> +			FTS_MAX_POINTS_SUPPORT);
> +		return -EINVAL;
> +	}

Since FTS_MAX_POINTS_SUPPORT exists, it seems FTS_MIN_POINTS_SUPPORT should too.

That being said, why not to support single-touch applications? It does not seem
you write this value back to a device register, so will the device not simply
report one contact if that is all that exists? input_mt_init_slots() is perfectly
happy with one slot.

> +	data->max_touch_number = val;
> +
> +	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		dev_err(dev, "Failed to request reset gpio, error %d\n", error);
> +		return error;
> +	}

Having a hardware reset pin is nice, but often it is given up in case the board
designer runs out of GPIO and pulls the reset pin up locally. Can it be optional?

> +
> +	return 0;
> +}
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> +	int error = 0;
> +	struct fts_ts_data *data;
> +
> +	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;
> +
> +	data->client = client;
> +
> +	error = fts_parse_dt(data);
> +	if (error)
> +		return error;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap allocation failed\n");

It would be nice to save and print the error code as you do above for the
reset GPIO.

> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	/*
> +	 * 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);

Nit: alignment

> +	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 const struct dev_pm_ops fts_dev_pm_ops = {
> +	.suspend = fts_pm_suspend,
> +	.resume = fts_pm_resume,
> +};

Please use the new DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr().

> +
> +static const struct of_device_id fts_match_table[] = {
> +	{ .compatible = "focaltech,fts5452", },
> +	{ .compatible = "focaltech,fts8719", },
> +	{ /* sentinel */ },

Nit: no need for a trailing comma after the sentinel, as no patch would
ever add a line below it.

> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_match_table);
> +
> +static struct i2c_driver fts_ts_driver = {
> +	.probe_new = fts_ts_probe,
> +	.driver = {
> +		.name = FTS_DRIVER_NAME,
> +		.pm = &fts_dev_pm_ops,
> +		.of_match_table = fts_match_table,
> +	},
> +};
> +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: inconsistent capitalization

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

Kind regards,
Jeff LaBundy

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

[Joel Selvaraj]

Hi Jeff LaBundy,

Thanks for your clear and in-depth review! It has been very helpful. I
will address them in v2.

Thanks,
Joel

[PATCH 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  | 39 +++++++++++++++++++
 1 file changed, 39 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 e0fda4d754fe..ecfd85bde966 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
@@ -270,6 +270,10 @@ &gmu {
 	status = "okay";
 };
 
+&gpi_dma1 {
+	status = "okay";
+};
+
 &gpu {
 	status = "okay";
 
@@ -368,6 +372,10 @@ &qupv3_id_0 {
 	status = "okay";
 };
 
+&qupv3_id_1 {
+	status = "okay";
+};
+
 &sdhc_2 {
 	status = "okay";
 
@@ -473,6 +481,37 @@ 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;
+		input-enable;
+	};
+
+	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;
+		input-enable;
+	};
+
+	ts_reset_sleep: ts-reset-sleep-state {
+		pins = "gpio32";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+		output-low;
+	};
 };
 
 &uart6 {
-- 
2.39.2

[PATCH 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 | 27 +++++++++++++++++++
 1 file changed, 27 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..a23be4c8e1bb 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,30 @@ &display_panel {
 	compatible = "ebbg,ft8719";
 	status = "okay";
 };
+
+&i2c14 {
+	status = "okay";
+
+	dmas =  <&gpi_dma1 0 6 QCOM_GPI_I2C>,
+		<&gpi_dma1 1 6 QCOM_GPI_I2C>;
+	dma-names = "tx", "rx";
+
+	touchscreen: focaltech@38 {
+		compatible = "focaltech,fts8719";
+		reg = <0x38>;
+		interrupt-parent = <&tlmm>;
+		interrupts = <31 IRQ_TYPE_EDGE_RISING>;
+
+		vddio-supply = <&vreg_l14a_1p8>;
+
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&ts_int_default &ts_reset_default>;
+		pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
+
+		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
+
+		touchscreen-size-x = <1080>;
+		touchscreen-size-y = <2246>;
+		focaltech,max-touch-number = <10>;
+	};
+};
-- 
2.39.2

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

[Krzysztof Kozlowski]

On 12/03/2023 10:32, 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>
> ---
>  .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 27 +++++++++++++++++++
>  1 file changed, 27 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..a23be4c8e1bb 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,30 @@ &display_panel {
>  	compatible = "ebbg,ft8719";
>  	status = "okay";
>  };
> +
> +&i2c14 {
> +	status = "okay";
> +
> +	dmas =  <&gpi_dma1 0 6 QCOM_GPI_I2C>,
> +		<&gpi_dma1 1 6 QCOM_GPI_I2C>;
> +	dma-names = "tx", "rx";
> +
> +	touchscreen: focaltech@38 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation



Best regards,
Krzysztof

[PATCH 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      | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 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..39f59ee3612a 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
@@ -473,21 +473,22 @@ zap-shader {
 &i2c5 {
 	status = "okay";
 
-	touchscreen@38 {
-		compatible = "focaltech,fts8719";
+	touchscreen: focaltech@38 {
+		compatible = "focaltech,fts5452";
 		reg = <0x38>;
-		wakeup-source;
+
 		interrupt-parent = <&tlmm>;
-		interrupts = <125 0x2>;
-		vdd-supply = <&vreg_l28a_3p0>;
-		vcc-i2c-supply = <&vreg_l14a_1p88>;
+		interrupts = <125 IRQ_TYPE_EDGE_FALLING>;
+
+		avdd-supply = <&vreg_l28a_3p0>;
+		vddio-supply = <&vreg_l14a_1p88>;
 
 		pinctrl-names = "default", "suspend";
 		pinctrl-0 = <&ts_int_active &ts_reset_active>;
 		pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
 
-		reset-gpio = <&tlmm 99 GPIO_ACTIVE_HIGH>;
-		irq-gpio = <&tlmm 125 GPIO_TRANSITORY>;
+		reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
 		touchscreen-size-x = <1080>;
 		touchscreen-size-y = <2160>;
 		focaltech,max-touch-number = <5>;
-- 
2.39.2

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

[Krzysztof Kozlowski]

On 12/03/2023 10:32, 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>
> ---
>  .../boot/dts/qcom/sdm845-shift-axolotl.dts      | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 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..39f59ee3612a 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> @@ -473,21 +473,22 @@ zap-shader {
>  &i2c5 {
>  	status = "okay";
>  
> -	touchscreen@38 {
> -		compatible = "focaltech,fts8719";
> +	touchscreen: focaltech@38 {

So you had good name and you replace it to something wrong... Drop the
label. This actually applies to all other patches, unless you need it.

Best regards,
Krzysztof