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

[Michael Karcher]

Am Mittwoch, den 26.05.2010, 10:51 +0400 schrieb Peter Lemenkov:
> With this patch boards info is stored in one common place. Previously, the
> information about different boards was spreaded between few different places,
> and manual synchronization was required (which already led to some discrepancies
> between them). So with this patch marking new boards as working based on user's
> status reports would me much easy than before.

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?
> --- /dev/null
> +++ b/board.h
> @@ -0,0 +1,235 @@
> +#ifndef __BOARD_H__
> +#define __BOARD_H__
>
> +#if INTERNAL_SUPPORT == 1
>
> +struct board_info {
> const char *vendor;
> const char *name;
> const char *url;
> const char *note;
> int working;
> int write_enable;
>
> +};

[...]
> +/* Please keep this list alphabetically ordered by vendor/board. */
> +const static struct board_info boards_known[] = {
> +#if defined(__i386__) || defined(__x86_64__)
> /* Verified working boards that don't need write-enables. */
> { "A-Trend",		"ATC-6220",		"http://www.motherboard.cz/mb/atrend/atc6220.htm", NULL, 1, 0 },

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

For the "working" field, please use OK/NT instead of 1/0 to be
consistent with other parts of flashrom.
> 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.
> 	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

Otherwise, I don't see any serious problems with your patch, but I have
neither tested nor thoroughly read all the code changes.

Regards,
  Michael Karcher