Re: [PATCH] dt-bindings: input: Goodix SPI HID Touchscreen

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/10/2024 04:08, Charles Wang wrote:
> The Goodix GT7986U touch controller report touch data according to the
> HID protocol through the SPI bus. However, it is incompatible with
> Microsoft's HID-over-SPI protocol.
> 
> Signed-off-by: Charles Wang <charles.goodix@gmail.com>
> ---
>  .../bindings/input/goodix,gt7375p.yaml        | 68 ++++++++++++++++---
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index 358cb8275..184d9c320 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -8,27 +8,27 @@ title: Goodix GT7375P touchscreen
>  
>  maintainers:
>    - Douglas Anderson <dianders@chromium.org>
> +  - Charles Wang <charles.goodix@gmail.com>
>  
>  description:
> -  Supports the Goodix GT7375P touchscreen.
> -  This touchscreen uses the i2c-hid protocol but has some non-standard
> -  power sequencing required.
> -
> -allOf:
> -  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> +  The Goodix GT7375P and GT7986U touchscreens support both SPI and I2C interfaces.
> +  With the I2C interface, they use the i2c-hid protocol but require non-standard
> +  power sequencing. With the SPI interface, they use a custom HID protocol that
> +  is incompatible with Microsoft's HID-over-SPI protocol.
>  
>  properties:
>    compatible:
>      oneOf:
> -      - const: goodix,gt7375p
> +      - items:
> +          - const: goodix,gt7375p

That's not a necessary change. Keep old code here.

>        - items:
>            - const: goodix,gt7986u
>            - const: goodix,gt7375p
> +      - items:
> +          - const: goodix,gt7986u

Hm? This does not make much sense. Device either is or is not compatible
with gt7375p. Cannot be both.

>  
>    reg:
> -    enum:
> -      - 0x5d
> -      - 0x14
> +    maxItems: 1
>  
>    interrupts:
>      maxItems: 1
> @@ -57,6 +57,15 @@ properties:
>        This property is used to avoid the back-powering issue.
>      type: boolean
>  
> +  goodix,hid-report-addr:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The register address for retrieving HID report data.
> +      This address is related to the device firmware and may
> +      change after a firmware update.

How is this supposed to work? DTS will stay fixed, you cannot change it
just because firmware changed. User loads new firmware with different
address, but DTS will have to use old address - so broken property.

> +
> +  spi-max-frequency: true

Drop

> +
>  required:
>    - compatible
>    - reg
> @@ -64,6 +73,25 @@ required:
>    - reset-gpios
>    - vdd-supply
>  
> +allOf:
> +  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          items:
> +            - const: goodix,gt7986u
> +    then:
> +      required:
> +        - goodix,hid-report-addr
> +    else:
> +      properties:
> +        goodix,hid-report-addr: false
> +        spi-max-frequency: false

Why? GT7375P also supports SPI.

> +        reg:
> +          enum: [0x5d, 0x14]
> +
>  additionalProperties: false

This becomes now: unevaluatedProperties: false

>  
>  examples:
> @@ -87,3 +115,23 @@ examples:
>          vdd-supply = <&pp3300_ts>;
>        };
>      };
> +

Best regards,
Krzysztof