David Hendricks has posted comments on this change. ( https://review.coreboot.org/23021 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/#/c/23021/5/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23021/5/flashrom.c@1379 PS5, Line 1379:
I could think of a future use that might be useful if/once fmap support is implemented: if you use a […]
More specifically, to make sure that the include args for write operations don't overlap. For reading it should be fine.
https://review.coreboot.org/#/c/23021/7/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23021/7/flashrom.c@1380 PS7, Line 1380: if (!entry->included || !entry->file) : continue;
Optionally: […]
Actually, I don't think we should check for entry->file here. Doing so will require that all included entries have a file specified which is not always desirable.
The higher-level logic should check if an argument to -w was specified to decide what to do here: - If a file is specified for -w, then we do not need a file specified for each -i arg. - If a file is not specified for -w, then we need a file specified for each -i arg.
Removing the check here will also obviate the need for my get_num_include_args_with_files() hack in PS11.
https://review.coreboot.org/#/c/23021/11/layout.c File layout.c:
https://review.coreboot.org/#/c/23021/11/layout.c@174 PS11, Line 174: Use the -l/--layout parameter to specify\n" : "a layout file. We should probably clarify this comment or get rid of the latter half since the layout can come from a few different sources (layout file, IFD, and soon FMAP).
-
David Hendricks (Code Review)