Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass <s...@chromium.org> wrote:
>
[...]
> > > > + }
> > >
> > > This really should be in the device tree so what you are doing here is
> > > quite strange.
> >
> > Like I had mentioned in my earlier emails, the TPM device has a
> > builtin RNG functionality, which is non-optional. So I don't
> > understand why we need to use a device tree subnode here. Whether the
> > device is being bound to the parent is being controlled by the TPM_RNG
> > config that you asked me to put in my previous version, which I am
> > doing.
>
> See how PMICs work, for example. We have GPIOs, regulators and
> suchlike in the PMIC and we add subnodes for them in the DT. It is
> just how it is done.
>
> Driver model is designed to automatically bind devices based on the
> device tree. There are cases where it is necessary to manually bind
> things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The
TPM is the device and an encoded command to that device returns a
random number. There's no binding initiating etc.
>
> Finally, I know you keep saying that random numbers are only needed in
> U-Boot proper, but if I want a random number in SPL, it may not work,
> since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
>
> So that is why I say that what you are doing is quite strange. Perhaps
> you are coming from a different project, which does things
> differently.
I don't personally find it strange. The device is already described
in the DTS and I don't see a strong reason to deviate for the upstream
version again.
Regards
/Ilias
>
> >
> > If you want to manually bind it, please call
> > > device_find_first_child_by_uclass() first to make sure it isn't
> > > already there.
> >
> > Okay
> >
> > >
> > > Also you should bind it in the bind() method, not in probe().
> >
> > Okay
> >
> > >
> > > This is the code used for the same thing in the bootstd series:
> > >
> > > struct udevice *bdev;
> > > int ret;
> > >
> > > ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev);
> > > if (ret) {
> > > if (ret != -ENODEV) {
> > > log_debug("Cannot access bootdev device\n");
> > > return ret;
> > > }
> > >
> > > ret = bootdev_bind(parent, drv_name, "bootdev", &bdev);
> > > if (ret) {
> > > log_debug("Cannot create bootdev device\n");
> > > return ret;
> > > }
> > > }
> > >
> > >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > UCLASS_DRIVER(tpm) = {
> > > > - .id = UCLASS_TPM,
> > > > - .name = "tpm",
> > > > - .flags = DM_UC_FLAG_SEQ_ALIAS,
> > > > + .id = UCLASS_TPM,
> > > > + .name = "tpm",
> > > > + .flags = DM_UC_FLAG_SEQ_ALIAS,
> > > > #if CONFIG_IS_ENABLED(OF_REAL)
> > > > - .post_bind = dm_scan_fdt_dev,
> > > > + .post_bind = dm_scan_fdt_dev,
> > > > #endif
> > > > + .post_probe = tpm_uclass_post_probe,
> > >
> > > Should be post_bind.
> >
> > Okay
>
> [..]
>
> Regards,
> Simon
