<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 18, 2015 at 2:47 PM, Julius Werner <span dir="ltr"><<a href="mailto:jwerner@chromium.org" target="_blank">jwerner@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">> I think nobody disagrees that type checking is a bad idea here.<br>
<br>
</span>I ain't not unsure that you failed to not make no mistake with the<br>
missing lack of double negatives there... ;)<br>
<span class=""><br>
> I don't think this argument makes sense for code that is being actively<br>
> developed in other code bases and imported into coreboot. Of course, if<br>
> you're importing stable code and don't expect much churn, tidying things up<br>
> is a fine idea. But increasing deltas while a project is still under active<br>
> development only serves to make integration and maintenance efforts more<br>
> troublesome and prone to error. It's not a productive use of anyone's time<br>
> when there are real bugs to solve.<br>
><br>
> Vendors often have code which they have already qualified and are<br>
> understandably reluctant to make any changes to it, even trivial aesthetic<br>
> ones. I'd like to make it easier for them to contribute directly to<br>
> coreboot, and throwing up artificial barriers does not help them gain<br>
> traction.<br>
<br>
</span>Do we really want to facilitate more of these wholesale imports of<br>
untouched, existing code dumps from other sources into coreboot?</blockquote><div><br></div><div>I don't think that's necessarily a bad thing during heavy development. Obviously we'd want to tidy things up after the fact and use wrappers until then, but insofar as this whole write32/writel and arg ordering discussion goes there hasn't been much to aspire to in terms of tidying things.</div><div><br></div><div>So basically what I'm advocating is to define a "proper" approach that we apply to coreboot in general, while being flexible about importing code. Once we define the proper approach, then applying an spatch to bring the style into conformance and remove the scaffolding should be relatively painless.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> It<br>
seems to me that those always end up bad for us... code is hard to<br>
read and follow because of switched conventions,</blockquote><div><br></div><div>Yep. And for the person porting the code, switching conventions on-the-fly might be even more confusing. I generally like to see code working before making such changes.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> it could have<br>
depended on different requirements for the environment than what<br>
coreboot provides, it often includes a lot of hacky and<br>
overcomplicated code that the original use case might have needed but<br>
we don't, people will end up making changes after the import that<br>
desync it with the source, etc.</blockquote><div><br></div><div>True, and I think it's more productive to prioritize fixing those issues over aesthetic ones.</div><div><br></div><div>I'm all for re-factoring code. I just don't think forcing huge deltas from the get-go during heavy development is a great way to do it.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> I think for small stuff like<br>
individual drivers we're better off just rewriting them with a sound<br>
design from the ground up tailored to our use case (at least that<br>
guarantees that someone really understood and thought through how it<br>
all works within the coreboot context).</blockquote><div><br></div><div>Agreed - For small stuff I'm totally on-board.</div><div><br></div><div>That might not be the case, however, for more complicated stuff.  For example, if a vendor updates DRAM init code in u-boot multiple times over a period of weeks/months and we need to apply the same updates to coreboot, having hundreds of cosmetic changes show up in the diff just makes the porting process more difficult and more prone to error.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> For really large scale<br>
external imports (like AMD Agesa), we can stow it away in vendorcode/<br>
with translator headers to allow it to keep its own conventions<br>
completely unchanged, without risk of it leaking out into the rest of<br>
coreboot.<br>
</blockquote></div><br>Yep.<br clear="all"><div><br></div>-- <br><div class="gmail_signature">David Hendricks (dhendrix)<br>Systems Software Engineer, Google Inc.</div>
</div></div>