Re: [PATCH v3 0/4] HID: touchscreen: add himax hid-over-spi driver

From: Doug Anderson <dianders@chromium.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Tylor Yang <tylor_yang@himax.corp-partner.google.com>,
	Tomasz Figa <tfiga@chromium.org>,
	jingyliang@chromium.org,
	poyuan_chang@himax.corp-partner.google.com, hbarnor@chromium.org,
	jikos@kernel.org, wuxy23@lenovo.com, conor+dt@kernel.org,
	luolm1@lenovo.com, robh+dt@kernel.org, dmitry.torokhov@gmail.com,
	devicetree@vger.kernel.org, krzysztof.kozlowski+dt@linaro.org,
	poyu_hung@himax.corp-partner.google.com,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	benjamin.tissoires@redhat.com
Subject: Re: [PATCH v3 0/4] HID: touchscreen: add himax hid-over-spi driver
Date: Tue, 17 Oct 2023 14:41:50 -0700	[thread overview]
Message-ID: <CAD=FV=X2kZcyeyK1SBcXaViBft4F6XYtA6+JwBqJswU41V9kUQ@mail.gmail.com> (raw)
In-Reply-To: <6c7d9c92-7616-4fad-806e-44302c33b63c@linaro.org>

Hi,

On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/10/2023 11:18, Tylor Yang wrote:
> > Hello,
> >
> > This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> > This driver takes a position in [1], it intends to take advantage of SPI
> > transfer speed and HID interface.
> >
>
> Dear Google/Chromium folks,
>
> As a multi-billion company I am sure you can spare some small amount of
> time/effort/money for internal review before using community for this
> purpose. I mean reviewing trivial issues, like coding style, or just
> running checkpatch. You know, the obvious things.
>
> There is no need to use expensive time of community reviewers to review
> very simple mistakes, the ones which we fixed in Linux kernel years ago
> (also with automated tools). You can and you should do it, before
> submitting drivers for community review.

We can certainly talk more about this, but a quick reply is:

1. If a patch really looks super bad to you then the right thing for
you to do is to respond to the patch with some canned response saying
"you didn't even do these basic things--please read the documentation
and work with someone at Google to get a basic review". This seems
like a perfectly legit response and I don't think you should do more
than that.

2. IMO as a general rule "internal review" should be considered
harmful. When you're a new submitter then absolutely you should get
some internal review from someone who has done this before, but making
"internal review" a requirement for all patches leads to frustration
all around. It leads to people redesigning their code in response to
"internal review" and then getting frustrated when external
maintainers tell them to do something totally different. ...then
upstream reviewers respond to the frustration with "Why were you
designing your code behind closed doors? If you had done the review in
the public and on the mailing lists then someone could have stopped
you before you changed everything".

3. The ChromeOS team is organized much more like the upstream
community than a big hierarchical corporation. Just as it's not easy
for you to control the behavior of other maintainers, it is not
trivial for one person on the team to control what others on the team
will do. We could make an attempt to institute rules like "all patches
must go through internal review before being posted", but as per #2 I
don't think this is a good idea. The ChromeOS team has even less
control over what our partners may or may not do. In general it is
always a struggle to get partners to even start working upstream and
IMO it's a win when I see a partner post a patch. We should certainly
help partners be successful here, but the right way to do that is by
offering them support.

About the best we can do is to provide good documentation for people
learning how to send patches. Right now the ChromeOS kernel docs [1]
suggest using "patman" to send patches and I have seen many partners
do this. Patman will, at the very least, run checkpatch for you. Our
instructions also say that you should make sure you run "checkpatch"
yourself if you don't run patman. If people aren't following these
docs that we already have then there's not much we can do.

So I guess the tl;dr from my side:

a) People should absolutely be posting on mailing lists and not (as a
rule) doing "internal review".

b) If a patch looks really broken to you, don't get upset and don't
waste your time. Just respond and say that you'll look at it once it
looks better and suggest that they get a review (preferably on the
mailing lists!) from someone they're working with at Google.

https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#send-out-the-patch-using-patman

-Doug