[U-Boot] [PATCH 1/1] Adding MSCC PHY-VSC8530-VSC8531-VSC8540-VSC8541

[Alexey Brodkin]

Hi Joe,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> Sent: Thursday, December 08, 2016 8:05 PM
> To: Alexey Brodkin <Alexey.Brodkin at synopsys.com>
> Cc: john.haechten at microsemi.com; u-boot at lists.denx.de; joe.hershberger at 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.Brodkin at 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