Attention is currently required from: Jason Glenesk, Marshall Dawson, Tim Wawrzynczak, Subrata Banik, Angel Pons, Rob Barnes, Patrick Rudolph, Felix Held. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56628 )
Change subject: arch/x86: Refactor the SMBIOS type 17 write function ......................................................................
Patch Set 21:
(3 comments)
File tests/lib/dimm_info_util-test.c:
https://review.coreboot.org/c/coreboot/+/56628/comment/8f9f8c67_9bbae26a PS21, Line 189: const LargestIntegralType udimm_allowed[] = { : DDR5_SPD_UDIMM, DDR5_SPD_MINI_UDIMM, : }; : : const LargestIntegralType rdimm_allowed[] = { DDR5_SPD_RDIMM, DDR5_SPD_MINI_RDIMM }; : could you merge all of these tests into one, which would get `udimm_allowed`, `rdimm_allowed` and `expected_module_type` as arguments? Could not all of these test be a generalized in test_smbios_form_factor_to_spd_mod_type_parametrized() and get called by test_smbios_form_factor_to_spd_mod_type()? You have `struct memory_info`, which you can extend to contain mentioned params.
https://review.coreboot.org/c/coreboot/+/56628/comment/8189dcb9_e0b1d8da PS21, Line 215: struct memory_info How about: `struct smbios_form_factor_test_info`? Would it be better?
https://review.coreboot.org/c/coreboot/+/56628/comment/e331877b_df6139d0 PS21, Line 219: { MEMORY_TYPE_DDR2, test_smbios_form_factor_to_spd_mod_type_on_ddr2 }, : { MEMORY_TYPE_DDR3, test_smbios_form_factor_to_spd_mod_type_on_ddr3 }, : { MEMORY_TYPE_DDR4, test_smbios_form_factor_to_spd_mod_type_on_ddr4 }, : { MEMORY_TYPE_DDR5, test_smbios_form_factor_to_spd_mod_type_on_ddr5 }, I think, that ddr tests can be in another test case, than lpddrx. They require only one argument: smbios_memory_type, when ddrX require more (after applying tests generalization, I was writing in comment above)
-
Jakub Czapiga (Code Review)