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

[Simon Glass]

Hi Masahiro,

On 21 November 2014 08:04, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> 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?

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