| Message ID | 20221103143118.2199316-2-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: dp83867: add DT bindings and support for active low LEDs | expand |
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
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
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/
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
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
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(+)