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
