Re: [flashrom] [PATCH 1/7] Introduce and use enum test_state

[Carl-Daniel Hailfinger]

Am 03.03.2012 21:11 schrieb Stefan Tauner:
> Previously boards in the wiki were tagged either as working or as known
> bad. But we added support to various boards via board enables that were
> then never tested because the owners have not reported back. This can
> now be tagged with NT and is shown appropriately.
>
> Also, the underlying data structure indicating state was converted from
> macros to an enum while preserving original integer values.
>
> Because all lines specifying supported boards and laptops were touched
> anyway, this patch also re-indents them.
>
> TODO: change other occurrences to use it.  wanted to get feedack first.
>
> Signed-off-by: Stefan Tauner
>
> diff --git a/print_wiki.c b/print_wiki.c
> index 377154d..9a9cd83 100644
> --- a/print_wiki.c
> +++ b/print_wiki.c
> @@ -136,9 +136,9 @@ static void wiki_helper(const char *devicetype, int cols,
>    const struct board_match *b = board_matches;
>
> for (i = 0; boards[i].vendor != NULL; i++) {
> if (boards[i].working)
> if (boards[i].working == OK)
> boardcount_good++;
> else
> if (boards[i].working == BAD)
> boardcount_bad++;

You could replace that construct with a switch(), and it might make
sense to count untested boards as well.
> }
>
> @@ -171,7 +171,8 @@ static void wiki_helper(const char *devicetype, int cols,
>    	       b[k].lb_vendor ? b[k].lb_vendor : "",
>    	       b[k].lb_vendor ? ":" : "",
>    	       b[k].lb_vendor ? b[k].lb_part : "",
>        (boards[i].working) ? "OK" : "No");
>        (boards[i].working == OK) ? "OK" :
>        (boards[i].working == NT) ? "?3" : "No");

