| Message ID | 1523541336-145953-4-git-send-email-phil@raspberrypi.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | lan78xx: Read configuration from Device Tree | expand |
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
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
> @@ -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
> > @@ -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.
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 |= HW_CFG_LED0_EN_ | HW_CFG_LED1_EN_; + lan78xx_write_reg(dev, HW_CFG, reg); + } + genphy_config_aneg(phydev); dev->fc_autoneg = phydev->autoneg;
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(+)