| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZujdtKcVcwMTmmOM@nixie71>
Date: Mon, 16 Sep 2024 20:39:00 -0500
From: Jeff LaBundy <jeff@...undy.com>
To: Oleh Kuzhylnyi <kuzhylol@...il.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-input@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Conor Dooley <conor.dooley@...rochip.com>, igor.opaniuk@...il.com,
Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v7 1/2] dt-bindings: input: touchscreen: add Hynitron
CST816X
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@...rochip.com>
> Signed-off-by: Oleh Kuzhylnyi <kuzhylol@...il.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@...il.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
Powered by blists - more mailing lists