Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39693 )
Change subject: drivers/spi/tpm: Add support for non CR50 SPI TPM2 ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/Kconfig... PS2, Line 21: Board has SPI TPM support nit: should say something different from the one below
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c@1... PS2, Line 198: SPI transfer error\n nit: Mention something about this being in the TPM driver?
https://review.coreboot.org/c/coreboot/+/39693/2/src/drivers/spi/tpm/tpm.c@2... PS2, Line 203: !CONFIG(TPM_CR50) && ( CONFIG() test is unnecessary here.
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/c/coreboot/+/39693/1/src/security/tpm/Kconfig@29 PS1, Line 29: || MAINBOARD_HAS_SPI_TPM_CR50 || MAINBOARD_HAS_CRB_TPM \
Well, I don't understand why everything is aligned after "MAINBOARD_HAS_I2C_TPM_GENERIC". […]
I think the intention is still that Kconfig files adhere to the usual 80 (or now 96) character limit. Of course, they're currently probably not all compliant with that...
I agree that this looks odd and aligning after "depends on " would probably be better.