Attention is currently required from: Julius Werner. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59494 )
Change subject: libpayload/libc/fmap: Implement new FlashMap API ......................................................................
Patch Set 4:
(5 comments)
File payloads/libpayload/include/fmap.h:
https://review.coreboot.org/c/coreboot/+/59494/comment/59a10813_4eee0595 PS2, Line 9: lib_sysinfo.fmap_offset and boot device read function
I don't think we need to support the second case? lib_sysinfo.fmap_cache should always exist. […]
Depthcharge does it this way, so I thought I'd do it this way too. But if you say, that fmap_cache will always be present in normal cases, then it can be simplified :)
https://review.coreboot.org/c/coreboot/+/59494/comment/4ef19444_6936a9e6 PS2, Line 10: *
nit: missing space. also maybe clarify what is returned on error.
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/77926f47_deec7992 PS2, Line 11: int
Should we use cb_err_t here?
Done
File payloads/libpayload/libc/fmap.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/509f4c27_3e1abae5 PS2, Line 113:
Maybe add a comment to make the line between new code and old, deprecated code here clearer.
Done
File payloads/libpayload/tests/libc/fmap_region_by_name-test.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/0ca936d9_c02830e2 PS1, Line 44: SETUP_LIBPAYLOAD_BOOT_DEVICE_READ_MOCK(&test_fmap, fmap_offset, sizeof(test_fmap));
I feel like this test is a bit too white-boxy (i.e. […]
Removed, as new FMAP APi does not use boot media.
-
Jakub Czapiga (Code Review)