The ?3 looks odd, but I assume you tested it.
> if (boards[i].note) {
> 	printf("<sup>%d</sup>\n", num_notes + 1);
>
> diff --git a/print.c b/print.c
> index 1fdeac7..544a846 100644
> --- a/print.c
> +++ b/print.c
> @@ -545,428 +545,427 @@ void print_supported(void)
> [...]
> B("ZOTAC",	"Fusion-ITX WiFi (FUSION350-A-E)", 1, NULL, NULL),
> B("ZOTAC",	"GeForce 8200",		1, "http://pden.zotac.com/index.php?page=shop.product_details&product_id=129...", NULL),
> B("ZOTAC",	"H67-ITX WiFi (H67ITX-C-E)", 0, NULL, "Probing works (Winbond W25Q32, 4096 kB, SPI), but parts of the flash are problematic: descriptor is r/o (conforming to ICH reqs), ME region is locked."),
> B("ZOTAC",	"ZBOX HD-ID11",		1, "http://pdde.zotac.com/index.php?page=shop.product_details&product_id=240...", NULL),
> B("A-Trend",	"ATC-6220",				OK,	"http://www.motherboard.cz/mb/atrend/atc6220.htm", NULL),
> B("abit",	"A-S78H",				OK,	"http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?pMODEL_NAM...", NULL),
> B("abit",	"AN-M2",				OK,	"http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?DEFTITLE=Y...", NULL),
> B("abit",	"AV8",					OK,	"http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?DEFTITLE=Y...", NULL),

Is it possible that this patch made the board table one or two tabs
wider? The patch looks like that, and while I agree that some
files/sections should not have line length limits, adding another 16
columns of whitespace is something I'd like to avoid.

print_wiki related code is something I rarely touch (except for
programmer additions), so please don't expect in-depth reviews from me.
A cursory review suggests that the patch at least doesn't make the code
worse and I don't have any strong feelings about this code. If you feel
this patch is beneficial, I can send a weak Acked-by, more like Meh-by.

Regards,
Carl-Daniel

[PATCH 1/7] Introduce and use enum test_state

[Stefan Tauner]

On Sat, 03 Mar 2012 23:32:23 +0100
Carl-Daniel Hailfinger  wrote:
> Is it possible that this patch made the board table one or two tabs
> wider? The patch looks like that, and while I agree that some
> files/sections should not have line length limits, adding another 16
> columns of whitespace is something I'd like to avoid.

in general the url is the source of length problem.
does making the table longer by putting the URL in a line
of its own make sense?
i personally would drop the URL anyway.. useless, partially misleading
(for boards with multiple variants) and work for me (only, as i must
emphasize :)
iirc uwe was strongly against this, but i would be lying if would say
that his matters much to me in this case (because the tested stuff
branch is the most boring work i do for this project, not because i dont
respect his view).

[PATCH 1/7] Introduce and use enum test_state

[Carl-Daniel Hailfinger]

Am 28.07.2012 16:30 schrieb Stefan Tauner:
> On Sat, 03 Mar 2012 23:32:23 +0100
> Carl-Daniel Hailfinger  wrote:
> > Is it possible that this patch made the board table one or two tabs
> > wider? The patch looks like that, and while I agree that some
> > files/sections should not have line length limits, adding another 16
> > columns of whitespace is something I'd like to avoid.
>
> in general the url is the source of length problem.
> does making the table longer by putting the URL in a line
> of its own make sense?

It's certainly a novel idea, and it has some appeal.
> i personally would drop the URL anyway.. useless, partially misleading
> (for boards with multiple variants) and work for me (only, as i must
> emphasize :)

Yes, now that Uwe is essentially a fulltime sigrok developer, he's not
maintaining that stuff anymore.
What do you think about asking someone else to do the tested_stuff
maintenance?
> iirc uwe was strongly against this, but i would be lying if would say
> that his matters much to me in this case (because the tested stuff
> branch is the most boring work i do for this project, not because i dont
> respect his view).

Back then the policy was accepted because he handled the board testing
maintenance. We might have to rethink this.

Regards,
Carl-Daniel

[PATCH 1/7] Introduce and use enum test_state

[Stefan Tauner]

On Sat, 03 Mar 2012 23:32:23 +0100
Carl-Daniel Hailfinger  wrote:
> Am 03.03.2012 21:11 schrieb Stefan Tauner:
> > Previously boards in the wiki were tagged either as working or as known
> > bad. But we added support to various boards via board enables that were
> > then never tested because the owners have not reported back. This can
> > now be tagged with NT and is shown appropriately.
> >
> > Also, the underlying data structure indicating state was converted from
> > macros to an enum while preserving original integer values.
> >
> > Because all lines specifying supported boards and laptops were touched
> > anyway, this patch also re-indents them.
> >
> > TODO: change other occurrences to use it.  wanted to get feedack first.
> >
> > Signed-off-by: Stefan Tauner
> >
> > diff --git a/print_wiki.c b/print_wiki.c
> > index 377154d..9a9cd83 100644
> > --- a/print_wiki.c
> > +++ b/print_wiki.c
> > @@ -136,9 +136,9 @@ static void wiki_helper(const char *devicetype, int cols,
> >    const struct board_match *b = board_matches;
> >
> > for (i = 0; boards[i].vendor != NULL; i++) {
> > if (boards[i].working)
> > if (boards[i].working == OK)
> > boardcount_good++;
> > else
> > if (boards[i].working == BAD)
> > boardcount_bad++;
>
> You could replace that construct with a switch(), and it might make
> sense to count untested boards as well.

i could, but it would increase the line count by 100%, untested board
count was added.
> > }
> >
> > @@ -171,7 +171,8 @@ static void wiki_helper(const char *devicetype, int cols,
> >    	       b[k].lb_vendor ? b[k].lb_vendor : "",
> >    	       b[k].lb_vendor ? ":" : "",
> >    	       b[k].lb_vendor ? b[k].lb_part : "",
> >        (boards[i].working) ? "OK" : "No");
> >        (boards[i].working == OK) ? "OK" :
> >        (boards[i].working == NT) ? "?3" : "No");
>
> The ?3 looks odd, but I assume you tested it.

it is a mediawiki template:
http://flashrom.org/index.php?title=Template:%3F3&action=edit
> […]
> Is it possible that this patch made the board table one or two tabs
> wider? The patch looks like that, and while I agree that some
> files/sections should not have line length limits, adding another 16
> columns of whitespace is something I'd like to avoid.

reverted that part...

i have committed a refined version now in r1555, thanks for your
suggestions!