2021-11-03 11:50:14

by Alistair Francis

[permalink] [raw]
Subject: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

From: Mylène Josserand <[email protected]>

Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings
documentation. It can use I2C or SPI bus.
This touchscreen can handle some defined zone that are designed and
sent as button. To be able to customize the keycode sent, the
"linux,code" property in a "button" sub-node can be used.

Signed-off-by: Mylène Josserand <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Alistair Francis <[email protected]>
---
.../input/touchscreen/cypress,tt21000.yaml | 92 +++++++++++++++++++
1 file changed, 92 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml b/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
new file mode 100644
index 000000000000..ff7eca412440
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/cypress,cyttsp5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cypress TT2100 touchscreen controller
+
+description: The Cypress TT2100 series (also known as "CYTTSP5" after
+ the marketing name Cypress TrueTouch Standard Product series 5).
+
+maintainers:
+ - Alistair Francis <[email protected]>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ const: cypress,tt21000
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ vdd-supply:
+ description: Regulator for voltage.
+
+ reset-gpios:
+ maxItems: 1
+
+ linux,code:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: EV_ABS specific event code generated by the axis.
+
+patternProperties:
+ "^button-[0-9]+$":
+ type: object
+ properties:
+ linux,code:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: Keycode to emit
+
+ required:
+ - linux,code
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - vdd-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/input/linux-event-codes.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@24 {
+ compatible = "cypress,tt2100";
+ reg = <0x24>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&tp_reset_ds203>;
+ interrupt-parent = <&pio>;
+ interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
+ vdd-supply = <&reg_touch>;
+
+ button@0 {
+ linux,code = <KEY_HOMEPAGE>;
+ };
+
+ button@1 {
+ linux,code = <KEY_MENU>;
+ };
+
+ button@2 {
+ linux,code = <KEY_BACK>;
+ };
+ };
+ };
+...
--
2.31.1


2021-11-03 23:12:09

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

On Wed, 03 Nov 2021 21:48:28 +1000, Alistair Francis wrote:
> From: Mylène Josserand <[email protected]>
>
> Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings
> documentation. It can use I2C or SPI bus.
> This touchscreen can handle some defined zone that are designed and
> sent as button. To be able to customize the keycode sent, the
> "linux,code" property in a "button" sub-node can be used.
>
> Signed-off-by: Mylène Josserand <[email protected]>
> Message-Id: <[email protected]>
> Signed-off-by: Alistair Francis <[email protected]>
> ---
> .../input/touchscreen/cypress,tt21000.yaml | 92 +++++++++++++++++++
> 1 file changed, 92 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.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:
./Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml: $id: relative path/filename doesn't match actual path or filename
expected: http://devicetree.org/schemas/input/touchscreen/cypress,tt21000.yaml#
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dts:37.26-39.19: Warning (unit_address_vs_reg): /example-0/i2c/touchscreen@24/button@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dts:41.26-43.19: Warning (unit_address_vs_reg): /example-0/i2c/touchscreen@24/button@1: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dts:45.26-47.19: Warning (unit_address_vs_reg): /example-0/i2c/touchscreen@24/button@2: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.example.dt.yaml:0:0: /example-0/i2c/touchscreen@24: failed to match any schema with compatible: ['cypress,tt2100']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1550218

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.

2021-11-05 14:25:28

by Andreas Kemnade

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

Hi,

I finally found time to test this.

On Wed, 3 Nov 2021 21:48:28 +1000
Alistair Francis <[email protected]> wrote:

