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

[Charles Wang]

Hi Krzysztof,

On Fri, Oct 18, 2024 at 07:59:46AM +0200, Krzysztof Kozlowski wrote:
> 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@xxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxx>
> > +  - Charles Wang <charles.goodix@xxxxxxxxx>
> >  
> >  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.
>

Ack,

> >        - 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.
>

Ack,

> >  
> >    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
>

Ack,

> > +
> >  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.
>

No, only GT7986U support SPI. What I'm trying to express here is that
the GT7375P does not support the properties 'goodix,hid-report-addr'
and 'spi-max-frequency. Is there any issue with writing it this way?

> > +        reg:
> > +          enum: [0x5d, 0x14]
> > +
> >  additionalProperties: false
> 
> This becomes now: unevaluatedProperties: false
>

Ack,

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

Best regards,
Charles