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

[Igor Grinberg]

[...]

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

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

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.
Implementing the stub in fdtdec.h can be a bonus for all of us...

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

[...]


-- 
Regards,
Igor.