Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 22:
(9 comments)
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 51: tatic int cr50_firmware_version[3]; You do not need a global, please avoid them and parse though the call stack instead.
``` struct cr50_ver { uint8_t major; uint8_t minor; uint8_t patch; }; ```
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 434: static int cr50_supports_board_cfg(void) `static int cr50_supports_board_cfg(struct cr50_ver ver)` is the correct signature. Although these names are inconsistent with the rest of the file - there is a lot of mixing cr50 and tpm together. Consider the identifier `tpm_board_cfg_supported` instead? The signature and intent are then clearly encoded together for what the purpose is here.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50 tpm
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 487: cr50 tpm
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 515: tatic void parse_version(const char *version_str) `static int parse_version(const char *version_str, struct cr50_ver *ver)` is the correct signature.
Also move this function above the version check function above. Consider also a better name consistent with the rest of the file, `tpm_parse_fw_ver()` perhaps?
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 521: printk(BIOS_ERR, "Did not recognize Cr50 version prefix\n"); : return; return -1,2,3.. and 0 on success then print errors at call site. This way functions become more pure and easier to unit-test since we want to do more unit-testing in coreboot. Further you can use the return value to determine how the parse went wrong with just one printf.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 525: cr50_firmware_version[0] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing first dot in Cr50 version\n"); : return; : } : cr50_firmware_version[1] = skip_atoi(&number); : if (*number++ != '.') { : printk(BIOS_ERR, "Error parsing second dot in Cr50 version\n"); : return; : } : cr50_firmware_version[2] = skip_atoi(&number); parse out major.minor.patch into itermediates and only write the result into the param at the end when it is considered valid.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 638: parse_version(version_str); `struct cr50_ver cr50_fw_ver;` then pass `&cr50_fw_ver` and validate the return valid of the parse function if you can use it later.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 641: (); `set_cr50_board_cfg(cr50_fw_ver)`