[U-Boot] [PATCH v2 04/16] dm: core: Add a new api to get indexed device address

[Mugunthan V N <mugunthanvnm@ti.com>]

On Friday 06 November 2015 05:37 PM, Simon Glass wrote:
> +Stephen
> 
> Hi,
> 
> On 4 November 2015 at 01:16, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>> Add new api to get device address based on index.
>>
>> Signed-off-by: Mugunthan V N <mugunthanvnm@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.
> 

Okay

>>
>> 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.
> 

Okay, will use these in next version.

>>                 /*
>>                  * 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?
> 

Will add more doc in next version

Regards
Mugunthan V N