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@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > Charles,
> > > > >
> > > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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