Patchwork [U-Boot,1/2] phylib: reset mii bus only if reset handler is registered

login
register
mail settings
Submitter Vladimir Zapolskiy
Date Sept. 5, 2011, 5:24 p.m.
Message ID <1315243448-29959-2-git-send-email-vz@mleia.com>
Download mbox | patch
Permalink /patch/113413/
State Accepted
Commit e3a77218a256edbe201112a39beeed8adcabae3f
Headers show

Comments

Vladimir Zapolskiy - Sept. 5, 2011, 5:24 p.m.
This change allows to cope with a mii bus device registered using
miiphy_register(), which doesn't assign a default reset handler.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/net/phy/phy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Detlev Zundel - Sept. 9, 2011, 1:27 p.m.
Hi Vladimir,

> This change allows to cope with a mii bus device registered using
> miiphy_register(), which doesn't assign a default reset handler.

Looks good, so

Acked-by: Detlev Zundel <dzu@denx.de>

> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>

Cheers
  Detlev
Wolfgang Denk - Sept. 9, 2011, 10:08 p.m.
Dear Vladimir Zapolskiy,

In message <1315243448-29959-2-git-send-email-vz@mleia.com> you wrote:
> This change allows to cope with a mii bus device registered using
> miiphy_register(), which doesn't assign a default reset handler.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/net/phy/phy.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk
Andy Fleming - Sept. 22, 2011, 9:44 p.m.
On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> This change allows to cope with a mii bus device registered using
> miiphy_register(), which doesn't assign a default reset handler.
>
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/net/phy/phy.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index ce69c19..8da7688 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -692,7 +692,8 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
>        struct phy_device *phydev;
>
>        /* Reset the bus */
> -       bus->reset(bus);
> +       if (bus->reset)
> +               bus->reset(bus);


The change is a good idea, but I find the motivation for it strange.
If you register a bus with miiphy_register, you are declaring your
intent to use the legacy PHY interface. But phy_connect() is part of
the new phylib API. It was not intended that combining the two work at
all. Looking at the code, I see no reason it wouldn't work, but I
question why you would do that, instead of creating a proper MDIO
driver?

Andy
Mike Frysinger - Sept. 22, 2011, 10:13 p.m.
On Thursday, September 22, 2011 17:44:11 Andy Fleming wrote:
> On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> > This change allows to cope with a mii bus device registered using
> > miiphy_register(), which doesn't assign a default reset handler.
> > 
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -692,7 +692,8 @@ struct phy_device *phy_connect(struct mii_dev *bus,
> > int addr, struct phy_device *phydev;
> > 
> >        /* Reset the bus */
> > -       bus->reset(bus);
> > +       if (bus->reset)
> > +               bus->reset(bus);
> 
> The change is a good idea, but I find the motivation for it strange.
> If you register a bus with miiphy_register, you are declaring your
> intent to use the legacy PHY interface. But phy_connect() is part of
> the new phylib API. It was not intended that combining the two work at
> all. Looking at the code, I see no reason it wouldn't work, but I
> question why you would do that, instead of creating a proper MDIO
> driver?

hmm, is there an #ifdef check we could add that would cause an #error if 
people mix the old and the new ?  i think it makes sense to just force 
everyone to migrate to the new ...
-mike
Andy Fleming - Sept. 22, 2011, 10:19 p.m.
On Thu, Sep 22, 2011 at 5:13 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday, September 22, 2011 17:44:11 Andy Fleming wrote:
>> On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
>> >        /* Reset the bus */
>> > -       bus->reset(bus);
>> > +       if (bus->reset)
>> > +               bus->reset(bus);
>>
>> The change is a good idea, but I find the motivation for it strange.
>> If you register a bus with miiphy_register, you are declaring your
>> intent to use the legacy PHY interface. But phy_connect() is part of
>> the new phylib API. It was not intended that combining the two work at
>> all. Looking at the code, I see no reason it wouldn't work, but I
>> question why you would do that, instead of creating a proper MDIO
>> driver?
>
> hmm, is there an #ifdef check we could add that would cause an #error if
> people mix the old and the new ?  i think it makes sense to just force
> everyone to migrate to the new ...


I suppose we could add a check in phy_connect to see whether the read
function is actually legacy_miiphy_read. If you don't call
phy_connect(), then you don't get a handle to the PHY device, and
aren't using phylib.

I don't think we're ready to disallow using the two concurrently in
separate drivers.

Andy

Patch

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ce69c19..8da7688 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -692,7 +692,8 @@  struct phy_device *phy_connect(struct mii_dev *bus, int addr,
 	struct phy_device *phydev;
 
 	/* Reset the bus */
-	bus->reset(bus);
+	if (bus->reset)
+		bus->reset(bus);
 
 	/* Wait 15ms to make sure the PHY has come out of hard reset */
 	udelay(15000);