Namyoon Woo 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 3:
(9 comments)
https://review.coreboot.org/c/flashrom/+/44879/1/dummyflasher.c File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/44879/1/dummyflasher.c@395 PS1, Line 395: if (!strncmp(tmp, VARIABLE_SIZE_CHIP_NAME, : strlen(VARIABLE_SIZE_CHIP_NAME))) {
Sorry, what I mean is that you can remove the line wrap here.
Done
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?
This is moved to above line 347, because it needed to stat emu_persistent_image in case that SIZE_AUTO was given in parameter.
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
Done
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: […]
Done
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
Done
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? […]
Added more parse error handling codes.
K => size shall be 0, and processed as an error at the end. 1G => atoi('1G') shall be 0, and processed as an error at the end. 87654321 => shall be processed as an error since it is not a multiple of 1024. -512m => shall be processed as an error since it is negative. 1 => shall be processed as an error since it is not a multiple of 1024. big_enough => size shall be 0, and processed as an error at the end. 0 => size shall be 0, and processed as an error at the end.
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)
Done
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
Done. Moved to the line 427 in patchset#3.
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?
Done