| Message ID | 20210319155710.2793637-1-m.tretter@pengutronix.de |
|---|---|
| Headers | show |
| Series | net: phy: dp83867: Configure LED modes via device tree | expand |
On Fri, 19 Mar 2021 16:57:10 +0100, Michael Tretter wrote: > From: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > > The DP83867 supports four configurable LEDs. Several functions can be > multiplexed to these LEDs. The multiplexing can be configured in the > LEDCR1 register. > > Add support for changing the multiplexing of the LEDs via device tree. > > Signed-off-by: Thomas Haemmerle <thomas.haemmerle@wolfvision.net> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/net/phy/dp83867.c | 57 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index 9bd9a5c0b1db..dfcac95941f3 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -25,6 +25,7 @@ > #define MII_DP83867_MICR 0x12 > #define MII_DP83867_ISR 0x13 > #define DP83867_CFG2 0x14 > +#define DP83867_LEDCR1 0x18 > #define DP83867_CFG3 0x1e > #define DP83867_CTRL 0x1f > > @@ -138,6 +139,12 @@ > #define DP83867_DOWNSHIFT_4_COUNT 4 > #define DP83867_DOWNSHIFT_8_COUNT 8 > > +/* LEDCR1 bits */ > +#define DP83867_LEDCR1_LED_0_SEL GENMASK(3, 0) > +#define DP83867_LEDCR1_LED_1_SEL GENMASK(7, 4) > +#define DP83867_LEDCR1_LED_2_SEL GENMASK(11, 8) > +#define DP83867_LEDCR1_LED_GPIO_SEL GENMASK(15, 12) > + > /* CFG3 bits */ > #define DP83867_CFG3_INT_OE BIT(7) > #define DP83867_CFG3_ROBUST_AUTO_MDIX BIT(9) > @@ -154,6 +161,14 @@ enum { > DP83867_PORT_MIRROING_DIS, > }; > > +enum { > + DP83867_LED_0, > + DP83867_LED_1, > + DP83867_LED_2, > + DP83867_LED_GPIO, > + DP83867_LED_MAX, > +}; > + > struct dp83867_private { > u32 rx_id_delay; > u32 tx_id_delay; > @@ -165,6 +180,7 @@ struct dp83867_private { > bool set_clk_output; > u32 clk_output_sel; > bool sgmii_ref_clk_en; > + u32 led_mode[DP83867_LED_MAX]; > }; > > static int dp83867_ack_interrupt(struct phy_device *phydev) > @@ -521,6 +537,27 @@ static int dp83867_verify_rgmii_cfg(struct phy_device *phydev) > } > > #if IS_ENABLED(CONFIG_OF_MDIO) > +static int dp83867_of_led_mode_read(struct device_node *of_node, > + const char *led_name, u32 *mode) > +{ > + u32 tmp; > + int index; > + int err; > + > + index = of_property_match_string(of_node, "ti,dp83867-led-mode-names", > + led_name); > + err = of_property_read_u32_index(of_node, "ti,dp83867-led-modes", > + index, tmp); This should have been &tmp. I will wait for some more review feedback by others especially for the dt bindings and send a v2. > + if (err) > + return err; > + if (tmp == 0xc || tmp >= 0xf) > + return -EINVAL; > + > + *mode = tmp; > + > + return 0; > +} > + > static int dp83867_of_init(struct phy_device *phydev) > { > struct dp83867_private *dp83867 = phydev->priv; > @@ -614,6 +651,15 @@ static int dp83867_of_init(struct phy_device *phydev) > return -EINVAL; > } > > + dp83867_of_led_mode_read(of_node, "led-0", > + &dp83867->led_mode[DP83867_LED_0]); > + dp83867_of_led_mode_read(of_node, "led-1", > + &dp83867->led_mode[DP83867_LED_1]); > + dp83867_of_led_mode_read(of_node, "led-2", > + &dp83867->led_mode[DP83867_LED_2]); > + dp83867_of_led_mode_read(of_node, "led-gpio", > + &dp83867->led_mode[DP83867_LED_GPIO]); > + > return 0; > } > #else > @@ -632,6 +678,11 @@ static int dp83867_probe(struct phy_device *phydev) > if (!dp83867) > return -ENOMEM; > > + dp83867->led_mode[DP83867_LED_0] = DP83867_LED_LINK_EST; > + dp83867->led_mode[DP83867_LED_1] = DP83867_LED_1000_BT_LINK; > + dp83867->led_mode[DP83867_LED_2] = DP83867_LED_RX_TX_ACT; > + dp83867->led_mode[DP83867_LED_GPIO] = DP83867_LED_100_BT_LINK; > + > phydev->priv = dp83867; > > return dp83867_of_init(phydev); > @@ -792,6 +843,12 @@ static int dp83867_config_init(struct phy_device *phydev) > phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, val); > } > > + val = FIELD_PREP(DP83867_LEDCR1_LED_0_SEL, dp83867->led_mode[DP83867_LED_0]) | > + FIELD_PREP(DP83867_LEDCR1_LED_1_SEL, dp83867->led_mode[DP83867_LED_1]) | > + FIELD_PREP(DP83867_LEDCR1_LED_2_SEL, dp83867->led_mode[DP83867_LED_2]) | > + FIELD_PREP(DP83867_LEDCR1_LED_GPIO_SEL, dp83867->led_mode[DP83867_LED_GPIO]); > + phy_write(phydev, DP83867_LEDCR1, val); > + > val = phy_read(phydev, DP83867_CFG3); > /* Enable Interrupt output INT_OE in CFG3 register */ > if (phy_interrupt_is_valid(phydev)) > -- > 2.29.2 > >
On Fri, 19 Mar 2021 22:19:44 +0100, Andrew Lunn wrote: > On Fri, Mar 19, 2021 at 04:57:08PM +0100, Michael Tretter wrote: > > The dp83867 has 4 LED pins, which can be multiplexed with different functions > > of the phy. > > > > This series adds a device tree binding to describe the multiplexing of the > > functions to the LEDs and implements the binding for the dp83867 phy. > > > > I found existing bindings for configuring the LED modes for other phys: > > > > In Documentation/devicetree/bindings/net/micrel.txt, the binding is not > > flexible enough for the use case in the dp83867, because there is a value for > > each LED configuration, which would be a lot of values for the dp83867. > > > > In Documentation/devicetree/bindings/net/mscc-phy-vsc8532.txt, there is a > > separate property for each LED, which would work, but I found rather > > unintuitive compared to how clock bindings etc. work. > > > > The new binding defines two properties: one for the led names and another > > property for the modes of the LEDs with defined values in the same order. > > Currently, the binding is specific to the dp83867, but I guess that the > > binding could be made more generic and used for other phys, too. > > There is some work going on to manage PHY LEDs just like other LEDs in > Linux, using /sys/class/leds. > > Please try to help out with that work, rather than adding yet another > DT binding. Oh, thanks. That's even better. For reference: https://lore.kernel.org/netdev/20190813191147.19936-1-mka@chromium.org/ Michael