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:
> > From: Raju Lakkaraju <raju.lakkar...@microsemi.com>
> >
> > 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.
> > +- 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.
> > +
> > + 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.
> > + 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.
> > + 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.
