Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44879 )
Change subject: support variable-size SPI chip for dummy programmer ......................................................................
Patch Set 2: Code-Review-1
(8 comments)
I don't like the parsing of the command-line parameters at all.
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@a379 PS2, Line 379: /* Will be freed by shutdown function if necessary. */ Why is this gone?
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@288 PS2, Line 288: if (!strcmp(tmp, "auto")) : size = SIZE_AUTO; : This branch of the if should have braces
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@293 PS2, Line 293: case 'k': case 'K': I'd put these two case statements in separate lines:
case 'k': case 'K':
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@303 PS2, Line 303: tmp[strlen(tmp) - 1] = '\0'; This should be on a separate line
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@304 PS2, Line 304: atoi(tmp) What will happen if I specify the following sizes as a parameter?
K 1G 87654321 -512m 1 big_enough 0
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@388 PS2, Line 388: 4MB that's 4 MiB (4 MB would be size=4000000)
https://review.coreboot.org/c/flashrom/+/44879/2/dummyflasher.c@963 PS2, Line 963: if (emu_chip_size % 1024) : msg_perr("%s: emu_chip_size is not multipler of 1024.\n", : __func__); Please move this to the parsing of the commmandline arguments
https://review.coreboot.org/c/flashrom/+/44879/2/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/44879/2/flashchips.h@39 PS2, Line 39: #define VARIABLE_SIZE_MANUF_ID 0x3eaf : #define VARIABLE_SIZE_DEVICE_ID 0x10af Where do these IDs come from? Why not use PROGMANUF_ID and PROGDEV_ID instead?