[PATCH 0/2] net: phy: dp83867: add DT bindings and support for active low LEDs

[PATCH 0/2] net: phy: dp83867: add DT bindings and support for active low LEDs

[Rasmus Villemoes]

We have a board with link/activity LEDs driven by the LED_0 and LED_2
pins of the dp83867, but they are currently reversed. Add DT bindings
to allow specifying they are active low, and hook up driver support.

Rasmus Villemoes (2):
  dt-bindings: dp83867: define ti,ledX-active-low properties
  net: phy: dp83867: implement support for ti,ledX-active-low bindings

 .../devicetree/bindings/net/ti,dp83867.yaml   | 15 +++++++++
 drivers/net/phy/dp83867.c                     | 32 +++++++++++++++++++
 2 files changed, 47 insertions(+)

-- 
2.37.2

[PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties

[Rasmus Villemoes]

The dp83867 has three LED_X pins that can be used to drive LEDs. They
are by default driven active high, but on some boards the reverse is
needed. Add bindings to allow a board to specify that they should be
active low.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../devicetree/bindings/net/ti,dp83867.yaml       | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index b8c0e4b5b494..8483544e28c3 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -118,6 +118,21 @@ properties:
       Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h for applicable
       values.
 
+  ti,led0-active-low:
+    type: boolean
+    description: |
+      This denotes that the LED_0 pin should be driven as active low.
+
+  ti,led1-active-low:
+    type: boolean
+    description: |
+      This denotes that the LED_1 pin should be driven as active low.
+
+  ti,led2-active-low:
+    type: boolean
+    description: |
+      This denotes that the LED_2 pin should be driven as active low.
+
 required:
   - reg
 
-- 
2.37.2

Re: [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties

[Andrew Lunn]

On Thu, Nov 03, 2022 at 03:31:17PM +0100, Rasmus Villemoes wrote:
> The dp83867 has three LED_X pins that can be used to drive LEDs. They
> are by default driven active high, but on some boards the reverse is
> needed. Add bindings to allow a board to specify that they should be
> active low.

Somebody really does need to finish the PHY LEDs via /sys/class/leds.
It looks like this would then be a reasonable standard property:
active-low, not a vendor property.

Please help out with the PHY LEDs patches.

       Andrew

Re: [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties

[Rasmus Villemoes]

On 03/11/2022 23.17, Andrew Lunn wrote:
> On Thu, Nov 03, 2022 at 03:31:17PM +0100, Rasmus Villemoes wrote:
>> The dp83867 has three LED_X pins that can be used to drive LEDs. They
>> are by default driven active high, but on some boards the reverse is
>> needed. Add bindings to allow a board to specify that they should be
>> active low.
> 
> Somebody really does need to finish the PHY LEDs via /sys/class/leds.
> It looks like this would then be a reasonable standard property:
> active-low, not a vendor property.
> 
> Please help out with the PHY LEDs patches.

So how do you imagine this to work in DT? Should the dp83867 phy node
grow a subnode like this?

  leds {
    #address-cells = <1>;
    #size-cells = <0>;

    led@0 {
      reg = <0>;
      active-low;
    };
    led@2 {
      reg = <2>;
      active-low;
    };
  };

Since the phy drives the leds automatically based on (by default)
link/activity, there's not really any need for a separate LED driver nor
do I see what would be gained by somehow listing the LEDs in
/sys/class/leds. Please expand.

Rasmus

Re: [PATCH 1/2] dt-bindings: dp83867: define ti, ledX-active-low properties

[Alexander Stein]

Am Freitag, 4. November 2022, 08:17:44 CET schrieb Rasmus Villemoes:
> On 03/11/2022 23.17, Andrew Lunn wrote:
> > On Thu, Nov 03, 2022 at 03:31:17PM +0100, Rasmus Villemoes wrote:
> >> The dp83867 has three LED_X pins that can be used to drive LEDs. They
> >> are by default driven active high, but on some boards the reverse is
> >> needed. Add bindings to allow a board to specify that they should be
> >> active low.
> > 
> > Somebody really does need to finish the PHY LEDs via /sys/class/leds.
> > It looks like this would then be a reasonable standard property:
> > active-low, not a vendor property.
> > 
> > Please help out with the PHY LEDs patches.
> 
> So how do you imagine this to work in DT? Should the dp83867 phy node
> grow a subnode like this?
> 
>   leds {
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     led@0 {
>       reg = <0>;
>       active-low;
>     };
>     led@2 {
>       reg = <2>;
>       active-low;
>     };
>   };
> 
> Since the phy drives the leds automatically based on (by default)
> link/activity, there's not really any need for a separate LED driver nor
> do I see what would be gained by somehow listing the LEDs in
> /sys/class/leds. Please expand.

There have been several tries to support LED support directly per DT, e.g. [1] 
& [2]. I assume Andrew is referring to [3].

Best regards,
Alexander

[1] https://lore.kernel.org/netdev/YFUVcLCzONhPmeh8@lunn.ch/T/
[2] https://www.spinics.net/lists/netdev/msg677827.html
[3] https://patches.linaro.org/project/linux-leds/cover/
20220503151633.18760-1-ansuelsmth@gmail.com/

Re: [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties

[Andrew Lunn]

On Fri, Nov 04, 2022 at 08:17:44AM +0100, Rasmus Villemoes wrote:
> On 03/11/2022 23.17, Andrew Lunn wrote:
> > On Thu, Nov 03, 2022 at 03:31:17PM +0100, Rasmus Villemoes wrote:
> >> The dp83867 has three LED_X pins that can be used to drive LEDs. They
> >> are by default driven active high, but on some boards the reverse is
> >> needed. Add bindings to allow a board to specify that they should be
> >> active low.
> > 
> > Somebody really does need to finish the PHY LEDs via /sys/class/leds.
> > It looks like this would then be a reasonable standard property:
> > active-low, not a vendor property.
> > 
> > Please help out with the PHY LEDs patches.
> 
> So how do you imagine this to work in DT? Should the dp83867 phy node
> grow a subnode like this?
> 
>   leds {
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     led@0 {
>       reg = <0>;
>       active-low;
>     };
>     led@2 {
>       reg = <2>;
>       active-low;
>     };
>   };

Yes, something like that. They should follow the DT binding for LEDs.
Documentation/devicetree/bindings/leds/common.yaml

> 
> Since the phy drives the leds automatically based on (by default)
> link/activity, there's not really any need for a separate LED driver nor
> do I see what would be gained by somehow listing the LEDs in
> /sys/class/leds. Please expand.

The PHY driver would be the LED driver, hopefully with some shared
code in phylib. You can then use the standard Linux LED way of
configuring what the LED means, using triggers. Those triggers get
offloaded to the hardware when possible, or done in software when not.
The DT binding would then follow the common LED binding.

What i want to get away from is that there is no consistent DT binding
for PHY leds. In fact, there are at least four different ways to
configure PHY leds, and you want to add a fifth.

	  Andrew

[PATCH 2/2] net: phy: dp83867: implement support for ti,ledX-active-low bindings

[Rasmus Villemoes]

The LED_X_POLARITY bits in the LEDCR2 register are default 1, meaning
the LEDs are driven as active high. On some boards, the LEDs are
active low, so implement support for clearing those bits when the
corresponding ti,ledX-active-low DT property is present.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/phy/dp83867.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 417527f8bbf5..8e8078ef2881 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -26,6 +26,7 @@
 #define MII_DP83867_MICR	0x12
 #define MII_DP83867_ISR		0x13
 #define DP83867_CFG2		0x14
+#define DP83867_LEDCR2		0x19
 #define DP83867_CFG3		0x1e
 #define DP83867_CTRL		0x1f
 
@@ -140,6 +141,11 @@
 #define DP83867_DOWNSHIFT_8_COUNT	8
 #define DP83867_SGMII_AUTONEG_EN	BIT(7)
 
+/* LEDCR2 bits */
+#define DP83867_LEDCR2_LED_0_POLARITY		BIT(2)
+#define DP83867_LEDCR2_LED_1_POLARITY		BIT(6)
+#define DP83867_LEDCR2_LED_2_POLARITY		BIT(10)
+
 /* CFG3 bits */
 #define DP83867_CFG3_INT_OE			BIT(7)
 #define DP83867_CFG3_ROBUST_AUTO_MDIX		BIT(9)
@@ -167,6 +173,9 @@ struct dp83867_private {
 	bool set_clk_output;
 	u32 clk_output_sel;
 	bool sgmii_ref_clk_en;
+	bool led0_active_low;
+	bool led1_active_low;
+	bool led2_active_low;
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -658,6 +667,13 @@ static int dp83867_of_init(struct phy_device *phydev)
 		return -EINVAL;
 	}
 
+	dp83867->led0_active_low = of_property_read_bool(of_node,
+							 "ti,led0-active-low");
+	dp83867->led1_active_low = of_property_read_bool(of_node,
+							 "ti,led1-active-low");
+	dp83867->led2_active_low = of_property_read_bool(of_node,
+							 "ti,led2-active-low");
+
 	return 0;
 }
 #else
@@ -890,6 +906,22 @@ static int dp83867_config_init(struct phy_device *phydev)
 			       mask, val);
 	}
 
+	if (dp83867->led0_active_low) {
+		ret = phy_modify(phydev, DP83867_LEDCR2, DP83867_LEDCR2_LED_0_POLARITY, 0);
+		if (ret)
+			return ret;
+	}
+	if (dp83867->led1_active_low) {
+		ret = phy_modify(phydev, DP83867_LEDCR2, DP83867_LEDCR2_LED_1_POLARITY, 0);
+		if (ret)
+			return ret;
+	}
+	if (dp83867->led2_active_low) {
+		ret = phy_modify(phydev, DP83867_LEDCR2, DP83867_LEDCR2_LED_2_POLARITY, 0);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.37.2