[U-Boot] [PATCH v2] dm:gpio:mxc add DT support

[Peng Fan <B51431@freescale.com>]

Hi Igor,

On 1/21/2015 5:18 PM, Igor Grinberg wrote:
> [...]
>
>>>> @@ -295,12 +282,77 @@ static int mxc_gpio_probe(struct udevice *dev)
>>>>        return 0;
>>>>    }
>>>>    +#ifdef CONFIG_OF_CONTROL
>>>> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device)
>>>> +{
>>>> +    fdt_addr_t addr;
>>>> +    addr = fdtdec_get_addr(gd->fdt_blob, device->of_offset, "reg");
>>>> +    if (addr == FDT_ADDR_T_NONE)
>>>> +        return NULL;
>>>> +    else
>>>> +        return (struct gpio_regs *)addr;
>>>> +}
>>>> +#else
>>>> +static struct gpio_regs *mxc_get_gpio_addr(struct udevice *device)
>>>> +{
>>>> +    return NULL;
>>>> +}
>>>> +#endif
>>> In general, I'm fine with this concept, but I believe we should implement
>>> a stub for fdtdec_get_addr() function in the fdtdec.h (say just returning
>>> FDT_ADDR_T_NONE), as otherwise we might end up with multiple drivers
>>> implementing the same noop callback just to work around a poor fdtdec_*()
>>> interface.
>> I tried to implement a stub function in fdtdec.h like this:
>> __weak fdt_addr_t fdtdec_get_addr_wrap(xxxx)
>> {
>>      return FDT_ADDR_T_NONE;
>> }
>> And in driver code, implement non weak version as following:
>> #ifdef CONFIG_OF_CONTROL
>> fdt_addr_t fdtdec_get_addr_wrap(xxxx)
>> {
>>      ..........
>> }
>> #endif
>> But gcc complains about conficting types, since we have a weak
>> implementation in header file and a strong implementation in c file.
>> If the weak one is in fdtxx c file, no error, but i thinke this is not
>> a good idea to put this in fdtxx c file. If we do not want DT,
>> but only DM, DT code should not be compiled into the final image.
> Right. Putting the __weak function inside fdtxx c file will not work either
> as it is not compiled for !CONFIG_OF_CONTROL.
>
>> I tried another way, add the following piece code in
>> driver/core/device.c and function prototype in device.h,
>> "
>> #ifdef CONFIG_OF_CONTROL
>> void *dev_reg_addr(struct udevice *dev)
>> {
>>      fdt_addr_t addr;
>>
>>      addr = fdtdev_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>>      if (addr == FDT_ADDR_T_NONE)
>>          return NULL;
>>      else
>>          return (void *)addr
>> }
>> #else
>> void *dev_reg_addr(struct udevice *dev)
>> {
>>      return NULL;
>> }
>> #endif
>> "
>> I think `#ifdef` is needed here. I think this way is better that put
>> stub function in fdtdec.h. Using this way, the driver code can just
>> `add = dev_reg_addr(device)` to get reg address.
> You will need to check "if (!add) ..." in the driver anyway...
Yeah.
>
> Yes, I agree - abstracting the dev_reg_addr() function is a great idea!
> It will improve the situation for all drivers that will use dev_get_addr().
ok. I'll use the upper code and make a single patch in v3 patch set.
>
> Also, I think that in *addition* to the above, implementing a stub for
> fdtdev_get_addr() in fdtdec.h will make it even better, so you will
> not need the ifdef in driver/core/device.c too and also improve the
> fdtdec interface flexibility for any other (whatever will it be) case
> the driver/other code will need to call fdtdev_get_addr() explicitly.
>
> Having said the above, I must say that I'm really a fan of how Linux
> interfaces deals with the CONFIG_* options, especially DT related ones.
>
> So, I think that implementing your idea in driver/core/device.c is
> good enough for merging.
Thanks.
> Implementing the stub in fdtdec.h can be a bonus for all of us...
I does not come out a good idea about this, so this will not be in the 
v3 patch set.  Make patch small:)
>> Maybe the upper piece code should be put in a new file named
>> device-util.c in directory device/core but not device.c?
>>
> Well, I think new file will not have any real improvement over the
> above ideas and concepts.
>
> [...]
>
>
Thanks,
Peng.