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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06151891-a260-450c-b688-fff18638e627@kernel.org>
Date: Fri, 18 Oct 2024 07:59:46 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Charles Wang <charles.goodix@...il.com>, dmitry.torokhov@...il.com,
 hbarnor@...omium.org, dianders@...omium.org, conor.dooley@...rochip.com
Cc: jikos@...nel.org, bentiss@...nel.org, linux-input@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: input: Goodix SPI HID Touchscreen

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@...il.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@...omium.org>
> +  - Charles Wang <charles.goodix@...il.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