[U-Boot] [PATCH v3 1/4] dm: introduce dev_get_addr interface

[Simon Glass]

Hi Peng,

On 5 February 2015 at 23:44, Peng Fan <B51431 at freescale.com> wrote:
> Hi, Simon
>
>
> On 1/23/2015 5:25 AM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 21 January 2015 at 04:09, Peng Fan <Peng.Fan at freescale.com> wrote:
>>>
>>> Abstracting dev_get_addr can improve drivers that want to
>>> get device's address.
>>>
>>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>>> ---
>>>   drivers/core/device.c | 19 +++++++++++++++++++
>>>   1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index 963b16f..0ba5c76 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -12,6 +12,7 @@
>>>   #include <common.h>
>>>   #include <fdtdec.h>
>>>   #include <malloc.h>
>>> +#include <libfdt.h>
>>>   #include <dm/device.h>
>>>   #include <dm/device-internal.h>
>>>   #include <dm/lists.h>
>>> @@ -390,3 +391,21 @@ ulong dev_get_of_data(struct udevice *dev)
>>>   {
>>>          return dev->of_id->data;
>>>   }
>>> +
>>> +#ifdef CONFIG_OF_CONTROL
>>> +void *dev_get_addr(struct udevice *dev)
>>
>> My approach so far has been to use a ulong for the device address
>> (e.g. in platform data) and only use a pointer when we know the type
>> (e.g. struct disp_ctlr *), typically in driver-private data.
>>
>> So do you think it would be better to return FDT_ADDR_T_NONE?
>
> Sorry for the long time delay to reply.
> Do you agree this way using ulong as the return type?
> "
> #ifdef CONFIG_OF_CONTROL
> unsigned long dev_get_addr(struct udevice *dev)
> {
>     fdt_addr_t addr;
>     addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>     return addr;
> }
> #else
> unsigned long dev_get_addr(struct udevice *dev)
> {
>     return FDT_ADDR_T_NONE;
> }
> #endif
> "

Sure, but can you return fdt_addr_t?

> Is it better to move this piece of code to include/dm/device.h and using
> static inline prototype? or put them still in driver/core/device.c?

I'd suggest:
- static inline in the header for the case where CONFIG_OF_CONTROL is
not defined
- full implementation in device.c when that is defined

>>>
>>> +{
>>> +       fdt_addr_t addr;
>>> +
>>> +       addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>>> +       if (addr == FDT_ADDR_T_NONE)
>>> +               return NULL;
>>> +       else
>>> +               return (void *)addr;
>>> +}
>>> +#else
>>> +void *dev_get_addr(struct udevice *dev)
>>> +{
>>> +       return NULL;
>>> +}
>>> +#endif
>>> --
>>> 1.8.4

Regards,
Simon