On 20.04.2009 14:28, Luc Verhaegen wrote:
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@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?
Coreboot is NOT a BIOS. If you really insist calling coreboot a BIOS, why not call it LinuxBIOS?
Your statement is more about politics than anything else.
There was almost universal agreement about the name change from LinuxBIOS to coreboot (well, I was unhappy initially, but I soon found out that the absence of BIOS in the new name is really beneficial). One of the reasons for the name change was that coreboot is NOT a BIOS nor should it ever be called one.
Feel free to propose calling coreboot a BIOS in a separate RFC on this list.
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
Ah yes, I forgot. In http://www.coreboot.org/pipermail/coreboot/2009-January/044129.html you promised to "create all board enable functions and table entries for the foreseeable future for everyone who asks once we switch back to the old layout [...] next to keeping X from degrading even further." Was that irony or a real promise?
The rest of the patch makes board entries unreadable.
We had this one before. It makes it unreadable for you. But not for everyone.
Well, more flashrom developers on this list favour the multiline solution than those opposed to it. That means I'm part of the majority. And I do help people on IRC add board enables.
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.
That's a straw man. I didn't claim any of the points you try to question above.
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?
If your workflow is irrelevant, why would we want to accommodate it?
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.
Unless you get to define everbody's common sense (which is inherently subjective), it is unlikely that everybody (in the strict sense) will have to acknowledge your view. After all, I'm a person (and thus part of the "everybody" group of people) and I disagree with you. You could claim that I don't have common sense, but I hope you won't do that.
[...] What will you do when it reaches a hundred or so entries? Will you be happy with a 1400line mess?
Is any table with 1400 lines a mess?
Do you think that this table will stop growing at some point?
Why should it? Hardware will probably have quirks for the foreseeable future.
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?
I expect at least half a dozen entries with flash translation chips because these boards exist out there.
How will people tell whether something similar has already been added?
Like they do now.
Will you impose some entry ordering scheme (which, in light of likely future table growth, is actually a useful change)?
Your patch doesn't touch that issue, so the point is orthogonal to our discussion.
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.
See above. There are lots of possible ordering schemes, but deciding which makes sense is a different matter.
It really is creating a situation where you are unable to see the forest through the trees.
Grep works nicely.
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.
So you suggest to keep these lines. Fine.
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.
Adding unique subsystem IDs to that struct is hard. First, I know for a fact that my laptop and a related model have identical subsystem IDs. That alone is proof that subsystem IDs are not always unique. Unless you know all subsystem IDs of all boards from a given manufacturer, you can't say for sure whether the board enable you're adding subsystem IDs for is actually _not_ going to trigger on another board. To make this even more fun, which subsystem ID will you use
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.
You must be joking. If you can _prove_ that, you'll probably get a PhD in psychology right away. If pressing tab once really forces people to think for minutes about something that is neither easy nor obvious, I want a tab key in every conversation.
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.
Let me modify one word of your statement to express my opinion:
For existing developers, at least those willing to not religiously stick to dogmas when it makes sense, the multiline table is the better option.
Regards, Carl-Daniel