David Hendricks has posted comments on this change. ( https://review.coreboot.org/17953 )
Change subject: Add option to read ROM layout from IFD ......................................................................
Patch Set 7: Code-Review-1
(6 comments)
Good stuff overall. My only qualm is with using '-d' for a vendor-specific option since we don't have many of those to go around. Is that really needed?
https://review.coreboot.org/#/c/17953/5/cli_classic.c File cli_classic.c:
PS5, Line 60: " -d | --ifd read layout from an Intel Firmware Descriptor\n" We only have so many useful single characters to choose from, so I'd prefer to reserve shortopts for generic options. Since IFD is specific to Intel I think we should just use the longopt here.
https://review.coreboot.org/#/c/17953/7/cli_classic.c File cli_classic.c:
PS7, Line 60: " -d | --ifd Since we only have a few useful single letters available for short opts, I think we should reserve them for generic options. Since IFD is Intel-specific I'd like to just use --ifd. (Same goes for chromiumos-specific fmap stuff)
PS7, Line 574: if (ifd) : fl_layout_release(layout); Hm, it would be nice if this could be called unconditionally, or perhaps just gated by 'if (layout)'. That way we call it no matter where we get the layout from.
(comment added to https://review.coreboot.org/#/c/17946)
https://review.coreboot.org/#/c/17953/5/libflashrom.c File libflashrom.c:
Line 34: #include "hwaccess.h" Needs to be guarded with #if defined(__i386__) || defined(__x86_64__)
PS5, Line 339: ret = 2; : goto _unmap_ret; : } : m also needs to be guarded for x86-only
PS5, Line 345: ret = 3; : goto _unmap_ret; : } : same as above