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

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

[Charles Wang]

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
       - items:
           - const: goodix,gt7986u
           - const: goodix,gt7375p
+      - items:
+          - const: goodix,gt7986u
 
   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.
+
+  spi-max-frequency: true
+
 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
+        reg:
+          enum: [0x5d, 0x14]
+
 additionalProperties: false
 
 examples:
@@ -87,3 +115,23 @@ examples:
         vdd-supply = <&pp3300_ts>;
       };
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@0 {
+        compatible = "goodix,gt7986u";
+        reg = <0>;
+        interrupt-parent = <&gpio>;
+        interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+        reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+        spi-max-frequency = <10000000>;
+        goodix,hid-report-addr = <0x22c8c>;
+        vdd-supply = <&pp3300_ts>;
+      };
+    };
-- 
2.43.0

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

[Krzysztof Kozlowski]

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

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

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

[Krzysztof Kozlowski]

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

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

[Doug Anderson]

Hi,

On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)

I'm happy to let device tree folks make the call here, but IMO it
would be much cleaner to just consider the I2C-connected GT7986U and
the SPI-connected GT7986U to be different things and just use a
different compatible string for them. So essentially go back to your
v7 patch from before [1] but change the compatible to
"goodix,gt7986u-spi". If, for instance, this device also had a USB
interface then I don't think we'd try to cram it into the same
bindings even though the same physical chip was present...

-Doug

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

[Charles Wang]

Hi Krzysztof,

On Fri, Oct 18, 2024 at 01:41:41PM +0200, Krzysztof Kozlowski wrote:
> On 18/10/2024 13:18, Charles Wang wrote:
> > 
> > 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.
>

Sorry for missing this issue in my previous response.
Honestly, although the likelihood of this address changing is low, it is
indeed possible for it to change due to a firmware update during the factory
debugging phase. However, for machines that users have, we will ensure that
this address will not be altered as a result of a firmware upgrade.

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

Sorry, there is an error in the description. Currently, only the GT7986U
supports SPI, I will change the description.

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

Ack,

Best regards,
Charles

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

[Charles Wang]

Hi Doug

On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> 
> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> 
> I'm happy to let device tree folks make the call here, but IMO it
> would be much cleaner to just consider the I2C-connected GT7986U and
> the SPI-connected GT7986U to be different things and just use a
> different compatible string for them. So essentially go back to your
> v7 patch from before [1] but change the compatible to
> "goodix,gt7986u-spi". If, for instance, this device also had a USB
> interface then I don't think we'd try to cram it into the same
> bindings even though the same physical chip was present...
> 

Honestly, I agree with this approach, but Krzysztof seems to prefer
extending the existing binding.

Best regards,
Charles

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

[Krzysztof Kozlowski]

On 19/10/2024 04:46, Charles Wang wrote:
>>>>> +  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.
>>
> 
> Sorry for missing this issue in my previous response.
> Honestly, although the likelihood of this address changing is low, it is
> indeed possible for it to change due to a firmware update during the factory
> debugging phase. However, for machines that users have, we will ensure that
> this address will not be altered as a result of a firmware upgrade.

Then it feels like property is not really needed - all boards will have
exactly same value.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 19/10/2024 04:55, Charles Wang wrote:
> Hi Doug
> 
> On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
>>
>> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
>>
>> I'm happy to let device tree folks make the call here, but IMO it
>> would be much cleaner to just consider the I2C-connected GT7986U and
>> the SPI-connected GT7986U to be different things and just use a

Same device, you cannot have different compatibles. The way how the same
(literally same chip) device sits on the bus is not part of the binding,
thus no different compatibles.

>> different compatible string for them. So essentially go back to your
>> v7 patch from before [1] but change the compatible to
>> "goodix,gt7986u-spi". If, for instance, this device also had a USB
>> interface then I don't think we'd try to cram it into the same
>> bindings even though the same physical chip was present...
>>
> 
> Honestly, I agree with this approach, but Krzysztof seems to prefer
> extending the existing binding.

I prefer not to have warnings and that was the problem with original
patchset. I am fine with splitting different models between different
binding schemas/files, but not the same device in two files.

Best regards,
Krzysztof

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

[Doug Anderson]

Hi,

On Mon, Oct 21, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/10/2024 04:55, Charles Wang wrote:
> > Hi Doug
> >
> > On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> >>
> >> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> >>
> >> I'm happy to let device tree folks make the call here, but IMO it
> >> would be much cleaner to just consider the I2C-connected GT7986U and
> >> the SPI-connected GT7986U to be different things and just use a
>
> Same device, you cannot have different compatibles. The way how the same
> (literally same chip) device sits on the bus is not part of the binding,
> thus no different compatibles.

I don't want to belabour the point too much, but this doesn't feel
completely black and white here.

"Same chip": a whole lot of laptops and phones all use the "same chip"
(same SoC) yet are different products. ...or you can look at the fact
that many peripherals have the same STM32 or Nuvoton chip in them but
are wildly different peripherals.

In this case, Goodix may have made an ASIC called "GT7986U" that has
some type of CPU on it that can run firmware that can talk as an I2C
device or a SPI device. This ASIC may be intended to be used as a
touchscreen controller, but fundamentally it doesn't feel that
different from an STM32. You can build different boards designs with
the "GT7986U" on it and those boards are intended to run different
firmware.

People manufacturing touch controller boards presumably put this
"GT7986U" on their touch controller board, maybe set certain
strappings telling it that it's talking over SPI or I2C or maybe just
decide which pins they're going to wire out to the board-to-board
connector on the touch controller board. A touch controller board
intended to talk over SPI may look 98% the same as a touch controller
board intended to talk over I2C, but what percentage of "sameness"
means that we need the same compatible string?

Would things be different if Goodix decided to manufacture touch
controller boards themselves and sold two SKUs: a GT7986U-S and a
GT7986U-I?

I would also note that (reading back in previous conversations) I
think Charles said that they run different firmware on the SPI vs. I2C
touch controllers. As I understand it, the firmware running on a
device can make it a different device from a device tree perspective.
The device tree does its best to describe just the hardware but it can
get fuzzy. For instance the "VID/PID" of a USB device is usually
something programmable and could be updateable by a firmware change
but we still may need to encode the VID/PID of the firmware that is
intended to run on the device in the device tree.

Anyway, I'm happy to be quiet about this and fine if folks want to
continue to work towards a "unified" binding. It makes me a little
uncomfortable that I'll still end up listed as a "maintainer" of the
unified binding because I don't totally agree with it, but I'm also
pragmatic and I'd rather have something that can land.

-Doug

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

[Charles Wang]

Hi Doug,

On Mon, Oct 21, 2024 at 08:37:32AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Oct 21, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 19/10/2024 04:55, Charles Wang wrote:
> > > Hi Doug
> > >
> > > On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> > >>
> > >> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> > >>
> > >> I'm happy to let device tree folks make the call here, but IMO it
> > >> would be much cleaner to just consider the I2C-connected GT7986U and
> > >> the SPI-connected GT7986U to be different things and just use a
> >
> > Same device, you cannot have different compatibles. The way how the same
> > (literally same chip) device sits on the bus is not part of the binding,
> > thus no different compatibles.
> 
> I don't want to belabour the point too much, but this doesn't feel
> completely black and white here.
> 
> "Same chip": a whole lot of laptops and phones all use the "same chip"
> (same SoC) yet are different products. ...or you can look at the fact
> that many peripherals have the same STM32 or Nuvoton chip in them but
> are wildly different peripherals.
> 
> In this case, Goodix may have made an ASIC called "GT7986U" that has
> some type of CPU on it that can run firmware that can talk as an I2C
> device or a SPI device. This ASIC may be intended to be used as a
> touchscreen controller, but fundamentally it doesn't feel that
> different from an STM32. You can build different boards designs with
> the "GT7986U" on it and those boards are intended to run different
> firmware.
> 
> People manufacturing touch controller boards presumably put this
> "GT7986U" on their touch controller board, maybe set certain
> strappings telling it that it's talking over SPI or I2C or maybe just
> decide which pins they're going to wire out to the board-to-board
> connector on the touch controller board. A touch controller board
> intended to talk over SPI may look 98% the same as a touch controller
> board intended to talk over I2C, but what percentage of "sameness"
> means that we need the same compatible string?
> 
> Would things be different if Goodix decided to manufacture touch
> controller boards themselves and sold two SKUs: a GT7986U-S and a
> GT7986U-I?
> 
> I would also note that (reading back in previous conversations) I
> think Charles said that they run different firmware on the SPI vs. I2C
> touch controllers. As I understand it, the firmware running on a
> device can make it a different device from a device tree perspective.
> The device tree does its best to describe just the hardware but it can
> get fuzzy. For instance the "VID/PID" of a USB device is usually
> something programmable and could be updateable by a firmware change
> but we still may need to encode the VID/PID of the firmware that is
> intended to run on the device in the device tree.
> 
> Anyway, I'm happy to be quiet about this and fine if folks want to
> continue to work towards a "unified" binding. It makes me a little
> uncomfortable that I'll still end up listed as a "maintainer" of the
> unified binding because I don't totally agree with it, but I'm also
> pragmatic and I'd rather have something that can land.
> 

Thank you very much for your attention. Your understanding of the GT7986U
SPI and I2C devices is correct. There is no fundamental difference between
them and the STM32, as they are all ASIC devices. The functionality of the
device is determined by the firmware that is loaded, although the GT7986U
is an ASIC specifically designed for touchscreens.

Additionally, the firmware and devices are generally bound to specific touch
panels, meaning that firmware intended for SPI will not function properly on
an I2C touch panel.

Best regards,
Charles

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

[Doug Anderson]

Hi,

On Tue, Oct 22, 2024 at 12:19 AM Charles Wang <charles.goodix@gmail.com> wrote:
>
> Hi Doug,
>
> On Mon, Oct 21, 2024 at 08:37:32AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Oct 21, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On 19/10/2024 04:55, Charles Wang wrote:
> > > > Hi Doug
> > > >
> > > > On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> > > >>
> > > >> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> > > >>
> > > >> I'm happy to let device tree folks make the call here, but IMO it
> > > >> would be much cleaner to just consider the I2C-connected GT7986U and
> > > >> the SPI-connected GT7986U to be different things and just use a
> > >
> > > Same device, you cannot have different compatibles. The way how the same
> > > (literally same chip) device sits on the bus is not part of the binding,
> > > thus no different compatibles.
> >
> > I don't want to belabour the point too much, but this doesn't feel
> > completely black and white here.
> >
> > "Same chip": a whole lot of laptops and phones all use the "same chip"
> > (same SoC) yet are different products. ...or you can look at the fact
> > that many peripherals have the same STM32 or Nuvoton chip in them but
> > are wildly different peripherals.
> >
> > In this case, Goodix may have made an ASIC called "GT7986U" that has
> > some type of CPU on it that can run firmware that can talk as an I2C
> > device or a SPI device. This ASIC may be intended to be used as a
> > touchscreen controller, but fundamentally it doesn't feel that
> > different from an STM32. You can build different boards designs with
> > the "GT7986U" on it and those boards are intended to run different
> > firmware.
> >
> > People manufacturing touch controller boards presumably put this
> > "GT7986U" on their touch controller board, maybe set certain
> > strappings telling it that it's talking over SPI or I2C or maybe just
> > decide which pins they're going to wire out to the board-to-board
> > connector on the touch controller board. A touch controller board
> > intended to talk over SPI may look 98% the same as a touch controller
> > board intended to talk over I2C, but what percentage of "sameness"
> > means that we need the same compatible string?
> >
> > Would things be different if Goodix decided to manufacture touch
> > controller boards themselves and sold two SKUs: a GT7986U-S and a
> > GT7986U-I?
> >
> > I would also note that (reading back in previous conversations) I
> > think Charles said that they run different firmware on the SPI vs. I2C
> > touch controllers. As I understand it, the firmware running on a
> > device can make it a different device from a device tree perspective.
> > The device tree does its best to describe just the hardware but it can
> > get fuzzy. For instance the "VID/PID" of a USB device is usually
> > something programmable and could be updateable by a firmware change
> > but we still may need to encode the VID/PID of the firmware that is
> > intended to run on the device in the device tree.
> >
> > Anyway, I'm happy to be quiet about this and fine if folks want to
> > continue to work towards a "unified" binding. It makes me a little
> > uncomfortable that I'll still end up listed as a "maintainer" of the
> > unified binding because I don't totally agree with it, but I'm also
> > pragmatic and I'd rather have something that can land.
> >
>
> Thank you very much for your attention. Your understanding of the GT7986U
> SPI and I2C devices is correct. There is no fundamental difference between
> them and the STM32, as they are all ASIC devices. The functionality of the
> device is determined by the firmware that is loaded, although the GT7986U
> is an ASIC specifically designed for touchscreens.
>
> Additionally, the firmware and devices are generally bound to specific touch
> panels, meaning that firmware intended for SPI will not function properly on
> an I2C touch panel.

Just to get clarity: how is GT7986U delivered? For instance:

1. Maybe Goodix produces touchscreen controller boards and ships them
to customers for use in their products. In this case, does Goodix ship
a single board with two connectors, or a separate board for SPI vs.
I2C? I would have to believe that maybe a "dev" board might have both
connectors and a bunch of jumpers/switches to choose which ones to
use, but it feels unlikely someone would ship that in any quantity.

2. Maybe Goodix provides schematics for customers to produce their own
touchscreen controller boards and they tell customers to either hook
up the SPI lines and load the SPI firmware or hook up the I2C lines
and load the I2C firmware. In this case the assumption is that
customers using the same communication method are following the
schematics closely enough that they all behave the same and thus we
don't need some extra distinction.

In either case it seems like a touchscreen controller board that talks
over SPI and one that talks over I2C are two different products and
thus (to me) should have two distinct compatible strings. This is not
one device that merely has multiple interfaces.


-Doug

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

[Charles Wang]

Hi,

On Tue, Oct 22, 2024 at 09:12:33AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 22, 2024 at 12:19 AM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > Hi Doug,
> >
> > On Mon, Oct 21, 2024 at 08:37:32AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Oct 21, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > On 19/10/2024 04:55, Charles Wang wrote:
> > > > > Hi Doug
> > > > >
> > > > > On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> > > > >>
> > > > >> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> > > > >>
> > > > >> I'm happy to let device tree folks make the call here, but IMO it
> > > > >> would be much cleaner to just consider the I2C-connected GT7986U and
> > > > >> the SPI-connected GT7986U to be different things and just use a
> > > >
> > > > Same device, you cannot have different compatibles. The way how the same
> > > > (literally same chip) device sits on the bus is not part of the binding,
> > > > thus no different compatibles.
> > >
> > > I don't want to belabour the point too much, but this doesn't feel
> > > completely black and white here.
> > >
> > > "Same chip": a whole lot of laptops and phones all use the "same chip"
> > > (same SoC) yet are different products. ...or you can look at the fact
> > > that many peripherals have the same STM32 or Nuvoton chip in them but
> > > are wildly different peripherals.
> > >
> > > In this case, Goodix may have made an ASIC called "GT7986U" that has
> > > some type of CPU on it that can run firmware that can talk as an I2C
> > > device or a SPI device. This ASIC may be intended to be used as a
> > > touchscreen controller, but fundamentally it doesn't feel that
> > > different from an STM32. You can build different boards designs with
> > > the "GT7986U" on it and those boards are intended to run different
> > > firmware.
> > >
> > > People manufacturing touch controller boards presumably put this
> > > "GT7986U" on their touch controller board, maybe set certain
> > > strappings telling it that it's talking over SPI or I2C or maybe just
> > > decide which pins they're going to wire out to the board-to-board
> > > connector on the touch controller board. A touch controller board
> > > intended to talk over SPI may look 98% the same as a touch controller
> > > board intended to talk over I2C, but what percentage of "sameness"
> > > means that we need the same compatible string?
> > >
> > > Would things be different if Goodix decided to manufacture touch
> > > controller boards themselves and sold two SKUs: a GT7986U-S and a
> > > GT7986U-I?
> > >
> > > I would also note that (reading back in previous conversations) I
> > > think Charles said that they run different firmware on the SPI vs. I2C
> > > touch controllers. As I understand it, the firmware running on a
> > > device can make it a different device from a device tree perspective.
> > > The device tree does its best to describe just the hardware but it can
> > > get fuzzy. For instance the "VID/PID" of a USB device is usually
> > > something programmable and could be updateable by a firmware change
> > > but we still may need to encode the VID/PID of the firmware that is
> > > intended to run on the device in the device tree.
> > >
> > > Anyway, I'm happy to be quiet about this and fine if folks want to
> > > continue to work towards a "unified" binding. It makes me a little
> > > uncomfortable that I'll still end up listed as a "maintainer" of the
> > > unified binding because I don't totally agree with it, but I'm also
> > > pragmatic and I'd rather have something that can land.
> > >
> >
> > Thank you very much for your attention. Your understanding of the GT7986U
> > SPI and I2C devices is correct. There is no fundamental difference between
> > them and the STM32, as they are all ASIC devices. The functionality of the
> > device is determined by the firmware that is loaded, although the GT7986U
> > is an ASIC specifically designed for touchscreens.
> >
> > Additionally, the firmware and devices are generally bound to specific touch
> > panels, meaning that firmware intended for SPI will not function properly on
> > an I2C touch panel.
> 
> Just to get clarity: how is GT7986U delivered? For instance:
> 
> 1. Maybe Goodix produces touchscreen controller boards and ships them
> to customers for use in their products. In this case, does Goodix ship
> a single board with two connectors, or a separate board for SPI vs.
> I2C? I would have to believe that maybe a "dev" board might have both
> connectors and a bunch of jumpers/switches to choose which ones to
> use, but it feels unlikely someone would ship that in any quantity.
> 
> 2. Maybe Goodix provides schematics for customers to produce their own
> touchscreen controller boards and they tell customers to either hook
> up the SPI lines and load the SPI firmware or hook up the I2C lines
> and load the I2C firmware. In this case the assumption is that
> customers using the same communication method are following the
> schematics closely enough that they all behave the same and thus we
> don't need some extra distinction.
> 
> In either case it seems like a touchscreen controller board that talks
> over SPI and one that talks over I2C are two different products and
> thus (to me) should have two distinct compatible strings. This is not
> one device that merely has multiple interfaces.
> 

Goodix's approach is similar to Method 2. First, Goodix provides the
schematics and the chips (including initial firmware, no touch function)
to customers, and customers design their touchscreen controller boards and
decide whether to use the I2C or SPI interface. Then, Goodix modifies and
debugs the firmware based on the customer's design and provides the final
firmware for customers to upgrade.

It is important to note that the type of driver used by the final device
is related not only to the bus type but also to the final firmware. Even
when using the same I2C bus, different drivers may be needed, such as
hid-i2c or a customer-specific driver.

Best regards,
Charles

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

[Doug Anderson]

Hi,

On Tue, Oct 22, 2024 at 11:44 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> Hi,
>
> On Tue, Oct 22, 2024 at 09:12:33AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Oct 22, 2024 at 12:19 AM Charles Wang <charles.goodix@gmail.com> wrote:
> > >
> > > Hi Doug,
> > >
> > > On Mon, Oct 21, 2024 at 08:37:32AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Mon, Oct 21, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >
> > > > > On 19/10/2024 04:55, Charles Wang wrote:
> > > > > > Hi Doug
> > > > > >
> > > > > > On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> > > > > >>
> > > > > >> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> > > > > >>
> > > > > >> I'm happy to let device tree folks make the call here, but IMO it
> > > > > >> would be much cleaner to just consider the I2C-connected GT7986U and
> > > > > >> the SPI-connected GT7986U to be different things and just use a
> > > > >
> > > > > Same device, you cannot have different compatibles. The way how the same
> > > > > (literally same chip) device sits on the bus is not part of the binding,
> > > > > thus no different compatibles.
> > > >
> > > > I don't want to belabour the point too much, but this doesn't feel
> > > > completely black and white here.
> > > >
> > > > "Same chip": a whole lot of laptops and phones all use the "same chip"
> > > > (same SoC) yet are different products. ...or you can look at the fact
> > > > that many peripherals have the same STM32 or Nuvoton chip in them but
> > > > are wildly different peripherals.
> > > >
> > > > In this case, Goodix may have made an ASIC called "GT7986U" that has
> > > > some type of CPU on it that can run firmware that can talk as an I2C
> > > > device or a SPI device. This ASIC may be intended to be used as a
> > > > touchscreen controller, but fundamentally it doesn't feel that
> > > > different from an STM32. You can build different boards designs with
> > > > the "GT7986U" on it and those boards are intended to run different
> > > > firmware.
> > > >
> > > > People manufacturing touch controller boards presumably put this
> > > > "GT7986U" on their touch controller board, maybe set certain
> > > > strappings telling it that it's talking over SPI or I2C or maybe just
> > > > decide which pins they're going to wire out to the board-to-board
> > > > connector on the touch controller board. A touch controller board
> > > > intended to talk over SPI may look 98% the same as a touch controller
> > > > board intended to talk over I2C, but what percentage of "sameness"
> > > > means that we need the same compatible string?
> > > >
> > > > Would things be different if Goodix decided to manufacture touch
> > > > controller boards themselves and sold two SKUs: a GT7986U-S and a
> > > > GT7986U-I?
> > > >
> > > > I would also note that (reading back in previous conversations) I
> > > > think Charles said that they run different firmware on the SPI vs. I2C
> > > > touch controllers. As I understand it, the firmware running on a
> > > > device can make it a different device from a device tree perspective.
> > > > The device tree does its best to describe just the hardware but it can
> > > > get fuzzy. For instance the "VID/PID" of a USB device is usually
> > > > something programmable and could be updateable by a firmware change
> > > > but we still may need to encode the VID/PID of the firmware that is
> > > > intended to run on the device in the device tree.
> > > >
> > > > Anyway, I'm happy to be quiet about this and fine if folks want to
> > > > continue to work towards a "unified" binding. It makes me a little
> > > > uncomfortable that I'll still end up listed as a "maintainer" of the
> > > > unified binding because I don't totally agree with it, but I'm also
> > > > pragmatic and I'd rather have something that can land.
> > > >
> > >
> > > Thank you very much for your attention. Your understanding of the GT7986U
> > > SPI and I2C devices is correct. There is no fundamental difference between
> > > them and the STM32, as they are all ASIC devices. The functionality of the
> > > device is determined by the firmware that is loaded, although the GT7986U
> > > is an ASIC specifically designed for touchscreens.
> > >
> > > Additionally, the firmware and devices are generally bound to specific touch
> > > panels, meaning that firmware intended for SPI will not function properly on
> > > an I2C touch panel.
> >
> > Just to get clarity: how is GT7986U delivered? For instance:
> >
> > 1. Maybe Goodix produces touchscreen controller boards and ships them
> > to customers for use in their products. In this case, does Goodix ship
> > a single board with two connectors, or a separate board for SPI vs.
> > I2C? I would have to believe that maybe a "dev" board might have both
> > connectors and a bunch of jumpers/switches to choose which ones to
> > use, but it feels unlikely someone would ship that in any quantity.
> >
> > 2. Maybe Goodix provides schematics for customers to produce their own
> > touchscreen controller boards and they tell customers to either hook
> > up the SPI lines and load the SPI firmware or hook up the I2C lines
> > and load the I2C firmware. In this case the assumption is that
> > customers using the same communication method are following the
> > schematics closely enough that they all behave the same and thus we
> > don't need some extra distinction.
> >
> > In either case it seems like a touchscreen controller board that talks
> > over SPI and one that talks over I2C are two different products and
> > thus (to me) should have two distinct compatible strings. This is not
> > one device that merely has multiple interfaces.
> >
>
> Goodix's approach is similar to Method 2. First, Goodix provides the
> schematics and the chips (including initial firmware, no touch function)
> to customers, and customers design their touchscreen controller boards and
> decide whether to use the I2C or SPI interface. Then, Goodix modifies and
> debugs the firmware based on the customer's design and provides the final
> firmware for customers to upgrade.

OK, thanks!

From the above that means that if someone uses the "goodix,gt7986u"
compatible today (with what's landed in mainline) then by that they
mean "This is a touchscreen that's compatible with a Goodix-defined
standard way of talking to i2c-based touchscreens built atop a GT7986U
touchscreen controller". With what's landed in mainline that "standard
way" is the "i2c-hid" protocol plus a reset line (which IIRC is not
part of the i2c-hid standard) plus a defined power up/power down
sequence.

I suppose one conclusion one might make is that we never should have
used "goodix,gt7986u" as a compatible string in the first place and
should have instead added a new compatible string for every actual
instantiation of a touchscreen. So when Vendor1 made touchscreen 1234
based on GT7986U then we could have used the compatible
"vendor1,touchscreen1234" and then when Vendor2 made touchscreen 5678
based on GT7986U we could have used the compatible
"vendor2,touchscreen5678". Should we have done this / should we do it
in the future? I don't know. If everyone using GT7986U is adhering to
the same interface then it doesn't buy us a ton and adds lots more
bindings. I think I ended up originally adding the Goodix GT7375P
bindings because someone gave me a datasheet with all the power
sequencing and timings that came from Goodix and said it was for the
"Goodix GT7375P". Given the fact that Goodix provides such a datasheet
and it includes power sequencing is a strong indicator that there
truly is a standard and we can use that.

In any case, if we _had_ used a different compatible for each actual
touchscreen implementation then we wouldn't be having this discussion.
Those touchscreens that shipped with a controller board that had SPI
connections and SPI firmware would have had obviously different
compatible strings than the touchscreens that shipped with a
controller board designed for I2C.

If we _do_ want to keep using a compatible like "goodix,gt7986u" then,
IMO, it's beneficial to also have a SPI-variant compatible like
"goodix,gt7986u-spi". This is not a second interface to one device but
it's actually a distinct interface compared to the Goodix I2C
interface. Note: this assumes there isn't some hidden benefit to
having a combined "I2C/SPI" bindings file. I find having the combined
file buys me nothing and just makes it more confusing / adds
complexity. Is there some benefit I'm missing other than towing the
line of "one chip, one compatible"?


> It is important to note that the type of driver used by the final device
> is related not only to the bus type but also to the final firmware. Even
> when using the same I2C bus, different drivers may be needed, such as
> hid-i2c or a customer-specific driver.

