Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: [RFC] Replace strapping_id entries with new board configuration entry ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 263: struct cb_strapping_id { Remove?
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 326: dram_id Since we're keeping the name ram_code in sysinfo (and we probably should, since external payloads may depend on it), shouldn't we just keep using that name in your new code as well for consistency?
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/5/payloads/libpayload/include... PS5, Line 110: #define UNDEFINED_STRAPPING_ID_64 (~(uint64_t)0) I think you can just change the existing macro to 64-bit, it will auto-convert down to ~0 when assigned to a 32-bit variable.
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/1/src/lib/coreboot_table.c@20 PS1, Line 20: #include <version.h>
Ack
Paul, just as a general thought, for these little drive-by cleanups I think we should be careful to not give people too much trouble with them or we're just going to discourage them from being done at all. In general you are right that separate concerns belong in separate patches, but I think a small significance threshold needs to be met for that to apply. There certainly is *some* extra effort involved every time you split a patch and if you force people to do that for very trivial things, they may just not do them at all instead. I'd rather have our headers occasionally get sorted in the "wrong" patch than never.
(Case in point, Tim followed your suggestion for this patch but I don't think he actually uploaded a separate one for the headers. ;) )