* [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).