Jes Klinke 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 27:
(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 com […]
Done
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, […]
I do not like such use-once variable, unless its name provides some information that is not otherwise available. In this case, the structure of the if statement, and the debug error message, makes it clear that the return must be a success/error code. Unless Coreboot has some general guideline against side effect in the conditions of if statements, I prefer to keep it the way it is.