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 ");
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...", NULL ),
- BOARD_ENTRY( "Abit", "Fatal1ty F-I90HD", OK, "http://www.abit.com.tw/page/de/motherboard/motherboard_detail.php?pMODEL_NAM...", 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.