Am Mittwoch, den 26.05.2010, 17:54 +0400 schrieb Peter Lemenkov:
I fully appreciate the idea of merging the tables from print.c and print_wiki.c . Do you know http://patchwork.coreboot.org/patch/1044/ which I submitted with basically the same idea?
Unfortunately I wasn't aware of your patch - sorry for stealing your idea. That was unintentional.
No problem. Just wanted to point out that you might want to have it look at it.
Is it *really* necessary to put this data table into a header file? It means it will be included *twice* if you want print_wiki support.
Moved all data to board_enable.c to avoid duplicating.
Now, I think technically your patch is good, but we are entering political territory. I advice to use print.c, really. The oppinions in the flashrom project about whether board URL/note info is a good idea in the source code differ. This is the reason we currently have the split. The pro-URL guys may do what they want in print_wiki.c, and the contra-URL guys just don't look at that file, pretending it doesn't exist; at least they don't see them and don't care. This is obviously a way of handling the tables which is doomed to be deynchronized quickly. The URL-handling code thus has been written to work fine and list boards in the Wiki, even if no URL entry has been made yet.
There are many good points in merging the two tables, and I think there is a latent consensus in that it is sensible to have the tables merged. Now, board_enable.c. This file is meant to be just about board enabling. The king (please take no offense!) of this file rejects adding stuff to this file not related to boards that need a board enable, so adding to that file is probably a no-go. As print.c already lists all the boards, I think print.c is the better location.
Another change to current code is that you code will unconditionally include all the URL strings into the executable, even if wiki printing is not enabled, without ever using them.
Fixed. I used the idea from your patch, mentioned above.
Great.
For the "working" field, please use OK/NT instead of 1/0 to be consistent with other parts of flashrom.
Done.
I didn't tell you last time, but having the working field behind the URL/note field in the line moves it out of the 80 column screen. The contra-URL fraction might be annoyed by it. Please put URL/note last in the structure.
Whether you *need* an option for the board does not depend on whether a coreboot ID for that board is present, but on the set of PCI IDs in the board enable table, read the comment in board_enable.c, please
I don't fully understand - the idea of this particular chunk of code is just to know whether board_enable is required by using its name, and not by its pciids (we're just traversing board's names list).
This is completely right and should be done that way.
After finding that the mobo does require board_enable (by looking at the vendor_name and board_name fiends in board_pciid_enables), I'm trying to find whether autodetection is possible (empty coreboot ids)
But this is wrong. empty coreboot IDs mean that the board is not detected using coreboot IDs. *Usually* that means the board is autodetected using PCI IDs. There might be board entries for boards with coreboot IDs that can be autodetected without, though. Especially since we have DMI matching. This makes it possible to have the board detected by PCI dev IDs (w/o subsystem IDs) + DMI on vendor BIOS and PCI dev IDs (w/o subsystem IDs) + coreboot IDs on coreboot.
and if it's not possible, then I construct appropriate command-line switch for that board (from coreboot ids). Perhaps I'm missing something here, but I was sure that I don't need to look at the other fields of board_pciid_enables except mentioned above.
Autodetection is (IIRC) possible if - 2 device IDs are present AND - 1st ID set is complete (including subsystem IDs) OR - DMI info is present
Anyway, here is a new patch attached (I completely get rid of board.h introduced in previous version of patch).
Which seems like a good thing.
Thanks for your work, Michael Karcher