Hi Joe, > -----Original Message----- > From: Joe Hershberger [mailto:joe.hershber...@gmail.com] > Sent: Thursday, December 08, 2016 8:05 PM > To: Alexey Brodkin <alexey.brod...@synopsys.com> > Cc: john.haech...@microsemi.com; u-boot@lists.denx.de; joe.hershber...@ni.com > Subject: Re: [U-Boot] [PATCH 1/1] Adding MSCC > PHY-VSC8530-VSC8531-VSC8540-VSC8541 > > 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.brod...@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.
Well especially here IMHO there's no need to separate 2 temporary vars. Still it's more a matter of your taste/preference. Moreover I'm not really asking to do that change but wondering about reasons and with good explanation I'm fine with it as it is. > >> + > >> + /* 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. Again just an example of how I would do that. But I may agree that it may look a bit ugly :) Anyways thanks for your input. -Alexey _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot