linux-kernel - Re: [PATCH] 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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAD=FV=WtbBnCX6fcqBFYPO3C=zw8YhKmJ35hW920JWt0MvDqPw@mail.gmail.com>
Date: Wed, 30 Oct 2024 11:14:26 -0700
From: Doug Anderson <dianders@...omium.org>
To: Charles Wang <charles.goodix@...il.com>
Cc: Rob Herring <robh@...nel.org>, dmitry.torokhov@...il.com, hbarnor@...omium.org, 
	conor.dooley@...rochip.com, jikos@...nel.org, bentiss@...nel.org, 
	linux-input@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH] dt-bindings: input: Goodix SPI HID Touchscreen

Hi,

On Tue, Oct 29, 2024 at 11:57 PM Charles Wang <charles.goodix@...il.com> 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@...nel.org> wrote:
> > >
> > > On Fri, Oct 25, 2024 at 10:29 AM Doug Anderson <dianders@...omium.org> wrote:
> > > >
> > > > Charles,
> > > >
> > > > On Fri, Oct 25, 2024 at 5:03 AM Krzysztof Kozlowski <krzk@...nel.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.
> >
>
> 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.


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

:-)