[PATCH 0/3] add dt configuration for dp83867 led modes

[PATCH 0/3] add dt configuration for dp83867 led modes

[Tim Harvey]

This series adds a dt-prop ti,led-modes to configure dp83867 PHY led
modes, adds the code to implement it, and updates some board dt files
to use the new property.

Tim Harvey (3):
  dt-bindings: net: phy: dp83867: add LED mode property
  net: phy: dp83867: add LED mode configuration via dt
  arm64: dts: imx8m*-venice: add dp83867 PHY LED mode configuration

 .../devicetree/bindings/net/ti,dp83867.yaml   |  6 ++++
 .../dts/freescale/imx8mm-venice-gw700x.dtsi   |  6 ++++
 .../dts/freescale/imx8mm-venice-gw7902.dts    |  6 ++++
 .../dts/freescale/imx8mn-venice-gw7902.dts    |  6 ++++
 drivers/net/phy/dp83867.c                     | 32 +++++++++++++++++--
 include/dt-bindings/net/ti-dp83867.h          | 16 ++++++++++
 6 files changed, 70 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/3] dt-bindings: net: phy: dp83867: add LED mode property

[Tim Harvey]

Add description for new property ti,led-modes in binding file.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 .../devicetree/bindings/net/ti,dp83867.yaml      |  6 ++++++
 include/dt-bindings/net/ti-dp83867.h             | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.yaml b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
index b8c0e4b5b494..8b84c34d389f 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83867.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83867.yaml
@@ -118,6 +118,12 @@ properties:
       Transmitt FIFO depth- see dt-bindings/net/ti-dp83867.h for applicable
       values.
 
+  ti,led-modes:
+    description: |
+      List of LED modes - see dt-bindings/net/ti-dp83867.h for applicable
+      values.
+    $ref: schemas/types.yaml#/definitions/uint32-array
+
 required:
   - reg
 
diff --git a/include/dt-bindings/net/ti-dp83867.h b/include/dt-bindings/net/ti-dp83867.h
index 6fc4b445d3a1..ea3e17b27427 100644
--- a/include/dt-bindings/net/ti-dp83867.h
+++ b/include/dt-bindings/net/ti-dp83867.h
@@ -50,4 +50,20 @@
 #define DP83867_CLK_O_SEL_REF_CLK		0xC
 /* Special flag to indicate clock should be off */
 #define DP83867_CLK_O_SEL_OFF			0xFFFFFFFF
+
+/* LED_CFG - LED configuration selection */
+#define DP83867_LED_SEL_LINK			0
+#define DP83867_LED_SEL_ACT			1
+#define DP83867_LED_SEL_ACT_TX			2
+#define DP83867_LED_SEL_ACT_RX			3
+#define DP83867_LED_SEL_COL			4
+#define DP83867_LED_SEL_LINK_1000BT		5
+#define DP83867_LED_SEL_LINK_100BT		6
+#define DP83867_LED_SEL_LINK_10BT		7
+#define DP83867_LED_SEL_LINK_10_100BT		8
+#define DP83867_LED_SEL_LINK_100_1000BT		9
+#define DP83867_LED_SEL_FULL_DUPLEX		10
+#define DP83867_LED_SEL_LINK_ACT		11
+#define DP83867_LED_SEL_ERR_TX_RX		13
+#define DP83867_LED_SEL_ERR_RX			14
 #endif
-- 
2.25.1

[PATCH 2/3] net: phy: dp83867: add LED mode configuration via dt

[Tim Harvey]

Add configuration of LED modes per device-tree property ti,led-modes.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/net/phy/dp83867.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index 6939563d3b7c..008941a8d6aa 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_LEDCR1		0x18
 #define DP83867_CFG3		0x1e
 #define DP83867_CTRL		0x1f
 
@@ -150,6 +151,10 @@
 /* FLD_THR_CFG */
 #define DP83867_FLD_THR_CFG_ENERGY_LOST_THR_MASK	0x7
 
