Re: [PATCH v2 net-next 1/2] net: phy: Add Edge-rate driver for Microsemi PHYs.

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:

quoted

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the Edge-rate as per review comments and
re-sending code for review

Signed-off-by: Raju Lakkaraju <redacted>
---
 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.

quoted

+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.

quoted

+#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

quoted

+
+     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.

quoted

+             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.