Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220419163749.1.Ica39bd8d1494f750a70a668e5929f6e5e63241f2@changeid>
 <c8f5ada2-c5cc-fe1b-bdeb-8b7d134250ee@molgen.mpg.de>
In-Reply-To: <c8f5ada2-c5cc-fe1b-bdeb-8b7d134250ee@molgen.mpg.de>
From:   Jes Klinke <jbk@google.com>
Date:   Thu, 21 Apr 2022 09:07:21 -0700
Message-ID: <CANMTAZRp5GPwrJNO4RY1KpvABgiuOMYEtwm+jdYfX5JRH8FpSA@mail.gmail.com>
Subject: Re: [PATCH v2] tpm: cr50: Add new device/vendor ID 0x504a6666
To:     Paul Menzel <pmenzel@molgen.mpg.de>
Cc:     "Jes B. Klinke" <jbk@chromium.org>,
        linux-integrity@vger.kernel.org,
        Jarkko Sakkinen <jarkko@kernel.org>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Peter Huewe <peterhuewe@gmx.de>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Dear Paul,

On Wed, Apr 20, 2022 at 3:52 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Jes,
>
>
> Am 20.04.22 um 01:37 schrieb Jes B. Klinke:
> > Accept one additional numerical value of DID:VID for next generation
> > Google TPM with new firmware, to be used in future Chromebooks.
> >
> > The TPM with the new firmware has the code name TI50, and is going to
> > use the same interfaces.
> >
> > Signed-off-by: Jes B. Klinke <jbk@chromium.org>
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >
> >   drivers/char/tpm/tpm_tis_i2c_cr50.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm=
_tis_i2c_cr50.c
> > index f6c0affbb4567..4ddb8ff3a8569 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 TP=
M 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 ven=
dor ID reg value */
> >   #define TPM_CR50_I2C_MAX_RETRIES    3               /* Max retries du=
e to I2C errors */
> >   #define TPM_CR50_I2C_RETRY_DELAY_LO 55              /* Min usecs betw=
een retries on I2C */
> >   #define TPM_CR50_I2C_RETRY_DELAY_HI 65              /* Max usecs betw=
een retries on I2C */
> > @@ -742,15 +743,15 @@ static int tpm_cr50_i2c_probe(struct i2c_client *=
client)
> >       }
> >
> >       vendor =3D le32_to_cpup((__le32 *)buf);
> > -     if (vendor !=3D TPM_CR50_I2C_DID_VID) {
> > +     if (vendor !=3D TPM_CR50_I2C_DID_VID && vendor !=3D TPM_TI50_I2C_=
DID_VID) {
> >               dev_err(dev, "Vendor ID did not match! ID was %08x\n", ve=
ndor);
> >               tpm_cr50_release_locality(chip, true);
> >               return -ENODEV;
> >       }
> >
> > -     dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
> > +     dev_info(dev, "%s TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
> > +              vendor =3D=3D TPM_TI50_I2C_DID_VID ? "ti50" : "cr50",
> >                client->addr, client->irq, vendor >> 16);
> > -
>
> Remove this blank line wasn=E2=80=99t needed, I gues.

You are right, I should probably have left the blank line untouched.
It does not hurt the readability of the code much to remove it,
though.

> >       return tpm_chip_register(chip);
> >   }
> >
>
> Thank for addressing the other comments, and congratulations on getting
> your first commit into the Linux kernel.

When I think about it, I did propose a kernel patch in 2002, to
introduce /proc/sys/kernel/core_pattern, so this is my second patch.
All the mailing lists, and formal review process this time seemed
daunting at first, though, so thank you for guiding me through the
process.

Regards
Jes