+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
