Re: [PATCH net-next 5/5] net: phy: add support to get Master-Slave configuration

[Raju Lakkaraju <Raju.Lakkaraju@microchip.com>]

Hi Andrew,

Thank you for review comments.

The 06/14/2022 23:21, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jun 14, 2022 at 04:04:24PM +0530, Raju Lakkaraju wrote:
> > Implement reporting the Master-Slave configuration and state
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> > ---
> >  drivers/net/phy/mxl-gpy.c | 55 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> > index 5ce1bf03bbd7..cf625ced4ec1 100644
> > --- a/drivers/net/phy/mxl-gpy.c
> > +++ b/drivers/net/phy/mxl-gpy.c
> > @@ -27,11 +27,19 @@
> >  #define PHY_ID_GPY241BM              0x67C9DE80
> >  #define PHY_ID_GPY245B               0x67C9DEC0
> >
> > +#define PHY_STD_GCTRL                0x09    /* Gbit ctrl */
> > +#define PHY_STD_GSTAT                0x0A    /* Gbit status */
> 
> #define MII_CTRL1000            0x09    /* 1000BASE-T control          */
> #define MII_STAT1000            0x0a    /* 1000BASE-T status           */
> 
> from mii.h
> 

I can use generic master-slave function.
I will remove these changes

> >  #define PHY_MIISTAT          0x18    /* MII state */
> >  #define PHY_IMASK            0x19    /* interrupt mask */
> >  #define PHY_ISTAT            0x1A    /* interrupt status */
> >  #define PHY_FWV                      0x1E    /* firmware version */
> >
> > +#define PHY_STD_GCTRL_MS     BIT(11)
> > +#define PHY_STD_GCTRL_MSEN   BIT(12)
> > +
> > +#define PHY_STD_GSTAT_MSRES  BIT(14)
> > +#define PHY_STD_GSTAT_MSFAULT        BIT(15)
> 
> If the device is just following the standard, there should not be any
> need to add defines, they should already exist. And if it does follow
> the standard there are probably helpers you can use.
> 

Accepted. 
Device following the standard. I will use generic master-slave function

> >  #define PHY_MIISTAT_SPD_MASK GENMASK(2, 0)
> >  #define PHY_MIISTAT_DPX              BIT(3)
> >  #define PHY_MIISTAT_LS               BIT(10)
> > @@ -160,6 +168,48 @@ static bool gpy_2500basex_chk(struct phy_device *phydev)
> >       return true;
> >  }
> >
> > +static int gpy_master_slave_cfg_get(struct phy_device *phydev)
> > +{
> > +     int state;
> > +     int cfg;
> > +     int ret;
> > +
> > +     ret = phy_read(phydev, PHY_STD_GCTRL);
> > +     if (ret < 0) {
> > +             phydev_err(phydev, "Error: MDIO register access failed: %d\n",
> > +                        ret);
> > +             return ret;
> > +     }
> > +
> > +     if (ret & PHY_STD_GCTRL_MSEN)
> > +             if (ret & PHY_STD_GCTRL_MS)
> > +                     cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
> > +             else
> > +                     cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
> > +     else
> > +             cfg = MASTER_SLAVE_CFG_MASTER_PREFERRED;
> > +
> > +     ret = phy_read(phydev, PHY_STD_GSTAT);
> > +     if (ret < 0) {
> > +             phydev_err(phydev, "Error: MDIO register access failed: %d\n",
> > +                        ret);
> > +             return ret;
> > +     }
> > +
> > +     if (ret & PHY_STD_GSTAT_MSFAULT)
> > +             state = MASTER_SLAVE_STATE_ERR;
> > +     else
> > +             if (ret & PHY_STD_GSTAT_MSRES)
> > +                     state = MASTER_SLAVE_STATE_MASTER;
> > +             else
> > +                     state = MASTER_SLAVE_STATE_SLAVE;
> > +
> > +     phydev->master_slave_get = cfg;
> > +     phydev->master_slave_state = state;
> > +
> > +     return 0;
> 
> Would genphy_read_master_slave() work?

Yes. 
I can use genphy_read_master_slave() function.

> 
>       Andrew

-- 

Thanks,
Raju