Re: [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

Regards,
  Michael Karcher