Hi Ilias On Mon, 22 Jul 2024 at 08:35, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi all > > On Sun, 21 Jul 2024 at 13:08, Simon Glass <s...@chromium.org> wrote: > > > > Hi, > > > > On Mon, 15 Jul 2024 at 12:23, <lukas.funke-...@weidmueller.com> wrote: > > > > > > From: Lukas Funke <lukas.fu...@weidmueller.com> > > > > > > tpm_tis_wait_init() is using the 'chip->timeout_b' field which is > > > initialized in tpm_tis_init(). However, the init-function is called > > > *after* tpm_tis_wait_init() introducing an uninitalized field access. > > > > > > This commit switches both routines. > > > > > > Signed-off-by: Lukas Funke <lukas.fu...@weidmueller.com> > > > --- > > > > > > drivers/tpm/tpm2_tis_spi.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c > > > index b0fe97ab1d0..5a4dbfd3ccb 100644 > > > --- a/drivers/tpm/tpm2_tis_spi.c > > > +++ b/drivers/tpm/tpm2_tis_spi.c > > > @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev) > > > /* Ensure a minimum amount of time elapsed since reset of the TPM > > > */ > > > mdelay(drv_data->time_before_first_cmd_ms); > > > > > > + tpm_tis_ops_register(dev, &phy_ops); > > > + ret = tpm_tis_init(dev); > > > + if (ret) > > > + goto err; > > > + > > > ret = tpm_tis_wait_init(dev, chip->locality); > > > if (ret) { > > > log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__); > > > return ret; > > > } > > > > > > - tpm_tis_ops_register(dev, &phy_ops); > > > - ret = tpm_tis_init(dev); > > > - if (ret) > > > - goto err; > > > - > > > priv->pcr_count = drv_data->pcr_count; > > > priv->pcr_select_min = drv_data->pcr_select_min; > > > priv->version = TPM_V2; > > > -- > > > 2.30.2 > > > > > > > This needs a Fixes tag for a5c30c26b28 (HEAD) tpm: Use the new API on > > tpm2 spi driver > > > > Yes please we need a fixes tag > > > The old code set up the timeouts first, then did the wait_init. > > Presumably the point of wait_init is to wait before doing the init, so > > we should try to keep that behaviour, unless it is actually wrong. > > > > So my thought would be to move the setup of the required timeout into > > tpm_tis_ops_register(), instead. > > tpm_tis_ops_register() is setting up the bus accesses and I'd prefer > to keep it that way. > Since this is a static function, we can fold it in tpm_tis_init(), > which makes more sense
Since this is used by i2c and SPI we need to make sure that both work. I believe the way it used to be, some initial values were set for the timeouts. The naming is somewhat confusing I suppose. Regards, Simon