Right. ...the firmware that's on the device matters and distinct
firmware can make a distinct device, and IMO a GT7986U loaded with I2C
firmware is a distinct device than a GT7986U loaded with SPI firmware.
They are not the same and thus don't need the same compatible.


-Doug

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

[Charles Wang]

Hi,

On Wed, Oct 23, 2024 at 12:35:09PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 22, 2024 at 11:44 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Oct 22, 2024 at 09:12:33AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Oct 22, 2024 at 12:19 AM Charles Wang <charles.goodix@gmail.com> wrote:
> > > >
> > > > Hi Doug,
> > > >
> > > > On Mon, Oct 21, 2024 at 08:37:32AM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Oct 21, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > > >
> > > > > > On 19/10/2024 04:55, Charles Wang wrote:
> > > > > > > Hi Doug
> > > > > > >
> > > > > > > On Fri, Oct 18, 2024 at 01:48:56PM -0700, Doug Anderson wrote:
> > > > > > >>
> > > > > > >> On Thu, Oct 17, 2024 at 7:09 PM Charles Wang <charles.goodix@gmail.com> 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(-)
> > > > > > >>
> > > > > > >> I'm happy to let device tree folks make the call here, but IMO it
> > > > > > >> would be much cleaner to just consider the I2C-connected GT7986U and
> > > > > > >> the SPI-connected GT7986U to be different things and just use a
> > > > > >
> > > > > > Same device, you cannot have different compatibles. The way how the same
> > > > > > (literally same chip) device sits on the bus is not part of the binding,
> > > > > > thus no different compatibles.
> > > > >
> > > > > I don't want to belabour the point too much, but this doesn't feel
> > > > > completely black and white here.
> > > > >
> > > > > "Same chip": a whole lot of laptops and phones all use the "same chip"
> > > > > (same SoC) yet are different products. ...or you can look at the fact
> > > > > that many peripherals have the same STM32 or Nuvoton chip in them but
> > > > > are wildly different peripherals.
> > > > >
> > > > > In this case, Goodix may have made an ASIC called "GT7986U" that has
> > > > > some type of CPU on it that can run firmware that can talk as an I2C
> > > > > device or a SPI device. This ASIC may be intended to be used as a
> > > > > touchscreen controller, but fundamentally it doesn't feel that
> > > > > different from an STM32. You can build different boards designs with
> > > > > the "GT7986U" on it and those boards are intended to run different
> > > > > firmware.
> > > > >
> > > > > People manufacturing touch controller boards presumably put this
> > > > > "GT7986U" on their touch controller board, maybe set certain
> > > > > strappings telling it that it's talking over SPI or I2C or maybe just
> > > > > decide which pins they're going to wire out to the board-to-board
> > > > > connector on the touch controller board. A touch controller board
> > > > > intended to talk over SPI may look 98% the same as a touch controller
> > > > > board intended to talk over I2C, but what percentage of "sameness"
> > > > > means that we need the same compatible string?
> > > > >
> > > > > Would things be different if Goodix decided to manufacture touch
> > > > > controller boards themselves and sold two SKUs: a GT7986U-S and a
> > > > > GT7986U-I?
> > > > >
> > > > > I would also note that (reading back in previous conversations) I
> > > > > think Charles said that they run different firmware on the SPI vs. I2C
> > > > > touch controllers. As I understand it, the firmware running on a
> > > > > device can make it a different device from a device tree perspective.
> > > > > The device tree does its best to describe just the hardware but it can
> > > > > get fuzzy. For instance the "VID/PID" of a USB device is usually
> > > > > something programmable and could be updateable by a firmware change
> > > > > but we still may need to encode the VID/PID of the firmware that is
> > > > > intended to run on the device in the device tree.
> > > > >
> > > > > Anyway, I'm happy to be quiet about this and fine if folks want to
> > > > > continue to work towards a "unified" binding. It makes me a little
> > > > > uncomfortable that I'll still end up listed as a "maintainer" of the
> > > > > unified binding because I don't totally agree with it, but I'm also
> > > > > pragmatic and I'd rather have something that can land.
> > > > >
> > > >
> > > > Thank you very much for your attention. Your understanding of the GT7986U
> > > > SPI and I2C devices is correct. There is no fundamental difference between
> > > > them and the STM32, as they are all ASIC devices. The functionality of the
> > > > device is determined by the firmware that is loaded, although the GT7986U
> > > > is an ASIC specifically designed for touchscreens.
> > > >
> > > > Additionally, the firmware and devices are generally bound to specific touch
> > > > panels, meaning that firmware intended for SPI will not function properly on
> > > > an I2C touch panel.
> > >
> > > Just to get clarity: how is GT7986U delivered? For instance:
> > >
> > > 1. Maybe Goodix produces touchscreen controller boards and ships them
> > > to customers for use in their products. In this case, does Goodix ship
> > > a single board with two connectors, or a separate board for SPI vs.
> > > I2C? I would have to believe that maybe a "dev" board might have both
> > > connectors and a bunch of jumpers/switches to choose which ones to
> > > use, but it feels unlikely someone would ship that in any quantity.
> > >
> > > 2. Maybe Goodix provides schematics for customers to produce their own
> > > touchscreen controller boards and they tell customers to either hook
> > > up the SPI lines and load the SPI firmware or hook up the I2C lines
> > > and load the I2C firmware. In this case the assumption is that
> > > customers using the same communication method are following the
> > > schematics closely enough that they all behave the same and thus we
> > > don't need some extra distinction.
> > >
> > > In either case it seems like a touchscreen controller board that talks
> > > over SPI and one that talks over I2C are two different products and
> > > thus (to me) should have two distinct compatible strings. This is not
> > > one device that merely has multiple interfaces.
> > >
> >
> > Goodix's approach is similar to Method 2. First, Goodix provides the
> > schematics and the chips (including initial firmware, no touch function)
> > to customers, and customers design their touchscreen controller boards and
> > decide whether to use the I2C or SPI interface. Then, Goodix modifies and
> > debugs the firmware based on the customer's design and provides the final
> > firmware for customers to upgrade.
> 
> OK, thanks!
> 
> From the above that means that if someone uses the "goodix,gt7986u"
> compatible today (with what's landed in mainline) then by that they
> mean "This is a touchscreen that's compatible with a Goodix-defined
> standard way of talking to i2c-based touchscreens built atop a GT7986U
> touchscreen controller". With what's landed in mainline that "standard
> way" is the "i2c-hid" protocol plus a reset line (which IIRC is not
> part of the i2c-hid standard) plus a defined power up/power down
> sequence.
> 
> I suppose one conclusion one might make is that we never should have
> used "goodix,gt7986u" as a compatible string in the first place and
> should have instead added a new compatible string for every actual
> instantiation of a touchscreen. So when Vendor1 made touchscreen 1234
> based on GT7986U then we could have used the compatible
> "vendor1,touchscreen1234" and then when Vendor2 made touchscreen 5678
> based on GT7986U we could have used the compatible
> "vendor2,touchscreen5678". Should we have done this / should we do it
> in the future? I don't know. If everyone using GT7986U is adhering to
> the same interface then it doesn't buy us a ton and adds lots more
> bindings. I think I ended up originally adding the Goodix GT7375P
> bindings because someone gave me a datasheet with all the power
> sequencing and timings that came from Goodix and said it was for the
> "Goodix GT7375P". Given the fact that Goodix provides such a datasheet
> and it includes power sequencing is a strong indicator that there
> truly is a standard and we can use that.
> 
> In any case, if we _had_ used a different compatible for each actual
> touchscreen implementation then we wouldn't be having this discussion.
> Those touchscreens that shipped with a controller board that had SPI
> connections and SPI firmware would have had obviously different
> compatible strings than the touchscreens that shipped with a
> controller board designed for I2C.
> 
> If we _do_ want to keep using a compatible like "goodix,gt7986u" then,
> IMO, it's beneficial to also have a SPI-variant compatible like
> "goodix,gt7986u-spi". This is not a second interface to one device but
> it's actually a distinct interface compared to the Goodix I2C
> interface. Note: this assumes there isn't some hidden benefit to
> having a combined "I2C/SPI" bindings file. I find having the combined
> file buys me nothing and just makes it more confusing / adds
> complexity. Is there some benefit I'm missing other than towing the
> line of "one chip, one compatible"?
> 
> 

Yes, adding compatible entries in the format of "vendor1,touchscreen1234"
could indeed address these issues. However, as you pointed out, this
approach would significantly increase the number of bindings, and making
it quite challenging for Linux users to locate the corresponding binding
information using only the SKU ID on the chip.

> > It is important to note that the type of driver used by the final device
> > is related not only to the bus type but also to the final firmware. Even
> > when using the same I2C bus, different drivers may be needed, such as
> > hid-i2c or a customer-specific driver.
> 
> Right. ...the firmware that's on the device matters and distinct
> firmware can make a distinct device, and IMO a GT7986U loaded with I2C
> firmware is a distinct device than a GT7986U loaded with SPI firmware.
> They are not the same and thus don't need the same compatible.
> 

