Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45588 )
Change subject: spd: move mainboard_get_dram_part_num to src/vendor/google/chromeos ......................................................................
Patch Set 7:
(3 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/45588/7/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/45588/7/src/include/spd_bin.h@50 PS7, Line 50: bool mainboard_get_dram_part_num(const char **part_num, size_t *len); Is len inclusive or exclusive of the NUL terminator? Please add a comment here indicating expectations of the API.
And shouldn't this declaration be in memory_info.h? I don't see how it directly relates to SPD.
https://review.coreboot.org/c/coreboot/+/45588/7/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45588/7/src/vendorcode/google/chrom... PS7, Line 20: CONFIG_PART_NUMBER_IN_CBI We're relying on each mainboard's setting to expose this Kconfig option? Any reason we wouldn't have that option under Kconfig in vc/google/chromeos ?
Aslo, I think PART_NUMBER_IN_CBI is pretty generic. We should put 'DRAM' in there.
https://review.coreboot.org/c/coreboot/+/45588/7/src/vendorcode/google/chrom... PS7, Line 20: spd I don't think this file is named adequately. SPD is not contributing to what the function is providing.