[flashrom] [PATCH] Add IT8705 autodetection

[Michael Karcher]

Am Donnerstag, den 08.07.2010, 10:30 +0200 schrieb Carl-Daniel
Hailfinger:

> +	portpos = extract_param(&programmer_param, "it87spiport", ",:");
> +	if (portpos) {
> +		char *endptr = NULL;
> +		unsigned long forced_flashport;
> +		forced_flashport = strtoul(portpos, &endptr, 0);
> +		/* Port 0, port >65535 and garbage strings are rejected. */
> +		if (!forced_flashport || (forced_flashport > 65535) ||
> +		    (forced_flashport & 0xf007) || (*endptr != '\0')) {
ports below 0x100 should be rejected for PC hardware in my oppinion,
even if you can configure them. I also dislike the > 65536 test and the
seperate test for bits 12 to 15. There are two ways to write it I like
better, with (a) being in the spirit of what you have written, and (b)
is trying to be clever.

a) (forced_flashport < 0x100) || (forced_flashport >= 0x1000) ||
   (forced_flashport & 0x7)

b) (forced_flashport < 0x100) ||
   ((forced_flashport & 0xFF8) != forced_flashport)


> +			msg_perr("Error: it87spiport specified, but no valid "
> +				 "port specified. Port must be a multiple of 8 "
> +				 "and lie between 8 and 4088.\n");
Can't you print hex numbers here ("between 0x100 and 0xFF8")? strtoul
can parse them too. And I consider everyone crazy who specifies port
numbers in decimal.

> +			forced_flashport = 0;
Why this assignment? This is a local variable that goes out-of-scope
anyway.

Except for cross-checking the data sheet (Uwe, will you do?) I think the
patch is ready to be committed on the next iteration.

Regards,
  Michael Karcher