On Thu, Aug 18, 2022 at 11:49:53AM -0600, Simon Glass wrote:
> Hi Michal,
>
> On Wed, 17 Aug 2022 at 02:28, Michal Suchanek <msucha...@suse.de> wrote:
> >
> > When probing a device fails NULL pointer is returned, and other devices
> > cannot be iterated. Skip to next device on error instead.
> >
> > Fixes: 6494d708bf ("dm: Add base driver model support")
> > Signed-off-by: Michal Suchanek <msucha...@suse.de>
> > ---
> > v2: Fix up tests
> >
> > Note: there is seemingly bogus repeated device_remove(parent,
> DM_REMOVE_NORMAL);
> > but I have no idea what the intent was, and fixing that is out of the
> > scope of this patch anyway.
>
> This is to remove child devices that have been probed, so that we get back
> to the original state.
Thanks, it makes sense now.
> >
> > drivers/core/uclass.c | 30 +++++++++++++++++++++---------
> > test/dm/test-fdt.c | 20 ++++++++++++++++----
> > 2 files changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> > index 08d9ed82de..ccf7d59141 100644
> > --- a/drivers/core/uclass.c
> > +++ b/drivers/core/uclass.c
> > @@ -574,16 +574,31 @@ int uclass_get_device_by_phandle(enum uclass_id id,
> struct udevice *parent,
> > }
> > #endif
> >
> > +/* Starting from the given device return first device in the uclass that
> probes successfully */
> > +static int __uclass_next_device(struct udevice *dev, int ret, struct
> udevice **devp)
>
> Can you avoid __ as this is reserved for compiler. Perhaps use a single
> underscore?
>
> Please check 80cols
whatever
> > +{
> > + if (!dev) {
> > + *devp = dev;
> > + return 0;
> > + }
>
> Is this for people that call next after they shouldn't?
This is for the case if there is nothing.
> > + while ((ret = uclass_get_device_tail(dev, ret, devp))) {
> > + ret = uclass_find_next_device(&dev);
> > + if (!dev) {
> > + *devp = dev;
> > + return 0;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > int uclass_first_device(enum uclass_id id, struct udevice **devp)
> > {
> > - struct udevice *dev;
> > + struct udevice *dev = NULL;
>
> Can you drop the NULL assignment? uclass_find_first_device() sets dev to
> NULL anyway, as a first step.
>
> > int ret;
> >
> > - *devp = NULL;
>
> Is this safe to remove? If there is nothing, then
it's the same as uclass_next_device() on the last device.
>
> > ret = uclass_find_first_device(id, &dev);
> > - if (!dev)
> > - return 0;
> > - return uclass_get_device_tail(dev, ret, devp);
> > + return __uclass_next_device(dev, ret, devp);
> > }
> >
> > int uclass_first_device_err(enum uclass_id id, struct udevice **devp)
> > @@ -604,11 +619,8 @@ int uclass_next_device(struct udevice **devp)
> > struct udevice *dev = *devp;
> > int ret;
> >
> > - *devp = NULL;
> > ret = uclass_find_next_device(&dev);
> > - if (!dev)
> > - return 0;
> > - return uclass_get_device_tail(dev, ret, devp);
> > + return __uclass_next_device(dev, ret, devp);
> > }
>
> This is a major change in behaviour, so please do update the API docs at
> dm/uclass.h
Yes, those need updating as well.
Thanks
Michal
