[flashrom] [PATCH 1/1] Unify board info handling

[Michael Karcher]

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