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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/10/2024 13:18, Charles Wang wrote:
> 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@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.
>>
> 
> 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

Description earlier said:
"The Goodix GT7375P and GT7986U touchscreens support both SPI and I2C
interfaces."

so both support?

> the GT7375P does not support the properties 'goodix,hid-report-addr'
> and 'spi-max-frequency. Is there any issue with writing it this way?

spi-max-frequency could stay, assuming device does not support SPI.

Best regards,
Krzysztof