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.
>
> 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
> +{
> + if (!dev) {
> + *devp = dev;
> + return 0;
> + }
Is this for people that call next after they shouldn't?
> + 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
> 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
>
> int uclass_next_device_err(struct udevice **devp)
> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> index 6118ad42ca..165b4f5554 100644
> --- a/test/dm/test-fdt.c
> +++ b/test/dm/test-fdt.c
> @@ -417,16 +417,28 @@ static int dm_test_first_next_device(struct
unit_test_state *uts)
> pdata = dev_get_plat(dev);
> pdata->probe_err = -ENOMEM;
> device_remove(parent, DM_REMOVE_NORMAL);
> - ut_assertok(uclass_first_device(UCLASS_TEST_PROBE, &dev));
> - ut_asserteq(-ENOMEM, uclass_next_device(&dev));
> - ut_asserteq_ptr(dev, NULL);
> + for (ret = uclass_first_device(UCLASS_TEST_PROBE, &dev), count =
0;
> + dev;
> + ret = uclass_next_device(&dev)) {
> + count++;
> + parent = dev_get_parent(dev);
> + }
> + ut_assertok(ret);
> + ut_asserteq(3, count);
>
> /* Now an error on the first one */
> ut_assertok(uclass_get_device(UCLASS_TEST_PROBE, 0, &dev));
> pdata = dev_get_plat(dev);
> pdata->probe_err = -ENOENT;
> device_remove(parent, DM_REMOVE_NORMAL);
> - ut_asserteq(-ENOENT, uclass_first_device(UCLASS_TEST_PROBE,
&dev));
> + for (ret = uclass_first_device(UCLASS_TEST_PROBE, &dev), count =
0;
> + dev;
> + ret = uclass_next_device(&dev)) {
> + count++;
> + parent = dev_get_parent(dev);
> + }
> + ut_assertok(ret);
> + ut_asserteq(2, count);
>
> return 0;
> }
> --
> 2.37.1
>
Regards,
Simon
