[flashrom] [PATCH] Add IT8705 autodetection

[Michael Karcher]

Am Mittwoch, den 07.07.2010, 14:16 +0200 schrieb Carl-Daniel Hailfinger:

> +	/* Non-default port requested? */
> +	portpos = extract_param(&programmer_param, "it87spiport", ",:");
> +	if (portpos) {
> +		char *endptr = NULL;
> +		if (strlen(portpos))
> +			forced_flashport = strtoul(portpos, &endptr, 0);
> +		/* Port 0, port >65535 and garbage strings are rejected. */
> +		if (!forced_flashport || (forced_flashport > 65535) ||
> +		    (endptr && (*endptr != '\0'))) {
I don't get the idea of the if in the beginning. The strtoul mangpage
states:
 "If there were no digits at all, strtol() stores the original value of
  nptr in *endptr (and returns 0)."
So you can just omit the "if (strlen(portpos))" test, because strtoul()
returns zero in this case (and doesn't change forced_flashport). If
strtoul is executed unconditionally, you can also throw away the check
for endptr not being NULL, as endptr is *always* set by strtoul.[1]

Alignment verification would be nice here, too. The datasheet should
state the required alignment.

> +	if (forced_flashport) {
> +		flashport = (uint16_t)forced_flashport;
> +		msg_pinfo("Forcing serial flash port 0x%04x\n",
> +			  flashport);
> +		sio_write(port, 0x64, (flashport >> 8));
> +		sio_write(port, 0x65, (flashport & 0xff));
> +	}
Can't you put this in an else branch of the if() shown above, or, in my
oppinion even better: Invert the condition to something like

if(parameter makes sense)
    set flashport & setup hardware
else
    print error message & exit(1)

The remaining stuff looks OK, but I didn't cross-check against the
IT8705 datasheet. Do you want a cross-check too?

Regards,
  Michael Karcher

[1]: It is superflous also right now, as !forced_flashport would have
short-circuited the *endptr test if strtoul was not called.