Re: [U-Boot] [PATCH v2 2/3] dm: add a command to list devices in a tree-like format

Hi Masahiro,

On 21 November 2014 08:04, Masahiro Yamada yamada.m@jp.panasonic.com wrote:

Hi Simon,

On Thu, 20 Nov 2014 15:00:41 +0000 Simon Glass sjg@chromium.org wrote:

...

Hi Masahiro,

On 20 November 2014 12:20, Masahiro Yamada yamada.m@jp.panasonic.com wrote:

...

We are adding more and more drivers to driver model these days. Compared with the ad-hoc driver system, it seems pretty difficult to understand how drivers are working on driver model. For ex.

• Which devices have been bound?
• Which devices have been probed?
• Which is the parent of this device?

etc.

I hope this tool will help us test/review driver model patches.

Just hit "showdev" on the command line and it will list all the bound devices in a tree-like format with Class and Probed flag.

This looks very similar to the 'dm tree' command. Can we unify these? Perhaps move the commands into common/cmd_dm.c? Then we can use the CONFIG_CMD_DM option instead of a new one.

Also I think 'dm xxx' is better than things 'showdev'. The 'dm' prefix can be used for all DM commands.

Sorry, I did not know you had already implemented a similar one. There is no point to have two commands to do equivalent stuff.

The difference is some appearances and that pointer addresses are missing from mine.

I think my 4 columns indentation is more readable, but it might be my personal preference.

So, if you prefer the current one, feel free to reject mine, of course. Or shall I change "dm tree" to something like mine?

What shall I do?

Yours is prettier - I've been meaning to fix the formatting one day. So let's go with yours if you you can address the comments and overwrite or adjust 'dm tree'.

...

...

+#include <common.h> +#include <linux/string.h> +#include <linux/list.h> +#include <dm/device.h> +#include <dm/uclass.h>

+DECLARE_GLOBAL_DATA_PTR;

+#define INDENT1 " " +#define INDENT2 "| " +#define INDENT3 "\-- " +#define INDENT4 "|-- "

Is there really any value in these?

Not really. I just thought that it might be helpful if we want to change the indentation width.

...

...

+static void show_devices(struct udevice *dev, int depth, int last_flag) +{

•   int i, is_last;
  

•   struct udevice *child;
  

•   char class_name[12];
  

•   /* print the first 11 characters to not break the tree-format. */
  

•   strlcpy(class_name, dev->uclass->uc_drv->name, sizeof(class_name));
  

•   printf(" %-11s ", class_name);
  

•   printf("[ %c ]    ", dev->flags & DM_FLAG_ACTIVATED ? '+' : ' ');
  

This line could be combined with the above.

Yes, indeed.

...

...

•   for (i = depth; i >= 0; i--) {
  

•           is_last = (last_flag >> i) & 1;
  

•           if (i) {
  

•                   if (is_last)
  

•                           printf(INDENT1);
  

•                   else
  

•                           printf(INDENT2);
  

•           } else {
  

•                   if (is_last)
  

•                           printf(INDENT3);
  

•                   else
  

•                           printf(INDENT4);
  

•           }
  

•   }
  

•   printf("%s\n", dev->name);
  

•   list_for_each_entry(child, &dev->child_head, sibling_node) {
  

•           is_last = list_is_last(&child->sibling_node, &dev->child_head);
  

•           show_devices(child, depth + 1, (last_flag << 1) | is_last);
  

•   }
  

+}

+static int do_showdev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{

•   printf(" Class       Probed   Name\n");
  

Remove the first space?

I inserted the first space for readability, but it might not be necessary.

BTW, I read your code but I could not understand why you used list_for_each_entry_safe() in display_succ(), whilst list_for_each_entry() in do_dm_dump_uclass().

Is there a special reason for inconsintency?

I don't think we need the safe version. Previously the code may have probed devices as it worked, which could cause problems. But that was a bug.

Regards, Simon