Hi Andrew, Thank you for review the code and valuable comments.
On Thu, Sep 08, 2016 at 03:14:15PM +0200, Andrew Lunn wrote: > EXTERNAL EMAIL > > > On Thu, Sep 08, 2016 at 02:47:21PM +0530, Raju Lakkaraju wrote: > > From: Raju Lakkaraju <raju.lakkar...@microsemi.com> > > > > Used Device Tree to configure the Edge-rate as per review comments and > > re-sending code for review > > > > Signed-off-by: Raju Lakkaraju <raju.lakkar...@microsemi.com> > > --- > > drivers/net/phy/mscc.c | 76 > > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > Hi Raju > > You need to also document the new property in the device tree binding > documentation. > Sure. I will do. I created device tree binding header file. i will submit in different patch. > > +static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, > > + u8 edge_rate) > > No spaces place. > I ran the checkpatch. I did not find any error. I created another workspace and applied the same patch. It shows the correct alignement. I have used tabs (8 space width). then some spaces to align braces. > > +#ifdef CONFIG_OF_MDIO > > +static int vsc8531_of_init(struct phy_device *phydev) > > +{ > > + int rc; > > + struct vsc8531_private *vsc8531 = phydev->priv; > > + struct device *dev = &phydev->mdio.dev; > > + struct device_node *of_node = dev->of_node; > > + > > + if (!of_node) > > + return -ENODEV; > > + > > + rc = of_property_read_u8(of_node, "vsc8531,edge-rate", > > + &vsc8531->edge_rate); > > Until you have written the Documentation, it is hard for me to tell, > but device tree bindings should use real units, like seconds, Ohms, > Farads, etc. Is the edge rate in nS? Or is it some magic value which > just gets written into the register? > This is some magic value which just gets written into the register. In device tree file, defined in davinci_mdio structure: vsc8531_0: ethernet-phy@0 { compatible = "ethernet-phy-id0007.0570"; reg = <0>; vsc8531,edge-rate = /bits/ 8 <MSCC_EDGE_RATE_CNTL_FASTEST>; }; In device tree binding header file, MACRO has defined as i.e. include/dt-bindings/net/mscc-vsc8531.h /* MAC interface Edge rate control pad */ #define MSCC_EDGE_RATE_CNTL_SLOWEST 0x0 #define MSCC_EDGE_RATE_CNTL_PLUS_1 0x1 #define MSCC_EDGE_RATE_CNTL_PLUS_2 0x2 #define MSCC_EDGE_RATE_CNTL_PLUS_3 0x3 #define MSCC_EDGE_RATE_CNTL_PLUS_4 0x4 #define MSCC_EDGE_RATE_CNTL_PLUS_5 0x5 #define MSCC_EDGE_RATE_CNTL_PLUS_6 0x6 #define MSCC_EDGE_RATE_CNTL_FASTEST 0x7 > > + > > + return rc; > > +} > > +#else > > +static int vsc8531_of_init(struct phy_device *phydev) > > +{ > > + return 0; > > +} > > +#endif /* CONFIG_OF_MDIO */ > > + > > static int vsc85xx_config_init(struct phy_device *phydev) > > { > > int rc; > > + struct vsc8531_private *vsc8531; > > + > > + if (!phydev->priv) { > > How can this happen? > VSC 8531 driver don't have any private structure assigned initially. Allways priv points to NULL. Allocate vsc8531 private structure and initialize by calling vsc8531_of_init( ) function. > > + vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), > > + GFP_KERNEL); > > + if (!vsc8531) > > + return -ENOMEM; > > + > > + phydev->priv = vsc8531; > > + rc = vsc8531_of_init(phydev); > > + if (rc) > > + return rc; > > + } else { > > + vsc8531 = (struct vsc8531_private *)phydev->priv; > > + } > > > > rc = vsc85xx_default_config(phydev); > > if (rc) > > return rc; > > + > > + rc = vsc85xx_edge_rate_cntl_set(phydev, vsc8531->edge_rate); > > If there is no vsc8531,edge-rate property in device tree, is the phy > going to work O.K, if you configure it for 0nS edges? Or should there > be some default value assigned? > Yes. Default values configured as Fast Edge rate control (i.e.0b111). Edge rate control has defined 3 bits (Bit 7:5) in register. Hardware default value is 3 (i.e. 0b111) > Thanks > Andrew Thanks Raju.