Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 8:
(3 comments)
This is much better now, but I still think it'll be even better if we can have vpd_gets and vpd_getbool in same usage for both RAM stage and ROM stage. They can then live in a shared (maybe vpd_common) place.
In fact, from your current implementation I wonder if it'll make more sense to have
vpd.c // common utils, which based on a function like vpd_load_blob(type) vpd_cbmem.c // fetch and build cache in CBMEM, providing vpd_load_blob from CBMEM vpd_romstage.c, or vpd_nocache.c // providing vpd_load_blob directly
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... File src/drivers/vpd/vpd_common.c:
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 95: vpd_find_common maybe rename to vpd_find_from_blob, which is more descriptive than find_common.
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 116: bufcpy we don't encourage acronyms in naming unless if it's very well known/trivial.
Try something like
vpd_copy_buffer or vpd_copy_string.
https://review.coreboot.org/c/coreboot/+/34634/8/src/drivers/vpd/vpd_common.... PS8, Line 122: if (size > (string_size + 1)) { what about
int copy_size = MIN(size - 1, string_size); memcpy(buffer, string_addr, copy_size); buffer[copy_size] = '\0';