diff mbox series

[v8,2/2] dt-bindings: input: Goodix SPI HID Touchscreen

Message ID 20240926044217.9285-3-charles.goodix@gmail.com
State Superseded
Headers show
Series HID: add initial support for Goodix HID-over-SPI touchscreen | expand

Commit Message

Charles Wang Sept. 26, 2024, 4:42 a.m. UTC 
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.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
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

Comments

Charles Wang Sept. 30, 2024, 3:22 a.m. UTC | #1 
Hi Krzysztof,
Thank you very much for your advice.

On Thu, Sep 26, 2024 at 02:32:05PM +0200, Krzysztof Kozlowski wrote:
> On 26/09/2024 11:57, Charles Wang wrote:
> >>>  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
> >>
> >> NAK, you duplicate again the binding. You cannot have bus-flavors.
> >> Device is the same.
> >>
> > 
> > Could you provide some suggestions regarding this issue?
> 
> What is exactly the question or problem? There is a binding for this
> device. Extend it with SPI parts, e.g.
> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml#L22
> 

This seems a little different from the adxl313.yaml.

The issue we're encountering involves the chip model gt7986u,
which supports both I2C and SPI interfaces. For the I2C interface
(using the HID-over-I2C driver), it has already been declared in
the goodix,gt7375p.yaml file as follows:

i2c {
  #address-cells = <1>;
  #size-cells = <0>;

  ap_ts: touchscreen@5d {
    compatible = "goodix,gt7986u";
  }
}

Currently, our design requires utilizing the SPI interface with
a custom SPI driver. However, the declarations within the binding
file have led to conflicts, as shown here:

spi {
  #address-cells = <1>;
  #size-cells = <0>;

  touchscreen@0 {
    compatible = "goodix,gt7986u";
  }
}

Should I consider merging both YAML files into a single one to fix this?

Best regards!
Charles
Krzysztof Kozlowski Sept. 30, 2024, 8:08 a.m. UTC | #2 
On 30/09/2024 09:56, Charles Wang wrote:
> Hi Krzysztof,
> 
> On Mon, Sep 30, 2024 at 08:42:22AM +0200, Krzysztof Kozlowski wrote:
>> On 30/09/2024 05:22, Charles Wang wrote:
>>> Hi Krzysztof,
>>> Thank you very much for your advice.
>>>
>>> On Thu, Sep 26, 2024 at 02:32:05PM +0200, Krzysztof Kozlowski wrote:
>>>> On 26/09/2024 11:57, Charles Wang wrote:
>>>>>>>  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
>>>>>>
>>>>>> NAK, you duplicate again the binding. You cannot have bus-flavors.
>>>>>> Device is the same.
>>>>>>
>>>>>
>>>>> Could you provide some suggestions regarding this issue?
>>>>
>>>> What is exactly the question or problem? There is a binding for this
>>>> device. Extend it with SPI parts, e.g.
>>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml#L22
>>>>
>>>
>>> This seems a little different from the adxl313.yaml.
>>
>> Hm? I am reading below:
>>
>>>
>>> The issue we're encountering involves the chip model gt7986u,
>>> which supports both I2C and SPI interfaces. For the I2C interface
>>> (using the HID-over-I2C driver), it has already been declared in
>>> the goodix,gt7375p.yaml file as follows:
>>>
>>> i2c {
>>>   #address-cells = <1>;
>>>   #size-cells = <0>;
>>>
>>>   ap_ts: touchscreen@5d {
>>>     compatible = "goodix,gt7986u";
>>>   }
>>> }
>>>
>>> Currently, our design requires utilizing the SPI interface with
>>> a custom SPI driver. However, the declarations within the binding
>>> file have led to conflicts, as shown here:
>>>
>>> spi {
>>>   #address-cells = <1>;
>>>   #size-cells = <0>;
>>>
>>>   touchscreen@0 {
>>>     compatible = "goodix,gt7986u";
>>>   }
>>> }
>>>
>>> Should I consider merging both YAML files into a single one to fix this?
>>
>> And there is no difference. I don't understand the problem.
>>
> I'm sorry for the confusion regarding your comment
> 
> "And there is no difference." 
> 
> Are you implying that the issue we are encountering is no different from

I don't understand the issue.

> 'adi,adxl313.yaml'? Or are you suggesting that the gt7986u device should
> be treated as the same entity for both I2C and SPI interfaces?

I told you what to do - extend existing binding. I gave you example how
one binding is for both I2C and SPI devices.

> 
> Original error messages: https://lore.kernel.org/all/CAL_Jsq+QfTtRj_JCqXzktQ49H8VUnztVuaBjvvkg3fwEHniUHw@mail.gmail.com/
> 
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - goodix,gt7986u
> 
> This is already documented in goodix,gt7375p.yaml. Now linux-next has warnings:
> 
> /builds/robherring/linux-dt/Documentation/devicetree/bindings/input/goodix,gt7986u.example.dtb:
> touchscreen@0: compatible: 'oneOf' conditional failed, one must be
> fixed:
>         ['goodix,gt7986u'] is too short
>         'goodix,gt7375p' was expected
>         from schema $id:
> 
> I understand this error message to mean the same chip model is redundantly declared
> in two separate files.

That was old error, which I fixed by reverting your patches.

> 
> Is my understanding incorrect? Could you provide more explicit guidance?
> 
> Note that the 'gt7986u' uses different drivers and has distinct device property
> for its I2C and SPI interfaces.

Drivers don't matter. We talk about bindings here. One device, one
binding, one compatible. Regardless of the bus.


Best regards,
Krzysztof
diff mbox series

Patch

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