[U-Boot] [PATCH v4 4/5] dm: core: Don't include ofnode functions with of-platdata

[Walter Lozano]

Hi Simon

On 29/12/19 22:21, Simon Glass wrote:
> Hi Walter,
>
> On Thu, 7 Nov 2019 at 12:47, Walter Lozano <walter.lozano at 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 at 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.


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