Ack. But before we find a perfect solution, I will modify the compatible
entry to "goodix,gt7986u-spi" so that this matter can move forward.

Best regards,
Charles

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

[Charles Wang]

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,gt7986u.yaml        | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7986u.yaml

diff --git a/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
new file mode 100644
index 000000000..849117639
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/goodix,gt7986u.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GOODIX GT7986U SPI HID Touchscreen
+
+maintainers:
+  - Charles Wang <charles.goodix@gmail.com>
+
+description: Supports the Goodix GT7986U touchscreen.
+  This touch controller reports data packaged according to the HID protocol,
+  but is incompatible with Microsoft's HID-over-SPI protocol.
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - goodix,gt7986u-spi
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  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.
+
+  spi-max-frequency: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+  - goodix,hid-report-addr
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@0 {
+        compatible = "goodix,gt7986u-spi";
+        reg = <0>;
+        interrupt-parent = <&gpio>;
+        interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+        reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+        spi-max-frequency = <10000000>;
+        goodix,hid-report-addr = <0x22c8c>;
+      };
+    };
+
+...
-- 
2.43.0

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

[Krzysztof Kozlowski]

On Fri, Oct 25, 2024 at 07:46:43PM +0800, 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,gt7986u.yaml        | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> new file mode 100644
> index 000000000..849117639
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/goodix,gt7986u.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GOODIX GT7986U SPI HID Touchscreen

GOODIX or Goodix?

> +
> +maintainers:
> +  - Charles Wang <charles.goodix@gmail.com>
> +
> +description: Supports the Goodix GT7986U touchscreen.
> +  This touch controller reports data packaged according to the HID protocol,
> +  but is incompatible with Microsoft's HID-over-SPI protocol.
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - goodix,gt7986u-spi

Compatible is already documented and nothing here explains why we should
spi variant.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  goodix,hid-report-addr:

I do not see this patch addressing previous review. Sending something
like this as v1 after long discussions also does not help.

No, you keep sending the same and the same, without improvements.

> +    $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.
> +
> +  spi-max-frequency: true
> +
> +additionalProperties: false

Wasn't there a comment about this as well? This goes after required:
block.

Best regards,
Krzysztof

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

[Doug Anderson]

Charles,

On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - goodix,gt7986u-spi
>
> Compatible is already documented and nothing here explains why we should
> spi variant.
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  goodix,hid-report-addr:
>
> I do not see this patch addressing previous review. Sending something
> like this as v1 after long discussions also does not help.

Krzysztof is right that it's better to wait until we get consensus on
the previous discussion before sending a new patch. I know you were
just trying to help move things forward, but because of the way the
email workflow works, sending a new version tends to fork the
discussion into two threads and adds confusion.

I know Krzysztof and Rob have been silent during our recent
discussion, but it's also a long discussion. I've been assuming that
they will take some time to digest and reply in a little bit. If they
didn't, IMO it would have been reasonable to explicitly ask them for
feedback in the other thread after giving a bit of time.

As Krzysztof mentioned, if/when you send the "goodix,gt7986u-spi"
bindings again you'd want to:

* Make sure it's marked as v2.

* Make sure any previous review feedback has been addressed. For
instance, I think Krzysztof requested that you _remove_ the
goodix,hid-report-addr from the bindings and hardcode this into the
driver because every GT7986U will have the same hid-report-addr. I
know that kinda got lost in the discussion but it still needs to be
addressed or at least responded to. I guess there was at least one
other comment about "additionalProperties" that you should look for
and address.

* Make sure there's some type of version history after-the-cut. Tools
like "patman" and "b4" can help with this.

* The commit message should proactively address concerns that came up
during the review process. In this case if we go with
"goodix,gt7986u-spi" the commit message would want to say something
explaining _why_ the "-spi" suffix is appropriate here even though
normally it wouldn't be. That will help anyone digging through
history.

-Doug

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

[Rob Herring]

On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Charles,
>
> On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - goodix,gt7986u-spi
> >
> > Compatible is already documented and nothing here explains why we should
> > spi variant.
> >
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  goodix,hid-report-addr:
> >
> > I do not see this patch addressing previous review. Sending something
> > like this as v1 after long discussions also does not help.
>
> Krzysztof is right that it's better to wait until we get consensus on
> the previous discussion before sending a new patch. I know you were
> just trying to help move things forward, but because of the way the
> email workflow works, sending a new version tends to fork the
> discussion into two threads and adds confusion.
>
> I know Krzysztof and Rob have been silent during our recent
> discussion, but it's also a long discussion. I've been assuming that
> they will take some time to digest and reply in a little bit. If they
> didn't, IMO it would have been reasonable to explicitly ask them for
> feedback in the other thread after giving a bit of time.

If the firmware creates fundamentally different interfaces, then
different compatibles makes sense. If the same driver handles both bus
interfaces, then 1 compatible should be fine. The addition of '-spi'
to the compatible doesn't give any indication of a different
programming model. I wouldn't care except for folks who will see it
and just copy it when their only difference is the bus interface and
we get to have the same discussion all over again. So if appending
'-spi' is the only thing you can come up with, make it abundantly
clear so that others don't blindly copy it. The commit msg is useful
for convincing us, but not for that purpose.

Rob

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

[Doug Anderson]

Hi,

On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Charles,
> >
> > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - goodix,gt7986u-spi
> > >
> > > Compatible is already documented and nothing here explains why we should
> > > spi variant.
> > >
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  reset-gpios:
> > > > +    maxItems: 1
> > > > +
> > > > +  goodix,hid-report-addr:
> > >
> > > I do not see this patch addressing previous review. Sending something
> > > like this as v1 after long discussions also does not help.
> >
> > Krzysztof is right that it's better to wait until we get consensus on
> > the previous discussion before sending a new patch. I know you were
> > just trying to help move things forward, but because of the way the
> > email workflow works, sending a new version tends to fork the
> > discussion into two threads and adds confusion.
> >
> > I know Krzysztof and Rob have been silent during our recent
> > discussion, but it's also a long discussion. I've been assuming that
> > they will take some time to digest and reply in a little bit. If they
> > didn't, IMO it would have been reasonable to explicitly ask them for
> > feedback in the other thread after giving a bit of time.
>
> If the firmware creates fundamentally different interfaces, then
> different compatibles makes sense. If the same driver handles both bus
> interfaces, then 1 compatible should be fine. The addition of '-spi'
> to the compatible doesn't give any indication of a different
> programming model. I wouldn't care except for folks who will see it
> and just copy it when their only difference is the bus interface and
> we get to have the same discussion all over again. So if appending
> '-spi' is the only thing you can come up with, make it abundantly
> clear so that others don't blindly copy it. The commit msg is useful
> for convincing us, but not for that purpose.

OK, makes sense. Charles: Can you think of any better description for
this interface than "goodix,gt7986u-spi"? I suppose you could make it
super obvious that it's running different firmware with
"goodix,gt7986u-spifw" and maybe that would be a little better.

I think what Rob is asking for to make it super obvious is that in the
"description" of the binding you mention that in this case we're
running a substantially different firmware than GT7986U touchscreens
represented by the "goodix,gt7986u" binding and thus is considered a
distinct device.

At this point, IMO you could wait until Monday in case Krzysztof wants
to add his $0.02 worth and then you could send a "v2" patch addressing
the comments so far, though of course you could continue to reply to
this thread if you have further questions / comments.

-Doug

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

[Dmitry Torokhov]

On Fri, Oct 25, 2024 at 09:19:14AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Charles,
> > >
> > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - goodix,gt7986u-spi
> > > >
> > > > Compatible is already documented and nothing here explains why we should
> > > > spi variant.
> > > >
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  reset-gpios:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  goodix,hid-report-addr:
> > > >
> > > > I do not see this patch addressing previous review. Sending something
> > > > like this as v1 after long discussions also does not help.
> > >
> > > Krzysztof is right that it's better to wait until we get consensus on
> > > the previous discussion before sending a new patch. I know you were
> > > just trying to help move things forward, but because of the way the
> > > email workflow works, sending a new version tends to fork the
> > > discussion into two threads and adds confusion.
> > >
> > > I know Krzysztof and Rob have been silent during our recent
> > > discussion, but it's also a long discussion. I've been assuming that
> > > they will take some time to digest and reply in a little bit. If they
> > > didn't, IMO it would have been reasonable to explicitly ask them for
> > > feedback in the other thread after giving a bit of time.
> >
> > If the firmware creates fundamentally different interfaces, then
> > different compatibles makes sense. If the same driver handles both bus
> > interfaces, then 1 compatible should be fine. The addition of '-spi'
> > to the compatible doesn't give any indication of a different
> > programming model. I wouldn't care except for folks who will see it
> > and just copy it when their only difference is the bus interface and
> > we get to have the same discussion all over again. So if appending
> > '-spi' is the only thing you can come up with, make it abundantly
> > clear so that others don't blindly copy it. The commit msg is useful
> > for convincing us, but not for that purpose.
> 
> OK, makes sense. Charles: Can you think of any better description for
> this interface than "goodix,gt7986u-spi"? I suppose you could make it
> super obvious that it's running different firmware with
> "goodix,gt7986u-spifw" and maybe that would be a little better.

