Re: [PATCH v2 net-next] net: phy: Add LED mode driver for Microsemi PHYs.

Hi Andrew,

Thank you for given review comments.

On Wed, Feb 01, 2017 at 02:55:55PM +0100, Andrew Lunn wrote:

EXTERNAL EMAIL


On Wed, Feb 01, 2017 at 06:23:46PM +0530, Raju Lakkaraju wrote:

quoted

From: Raju Lakkaraju <Raju.Lakkaraju-dzo6w/eZyo2tG0bUXCXiUA@public.gmane.org>

LED Mode:
+                       "include/dt-bindings/net/mscc-phy-vsc8531.h".
+                       Default value is 1.

1 is not very useful. What does it mean? How about:

                        Default value is LINK_1000_ACTIVITY (1)

Accepted.

quoted

+- vsc8531,led-1-mode : LED mode. Specify how the LED[1] should behave.
+                       Allowed values are define in
+     int rc;
+
+     if (!of_node)
+             return -ENODEV;
+
+     rc = of_property_read_u8(of_node, led, &led_mode);
+     if ((rc == 0) &&
+         ((led_mode > 15) || (led_mode == 7) || (led_mode == 11))) {
+             phydev_err(phydev, "DT %s invalid\n", led);
+             return -EINVAL;
+     } else if (rc == -EINVAL) {
+             return default_mode;
+     }

I don't think you understood my comment about of_property_read_u8()
not modifying &led_mode on error. Please read the comment again, and
simplify this.

If i understand your comment correctly, in case of return value -EINVAL, it
should return error. Am i correct?

i would perfer in case of of_property_read_u8( ) returns -EINVAL, 
i would like to configure LEDs with default state. Shall i configure?

Still if you think i did not understand your comments, please write little
bit more.

quoted

+
+     return led_mode;
+}
+
 #else
 static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
 {
      return 0;
 }
+
+static int vsc85xx_dt_led_mode_get(struct phy_device *phydev,
+                                char *led,
+                                u8 default_mode)
+{
+     return default_mode;
+}
 #endif /* CONFIG_OF_MDIO */

 static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)

@@ -499,6 +562,13 @@ static int vsc85xx_config_init(struct phy_device *phydev)
      if (rc)
              return rc;

+     rc = vsc85xx_led_cntl_set(phydev, 1, vsc8531->led_1_mode);
+     if (rc)
+             return rc;

Blank line please.

Accepted.

quoted

+     rc = vsc85xx_led_cntl_set(phydev, 0, vsc8531->led_0_mode);
+     if (rc)
+             return rc;
+
      rc = genphy_config_init(phydev);

      return rc;

@@ -555,8 +625,9 @@ static int vsc85xx_read_status(struct phy_device *phydev)
+     led_mode = vsc85xx_dt_led_mode_get(phydev, "vsc8531,led-0-mode",
+                                        LINK_1000_ACTIVITY);
+     if (led_mode < 0)
+             return led_mode;
+     vsc8531->led_0_mode = led_mode;

Blank line.

Accepted.

quoted

+     led_mode = vsc85xx_dt_led_mode_get(phydev, "vsc8531,led-1-mode",
+                                        LINK_100_ACTIVITY);
+#define AUTONEG_FAULT           12
+#define SERIAL_MODE             13
+#define FORCE_LED_OFF           14
+#define FORCE_LED_ON            15

You should prefix these with VSC8531_. Otherwise there is potential to
have issues with two different PHYs defining some of these frequently
used macros.

Accepted.

     Andrew

---
Thanks,
Raju.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html