David Hendricks has posted comments on this change. ( https://review.coreboot.org/17945 )
Change subject: Add functions to read/erase/write/verify by layout ......................................................................
Patch Set 4: Code-Review+2
(3 comments)
Better! I have a few cosmetic nits for you to consider, but overall I'd be fine with committing this as is.
FWIW for testing I'm trying to bring in some patches to handle -i region[:<filename>] syntax (https://patchwork.coreboot.org/patch/4076/) and the new test script from chromium (https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/master/t...). It sounds like you already did a fair amount of manual testing, too.
https://review.coreboot.org/#/c/17945/4/flashrom.c File flashrom.c:
PS4, Line 1764: size_t int to match num_entries?
Line 1855: /* nit: add a newline above to separate the declarations and the body
PS4, Line 1886: newc + start - info->erase_start Thanks for updating the offset calculation. However, it somehow didn't end up as clear as I had hoped, probably because "start" was added and so when re-reading this I had to mentally substitute "info->region_end + 1" for "start".
How about creating another variable above, rel_start or something, to store "start - info->erase_start"? That makes it even more clear that we're adding a relative address to newc and also reduces arithmetic in the function calls (same rationale for creating "start" and "len" to begin with, I presume).
-
David Hendricks (Code Review)