Hi Walter,
On Thu, 7 Nov 2019 at 12:47, Walter Lozano <walter.loz...@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 <s...@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
