[PATCH v2 2/2] uclass: Cleanup uclass_find_next_device
Andrew Goodbody
andrew.goodbody at linaro.org
Tue Jul 15 17:00:52 CEST 2025
On 15/07/2025 13:06, Quentin Schulz wrote:
> Hi Andrew,
>
> On 7/15/25 11:23 AM, Andrew Goodbody wrote:
>> uclass_find_next_device always returns 0, so instead make it a void and
>> update calling sites.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
>> ---
>> board/emulation/qemu-ppce500/qemu-ppce500.c | 4 ++--
>> boot/bootdev-uclass.c | 2 +-
>> cmd/regulator.c | 4 ++--
>> drivers/core/uclass.c | 16 +++++-----------
>> drivers/power/regulator/regulator-uclass.c | 10 ++++++----
>> drivers/remoteproc/rproc-uclass.c | 6 ++++--
>> include/dm/uclass-internal.h | 4 +---
>> test/dm/core.c | 28 +++++++++++++
>> +--------------
>> 8 files changed, 35 insertions(+), 39 deletions(-)
>>
>> diff --git a/board/emulation/qemu-ppce500/qemu-ppce500.c b/board/
>> emulation/qemu-ppce500/qemu-ppce500.c
>> index 40d295dbf06..58de4a05296 100644
>> --- a/board/emulation/qemu-ppce500/qemu-ppce500.c
>> +++ b/board/emulation/qemu-ppce500/qemu-ppce500.c
>> @@ -170,9 +170,9 @@ int misc_init_r(void)
>> * Detect the presence of the platform bus node, and
>> * create a virtual memory mapping for it.
>> */
>> - for (ret = uclass_find_first_device(UCLASS_SIMPLE_BUS, &dev);
>> + for (uclass_find_first_device(UCLASS_SIMPLE_BUS, &dev);
>> dev;
>> - ret = uclass_find_next_device(&dev)) {
>> + uclass_find_next_device(&dev)) {
>> if (device_is_compatible(dev, "qemu,platform")) {
>> struct simple_bus_plat *plat = dev_get_uclass_plat(dev);
>> diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
>> index 3791ebfcb42..6718c482e52 100644
>> --- a/boot/bootdev-uclass.c
>> +++ b/boot/bootdev-uclass.c
>> @@ -216,7 +216,7 @@ void bootdev_list(bool probe)
>> if (probe)
>> ret = uclass_next_device_check(&dev);
>> else
>> - ret = uclass_find_next_device(&dev);
>> + uclass_find_next_device(&dev);
>
> I think we need to ret = 0 here as well? Otherwise the printf at the top
> of the for-loop will return whatever was the last return value?
Yes, will fix.
>> }
>> printf("--- ------ ------ -------- ------------------\n");
>> printf("(%d bootdev%s)\n", i, i != 1 ? "s" : "");
>> diff --git a/cmd/regulator.c b/cmd/regulator.c
>> index da298090bb7..db843283662 100644
>> --- a/cmd/regulator.c
>> +++ b/cmd/regulator.c
>> @@ -97,9 +97,9 @@ static int do_list(struct cmd_tbl *cmdtp, int flag,
>> int argc,
>> "Parent");
>> for (ret = uclass_find_first_device(UCLASS_REGULATOR, &dev); dev;
>> - ret = uclass_find_next_device(&dev)) {
>> + uclass_find_next_device(&dev)) {
>> if (ret)
>> - continue;
>> + break;
>
> I think it would make this more readable by having
>
> ret = uclass_find_first_device(UCLASS_REGULATOR, &dev);
> if (ret)
> return ret;
>
> for (; dev; uclass_find_next_device(&dev)) {
> uc_pdata...
> }
>
> return ret;
>
> What do you think? This also avoids checking for ret in every loop
> considering it won't ever change.
OK, will change as suggested.
> [...]
>
>> diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/
>> power/regulator/regulator-uclass.c
>> index 09567eb9dbb..eac025c9c02 100644
>> --- a/drivers/power/regulator/regulator-uclass.c
>> +++ b/drivers/power/regulator/regulator-uclass.c
>> @@ -261,10 +261,10 @@ int regulator_get_by_platname(const char
>> *plat_name, struct udevice **devp)
>> *devp = NULL;
>> for (ret = uclass_find_first_device(UCLASS_REGULATOR, &dev); dev;
>> - ret = uclass_find_next_device(&dev)) {
>> + uclass_find_next_device(&dev)) {
>> if (ret) {
>> dev_dbg(dev, "ret=%d\n", ret);
>> - continue;
>> + break;
>> }
>
> Same here.
>
>> uc_pdata = dev_get_uclass_plat(dev);
>> @@ -411,8 +411,10 @@ static bool regulator_name_is_unique(struct
>> udevice *check_dev,
>> int len;
>> for (ret = uclass_find_first_device(UCLASS_REGULATOR, &dev); dev;
>> - ret = uclass_find_next_device(&dev)) {
>> - if (ret || dev == check_dev)
>> + uclass_find_next_device(&dev)) {
>> + if (ret)
>> + break;
>
> Ditto.
>
>> + if (dev == check_dev)
>> continue;
>> uc_pdata = dev_get_uclass_plat(dev);
>> diff --git a/drivers/remoteproc/rproc-uclass.c b/drivers/remoteproc/
>> rproc-uclass.c
>> index 3233ff80419..7916cb27e3d 100644
>> --- a/drivers/remoteproc/rproc-uclass.c
>> +++ b/drivers/remoteproc/rproc-uclass.c
>> @@ -56,8 +56,10 @@ static int for_each_remoteproc_device(int (*fn)
>> (struct udevice *dev,
>> int ret;
>> for (ret = uclass_find_first_device(UCLASS_REMOTEPROC, &dev); dev;
>> - ret = uclass_find_next_device(&dev)) {
>> - if (ret || dev == skip_dev)
>> + uclass_find_next_device(&dev)) {
>> + if (ret)
>> + return ret;
>
> Ditto.
>
>> + if (dev == skip_dev)
>> continue;
>> uc_pdata = dev_get_uclass_plat(dev);
>> ret = fn(dev, uc_pdata, data);
>> diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h
>> index 3ddcdd21439..9cb3f090271 100644
>> --- a/include/dm/uclass-internal.h
>> +++ b/include/dm/uclass-internal.h
>> @@ -149,10 +149,8 @@ int uclass_find_first_device(enum uclass_id id,
>> struct udevice **devp);
>> *
>> * The device is not prepared for use - this is an internal function.
>> * The function uclass_get_device_tail() can be used to probe the
>> device.
>> - *
>> - * Return: 0 if OK (found or not found), -ve on error
>> */
>> -int uclass_find_next_device(struct udevice **devp);
>> +void uclass_find_next_device(struct udevice **devp);
>> /**
>> * uclass_find_device_by_namelen() - Find uclass device based on ID
>> and name
>> diff --git a/test/dm/core.c b/test/dm/core.c
>> index 959b834576f..201daee6836 100644
>> --- a/test/dm/core.c
>> +++ b/test/dm/core.c
>> @@ -737,7 +737,7 @@ static int dm_test_device_reparent(struct
>> unit_test_state *uts)
>> /* try to get devices */
>> for (ret = uclass_find_first_device(UCLASS_TEST, &dev);
>> dev;
>> - ret = uclass_find_next_device(&dev)) {
>> + uclass_find_next_device(&dev)) {
>> ut_assert(!ret);
>
> Ditto for all the tests in test/dm/core.c.
The for loops in test/dm/core.c mostly reduce to being empty as the only
test left inside is that dev is not NULL, but in that case the for loop
terminates so the test can never fail. So the for loops can mostly be
removed as they do nothing.
Thanks,
Andrew
> Cheers,
> Quentin
More information about the U-Boot
mailing list