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

[Masahiro Yamada]

Hi Simon,


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

> Hi Masahiro,
> 
> On 20 November 2014 12:20, Masahiro Yamada <yamada.m at 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?



> > +
> > +#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?


Best Regards
Masahiro Yamada