+/* LED Configuration 1 bits */
+#define DP83867_LED_MODE_SHIFT			4
+#define DP83867_LED_MODE_MASK			0xf
+
 enum {
 	DP83867_PORT_MIRROING_KEEP,
 	DP83867_PORT_MIRROING_EN,
@@ -167,6 +172,7 @@ struct dp83867_private {
 	bool set_clk_output;
 	u32 clk_output_sel;
 	bool sgmii_ref_clk_en;
+	int led_modes[4];
 };
 
 static int dp83867_ack_interrupt(struct phy_device *phydev)
@@ -573,7 +579,7 @@ static int dp83867_of_init(struct phy_device *phydev)
 	struct dp83867_private *dp83867 = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	struct device_node *of_node = dev->of_node;
-	int ret;
+	int ret, led;
 
 	if (!of_node)
 		return -ENODEV;
@@ -658,6 +664,13 @@ static int dp83867_of_init(struct phy_device *phydev)
 		return -EINVAL;
 	}
 
+	if (of_property_read_u32_array(of_node, "ti,led-modes",
+				       dp83867->led_modes,
+				       ARRAY_SIZE(dp83867->led_modes))) {
+		for (led = 0; led < ARRAY_SIZE(dp83867->led_modes); led++)
+			dp83867->led_modes[led] = -EINVAL;
+	}
+
 	return 0;
 }
 #else
@@ -665,6 +678,7 @@ static int dp83867_of_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
 	u16 delay;
+	int led;
 
 	/* For non-OF device, the RX and TX ID values are either strapped
 	 * or take from default value. So, we init RX & TX ID values here
@@ -682,6 +696,10 @@ static int dp83867_of_init(struct phy_device *phydev)
 	 */
 	dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN / 2;
 
+	/* LED mode unconfigured */
+	for (led = 0; led < ARRAY_SIZE(dp83867->led_modes); led++)
+		dp83867->led_modes[led] = -EINVAL;
+
 	return 0;
 }
 #endif /* CONFIG_OF_MDIO */
@@ -703,7 +721,7 @@ static int dp83867_probe(struct phy_device *phydev)
 static int dp83867_config_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
-	int ret, val, bs;
+	int ret, val, bs, led;
 	u16 delay;
 
 	/* Force speed optimization for the PHY even if it strapped */
@@ -882,6 +900,16 @@ static int dp83867_config_init(struct phy_device *phydev)
 			       mask, val);
 	}
 
+	/* LED Configuration */
+	val = phy_read(phydev, DP83867_LEDCR1);
+	for (led = 0; led < ARRAY_SIZE(dp83867->led_modes); led++) {
+		if (dp83867->led_modes[led] != -EINVAL) {
+			val &= ~(DP83867_LED_MODE_MASK << (DP83867_LED_MODE_SHIFT * led));
+			val |= (dp83867->led_modes[led] << (DP83867_LED_MODE_SHIFT * led));
+		}
+	}
+	phy_write(phydev, DP83867_LEDCR1, val);
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 3/3] arm64: dts: imx8m*-venice: add dp83867 PHY LED mode configuration

[Tim Harvey]

Add configuration for dp83867 PHY LED mode via ti,led-modes property.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi | 6 ++++++
 arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts  | 6 ++++++
 arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts  | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
index c305e325d007..bb9928153ff0 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
@@ -111,6 +111,12 @@ ethphy0: ethernet-phy@0 {
 			reg = <0>;
 			ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
 			ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
+			ti,led-modes = <
+				DP83867_LED_SEL_LINK
+				DP83867_LED_SEL_LINK_1000BT
+				DP83867_LED_SEL_LINK_ACT
+				DP83867_LED_SEL_LINK
+			>;
 			tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
 			rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
 		};
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts
index 11481e09c75b..d7de555cf5e1 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7902.dts
@@ -253,6 +253,12 @@ ethphy0: ethernet-phy@0 {
 			reg = <0>;
 			ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
 			ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
+			ti,led-modes = <
+				DP83867_LED_SEL_LINK
+				DP83867_LED_SEL_LINK_1000BT
+				DP83867_LED_SEL_LINK_ACT
+				DP83867_LED_SEL_LINK
+			>;
 			tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
 			rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
 		};
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
index 97582db71ca8..8e61966c8dd0 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-venice-gw7902.dts
@@ -248,6 +248,12 @@ ethphy0: ethernet-phy@0 {
 			reg = <0>;
 			ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
 			ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
+			ti,led-modes = <
+				DP83867_LED_SEL_LINK
+				DP83867_LED_SEL_LINK_1000BT
+				DP83867_LED_SEL_LINK_ACT
+				DP83867_LED_SEL_LINK
+			>;
 			tx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
 			rx-fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
 		};
