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 23:
(4 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];
I agree with Jes, this is a global for good reason and this API currently already has the assumption […]
Keep in mind that when I reviewed this commit, as Furquan noted above, I only observed the current usage and it appeared to me a singleton was not necessary.
ACK on the singleton state cache for API purposes. I see there is a bit of fixation on the semantics of the API and I appricate that perspective.
However functions that can be made pure by taking it as a argument and computing some result should be kept pure and not be a closures over the global state of the singleton whenever possible. I also think it is a reasonable proposition to keep control flow bounded otherwise things start to rot into everything is a singleton where subtle bugs become an emergent phenomena.
TL;DR - functions that could just as well take the state as a argument should take the state as an argument irregardless of the singleton requirement for API purposes.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 449: cr50
+1. […]
ACK, I don't know the full reasonings behind all the conventions so my view isn't strong either way - Resolved.
https://review.coreboot.org/c/coreboot/+/43741/22/src/drivers/spi/tpm/tpm.c@... PS22, Line 487: cr50
tpm
Ack
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;
I think printing here or at the caller is fine either way but in general we should always try to mak […]
Respectfully I disagree. Writing functions to be more pure so that they can be better testable is the correct way around not trying to deal with state in mocks that forever get more and more complex to realise. Previous code is no excuse for new code.
Specifically here, there is zero reason why this function couldn't be a pure function that is self-contained with return values that indicate the specific parse issue. That is my point, not the printf itself. This purity theme is central to my other commits in this commit.