Re: [PATCH v5 06/15] stdio: Fix class iteration in stdio_add_devices()

[Simon Glass]

Hi Michal,

On Tue, 27 Sept 2022 at 15:38, Michal Suchanek  wrote:
>
> There is a complaint in the code that iterates keyboards that we don't
> have the _check variant of class iterator but we in fact do, use it.
>
> In the code that iterates video devices there is an attempt to print
> errors but the simple iterator does not return a device when there is an
> error. Use the _check variant of the iterator as well.
>
> Also print the symbolic error.

Again please drop this.

>
> Fixes: b206cd7372 ("dm: stdio: Plumb in the new keyboard uclass")
> Fixes: e3b81c1c0d ("dm: stdio: video: Plumb the video uclass into stdio")

I think overall you are a bit too zealous with your Fixes tags. The
comment was written in 2015 and the function you mention was added in
2017.

> Signed-off-by: Michal Suchanek 
> ---
>  common/stdio.c | 33 ++---
>  1 file changed, 14 insertions(+), 19 deletions(-)

With the above fixed:

Reviewed-by: Simon Glass 

[PATCH v5 06/15] stdio: Fix class iteration in stdio_add_devices()

[Michal Suchanek]

There is a complaint in the code that iterates keyboards that we don't
have the _check variant of class iterator but we in fact do, use it.

In the code that iterates video devices there is an attempt to print
errors but the simple iterator does not return a device when there is an
error. Use the _check variant of the iterator as well.

Also print the symbolic error.

Fixes: b206cd7372 ("dm: stdio: Plumb in the new keyboard uclass")
Fixes: e3b81c1c0d ("dm: stdio: video: Plumb the video uclass into stdio")
Signed-off-by: Michal Suchanek 
---
 common/stdio.c | 33 ++---
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/common/stdio.c b/common/stdio.c
index 92161a0df8..17f0aef76d 100644
--- a/common/stdio.c
+++ b/common/stdio.c
@@ -306,7 +306,6 @@ int stdio_init_tables(void)
 int stdio_add_devices(void)
 {
struct udevice *dev;
-   struct uclass *uc;
int ret;
 
if (IS_ENABLED(CONFIG_DM_KEYBOARD)) {
@@ -316,24 +315,18 @@ int stdio_add_devices(void)
 * have a list of input devices to start up in the stdin
 * environment variable. That work probably makes more sense
 * when stdio itself is converted to driver model.
-*
-* TODO(s...@chromium.org): Convert changing
-* uclass_first_device() etc. to return the device even on
-* error. Then we could use that here.
 */
-   ret = uclass_get(UCLASS_KEYBOARD, );
-   if (ret)
-   return ret;
 
/*
 * Don't report errors to the caller - assume that they are
 * non-fatal
 */
-   uclass_foreach_dev(dev, uc) {
-   ret = device_probe(dev);
+   for (ret = uclass_first_device_check(UCLASS_KEYBOARD, );
+   dev;
+   ret = uclass_next_device_check()) {
if (ret)
-   printf("Failed to probe keyboard '%s'\n",
-  dev->name);
+   printf("%s: Failed to probe keyboard '%s' 
(ret=%d %s)\n",
+  __func__, dev->name, ret, 
errno_str(ret));
}
}
 #if CONFIG_IS_ENABLED(SYS_I2C_LEGACY)
@@ -353,13 +346,15 @@ int stdio_add_devices(void)
int ret;
 
if (!IS_ENABLED(CONFIG_SYS_CONSOLE_IS_IN_ENV)) {
-   for (ret = uclass_first_device(UCLASS_VIDEO, );
-vdev;
-ret = uclass_next_device())
-   ;
-   if (ret)
-   printf("%s: Video device failed (ret=%d)\n",
-  __func__, ret);
+   for (ret = uclass_first_device_check(UCLASS_VIDEO,
+);
+   vdev;
+   ret = uclass_next_device_check()) {
+   if (ret)
+   printf("%s: Failed to probe video 
device '%s' (ret=%d %s)\n",
+  __func__, dev->name,
+  ret, errno_str(ret));
+   }
}
if (IS_ENABLED(CONFIG_SPLASH_SCREEN) &&
IS_ENABLED(CONFIG_CMD_BMP))
-- 
2.37.3