[PATCH 1/5] dm: core: Fix devfdt_get_addr_ptr return value

[Sean Anderson <seanga2@gmail.com>]

On 5/28/20 10:32 AM, Simon Glass wrote:
> On Wed, 27 May 2020 at 00:45, Ovidiu Panait <ovidiu.panait@windriver.com> wrote:
>>
>> According to the description of devfdt_get_addr_ptr, this function should
>> return NULL on failure, but currently it returns (void *)FDT_ADDR_T_NONE.
>>
>> This is also a problem because there are two definitions for
>> dev_read_addr_ptr, depending on CONFIG_DM_DEV_READ_INLINE:
>>
>> 1. one returning NULL on failure (drivers/core/read.c):
>> void *dev_read_addr_ptr(const struct udevice *dev)
>> {
>>         fdt_addr_t addr = dev_read_addr(dev);
>>
>>         return (addr == FDT_ADDR_T_NONE) ? NULL : map_sysmem(addr, 0);
>> }
>>
>> 2. another one, which is a wrapper over devfdt_get_addr_ptr, returning
>> (void *)FDT_ADDR_T_NONE (include/dm/read.h)
>>
>> static inline void *dev_read_addr_ptr(const struct udevice *dev)
>> {
>>         return devfdt_get_addr_ptr(dev);
>> }
>>
>> Currently, some drivers which make use of devfdt_get_addr_ptr check the
>> return value for NULL:
>> drivers/i2c/mvtwsi.c
>> drivers/i2c/designware_i2c.c
>> drivers/usb/host/ehci-zynq.c
>>
>> while others check the return value for (void *)FDT_ADDR_T_NONE:
>> drivers/pinctrl/mvebu/pinctrl-mvebu.c
>> drivers/timer/ast_timer.c
>> drivers/watchdog/ast_wdt.c
>>
>> Fix this by making devfdt_get_addr_ptr return NULL on failure, as
>> described in the function comments. Also, update the drivers currently
>> checking (void *)FDT_ADDR_T_NONE to check for NULL.
>>
>> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
>> ---
>>
>>  drivers/clk/aspeed/clk_ast2500.c      | 4 ++--
>>  drivers/core/fdtaddr.c                | 5 ++++-
>>  drivers/i2c/ast_i2c.c                 | 4 ++--
>>  drivers/pinctrl/mvebu/pinctrl-mvebu.c | 2 +-
>>  drivers/timer/ast_timer.c             | 4 ++--
>>  drivers/watchdog/ast_wdt.c            | 4 ++--
>>  6 files changed, 13 insertions(+), 10 deletions(-)
>>
> 
> +Sean Anderson

It looks like I never actually got CC'd

> 
> I did already review another patch on this topic[1].  I'm not sure
> when that is going to land though. Perhaps this one should be rebased
> on that? Or Matthias can check with the other custodian to see what is
> happening with that series.

Hopefully soon; I believe the last holdup was passing CI.

> 
> I've sent a patch for a test which detects the inconsistency[2]. It is
> very easy to write a new test so I encourage people to do that
> whenever there is a fix like this. So with that:

I think that's a good idea. However, from what I can see, that patch
tests dev_read_addr_ptr, not devfdt_get_addr_ptr. You may also want to
check the error case, to make sure FDT_ADDR_T_NONE gets returned.

> 
> Tested on sandbox
> Tested-by: Simon Glass <sjg@chromium.org>
> 
> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200521161503.384823-10-seanga2 at gmail.com/
> [2] http://patchwork.ozlabs.org/project/uboot/patch/20200528082045.1.Ibd999a0382164a37d1e59c00c8c7a9ff92b8f53a at changeid/
> 

--Sean