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 26:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c@... PS26, Line 456: /* Parsing succeeded, store complete value. */ : cr50_firmware_version = scratch; This looks like a typo here, this still overwrites the global and not the function param and the comment isn't required either. Just copy-paste this:
``` static int cr50_parse_fw_version(const char *version_str, struct cr50_firmware_version *ver) { int epoch, major, minor;
char *number = strstr(version_str, " RW_A:"); if (!number) number = strstr(version_str, " RW_B:"); if (!number) return -1; number += 6; /* Skip past the colon. */
epoch = skip_atoi(&number); if (*number++ != '.') return -2; major = skip_atoi(&number); if (*number++ != '.') return -2; minor = skip_atoi(&number);
ver->epoch = epoch; ver->major = major; ver->minor = minor;
return 0; } ```
https://review.coreboot.org/c/coreboot/+/43741/26/src/drivers/spi/tpm/tpm.c@... PS26, Line 639: if (cr50_parse_fw_version(version_str, &cr50_firmware_version)) { : printk(BIOS_ERR, "Did not recognize Cr50 version format\n"); : return -1; : } optional, you could do here, ``` int ver_valid = cr50_parse_fw_version(version_str, &cr50_firmware_version); if (ver_valid < 0) { printk(BIOS_ERR, "Did not recognize Cr50 version format with %d\n", ver_valid); return -1; } ```