Julius Werner 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:
(3 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];
Passing reference to the stack frame works in one of the two places in which is it used in this CL. […]
I agree with Jes, this is a global for good reason and this API currently already has the assumption that the TPM has been initialized before calling any of the other functions, so adding one more with that dependency doesn't really make a difference.
Since the variable is already static I don't see a reason to have an extra getter function for use within this file. If we later want more code to query this from the outside, then yes, this should export a getter function that returns a const pointer.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
tpm
I would actually prefer to go the other way and rename all instances of TPM_BOARD_CFG with CR50_BOARD_CFG. This is an entirely Cr50-specific thing with no relation to the TPM spec.
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. […]
I think printing here or at the caller is fine either way but in general we should always try to make the unit tests fit coreboot and not the other way round. Having a printk in a unit-tested function is no problem, we already have some of those. (In the same way, if you're worried about this function writing its result back to a static global, we have mountains of coreboot code that manipulate globals for often good reason and we shouldn't just start rewriting the data paths in half our code base to change that. There are other options to make such code testable that we should explore instead.)