* [PATCH net-next] net: phy: adjust fixed_phy_register() return value @ 2014-10-01 21:45 Petri Gynther 2014-10-01 21:56 ` Florian Fainelli 2014-10-04 12:07 ` Thomas Petazzoni 0 siblings, 2 replies; 7+ messages in thread From: Petri Gynther @ 2014-10-01 21:45 UTC (permalink / raw) To: netdev; +Cc: davem, f.fainelli, thomas.petazzoni Adjust fixed_phy_register() to return struct phy_device *, so that it becomes easy to use fixed PHYs without device tree support: phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL); fixed_phy_set_link_update(phydev, fixed_phy_link_update); phy_connect_direct(netdev, phydev, handler_fn, phy_interface); This change is a prerequisite for modifying bcmgenet driver to work without a device tree on Broadcom's MIPS-based 7xxx platforms. Signed-off-by: Petri Gynther <pgynther@google.com> --- drivers/net/phy/fixed.c | 16 ++++++++-------- drivers/of/of_mdio.c | 7 +++++-- include/linux/phy_fixed.h | 14 +++++++------- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c index 5b19fbb..47872caa 100644 --- a/drivers/net/phy/fixed.c +++ b/drivers/net/phy/fixed.c @@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(fixed_phy_del); static int phy_fixed_addr; static DEFINE_SPINLOCK(phy_fixed_addr_lock); -int fixed_phy_register(unsigned int irq, - struct fixed_phy_status *status, - struct device_node *np) +struct phy_device *fixed_phy_register(unsigned int irq, + struct fixed_phy_status *status, + struct device_node *np) { struct fixed_mdio_bus *fmb = &platform_fmb; struct phy_device *phy; @@ -246,19 +246,19 @@ int fixed_phy_register(unsigned int irq, spin_lock(&phy_fixed_addr_lock); if (phy_fixed_addr == PHY_MAX_ADDR) { spin_unlock(&phy_fixed_addr_lock); - return -ENOSPC; + return ERR_PTR(-ENOSPC); } phy_addr = phy_fixed_addr++; spin_unlock(&phy_fixed_addr_lock); ret = fixed_phy_add(PHY_POLL, phy_addr, status); if (ret < 0) - return ret; + return ERR_PTR(ret); phy = get_phy_device(fmb->mii_bus, phy_addr, false); if (!phy || IS_ERR(phy)) { fixed_phy_del(phy_addr); - return -EINVAL; + return ERR_PTR(-EINVAL); } of_node_get(np); @@ -269,10 +269,10 @@ int fixed_phy_register(unsigned int irq, phy_device_free(phy); of_node_put(np); fixed_phy_del(phy_addr); - return ret; + return ERR_PTR(ret); } - return 0; + return phy; } static int __init fixed_mdio_bus_init(void) diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index a85d800..907baa9 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -286,6 +286,7 @@ int of_phy_register_fixed_link(struct device_node *np) struct device_node *fixed_link_node; const __be32 *fixed_link_prop; int len; + struct phy_device *phy; /* New binding */ fixed_link_node = of_get_child_by_name(np, "fixed-link"); @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np) status.asym_pause = of_property_read_bool(fixed_link_node, "asym-pause"); of_node_put(fixed_link_node); - return fixed_phy_register(PHY_POLL, &status, np); + phy = fixed_phy_register(PHY_POLL, &status, np); + return (!phy || IS_ERR(phy)); } /* Old binding */ @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np) status.speed = be32_to_cpu(fixed_link_prop[2]); status.pause = be32_to_cpu(fixed_link_prop[3]); status.asym_pause = be32_to_cpu(fixed_link_prop[4]); - return fixed_phy_register(PHY_POLL, &status, np); + phy = fixed_phy_register(PHY_POLL, &status, np); + return (!phy || IS_ERR(phy)); } return -ENODEV; diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h index 9411386..f2ca1b4 100644 --- a/include/linux/phy_fixed.h +++ b/include/linux/phy_fixed.h @@ -14,9 +14,9 @@ struct device_node; #ifdef CONFIG_FIXED_PHY extern int fixed_phy_add(unsigned int irq, int phy_id, struct fixed_phy_status *status); -extern int fixed_phy_register(unsigned int irq, - struct fixed_phy_status *status, - struct device_node *np); +extern struct phy_device *fixed_phy_register(unsigned int irq, + struct fixed_phy_status *status, + struct device_node *np); extern void fixed_phy_del(int phy_addr); extern int fixed_phy_set_link_update(struct phy_device *phydev, int (*link_update)(struct net_device *, @@ -27,11 +27,11 @@ static inline int fixed_phy_add(unsigned int irq, int phy_id, { return -ENODEV; } -static inline int fixed_phy_register(unsigned int irq, - struct fixed_phy_status *status, - struct device_node *np) +static inline struct phy_device *fixed_phy_register(unsigned int irq, + struct fixed_phy_status *status, + struct device_node *np) { - return -ENODEV; + return ERR_PTR(-ENODEV); } static inline int fixed_phy_del(int phy_addr) { -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value 2014-10-01 21:45 [PATCH net-next] net: phy: adjust fixed_phy_register() return value Petri Gynther @ 2014-10-01 21:56 ` Florian Fainelli 2014-10-01 22:16 ` Petri Gynther 2014-10-04 12:07 ` Thomas Petazzoni 1 sibling, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2014-10-01 21:56 UTC (permalink / raw) To: Petri Gynther, netdev; +Cc: davem, thomas.petazzoni On 10/01/2014 02:45 PM, Petri Gynther wrote: > Adjust fixed_phy_register() to return struct phy_device *, so that > it becomes easy to use fixed PHYs without device tree support: > > phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL); > fixed_phy_set_link_update(phydev, fixed_phy_link_update); > phy_connect_direct(netdev, phydev, handler_fn, phy_interface); > > This change is a prerequisite for modifying bcmgenet driver to work > without a device tree on Broadcom's MIPS-based 7xxx platforms. Without a Device Tree enabled platform, you still have the fixed_phy_add() API to register a fixed PHY at a known location (the second parameter), did we someone broke that with Thomas' patches? You could for instance register a fixed PHY at address 1 for the GENET/MoCA instance to find it. Other than that, I am not opposed to the change, since it allows us to eventually get rid of fixed_phy_add() in the future. Thanks! > > Signed-off-by: Petri Gynther <pgynther@google.com> > --- > drivers/net/phy/fixed.c | 16 ++++++++-------- > drivers/of/of_mdio.c | 7 +++++-- > include/linux/phy_fixed.h | 14 +++++++------- > 3 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c > index 5b19fbb..47872caa 100644 > --- a/drivers/net/phy/fixed.c > +++ b/drivers/net/phy/fixed.c > @@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(fixed_phy_del); > static int phy_fixed_addr; > static DEFINE_SPINLOCK(phy_fixed_addr_lock); > > -int fixed_phy_register(unsigned int irq, > - struct fixed_phy_status *status, > - struct device_node *np) > +struct phy_device *fixed_phy_register(unsigned int irq, > + struct fixed_phy_status *status, > + struct device_node *np) > { > struct fixed_mdio_bus *fmb = &platform_fmb; > struct phy_device *phy; > @@ -246,19 +246,19 @@ int fixed_phy_register(unsigned int irq, > spin_lock(&phy_fixed_addr_lock); > if (phy_fixed_addr == PHY_MAX_ADDR) { > spin_unlock(&phy_fixed_addr_lock); > - return -ENOSPC; > + return ERR_PTR(-ENOSPC); > } > phy_addr = phy_fixed_addr++; > spin_unlock(&phy_fixed_addr_lock); > > ret = fixed_phy_add(PHY_POLL, phy_addr, status); > if (ret < 0) > - return ret; > + return ERR_PTR(ret); > > phy = get_phy_device(fmb->mii_bus, phy_addr, false); > if (!phy || IS_ERR(phy)) { > fixed_phy_del(phy_addr); > - return -EINVAL; > + return ERR_PTR(-EINVAL); > } > > of_node_get(np); > @@ -269,10 +269,10 @@ int fixed_phy_register(unsigned int irq, > phy_device_free(phy); > of_node_put(np); > fixed_phy_del(phy_addr); > - return ret; > + return ERR_PTR(ret); > } > > - return 0; > + return phy; > } > > static int __init fixed_mdio_bus_init(void) > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index a85d800..907baa9 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -286,6 +286,7 @@ int of_phy_register_fixed_link(struct device_node *np) > struct device_node *fixed_link_node; > const __be32 *fixed_link_prop; > int len; > + struct phy_device *phy; > > /* New binding */ > fixed_link_node = of_get_child_by_name(np, "fixed-link"); > @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np) > status.asym_pause = of_property_read_bool(fixed_link_node, > "asym-pause"); > of_node_put(fixed_link_node); > - return fixed_phy_register(PHY_POLL, &status, np); > + phy = fixed_phy_register(PHY_POLL, &status, np); > + return (!phy || IS_ERR(phy)); > } > > /* Old binding */ > @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np) > status.speed = be32_to_cpu(fixed_link_prop[2]); > status.pause = be32_to_cpu(fixed_link_prop[3]); > status.asym_pause = be32_to_cpu(fixed_link_prop[4]); > - return fixed_phy_register(PHY_POLL, &status, np); > + phy = fixed_phy_register(PHY_POLL, &status, np); > + return (!phy || IS_ERR(phy)); > } > > return -ENODEV; > diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h > index 9411386..f2ca1b4 100644 > --- a/include/linux/phy_fixed.h > +++ b/include/linux/phy_fixed.h > @@ -14,9 +14,9 @@ struct device_node; > #ifdef CONFIG_FIXED_PHY > extern int fixed_phy_add(unsigned int irq, int phy_id, > struct fixed_phy_status *status); > -extern int fixed_phy_register(unsigned int irq, > - struct fixed_phy_status *status, > - struct device_node *np); > +extern struct phy_device *fixed_phy_register(unsigned int irq, > + struct fixed_phy_status *status, > + struct device_node *np); > extern void fixed_phy_del(int phy_addr); > extern int fixed_phy_set_link_update(struct phy_device *phydev, > int (*link_update)(struct net_device *, > @@ -27,11 +27,11 @@ static inline int fixed_phy_add(unsigned int irq, int phy_id, > { > return -ENODEV; > } > -static inline int fixed_phy_register(unsigned int irq, > - struct fixed_phy_status *status, > - struct device_node *np) > +static inline struct phy_device *fixed_phy_register(unsigned int irq, > + struct fixed_phy_status *status, > + struct device_node *np) > { > - return -ENODEV; > + return ERR_PTR(-ENODEV); > } > static inline int fixed_phy_del(int phy_addr) > { > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value 2014-10-01 21:56 ` Florian Fainelli @ 2014-10-01 22:16 ` Petri Gynther 2014-10-02 0:33 ` Florian Fainelli 0 siblings, 1 reply; 7+ messages in thread From: Petri Gynther @ 2014-10-01 22:16 UTC (permalink / raw) To: Florian Fainelli; +Cc: netdev, David Miller, thomas.petazzoni Hi Florian, On Wed, Oct 1, 2014 at 2:56 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 10/01/2014 02:45 PM, Petri Gynther wrote: >> Adjust fixed_phy_register() to return struct phy_device *, so that >> it becomes easy to use fixed PHYs without device tree support: >> >> phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL); >> fixed_phy_set_link_update(phydev, fixed_phy_link_update); >> phy_connect_direct(netdev, phydev, handler_fn, phy_interface); >> >> This change is a prerequisite for modifying bcmgenet driver to work >> without a device tree on Broadcom's MIPS-based 7xxx platforms. > > Without a Device Tree enabled platform, you still have the > fixed_phy_add() API to register a fixed PHY at a known location (the > second parameter), did we someone broke that with Thomas' patches? > If a driver uses fixed_phy_add() directly, it then has to call get_phy_device() and phy_device_register() as well. But, all that is already handled nicely inside fixed_phy_register(). And, fixed_phy_register() also handles the fixed PHY address allocation automatically. In my opinion, this change makes it really easy to use fixed PHYs from non-OF code, as I showed in the change description. -- Petri > You could for instance register a fixed PHY at address 1 for the > GENET/MoCA instance to find it. > > Other than that, I am not opposed to the change, since it allows us to > eventually get rid of fixed_phy_add() in the future. > > Thanks! > >> >> Signed-off-by: Petri Gynther <pgynther@google.com> >> --- >> drivers/net/phy/fixed.c | 16 ++++++++-------- >> drivers/of/of_mdio.c | 7 +++++-- >> include/linux/phy_fixed.h | 14 +++++++------- >> 3 files changed, 20 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c >> index 5b19fbb..47872caa 100644 >> --- a/drivers/net/phy/fixed.c >> +++ b/drivers/net/phy/fixed.c >> @@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(fixed_phy_del); >> static int phy_fixed_addr; >> static DEFINE_SPINLOCK(phy_fixed_addr_lock); >> >> -int fixed_phy_register(unsigned int irq, >> - struct fixed_phy_status *status, >> - struct device_node *np) >> +struct phy_device *fixed_phy_register(unsigned int irq, >> + struct fixed_phy_status *status, >> + struct device_node *np) >> { >> struct fixed_mdio_bus *fmb = &platform_fmb; >> struct phy_device *phy; >> @@ -246,19 +246,19 @@ int fixed_phy_register(unsigned int irq, >> spin_lock(&phy_fixed_addr_lock); >> if (phy_fixed_addr == PHY_MAX_ADDR) { >> spin_unlock(&phy_fixed_addr_lock); >> - return -ENOSPC; >> + return ERR_PTR(-ENOSPC); >> } >> phy_addr = phy_fixed_addr++; >> spin_unlock(&phy_fixed_addr_lock); >> >> ret = fixed_phy_add(PHY_POLL, phy_addr, status); >> if (ret < 0) >> - return ret; >> + return ERR_PTR(ret); >> >> phy = get_phy_device(fmb->mii_bus, phy_addr, false); >> if (!phy || IS_ERR(phy)) { >> fixed_phy_del(phy_addr); >> - return -EINVAL; >> + return ERR_PTR(-EINVAL); >> } >> >> of_node_get(np); >> @@ -269,10 +269,10 @@ int fixed_phy_register(unsigned int irq, >> phy_device_free(phy); >> of_node_put(np); >> fixed_phy_del(phy_addr); >> - return ret; >> + return ERR_PTR(ret); >> } >> >> - return 0; >> + return phy; >> } >> >> static int __init fixed_mdio_bus_init(void) >> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >> index a85d800..907baa9 100644 >> --- a/drivers/of/of_mdio.c >> +++ b/drivers/of/of_mdio.c >> @@ -286,6 +286,7 @@ int of_phy_register_fixed_link(struct device_node *np) >> struct device_node *fixed_link_node; >> const __be32 *fixed_link_prop; >> int len; >> + struct phy_device *phy; >> >> /* New binding */ >> fixed_link_node = of_get_child_by_name(np, "fixed-link"); >> @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np) >> status.asym_pause = of_property_read_bool(fixed_link_node, >> "asym-pause"); >> of_node_put(fixed_link_node); >> - return fixed_phy_register(PHY_POLL, &status, np); >> + phy = fixed_phy_register(PHY_POLL, &status, np); >> + return (!phy || IS_ERR(phy)); >> } >> >> /* Old binding */ >> @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np) >> status.speed = be32_to_cpu(fixed_link_prop[2]); >> status.pause = be32_to_cpu(fixed_link_prop[3]); >> status.asym_pause = be32_to_cpu(fixed_link_prop[4]); >> - return fixed_phy_register(PHY_POLL, &status, np); >> + phy = fixed_phy_register(PHY_POLL, &status, np); >> + return (!phy || IS_ERR(phy)); >> } >> >> return -ENODEV; >> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h >> index 9411386..f2ca1b4 100644 >> --- a/include/linux/phy_fixed.h >> +++ b/include/linux/phy_fixed.h >> @@ -14,9 +14,9 @@ struct device_node; >> #ifdef CONFIG_FIXED_PHY >> extern int fixed_phy_add(unsigned int irq, int phy_id, >> struct fixed_phy_status *status); >> -extern int fixed_phy_register(unsigned int irq, >> - struct fixed_phy_status *status, >> - struct device_node *np); >> +extern struct phy_device *fixed_phy_register(unsigned int irq, >> + struct fixed_phy_status *status, >> + struct device_node *np); >> extern void fixed_phy_del(int phy_addr); >> extern int fixed_phy_set_link_update(struct phy_device *phydev, >> int (*link_update)(struct net_device *, >> @@ -27,11 +27,11 @@ static inline int fixed_phy_add(unsigned int irq, int phy_id, >> { >> return -ENODEV; >> } >> -static inline int fixed_phy_register(unsigned int irq, >> - struct fixed_phy_status *status, >> - struct device_node *np) >> +static inline struct phy_device *fixed_phy_register(unsigned int irq, >> + struct fixed_phy_status *status, >> + struct device_node *np) >> { >> - return -ENODEV; >> + return ERR_PTR(-ENODEV); >> } >> static inline int fixed_phy_del(int phy_addr) >> { >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value 2014-10-01 22:16 ` Petri Gynther @ 2014-10-02 0:33 ` Florian Fainelli 2014-10-03 19:47 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Florian Fainelli @ 2014-10-02 0:33 UTC (permalink / raw) To: Petri Gynther; +Cc: netdev, David Miller, thomas.petazzoni On 10/01/2014 03:16 PM, Petri Gynther wrote: > Hi Florian, > > On Wed, Oct 1, 2014 at 2:56 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 10/01/2014 02:45 PM, Petri Gynther wrote: >>> Adjust fixed_phy_register() to return struct phy_device *, so that >>> it becomes easy to use fixed PHYs without device tree support: >>> >>> phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL); >>> fixed_phy_set_link_update(phydev, fixed_phy_link_update); >>> phy_connect_direct(netdev, phydev, handler_fn, phy_interface); >>> >>> This change is a prerequisite for modifying bcmgenet driver to work >>> without a device tree on Broadcom's MIPS-based 7xxx platforms. >> >> Without a Device Tree enabled platform, you still have the >> fixed_phy_add() API to register a fixed PHY at a known location (the >> second parameter), did we someone broke that with Thomas' patches? >> > > If a driver uses fixed_phy_add() directly, it then has to call > get_phy_device() and phy_device_register() as well. But, all that is > already handled nicely inside fixed_phy_register(). And, > fixed_phy_register() also handles the fixed PHY address allocation > automatically. Sure, the usual model we had before Thomas' patches was to specifically format phy_id to make sure we would bind to the "fixed-0" MDIO bus instead of another one, see drivers/net/ethernet/ti/cpmac.c for instance. > > In my opinion, this change makes it really easy to use fixed PHYs from > non-OF code, as I showed in the change description. Agreed, and I think it will allow us to get rid of the manual allocation API eventually. I let Thomas comment on whether he likes this or not. > > -- Petri > >> You could for instance register a fixed PHY at address 1 for the >> GENET/MoCA instance to find it. >> >> Other than that, I am not opposed to the change, since it allows us to >> eventually get rid of fixed_phy_add() in the future. >> >> Thanks! >> >>> >>> Signed-off-by: Petri Gynther <pgynther@google.com> >>> --- >>> drivers/net/phy/fixed.c | 16 ++++++++-------- >>> drivers/of/of_mdio.c | 7 +++++-- >>> include/linux/phy_fixed.h | 14 +++++++------- >>> 3 files changed, 20 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c >>> index 5b19fbb..47872caa 100644 >>> --- a/drivers/net/phy/fixed.c >>> +++ b/drivers/net/phy/fixed.c >>> @@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(fixed_phy_del); >>> static int phy_fixed_addr; >>> static DEFINE_SPINLOCK(phy_fixed_addr_lock); >>> >>> -int fixed_phy_register(unsigned int irq, >>> - struct fixed_phy_status *status, >>> - struct device_node *np) >>> +struct phy_device *fixed_phy_register(unsigned int irq, >>> + struct fixed_phy_status *status, >>> + struct device_node *np) >>> { >>> struct fixed_mdio_bus *fmb = &platform_fmb; >>> struct phy_device *phy; >>> @@ -246,19 +246,19 @@ int fixed_phy_register(unsigned int irq, >>> spin_lock(&phy_fixed_addr_lock); >>> if (phy_fixed_addr == PHY_MAX_ADDR) { >>> spin_unlock(&phy_fixed_addr_lock); >>> - return -ENOSPC; >>> + return ERR_PTR(-ENOSPC); >>> } >>> phy_addr = phy_fixed_addr++; >>> spin_unlock(&phy_fixed_addr_lock); >>> >>> ret = fixed_phy_add(PHY_POLL, phy_addr, status); >>> if (ret < 0) >>> - return ret; >>> + return ERR_PTR(ret); >>> >>> phy = get_phy_device(fmb->mii_bus, phy_addr, false); >>> if (!phy || IS_ERR(phy)) { >>> fixed_phy_del(phy_addr); >>> - return -EINVAL; >>> + return ERR_PTR(-EINVAL); >>> } >>> >>> of_node_get(np); >>> @@ -269,10 +269,10 @@ int fixed_phy_register(unsigned int irq, >>> phy_device_free(phy); >>> of_node_put(np); >>> fixed_phy_del(phy_addr); >>> - return ret; >>> + return ERR_PTR(ret); >>> } >>> >>> - return 0; >>> + return phy; >>> } >>> >>> static int __init fixed_mdio_bus_init(void) >>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c >>> index a85d800..907baa9 100644 >>> --- a/drivers/of/of_mdio.c >>> +++ b/drivers/of/of_mdio.c >>> @@ -286,6 +286,7 @@ int of_phy_register_fixed_link(struct device_node *np) >>> struct device_node *fixed_link_node; >>> const __be32 *fixed_link_prop; >>> int len; >>> + struct phy_device *phy; >>> >>> /* New binding */ >>> fixed_link_node = of_get_child_by_name(np, "fixed-link"); >>> @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np) >>> status.asym_pause = of_property_read_bool(fixed_link_node, >>> "asym-pause"); >>> of_node_put(fixed_link_node); >>> - return fixed_phy_register(PHY_POLL, &status, np); >>> + phy = fixed_phy_register(PHY_POLL, &status, np); >>> + return (!phy || IS_ERR(phy)); >>> } >>> >>> /* Old binding */ >>> @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np) >>> status.speed = be32_to_cpu(fixed_link_prop[2]); >>> status.pause = be32_to_cpu(fixed_link_prop[3]); >>> status.asym_pause = be32_to_cpu(fixed_link_prop[4]); >>> - return fixed_phy_register(PHY_POLL, &status, np); >>> + phy = fixed_phy_register(PHY_POLL, &status, np); >>> + return (!phy || IS_ERR(phy)); >>> } >>> >>> return -ENODEV; >>> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h >>> index 9411386..f2ca1b4 100644 >>> --- a/include/linux/phy_fixed.h >>> +++ b/include/linux/phy_fixed.h >>> @@ -14,9 +14,9 @@ struct device_node; >>> #ifdef CONFIG_FIXED_PHY >>> extern int fixed_phy_add(unsigned int irq, int phy_id, >>> struct fixed_phy_status *status); >>> -extern int fixed_phy_register(unsigned int irq, >>> - struct fixed_phy_status *status, >>> - struct device_node *np); >>> +extern struct phy_device *fixed_phy_register(unsigned int irq, >>> + struct fixed_phy_status *status, >>> + struct device_node *np); >>> extern void fixed_phy_del(int phy_addr); >>> extern int fixed_phy_set_link_update(struct phy_device *phydev, >>> int (*link_update)(struct net_device *, >>> @@ -27,11 +27,11 @@ static inline int fixed_phy_add(unsigned int irq, int phy_id, >>> { >>> return -ENODEV; >>> } >>> -static inline int fixed_phy_register(unsigned int irq, >>> - struct fixed_phy_status *status, >>> - struct device_node *np) >>> +static inline struct phy_device *fixed_phy_register(unsigned int irq, >>> + struct fixed_phy_status *status, >>> + struct device_node *np) >>> { >>> - return -ENODEV; >>> + return ERR_PTR(-ENODEV); >>> } >>> static inline int fixed_phy_del(int phy_addr) >>> { >>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value 2014-10-02 0:33 ` Florian Fainelli @ 2014-10-03 19:47 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2014-10-03 19:47 UTC (permalink / raw) To: f.fainelli; +Cc: pgynther, netdev, thomas.petazzoni From: Florian Fainelli <f.fainelli@gmail.com> Date: Wed, 01 Oct 2014 17:33:20 -0700 > I let Thomas comment on whether he likes this or not. I'm still waiting to see Thomas's feedback. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value 2014-10-01 21:45 [PATCH net-next] net: phy: adjust fixed_phy_register() return value Petri Gynther 2014-10-01 21:56 ` Florian Fainelli @ 2014-10-04 12:07 ` Thomas Petazzoni 2014-10-05 0:02 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: Thomas Petazzoni @ 2014-10-04 12:07 UTC (permalink / raw) To: Petri Gynther; +Cc: netdev, davem, f.fainelli Dear Petri Gynther, Sorry for the late answer. On Wed, 1 Oct 2014 14:45:09 -0700 (PDT), Petri Gynther wrote: > Adjust fixed_phy_register() to return struct phy_device *, so that > it becomes easy to use fixed PHYs without device tree support: > > phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL); > fixed_phy_set_link_update(phydev, fixed_phy_link_update); > phy_connect_direct(netdev, phydev, handler_fn, phy_interface); > > This change is a prerequisite for modifying bcmgenet driver to work > without a device tree on Broadcom's MIPS-based 7xxx platforms. > > Signed-off-by: Petri Gynther <pgynther@google.com> On the principle, I'm obviously fine, but I have one comment below. > /* New binding */ > fixed_link_node = of_get_child_by_name(np, "fixed-link"); > @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np) > status.asym_pause = of_property_read_bool(fixed_link_node, > "asym-pause"); > of_node_put(fixed_link_node); > - return fixed_phy_register(PHY_POLL, &status, np); > + phy = fixed_phy_register(PHY_POLL, &status, np); > + return (!phy || IS_ERR(phy)); > } > > /* Old binding */ > @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np) > status.speed = be32_to_cpu(fixed_link_prop[2]); > status.pause = be32_to_cpu(fixed_link_prop[3]); > status.asym_pause = be32_to_cpu(fixed_link_prop[4]); > - return fixed_phy_register(PHY_POLL, &status, np); > + phy = fixed_phy_register(PHY_POLL, &status, np); > + return (!phy || IS_ERR(phy)); I am not sure this return (!phy || IS_ERR(phy)) is doing the right thing. This function is supposed to return an error code on failure, or 0 on success. I don't see how your error handling returns an error code on failure. What about doing the more explicit: phy = fixed_phy_register(PHY_POLL, &status, np); if (IS_ERR(phy)) return PTR_ERR(phy); else return 0; Or am I missing something? Thanks, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value 2014-10-04 12:07 ` Thomas Petazzoni @ 2014-10-05 0:02 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2014-10-05 0:02 UTC (permalink / raw) To: thomas.petazzoni; +Cc: pgynther, netdev, f.fainelli From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Date: Sat, 4 Oct 2014 14:07:31 +0200 > I am not sure this return (!phy || IS_ERR(phy)) is doing the right > thing. This function is supposed to return an error code on failure, or > 0 on success. I don't see how your error handling returns an error code > on failure. What about doing the more explicit: > > phy = fixed_phy_register(PHY_POLL, &status, np); > if (IS_ERR(phy)) > return PTR_ERR(phy); > else > return 0; > > Or am I missing something? Agreed, there is no circumstance under which the new fixed_phy_register() should return a NULL pointer. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-10-05 0:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-01 21:45 [PATCH net-next] net: phy: adjust fixed_phy_register() return value Petri Gynther 2014-10-01 21:56 ` Florian Fainelli 2014-10-01 22:16 ` Petri Gynther 2014-10-02 0:33 ` Florian Fainelli 2014-10-03 19:47 ` David Miller 2014-10-04 12:07 ` Thomas Petazzoni 2014-10-05 0:02 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).