David Hendricks has posted comments on this change. ( https://review.coreboot.org/19342 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 8:
(10 comments)
I finally had a chance to address most of your comments from PS2, but there are a couple items outstanding (man page update, region and file names in layout).
https://review.coreboot.org/#/c/19342/8/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/19342/8/flashrom.8.tmpl@51 PS8, Line 51: [(\fB-l\fR <file>|\fB--ifd\fR) [\fB-i\fR <image>[:<file>]]] \ Need to explain the updated syntax below.
https://review.coreboot.org/#/c/19342/2/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/19342/2/flashrom.c@1308 PS2, Line 1308: if (flash->chip->printlock) : flash->chip->printlock(flash);
remove
Done
https://review.coreboot.org/#/c/19342/2/flashrom.c@1342 PS2, Line 1342: (intmax_t)image_stat.s removed this line
https://review.coreboot.org/#/c/19342/2/flashrom.c@1353 PS2, Line 1353: out:
IMHO, not having an empty line here was intentional.
Done
https://review.coreboot.org/#/c/19342/2/flashrom.c@1392 PS2, Line 1392: if (fflush(image)) {
If we make it the standard, the declaration should probably move
Done: https://review.coreboot.org/#/c/21367/
https://review.coreboot.org/#/c/19342/2/flashrom.c@1404 PS2, Line 1404: if (S_ISREG(image_stat.st_mode)) {
Now that we pass the flashctx, the size argument is redundant.
Done.
https://review.coreboot.org/#/c/19342/2/flashrom.c@1407 PS2, Line 1407: ret = 1;
ahem
removed.
https://review.coreboot.org/#/c/19342/2/flashrom.c@1427 PS2, Line 1427:
*const ? to match the other "variables"
Done
https://review.coreboot.org/#/c/19342/2/flashrom.c@1444 PS2, Line 1444: if (!layout->entries[i].included)
double blank line?
Done
https://review.coreboot.org/#/c/19342/8/layout.h File layout.h:
https://review.coreboot.org/#/c/19342/8/layout.h@46 PS8, Line 46: char *file; Still need to address comments in PS2