| Message ID | 20230907084731.2181381-1-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: dp83867: Add support for hardware blinking LEDs | expand |
Hi Sascha, thanks for the patch. Am Donnerstag, 7. September 2023, 10:47:31 CEST schrieb Sascha Hauer: > This implements the led_hw_* hooks to support hardware blinking LEDs on > the DP83867 phy. The driver supports all LED modes that have a > corresponding TRIGGER_NETDEV_* define. Error and collision do not have > a TRIGGER_NETDEV_* define, so these modes are currently not supported. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> This works as intended so far. Unfortunately this driver and the PHY LED framework do not support active-low LEDs (yet). Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> #TQMa8MxML/MBa8Mx > --- > drivers/net/phy/dp83867.c | 137 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 137 insertions(+) > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index e397e7d642d92..5f08f9d38bd7a 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -159,6 +159,23 @@ > #define DP83867_LED_DRV_EN(x) BIT((x) * 4) > #define DP83867_LED_DRV_VAL(x) BIT((x) * 4 + 1) > > +#define DP83867_LED_FN(idx, val) (((val) & 0xf) << ((idx) * 4)) > +#define DP83867_LED_FN_MASK(idx) (0xf << ((idx) * 4)) > +#define DP83867_LED_FN_RX_ERR 0xe /* Receive Error */ > +#define DP83867_LED_FN_RX_TX_ERR 0xd /* Receive Error or Transmit Error */ > +#define DP83867_LED_FN_LINK_RX_TX 0xb /* Link established, blink for rx or > tx activity */ +#define DP83867_LED_FN_FULL_DUPLEX 0xa /* Full duplex */ > +#define DP83867_LED_FN_LINK_100_1000_BT 0x9 /* 100/1000BT link established > */ +#define DP83867_LED_FN_LINK_10_100_BT 0x8 /* 10/100BT link established > */ +#define DP83867_LED_FN_LINK_10_BT 0x7 /* 10BT link established */ > +#define DP83867_LED_FN_LINK_100_BTX 0x6 /* 100 BTX link established */ > +#define DP83867_LED_FN_LINK_1000_BT 0x5 /* 1000 BT link established */ > +#define DP83867_LED_FN_COLLISION 0x4 /* Collision detected */ > +#define DP83867_LED_FN_RX 0x3 /* Receive activity */ > +#define DP83867_LED_FN_TX 0x2 /* Transmit activity */ > +#define DP83867_LED_FN_RX_TX 0x1 /* Receive or Transmit activity */ > +#define DP83867_LED_FN_LINK 0x0 /* Link established */ > + > enum { > DP83867_PORT_MIRROING_KEEP, > DP83867_PORT_MIRROING_EN, > @@ -1018,6 +1035,123 @@ dp83867_led_brightness_set(struct phy_device > *phydev, val); > } > > +static int dp83867_led_mode(u8 index, unsigned long rules) > +{ > + if (index >= DP83867_LED_COUNT) > + return -EINVAL; > + > + switch (rules) { > + case BIT(TRIGGER_NETDEV_LINK): > + return DP83867_LED_FN_LINK; > + case BIT(TRIGGER_NETDEV_LINK_10): > + return DP83867_LED_FN_LINK_10_BT; > + case BIT(TRIGGER_NETDEV_LINK_100): > + return DP83867_LED_FN_LINK_100_BTX; > + case BIT(TRIGGER_NETDEV_FULL_DUPLEX): > + return DP83867_LED_FN_FULL_DUPLEX; > + case BIT(TRIGGER_NETDEV_TX): > + return DP83867_LED_FN_TX; > + case BIT(TRIGGER_NETDEV_RX): > + return DP83867_LED_FN_RX; > + case BIT(TRIGGER_NETDEV_LINK_1000): > + return DP83867_LED_FN_LINK_1000_BT; > + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): > + return DP83867_LED_FN_RX_TX; > + case BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK_1000): > + return DP83867_LED_FN_LINK_100_1000_BT; > + case BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100): > + return DP83867_LED_FN_LINK_10_100_BT; > + case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | > BIT(TRIGGER_NETDEV_RX): + return DP83867_LED_FN_LINK_RX_TX; > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int dp83867_led_hw_is_supported(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int ret; > + > + ret = dp83867_led_mode(index, rules); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int dp83867_led_hw_control_set(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int mode, ret; > + > + mode = dp83867_led_mode(index, rules); > + if (mode < 0) > + return mode; > + > + ret = phy_modify(phydev, DP83867_LEDCR1, DP83867_LED_FN_MASK(index), > + DP83867_LED_FN(index, mode)); > + if (ret) > + return ret; > + > + return phy_modify(phydev, DP83867_LEDCR2, DP83867_LED_DRV_EN(index), 0); > +} > + > +static int dp83867_led_hw_control_get(struct phy_device *phydev, u8 index, > + unsigned long *rules) > +{ > + int val; > + > + val = phy_read(phydev, DP83867_LEDCR1); > + if (val < 0) > + return val; > + > + val &= DP83867_LED_FN_MASK(index); > + val >>= index * 4; > + > + switch (val) { > + case DP83867_LED_FN_LINK: > + *rules = BIT(TRIGGER_NETDEV_LINK); > + break; > + case DP83867_LED_FN_LINK_10_BT: > + *rules = BIT(TRIGGER_NETDEV_LINK_10); > + break; > + case DP83867_LED_FN_LINK_100_BTX: > + *rules = BIT(TRIGGER_NETDEV_LINK_100); > + break; > + case DP83867_LED_FN_FULL_DUPLEX: > + *rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX); > + break; > + case DP83867_LED_FN_TX: > + *rules = BIT(TRIGGER_NETDEV_TX); > + break; > + case DP83867_LED_FN_RX: > + *rules = BIT(TRIGGER_NETDEV_RX); > + break; > + case DP83867_LED_FN_LINK_1000_BT: > + *rules = BIT(TRIGGER_NETDEV_LINK_1000); > + break; > + case DP83867_LED_FN_RX_TX: > + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); > + break; > + case DP83867_LED_FN_LINK_100_1000_BT: > + *rules = BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK_1000); > + break; > + case DP83867_LED_FN_LINK_10_100_BT: > + *rules = BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100); > + break; > + case DP83867_LED_FN_LINK_RX_TX: > + *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | > + BIT(TRIGGER_NETDEV_RX); > + break; > + default: > + *rules = 0; > + break; > + } > + > + return 0; > +} > + > static struct phy_driver dp83867_driver[] = { > { > .phy_id = DP83867_PHY_ID, > @@ -1047,6 +1181,9 @@ static struct phy_driver dp83867_driver[] = { > .set_loopback = dp83867_loopback, > > .led_brightness_set = dp83867_led_brightness_set, > + .led_hw_is_supported = dp83867_led_hw_is_supported, > + .led_hw_control_set = dp83867_led_hw_control_set, > + .led_hw_control_get = dp83867_led_hw_control_get, > }, > }; > module_phy_driver(dp83867_driver);
> This works as intended so far. Unfortunately this driver and the PHY LED > framework do not support active-low LEDs (yet). Polarity is something which i've seen a few PHY devices have. It also seems like a core LED concept, not something specific to PHY LEDs. So i think this needs to be partially addressed in the LED core. Andrew
On Thu, Sep 07, 2023 at 10:47:31AM +0200, Sascha Hauer wrote: > This implements the led_hw_* hooks to support hardware blinking LEDs on > the DP83867 phy. The driver supports all LED modes that have a > corresponding TRIGGER_NETDEV_* define. Error and collision do not have > a TRIGGER_NETDEV_* define, so these modes are currently not supported. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Sat, Sep 09, 2023 at 04:27:31AM +0200, Andrew Lunn wrote: > > This works as intended so far. Unfortunately this driver and the PHY LED > > framework do not support active-low LEDs (yet). > > Polarity is something which i've seen a few PHY devices have. It also > seems like a core LED concept, not something specific to PHY LEDs. So > i think this needs to be partially addressed in the LED core. However, doesn't the LED layer deals with LED brightness, not by logic state? It certainly looks that way, and it's left up to the drivers themselves to deal with any polarity inversion - which makes sense if the core is just concerned about brightness. Introducing inversion in the core means drivers will be passed a brightness of "100" for off and "0" for on which, do you not think, starts to get rather silly?
On Sat, Sep 09, 2023 at 08:15:37AM +0100, Russell King (Oracle) wrote: > On Sat, Sep 09, 2023 at 04:27:31AM +0200, Andrew Lunn wrote: > > > This works as intended so far. Unfortunately this driver and the PHY LED > > > framework do not support active-low LEDs (yet). > > > > Polarity is something which i've seen a few PHY devices have. It also > > seems like a core LED concept, not something specific to PHY LEDs. So > > i think this needs to be partially addressed in the LED core. > > However, doesn't the LED layer deals with LED brightness, not by logic > state? It certainly looks that way, and it's left up to the drivers > themselves to deal with any polarity inversion - which makes sense if > the core is just concerned about brightness. > > Introducing inversion in the core means drivers will be passed a > brightness of "100" for off and "0" for on which, do you not think, > starts to get rather silly? Documentation/devicetree/bindings/leds/leds-pwm.yaml has: active-low: description: For PWMs where the LED is wired to supply rather than ground. type: boolean leds-pwm-multicolor.yaml and leds-bcm6358.txt uses the same property. There is also one case of: irled/ir-spi-led.yaml led-active-low: type: boolean description: Output is negated with a NOT gate. The implementation of the properties is repeated in each driver. So we probably need to add the parsing of this property in phy_device.c, and add a led_config() op to struct phy_device to pass polarity information to the driver. Andrew
On Thu, 7 Sep 2023 10:47:31 +0200 Sascha Hauer wrote: > This implements the led_hw_* hooks to support hardware blinking LEDs on > the DP83867 phy. The driver supports all LED modes that have a > corresponding TRIGGER_NETDEV_* define. Error and collision do not have > a TRIGGER_NETDEV_* define, so these modes are currently not supported. FWIW I think this patch got marked as Deferred in patchwork because it was posted mid-merge window. Could you repost?
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index e397e7d642d92..5f08f9d38bd7a 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -159,6 +159,23 @@ #define DP83867_LED_DRV_EN(x) BIT((x) * 4) #define DP83867_LED_DRV_VAL(x) BIT((x) * 4 + 1) +#define DP83867_LED_FN(idx, val) (((val) & 0xf) << ((idx) * 4)) +#define DP83867_LED_FN_MASK(idx) (0xf << ((idx) * 4)) +#define DP83867_LED_FN_RX_ERR 0xe /* Receive Error */ +#define DP83867_LED_FN_RX_TX_ERR 0xd /* Receive Error or Transmit Error */ +#define DP83867_LED_FN_LINK_RX_TX 0xb /* Link established, blink for rx or tx activity */ +#define DP83867_LED_FN_FULL_DUPLEX 0xa /* Full duplex */ +#define DP83867_LED_FN_LINK_100_1000_BT 0x9 /* 100/1000BT link established */ +#define DP83867_LED_FN_LINK_10_100_BT 0x8 /* 10/100BT link established */ +#define DP83867_LED_FN_LINK_10_BT 0x7 /* 10BT link established */ +#define DP83867_LED_FN_LINK_100_BTX 0x6 /* 100 BTX link established */ +#define DP83867_LED_FN_LINK_1000_BT 0x5 /* 1000 BT link established */ +#define DP83867_LED_FN_COLLISION 0x4 /* Collision detected */ +#define DP83867_LED_FN_RX 0x3 /* Receive activity */ +#define DP83867_LED_FN_TX 0x2 /* Transmit activity */ +#define DP83867_LED_FN_RX_TX 0x1 /* Receive or Transmit activity */ +#define DP83867_LED_FN_LINK 0x0 /* Link established */ + enum { DP83867_PORT_MIRROING_KEEP, DP83867_PORT_MIRROING_EN, @@ -1018,6 +1035,123 @@ dp83867_led_brightness_set(struct phy_device *phydev, val); } +static int dp83867_led_mode(u8 index, unsigned long rules) +{ + if (index >= DP83867_LED_COUNT) + return -EINVAL; + + switch (rules) { + case BIT(TRIGGER_NETDEV_LINK): + return DP83867_LED_FN_LINK; + case BIT(TRIGGER_NETDEV_LINK_10): + return DP83867_LED_FN_LINK_10_BT; + case BIT(TRIGGER_NETDEV_LINK_100): + return DP83867_LED_FN_LINK_100_BTX; + case BIT(TRIGGER_NETDEV_FULL_DUPLEX): + return DP83867_LED_FN_FULL_DUPLEX; + case BIT(TRIGGER_NETDEV_TX): + return DP83867_LED_FN_TX; + case BIT(TRIGGER_NETDEV_RX): + return DP83867_LED_FN_RX; + case BIT(TRIGGER_NETDEV_LINK_1000): + return DP83867_LED_FN_LINK_1000_BT; + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return DP83867_LED_FN_RX_TX; + case BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK_1000): + return DP83867_LED_FN_LINK_100_1000_BT; + case BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100): + return DP83867_LED_FN_LINK_10_100_BT; + case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return DP83867_LED_FN_LINK_RX_TX; + default: + return -EOPNOTSUPP; + } +} + +static int dp83867_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int ret; + + ret = dp83867_led_mode(index, rules); + if (ret < 0) + return ret; + + return 0; +} + +static int dp83867_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode, ret; + + mode = dp83867_led_mode(index, rules); + if (mode < 0) + return mode; + + ret = phy_modify(phydev, DP83867_LEDCR1, DP83867_LED_FN_MASK(index), + DP83867_LED_FN(index, mode)); + if (ret) + return ret; + + return phy_modify(phydev, DP83867_LEDCR2, DP83867_LED_DRV_EN(index), 0); +} + +static int dp83867_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + int val; + + val = phy_read(phydev, DP83867_LEDCR1); + if (val < 0) + return val; + + val &= DP83867_LED_FN_MASK(index); + val >>= index * 4; + + switch (val) { + case DP83867_LED_FN_LINK: + *rules = BIT(TRIGGER_NETDEV_LINK); + break; + case DP83867_LED_FN_LINK_10_BT: + *rules = BIT(TRIGGER_NETDEV_LINK_10); + break; + case DP83867_LED_FN_LINK_100_BTX: + *rules = BIT(TRIGGER_NETDEV_LINK_100); + break; + case DP83867_LED_FN_FULL_DUPLEX: + *rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX); + break; + case DP83867_LED_FN_TX: + *rules = BIT(TRIGGER_NETDEV_TX); + break; + case DP83867_LED_FN_RX: + *rules = BIT(TRIGGER_NETDEV_RX); + break; + case DP83867_LED_FN_LINK_1000_BT: + *rules = BIT(TRIGGER_NETDEV_LINK_1000); + break; + case DP83867_LED_FN_RX_TX: + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); + break; + case DP83867_LED_FN_LINK_100_1000_BT: + *rules = BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK_1000); + break; + case DP83867_LED_FN_LINK_10_100_BT: + *rules = BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK_100); + break; + case DP83867_LED_FN_LINK_RX_TX: + *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | + BIT(TRIGGER_NETDEV_RX); + break; + default: + *rules = 0; + break; + } + + return 0; +} + static struct phy_driver dp83867_driver[] = { { .phy_id = DP83867_PHY_ID, @@ -1047,6 +1181,9 @@ static struct phy_driver dp83867_driver[] = { .set_loopback = dp83867_loopback, .led_brightness_set = dp83867_led_brightness_set, + .led_hw_is_supported = dp83867_led_hw_is_supported, + .led_hw_control_set = dp83867_led_hw_control_set, + .led_hw_control_get = dp83867_led_hw_control_get, }, }; module_phy_driver(dp83867_driver);
This implements the led_hw_* hooks to support hardware blinking LEDs on the DP83867 phy. The driver supports all LED modes that have a corresponding TRIGGER_NETDEV_* define. Error and collision do not have a TRIGGER_NETDEV_* define, so these modes are currently not supported. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/phy/dp83867.c | 137 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+)