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

[Charles Wang <charles.goodix@gmail.com>]

On Mon, Sep 30, 2024 at 10:08:52AM +0200, Krzysztof Kozlowski wrote:
> 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.
> 

Sorry for my poor English expression.

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

Ack, thank you for the clarification and the example provided.

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

Okay, really appreciate your guidance.

Best regards,
Charles