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

[Krzysztof Kozlowski]

On 17/10/2023 23:41, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> 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".

No one expects forced internal review on mature contributions. We talk
here about a first time contribution where already basic mistakes were
made: like not using get_maintainers.pl, not using checkpatch, not using
other tools and finally sending code which does not look like Linux
kernel code at all.

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

I don't know who is exactly core team, who is partner. I see
"google.com" domain, so Google folks are responsible for not wasting
time of the community. If Google disagrees, please change the domain so
I will understand that and not feel like Google wants to use us all. I
am fine and I understand if small companies or individuals make such
mistakes. It feels like a waste of our time if Google makes such
mistakes. Google's (Alphabet's) revenue for 2022 was 282 billions USD
and net revenue was 59 billions USD.

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


Best regards,
Krzysztof