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

[Krzysztof Kozlowski <krzk@kernel.org>]

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