Messages in this thread | | | Date | Thu, 14 Apr 2022 17:06:24 +0200 | Subject | Re: [PATCH] tpm: cr50: Add new device/vendor ID 0x504a6666 | From | Paul Menzel <> |
| |
Dear Jarkko,
Am 14.04.22 um 14:08 schrieb Jarkko Sakkinen: > On Thu, Apr 07, 2022 at 12:21:44PM +0200, Paul Menzel wrote:
>> Thank you for your patch. >> >> Am 05.04.22 um 19:37 schrieb Jes B. Klinke: >>> Accept one additional numerical value of DID:VID for next generation >>> Google TPM, to be used in future Chromebooks. >> >> Maybe extend: >> >> … Google TPM with new firmware … >> >> The TPM with the new firmware has the code name TI50, and going to use the >> same interfaces. >> >>> This patch touches more lines than may seem necessary, as a result of >>> the need to move the error case to sit after the two recognized cases. >>> >>> Signed-off-by: Jes B. Klinke <jbk@chromium.org> >>> --- >>> >>> drivers/char/tpm/tpm_tis_i2c_cr50.c | 21 +++++++++++++-------- >>> 1 file changed, 13 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c >>> index f6c0affbb4567..bf54ebd6724b0 100644 >>> --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c >>> +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c >>> @@ -31,6 +31,7 @@ >>> #define TPM_CR50_TIMEOUT_SHORT_MS 2 /* Short timeout during transactions */ >>> #define TPM_CR50_TIMEOUT_NOIRQ_MS 20 /* Timeout for TPM ready without IRQ */ >>> #define TPM_CR50_I2C_DID_VID 0x00281ae0L /* Device and vendor ID reg value */ >>> +#define TPM_TI50_I2C_DID_VID 0x504a6666L /* Device and vendor ID reg value */ >>> #define TPM_CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */ >>> #define TPM_CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */ >>> #define TPM_CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */ >>> @@ -742,16 +743,20 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) >>> } >>> vendor = le32_to_cpup((__le32 *)buf); >>> - if (vendor != TPM_CR50_I2C_DID_VID) { >>> - dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor); >>> - tpm_cr50_release_locality(chip, true); >>> - return -ENODEV; >>> + if (vendor == TPM_CR50_I2C_DID_VID) { >>> + dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n", >>> + client->addr, client->irq, vendor >> 16); >>> + return tpm_chip_register(chip); >>> + } >>> + if (vendor == TPM_TI50_I2C_DID_VID) { >>> + dev_info(dev, "ti50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n", >>> + client->addr, client->irq, vendor >> 16); >>> + return tpm_chip_register(chip); >>> } >> >> Both branches are quite similar. Can a ternary operator be used? >> >> dev_info(dev, "%s TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n", >> (vendor == TPM_CR50_I2C_DID_VID) ? "cr50" : "ti50", client->addr, >> client->irq, vendor >> 16); >> return tpm_chip_register(chip); >> >> and the original flow be left? (A separate variable can also be added.) >> >>> - dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n", >>> - client->addr, client->irq, vendor >> 16); >>> - >>> - return tpm_chip_register(chip); >>> + dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor); >>> + tpm_cr50_release_locality(chip, true); >>> + return -ENODEV; >>> } >>> /**
> OK, these are legit suggestions. Paul, can you do these changes and add my > reviewed-by for the +1 version?
I guess you mean Jes?
Kind regards,
Paul
| |