Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34106 )
Change subject: src/drivers/crb: Add CRB Driver for TPM2 Support ......................................................................
Patch Set 12:
(6 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tis.c File src/drivers/crb/tis.c:
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tis.c@93 PS8, Line 93: const char *path = "\_SB_.TPM";
use acpi_device_path() […]
Ack
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tis.c@111 PS8, Line 111: acpigen_write_mem32fixed(1, TPM_CRB_BASE_ADDRESS, 0x5000);
TPM_CRB_BASE_ADDRESS can be read from *dev
What is the point here in NOT using the define?
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tpm.c File src/drivers/crb/tpm.c:
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tpm.c@48 PS8, Line 48: control_area.request = read32(CRB_REG(current_locality, CRB_REG_REQUEST));
all those lines are quite long. […]
Ack
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tpm.c@109 PS8, Line 109: return -1;
use break here?
removed the unreachable return - though this should be fine.
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tpm.c@113 PS8, Line 113: return -1;
unreachable
Ack
https://review.coreboot.org/c/coreboot/+/34106/8/src/drivers/crb/tpm.c@230 PS8, Line 230: // Write to Command Buffer
c++ comments are mixed with C89 comments
So?