[v7,1/2] dt-bindings: input: touchscreen: add Hynitron CST816X

Add documentation for the Hynitron CST816X touchscreen bindings.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Oleh Kuzhylnyi <kuzhylol@gmail.com>
---

Changes in v7:
 - Introduce the gestures field along with its sub-fields
 - Make reset-gpio property optional
 - Extend main description
 - Remove "touchscreen" reference

Changes in v6:
 - Fix minor tweak adviced by Krzysztof:
 - Move additionalProperties field after required

Changes in v5:
 - No code changes

Changes in v4:
 - Add Conor's Dooley "Reviewed-by" tag

Changes in v3:
 - Rename filename to hynitron,cst816s.yaml
 - Update description with display details

Changes in v2:
 - Apply pin definitions and DT headers
 - Use generic name for DT node
 - Drop status field

 .../input/touchscreen/hynitron,cst816s.yaml   | 126 ++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml

On 12/09/2024 15:28, Oleh Kuzhylnyi wrote:
> Add documentation for the Hynitron CST816X touchscreen bindings.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Drop. This is significantly different binding. Like 100% rewrite.

> Signed-off-by: Oleh Kuzhylnyi <kuzhylol@gmail.com>
> ---
> 
> Changes in v7:
>  - Introduce the gestures field along with its sub-fields
>  - Make reset-gpio property optional
>  - Extend main description
>  - Remove "touchscreen" reference
> 
> Changes in v6:
>  - Fix minor tweak adviced by Krzysztof:
>  - Move additionalProperties field after required
> 
> Changes in v5:
>  - No code changes
> 
> Changes in v4:
>  - Add Conor's Dooley "Reviewed-by" tag
> 
> Changes in v3:
>  - Rename filename to hynitron,cst816s.yaml
>  - Update description with display details
> 
> Changes in v2:
>  - Apply pin definitions and DT headers
>  - Use generic name for DT node
>  - Drop status field
> 
>  .../input/touchscreen/hynitron,cst816s.yaml   | 126 ++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml b/Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml
> new file mode 100644
> index 000000000000..99ac29da7a5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml
> @@ -0,0 +1,126 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/hynitron,cst816s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hynitron CST816S Touchscreen controller
> +
> +description:
> +  The CST816S is a touchscreen controller from Hynitron, which supports gesture
> +  recognition for swipe directions, tap, and long-press actions. This binding
> +  document defines the necessary properties for integrating the CST816S with
> +  a Linux system.
> +
> +maintainers:
> +  - Oleh Kuzhylnyi <kuzhylol@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - hynitron,cst816s
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      Optional GPIO line used to reset the touchscreen controller.
> +    optional: true

??? Please test your patches before sending.

Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Best regards,
Krzysztof

Hi Oleh,

On Thu, Sep 12, 2024 at 03:28:22PM +0200, Oleh Kuzhylnyi wrote:
> Add documentation for the Hynitron CST816X touchscreen bindings.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Oleh Kuzhylnyi <kuzhylol@gmail.com>
> ---
> 
> Changes in v7:
>  - Introduce the gestures field along with its sub-fields
>  - Make reset-gpio property optional
>  - Extend main description
>  - Remove "touchscreen" reference
> 
> Changes in v6:
>  - Fix minor tweak adviced by Krzysztof:
>  - Move additionalProperties field after required
> 
> Changes in v5:
>  - No code changes
> 
> Changes in v4:
>  - Add Conor's Dooley "Reviewed-by" tag
> 
> Changes in v3:
>  - Rename filename to hynitron,cst816s.yaml
>  - Update description with display details
> 
> Changes in v2:
>  - Apply pin definitions and DT headers
>  - Use generic name for DT node
>  - Drop status field
> 
>  .../input/touchscreen/hynitron,cst816s.yaml   | 126 ++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml b/Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml
> new file mode 100644
> index 000000000000..99ac29da7a5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/hynitron,cst816s.yaml
> @@ -0,0 +1,126 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/hynitron,cst816s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hynitron CST816S Touchscreen controller
> +
> +description:
> +  The CST816S is a touchscreen controller from Hynitron, which supports gesture
> +  recognition for swipe directions, tap, and long-press actions. This binding
> +  document defines the necessary properties for integrating the CST816S with
> +  a Linux system.
> +
> +maintainers:
> +  - Oleh Kuzhylnyi <kuzhylol@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - hynitron,cst816s
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      Optional GPIO line used to reset the touchscreen controller.
> +    optional: true

