From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by mx.groups.io with SMTP id smtpd.web12.23837.1574181009562594335 for ; Tue, 19 Nov 2019 08:30:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=I3S19nj6; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.44, mailfrom: pete@akeo.ie) Received: by mail-wr1-f44.google.com with SMTP id q15so11808179wrw.7 for ; Tue, 19 Nov 2019 08:30:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=8iQPLPupyUKebkOfve4d3AfXu2aQmmtdgB98aJBCNEM=; b=I3S19nj6s6kmfEZNa2or7KvcXmUdpr8SXvoQHh5FMeS1mWdvSTT0phrd53kW25MVyR MSSlw6ncjHJ+z4UKFtO0mBfRoDeoiyHbdyOKKi7IH7oM7xbimVPqlMxIzy4NROsQVvr5 NlvbyQgEBPaO6HyGSnfRFejz108xiRVrYua+0MCspRDzXTpbvSPLstkv+X+6X3UCsqN4 cym7uBbA3i0un5n9e/qJUbGIabvk9J5CGXuzHGbwOx5ak7ZF6eCnAbra1Yz/uWPHLk+D Cp97ZG+i6tovovkw4yYcQ/nhzyvyV89WIZ3kTuBAbSLmqzuqWJJ2UFiytcMqHHGwKHLk 7TRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=8iQPLPupyUKebkOfve4d3AfXu2aQmmtdgB98aJBCNEM=; b=T34IZtLZpHUnwF/Xwcc+ATcDhUhBWDzZx+xXskn2Q4r2X7LjVGNrPcz8XAw8+2gdX2 b4yiq+eiHIpjfmGid83W3a6epuQIGibNJpORaNt2pZNd8OeDfBbtg0zw9y2L7TBlMCS9 +0KfLDpQsVKLJe9opRbeWpGuXMPgGkBRpxoi302VwROQ7Kc4gpLdYJ9SERNF2fu7OlbV /bVUuxU3KJ2rw8u+nw88J8WG7qtdpDoJJ6qQL6H1IKha6lzCcYhyt2zxCCLh4eG41NMK 7DfUO+HeK31ptp52JBQhyGNmZEqvvJDDQWBGXZ5p2MpozQIyL9rmz0KVW++X1GflEZp4 3n0w== X-Gm-Message-State: APjAAAUAaQXsEJ5SRzzkJb3/Y2T3cFnU612+QmKT/djGP5n4f3JELEwC cqxFmaJqA3IBkvrDxJlw07fNaA== X-Google-Smtp-Source: APXvYqwGPuwLYhHkFftd0l/Z25DYAtft1YRCftdKrqWyPe+GL5ls5m4lvv38jRW3RV9+tTZy3i961A== X-Received: by 2002:adf:f743:: with SMTP id z3mr36926883wrp.200.1574181007920; Tue, 19 Nov 2019 08:30:07 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.67.47]) by smtp.googlemail.com with ESMTPSA id p14sm28671573wrq.72.2019.11.19.08.30.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Nov 2019 08:30:06 -0800 (PST) Subject: Re: [edk2-devel] [edk2-platforms][PATCH 1/8] Platform/RPi: Add model family detection To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20191114160740.10072-1-pete@akeo.ie> <20191114160740.10072-2-pete@akeo.ie> <20191118175156.GX7323@bivouac.eciton.net> <99b30bf5-a9c6-62aa-f7e2-7db9c2bc9848@akeo.ie> <20191118180521.GY7323@bivouac.eciton.net> <3e51f090-9391-1c59-5a64-c2927011ccb1@akeo.ie> From: "Pete Batard" Message-ID: <6dbfd653-ab36-09f4-2f0d-8a9675ed5fae@akeo.ie> Date: Tue, 19 Nov 2019 16:30:05 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 2019.11.19 15:07, Ard Biesheuvel wrote: > On Mon, 18 Nov 2019 at 19:32, Pete Batard wrote: >> >> On 2019.11.18 18:05, Leif Lindholm wrote: >>> On Mon, Nov 18, 2019 at 05:58:05PM +0000, Pete Batard wrote: >>>> On 2019.11.18 17:51, Leif Lindholm wrote: >>>>> On Thu, Nov 14, 2019 at 04:07:33PM +0000, Pete Batard wrote: >>>>>> From: Samer El-Haj-Mahmoud >>>>>> >>>>>> Add GetModelFamily to RASPBERRY_PI_FIRMWARE_PROTOCOL. >>>>>> >>>>>> This uses the board revision to return a numeric value representing >>>>>> the RPi family (1=RPi, 2=RPi2, 3=RPi3 and 4=RPi4). >>>>>> >>>>>> Knowing the Pi family will help us set the SD card routing when we >>>>>> introduce support for the Pi 4 and should also be easier to maintain >>>>>> than if using individual model detection. >>>>>> >>>>>> Also add a missing entry for the "Raspberry Pi Compute Module 3+" in >>>>>> RpiFirmwareGetModelName (). >>>>> >>>>> Can you drop the above line and include the below as 1/? in v2? >>>> >>>> Okay. >>>> >>>> Note that since you requested alphabetical for PCDs, I'm going to have an >>>> "Also" in 2/ (now 3/) since the existing PCDs in >>>> Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf are out of >>>> alphabetical order. >>> >>> Actually, I try to never request reordering of existing lines, so I >>> would be quite happy for you to skip the changes that would motivate >>> the use of the "also". >>> >>> I tend to apply a rule of trying to insert *new* (or moved) lines in a >>> way that will improve the existing order - or in messy cases at least >>> not make it worse. >>> >>> I have had it pointed out to me that this is maybe not entirely >>> obvious... >> >> Well, this is exactly what I would point out as an example of the strive >> for commit atomicity getting in the way of a more readable codebase as >> well as overall user experience (the users here being the developers who >> are dealing with the code). The reason I'm pointing this out is that, in >> the past, I have been dealing with projects that seemed to care more >> about keeping a squeaky clean commit history than they seemed to care >> about making the underlying code as good as it could possibly get, which >> resulted in increased pain for the developers having to contend with >> said codebase and ultimately end-users of the software produced from >> that codebase. >> >> Again, I would assert that there has to exist a middle ground between >> keeping a super-clean commit history and improving the source where it >> can indeed be improved at little cost, by not always defaulting to >> people having to devote extra time splitting patches. >> >> But I understand this is not my choice to make here. Thus I'll stay away >> from reordering that doesn't have to do with new PCDs being introduced. >> > > Please keep in mind that when open source maintainers take ownership > of your code, they assume the responsibility to ensure that it doesn't > get broken by future updates elsewhere in the codebase, often way > beyond the commercial lifetime of the product that is supported by > that code. This is a sizable effort, and an important part of managing > that effort is ensuring that the code is in an acceptable shape to > begin with, and what 'acceptable' means differs between different > maintainers. Not being able to revert a patch easily because it > touches unrelated code may make our lives more difficult years after > you have stopped caring about this platform entirely. I think you are actually exposing the root of the problem without realizing it here. Elements that may make a maintainer's life more difficult years after the contributor stopped working on it can actually be elements that makes, and will continue to make, a whole lot of developers' lives much easier right now. For instance, someone today or tomorrow (rather than 2 or 5 years down the line) can very well copy from code that got rejected as an "Also" (say, the one instance I found in the Pi source where a %s was used instead of a %a, which is an easy thing to miss if you're not paying attention) and find out they are wasting time on an issue that they would never have had to contend with, had the EDK2 maintainership been flexible with regards to what might be acceptable to piggyback on a patch that pertains to a specific file (IMO, fixing typos or style should always be acceptable as a piggyback, and I'd really like to hear how including such changes is effectively going to make the maintainers' job that harder down the line). And though this is a not directly related issue, I could also speak volumes on how myself, and I assume many, many other developers, have wasted countless hours (my current estimate puts that to around 4 to 5 hours in my case) on the current CRLF enforcing situation with the EDK2 codebase. All this to re-state that I wish there existed a balance between the well established needs of the maintainers, and what they envision might emerge as issues in the long run (which I assert tends to encourage them to preserve an existing status-quo), and the possibly not so well publicized pain points and time wastage that consumers of the codebase encounter, who, of course (and, depending on how this discussion goes, I might come to see as perhaps the wisest choice) generally tend to avoid venting their frustration on a mailing list that aims at concerning itself solely with technical discussions... In other words, if you are willing to consider how much more painful allowing the piggybacking of low-hanging "Also"'s onto existing patch may make your life as a maintainer down the line, please also be willing to envision the scenarios in which not allowing the same thing might actually be making the life of people who work with the codebase, and I'd really like to stress out that I'm really not talking only about myself here, harder right now. Regards, /Pete > > >