[3/4] lan78xx: Read LED modes from Device Tree

[3/4] lan78xx: Read LED modes from Device Tree

[Phil Elwell]

Add support for DT property "microchip,led-modes", a vector of two
cells (u32s) in the range 0-15, each of which sets the mode for one
of the two LEDs. Some possible values are:

    0=link/activity          1=link1000/activity
    2=link100/activity       3=link10/activity
    4=link100/1000/activity  5=link10/1000/activity
    6=link10/100/activity    14=off    15=on

Also use the presence of the DT property to indicate that the
LEDs should be enabled - necessary in the event that no valid OTP
or EEPROM is available.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d98397b..ffb483d 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 {
 	int ret;
 	u32 mii_adv;
+	u32 led_modes[2];
 	struct phy_device *phydev;
 
 	phydev = phy_find_first(dev->mdiobus);
@@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		(void)lan78xx_set_eee(dev->net, &edata);
 	}
 
+	if (!of_property_read_u32_array(dev->udev->dev.of_node,
+					"microchip,led-modes",
+					led_modes, ARRAY_SIZE(led_modes))) {
+		u32 reg;
+		int i;
+
+		reg = phy_read(phydev, 0x1d);
+		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
+			reg &= ~(0xf << (i * 4));
+			reg |= (led_modes[i] & 0xf) << (i * 4);
+		}
+		(void)phy_write(phydev, 0x1d, reg);
+
+		/* Ensure the LEDs are enabled */
+		lan78xx_read_reg(dev, HW_CFG, &reg);
+		reg |= HW_CFG_LED0_EN_ | HW_CFG_LED1_EN_;
+		lan78xx_write_reg(dev, HW_CFG, reg);
+	}
+
 	genphy_config_aneg(phydev);
 
 	dev->fc_autoneg = phydev->autoneg;

[3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
> Add support for DT property "microchip,led-modes", a vector of two
> cells (u32s) in the range 0-15, each of which sets the mode for one
> of the two LEDs. Some possible values are:
> 
>     0=link/activity          1=link1000/activity
>     2=link100/activity       3=link10/activity
>     4=link100/1000/activity  5=link10/1000/activity
>     6=link10/100/activity    14=off    15=on
> 
> Also use the presence of the DT property to indicate that the
> LEDs should be enabled - necessary in the event that no valid OTP
> or EEPROM is available.

I'm not a fan of this, but at the moment, we don't have anything
better.

Please follow what mscc does, add a header file for the LED settings.

       Andrew

> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
>  drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d98397b..ffb483d 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  {
>  	int ret;
>  	u32 mii_adv;
> +	u32 led_modes[2];
>  	struct phy_device *phydev;
>  
>  	phydev = phy_find_first(dev->mdiobus);
> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		(void)lan78xx_set_eee(dev->net, &edata);
>  	}
>  
> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> +					"microchip,led-modes",
> +					led_modes, ARRAY_SIZE(led_modes))) {
> +		u32 reg;
> +		int i;
> +
> +		reg = phy_read(phydev, 0x1d);
> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> +			reg &= ~(0xf << (i * 4));
> +			reg |= (led_modes[i] & 0xf) << (i * 4);
> +		}

Please add range checks for led_modes[i] and return -EINVAL if the
check fails.

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

[3/4] lan78xx: Read LED modes from Device Tree

[Phil Elwell]

Hi Andrew,

On 12/04/2018 15:26, Andrew Lunn wrote:
> On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote:
>> Add support for DT property "microchip,led-modes", a vector of two
>> cells (u32s) in the range 0-15, each of which sets the mode for one
>> of the two LEDs. Some possible values are:
>>
>>     0=link/activity          1=link1000/activity
>>     2=link100/activity       3=link10/activity
>>     4=link100/1000/activity  5=link10/1000/activity
>>     6=link10/100/activity    14=off    15=on
>>
>> Also use the presence of the DT property to indicate that the
>> LEDs should be enabled - necessary in the event that no valid OTP
>> or EEPROM is available.
> 
> I'm not a fan of this, but at the moment, we don't have anything
> better.
> 
> Please follow what mscc does, add a header file for the LED settings.

Good idea.

> 
>>
>> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>> ---
>>  drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index d98397b..ffb483d 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>>  {
>>  	int ret;
>>  	u32 mii_adv;
>> +	u32 led_modes[2];
>>  	struct phy_device *phydev;
>>  
>>  	phydev = phy_find_first(dev->mdiobus);
>> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>>  		(void)lan78xx_set_eee(dev->net, &edata);
>>  	}
>>  
>> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
>> +					"microchip,led-modes",
>> +					led_modes, ARRAY_SIZE(led_modes))) {
>> +		u32 reg;
>> +		int i;
>> +
>> +		reg = phy_read(phydev, 0x1d);
>> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
>> +			reg &= ~(0xf << (i * 4));
>> +			reg |= (led_modes[i] & 0xf) << (i * 4);
>> +		}
> 
> Please add range checks for led_modes[i] and return -EINVAL if the
> check fails.

Will do.

Thanks,

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

[3/4] lan78xx: Read LED modes from Device Tree

[Andrew Lunn]

> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		(void)lan78xx_set_eee(dev->net, &edata);
>  	}
>  
> +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> +					"microchip,led-modes",
> +					led_modes, ARRAY_SIZE(led_modes))) {
> +		u32 reg;
> +		int i;
> +
> +		reg = phy_read(phydev, 0x1d);
> +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> +			reg &= ~(0xf << (i * 4));
> +			reg |= (led_modes[i] & 0xf) << (i * 4);
> +		}
> +		(void)phy_write(phydev, 0x1d, reg);

Poking PHY registers directly from the MAC driver is not always a good
idea. This MAC driver does that in a few places :-(

What do we know about the PHY? It is built into the device or is it
external? If it is external, how do you know the LED register is at
0x1d?

The safest place to do this is in the PHY driver, and place these OF
properties into the PHY node.

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

[3/4] lan78xx: Read LED modes from Device Tree

[Woojung.Huh]

> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		(void)lan78xx_set_eee(dev->net, &edata);
> >  	}
> >
> > +	if (!of_property_read_u32_array(dev->udev->dev.of_node,
> > +					"microchip,led-modes",
> > +					led_modes, ARRAY_SIZE(led_modes))) {
> > +		u32 reg;
> > +		int i;
> > +
> > +		reg = phy_read(phydev, 0x1d);
> > +		for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> > +			reg &= ~(0xf << (i * 4));
> > +			reg |= (led_modes[i] & 0xf) << (i * 4);
> > +		}
> > +		(void)phy_write(phydev, 0x1d, reg);
> 
> Poking PHY registers directly from the MAC driver is not always a good
> idea. This MAC driver does that in a few places :-(
Agree but, some are for workaround unfortunately.

> What do we know about the PHY? It is built into the device or is it
> external? If it is external, how do you know the LED register is at
> 0x1d?
This register is not defined in include/linux/microchipphy.h. :(
Also agree that there parts should be applied to internal PHY only.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html