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

[Peter Lemenkov]

Hello!

2010/5/26 Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>:

> 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.

> 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.

> 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.

> For the "working" field, please use OK/NT instead of 1/0 to be
> consistent with other parts of flashrom.

Done.

>> +             if(boards[i].write_enable){
> You can omit storing the "write enable" flag by just traversing the
> board_pciid_enable table for each board, and print "  -" if the board is
> not listed in the table. This circumvents another source of possible
> inconsistencies.

Fixed it too.

>> +                     for (j = 0; b[j].vendor_name != NULL; j++) {
>> +                             if ((strcmp(b[j].vendor_name, boards[i].vendor) == 0) && (strcmp(b[j].board_name, boards[i].name) == 0)){
>> +                                     if (b[j].lb_vendor != NULL)
>> +                                             printf("-m %s:%s", b[j].lb_vendor, b[j].lb_part);
>> +                                     else
>> +                                             printf("(none, board is autodetected)");
> 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).
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)
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.

Anyway, here is a new patch attached (I completely get rid of board.h
introduced in previous version of patch).

-- 
With best regards, Peter Lemenkov.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Unify-board-info-handling.patch
Type: application/octet-stream
Size: 70809 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20100526/00510364/attachment.obj>