[...]
>>> @@ -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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
