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