Re: [PATCH net v2] net: phy: marvell: add Marvell specific PHY loopback

On Wed, Jan 12, 2022 at 05:33:44PM +0800, Mohammad Athari Bin Ismail wrote:

+static int marvell_loopback(struct phy_device *phydev, bool enable) +{

• if (enable) {

• u16 bmcr_ctl = 0, mscr2_ctl = 0;
  

• if (phydev->speed == SPEED_1000)
  

• 	bmcr_ctl = BMCR_SPEED1000;
  

• else if (phydev->speed == SPEED_100)
  

• 	bmcr_ctl = BMCR_SPEED100;
  

• if (phydev->duplex == DUPLEX_FULL)
  

• 	bmcr_ctl |= BMCR_FULLDPLX;
  

• phy_modify(phydev, MII_BMCR, ~0, bmcr_ctl);
  

Is there any point in doing a read-modify-write here if you're just setting all bits in the register? Wouldn't phy_write() be more appropriate? What about error handing?

• if (phydev->speed == SPEED_1000)
  

• 	mscr2_ctl = BMCR_SPEED1000;
  

• else if (phydev->speed == SPEED_100)
  

• 	mscr2_ctl = BMCR_SPEED100;
  

• phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
  

• 		 MII_88E1510_MSCR_2, BMCR_SPEED1000 |
  

• 		 BMCR_SPEED100, mscr2_ctl);
  

• /* Need soft reset to have speed configuration takes effect */
  

• genphy_soft_reset(phydev);
  

• /* FIXME: Based on trial and error test, it seem 1G need to have
  

•  * delay between soft reset and loopback enablement.
  

•  */
  

• if (phydev->speed == SPEED_1000)
  

• 	msleep(1000);
  

• return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
  

• 		  BMCR_LOOPBACK);
  

• } else {

• phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
  

Error handling?

Thanks.