-- 
2.25.1

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Andrew Lunn]

On Thu, Nov 17, 2022 at 04:15:45PM -0800, Tim Harvey wrote:
> This series adds a dt-prop ti,led-modes to configure dp83867 PHY led
> modes, adds the code to implement it, and updates some board dt files
> to use the new property.

Sorry, but NACK.

We need PHY leds to be controlled via /sys/class/leds. Everybody keeps
trying to add there own way to configure these things, rather than
have just one uniform way which all PHYs share.

     Andrew

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Tim Harvey]

On Thu, Nov 17, 2022 at 4:27 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Nov 17, 2022 at 04:15:45PM -0800, Tim Harvey wrote:
> > This series adds a dt-prop ti,led-modes to configure dp83867 PHY led
> > modes, adds the code to implement it, and updates some board dt files
> > to use the new property.
>
> Sorry, but NACK.
>
> We need PHY leds to be controlled via /sys/class/leds. Everybody keeps
> trying to add there own way to configure these things, rather than
> have just one uniform way which all PHYs share.
>
>      Andrew

Andrew,

I completely agree with you but I haven't seen how that can be done
yet. What support exists for a PHY driver to expose their LED
configuration to be used that way? Can you point me to an example?

Best Regards,

Tim

Re: [PATCH 1/3] dt-bindings: net: phy: dp83867: add LED mode property

[Rob Herring]

On Thu, 17 Nov 2022 16:15:46 -0800, Tim Harvey wrote:
> Add description for new property ti,led-modes in binding file.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  .../devicetree/bindings/net/ti,dp83867.yaml      |  6 ++++++
>  include/dt-bindings/net/ti-dp83867.h             | 16 ++++++++++++++++
>  2 files changed, 22 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/net/ti,dp83867.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/net/schemas/types.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221118001548.635752-2-tharvey@gateworks.com

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command.

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Andrew Lunn]

> Andrew,
> 
> I completely agree with you but I haven't seen how that can be done
> yet. What support exists for a PHY driver to expose their LED
> configuration to be used that way? Can you point me to an example?

Nobody has actually worked on this long enough to get code merged. e.g.
https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
https://lists.archive.carbon60.com/linux/kernel/3396223

This is probably the last attempt, which was not too far away from getting merged:
https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/

I seem to NACK a patch like yours every couple of months. If all that
wasted time was actually spent on a common framework, this would of
been solved years ago.

How important is it to you to control these LEDs? Enough to finish
this code and get it merged?

     Andrew

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Tim Harvey]

On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Andrew,
> >
> > I completely agree with you but I haven't seen how that can be done
> > yet. What support exists for a PHY driver to expose their LED
> > configuration to be used that way? Can you point me to an example?
>
> Nobody has actually worked on this long enough to get code merged. e.g.
> https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> https://lists.archive.carbon60.com/linux/kernel/3396223
>
> This is probably the last attempt, which was not too far away from getting merged:
> https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/
>
> I seem to NACK a patch like yours every couple of months. If all that
> wasted time was actually spent on a common framework, this would of
> been solved years ago.
>
> How important is it to you to control these LEDs? Enough to finish
> this code and get it merged?
>

Andrew,

Thanks for the links - the most recent attempt does look promising.
For whatever reason I don't have that series in my mail history so
it's not clear how I can respond to it.

Ansuel, are you planning on posting a v7 of 'Adds support for PHY LEDs
with offload triggers' [1]?

I'm not all that familiar with netdev led triggers. Is there a way to
configure the default offload blink mode via dt with your series? I
didn't quite follow how the offload function/blink-mode gets set.

Best Regards,

Tim
[1] https://patches.linaro.org/project/linux-leds/list/?submitter=10040

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Andrew Lunn]

On Fri, Nov 18, 2022 at 11:57:00AM -0800, Tim Harvey wrote:
> On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > Andrew,
> > >
> > > I completely agree with you but I haven't seen how that can be done
> > > yet. What support exists for a PHY driver to expose their LED
> > > configuration to be used that way? Can you point me to an example?
> >
> > Nobody has actually worked on this long enough to get code merged. e.g.
> > https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> > https://lists.archive.carbon60.com/linux/kernel/3396223
> >
> > This is probably the last attempt, which was not too far away from getting merged:
> > https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/
> >
> > I seem to NACK a patch like yours every couple of months. If all that
> > wasted time was actually spent on a common framework, this would of
> > been solved years ago.
> >
> > How important is it to you to control these LEDs? Enough to finish
> > this code and get it merged?
> >
> 
> Andrew,
> 
> Thanks for the links - the most recent attempt does look promising.
> For whatever reason I don't have that series in my mail history so
> it's not clear how I can respond to it.

apt-get install b4

> Ansuel, are you planning on posting a v7 of 'Adds support for PHY LEDs
> with offload triggers' [1]?
> 
> I'm not all that familiar with netdev led triggers. Is there a way to
> configure the default offload blink mode via dt with your series? I
> didn't quite follow how the offload function/blink-mode gets set.

The idea is that the PHY LEDs are just LEDs in the Linux LED
framework. So read Documentation/devicetree/bindings/leds/common.yaml.
The PHY should make use of these standard DT properties, including
linux,default-trigger.

	Andrew

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Tim Harvey]

On Sun, Nov 20, 2022 at 3:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Nov 18, 2022 at 11:57:00AM -0800, Tim Harvey wrote:
> > On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > Andrew,
> > > >
> > > > I completely agree with you but I haven't seen how that can be done
> > > > yet. What support exists for a PHY driver to expose their LED
> > > > configuration to be used that way? Can you point me to an example?
> > >
> > > Nobody has actually worked on this long enough to get code merged. e.g.
> > > https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> > > https://lists.archive.carbon60.com/linux/kernel/3396223
> > >
> > > This is probably the last attempt, which was not too far away from getting merged:
> > > https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/
> > >
> > > I seem to NACK a patch like yours every couple of months. If all that
> > > wasted time was actually spent on a common framework, this would of
> > > been solved years ago.
> > >
> > > How important is it to you to control these LEDs? Enough to finish
> > > this code and get it merged?
> > >
> >
> > Andrew,
> >
> > Thanks for the links - the most recent attempt does look promising.
> > For whatever reason I don't have that series in my mail history so
> > it's not clear how I can respond to it.
>
> apt-get install b4
>
> > Ansuel, are you planning on posting a v7 of 'Adds support for PHY LEDs
> > with offload triggers' [1]?
> >
> > I'm not all that familiar with netdev led triggers. Is there a way to
> > configure the default offload blink mode via dt with your series? I
> > didn't quite follow how the offload function/blink-mode gets set.
>
> The idea is that the PHY LEDs are just LEDs in the Linux LED
> framework. So read Documentation/devicetree/bindings/leds/common.yaml.
> The PHY should make use of these standard DT properties, including
> linux,default-trigger.
>
>         Andrew

Ansuel,

Are you planning on posting a v7 of 'Adds support for PHY LEDs with
offload triggers' [1]?

Best Regards,

Tim
[1] https://patches.linaro.org/project/linux-leds/list/?series=174704

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Christian Marangi]

On Thu, Dec 01, 2022 at 10:26:09AM -0800, Tim Harvey wrote:
> On Sun, Nov 20, 2022 at 3:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Fri, Nov 18, 2022 at 11:57:00AM -0800, Tim Harvey wrote:
> > > On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > Andrew,
> > > > >
> > > > > I completely agree with you but I haven't seen how that can be done
> > > > > yet. What support exists for a PHY driver to expose their LED
> > > > > configuration to be used that way? Can you point me to an example?
> > > >
> > > > Nobody has actually worked on this long enough to get code merged. e.g.
> > > > https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> > > > https://lists.archive.carbon60.com/linux/kernel/3396223
> > > >
> > > > This is probably the last attempt, which was not too far away from getting merged:
> > > > https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/
> > > >
> > > > I seem to NACK a patch like yours every couple of months. If all that
> > > > wasted time was actually spent on a common framework, this would of
> > > > been solved years ago.
> > > >
> > > > How important is it to you to control these LEDs? Enough to finish
> > > > this code and get it merged?
> > > >
> > >
> > > Andrew,
> > >
> > > Thanks for the links - the most recent attempt does look promising.
> > > For whatever reason I don't have that series in my mail history so
> > > it's not clear how I can respond to it.
> >
> > apt-get install b4
> >
> > > Ansuel, are you planning on posting a v7 of 'Adds support for PHY LEDs
> > > with offload triggers' [1]?
> > >
> > > I'm not all that familiar with netdev led triggers. Is there a way to
> > > configure the default offload blink mode via dt with your series? I
> > > didn't quite follow how the offload function/blink-mode gets set.
> >
> > The idea is that the PHY LEDs are just LEDs in the Linux LED
> > framework. So read Documentation/devicetree/bindings/leds/common.yaml.
> > The PHY should make use of these standard DT properties, including
> > linux,default-trigger.
> >
> >         Andrew
> 
> Ansuel,
> 
> Are you planning on posting a v7 of 'Adds support for PHY LEDs with
> offload triggers' [1]?
> 
> Best Regards,
> 
> Tim
> [1] https://patches.linaro.org/project/linux-leds/list/?series=174704

I can consider that only if there is a real interest for it and would
love some help by the netdev team to actually have a review from the
leds team...

I tried multiple time to propose it but I never got a review... only a
review from an external guy that wanted to follow his idea in every way
possible with the only intention of applying his code (sorry to be rude
about that but i'm more than happy to recover the work and search for a
common solution)

So yes this is still in my TODO list but it would help if others can
tell me that they want to actually review it. That would put that
project on priority and I would recover and push a v7.

-- 
	Ansuel

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Tim Harvey]

On Thu, Dec 1, 2022 at 10:31 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Thu, Dec 01, 2022 at 10:26:09AM -0800, Tim Harvey wrote:
> > On Sun, Nov 20, 2022 at 3:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Fri, Nov 18, 2022 at 11:57:00AM -0800, Tim Harvey wrote:
> > > > On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > >
> > > > > > Andrew,
> > > > > >
> > > > > > I completely agree with you but I haven't seen how that can be done
> > > > > > yet. What support exists for a PHY driver to expose their LED
> > > > > > configuration to be used that way? Can you point me to an example?
> > > > >
> > > > > Nobody has actually worked on this long enough to get code merged. e.g.
> > > > > https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> > > > > https://lists.archive.carbon60.com/linux/kernel/3396223
> > > > >
> > > > > This is probably the last attempt, which was not too far away from getting merged:
> > > > > https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/
> > > > >
> > > > > I seem to NACK a patch like yours every couple of months. If all that
> > > > > wasted time was actually spent on a common framework, this would of
> > > > > been solved years ago.
> > > > >
> > > > > How important is it to you to control these LEDs? Enough to finish
> > > > > this code and get it merged?
> > > > >
> > > >
> > > > Andrew,
> > > >
> > > > Thanks for the links - the most recent attempt does look promising.
> > > > For whatever reason I don't have that series in my mail history so
> > > > it's not clear how I can respond to it.
> > >
> > > apt-get install b4
> > >
> > > > Ansuel, are you planning on posting a v7 of 'Adds support for PHY LEDs
> > > > with offload triggers' [1]?
> > > >
> > > > I'm not all that familiar with netdev led triggers. Is there a way to
> > > > configure the default offload blink mode via dt with your series? I
> > > > didn't quite follow how the offload function/blink-mode gets set.
> > >
> > > The idea is that the PHY LEDs are just LEDs in the Linux LED
> > > framework. So read Documentation/devicetree/bindings/leds/common.yaml.
> > > The PHY should make use of these standard DT properties, including
> > > linux,default-trigger.
> > >
> > >         Andrew
> >
> > Ansuel,
> >
> > Are you planning on posting a v7 of 'Adds support for PHY LEDs with
> > offload triggers' [1]?
> >
> > Best Regards,
> >
> > Tim
> > [1] https://patches.linaro.org/project/linux-leds/list/?series=174704
>
> I can consider that only if there is a real interest for it and would
> love some help by the netdev team to actually have a review from the
> leds team...
>
> I tried multiple time to propose it but I never got a review... only a
> review from an external guy that wanted to follow his idea in every way
> possible with the only intention of applying his code (sorry to be rude
> about that but i'm more than happy to recover the work and search for a
> common solution)
>
> So yes this is still in my TODO list but it would help if others can
> tell me that they want to actually review it. That would put that
> project on priority and I would recover and push a v7.
>
> --
>         Ansuel

