Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45568 )
Change subject: drivers/tpm: Implement full PPI ......................................................................
Patch Set 5: Code-Review+1
(8 comments)
https://review.coreboot.org/c/coreboot/+/45568/5/src/commonlib/include/commo... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/45568/5/src/commonlib/include/commo... PS5, Line 89: spurious newline?
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/Kconfig File src/drivers/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/Kconfig@10 PS5, Line 10: TPM: Generate ACPI code for physical presence interface Instead of having a TPM prefix, how about:
Generate ACPI code to implement TPM physical presence interface
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/Kconfig@17 PS5, Line 17: Stub stub
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/Kconfig@17 PS5, Line 17: Tcg TCG
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/ppi.c File src/drivers/tpm/ppi.c:
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/ppi.c@41 PS5, Line 41: OS OSes
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/ppi.c@42 PS5, Line 42: Tcg TCG
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/ppi.c@563 PS5, Line 563: printk(BIOS_DEBUG, "PPI: Pending OS request: 0x%x (0x%x)\n", ppib->pprq, ppib->pprm);
line over 96 characters
Since the else-branch returns, you can invert the if-else branches (and invert the condition), then remove the else-branch. This saves you one indentation level and two Jenkins complaints:
ppib = (void *)cbmem_add(CBMEM_ID_TPM_PPI, sizeof(*ppib)); if (!ppib) { printk(BIOS_ERR, "PPI: Failed to add CBMEM\n"); return; } printk(BIOS_DEBUG, "PPI: Pending OS request: 0x%x (0x%x)\n", ppib->pprq, ppib->pprm); printk(BIOS_DEBUG, "PPI: OS response: CMD 0x%x = 0x%x\n", ppib->lppr, ppib->pprp);
https://review.coreboot.org/c/coreboot/+/45568/5/src/drivers/tpm/ppi.c@726 PS5, Line 726: tpm_ppi->ppi_version = BCD(1, 3); Shouldn't this match what `tpm_ppi_func1_cb` returns?