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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <dcb7a9de-a7c7-4d04-bc36-22f66ef17a38@kernel.org>
Date: Mon, 30 Sep 2024 10:08:52 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Charles Wang <charles.goodix@...il.com>
Cc: jikos@...nel.org, bentiss@...nel.org, hbarnor@...omium.org,
 linux-input@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Conor Dooley <conor.dooley@...rochip.com>
Subject: Re: [PATCH v8 2/2] dt-bindings: input: Goodix SPI HID Touchscreen

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