Ansuel,

Considering Andrew is nak'ing any phy code to configure LED's until a
solution using via /sys/class/leds is provided I would say there is
real interest.

It seems to me that you got very positive feedback for this last
series. I would think if you submitted without RFC it would catch more
eyes as well.

Best Regards,

Tim

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Christian Marangi]

On Thu, Dec 01, 2022 at 10:35:46AM -0800, Tim Harvey wrote:
> On Thu, Dec 1, 2022 at 10:31 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > On Thu, Dec 01, 2022 at 10:26:09AM -0800, Tim Harvey wrote:
> > > On Sun, Nov 20, 2022 at 3:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > On Fri, Nov 18, 2022 at 11:57:00AM -0800, Tim Harvey wrote:
> > > > > On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > > >
> > > > > > > Andrew,
> > > > > > >
> > > > > > > I completely agree with you but I haven't seen how that can be done
> > > > > > > yet. What support exists for a PHY driver to expose their LED
> > > > > > > configuration to be used that way? Can you point me to an example?
> > > > > >
> > > > > > Nobody has actually worked on this long enough to get code merged. e.g.
> > > > > > https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> > > > > > https://lists.archive.carbon60.com/linux/kernel/3396223
> > > > > >
> > > > > > This is probably the last attempt, which was not too far away from getting merged:
> > > > > > https://patches.linaro.org/project/linux-leds/cover/20220503151633.18760-1-ansuelsmth@gmail.com/
> > > > > >
> > > > > > I seem to NACK a patch like yours every couple of months. If all that
> > > > > > wasted time was actually spent on a common framework, this would of
> > > > > > been solved years ago.
> > > > > >
> > > > > > How important is it to you to control these LEDs? Enough to finish
> > > > > > this code and get it merged?
> > > > > >
> > > > >
> > > > > Andrew,
> > > > >
> > > > > Thanks for the links - the most recent attempt does look promising.
> > > > > For whatever reason I don't have that series in my mail history so
> > > > > it's not clear how I can respond to it.
> > > >
> > > > apt-get install b4
> > > >
> > > > > Ansuel, are you planning on posting a v7 of 'Adds support for PHY LEDs
> > > > > with offload triggers' [1]?
> > > > >
> > > > > I'm not all that familiar with netdev led triggers. Is there a way to
> > > > > configure the default offload blink mode via dt with your series? I
> > > > > didn't quite follow how the offload function/blink-mode gets set.
> > > >
> > > > The idea is that the PHY LEDs are just LEDs in the Linux LED
> > > > framework. So read Documentation/devicetree/bindings/leds/common.yaml.
> > > > The PHY should make use of these standard DT properties, including
> > > > linux,default-trigger.
> > > >
> > > >         Andrew
> > >
> > > Ansuel,
> > >
> > > Are you planning on posting a v7 of 'Adds support for PHY LEDs with
> > > offload triggers' [1]?
> > >
> > > Best Regards,
> > >
> > > Tim
> > > [1] https://patches.linaro.org/project/linux-leds/list/?series=174704
> >
> > I can consider that only if there is a real interest for it and would
> > love some help by the netdev team to actually have a review from the
> > leds team...
> >
> > I tried multiple time to propose it but I never got a review... only a
> > review from an external guy that wanted to follow his idea in every way
> > possible with the only intention of applying his code (sorry to be rude
> > about that but i'm more than happy to recover the work and search for a
> > common solution)
> >
> > So yes this is still in my TODO list but it would help if others can
> > tell me that they want to actually review it. That would put that
> > project on priority and I would recover and push a v7.
> >
> > --
> >         Ansuel
> 
> Ansuel,
> 
> Considering Andrew is nak'ing any phy code to configure LED's until a
> solution using via /sys/class/leds is provided I would say there is
> real interest.
> 
> It seems to me that you got very positive feedback for this last
> series. I would think if you submitted without RFC it would catch more
> eyes as well.
> 

Well yes that's the fun part. netdev really liked the concept and how it
was implemented (and actually also liked the use of a dedicated trigger
instead of bloating the netdev trigger)

But I never got a review from LED team and that result in having the
patch stalled and never merged... But ok I will recover the work and
recheck/retest everything from the start hoping to get more traction
now...

-- 
	Ansuel

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Andrew Lunn]

> But I never got a review from LED team and that result in having the
> patch stalled and never merged... But ok I will recover the work and
> recheck/retest everything from the start hoping to get more traction
> now...

There are a few emails suggesting the LED team has disappeared, and
there are a lot of patches waiting to be merged. I think they were
asking GregKH if he could do something about this.

https://lore.kernel.org/netdev/Y3zQ5ZtAU4NL3hG4@smile.fi.intel.com/

I don't know if anything has changed since then.

Until this is solved, i don't think you will get much from the LED
people. Best you can get is more netdev reviews.

	Andrew

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Alexander Stein]

Hello Ansuel,

Am Donnerstag, 1. Dezember 2022, 19:38:36 CET schrieb Christian Marangi:
> On Thu, Dec 01, 2022 at 10:35:46AM -0800, Tim Harvey wrote:
> > On Thu, Dec 1, 2022 at 10:31 AM Christian Marangi <ansuelsmth@gmail.com> 
wrote:
> > > On Thu, Dec 01, 2022 at 10:26:09AM -0800, Tim Harvey wrote:
> > > > On Sun, Nov 20, 2022 at 3:35 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > > On Fri, Nov 18, 2022 at 11:57:00AM -0800, Tim Harvey wrote:
> > > > > > On Fri, Nov 18, 2022 at 5:11 AM Andrew Lunn <andrew@lunn.ch> 
wrote:
> > > > > > > > Andrew,
> > > > > > > > 
> > > > > > > > I completely agree with you but I haven't seen how that can be
> > > > > > > > done
> > > > > > > > yet. What support exists for a PHY driver to expose their LED
> > > > > > > > configuration to be used that way? Can you point me to an
> > > > > > > > example?
> > > > > > > 
> > > > > > > Nobody has actually worked on this long enough to get code
> > > > > > > merged. e.g.
> > > > > > > https://lore.kernel.org/netdev/20201004095852.GB1104@bug/T/
> > > > > > > https://lists.archive.carbon60.com/linux/kernel/3396223
> > > > > > > 
> > > > > > > This is probably the last attempt, which was not too far away
> > > > > > > from getting merged:
> > > > > > > https://patches.linaro.org/project/linux-leds/cover/20220503151
> > > > > > > 633.18760-1-ansuelsmth@gmail.com/
> > > > > > > 
> > > > > > > I seem to NACK a patch like yours every couple of months. If all
> > > > > > > that
> > > > > > > wasted time was actually spent on a common framework, this would
> > > > > > > of
> > > > > > > been solved years ago.
> > > > > > > 
> > > > > > > How important is it to you to control these LEDs? Enough to
> > > > > > > finish
> > > > > > > this code and get it merged?
> > > > > > 
> > > > > > Andrew,
> > > > > > 
> > > > > > Thanks for the links - the most recent attempt does look
> > > > > > promising.
> > > > > > For whatever reason I don't have that series in my mail history so
> > > > > > it's not clear how I can respond to it.
> > > > > 
> > > > > apt-get install b4
> > > > > 
> > > > > > Ansuel, are you planning on posting a v7 of 'Adds support for PHY
> > > > > > LEDs
> > > > > > with offload triggers' [1]?
> > > > > > 
> > > > > > I'm not all that familiar with netdev led triggers. Is there a way
> > > > > > to
> > > > > > configure the default offload blink mode via dt with your series?
> > > > > > I
> > > > > > didn't quite follow how the offload function/blink-mode gets set.
> > > > > 
> > > > > The idea is that the PHY LEDs are just LEDs in the Linux LED
> > > > > framework. So read
> > > > > Documentation/devicetree/bindings/leds/common.yaml.
> > > > > The PHY should make use of these standard DT properties, including
> > > > > linux,default-trigger.
> > > > > 
> > > > >         Andrew
> > > > 
> > > > Ansuel,
> > > > 
> > > > Are you planning on posting a v7 of 'Adds support for PHY LEDs with
> > > > offload triggers' [1]?
> > > > 
> > > > Best Regards,
> > > > 
> > > > Tim
> > > > [1] https://patches.linaro.org/project/linux-leds/list/?series=174704
> > > 
> > > I can consider that only if there is a real interest for it and would
> > > love some help by the netdev team to actually have a review from the
> > > leds team...
> > > 
> > > I tried multiple time to propose it but I never got a review... only a
> > > review from an external guy that wanted to follow his idea in every way
> > > possible with the only intention of applying his code (sorry to be rude
> > > about that but i'm more than happy to recover the work and search for a
> > > common solution)
> > > 
> > > So yes this is still in my TODO list but it would help if others can
> > > tell me that they want to actually review it. That would put that
> > > project on priority and I would recover and push a v7.
> > > 
> > > --
> > > 
> > >         Ansuel
> > 
> > Ansuel,
> > 
> > Considering Andrew is nak'ing any phy code to configure LED's until a
> > solution using via /sys/class/leds is provided I would say there is
> > real interest.
> > 
> > It seems to me that you got very positive feedback for this last
> > series. I would think if you submitted without RFC it would catch more
> > eyes as well.
> 
> Well yes that's the fun part. netdev really liked the concept and how it
> was implemented (and actually also liked the use of a dedicated trigger
> instead of bloating the netdev trigger)
> 
> But I never got a review from LED team and that result in having the
> patch stalled and never merged... But ok I will recover the work and
> recheck/retest everything from the start hoping to get more traction
> now...