[...]
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + touchscreen@24 {
> + compatible = "cypress,tt2100";
> + reg = <0x24>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&tp_reset_ds203>;
> + interrupt-parent = <&pio>;
> + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
hmm, in the code is IRQ_TRIGGER_FALLING but here is LEVEL_LOW, hmm what
it is really?

> + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;

hmm, if the reset gpio at the chip is active low (I guess it is) that
would indicate an inverter between SoC and gpio. So a nonstandard setup
as an example, probably not a good idea.

Regards,
Andreas

2021-11-10 14:28:08

by Alistair Francis

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

On Sat, Nov 6, 2021 at 12:22 AM Andreas Kemnade <[email protected]> wrote:
>
> Hi,
>
> I finally found time to test this.
>
> On Wed, 3 Nov 2021 21:48:28 +1000
> Alistair Francis <[email protected]> wrote:
>
> [...]
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + touchscreen@24 {
> > + compatible = "cypress,tt2100";
> > + reg = <0x24>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&tp_reset_ds203>;
> > + interrupt-parent = <&pio>;
> > + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
> hmm, in the code is IRQ_TRIGGER_FALLING but here is LEVEL_LOW, hmm what
> it is really?

The reMarkable uses IRQ_TYPE_EDGE_FALLING, but this example isn't
based on that. It' based on the original documentation from the patch
series.

>
> > + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
>
> hmm, if the reset gpio at the chip is active low (I guess it is) that
> would indicate an inverter between SoC and gpio. So a nonstandard setup
> as an example, probably not a good idea.

It seems to be common for the cypress,tt2100, as the original
documentation and the rM2 both do this. Does the Kobo not do this?

Alistair

>
> Regards,
> Andreas

2021-11-10 17:37:13

by Andreas Kemnade

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

On Wed, 10 Nov 2021 22:59:50 +1000
Alistair Francis <[email protected]> wrote:

[...]
> >
> > > + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>;
> >
> > hmm, if the reset gpio at the chip is active low (I guess it is) that
> > would indicate an inverter between SoC and gpio. So a nonstandard setup
> > as an example, probably not a good idea.
>
> It seems to be common for the cypress,tt2100, as the original
> documentation and the rM2 both do this. Does the Kobo not do this?
>

You have a kind of double inversion here, so things are automagically fixed.
IMHO to describe it correctly would be to set GPIO_ACTIVE_LOW here
and in the driver

/* Reset the gpio to be in a reset state */
ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(ts->reset_gpio)) {
rc = PTR_ERR(ts->reset_gpio);
dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
return rc;
}
gpiod_set_value(ts->reset_gpio, 0);

That is the way how other active-low reset lines are handled.

Regards,
Andreas

2021-11-11 15:16:56

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

On Wed, Nov 10, 2021 at 6:37 PM Andreas Kemnade <[email protected]> wrote:
> Alistair Francis <[email protected]> wrote:

> You have a kind of double inversion here, so things are automagically fixed.
> IMHO to describe it correctly would be to set GPIO_ACTIVE_LOW here
> and in the driver
>
> /* Reset the gpio to be in a reset state */
> ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(ts->reset_gpio)) {
> rc = PTR_ERR(ts->reset_gpio);
> dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> return rc;
> }
> gpiod_set_value(ts->reset_gpio, 0);
>
> That is the way how other active-low reset lines are handled.

Correct.

This is a source of confusion, I contemplated just changing the name
of GPIOD_OUT_HIGH to GPIOD_OUT_ASSERTED etc to indicate
what is going on.

gpiod_set_value(ts->reset_gpio, 0) should similarly be interpreted
as "de-assert this line" no matter the polarity.

Yours,
Linus Walleij

2021-11-18 12:56:15

by Alistair Francis

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] Documentation: DT: bindings: input: Add documentation for cyttsp5

On Fri, Nov 12, 2021 at 1:16 AM Linus Walleij <[email protected]> wrote:
>
> On Wed, Nov 10, 2021 at 6:37 PM Andreas Kemnade <[email protected]> wrote:
> > Alistair Francis <[email protected]> wrote:
>
> > You have a kind of double inversion here, so things are automagically fixed.
> > IMHO to describe it correctly would be to set GPIO_ACTIVE_LOW here
> > and in the driver
> >
> > /* Reset the gpio to be in a reset state */
> > ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > if (IS_ERR(ts->reset_gpio)) {
> > rc = PTR_ERR(ts->reset_gpio);
> > dev_err(dev, "Failed to request reset gpio, error %d\n", rc);
> > return rc;
> > }
> > gpiod_set_value(ts->reset_gpio, 0);
> >
> > That is the way how other active-low reset lines are handled.
>
> Correct.
>
> This is a source of confusion, I contemplated just changing the name
> of GPIOD_OUT_HIGH to GPIOD_OUT_ASSERTED etc to indicate
> what is going on.
>
> gpiod_set_value(ts->reset_gpio, 0) should similarly be interpreted
> as "de-assert this line" no matter the polarity.

Thanks! I have fixed this

Alistair

>
> Yours,
> Linus Walleij