Hi Walter,
On Thu, 7 Nov 2019 at 12:47, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
Thanks for your patch.
On 7/11/19 12:53, Simon Glass wrote:
These functions cannot work with of-platdata since libfdt is not available. At present when dev_read_...() functions are used it produces error messages about ofnode which is confusing.
Adjust the Makefile and header to produce an error message for the actual dev_read...() function which is called. This makes it easier to see what code needs to be converted for use with of-platdata.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None Changes in v3:
Fix eth_dev_get_mac_address() call dev_read...() only when available
drivers/core/Makefile | 4 +++- include/dm/read.h | 3 +-- net/eth-uclass.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/core/Makefile b/drivers/core/Makefile index bce7467da1..b9e4a2aab1 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -13,6 +13,8 @@ obj-$(CONFIG_OF_LIVE) += of_access.o of_addr.o ifndef CONFIG_DM_DEV_READ_INLINE obj-$(CONFIG_OF_CONTROL) += read.o endif -obj-$(CONFIG_OF_CONTROL) += of_extra.o ofnode.o read_extra.o +ifdef CONFIG_$(SPL_TPL_)OF_LIBFDT +obj-$(CONFIG_$(SPL_TPL_)OF_CONTROL) += of_extra.o ofnode.o read_extra.o +endif
ccflags-$(CONFIG_DM_DEBUG) += -DDEBUG diff --git a/include/dm/read.h b/include/dm/read.h index d37fcb504d..4f02d07d00 100644 --- a/include/dm/read.h +++ b/include/dm/read.h @@ -43,8 +43,7 @@ static inline bool dev_of_valid(struct udevice *dev) return ofnode_valid(dev_ofnode(dev)); }
-#ifndef CONFIG_DM_DEV_READ_INLINE
+#if !defined(CONFIG_DM_DEV_READ_INLINE) || CONFIG_IS_ENABLED(OF_PLATDATA) /**
- dev_read_u32() - read a 32-bit integer from a device's DT property
I don't know if it has much sense, but as I understand it should be possible to use DM without OF_CONTROL by adding U_BOOT_DEVICE entries manually in a board file. Probably this won't be useful in mainline but still could be useful in some contexts. If this is true maybe this condition should be changed. In other words why not use !CONFIG_IS_ENABLED(CONFIG_OF_LIBFDT) instead of CONFIG_IS_ENABLED(OF_PLATDATA)?
Well if a U_BOOT_DEVICE is used (as you say, not in mainline), then dev_read_...() cannot be used anyway, since there is no device-tree node.
My goal here is to cause a compile/link error showing the code that calls dev_read_...(). At present the error shows up in this header instead, which is not very helpful.
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 3bd98b01ad..e3bfcdb6cc 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -462,7 +462,7 @@ static int eth_pre_unbind(struct udevice *dev)
static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN]) { -#if IS_ENABLED(CONFIG_OF_CONTROL) +#if CONFIG_IS_ENABLED(OF_CONTROL) const uint8_t *p;
p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);Should this kind of #if be changed to
#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
Here's my thinking:
It is an error to call it with of-platdata, so my cause is to cause an error (due to the unavailability of dev_read...() functions).
That way people see at build-time that they are doing something wrong.
If we change these to avoid the problem at build time, then it becomes a runtime problem....
Regards, Simon