Is there any chance for Microsoft-compatible HID-over-SPI versions of
the firmware for this chip? Will this require new compatible string? Or
it will be a different chip ID and the issue will be moot?

Thanks.

-- 
Dmitry

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

[Krzysztof Kozlowski]

On 25/10/2024 18:19, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
>>
>> On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
>>>
>>> Charles,
>>>
>>> On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - goodix,gt7986u-spi
>>>>
>>>> Compatible is already documented and nothing here explains why we should
>>>> spi variant.
>>>>
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  reset-gpios:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  goodix,hid-report-addr:
>>>>
>>>> I do not see this patch addressing previous review. Sending something
>>>> like this as v1 after long discussions also does not help.
>>>
>>> Krzysztof is right that it's better to wait until we get consensus on
>>> the previous discussion before sending a new patch. I know you were
>>> just trying to help move things forward, but because of the way the
>>> email workflow works, sending a new version tends to fork the
>>> discussion into two threads and adds confusion.
>>>
>>> I know Krzysztof and Rob have been silent during our recent
>>> discussion, but it's also a long discussion. I've been assuming that
>>> they will take some time to digest and reply in a little bit. If they
>>> didn't, IMO it would have been reasonable to explicitly ask them for
>>> feedback in the other thread after giving a bit of time.
>>
>> If the firmware creates fundamentally different interfaces, then
>> different compatibles makes sense. If the same driver handles both bus
>> interfaces, then 1 compatible should be fine. The addition of '-spi'
>> to the compatible doesn't give any indication of a different
>> programming model. I wouldn't care except for folks who will see it
>> and just copy it when their only difference is the bus interface and
>> we get to have the same discussion all over again. So if appending
>> '-spi' is the only thing you can come up with, make it abundantly
>> clear so that others don't blindly copy it. The commit msg is useful
>> for convincing us, but not for that purpose.
> 
> OK, makes sense. Charles: Can you think of any better description for
> this interface than "goodix,gt7986u-spi"? I suppose you could make it
> super obvious that it's running different firmware with
> "goodix,gt7986u-spifw" and maybe that would be a little better.
> 
> I think what Rob is asking for to make it super obvious is that in the
> "description" of the binding you mention that in this case we're
> running a substantially different firmware than GT7986U touchscreens
> represented by the "goodix,gt7986u" binding and thus is considered a
> distinct device.
> 
> At this point, IMO you could wait until Monday in case Krzysztof wants
> to add his $0.02 worth and then you could send a "v2" patch addressing
> the comments so far, though of course you could continue to reply to
> this thread if you have further questions / comments.

And to re-iterate: both commit msg and hardware description in the
binding must clearly explain this why another compatible was chosen for
the same device.

Best regards,
Krzysztof

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

[Charles Wang]

Hi Doug,

On Fri, Oct 25, 2024 at 08:29:13AM -0700, Doug Anderson wrote:
> Charles,
> 
> On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - goodix,gt7986u-spi
> >
> > Compatible is already documented and nothing here explains why we should
> > spi variant.
> >
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  goodix,hid-report-addr:
> >
> > I do not see this patch addressing previous review. Sending something
> > like this as v1 after long discussions also does not help.
> 
> Krzysztof is right that it's better to wait until we get consensus on
> the previous discussion before sending a new patch. I know you were
> just trying to help move things forward, but because of the way the
> email workflow works, sending a new version tends to fork the
> discussion into two threads and adds confusion.
> 
> I know Krzysztof and Rob have been silent during our recent
> discussion, but it's also a long discussion. I've been assuming that
> they will take some time to digest and reply in a little bit. If they
> didn't, IMO it would have been reasonable to explicitly ask them for
> feedback in the other thread after giving a bit of time.
> 
> As Krzysztof mentioned, if/when you send the "goodix,gt7986u-spi"
> bindings again you'd want to:
> 
> * Make sure it's marked as v2.
> 
> * Make sure any previous review feedback has been addressed. For
> instance, I think Krzysztof requested that you _remove_ the
> goodix,hid-report-addr from the bindings and hardcode this into the
> driver because every GT7986U will have the same hid-report-addr. I
> know that kinda got lost in the discussion but it still needs to be
> addressed or at least responded to. I guess there was at least one
> other comment about "additionalProperties" that you should look for
> and address.
> 
> * Make sure there's some type of version history after-the-cut. Tools
> like "patman" and "b4" can help with this.
> 
> * The commit message should proactively address concerns that came up
> during the review process. In this case if we go with
> "goodix,gt7986u-spi" the commit message would want to say something
> explaining _why_ the "-spi" suffix is appropriate here even though
> normally it wouldn't be. That will help anyone digging through
> history.
> 

I apologize for any confusion caused. As a newcomer, I am still learning
about the community practices.

Thank you very much for your patience and clear explanation. I will recheck
the previous review feedback and provide a new patch marked as v2.

Best regards,
Charles

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

[Charles Wang]

On Fri, Oct 25, 2024 at 09:19:14AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Charles,
> > >
> > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - goodix,gt7986u-spi
> > > >
> > > > Compatible is already documented and nothing here explains why we should
> > > > spi variant.
> > > >
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  reset-gpios:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  goodix,hid-report-addr:
> > > >
> > > > I do not see this patch addressing previous review. Sending something
> > > > like this as v1 after long discussions also does not help.
> > >
> > > Krzysztof is right that it's better to wait until we get consensus on
> > > the previous discussion before sending a new patch. I know you were
> > > just trying to help move things forward, but because of the way the
> > > email workflow works, sending a new version tends to fork the
> > > discussion into two threads and adds confusion.
> > >
> > > I know Krzysztof and Rob have been silent during our recent
> > > discussion, but it's also a long discussion. I've been assuming that
> > > they will take some time to digest and reply in a little bit. If they
> > > didn't, IMO it would have been reasonable to explicitly ask them for
> > > feedback in the other thread after giving a bit of time.
> >
> > If the firmware creates fundamentally different interfaces, then
> > different compatibles makes sense. If the same driver handles both bus
> > interfaces, then 1 compatible should be fine. The addition of '-spi'
> > to the compatible doesn't give any indication of a different
> > programming model. I wouldn't care except for folks who will see it
> > and just copy it when their only difference is the bus interface and
> > we get to have the same discussion all over again. So if appending
> > '-spi' is the only thing you can come up with, make it abundantly
> > clear so that others don't blindly copy it. The commit msg is useful
> > for convincing us, but not for that purpose.
> 
> OK, makes sense. Charles: Can you think of any better description for
> this interface than "goodix,gt7986u-spi"? I suppose you could make it
> super obvious that it's running different firmware with
> "goodix,gt7986u-spifw" and maybe that would be a little better.
> 
> I think what Rob is asking for to make it super obvious is that in the
> "description" of the binding you mention that in this case we're
> running a substantially different firmware than GT7986U touchscreens
> represented by the "goodix,gt7986u" binding and thus is considered a
> distinct device.
> 
> At this point, IMO you could wait until Monday in case Krzysztof wants
> to add his $0.02 worth and then you could send a "v2" patch addressing
> the comments so far, though of course you could continue to reply to
> this thread if you have further questions / comments.
> 

Thank you for your explanation, I understand your point. I want to clarify
that the gt7986u-spi and gt7986u indeed use two entirely different drivers
and two distinct firmware.

Using "goodix,gt7986u-spi" could indeed cause confusion. How about modifying
it to "goodix,gt7986u-losto" by adding a special code?

Additionally, I would like to confirm: when submitting the v2 patch, should
it be based on this thread or the previous discussion thread?

Best regards,
Charles
 

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

[Charles Wang]

Hi Dmitry,

