[U-Boot] [PATCH 11/14] usb: UniPhier: add UniPhier on-chip xHCI host driver support

[Simon Glass]

Hi Masahiro,

On 20 February 2015 at 05:12, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Tue, 17 Feb 2015 22:01:53 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
>> > +#include <common.h>
>> > +#include <linux/err.h>
>> > +#include <usb.h>
>> > +#include <fdtdec.h>
>> > +#include "xhci.h"
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +#define FDT            gd->fdt_blob
>>
>> Ick, please don't do this. Just use a local variable if you like.
>>
>> > +#define COMPAT         "panasonic,uniphier-xhci"
>>
>> This too.
>
> Why?
> Each of them appears twice in the code.

In the first case you could just a local variable as is done elsewhere
in the code:

const void *blob = gd->fdt_blob;

Then use 'blob'.

In the second case, you could easily restructure your loop to remove
the duplication. But really you should use the alias function in
fdtdec until you convert to driver model.

>
> Also, if I directly write the strings here, the line exceeds 80 columns
> and the code gets unreadable for line-wrapping.
>
>
> Anyway, this is not a big deal.
> You have already posted DM USB patch series.
> Once it is merged into the mainline, I will not have to scan a device tree
> in my own driver.

Even now you don't need to do that, as above.

>
>
>> > +
>> > +static int get_uniphier_xhci_base(int index, struct xhci_hccr **base)
>> > +{
>> > +       int offset;
>> > +
>> > +       for (offset = fdt_node_offset_by_compatible(FDT, 0, COMPAT);
>> > +            offset >= 0;
>> > +            offset = fdt_node_offset_by_compatible(FDT, offset, COMPAT)) {
>> > +               if (index == 0) {
>> > +                       *base = (struct xhci_hccr *)
>> > +                                       fdtdec_get_addr(FDT, offset, "reg");
>> > +                       return 0;
>> > +               }
>> > +               index--;
>> > +       }
>>
>> How about this"
>>
>> count = fdtdec_find_aliasses_for_id(gd->fdt_blob, "usb",
>> COMPAT_PANASONIC_XHCI, node_list, 4);
>> if (index >= count)
>>    return -ENOENT;
>> offset = node_list[index];
>>
>> then aliases will work. You need to ad the COMPAT to
>> include/fdtdec.h/c.  See ehci-tegra.c for an example.
>
>
> I do not like this idea.
>
>
> fdt_compat_id is one of what I think should be removed asap because:
>
>   - The two separete tables,  enum fdt_compat_id  and compat_names[],
>     must be maintained in sync.  If someone modifies just one of them,
>     it gets broken.  Anybody who checks those two tables are arranged
>      in the same order?
>
>   - It seems ridiculous to collect many devices from various platforms
>      into the common place.
>     If everyone starts to follow this way, the table grows too big soon.

The purpose is that we then know which drivers are directly looking at
the device tree. As we add driver model support, we remove these
strings. When the table is empty, we can remove all that support from
fdtdec, including aliases.

>
>
> Moreover, I am not happy about adding aliases to my device tree again.
>
> Rather, I am searching for an idea to avoid adding an alias to every device, like this.
> http://patchwork.ozlabs.org/patch/441922/

This is up to you.

Bu why? It allows people to make adjustments if needed. The numbering
of devices is an important thing in U-Boot - most commands use this
although I would like one day to allow devices to be named (just use
dev->name with device tree).

Anyway this is up to you.

Regards,
Simon