Hi Masahiro, On 23 February 2015 at 21:54, Masahiro Yamada <[email protected]> wrote: > Hi Simon, > > > On Fri, 20 Feb 2015 10:21:27 -0700 > Simon Glass <[email protected]> wrote: > >> Hi Masahiro, >> >> On 20 February 2015 at 05:12, Masahiro Yamada <[email protected]> >> wrote: >> > Hi Simon, >> > >> > >> > >> > On Tue, 17 Feb 2015 22:01:53 -0700 >> > Simon Glass <[email protected]> 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'. > > Sounds reasonable. > > >> 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. > > OK. > > I am still not convinced with one thing of the fdtdec_find_aliases_for_id(). > > I think it could take the compatibility string directly like this: > > node_offset = fdtdec_find_aliases_for_id(gd->fdt_blob, "usb", > "panasonic,uniphier-xhci", index); > > > Anyway, that is OK if it is temporal.
Yes it could - the current approach is really just to mark what needs conversion. > > I used this helper function in v4. > http://patchwork.ozlabs.org/patch/442791/ > > I hope DM USB is supported in the mainline soon. I think it will be. I posted a series a few weeks back and Vivek posted a different one. I'll ping him to see if he's had a look. > > > >> > >> > >> >> > + >> >> > +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. >> > > OK. If you assume this tabile is like a TODO list, it makes sense. Yes, we should remove it. I am chipping away at it - this morning I sent a series that removed two fo them. > > > > >> > >> > 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. > > I read your explanation and I am convinced. > > Let's do not apply this series. OK. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
