Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 35:
(3 comments)
This change is now ready for review.
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@195 PS22, Line 195: if string is not null terminated, copy local and add termination
I don't think that is really required. You can use printk with %. […]
Great trick ! I didn't know about that printk option.
https://review.coreboot.org/c/coreboot/+/45459/22/src/lib/spd_bin.c@197 PS22, Line 197: memcpy(spd_name, spd_name_ptr, len);
spd_name size needs to be taken into account here.
Done
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/45459/22/src/soc/intel/tigerlake/in... PS22, Line 7: #include <spd_bin.h>
What is this #include for?
Originally, my thinking was that a caller of mainboard_get_dram_part_num() would already be including soc/romstage.h, so if I added the new location of the prototype as an include here, no modification would be needed for all the source files needing the mainboard_get_dram_part_num prototype, keeping the files touched as part of this change minimal. I felt that the extra level of indirection, if you will, was worth not touching all those other files. However, that was before Furquan suggested collapsing down the calls / moving mainboard_get_dram_part_num() routines declared by mainboards to vendorcode/google/chromeos. With that change, there are less places that need the prototype, so it makes more sense for the caller to #include the header directly.
For my (and our code base's) future benefit, would you have changed all files that required the mainboard_get_dram_part_num prototype to include spd_bin.h directly, or reduce risk of touching the other files and just replace the prototype in this header file with an include containing the prototype like I did in this case?
I think in this case, there wouldn't have been much risk of adding an extra #include line to each of those files, but depending on how long it takes to merge this change, my experience has shown that having many mainboards connected to a CL can cause a fair amount of extra rebase work along the way as other mainboards move forward, so that was a weighting factor used in my decision as well.