Hi Alexey,
Thanks for the review. I agree with most of your comments, but I'll note where I don't.
On Thu, Dec 8, 2016 at 6:24 AM, Alexey Brodkin Alexey.Brodkin@synopsys.com wrote:
...
+static int mscc_vsc8531_vsc8541_init_scripts(struct phy_device *phydev) +{
u16 reg_val17;u16 reg_val18;Any reason to keep variables in different lines?
I think separate lines is cleaner and prefer it stay this way.
/* Set to Access Token Ring Registers */phy_write(phydev, MDIO_DEVAD_NONE,MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_TR);/* Update LinkDetectCtrl default to optimized values *//* Determined during Silicon Validation Testing */phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xA7F8);Cryptic values are not very nice to deal with. I'd advise you to use defines here. Moreover I'm wondering: a) why all this complicated setup is required? b) is it mentioned in any datasheet? If it is mentioned it worth adding a comment about source of those figures and explanation of the procedure itself. This will help mere mortals to figure out what is done here and if required people will use that knowledge as a reference in other projects.
reg_val17 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17);reg_val17 = bitfield_replace(reg_val17, MSCC_PHY_TR_LINKDETCTRL_POS,MSCC_PHY_TR_LINKDETCTRL_WIDTH, 3);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17, reg_val17);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x87F8);/* Update VgaThresh100 defaults to optimized values *//* Determined during Silicon Validation Testing */phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xAFA4);reg_val18 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18);reg_val18 = bitfield_replace(reg_val18, MSCC_PHY_TR_VGATHRESH100_POS,MSCC_PHY_TR_VGATHRESH100_WIDTH, 24);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18, reg_val18);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x8FA4);/* Update VgaGain10 defaults to optimized values *//* Determined during Silicon Validation Testing */phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0xAF92);reg_val18 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18);reg_val17 = phy_read(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17);reg_val18 = bitfield_replace(reg_val18, MSCC_PHY_TR_VGAGAIN10_U_POS,MSCC_PHY_TR_VGAGAIN10_U_WIDTH, 0);reg_val17 = bitfield_replace(reg_val17, MSCC_PHY_TR_VGAGAIN10_L_POS,MSCC_PHY_TR_VGAGAIN10_L_WIDTH, 1);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_18, reg_val18);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_DATA_17, reg_val17);phy_write(phydev, MDIO_DEVAD_NONE, MSCC_PHY_REG_TR_ADDR_16, 0x8F92);/* Set back to Access Standard Page Registers */phy_write(phydev, MDIO_DEVAD_NONE, MSCC_EXT_PAGE_ACCESS,MSCC_PHY_PAGE_STD);return 0;+}
+static int mscc_parse_status(struct phy_device *phydev) +{
u16 speed;u16 mii_reg;mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_AUX_CNTRL_STAT_REG);u16 speed, mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_AUX_CNTRL_STAT_REG);
This is especially ugly. It looks more like a python function returning a tuple than clean C code. Please do not do this.
if (mii_reg & MIIM_AUX_CNTRL_STAT_F_DUPLEX)phydev->duplex = DUPLEX_FULL;elsephydev->duplex = DUPLEX_HALF;speed = mii_reg & MIIM_AUX_CNTRL_STAT_SPEED_MASK;speed = speed >> MIIM_AUX_CNTRL_STAT_SPEED_POS;switch (speed) {case MIIM_AUX_CNTRL_STAT_SPEED_1000M:phydev->speed = SPEED_1000;break;case MIIM_AUX_CNTRL_STAT_SPEED_100M:phydev->speed = SPEED_100;break;case MIIM_AUX_CNTRL_STAT_SPEED_10M:phydev->speed = SPEED_10;break;default:phydev->speed = SPEED_10;break;}return 0;+}
+static int mscc_startup(struct phy_device *phydev) +{
int ret;ret = genphy_update_link(phydev);Merge two lines above together?
I could go either way on this one. I certainly wouldn't ask for this change, but I'm ok with it.
if (ret)return ret;