I was just trying to use your RFC patchset from May 2022 for dp83867 as well, 
with some success at least.
I have some comments, fixes and uncertainties. How do you want to progress? 
Resend so I can rebase on that? Anyway, put me on CC.

Best regards,
Alexander

Re: [PATCH 0/3] add dt configuration for dp83867 led modes

[Rasmus Villemoes]

On 07/12/2022 11.40, Alexander Stein wrote:
> Hello Ansuel,
> 
> Am Donnerstag, 1. Dezember 2022, 19:38:36 CET schrieb Christian Marangi:

>>> Considering Andrew is nak'ing any phy code to configure LED's until a
>>> solution using via /sys/class/leds is provided I would say there is
>>> real interest.
>>>
>>> It seems to me that you got very positive feedback for this last
>>> series. I would think if you submitted without RFC it would catch more
>>> eyes as well.
>>
>> Well yes that's the fun part. netdev really liked the concept and how it
>> was implemented (and actually also liked the use of a dedicated trigger
>> instead of bloating the netdev trigger)
>>
>> But I never got a review from LED team and that result in having the
>> patch stalled and never merged... But ok I will recover the work and
>> recheck/retest everything from the start hoping to get more traction
>> now...
> 
> I was just trying to use your RFC patchset from May 2022 for dp83867 as well, 
> with some success at least.
> I have some comments, fixes and uncertainties. How do you want to progress? 
> Resend so I can rebase on that? Anyway, put me on CC.

Yes, I'm also very interested in getting mainline support for hardware
LED triggers, and incidentally one of my peripherals also happens to be
a dp83867. So please also include me on cc if and when you find time to
post a v+1, and I'll help with testing and reviewing.

Rasmus