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

[Uwe Hermann]

Hi,

quick review below, I think this patch needs one more round.

I'm a bit undecided whether it's really a good idea to merge the tables
for good and bad boards, I kinda liked them being separate tables. But I
guess for users it's more intuitive to have one big table.


On Wed, May 26, 2010 at 09:37:59PM +0400, Peter Lemenkov wrote:
> Ok, here is a new patch (I still don't ubderstand the proper way I should
> change section, responsible for printing information about board_enable):

Hm, I don't understand what you try to say here.

 
> -	printf("\nSupported boards which need write-enable code (total: %d):"
> -	       "\n\nVendor:                  Board:                        "
> -	       "Required option:\n\n", boardcount);
> +	printf("\nKnown %s (total: %d):"

We need to explicitly list the numbers of (1) supported, (2) unsupported
boards (at least) here, the total number of "known" boards alone is not
useful and will confuse users.


> +	       "\n\nVendor:                  Board:                        Status:    Write-enable option?"

Please check the "flashrom -L" output, I think the last column output is
not always correctly aligned.

Also, maybe change "Write-enable option?" to "Required option:".


> +		if(boards[i].working == OK)
> +			printf("  good     ");
>  		else
> -			printf("(none, board is autodetected)\n");
> -	}
> +			printf("  BAD!     ");

I'd prefer "OK / BROKEN" or "OK / --BAD--" or something like that here,
but definately "OK" for the good boards, to be consistent.

Hm, actually "BAD" is not quite correct, it's more like "UNTESTED", the
board-enable may very well work fine, it's just not yet tested.
Do you differ between actually broken and just untested in the
tables/code?


> +	BOARD_ENTRY( "A-Trend",		"ATC-6220",			OK,	"http://www.motherboard.cz/mb/atrend/atc6220.htm",	NULL ),
> +	BOARD_ENTRY( "Abit",		"AX8",				OK,	"http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?DEFTITLE=Y&fMTYPE=Socket%20939&pMODEL_NAME=AX8",	NULL ),
> +	BOARD_ENTRY( "Abit",		"Fatal1ty F-I90HD",		OK,	"http://www.abit.com.tw/page/de/motherboard/motherboard_detail.php?pMODEL_NAME=Fatal1ty+F-I90HD&fMTYPE=LGA775",	NULL ),

Minor nitpick for the table:
To save horizontal space, maybe change "BOARD_ENTRY" to "B", doesn't
have to be really readable here, the table entries and header comment
speak for themselves in this case (IMHO).

Tabs for separating the first few entries is OK, but I'd use exactly one
space (not tab) after the "OK" entry and after the URL. Alignment will
automatically still be enforced for those columns where it's possible
(reasonably). The URL / comment fields don't make much sense to try to
align them and the tabs there just waste horizontal space.


> diff --git a/print_wiki.c b/print_wiki.c
> index 3d51bfa..cb90919 100644

Please check the wiki output (flashrom -z), it's partly broken right
now. You can test by pasting the -z output into any wiki page (and click
"preview", but please do not _save_ that test page).

  
> +		printf("|- bgcolor=\"#%s\"\n| %s || %s%s %s%s || %s%s%s%s || {{%s}}\n",
> +				(color) ? "eeeeee" : "dddddd",
> +				boards[i].vendor,
> +				boards[i].url ? "[" : "",
> +				boards[i].url ? boards[i].url : "",
> +				boards[i].name,
> +				boards[i].url ? "]" : "",
> +				b[k].lb_vendor ? "-m " : "—",
> +				b[k].lb_vendor ? b[k].lb_vendor : "",
> +				b[k].lb_vendor ? ":" : "",
> +				b[k].lb_vendor ? b[k].lb_part : "",
> +				(boards[i].working == OK) ? "OK" : "No");

The wiki tables get a bit wide if every board has an "-m foo:bar" column,
but it's OK for now, I think I have an idea how this can be improved in
an additional patch afterwards.


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org