Hi Simon
On 29/12/19 22:21, Simon Glass wrote:
Hi Walter,
On Thu, 7 Nov 2019 at 12:47, Walter Lozano <[email protected]> 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 <[email protected]>
---
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.
I now understand your point, thanks for your clarification.
I think that your approach will be help to improve the OF_PLATDATA
support, which is great, however I'm still looking for a nice way to
allow to take advantage of this kind of improvements when using
U_BOOT_DEVICE. If I get some time I will work on this and ask for your
review.
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....
Yes, you are right, thanks for your explanation.
Regards,
Regards,
Simon
