[coreboot] [PATCH] flashrom: Add -z for wiki syntax output (try 3)

[Carl-Daniel Hailfinger]

On 17.06.2009 16:10, Uwe Hermann wrote:
> Add a --list-supported-wiki / -z option which outputs the currently
> supported flash chips (and their status, size, and type), chipsets
> (plus status), mainboards (plus status), and external PCI devices
> usable as programmer to stdout.
>
> This allows for very easy pasting into the http://coreboot.org/Flashrom
> page, so we can keep that page up-to-date without much hassle.
>
> The list of boards is mostly new (known good ones which don't need
> write-enable code, and known-bad ones) and also lists URLs to the
> vendor's mainboard pages.
>
> Signed-off-by: Uwe Hermann <uwe at hermann-uwe.de>
>   

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
but see the comments below.

> Index: flashrom.8
> ===================================================================
> --- flashrom.8	(Revision 601)
> +++ flashrom.8	(Arbeitskopie)
>   

The --list-supported-wiki is missing in the synopsis section.

> @@ -123,6 +123,12 @@
>  to test an ERASE and/or WRITE operation, so make sure you only do that
>  if you have proper means to recover from failure!
>  .TP
> +.B "\-z, \-\-list\-supported-wiki"
>   

It would be really great if we could avoid short options for such
seldom-used features. However, that's a general TODO for some options
and we can try that later.

> Index: flashrom.c
> ===================================================================
> --- flashrom.c	(Revision 601)
> +++ flashrom.c	(Arbeitskopie)
> @@ -483,8 +483,8 @@
>  void usage(const char *name)
>  {
>  	printf("usage: %s [-VfLhR] [-E|-r file|-w file|-v file] [-c chipname] [-s addr]\n"
> -	       "       [-e addr] [-m [vendor:]part] [-l file] [-i image] [-p programmer] [file]\n\n",
> -	       name);
> +	       "       [-e addr] [-m [vendor:]part] [-l file] [-i image] "
> +	       "[-p programmer] [file]\n\n", name);
>   

[file] at the end should disappear. It's not your fault, but I just
noticed it. Will send a patch.


> @@ -505,12 +505,14 @@
>  	     "   -l | --layout <file.layout>:      read ROM layout from file\n"
>  	     "   -i | --image <name>:              only flash image name from flash layout\n"
>  	     "   -L | --list-supported:            print supported devices\n"
> +	     "   -z | --list-supported-wiki:       print supported devices in wiki syntax\n"
>  	     "   -p | --programmer <name>:         specify the programmer device\n"
> -	     "                                     (internal, dummy, nic3com, satasii, it87spi, ft2232spi)\n"
> +	     "                                     (internal, dummy, nic3com, satasii,\n"
> +	     "                                     it87spi, ft2232spi)\n"
>  	     "   -h | --help:                      print this help text\n"
>  	     "   -R | --version:                   print the version (release)\n"
> -	     "\nYou can specify one of -E, -r, -w, -v or no operation.\n"
> -	     "If no operation is specified, then all that happens"
> +	     "\nYou can specify one of -E, -r, -w, -v or no operation. "
> +	     "If no operation is\nspecified, then all that happens"
>  	     " is that flash info is dumped.\n\n");
>   

Can you kill the two-line change above? \n in the middle of a string
seriously hampers readability. Since we violate the 80-column
restriction in the code (but not the messages) above anyway, you could
split the strings at the \n marker if you really want to perform
cosmetic changes.


> Index: print.c
> ===================================================================
> --- print.c	(Revision 601)
> +++ print.c	(Arbeitskopie)
> @@ -222,3 +222,340 @@
>  	       "(total: %d):\n\n", boardcount);
>  	print_supported_boards_helper(boards_bad);
>  }
> +
> +#define CHIPSET_TH "{| border=\"0\" style=\"font-size: smaller\"\n\
> +|- bgcolor=\"#6699dd\"\n! align=\"left\" | Vendor\n\
> +! align=\"left\" | Southbridge\n! align=\"left\" | PCI IDs\n\
> +! align=\"left\" | Status\n\n"
>   

I'd really like to see const char *chipset_th instead of the #define.
First, it decreases the size of the binary. Second, we are safe against
unintended format strings in chipset_th (and others) if we specify them
as printk arguments after the format string.


> +	{ "ASUS",		"A8NE-FM/S",		"http://www.hardwareschotte.de/hardware/preise/proid_1266090/preis_ASUS+A8NE-FM" },
>   

Is that link to a pricing comparison website intentional?

I have to admit that I have a hard time parsing all the wiki syntax and
table emitting code, but I trust you on this.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/