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