| Message ID | 20200527064140.21391-1-ovidiu.panait@windriver.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] dm: core: Fix devfdt_get_addr_ptr return value | expand |
On 27/05/2020 08:41, Ovidiu Panait 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 at windriver.com> Reviewed-by: Matthias Brugger <mbrugger at suse.com> Simon can you have a look. If it's OK for you I can queue the series through my tree. Regards, Matthias > --- > > 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(-) > > diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_ast2500.c > index ccfeded30c..284f5f58d6 100644 > --- a/drivers/clk/aspeed/clk_ast2500.c > +++ b/drivers/clk/aspeed/clk_ast2500.c > @@ -498,8 +498,8 @@ static int ast2500_clk_ofdata_to_platdata(struct udevice *dev) > struct ast2500_clk_priv *priv = dev_get_priv(dev); > > priv->scu = devfdt_get_addr_ptr(dev); > - if (IS_ERR(priv->scu)) > - return PTR_ERR(priv->scu); > + if (!priv->scu) > + return -EINVAL; > > return 0; > } > diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c > index dfcb868f65..044d5871b0 100644 > --- a/drivers/core/fdtaddr.c > +++ b/drivers/core/fdtaddr.c > @@ -14,6 +14,7 @@ > #include <log.h> > #include <asm/io.h> > #include <dm/device-internal.h> > +#include <mapmem.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -154,7 +155,9 @@ fdt_addr_t devfdt_get_addr(const struct udevice *dev) > > void *devfdt_get_addr_ptr(const struct udevice *dev) > { > - return (void *)(uintptr_t)devfdt_get_addr_index(dev, 0); > + fdt_addr_t addr = devfdt_get_addr_index(dev, 0); > + > + return (addr == FDT_ADDR_T_NONE) ? NULL : map_sysmem(addr, 0); > } > > void *devfdt_remap_addr_index(const struct udevice *dev, int index) > diff --git a/drivers/i2c/ast_i2c.c b/drivers/i2c/ast_i2c.c > index 214362d04b..253e653666 100644 > --- a/drivers/i2c/ast_i2c.c > +++ b/drivers/i2c/ast_i2c.c > @@ -93,8 +93,8 @@ static int ast_i2c_ofdata_to_platdata(struct udevice *dev) > int ret; > > priv->regs = devfdt_get_addr_ptr(dev); > - if (IS_ERR(priv->regs)) > - return PTR_ERR(priv->regs); > + if (!priv->regs) > + return -EINVAL; > > ret = clk_get_by_index(dev, 0, &priv->clk); > if (ret < 0) { > diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c > index 2206e958ec..ac0377e796 100644 > --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c > +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c > @@ -194,7 +194,7 @@ int mvebu_pinctl_probe(struct udevice *dev) > } > > priv->base_reg = devfdt_get_addr_ptr(dev); > - if (priv->base_reg == (void *)FDT_ADDR_T_NONE) { > + if (!priv->base_reg) { > debug("%s: Failed to get base address\n", __func__); > return -EINVAL; > } > diff --git a/drivers/timer/ast_timer.c b/drivers/timer/ast_timer.c > index 3838601f54..9f28cbfcf9 100644 > --- a/drivers/timer/ast_timer.c > +++ b/drivers/timer/ast_timer.c > @@ -65,8 +65,8 @@ static int ast_timer_ofdata_to_platdata(struct udevice *dev) > struct ast_timer_priv *priv = dev_get_priv(dev); > > priv->regs = devfdt_get_addr_ptr(dev); > - if (IS_ERR(priv->regs)) > - return PTR_ERR(priv->regs); > + if (!priv->regs) > + return -EINVAL; > > priv->tmc = ast_get_timer_counter(priv->regs, AST_TICK_TIMER); > > diff --git a/drivers/watchdog/ast_wdt.c b/drivers/watchdog/ast_wdt.c > index 7e11465a57..a21f9a4d14 100644 > --- a/drivers/watchdog/ast_wdt.c > +++ b/drivers/watchdog/ast_wdt.c > @@ -91,8 +91,8 @@ static int ast_wdt_ofdata_to_platdata(struct udevice *dev) > struct ast_wdt_priv *priv = dev_get_priv(dev); > > priv->regs = devfdt_get_addr_ptr(dev); > - if (IS_ERR(priv->regs)) > - return PTR_ERR(priv->regs); > + if (!priv->regs) > + return -EINVAL; > > return 0; > } >
On 5/27/20 8:41 AM, Ovidiu Panait 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 at windriver.com> --- Reviewed-by: C?dric Le Goater <clg at kaod.org> > > 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(-) > > diff --git a/drivers/clk/aspeed/clk_ast2500.c > b/drivers/clk/aspeed/clk_ast2500.c index ccfeded30c..284f5f58d6 > 100644 --- a/drivers/clk/aspeed/clk_ast2500.c +++ > b/drivers/clk/aspeed/clk_ast2500.c @@ -498,8 +498,8 @@ static int > ast2500_clk_ofdata_to_platdata(struct udevice *dev) struct > ast2500_clk_priv *priv = dev_get_priv(dev); > > priv->scu = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->scu)) - > return PTR_ERR(priv->scu); + if (!priv->scu) + return -EINVAL; > > return 0; } diff --git a/drivers/core/fdtaddr.c > b/drivers/core/fdtaddr.c index dfcb868f65..044d5871b0 100644 --- > a/drivers/core/fdtaddr.c +++ b/drivers/core/fdtaddr.c @@ -14,6 +14,7 > @@ #include <log.h> #include <asm/io.h> #include > <dm/device-internal.h> +#include <mapmem.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -154,7 +155,9 @@ fdt_addr_t devfdt_get_addr(const struct udevice > *dev) > > void *devfdt_get_addr_ptr(const struct udevice *dev) { - return (void > *)(uintptr_t)devfdt_get_addr_index(dev, 0); + fdt_addr_t addr = > devfdt_get_addr_index(dev, 0); + + return (addr == FDT_ADDR_T_NONE) ? > NULL : map_sysmem(addr, 0); } > > void *devfdt_remap_addr_index(const struct udevice *dev, int index) > diff --git a/drivers/i2c/ast_i2c.c b/drivers/i2c/ast_i2c.c index > 214362d04b..253e653666 100644 --- a/drivers/i2c/ast_i2c.c +++ > b/drivers/i2c/ast_i2c.c @@ -93,8 +93,8 @@ static int > ast_i2c_ofdata_to_platdata(struct udevice *dev) int ret; > > priv->regs = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->regs)) - > return PTR_ERR(priv->regs); + if (!priv->regs) + return -EINVAL; > > ret = clk_get_by_index(dev, 0, &priv->clk); if (ret < 0) { diff --git > a/drivers/pinctrl/mvebu/pinctrl-mvebu.c > b/drivers/pinctrl/mvebu/pinctrl-mvebu.c index 2206e958ec..ac0377e796 > 100644 --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c +++ > b/drivers/pinctrl/mvebu/pinctrl-mvebu.c @@ -194,7 +194,7 @@ int > mvebu_pinctl_probe(struct udevice *dev) } > > priv->base_reg = devfdt_get_addr_ptr(dev); - if (priv->base_reg == > (void *)FDT_ADDR_T_NONE) { + if (!priv->base_reg) { debug("%s: Failed > to get base address\n", __func__); return -EINVAL; } diff --git > a/drivers/timer/ast_timer.c b/drivers/timer/ast_timer.c index > 3838601f54..9f28cbfcf9 100644 --- a/drivers/timer/ast_timer.c +++ > b/drivers/timer/ast_timer.c @@ -65,8 +65,8 @@ static int > ast_timer_ofdata_to_platdata(struct udevice *dev) struct > ast_timer_priv *priv = dev_get_priv(dev); > > priv->regs = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->regs)) - > return PTR_ERR(priv->regs); + if (!priv->regs) + return -EINVAL; > > priv->tmc = ast_get_timer_counter(priv->regs, AST_TICK_TIMER); > > diff --git a/drivers/watchdog/ast_wdt.c b/drivers/watchdog/ast_wdt.c > index 7e11465a57..a21f9a4d14 100644 --- a/drivers/watchdog/ast_wdt.c > +++ b/drivers/watchdog/ast_wdt.c @@ -91,8 +91,8 @@ static int > ast_wdt_ofdata_to_platdata(struct udevice *dev) struct ast_wdt_priv > *priv = dev_get_priv(dev); > > priv->regs = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->regs)) - > return PTR_ERR(priv->regs); + if (!priv->regs) + return -EINVAL; > > return 0; } >
On Wed, 27 May 2020 at 00:45, Ovidiu Panait <ovidiu.panait at 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 at 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 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. 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: Tested on sandbox Tested-by: Simon Glass <sjg at 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/
On 5/28/20 10:32 AM, Simon Glass wrote: > On Wed, 27 May 2020 at 00:45, Ovidiu Panait <ovidiu.panait at 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 at 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 at 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
Hi Sean, On Thu, 28 May 2020 at 13:29, Sean Anderson <seanga2 at gmail.com> wrote: > > On 5/28/20 10:32 AM, Simon Glass wrote: > > On Wed, 27 May 2020 at 00:45, Ovidiu Panait <ovidiu.panait at 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 at 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 Oops sorry. > > > > > > 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. Yes that's right...it is a test for dev_read_addr_ptr(), although in one case it calls the latter. > > > > > > Tested on sandbox > > Tested-by: Simon Glass <sjg at 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 > Regards, Simon
diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_ast2500.c index ccfeded30c..284f5f58d6 100644 --- a/drivers/clk/aspeed/clk_ast2500.c +++ b/drivers/clk/aspeed/clk_ast2500.c @@ -498,8 +498,8 @@ static int ast2500_clk_ofdata_to_platdata(struct udevice *dev) struct ast2500_clk_priv *priv = dev_get_priv(dev); priv->scu = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->scu)) - return PTR_ERR(priv->scu); + if (!priv->scu) + return -EINVAL; return 0; } diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c index dfcb868f65..044d5871b0 100644 --- a/drivers/core/fdtaddr.c +++ b/drivers/core/fdtaddr.c @@ -14,6 +14,7 @@ #include <log.h> #include <asm/io.h> #include <dm/device-internal.h> +#include <mapmem.h> DECLARE_GLOBAL_DATA_PTR; @@ -154,7 +155,9 @@ fdt_addr_t devfdt_get_addr(const struct udevice *dev) void *devfdt_get_addr_ptr(const struct udevice *dev) { - return (void *)(uintptr_t)devfdt_get_addr_index(dev, 0); + fdt_addr_t addr = devfdt_get_addr_index(dev, 0); + + return (addr == FDT_ADDR_T_NONE) ? NULL : map_sysmem(addr, 0); } void *devfdt_remap_addr_index(const struct udevice *dev, int index) diff --git a/drivers/i2c/ast_i2c.c b/drivers/i2c/ast_i2c.c index 214362d04b..253e653666 100644 --- a/drivers/i2c/ast_i2c.c +++ b/drivers/i2c/ast_i2c.c @@ -93,8 +93,8 @@ static int ast_i2c_ofdata_to_platdata(struct udevice *dev) int ret; priv->regs = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->regs)) - return PTR_ERR(priv->regs); + if (!priv->regs) + return -EINVAL; ret = clk_get_by_index(dev, 0, &priv->clk); if (ret < 0) { diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c index 2206e958ec..ac0377e796 100644 --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c @@ -194,7 +194,7 @@ int mvebu_pinctl_probe(struct udevice *dev) } priv->base_reg = devfdt_get_addr_ptr(dev); - if (priv->base_reg == (void *)FDT_ADDR_T_NONE) { + if (!priv->base_reg) { debug("%s: Failed to get base address\n", __func__); return -EINVAL; } diff --git a/drivers/timer/ast_timer.c b/drivers/timer/ast_timer.c index 3838601f54..9f28cbfcf9 100644 --- a/drivers/timer/ast_timer.c +++ b/drivers/timer/ast_timer.c @@ -65,8 +65,8 @@ static int ast_timer_ofdata_to_platdata(struct udevice *dev) struct ast_timer_priv *priv = dev_get_priv(dev); priv->regs = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->regs)) - return PTR_ERR(priv->regs); + if (!priv->regs) + return -EINVAL; priv->tmc = ast_get_timer_counter(priv->regs, AST_TICK_TIMER); diff --git a/drivers/watchdog/ast_wdt.c b/drivers/watchdog/ast_wdt.c index 7e11465a57..a21f9a4d14 100644 --- a/drivers/watchdog/ast_wdt.c +++ b/drivers/watchdog/ast_wdt.c @@ -91,8 +91,8 @@ static int ast_wdt_ofdata_to_platdata(struct udevice *dev) struct ast_wdt_priv *priv = dev_get_priv(dev); priv->regs = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->regs)) - return PTR_ERR(priv->regs); + if (!priv->regs) + return -EINVAL; return 0; }
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 at 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(-)