Change in coreboot[master]: mb/ocp/deltalake: Add SMBIOS OEM string for SPD register vendor ID
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44545 )
Change subject: mb/ocp/deltalake: Add SMBIOS OEM string for SPD register vendor ID ......................................................................
Patch Set 6: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/44545/6/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/ramstage.c:
https://review.coreboot.org/c/coreboot/+/44545/6/src/mainboard/ocp/deltalake... PS6, Line 73: #define SPD_REGVID_LEN 5 : /* Add leading zeros if needed to make it 4-digit long */ : static void string_fixup(uint16_t val, char *str) : { : if ((val & 0xffff) == 0) : snprintf(str, SPD_REGVID_LEN, "0000"); : else if ((val & 0xfff0) == 0) : snprintf(str, SPD_REGVID_LEN, "000%x", val); : else if ((val & 0xff00) == 0) : snprintf(str, SPD_REGVID_LEN, "00%x", val); : else if ((val & 0xf000) == 0) : snprintf(str, SPD_REGVID_LEN, "0%x", val); : else : snprintf(str, SPD_REGVID_LEN, "%x", val); : } This can be simplified into a single line:
snprintf(str, 5, "%04x", val);
You can also add the space here:
snprintf(str, 6, "%04x ", val);
Also, please rename the function (e.g. `write_oem_word`)
https://review.coreboot.org/c/coreboot/+/44545/6/src/mainboard/ocp/deltalake... PS6, Line 94: char *oem_str7 = NULL; If you initialize this to "" (empty string), then you don't need to specially handle the first loop iteration
https://review.coreboot.org/c/coreboot/+/44545/6/src/mainboard/ocp/deltalake... PS6, Line 105: oem_str7 = strconcat(spd_reg_vid, " "); Instead of adding the space here, why not do it in the helper function above? That way, only one `strconcat` call is necessary for each number
-
Angel Pons (Code Review)