[coreboot] [PATCH] flashrom - board enable - reconstruct table.

[Luc Verhaegen]

On Mon, Apr 20, 2009 at 01:17:43PM +0200, Carl-Daniel Hailfinger wrote:
> On 20.04.2009 12:12, Luc Verhaegen wrote:
> > Flashrom: board_enables: reconstruct table.
> >
> > This patch restores the pciid based board matching table. It makes this
> > table readable and hackable again, and the only disadvantage is that the
> > right margin is way beyond the rather dogmatic 80. All 0x0000 pci ids have
> > been string replaced by 0 to more easily spot missing ids, and extra
> > comments have been added to explain how the various entries are used.
> >
> > Signed-Off-By: Luc Verhaegen <libv at skynet.be>
> >   
> 
> The extra comments are good, but "coreboot bios" in such a text really
> hurts. Please change the word BIOS to all-caps and don't refer to
> coreboot as a BIOS. Thanks.

These are the lines in question:

* The coreboot ids are used two fold. When a coreboot bios is installed, it
* uniquely matches the coreboot board identification string. When a legacy
* bios is installed and when autodetection is not possible, these ids can be
* used to identify the board through a command line argument.

It is clear and concise.

Let's take the other option: "when coreboot is installed" What does that 
mean? Did the user check out the tree, did he install some of the utils, 
or did he install a coreboot bios image into his rom?

Your statement is more about politics than anything else.

> The documentation part can be committed after one more iteration.

I would never have altered these comments if it wasn't for the previous 
flamewar. For those just tuning in, this is the thread in the 
mailarchives: 
http://www.coreboot.org/pipermail/coreboot/2009-January/044086.html

> The rest of the patch makes board entries unreadable.

We had this one before. It makes it unreadable for you. But not for 
everyone. Today, with widescreen panels, it is easier to widen your 
editor and see the whole line under the condensed table (and currently, 
see the whole table!), than it is to see more than 2 entries under the 
unraveled one. All you have to do is get over the 80chars per line 
dogma. This is one of the few situations where imho it should be 
allowed, it is actually the only one that i can name where i would 
defend it.

> In the past few weeks, we had quite a few people in #coreboot who wanted
> to add support for their boards. The presence of struct member names
> helped immensely to explain how to add board enables. Some even figured
> it out from similar entries. Lowering the barrier for possible future
> developers is very important.

What makes you so certain that it is this change that made people add 4 
entries in 3 months? Couldn't that be because flashrom just is getting 
used more? And besides, this growth is not significantly faster than 
before.

> Since IIRC you said some time ago that the multiline layout hinders your
> workflow, feel free to submit any new board enables in the single-line
> layout. I'll fix them up.

Why is my workflow relevant? Do you really think that i have all my 
editors open at 160 chars all the time? My you are referring to my 
statements about being able to oversee everything more quickly with a 
condensed table. This is just common sense, everybody will have to 
acknowledge that one.

What about your workflow? How often have you added entries to this 
table?

Actually, since i have dealt with such tables in my graphics drivers 
before (and have done so for 5 years no, hence me creating this table 
in the first place); How often have you dealt with pci id based matching 
tables?

What will you do when it reaches a hundred or so entries? Will you be 
happy with a 1400line mess? Do you think that this table will stop 
growing at some point?

How many actual board enable functions will there be with multiple 
entries in the table? How much space will the actual enables take 
compared to the table? 

How will people tell whether something similar has already been added? 

Will you impose some entry ordering scheme (which, in light of likely 
future table growth, is actually a useful change)?

It rapidly becomes non-obvious when you don't have a tight table. And i 
know what you are going to say here: just make sure that they are added 
in whatever order already. And my answer to that is: you can't. Left or 
right, entries will be added that aren't ordered, and afterwards no-one 
will spot them being unordered anymore. In a tight table, you spot them 
right away and can fix it easily on the next iteration.

It really is creating a situation where you are unable to see the forest 
through the trees.

Also, you originally suggested to remove lines like = 0x0000; or = NULL; 
This is a very dangerous path. People will only see the last 2-3 
entries, and never look beyond. Remove a line, and it will not be 
thought of for the next entry. This is simply human behaviour.

What you then end up with is a whole range of boards where just a single 
pair of main ids, the coreboot ids, the string and the enable will be 
created. No autodetection will be provided any more, as the trivial 
addition of yet another argument-specified-only board is of course 
obvious for the person who added the board enable to begin with.

The end result then is that all users will just randomly go through the 
board names for ages, all the time, some of which will work, as the 
amount of pci devices really isn't that great. This is quite a dangerous 
situation and a really bad user experience.

With a condensed table, people are forced to pad, and will put more 
thought into their entries and will feel obliged to add subsystem ids.
This in turn leads to more autodetection and less people blindly forcing 
boardnames, and in general a better user experience.

So maybe making it "easier" for people to "understand" what the 
different entries are for (which actually boils down to being able to 
ignore how the entries are used), is not a goal that should be pursued 
above other goals. For existing developers, at least those willing to 
not religiously stick to dogmas when it makes sense, the condensed table 
is the better option.

Luc Verhaegen.