+Stephen Hi,
On 4 November 2015 at 01:16, Mugunthan V N <mugunthan...@ti.com> wrote: > Add new api to get device address based on index. > > Signed-off-by: Mugunthan V N <mugunthan...@ti.com> > --- > drivers/core/device.c | 16 ++++++++++++---- > include/dm/device.h | 10 ++++++++++ > 2 files changed, 22 insertions(+), 4 deletions(-) You are touching critical core code so I think we need to be careful here. > > diff --git a/drivers/core/device.c b/drivers/core/device.c > index 758f390..3cdcab5 100644 > --- a/drivers/core/device.c > +++ b/drivers/core/device.c > @@ -581,18 +581,21 @@ const char *dev_get_uclass_name(struct udevice *dev) > return dev->uclass->uc_drv->name; > } > > -fdt_addr_t dev_get_addr(struct udevice *dev) > +fdt_addr_t dev_get_addr_index(struct udevice *dev, int index) > { > #if CONFIG_IS_ENABLED(OF_CONTROL) > fdt_addr_t addr; > > if (CONFIG_IS_ENABLED(OF_TRANSLATE)) { > const fdt32_t *reg; > + int len = 0; > > - reg = fdt_getprop(gd->fdt_blob, dev->of_offset, "reg", NULL); > - if (!reg) > + reg = fdt_getprop(gd->fdt_blob, dev->of_offset, "reg", &len); > + if (!reg || (len <= (index * sizeof(fdt32_t) * 2))) > return FDT_ADDR_T_NONE; > > + reg += index * 2; > + This seems to be assuming that the address and size each occupy a single cell, but that is not the case in general (e.g. with 64-bit machines). I think you need to call fdt_addr_cells() and fdt_size_cells(). See fdtdec_get_addr_size_auto_parent() as an example. > /* > * Use the full-fledged translate function for complex > * bus setups. > @@ -608,7 +611,7 @@ fdt_addr_t dev_get_addr(struct udevice *dev) > addr = fdtdec_get_addr_size_auto_parent(gd->fdt_blob, > dev->parent->of_offset, > dev->of_offset, "reg", > - 0, NULL); > + index, NULL); > if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) { > if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS) > addr = simple_bus_translate(dev->parent, addr); > @@ -620,6 +623,11 @@ fdt_addr_t dev_get_addr(struct udevice *dev) > #endif > } > > +fdt_addr_t dev_get_addr(struct udevice *dev) > +{ > + return dev_get_addr_index(dev, 0); > +} > + > bool device_has_children(struct udevice *dev) > { > return !list_empty(&dev->child_head); > diff --git a/include/dm/device.h b/include/dm/device.h > index 28ba4ca..4de66d7 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -454,6 +454,16 @@ int device_find_next_child(struct udevice **devp); > fdt_addr_t dev_get_addr(struct udevice *dev); > > /** > + * dev_get_addr() - Get the reg property of a device dev_get_addr_index > + * > + * @dev: Pointer to a device > + * @index: reg address array index I don't think this is sufficiently clear. Can you add a comment saying that the 'reg' property can hold a list of <addr, size> pairs and @index is used to select which one is used? > + * > + * @return addr > + */ > +fdt_addr_t dev_get_addr_index(struct udevice *dev, int index); > + > +/** > * device_has_children() - check if a device has any children > * > * @dev: Device to check > -- > 2.6.2.280.g74301d6 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot