| Message ID | 1399739875-16434-2-git-send-email-zonque@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 05/10/2014 08:37 PM, Daniel Mack wrote: > Factor out some logic into of_mdio_parse_addr() so it can be reused > later. > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/of/of_mdio.c | 37 +++++++++++++++++++++++++------------ > 1 file changed, 25 insertions(+), 12 deletions(-) > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 9a95831..171f9d5 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -114,6 +114,29 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi > return 0; > } > > +static int of_mdio_parse_addr(struct device *dev, const struct device_node *np) > +{ > + const __be32 *paddr; > + u32 addr; > + int len; > + > + /* A PHY must have a reg property in the range [0-31] */ > + paddr = of_get_property(np, "reg", &len); > + if (!paddr || len < sizeof(*paddr)) { > + dev_err(dev, "%s has invalid PHY address\n", np->full_name); > + return -EINVAL; > + } > + > + addr = be32_to_cpup(paddr); Why not just use of_property_read_u32() instead of the above? At least that's a material for a follow-up patch... > + if (addr >= PHY_MAX_ADDR) { > + dev_err(dev, "%s PHY address %i is too large\n", > + np->full_name, addr); > + return -EINVAL; > + } > + > + return addr; > +} > + > /** > * of_mdiobus_register - Register mii_bus and create PHYs from the device tree > * @mdio: pointer to mii_bus structure [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sun, 11 May 2014 00:29:02 +0400 > Hello. > > On 05/10/2014 08:37 PM, Daniel Mack wrote: > >> Factor out some logic into of_mdio_parse_addr() so it can be reused >> later. > >> Signed-off-by: Daniel Mack <zonque@gmail.com> >> --- >> drivers/of/of_mdio.c | 37 +++++++++++++++++++++++++------------ >> 1 file changed, 25 insertions(+), 12 deletions(-) > >> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >> index 9a95831..171f9d5 100644 >> --- a/drivers/of/of_mdio.c >> +++ b/drivers/of/of_mdio.c >> @@ -114,6 +114,29 @@ static int of_mdiobus_register_phy(struct mii_bus >> *mdio, struct device_node *chi >> return 0; >> } >> >> +static int of_mdio_parse_addr(struct device *dev, const struct >> device_node *np) >> +{ >> + const __be32 *paddr; >> + u32 addr; >> + int len; >> + >> + /* A PHY must have a reg property in the range [0-31] */ >> + paddr = of_get_property(np, "reg", &len); >> + if (!paddr || len < sizeof(*paddr)) { >> + dev_err(dev, "%s has invalid PHY address\n", np->full_name); >> + return -EINVAL; >> + } >> + >> + addr = be32_to_cpup(paddr); > > Why not just use of_property_read_u32() instead of the above? At least > that's a material for a follow-up patch... Agreed, I would prefer if of_property_read_u32() is used here instead of this by hand expansion. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/13/2014 07:07 PM, David Miller wrote: > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Date: Sun, 11 May 2014 00:29:02 +0400 > >> Hello. >> >> On 05/10/2014 08:37 PM, Daniel Mack wrote: >> >>> Factor out some logic into of_mdio_parse_addr() so it can be reused >>> later. >> >>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>> --- >>> drivers/of/of_mdio.c | 37 +++++++++++++++++++++++++------------ >>> 1 file changed, 25 insertions(+), 12 deletions(-) >> >>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >>> index 9a95831..171f9d5 100644 >>> --- a/drivers/of/of_mdio.c >>> +++ b/drivers/of/of_mdio.c >>> @@ -114,6 +114,29 @@ static int of_mdiobus_register_phy(struct mii_bus >>> *mdio, struct device_node *chi >>> return 0; >>> } >>> >>> +static int of_mdio_parse_addr(struct device *dev, const struct >>> device_node *np) >>> +{ >>> + const __be32 *paddr; >>> + u32 addr; >>> + int len; >>> + >>> + /* A PHY must have a reg property in the range [0-31] */ >>> + paddr = of_get_property(np, "reg", &len); >>> + if (!paddr || len < sizeof(*paddr)) { >>> + dev_err(dev, "%s has invalid PHY address\n", np->full_name); >>> + return -EINVAL; >>> + } >>> + >>> + addr = be32_to_cpup(paddr); >> >> Why not just use of_property_read_u32() instead of the above? At least >> that's a material for a follow-up patch... > > Agreed, I would prefer if of_property_read_u32() is used here instead > of this by hand expansion. > Yes, I agree as well, but I guess I can add this as a follow-up patch, so we have it separate from the code move? Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Daniel Mack <zonque@gmail.com> Date: Tue, 13 May 2014 19:21:54 +0200 > On 05/13/2014 07:07 PM, David Miller wrote: >> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> Date: Sun, 11 May 2014 00:29:02 +0400 >> >>> Hello. >>> >>> On 05/10/2014 08:37 PM, Daniel Mack wrote: >>> >>>> Factor out some logic into of_mdio_parse_addr() so it can be reused >>>> later. >>> >>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>>> --- >>>> drivers/of/of_mdio.c | 37 +++++++++++++++++++++++++------------ >>>> 1 file changed, 25 insertions(+), 12 deletions(-) >>> >>>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >>>> index 9a95831..171f9d5 100644 >>>> --- a/drivers/of/of_mdio.c >>>> +++ b/drivers/of/of_mdio.c >>>> @@ -114,6 +114,29 @@ static int of_mdiobus_register_phy(struct mii_bus >>>> *mdio, struct device_node *chi >>>> return 0; >>>> } >>>> >>>> +static int of_mdio_parse_addr(struct device *dev, const struct >>>> device_node *np) >>>> +{ >>>> + const __be32 *paddr; >>>> + u32 addr; >>>> + int len; >>>> + >>>> + /* A PHY must have a reg property in the range [0-31] */ >>>> + paddr = of_get_property(np, "reg", &len); >>>> + if (!paddr || len < sizeof(*paddr)) { >>>> + dev_err(dev, "%s has invalid PHY address\n", np->full_name); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + addr = be32_to_cpup(paddr); >>> >>> Why not just use of_property_read_u32() instead of the above? At least >>> that's a material for a follow-up patch... >> >> Agreed, I would prefer if of_property_read_u32() is used here instead >> of this by hand expansion. >> > > Yes, I agree as well, but I guess I can add this as a follow-up patch, > so we have it separate from the code move? Please repost the patch series with the requested changes made, no need to start with excessive code from the beginning only to remove it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/13/2014 07:25 PM, David Miller wrote: > From: Daniel Mack <zonque@gmail.com> >> Yes, I agree as well, but I guess I can add this as a follow-up patch, >> so we have it separate from the code move? > > Please repost the patch series with the requested changes made, no need > to start with excessive code from the beginning only to remove it. Ok, no problem. I'll wait for Florian's test results and repost. Thanks for the input, everyone! Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 9a95831..171f9d5 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -114,6 +114,29 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi return 0; } +static int of_mdio_parse_addr(struct device *dev, const struct device_node *np) +{ + const __be32 *paddr; + u32 addr; + int len; + + /* A PHY must have a reg property in the range [0-31] */ + paddr = of_get_property(np, "reg", &len); + if (!paddr || len < sizeof(*paddr)) { + dev_err(dev, "%s has invalid PHY address\n", np->full_name); + return -EINVAL; + } + + addr = be32_to_cpup(paddr); + if (addr >= PHY_MAX_ADDR) { + dev_err(dev, "%s PHY address %i is too large\n", + np->full_name, addr); + return -EINVAL; + } + + return addr; +} + /** * of_mdiobus_register - Register mii_bus and create PHYs from the device tree * @mdio: pointer to mii_bus structure @@ -148,19 +171,9 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np) /* Loop over the child nodes and register a phy_device for each one */ for_each_available_child_of_node(np, child) { - /* A PHY must have a reg property in the range [0-31] */ - paddr = of_get_property(child, "reg", &len); - if (!paddr || len < sizeof(*paddr)) { + addr = of_mdio_parse_addr(&mdio->dev, child); + if (addr < 0) { scanphys = true; - dev_err(&mdio->dev, "%s has invalid PHY address\n", - child->full_name); - continue; - } - - addr = be32_to_cpup(paddr); - if (addr >= PHY_MAX_ADDR) { - dev_err(&mdio->dev, "%s PHY address %i is too large\n", - child->full_name, addr); continue; }
Factor out some logic into of_mdio_parse_addr() so it can be reused later. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/of/of_mdio.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)