Re: [PATCH] lan78xx: relocate mdix setting to phy driver

[Florian Fainelli]

On 11/15/2016 01:57 PM, woojung....@microchip.com wrote:
> From: Woojung Huh <woojung....@microchip.com>
> 
> Relocate mdix code to phy driver to be called at config_init().
> 
> Signed-off-by: Woojung Huh <woojung....@microchip.com>
> ---
>  drivers/net/phy/microchip.c | 43 +++++++++++++++++++++++++-
>  drivers/net/usb/lan78xx.c   | 73 
> ++-------------------------------------------
>  2 files changed, 45 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
> index 7c00e50..13553df 100644
> --- a/drivers/net/phy/microchip.c
> +++ b/drivers/net/phy/microchip.c
> @@ -106,6 +106,47 @@ static int lan88xx_set_wol(struct phy_device *phydev,
>       return 0;
>  }
>  
> +static void lan88xx_set_mdix(struct phy_device *phydev)
> +{
> +     int buf;
> +
> +     if (phydev->mdix == ETH_TP_MDI) {
> +             phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
> +                       LAN88XX_EXT_PAGE_SPACE_1);

Should you take this write out of the if/else

> +             buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);

And this one too

> +             buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
> +             phy_write(phydev, LAN88XX_EXT_MODE_CTRL,
> +                       buf | LAN88XX_EXT_MODE_CTRL_MDI_);
> +             phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
> +                       LAN88XX_EXT_PAGE_SPACE_0);

And this one too as well

> +     } else if (phydev->mdix == ETH_TP_MDI_X) {

switch/case statement would be more appropriate and it would be easier
to add support for future possible modes.

And once you take the write/read/write out of the switch case, it's just
a matter of translating phydev->mdix into the appropriate mask value to
write.

So essentially, this comes down to:

static void lan88xx_set_mdix(struct phy_device *phydev)
{
        int buf;
        int mask_val;

        switch (phydev->mdix) {
        case ETH_TP_MDI:
                mask_val = LAN88XX_EXT_MODE_CTRL_MDI_;
                break;
        case ETH_TP_MDI_X:
                mask_val = LAN88XX_EXT_MODE_CTRL_MDI_X_;
                break;
        case ETH_TP_MDI_AUTO:
                mask_val = LAN88XX_EXT_MODE_CTRL_AUTO_MDIX_:
                break:
        default:
                return;
        }

        phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
                  LAN88XX_EXT_PAGE_SPACE_1);
        buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);
        buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
        buf |= mask_val;
        phy_write(phydev, LAN88XX_EXT_MODE_CTRL, buf);
        phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
                  LAN88XX_EXT_PAGE_SPACE_0);
}

I leave it up to you whether you need to do this now, or as a subsequent
clean up patch.
-- 
Florian