On Fri, Oct 25, 2024 at 10:14:52AM -0700, Dmitry Torokhov wrote:
> On Fri, Oct 25, 2024 at 09:19:14AM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Charles,
> > > >
> > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - goodix,gt7986u-spi
> > > > >
> > > > > Compatible is already documented and nothing here explains why we should
> > > > > spi variant.
> > > > >
> > > > > > +
> > > > > > +  reg:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  interrupts:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  reset-gpios:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  goodix,hid-report-addr:
> > > > >
> > > > > I do not see this patch addressing previous review. Sending something
> > > > > like this as v1 after long discussions also does not help.
> > > >
> > > > Krzysztof is right that it's better to wait until we get consensus on
> > > > the previous discussion before sending a new patch. I know you were
> > > > just trying to help move things forward, but because of the way the
> > > > email workflow works, sending a new version tends to fork the
> > > > discussion into two threads and adds confusion.
> > > >
> > > > I know Krzysztof and Rob have been silent during our recent
> > > > discussion, but it's also a long discussion. I've been assuming that
> > > > they will take some time to digest and reply in a little bit. If they
> > > > didn't, IMO it would have been reasonable to explicitly ask them for
> > > > feedback in the other thread after giving a bit of time.
> > >
> > > If the firmware creates fundamentally different interfaces, then
> > > different compatibles makes sense. If the same driver handles both bus
> > > interfaces, then 1 compatible should be fine. The addition of '-spi'
> > > to the compatible doesn't give any indication of a different
> > > programming model. I wouldn't care except for folks who will see it
> > > and just copy it when their only difference is the bus interface and
> > > we get to have the same discussion all over again. So if appending
> > > '-spi' is the only thing you can come up with, make it abundantly
> > > clear so that others don't blindly copy it. The commit msg is useful
> > > for convincing us, but not for that purpose.
> > 
> > OK, makes sense. Charles: Can you think of any better description for
> > this interface than "goodix,gt7986u-spi"? I suppose you could make it
> > super obvious that it's running different firmware with
> > "goodix,gt7986u-spifw" and maybe that would be a little better.
> 
> Is there any chance for Microsoft-compatible HID-over-SPI versions of
> the firmware for this chip? Will this require new compatible string? Or
> it will be a different chip ID and the issue will be moot?
> 
No, the SPI hardware design of this chip does not meet the requirements for
the Microsoft HID-over-SPI protocol. A new chip with a new ID will be
developed to support the Microsoft HID-over-SPI protocol.

Thanks,
Charles

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

[Doug Anderson]

Hi,

On Tue, Oct 29, 2024 at 11:57 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 09:19:14AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Charles,
> > > >
> > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > >
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - goodix,gt7986u-spi
> > > > >
> > > > > Compatible is already documented and nothing here explains why we should
> > > > > spi variant.
> > > > >
> > > > > > +
> > > > > > +  reg:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  interrupts:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  reset-gpios:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  goodix,hid-report-addr:
> > > > >
> > > > > I do not see this patch addressing previous review. Sending something
> > > > > like this as v1 after long discussions also does not help.
> > > >
> > > > Krzysztof is right that it's better to wait until we get consensus on
> > > > the previous discussion before sending a new patch. I know you were
> > > > just trying to help move things forward, but because of the way the
> > > > email workflow works, sending a new version tends to fork the
> > > > discussion into two threads and adds confusion.
> > > >
> > > > I know Krzysztof and Rob have been silent during our recent
> > > > discussion, but it's also a long discussion. I've been assuming that
> > > > they will take some time to digest and reply in a little bit. If they
> > > > didn't, IMO it would have been reasonable to explicitly ask them for
> > > > feedback in the other thread after giving a bit of time.
> > >
> > > If the firmware creates fundamentally different interfaces, then
> > > different compatibles makes sense. If the same driver handles both bus
> > > interfaces, then 1 compatible should be fine. The addition of '-spi'
> > > to the compatible doesn't give any indication of a different
> > > programming model. I wouldn't care except for folks who will see it
> > > and just copy it when their only difference is the bus interface and
> > > we get to have the same discussion all over again. So if appending
> > > '-spi' is the only thing you can come up with, make it abundantly
> > > clear so that others don't blindly copy it. The commit msg is useful
> > > for convincing us, but not for that purpose.
> >
> > OK, makes sense. Charles: Can you think of any better description for
> > this interface than "goodix,gt7986u-spi"? I suppose you could make it
> > super obvious that it's running different firmware with
> > "goodix,gt7986u-spifw" and maybe that would be a little better.
> >
> > I think what Rob is asking for to make it super obvious is that in the
> > "description" of the binding you mention that in this case we're
> > running a substantially different firmware than GT7986U touchscreens
> > represented by the "goodix,gt7986u" binding and thus is considered a
> > distinct device.
> >
> > At this point, IMO you could wait until Monday in case Krzysztof wants
> > to add his $0.02 worth and then you could send a "v2" patch addressing
> > the comments so far, though of course you could continue to reply to
> > this thread if you have further questions / comments.
> >
>
> Thank you for your explanation, I understand your point. I want to clarify
> that the gt7986u-spi and gt7986u indeed use two entirely different drivers
> and two distinct firmware.
>
> Using "goodix,gt7986u-spi" could indeed cause confusion. How about modifying
> it to "goodix,gt7986u-losto" by adding a special code?

If "lotso" somehow means something real to people using this product
then that seems OK to me. If "lotso" is just a made up word because
you don't want to use "spi" or "spifw" then it's not great. In either
case you'll want to summarize our discussion here in your
"description" in the yaml and in the commit message.


> Additionally, I would like to confirm: when submitting the v2 patch, should
> it be based on this thread or the previous discussion thread?

No, v2 should _not_ be In-Reply-To this thread. It'll start a new
thread. You can add a link (via lore.kernel.org/r/<message-id>) to the
old discussion in your cover letter and/or version history.

Said another way:
* New versions of patches create new threads.
* The fact that new versions of patches create new threads is why
people usually want open questions answered before the next version is
sent.

:-)

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

[Charles Wang]

Hi,

On Fri, Oct 25, 2024 at 02:03:11PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Oct 25, 2024 at 07:46:43PM +0800, 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,gt7986u.yaml        | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> > new file mode 100644
> > index 000000000..849117639
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/goodix,gt7986u.yaml
> > @@ -0,0 +1,71 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/goodix,gt7986u.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: GOODIX GT7986U SPI HID Touchscreen
> 
> GOODIX or Goodix?

Thank you for catching this, the name should be Goodix.

>
> > +
> > +maintainers:
> > +  - Charles Wang <charles.goodix@gmail.com>
> > +
> > +description: Supports the Goodix GT7986U touchscreen.
> > +  This touch controller reports data packaged according to the HID protocol,
> > +  but is incompatible with Microsoft's HID-over-SPI protocol.
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - goodix,gt7986u-spi
> 
> Compatible is already documented and nothing here explains why we should
> spi variant.
> 

Ack. I will modify the compatible name based on our discussion and include an
appropriate description.

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  goodix,hid-report-addr:
> 
> I do not see this patch addressing previous review. Sending something
> like this as v1 after long discussions also does not help.
> 
> No, you keep sending the same and the same, without improvements.
>

I apologize for overlooking the discussions regarding this issue.

I would like to clarify that while the current boards use the same address,
but newly designed boards in the future may require different addresses.

Retaining this property would likely offer more flexibility.

>
> > +    $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.
> > +
> > +  spi-max-frequency: true
> > +
> > +additionalProperties: false
> 
> Wasn't there a comment about this as well? This goes after required:
> block.
>

Ack, will change to unevaluatedProperties in the next version.

Best regards,
Charles

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

[Charles Wang]

On Wed, Oct 30, 2024 at 11:14:26AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 29, 2024 at 11:57 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 09:19:14AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Oct 25, 2024 at 8:59 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@chromium.org> wrote:
> > > > >
> > > > > Charles,
> > > > >
> > > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > > >
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    enum:
> > > > > > > +      - goodix,gt7986u-spi
> > > > > >
> > > > > > Compatible is already documented and nothing here explains why we should
> > > > > > spi variant.
> > > > > >
> > > > > > > +
> > > > > > > +  reg:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  interrupts:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  reset-gpios:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  goodix,hid-report-addr:
> > > > > >
> > > > > > I do not see this patch addressing previous review. Sending something
> > > > > > like this as v1 after long discussions also does not help.
> > > > >
> > > > > Krzysztof is right that it's better to wait until we get consensus on
> > > > > the previous discussion before sending a new patch. I know you were
> > > > > just trying to help move things forward, but because of the way the
> > > > > email workflow works, sending a new version tends to fork the
> > > > > discussion into two threads and adds confusion.
> > > > >
> > > > > I know Krzysztof and Rob have been silent during our recent
> > > > > discussion, but it's also a long discussion. I've been assuming that
> > > > > they will take some time to digest and reply in a little bit. If they
> > > > > didn't, IMO it would have been reasonable to explicitly ask them for
> > > > > feedback in the other thread after giving a bit of time.
> > > >
> > > > If the firmware creates fundamentally different interfaces, then
> > > > different compatibles makes sense. If the same driver handles both bus
> > > > interfaces, then 1 compatible should be fine. The addition of '-spi'
> > > > to the compatible doesn't give any indication of a different
> > > > programming model. I wouldn't care except for folks who will see it
> > > > and just copy it when their only difference is the bus interface and
> > > > we get to have the same discussion all over again. So if appending
> > > > '-spi' is the only thing you can come up with, make it abundantly
> > > > clear so that others don't blindly copy it. The commit msg is useful
> > > > for convincing us, but not for that purpose.
> > >
> > > OK, makes sense. Charles: Can you think of any better description for
> > > this interface than "goodix,gt7986u-spi"? I suppose you could make it
> > > super obvious that it's running different firmware with
> > > "goodix,gt7986u-spifw" and maybe that would be a little better.
> > >
> > > I think what Rob is asking for to make it super obvious is that in the
> > > "description" of the binding you mention that in this case we're
> > > running a substantially different firmware than GT7986U touchscreens
> > > represented by the "goodix,gt7986u" binding and thus is considered a
> > > distinct device.
> > >
> > > At this point, IMO you could wait until Monday in case Krzysztof wants
> > > to add his $0.02 worth and then you could send a "v2" patch addressing
> > > the comments so far, though of course you could continue to reply to
> > > this thread if you have further questions / comments.
> > >
> >
> > Thank you for your explanation, I understand your point. I want to clarify
> > that the gt7986u-spi and gt7986u indeed use two entirely different drivers
> > and two distinct firmware.
> >
> > Using "goodix,gt7986u-spi" could indeed cause confusion. How about modifying
> > it to "goodix,gt7986u-losto" by adding a special code?
> 
> If "lotso" somehow means something real to people using this product
> then that seems OK to me. If "lotso" is just a made up word because
> you don't want to use "spi" or "spifw" then it's not great. In either
> case you'll want to summarize our discussion here in your
> "description" in the yaml and in the commit message.
> 

