| Message ID | 20230312093249.1846993-2-joelselvaraj.oss@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Focaltech FTS Touchscreen | expand |
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
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
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
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
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 --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>; + }; + };