* [PATCH 0/2] net: phy: dp83867: add DT bindings and support for active low LEDs
@ 2022-11-03 14:31 Rasmus Villemoes
2022-11-03 14:31 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Rasmus Villemoes
2022-11-03 14:31 ` [PATCH 2/2] net: phy: dp83867: implement support for ti,ledX-active-low bindings Rasmus Villemoes
0 siblings, 2 replies; 7+ messages in thread
From: Rasmus Villemoes @ 2022-11-03 14:31 UTC (permalink / raw
To: Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, Rob Herring, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties
2022-11-03 14:31 [PATCH 0/2] net: phy: dp83867: add DT bindings and support for active low LEDs Rasmus Villemoes
@ 2022-11-03 14:31 ` Rasmus Villemoes
2022-11-03 22:17 ` Andrew Lunn
2022-11-03 14:31 ` [PATCH 2/2] net: phy: dp83867: implement support for ti,ledX-active-low bindings Rasmus Villemoes
1 sibling, 1 reply; 7+ messages in thread
From: Rasmus Villemoes @ 2022-11-03 14:31 UTC (permalink / raw
To: Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, Rob Herring, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] net: phy: dp83867: implement support for ti,ledX-active-low bindings
2022-11-03 14:31 [PATCH 0/2] net: phy: dp83867: add DT bindings and support for active low LEDs Rasmus Villemoes
2022-11-03 14:31 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Rasmus Villemoes
@ 2022-11-03 14:31 ` Rasmus Villemoes
1 sibling, 0 replies; 7+ messages in thread
From: Rasmus Villemoes @ 2022-11-03 14:31 UTC (permalink / raw
To: Andrew Lunn, Heiner Kallweit, Russell King
Cc: netdev, devicetree, Rob Herring, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties
2022-11-03 14:31 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Rasmus Villemoes
@ 2022-11-03 22:17 ` Andrew Lunn
2022-11-04 7:17 ` Rasmus Villemoes
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Lunn @ 2022-11-03 22:17 UTC (permalink / raw
To: Rasmus Villemoes
Cc: Heiner Kallweit, Russell King, netdev, devicetree, Rob Herring
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties
2022-11-03 22:17 ` Andrew Lunn
@ 2022-11-04 7:17 ` Rasmus Villemoes
2022-11-04 8:11 ` [PATCH 1/2] dt-bindings: dp83867: define ti, ledX-active-low properties Alexander Stein
2022-11-04 13:21 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Andrew Lunn
0 siblings, 2 replies; 7+ messages in thread
From: Rasmus Villemoes @ 2022-11-04 7:17 UTC (permalink / raw
To: Andrew Lunn
Cc: Heiner Kallweit, Russell King, netdev, devicetree, Rob Herring
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: dp83867: define ti, ledX-active-low properties
2022-11-04 7:17 ` Rasmus Villemoes
@ 2022-11-04 8:11 ` Alexander Stein
2022-11-04 13:21 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Andrew Lunn
1 sibling, 0 replies; 7+ messages in thread
From: Alexander Stein @ 2022-11-04 8:11 UTC (permalink / raw
To: Andrew Lunn, Rasmus Villemoes
Cc: Heiner Kallweit, Russell King, netdev, devicetree, Rob Herring
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/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties
2022-11-04 7:17 ` Rasmus Villemoes
2022-11-04 8:11 ` [PATCH 1/2] dt-bindings: dp83867: define ti, ledX-active-low properties Alexander Stein
@ 2022-11-04 13:21 ` Andrew Lunn
1 sibling, 0 replies; 7+ messages in thread
From: Andrew Lunn @ 2022-11-04 13:21 UTC (permalink / raw
To: Rasmus Villemoes
Cc: Heiner Kallweit, Russell King, netdev, devicetree, Rob Herring
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-04 13:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-03 14:31 [PATCH 0/2] net: phy: dp83867: add DT bindings and support for active low LEDs Rasmus Villemoes
2022-11-03 14:31 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Rasmus Villemoes
2022-11-03 22:17 ` Andrew Lunn
2022-11-04 7:17 ` Rasmus Villemoes
2022-11-04 8:11 ` [PATCH 1/2] dt-bindings: dp83867: define ti, ledX-active-low properties Alexander Stein
2022-11-04 13:21 ` [PATCH 1/2] dt-bindings: dp83867: define ti,ledX-active-low properties Andrew Lunn
2022-11-03 14:31 ` [PATCH 2/2] net: phy: dp83867: implement support for ti,ledX-active-low bindings Rasmus Villemoes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).