Thank you for the spin, but I think there is some misunderstanding here.
You do not need to explicitly define the property as optional; I do not
think this is even a keyword.

All properties are optional unless explicitly listed under the 'required'
keyword, so all that needs to be done here is simply remove 'reset-gpios'
from under the 'required' keyword as you have done.

> +
> +  gestures:
> +    type: object
> +    description:
> +      A list of gestures supported by the CST816S touchscreen controller and
> +      their associated Linux input event codes.
> +    optional: true
> +
> +    properties:
> +      "^.*$":

Again, I think there is some misunderstanding here; I do not think this
can compile, as evidenced by the bot and others' feedback. Even if this
regex were valid, it would need to be placed under a 'patternProperties'
keyword. However, this is not really an optional approach either.

I think it can help to put yourself in a customer's perspective; it will
not be clear to many people what is the "gesture ID". This seems like an
arbitrary index that is only relevant within the driver code.

In general, there are two common ways to specify key codes for a touch
controller that can recognize gesture events:

1. By name. Each gesture is represented by a child node with a unique
   name (e.g. 'gesture-x-neg'), and your regex looks something like the
   following:

   patternProperties:
     "^gesture-(x|y)-(pos|neg)$":

   The driver then matches child nodes by name, and the customer need
   not have any sense of the order in which interrupt status bits appear
   within the register map.

   Each child node then specifies the 'linux,code' property, as well
   as any other properties relevant to the gesture.

2. A single array with length equal to the number of gestures reported
   by the device, defined as follows:

   properties:
     linux,keycodes: true

Option (1) is useful if you must support other properties associated
with each gesture such as distance threshold, timing parameters, etc.
If the HW does not expose any such properties and the key code is the
sole parameter associated with each gesture, then option (2) is sufficient.

There are many upstream bindings that use either approach; please follow
them. In either case, 'linux,code' and 'linux,keycodes' are both standard
properties defined in [1], please study this binding and reference it in
this one.

> +        type: object
> +        description:
> +          Each child node represents a gesture that the touchscreen controller
> +          can recognize.
> +
> +        properties:
> +          cst816x,gesture:
> +            description:
> +              Numeric value representing the gesture ID recognized by the
> +              CST816S touchscreen controller.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +          linux,code:
> +            description:
> +              Linux input event code (from linux/input-event-codes.h) that
> +              corresponds to the gesture.
> +            $ref: /schemas/types.yaml#/definitions/uint32

This property's description and type are already defined in [1]; they
do not need to be redefined here so long as [1] is referenced correctly.

> +
> +        required:
> +          - cst816x,gesture
> +          - linux,code
> +
> +        additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/input/linux-event-codes.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      touchscreen@15 {
> +        compatible = "hynitron,cst816s";
> +        reg = <0x15>;
> +        interrupt-parent = <&gpio0>;
> +        interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +        reset-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
> +
> +        gestures {
> +          swipe_up {
> +            cst816x,gesture = <0x1>;
> +            linux,code = <BTN_FORWARD>;
> +          };
> +
> +          swipe_down {
> +            cst816x,gesture = <0x2>;
> +            linux,code = <BTN_BACK>;
> +          };
> +
> +          swipe_left {
> +            cst816x,gesture = <0x3>;
> +            linux,code = <BTN_LEFT>;
> +          };
> +
> +          swipe_right {
> +            cst816x,gesture = <0x4>;
> +            linux,code = <BTN_RIGHT>;
> +          };
> +
> +          single_tap {
> +            cst816x,gesture = <0x5>;
> +            linux,code = <BTN_TOUCH>;
> +          };
> +
> +          long_press {
> +            cst816x,gesture = <0xC>;
> +            linux,code = <BTN_TOOL_TRIPLETAP>;
> +          };
> +        };
> +      };
> +    };
> +
> +...
> -- 
> 2.34.1
> 

[1] Documentation/devicetree/bindings/input/input.yaml

Kind regards,
Jeff LaBundy