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.
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
