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

Hi Andrew,

Thank you for review the code.

On Fri, Sep 09, 2016 at 03:18:32PM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL

quoted

quoted

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.

Sorry, i worded that poorly. I was meaning between the u8 and edge. A
single space is enough.

I accepted your suggestion.

quoted

quoted

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.

Magic values are generally not accepted in device tree bindings. Both
Micrel and Renesas define their clock skew in ps, for example. Since
this is rise time, it should also be possible to define it in a unit
of time.

I accepted your comment. I had discussion with my hardware team and explained
the code review comments.
They asked me to define as picoseconds as units.

quoted

quoted

quoted

 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.

So if it cannot happen, don't check for it.

Also, by convention, you allocate memory in the .probe() function of a
driver. Please do it there.

I accepted your review comment. 
I will re-send the patch with updates.

        Andrew

---
Thanks,
Raju.