diff mbox series

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

Message ID 20230312093249.1846993-2-joelselvaraj.oss@gmail.com
State New
Headers show
Series Add support for Focaltech FTS Touchscreen | expand

Commit Message

Joel Selvaraj March 12, 2023, 9:32 a.m. UTC
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

Comments

Krzysztof Kozlowski March 12, 2023, 8:47 p.m. UTC | #1
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
Joel Selvaraj March 13, 2023, 12:21 a.m. UTC | #2
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
Krzysztof Kozlowski March 13, 2023, 6:42 a.m. UTC | #3
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
Joel Selvaraj March 13, 2023, 6:53 a.m. UTC | #4
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
Rob Herring March 14, 2023, 2:10 p.m. UTC | #5
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.
diff mbox series

Patch

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>;
+      };
+    };