[flashrom] [PATCH] flashrom 0.9.2

[Michael Karcher]

> @@ -42,10 +45,11 @@
>  option is
>  .B not
>  recommended, you should only use it if you know what you are doing and you
> -feel that the time for verification takes too long.
> +feel that the time for verification takes too long. Please note that flashrom
> +will automatically verify all operations unless you specify \-\-noverify.
Reword the whole paragraph instead of adding the note, like
-n, --noverify
  Skip the automatic verification after writing...
Especially, I don't like "all operations", as the verify is only implied
for write, but read and erase are also "operations".

> +	       /* FIXME: --mainboard should be a programmer parameter */
Right.

> +	       "   -f | --force                      force something\n"
"force specific operations (see man page)" ?

> +	       "   -n | --noverify                   don't auto-verify\n"
> +	       "   -l | --layout <file>              read ROM layout from "
> +	       "file\n"
Style: I would indent "file" up to "read ROM layout". Your choice.

> +#if 0
> +	/* FIXME: This is broken. printf_debug is a no-op because verbose=0
> +	 * at this point.
> +	 */
>  	if (argc > 1) {
>  		/* Yes, print them. */
>  		printf_debug("The arguments are:\n");
>  		for (i = 1; i < argc; ++i)
>  			printf_debug("%s\n", argv[i]);
>  	}
> +#endif
Throw it out completely. Whoever just wants to see the parameters
flashrom is called with should write a wrapper script.

> -				exit(1);
> +				cli_classic_abort_usage(argv[0]);
Don't dump the usage on syntax errors. It clutters output too much so
that you don't find the error message. Just print a one-liner like 'run
"flashrom --help" for usage info'. I especially liked the debian tool
that outputs "You need --help.", but don't recommend this very phrase to
flashrom.

>  			if (programmer == PROGRAMMER_INVALID) {
>  				printf("Error: Unknown programmer %s.\n", optarg);
> -				exit(1);
> +				cli_classic_abort_usage(argv[0]);
>  			}
In this case, list the supported programmers (and just them).

> -		msg_cinfo("VERIFIED.          \n");
> +		msg_cinfo("VERIFIED.\n");
The spaces probably were to clear address counter characters. Did you
verify that it is not needed with current chip drivers?

The remaining stuff looks good, you get a tentative
Acked-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

Regards,
  Michael Karcher