Okay, got it.

> 
> > Additionally, I would like to confirm: when submitting the v2 patch, should
> > it be based on this thread or the previous discussion thread?
> 
> No, v2 should _not_ be In-Reply-To this thread. It'll start a new
> thread. You can add a link (via lore.kernel.org/r/<message-id>) to the
> old discussion in your cover letter and/or version history.
> 
> Said another way:
> * New versions of patches create new threads.
> * The fact that new versions of patches create new threads is why
> people usually want open questions answered before the next version is
> sent.
> 

Okay, thank you very much for your patient explanation.

Best regards,
Charles

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

[Doug Anderson]

Hi,

On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> > > +  goodix,hid-report-addr:
> >
> > I do not see this patch addressing previous review. Sending something
> > like this as v1 after long discussions also does not help.
> >
> > No, you keep sending the same and the same, without improvements.
> >
>
> I apologize for overlooking the discussions regarding this issue.
>
> I would like to clarify that while the current boards use the same address,
> but newly designed boards in the future may require different addresses.
>
> Retaining this property would likely offer more flexibility.

I don't feel very strongly about it, but maybe Krzysztof does?
Possibly the path of least resistance would be:

1. You drop the property from the bindings.

2. You hardcode it in the driver to be the normal value.

3. If/when someone actually needs a different value then we can add it
as an optional property in the bindings and fall back to the default
value if the property isn't present.

What do you think? If you feel strongly about keeping the
"hid-report-addr" then you can certainly keep making your case.
However, it's probably best to wait to get agreement from Krzysztof
(or one of the other DT maintainers) before sending your next version
unless you're going to take the "path of least resistance" that I talk
about above.

-Doug

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

[Charles Wang]

Hi Doug,

On Thu, Oct 31, 2024 at 10:58:07AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > > > +  goodix,hid-report-addr:
> > >
> > > I do not see this patch addressing previous review. Sending something
> > > like this as v1 after long discussions also does not help.
> > >
> > > No, you keep sending the same and the same, without improvements.
> > >
> >
> > I apologize for overlooking the discussions regarding this issue.
> >
> > I would like to clarify that while the current boards use the same address,
> > but newly designed boards in the future may require different addresses.
> >
> > Retaining this property would likely offer more flexibility.
> 
> I don't feel very strongly about it, but maybe Krzysztof does?
> Possibly the path of least resistance would be:
> 
> 1. You drop the property from the bindings.
> 
> 2. You hardcode it in the driver to be the normal value.
> 
> 3. If/when someone actually needs a different value then we can add it
> as an optional property in the bindings and fall back to the default
> value if the property isn't present.
> 
> What do you think? If you feel strongly about keeping the
> "hid-report-addr" then you can certainly keep making your case.
> However, it's probably best to wait to get agreement from Krzysztof
> (or one of the other DT maintainers) before sending your next version
> unless you're going to take the "path of least resistance" that I talk
> about above.
>

Agreed, let's wait and see the opinions of Krzysztof (or the other DT
maintainers).

Thanks,
Charles

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

[Doug Anderson]

Charles,

On Thu, Oct 31, 2024 at 6:33 PM Charles Wang <charles.goodix@gmail.com> wrote:
>
> Hi Doug,
>
> On Thu, Oct 31, 2024 at 10:58:07AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
> > >
> > > > > +  goodix,hid-report-addr:
> > > >
> > > > I do not see this patch addressing previous review. Sending something
> > > > like this as v1 after long discussions also does not help.
> > > >
> > > > No, you keep sending the same and the same, without improvements.
> > > >
> > >
> > > I apologize for overlooking the discussions regarding this issue.
> > >
> > > I would like to clarify that while the current boards use the same address,
> > > but newly designed boards in the future may require different addresses.
> > >
> > > Retaining this property would likely offer more flexibility.
> >
> > I don't feel very strongly about it, but maybe Krzysztof does?
> > Possibly the path of least resistance would be:
> >
> > 1. You drop the property from the bindings.
> >
> > 2. You hardcode it in the driver to be the normal value.
> >
> > 3. If/when someone actually needs a different value then we can add it
> > as an optional property in the bindings and fall back to the default
> > value if the property isn't present.
> >
> > What do you think? If you feel strongly about keeping the
> > "hid-report-addr" then you can certainly keep making your case.
> > However, it's probably best to wait to get agreement from Krzysztof
> > (or one of the other DT maintainers) before sending your next version
> > unless you're going to take the "path of least resistance" that I talk
> > about above.
> >
>
> Agreed, let's wait and see the opinions of Krzysztof (or the other DT
> maintainers).

As I went back and looked at this again, I'm curious: I don't know
much about how your protocol works, but is there any reason why your
driver can't figure out this "hid-report-addr" dynamically? Is there
some reason you can't talk to the device and ask it what the
"hid-report-addr" should be? From skimming through your driver there
appear to already be a few hardcoded addresses. Can one of those
provide you the info you need?


-Doug

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

[Charles Wang]

On Mon, Nov 04, 2024 at 11:36:50AM -0800, Doug Anderson wrote:
> Charles,
> 
> On Thu, Oct 31, 2024 at 6:33 PM Charles Wang <charles.goodix@gmail.com> wrote:
> >
> > Hi Doug,
> >
> > On Thu, Oct 31, 2024 at 10:58:07AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Oct 30, 2024 at 7:37 PM Charles Wang <charles.goodix@gmail.com> wrote:
> > > >
> > > > > > +  goodix,hid-report-addr:
> > > > >
> > > > > I do not see this patch addressing previous review. Sending something
> > > > > like this as v1 after long discussions also does not help.
> > > > >
> > > > > No, you keep sending the same and the same, without improvements.
> > > > >
> > > >
> > > > I apologize for overlooking the discussions regarding this issue.
> > > >
> > > > I would like to clarify that while the current boards use the same address,
> > > > but newly designed boards in the future may require different addresses.
> > > >
> > > > Retaining this property would likely offer more flexibility.
> > >
> > > I don't feel very strongly about it, but maybe Krzysztof does?
> > > Possibly the path of least resistance would be:
> > >
> > > 1. You drop the property from the bindings.
> > >
> > > 2. You hardcode it in the driver to be the normal value.
> > >
> > > 3. If/when someone actually needs a different value then we can add it
> > > as an optional property in the bindings and fall back to the default
> > > value if the property isn't present.
> > >
> > > What do you think? If you feel strongly about keeping the
> > > "hid-report-addr" then you can certainly keep making your case.
> > > However, it's probably best to wait to get agreement from Krzysztof
> > > (or one of the other DT maintainers) before sending your next version
> > > unless you're going to take the "path of least resistance" that I talk
> > > about above.
> > >
> >
> > Agreed, let's wait and see the opinions of Krzysztof (or the other DT
> > maintainers).
> 
> As I went back and looked at this again, I'm curious: I don't know
> much about how your protocol works, but is there any reason why your
> driver can't figure out this "hid-report-addr" dynamically? Is there
> some reason you can't talk to the device and ask it what the
> "hid-report-addr" should be? From skimming through your driver there
> appear to already be a few hardcoded addresses. Can one of those
> provide you the info you need?
> 

Similar to a standard i2c-hid driver, which requires configuring the
address for hid-descr-addr in the DTS, other address information is
obtained using this address.

In theory, we can dynamically obtain most of the addresses from the chip.
However, for this chip, there always needs to be a known address for the
driver to communicate with, whether this address is 0x0000 or 0x0001,
and this address is related to the firmware.

Regarding this issue, since no further review comments received.
I think I can first remove the address as your previous suggestion
and use a default address for communication in the driver. In the
future, we can upgrade the firmware and driver to achieve dynamic
address acquisition.

Thanks,
Charles