Patchwork [U-Boot,2/2] phylib: remove a couple of redundant code lines

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

Comments

Vladimir Zapolskiy - Sept. 5, 2011, 5:24 p.m.
This change slightly improves readability of the phydev speed/duplex
assignment logic.

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

> This change slightly improves readability of the phydev speed/duplex
> assignment logic.

Agreed.

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

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

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

In message <1315243448-29959-3-git-send-email-vz@mleia.com> you wrote:
> This change slightly improves readability of the phydev speed/duplex
> assignment logic.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/net/phy/phy.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk
Andy Fleming - Sept. 22, 2011, 9:25 p.m.
On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> This change slightly improves readability of the phydev speed/duplex
> assignment logic.


Shoot, I just saw this patch in my tree. It's incorrect.


> @@ -318,13 +318,10 @@ static int genphy_parse_link(struct phy_device *phydev)
>                lpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE);
>                lpa &= phy_read(phydev, MDIO_DEVAD_NONE, MII_LPA);
>
> -               if (lpa & (LPA_100FULL | LPA_100HALF)) {
> +               if (lpa & (LPA_100FULL | LPA_100HALF))
>                        phydev->speed = SPEED_100;
>
> -                       if (lpa & LPA_100FULL)
> -                               phydev->duplex = DUPLEX_FULL;
> -
> -               } else if (lpa & LPA_10FULL)
> +               if (lpa & (LPA_100FULL | LPA_10FULL))
>                        phydev->duplex = DUPLEX_FULL;


The lines weren't redundant. The logic is (and probably should be
better commented):

Find the intersection of the advertised capabilities of both sides of
the link (lpa)
From that intersection, find the highest capability we can run at
(that will be the negotiated link)

Now imagine that the intersection (lpa) is (LPA_100HALF | LPA_10FULL).

The code will now set phydev->speed to 100, and phydev->duplex to 1,
but this link does not support 100FULL.

Andy
Wolfgang Denk - Sept. 23, 2011, 6:06 a.m.
Dear Andy Fleming,

In message <CAKWjMd5HGT9df76vPFs8B5sFQYWoAN1bGmt2vRihN0cTa1boug@mail.gmail.com> you wrote:
>
> Shoot, I just saw this patch in my tree. It's incorrect.

Argh...

> The lines weren't redundant. The logic is (and probably should be
> better commented):
> 
> Find the intersection of the advertised capabilities of both sides of
> the link (lpa)
> From that intersection, find the highest capability we can run at
> (that will be the negotiated link)
> 
> Now imagine that the intersection (lpa) is (LPA_100HALF | LPA_10FULL).
> 
> The code will now set phydev->speed to 100, and phydev->duplex to 1,
> but this link does not support 100FULL.

Do we agree that I should revert this commit?

Best regards,

Wolfgang Denk
Vladimir Zapolskiy - Sept. 26, 2011, 7:24 p.m.
Hello Wolfgang,

On 23.09.2011 09:06, Wolfgang Denk wrote:
> Dear Andy Fleming,
>
> In message<CAKWjMd5HGT9df76vPFs8B5sFQYWoAN1bGmt2vRihN0cTa1boug@mail.gmail.com>  you wrote:
>>
>> Shoot, I just saw this patch in my tree. It's incorrect.
>
> Argh...
>
>> The lines weren't redundant. The logic is (and probably should be
>> better commented):
>>
>> Find the intersection of the advertised capabilities of both sides of
>> the link (lpa)
>>  From that intersection, find the highest capability we can run at
>> (that will be the negotiated link)
>>
>> Now imagine that the intersection (lpa) is (LPA_100HALF | LPA_10FULL).
>>
>> The code will now set phydev->speed to 100, and phydev->duplex to 1,
>> but this link does not support 100FULL.
>
> Do we agree that I should revert this commit?
>
due to Andy's explanation I agree that this change is fishy and it 
should be reverted.

As a U-Boot user I'd greatly appreciate, if you add the above note to 
the commit message.

Sorry for inconvenience.
Wolfgang Denk - Sept. 28, 2011, 7:05 p.m.
Dear Vladimir Zapolskiy,

In message <1315243448-29959-3-git-send-email-vz@mleia.com> you wrote:
> This change slightly improves readability of the phydev speed/duplex
> assignment logic.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/net/phy/phy.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)

Patch reverted.

Best regards,

Wolfgang Denk

Patch

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8da7688..833a051 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -318,13 +318,10 @@  static int genphy_parse_link(struct phy_device *phydev)
 		lpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE);
 		lpa &= phy_read(phydev, MDIO_DEVAD_NONE, MII_LPA);
 
-		if (lpa & (LPA_100FULL | LPA_100HALF)) {
+		if (lpa & (LPA_100FULL | LPA_100HALF))
 			phydev->speed = SPEED_100;
 
-			if (lpa & LPA_100FULL)
-				phydev->duplex = DUPLEX_FULL;
-
-		} else if (lpa & LPA_10FULL)
+		if (lpa & (LPA_100FULL | LPA_10FULL))
 			phydev->duplex = DUPLEX_FULL;
 	} else {
 		u32 bmcr = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);