[coreboot-gerrit] Change in coreboot[master]: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems

[Edward O'Callaghan (Code Review)]

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:

(1 comment)

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];
> Actually, Karthik is working on some patches that might need stashing of cr50 firmware version somew […]

Well that isn't happening in this patch and the control flow isn't currently good by doing this.

Why not stash it at the buttom of the call frame stack and pass by reference up? I suspect we can resolve this in the subsequent patches if a singleton really is required, however it is clearly not required here. In the absolute worst case of requiring a singleton pattern, one should access the state variable via a getter/setter functions and have a default value in the getter for uninitialised.