Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E
@ 2019-08-13 19:11 Matthias Kaehlcke
  2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
  To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit
  Cc: netdev, devicetree, linux-kernel, Douglas Anderson, Matthias Kaehlcke

This series adds a generic binding to configure PHY LEDs through
the device tree, and phylib support for reading the information
from the DT. PHY drivers that support the generic binding should
implement the new hook .config_led.

Enable DT configuration of the RTL8211E LEDs by implementing the
.config_led hook of the driver. Certain registers of the RTL8211E
can only be accessed through a vendor specific extended page
mechanism. Extended pages need to be accessed for the LED
configuration. This series adds helpers to facilitate accessing
extended pages.

Matthias Kaehlcke (4):
  dt-bindings: net: phy: Add subnode for LED configuration
  net: phy: Add support for generic LED configuration through the DT
  net: phy: realtek: Add helpers for accessing RTL8211x extension pages
  net: phy: realtek: Add LED configuration support for RTL8211E

 .../devicetree/bindings/net/ethernet-phy.yaml |  59 ++++++++
 drivers/net/phy/phy_device.c                  |  72 +++++++++
 drivers/net/phy/realtek.c                     | 137 ++++++++++++++++--
 include/linux/phy.h                           |  22 +++
 4 files changed, 275 insertions(+), 15 deletions(-)

-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
  2019-08-13 19:11 [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E Matthias Kaehlcke
@ 2019-08-13 19:11 ` Matthias Kaehlcke
  2019-08-13 19:54   ` Andrew Lunn
                     ` (2 more replies)
  2019-08-13 19:11 ` [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT Matthias Kaehlcke
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
  To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit
  Cc: netdev, devicetree, linux-kernel, Douglas Anderson, Matthias Kaehlcke

The LED behavior of some Ethernet PHYs is configurable. Add an
optional 'leds' subnode with a child node for each LED to be
configured. The binding aims to be compatible with the common
LED binding (see devicetree/bindings/leds/common.txt).

A LED can be configured to be:

- 'on' when a link is active, some PHYs allow configuration for
  certain link speeds
  speeds
- 'off'
- blink on RX/TX activity, some PHYs allow configuration for
  certain link speeds

For the configuration to be effective it needs to be supported by
the hardware and the corresponding PHY driver.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- none

Changes in v5:
- renamed triggers from 'phy_link_<speed>_active' to 'phy-link-<speed>'
- added entries for 'phy-link-<speed>-activity'
- added 'phy-link' and 'phy-link-activity' for triggers with any link
  speed
- added entry for trigger 'none'

Changes in v4:
- patch added to the series
---
 .../devicetree/bindings/net/ethernet-phy.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index f70f18ff821f..98ba320f828b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,50 @@ properties:
       Delay after the reset was deasserted in microseconds. If
       this property is missing the delay will be skipped.
 
+patternProperties:
+  "^leds$":
+    type: object
+    description:
+      Subnode with configuration of the PHY LEDs.
+
+    patternProperties:
+      "^led@[0-9]+$":
+        type: object
+        description:
+          Subnode with the configuration of a single PHY LED.
+
+    properties:
+      reg:
+        description:
+          The ID number of the LED, typically corresponds to a hardware ID.
+        $ref: "/schemas/types.yaml#/definitions/uint32"
+
+      linux,default-trigger:
+        description:
+          This parameter, if present, is a string specifying the trigger
+          assigned to the LED. Supported triggers are:
+            "none" - LED will be solid off
+            "phy-link" - LED will be solid on when a link is active
+            "phy-link-10m" - LED will be solid on when a 10Mb/s link is active
+            "phy-link-100m" - LED will be solid on when a 100Mb/s link is active
+            "phy-link-1g" - LED will be solid on when a 1Gb/s link is active
+            "phy-link-10g" - LED will be solid on when a 10Gb/s link is active
+            "phy-link-activity" - LED will be on when link is active and blink
+                                  off with activity.
+            "phy-link-10m-activity" - LED will be on when 10Mb/s link is active
+                                      and blink off with activity.
+            "phy-link-100m-activity" - LED will be on when 100Mb/s link is
+                                       active and blink off with activity.
+            "phy-link-1g-activity" - LED will be on when 1Gb/s link is active
+                                     and blink off with activity.
+            "phy-link-10g-activity" - LED will be on when 10Gb/s link is active
+                                      and blink off with activity.
+
+        $ref: "/schemas/types.yaml#/definitions/string"
+
+    required:
+      - reg
+
 required:
   - reg
 
@@ -173,5 +217,20 @@ examples:
             reset-gpios = <&gpio1 4 1>;
             reset-assert-us = <1000>;
             reset-deassert-us = <2000>;
+
+            leds {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                led@0 {
+                    reg = <0>;
+                    linux,default-trigger = "phy-link-1g";
+                };
+
+                led@1 {
+                    reg = <1>;
+                    linux,default-trigger = "phy-link-100m-activity";
+                };
+            };
         };
     };
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT
  2019-08-13 19:11 [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E Matthias Kaehlcke
  2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
@ 2019-08-13 19:11 ` Matthias Kaehlcke
  2019-08-13 19:56   ` Andrew Lunn
  2019-08-13 19:11 ` [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages Matthias Kaehlcke
  2019-08-13 19:11 ` [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E Matthias Kaehlcke
  3 siblings, 1 reply; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
  To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit
  Cc: netdev, devicetree, linux-kernel, Douglas Anderson, Matthias Kaehlcke

For PHYs with a device tree node look for LED trigger configuration
using the generic binding, if it exists try to apply it via the new
driver hook .config_led.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- delete unnecessary of_node_put() inside for_each_child_of_node()
  loop
- use continue instead of goto in of_phy_config_leds()
- check return value of ->config_led() and print a warning if !0

Changes in v5:
- add callback to configure a LED to the PHY driver, instead of
  having the driver retrieve the DT data
- use new trigger names
- added support for trigger 'none'
- release DT nodes after use
- renamed 'PHY_LED_LINK_*' to 'PHY_LED_TRIGGER_LINK_*'
- added anonymous struct to struct phy_led_config to track
  'activity' in a separate flag. this could be changed to 'flags' if
  needed/desired.
- updated commit message (previous subject was 'net: phy: Add
  function to retrieve LED configuration from the DT')

Changes in v4:
- patch added to the series
---
 drivers/net/phy/phy_device.c | 72 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 22 +++++++++++
 2 files changed, 94 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b5cb87f3866..80315777ae67 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -29,6 +29,7 @@
 #include <linux/phy_led_triggers.h>
 #include <linux/mdio.h>
 #include <linux/io.h>
+#include <linux/of.h>
 #include <linux/uaccess.h>
 
 MODULE_DESCRIPTION("PHY library");
@@ -1064,6 +1065,75 @@ static int phy_poll_reset(struct phy_device *phydev)
 	return 0;
 }
 
+static void of_phy_config_leds(struct phy_device *phydev)
+{
+	struct device_node *np, *child;
+	struct phy_led_config cfg;
+	const char *trigger;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO) || !phydev->drv->config_led)
+		return;
+
+	np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
+	if (!np)
+		return;
+
+	for_each_child_of_node(np, child) {
+		u32 led;
+
+		if (of_property_read_u32(child, "reg", &led))
+			continue;
+
+		ret = of_property_read_string(child, "linux,default-trigger",
+					      &trigger);
+		if (ret)
+			trigger = "none";
+
+		memset(&cfg, 0, sizeof(cfg));
+
+		if (!strcmp(trigger, "none")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_NONE;
+		} else if (!strcmp(trigger, "phy-link")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK;
+		} else if (!strcmp(trigger, "phy-link-10m")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
+		} else if (!strcmp(trigger, "phy-link-100m")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
+		} else if (!strcmp(trigger, "phy-link-1g")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
+		} else if (!strcmp(trigger, "phy-link-10g")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
+		} else if (!strcmp(trigger, "phy-link-activity")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK;
+			cfg.trigger.activity = true;
+		} else if (!strcmp(trigger, "phy-link-10m-activity")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
+			cfg.trigger.activity = true;
+		} else if (!strcmp(trigger, "phy-link-100m-activity")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
+			cfg.trigger.activity = true;
+		} else if (!strcmp(trigger, "phy-link-1g-activity")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
+			cfg.trigger.activity = true;
+		} else if (!strcmp(trigger, "phy-link-10g-activity")) {
+			cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
+			cfg.trigger.activity = true;
+		} else {
+			phydev_warn(phydev, "trigger '%s' for LED%d is invalid\n",
+				    trigger, led);
+			continue;
+		}
+
+		ret = phydev->drv->config_led(phydev, led, &cfg);
+		if (ret)
+			phydev_warn(phydev, "trigger '%s' for LED%d not supported\n",
+				    trigger, led);
+	}
+
+	of_node_put(np);
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
@@ -1087,6 +1157,8 @@ int phy_init_hw(struct phy_device *phydev)
 	if (phydev->drv->config_init)
 		ret = phydev->drv->config_init(phydev);
 
+	of_phy_config_leds(phydev);
+
 	return ret;
 }
 EXPORT_SYMBOL(phy_init_hw);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 462b90b73f93..3a07390fc5e9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -325,6 +325,24 @@ struct phy_c45_device_ids {
 	u32 device_ids[8];
 };
 
+/* Triggers for PHY LEDs */
+enum phy_led_trigger {
+	PHY_LED_TRIGGER_NONE,
+	PHY_LED_TRIGGER_LINK,
+	PHY_LED_TRIGGER_LINK_10M,
+	PHY_LED_TRIGGER_LINK_100M,
+	PHY_LED_TRIGGER_LINK_1G,
+	PHY_LED_TRIGGER_LINK_10G,
+};
+
+/* Configuration of a single PHY LED */
+struct phy_led_config {
+	struct {
+		enum phy_led_trigger t;
+		bool activity;
+	} trigger;
+};
+
 /* phy_device: An instance of a PHY
  *
  * drv: Pointer to the driver for this PHY instance
@@ -626,6 +644,10 @@ struct phy_driver {
 			    struct ethtool_tunable *tuna,
 			    const void *data);
 	int (*set_loopback)(struct phy_device *dev, bool enable);
+
+	/* Configure a PHY LED */
+	int (*config_led)(struct phy_device *dev, int led,
+			  struct phy_led_config *cfg);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages
  2019-08-13 19:11 [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E Matthias Kaehlcke
  2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
  2019-08-13 19:11 ` [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT Matthias Kaehlcke
@ 2019-08-13 19:11 ` Matthias Kaehlcke
  2019-08-13 20:09   ` Andrew Lunn
  2019-08-13 19:11 ` [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E Matthias Kaehlcke
  3 siblings, 1 reply; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
  To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit
  Cc: netdev, devicetree, linux-kernel, Douglas Anderson, Matthias Kaehlcke

Some RTL8211x PHYs have extension pages, which can be accessed
after selecting a page through a custom method. Add a function to
modify bits in a register of an extension page and a helper for
selecting an ext page. Use rtl8211x_modify_ext_paged() in
rtl8211e_config_init() instead of doing things 'manually'.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- none

Changes in v5:
- renamed 'rtl8211e_<action>_ext_page' to 'rtl8211x_<action>_ext_page'
- updated commit message

Changes in v4:
- don't add constant RTL8211E_EXT_PAGE, it's only used once,
  use a literal instead
- pass 'oldpage' to phy_restore_page() in rtl8211e_select_ext_page(),
  not 'page'
- return 'oldpage' in rtl8211e_select_ext_page()
- use __phy_modify() in rtl8211e_modify_ext_paged() instead of
  reimplementing __phy_modify_changed()
- in rtl8211e_modify_ext_paged() return directly when
  rtl8211e_select_ext_page() fails

Changes in v3:
- use the new function in rtl8211e_config_init() instead of
  doing things 'manually'
- use existing RTL8211E_EXT_PAGE instead of adding a new define
- updated commit message

Changes in v2:
- use phy_select_page() and phy_restore_page(), get rid of
  rtl8211e_restore_page()
- s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
- updated commit message
---
 drivers/net/phy/realtek.c | 47 +++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945eb829..a5b3708dc4d8 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -53,6 +53,36 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl8211x_select_ext_page(struct phy_device *phydev, int page)
+{
+	int ret, oldpage;
+
+	oldpage = phy_select_page(phydev, 7);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, page);
+	if (ret)
+		return phy_restore_page(phydev, oldpage, ret);
+
+	return oldpage;
+}
+
+static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
+				     u32 regnum, u16 mask, u16 set)
+{
+	int ret = 0;
+	int oldpage;
+
+	oldpage = rtl8211x_select_ext_page(phydev, page);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = __phy_modify(phydev, regnum, mask, set);
+
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -184,7 +214,6 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
-	int ret = 0, oldpage;
 	u16 val;
 
 	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
@@ -213,19 +242,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
 	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
 	 * for details).
 	 */
-	oldpage = phy_select_page(phydev, 0x7);
-	if (oldpage < 0)
-		goto err_restore_page;
-
-	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
-	if (ret)
-		goto err_restore_page;
-
-	ret = __phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
-			   val);
-
-err_restore_page:
-	return phy_restore_page(phydev, oldpage, ret);
+	return rtl8211x_modify_ext_paged(phydev, 0xa4, 0x1c,
+					 RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+					 val);
 }
 
 static int rtl8211b_suspend(struct phy_device *phydev)
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-13 19:11 [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E Matthias Kaehlcke
                   ` (2 preceding siblings ...)
  2019-08-13 19:11 ` [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages Matthias Kaehlcke
@ 2019-08-13 19:11 ` Matthias Kaehlcke
  2019-08-13 20:14   ` Andrew Lunn
  2019-08-16 20:13   ` Pavel Machek
  3 siblings, 2 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
  To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit
  Cc: netdev, devicetree, linux-kernel, Douglas Anderson, Matthias Kaehlcke

Add a .config_led hook which is called by the PHY core when
configuration data for a PHY LED is available. Each LED can be
configured to be solid 'off, solid 'on' for certain (or all)
link speeds or to blink on RX/TX activity.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- return -EOPNOTSUPP if trigger is not supported, don't log warning
- don't log errors if MDIO ops fail, this is rare and the phy_device
  will log a warning
- added parentheses around macro argument used in arithmetics to
  avoid possible operator precedence issues
- minor formatting changes

Changes in v5:
- use 'config_leds' driver callback instead of requesting the DT
  configuration
- added support for trigger 'none'
- always disable EEE LED mode when a LED is configured. We have no
  device data struct to keep track of its state, the number of LEDs
  is limited, so the overhead of disabling it multiple times (once for
  each LED that is configured) during initialization is negligible
- print warning when disabling EEE LED mode fails
- updated commit message (previous subject was 'net: phy: realtek:
  configure RTL8211E LEDs')

Changes in v4:
- use the generic PHY LED binding
- keep default/current configuration if none is specified
- added rtl8211e_disable_eee_led_mode()
  - was previously in separate patch, however since we always want to
    disable EEE LED mode when a LED configuration is specified it makes
    sense to just add the function here.
- don't call phy_restore_page() in rtl8211e_config_leds() if
  selection of the extended page failed.
- use phydev_warn() instead of phydev_err() if LED configuration
  fails since we don't bail out
- use hex number to specify page for consistency
- add hex number to comment about ext page 44 to facilitate searching

Changes in v3:
- sanity check led-modes values
- set LACR bits in a more readable way
- use phydev_err() instead of dev_err()
- log an error if LED configuration fails

Changes in v2:
- patch added to the series
---
 drivers/net/phy/realtek.c | 90 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a5b3708dc4d8..2bca3b91d43d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,8 +9,9 @@
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
  */
 #include <linux/bitops.h>
-#include <linux/phy.h>
+#include <linux/bits.h>
 #include <linux/module.h>
+#include <linux/phy.h>
 
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
@@ -26,6 +27,19 @@
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
+/* RTL8211E page 5 */
+#define RTL8211E_EEE_LED_MODE1			0x05
+#define RTL8211E_EEE_LED_MODE2			0x06
+
+/* RTL8211E extension page 44 (0x2c) */
+#define RTL8211E_LACR				0x1a
+#define RLT8211E_LACR_LEDACTCTRL_SHIFT		4
+#define RTL8211E_LCR				0x1c
+
+#define LACR_MASK(led)				BIT(4 + (led))
+#define LCR_MASK(led)				GENMASK(((led) * 4) + 2,\
+							(led) * 4)
+
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
@@ -83,6 +97,79 @@ static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
 	return phy_restore_page(phydev, oldpage, ret);
 }
 
+static void rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
+{
+	int oldpage;
+	int err = 0;
+
+	oldpage = phy_select_page(phydev, 5);
+	if (oldpage < 0)
+		goto out;
+
+	/* write magic values to disable EEE LED mode */
+	err = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
+	if (err)
+		goto out;
+
+	err = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
+
+out:
+	if (err)
+		phydev_warn(phydev, "failed to disable EEE LED mode: %d\n",
+			    err);
+
+	phy_restore_page(phydev, oldpage, err);
+}
+
+static int rtl8211e_config_led(struct phy_device *phydev, int led,
+			       struct phy_led_config *cfg)
+{
+	u16 lacr_bits = 0, lcr_bits = 0;
+	int oldpage, ret;
+
+	switch (cfg->trigger.t) {
+	case PHY_LED_TRIGGER_LINK:
+		lcr_bits = 7 << (led * 4);
+		break;
+
+	case PHY_LED_TRIGGER_LINK_10M:
+		lcr_bits = 1 << (led * 4);
+		break;
+
+	case PHY_LED_TRIGGER_LINK_100M:
+		lcr_bits = 2 << (led * 4);
+		break;
+
+	case PHY_LED_TRIGGER_LINK_1G:
+		lcr_bits |= 4 << (led * 4);
+		break;
+
+	case PHY_LED_TRIGGER_NONE:
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (cfg->trigger.activity)
+		lacr_bits = BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + led);
+
+	rtl8211e_disable_eee_led_mode(phydev);
+
+	oldpage = rtl8211x_select_ext_page(phydev, 0x2c);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = __phy_modify(phydev, RTL8211E_LACR, LACR_MASK(led), lacr_bits);
+	if (ret)
+		goto err;
+
+	ret = __phy_modify(phydev, RTL8211E_LCR, LCR_MASK(led), lcr_bits);
+
+err:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -330,6 +417,7 @@ static struct phy_driver realtek_drvs[] = {
 		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
+		.config_led	= &rtl8211e_config_led,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
-- 
2.23.0.rc1.153.gdeed80330f-goog


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
  2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
@ 2019-08-13 19:54   ` Andrew Lunn
  2019-08-16 20:13   ` Pavel Machek
  2019-08-27 16:12   ` Rob Herring
  2 siblings, 0 replies; 24+ messages in thread
From: Andrew Lunn @ 2019-08-13 19:54 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On Tue, Aug 13, 2019 at 12:11:44PM -0700, Matthias Kaehlcke wrote:
> The LED behavior of some Ethernet PHYs is configurable. Add an
> optional 'leds' subnode with a child node for each LED to be
> configured. The binding aims to be compatible with the common
> LED binding (see devicetree/bindings/leds/common.txt).
> 
> A LED can be configured to be:
> 
> - 'on' when a link is active, some PHYs allow configuration for
>   certain link speeds
>   speeds
> - 'off'
> - blink on RX/TX activity, some PHYs allow configuration for
>   certain link speeds
> 
> For the configuration to be effective it needs to be supported by
> the hardware and the corresponding PHY driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT
  2019-08-13 19:11 ` [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT Matthias Kaehlcke
@ 2019-08-13 19:56   ` Andrew Lunn
  0 siblings, 0 replies; 24+ messages in thread
From: Andrew Lunn @ 2019-08-13 19:56 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On Tue, Aug 13, 2019 at 12:11:45PM -0700, Matthias Kaehlcke wrote:
> For PHYs with a device tree node look for LED trigger configuration
> using the generic binding, if it exists try to apply it via the new
> driver hook .config_led.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages
  2019-08-13 19:11 ` [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages Matthias Kaehlcke
@ 2019-08-13 20:09   ` Andrew Lunn
  0 siblings, 0 replies; 24+ messages in thread
From: Andrew Lunn @ 2019-08-13 20:09 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On Tue, Aug 13, 2019 at 12:11:46PM -0700, Matthias Kaehlcke wrote:
> Some RTL8211x PHYs have extension pages, which can be accessed
> after selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a helper for
> selecting an ext page. Use rtl8211x_modify_ext_paged() in
> rtl8211e_config_init() instead of doing things 'manually'.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-13 19:11 ` [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E Matthias Kaehlcke
@ 2019-08-13 20:14   ` Andrew Lunn
  2019-08-13 20:46     ` Matthias Kaehlcke
  2019-08-16 20:13   ` Pavel Machek
  1 sibling, 1 reply; 24+ messages in thread
From: Andrew Lunn @ 2019-08-13 20:14 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

> +static int rtl8211e_config_led(struct phy_device *phydev, int led,
> +			       struct phy_led_config *cfg)
> +{
> +	u16 lacr_bits = 0, lcr_bits = 0;
> +	int oldpage, ret;
> +

You should probably check that led is 0 or 1. 

Otherwise this looks good.

	  Andrew

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-13 20:14   ` Andrew Lunn
@ 2019-08-13 20:46     ` Matthias Kaehlcke
  0 siblings, 0 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-13 20:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On Tue, Aug 13, 2019 at 10:14:11PM +0200, Andrew Lunn wrote:
> > +static int rtl8211e_config_led(struct phy_device *phydev, int led,
> > +			       struct phy_led_config *cfg)
> > +{
> > +	u16 lacr_bits = 0, lcr_bits = 0;
> > +	int oldpage, ret;
> > +
> 
> You should probably check that led is 0 or 1. 

Actually this PHY has up to 3 LEDs, but yes, good point to validate
the parameter. I'll wait a day or two for if there is other feedback
and send a new version with the check.

> Otherwise this looks good.

Thanks for the reviews!

Matthias

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
  2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
  2019-08-13 19:54   ` Andrew Lunn
@ 2019-08-16 20:13   ` Pavel Machek
  2019-08-16 22:04     ` Matthias Kaehlcke
  2019-08-27 16:12   ` Rob Herring
  2 siblings, 1 reply; 24+ messages in thread
From: Pavel Machek @ 2019-08-16 20:13 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit, netdev, devicetree,
	linux-kernel, Douglas Anderson

Hi!

Please Cc led mailing lists on led issues.


On Tue 2019-08-13 12:11:44, Matthias Kaehlcke wrote:
> The LED behavior of some Ethernet PHYs is configurable. Add an
> optional 'leds' subnode with a child node for each LED to be
> configured. The binding aims to be compatible with the common
> LED binding (see devicetree/bindings/leds/common.txt).
> 
> A LED can be configured to be:
> 
> - 'on' when a link is active, some PHYs allow configuration for
>   certain link speeds
>   speeds
> - 'off'
> - blink on RX/TX activity, some PHYs allow configuration for
>   certain link speeds
> 
> For the configuration to be effective it needs to be supported by
> the hardware and the corresponding PHY driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

> @@ -173,5 +217,20 @@ examples:
>              reset-gpios = <&gpio1 4 1>;
>              reset-assert-us = <1000>;
>              reset-deassert-us = <2000>;
> +
> +            leds {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                led@0 {
> +                    reg = <0>;
> +                    linux,default-trigger = "phy-link-1g";
> +                };

Because this affects us.

Is the LED software controllable? Or can it do limited subset of triggers you listed?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-13 19:11 ` [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E Matthias Kaehlcke
  2019-08-13 20:14   ` Andrew Lunn
@ 2019-08-16 20:13   ` Pavel Machek
  2019-08-16 21:27     ` Matthias Kaehlcke
  1 sibling, 1 reply; 24+ messages in thread
From: Pavel Machek @ 2019-08-16 20:13 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit, netdev, devicetree,
	linux-kernel, Douglas Anderson

On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
> Add a .config_led hook which is called by the PHY core when
> configuration data for a PHY LED is available. Each LED can be
> configured to be solid 'off, solid 'on' for certain (or all)
> link speeds or to blink on RX/TX activity.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

THis really needs to go through the LED subsystem, and use the same userland
interfaces as the rest of the system.

Sorry.

NAK.

									Pavel


> ---
> Changes in v6:
> - return -EOPNOTSUPP if trigger is not supported, don't log warning
> - don't log errors if MDIO ops fail, this is rare and the phy_device
>   will log a warning
> - added parentheses around macro argument used in arithmetics to
>   avoid possible operator precedence issues
> - minor formatting changes
> 
> Changes in v5:
> - use 'config_leds' driver callback instead of requesting the DT
>   configuration
> - added support for trigger 'none'
> - always disable EEE LED mode when a LED is configured. We have no
>   device data struct to keep track of its state, the number of LEDs
>   is limited, so the overhead of disabling it multiple times (once for
>   each LED that is configured) during initialization is negligible
> - print warning when disabling EEE LED mode fails
> - updated commit message (previous subject was 'net: phy: realtek:
>   configure RTL8211E LEDs')
> 
> Changes in v4:
> - use the generic PHY LED binding
> - keep default/current configuration if none is specified
> - added rtl8211e_disable_eee_led_mode()
>   - was previously in separate patch, however since we always want to
>     disable EEE LED mode when a LED configuration is specified it makes
>     sense to just add the function here.
> - don't call phy_restore_page() in rtl8211e_config_leds() if
>   selection of the extended page failed.
> - use phydev_warn() instead of phydev_err() if LED configuration
>   fails since we don't bail out
> - use hex number to specify page for consistency
> - add hex number to comment about ext page 44 to facilitate searching
> 
> Changes in v3:
> - sanity check led-modes values
> - set LACR bits in a more readable way
> - use phydev_err() instead of dev_err()
> - log an error if LED configuration fails
> 
> Changes in v2:
> - patch added to the series
> ---
>  drivers/net/phy/realtek.c | 90 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a5b3708dc4d8..2bca3b91d43d 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,8 +9,9 @@
>   * Copyright (c) 2004 Freescale Semiconductor, Inc.
>   */
>  #include <linux/bitops.h>
> -#include <linux/phy.h>
> +#include <linux/bits.h>
>  #include <linux/module.h>
> +#include <linux/phy.h>
>  
>  #define RTL821x_PHYSR				0x11
>  #define RTL821x_PHYSR_DUPLEX			BIT(13)
> @@ -26,6 +27,19 @@
>  #define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> +/* RTL8211E page 5 */
> +#define RTL8211E_EEE_LED_MODE1			0x05
> +#define RTL8211E_EEE_LED_MODE2			0x06
> +
> +/* RTL8211E extension page 44 (0x2c) */
> +#define RTL8211E_LACR				0x1a
> +#define RLT8211E_LACR_LEDACTCTRL_SHIFT		4
> +#define RTL8211E_LCR				0x1c
> +
> +#define LACR_MASK(led)				BIT(4 + (led))
> +#define LCR_MASK(led)				GENMASK(((led) * 4) + 2,\
> +							(led) * 4)
> +
>  #define RTL8211F_INSR				0x1d
>  
>  #define RTL8211F_TX_DELAY			BIT(8)
> @@ -83,6 +97,79 @@ static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
>  	return phy_restore_page(phydev, oldpage, ret);
>  }
>  
> +static void rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
> +{
> +	int oldpage;
> +	int err = 0;
> +
> +	oldpage = phy_select_page(phydev, 5);
> +	if (oldpage < 0)
> +		goto out;
> +
> +	/* write magic values to disable EEE LED mode */
> +	err = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
> +	if (err)
> +		goto out;
> +
> +	err = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
> +
> +out:
> +	if (err)
> +		phydev_warn(phydev, "failed to disable EEE LED mode: %d\n",
> +			    err);
> +
> +	phy_restore_page(phydev, oldpage, err);
> +}
> +
> +static int rtl8211e_config_led(struct phy_device *phydev, int led,
> +			       struct phy_led_config *cfg)
> +{
> +	u16 lacr_bits = 0, lcr_bits = 0;
> +	int oldpage, ret;
> +
> +	switch (cfg->trigger.t) {
> +	case PHY_LED_TRIGGER_LINK:
> +		lcr_bits = 7 << (led * 4);
> +		break;
> +
> +	case PHY_LED_TRIGGER_LINK_10M:
> +		lcr_bits = 1 << (led * 4);
> +		break;
> +
> +	case PHY_LED_TRIGGER_LINK_100M:
> +		lcr_bits = 2 << (led * 4);
> +		break;
> +
> +	case PHY_LED_TRIGGER_LINK_1G:
> +		lcr_bits |= 4 << (led * 4);
> +		break;
> +
> +	case PHY_LED_TRIGGER_NONE:
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (cfg->trigger.activity)
> +		lacr_bits = BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + led);
> +
> +	rtl8211e_disable_eee_led_mode(phydev);
> +
> +	oldpage = rtl8211x_select_ext_page(phydev, 0x2c);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = __phy_modify(phydev, RTL8211E_LACR, LACR_MASK(led), lacr_bits);
> +	if (ret)
> +		goto err;
> +
> +	ret = __phy_modify(phydev, RTL8211E_LCR, LCR_MASK(led), lcr_bits);
> +
> +err:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl8201_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> @@ -330,6 +417,7 @@ static struct phy_driver realtek_drvs[] = {
>  		.config_init	= &rtl8211e_config_init,
>  		.ack_interrupt	= &rtl821x_ack_interrupt,
>  		.config_intr	= &rtl8211e_config_intr,
> +		.config_led	= &rtl8211e_config_led,
>  		.suspend	= genphy_suspend,
>  		.resume		= genphy_resume,
>  		.read_page	= rtl821x_read_page,
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-16 20:13   ` Pavel Machek
@ 2019-08-16 21:27     ` Matthias Kaehlcke
  2019-08-16 22:12       ` Florian Fainelli
  2019-08-17 14:05       ` Pavel Machek
  0 siblings, 2 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-16 21:27 UTC (permalink / raw)
  To: Pavel Machek
  Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit, netdev, devicetree,
	linux-kernel, Douglas Anderson

On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
> On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
> > Add a .config_led hook which is called by the PHY core when
> > configuration data for a PHY LED is available. Each LED can be
> > configured to be solid 'off, solid 'on' for certain (or all)
> > link speeds or to blink on RX/TX activity.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> THis really needs to go through the LED subsystem,

Sorry, I used what get_maintainers.pl threw at me, I should have
manually cc-ed the LED list.

> and use the same userland interfaces as the rest of the system.

With the PHY maintainers we discussed to define a binding that is
compatible with that of the LED one, to have the option to integrate
it with the LED subsystem later. The integration itself is beyond the
scope of this patchset.

The PHY LED configuration is a low priority for the project I'm
working on. I wanted to make an attempt to upstream it and spent
already significantly more time on it than planned, if integration
with the LED framework now is a requirement please consider this
series abandonded.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
  2019-08-16 20:13   ` Pavel Machek
@ 2019-08-16 22:04     ` Matthias Kaehlcke
  2019-08-19  0:29       ` Andrew Lunn
  0 siblings, 1 reply; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-16 22:04 UTC (permalink / raw)
  To: Pavel Machek
  Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit, netdev, devicetree,
	linux-kernel, Douglas Anderson

On Fri, Aug 16, 2019 at 10:13:38PM +0200, Pavel Machek wrote:
> Hi!
> 
> Please Cc led mailing lists on led issues.

sorry for missing this

> On Tue 2019-08-13 12:11:44, Matthias Kaehlcke wrote:
> > The LED behavior of some Ethernet PHYs is configurable. Add an
> > optional 'leds' subnode with a child node for each LED to be
> > configured. The binding aims to be compatible with the common
> > LED binding (see devicetree/bindings/leds/common.txt).
> > 
> > A LED can be configured to be:
> > 
> > - 'on' when a link is active, some PHYs allow configuration for
> >   certain link speeds
> >   speeds
> > - 'off'
> > - blink on RX/TX activity, some PHYs allow configuration for
> >   certain link speeds
> > 
> > For the configuration to be effective it needs to be supported by
> > the hardware and the corresponding PHY driver.
> > 
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> > @@ -173,5 +217,20 @@ examples:
> >              reset-gpios = <&gpio1 4 1>;
> >              reset-assert-us = <1000>;
> >              reset-deassert-us = <2000>;
> > +
> > +            leds {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                led@0 {
> > +                    reg = <0>;
> > +                    linux,default-trigger = "phy-link-1g";
> > +                };
> 
> Because this affects us.
> 
> Is the LED software controllable?

it might be for certain PHYs, integration with the LED framework is
not part of this series.

> Or can it do limited subset of triggers you listed?

it depends on the PHY. The one in this series (RTL8211E) only supports
a limited subset of the listed triggers.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-16 21:27     ` Matthias Kaehlcke
@ 2019-08-16 22:12       ` Florian Fainelli
  2019-08-16 22:39         ` Doug Anderson
  2019-08-16 22:40         ` Matthias Kaehlcke
  2019-08-17 14:05       ` Pavel Machek
  1 sibling, 2 replies; 24+ messages in thread
From: Florian Fainelli @ 2019-08-16 22:12 UTC (permalink / raw)
  To: Matthias Kaehlcke, Pavel Machek
  Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On 8/16/19 2:27 PM, Matthias Kaehlcke wrote:
> On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
>> On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
>>> Add a .config_led hook which is called by the PHY core when
>>> configuration data for a PHY LED is available. Each LED can be
>>> configured to be solid 'off, solid 'on' for certain (or all)
>>> link speeds or to blink on RX/TX activity.
>>>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>
>> THis really needs to go through the LED subsystem,
> 
> Sorry, I used what get_maintainers.pl threw at me, I should have
> manually cc-ed the LED list.
> 
>> and use the same userland interfaces as the rest of the system.
> 
> With the PHY maintainers we discussed to define a binding that is
> compatible with that of the LED one, to have the option to integrate
> it with the LED subsystem later. The integration itself is beyond the
> scope of this patchset.
> 
> The PHY LED configuration is a low priority for the project I'm
> working on. I wanted to make an attempt to upstream it and spent
> already significantly more time on it than planned, if integration
> with the LED framework now is a requirement please consider this
> series abandonded.

While I have an appreciation for how hard it can be to work in a
corporate environment while doing upstream first and working with
virtually unbounded goals (in time or scope) due to maintainers and
reviewers, that kind of statement can hinder your ability to establish
trust with peers in the community as it can be read as take it or leave it.

The LED subsystem integration can definitively come in later from my 2
cents perspective and this patch series as it stands is valuable and
avoids inventing new bindings.
-- 
Florian

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-16 22:12       ` Florian Fainelli
@ 2019-08-16 22:39         ` Doug Anderson
  2019-08-23 19:58           ` Florian Fainelli
  2019-08-16 22:40         ` Matthias Kaehlcke
  1 sibling, 1 reply; 24+ messages in thread
From: Doug Anderson @ 2019-08-16 22:39 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Matthias Kaehlcke, Pavel Machek, David S . Miller, Rob Herring,
	Mark Rutland, Andrew Lunn, Heiner Kallweit, netdev, devicetree,
	LKML

Hi,

On Fri, Aug 16, 2019 at 3:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 8/16/19 2:27 PM, Matthias Kaehlcke wrote:
> > On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
> >> On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
> >>> Add a .config_led hook which is called by the PHY core when
> >>> configuration data for a PHY LED is available. Each LED can be
> >>> configured to be solid 'off, solid 'on' for certain (or all)
> >>> link speeds or to blink on RX/TX activity.
> >>>
> >>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>
> >> THis really needs to go through the LED subsystem,
> >
> > Sorry, I used what get_maintainers.pl threw at me, I should have
> > manually cc-ed the LED list.
> >
> >> and use the same userland interfaces as the rest of the system.
> >
> > With the PHY maintainers we discussed to define a binding that is
> > compatible with that of the LED one, to have the option to integrate
> > it with the LED subsystem later. The integration itself is beyond the
> > scope of this patchset.
> >
> > The PHY LED configuration is a low priority for the project I'm
> > working on. I wanted to make an attempt to upstream it and spent
> > already significantly more time on it than planned, if integration
> > with the LED framework now is a requirement please consider this
> > series abandonded.
>
> While I have an appreciation for how hard it can be to work in a
> corporate environment while doing upstream first and working with
> virtually unbounded goals (in time or scope) due to maintainers and
> reviewers, that kind of statement can hinder your ability to establish
> trust with peers in the community as it can be read as take it or leave it.

You think so?  I feel like Matthias is simply expressing the reality
of the situation here and I'd rather see a statement like this posted
than the series just silently dropped.  Communication is good.

In general on Chrome OS we don't spent lots of time tweaking with
Ethernet and even less time tweaking with Ethernet on ARM boards where
you might need a binding like this, so it's pretty hard to justify up
the management chain spending massive amounts of resources on it.  In
this case we have two existing ARM boards which we're trying to uprev
from 3.14 to 4.19 which were tweaking the Ethernet driver in some
downstream code.  We thought it would be nice to try to come up with a
solution that could land upstream, which is usually what we try to do
in these cases.

Normally if there is some major architecture needed that can't fit in
the scope of a project, we would do a downstream solution for the
project and then fork off the task (maybe by a different Engineer or a
contractor) to get a solution that can land upstream.  ...but in this
case it seems hard to justify because it's unlikely we would need it
again anytime remotely soon.

So I guess the alternatives to what Matthias did would have been:

A) Don't even try to upstream.  Seems worse.  At least this way
there's something a future person can start from and the discussion is
rolling.

B) Keep spending tons of time on something even though management
doesn't want him to.  Seems worse.

C) Spend his nights and weekends working on this.  Seems worse.

D) Silently stop working on it without saying "I'm going to stop".  Seems worse.

...unless you have a brilliant "E)" I think what Matthias did here is
exactly right.

BTW: I'm giving a talk on this topic next week at ELC [1].  If you're
going to be there feel free to attend.  ...or just read the slides if
not.


> The LED subsystem integration can definitively come in later from my 2
> cents perspective and this patch series as it stands is valuable and
> avoids inventing new bindings.

If something like this series can land and someone can later try to
make the situation better then I think that would be awesome.  I don't
think Matthias is saying "I won't spin" or "I won't take feedback".
He's just expressing that he can't keep working on this indefinitely.



[1] https://ossna19.sched.com/event/PVSV/how-chrome-os-works-with-upstream-linux-douglas-anderson-google

-Doug

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-16 22:12       ` Florian Fainelli
  2019-08-16 22:39         ` Doug Anderson
@ 2019-08-16 22:40         ` Matthias Kaehlcke
  1 sibling, 0 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-16 22:40 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Pavel Machek, David S . Miller, Rob Herring, Mark Rutland,
	Andrew Lunn, Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On Fri, Aug 16, 2019 at 03:12:47PM -0700, Florian Fainelli wrote:
> On 8/16/19 2:27 PM, Matthias Kaehlcke wrote:
> > On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
> >> On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
> >>> Add a .config_led hook which is called by the PHY core when
> >>> configuration data for a PHY LED is available. Each LED can be
> >>> configured to be solid 'off, solid 'on' for certain (or all)
> >>> link speeds or to blink on RX/TX activity.
> >>>
> >>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>
> >> THis really needs to go through the LED subsystem,
> > 
> > Sorry, I used what get_maintainers.pl threw at me, I should have
> > manually cc-ed the LED list.
> > 
> >> and use the same userland interfaces as the rest of the system.
> > 
> > With the PHY maintainers we discussed to define a binding that is
> > compatible with that of the LED one, to have the option to integrate
> > it with the LED subsystem later. The integration itself is beyond the
> > scope of this patchset.
> > 
> > The PHY LED configuration is a low priority for the project I'm
> > working on. I wanted to make an attempt to upstream it and spent
> > already significantly more time on it than planned, if integration
> > with the LED framework now is a requirement please consider this
> > series abandonded.
> 
> While I have an appreciation for how hard it can be to work in a
> corporate environment while doing upstream first and working with
> virtually unbounded goals (in time or scope) due to maintainers and
> reviewers, that kind of statement can hinder your ability to establish
> trust with peers in the community as it can be read as take it or leave it.

I'm really just stating the reality here. We strongly prefer landing
patches upstream over doing custom hacks, and depending on the
priority of a given feature/sub-system and impact on schedule we can
allocate more time on it or less. In some cases/at some point a
downstream patch is just good enough.

I definitely don't intend to get a patchset landed if it isn't deemed
ready or suitable at all. In this case I just can't justify to spend
significantly more time on it. IMO it is better to be clear on this,
not to pressure maintainers to take a patch, but so people know what
to expect. This information can also help if someone comes across this
patchset in the future and wonders about its status.

btw, a birdie told me there will be a talk next week at ELC in San
Diego on how Chrome OS works with upstream, discussing pros and
cons for both the project and upstream. For those who are intersted
in the topic but can't make it to the conference, the slides are
already online and IMO have good information:
https://static.sched.com/hosted_files/ossna19/9c/ELC19_ChromeOSAndUpstream.pdf

Cheers

Matthias

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-16 21:27     ` Matthias Kaehlcke
  2019-08-16 22:12       ` Florian Fainelli
@ 2019-08-17 14:05       ` Pavel Machek
  2019-08-19  0:37         ` Andrew Lunn
  1 sibling, 1 reply; 24+ messages in thread
From: Pavel Machek @ 2019-08-17 14:05 UTC (permalink / raw)
  To: Matthias Kaehlcke, jacek.anaszewski, linux-leds, dmurphy
  Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
	Florian Fainelli, Heiner Kallweit, netdev, devicetree,
	linux-kernel, Douglas Anderson

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

On Fri 2019-08-16 14:27:28, Matthias Kaehlcke wrote:
> On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
> > On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
> > > Add a .config_led hook which is called by the PHY core when
> > > configuration data for a PHY LED is available. Each LED can be
> > > configured to be solid 'off, solid 'on' for certain (or all)
> > > link speeds or to blink on RX/TX activity.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > 
> > THis really needs to go through the LED subsystem,
> 
> Sorry, I used what get_maintainers.pl threw at me, I should have
> manually cc-ed the LED list.
> 
> > and use the same userland interfaces as the rest of the system.
> 
> With the PHY maintainers we discussed to define a binding that is
> compatible with that of the LED one, to have the option to integrate
> it with the LED subsystem later. The integration itself is beyond the
> scope of this patchset.

Yes, I believe the integration is neccessary. Using same binding is
neccessary for that, but not sufficient. For example, we need
compatible trigger names, too.

So... I'd really like to see proper integration is possible before we
merge this.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
  2019-08-16 22:04     ` Matthias Kaehlcke
@ 2019-08-19  0:29       ` Andrew Lunn
  0 siblings, 0 replies; 24+ messages in thread
From: Andrew Lunn @ 2019-08-19  0:29 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Pavel Machek, David S . Miller, Rob Herring, Mark Rutland,
	Florian Fainelli, Heiner Kallweit, netdev, devicetree,
	linux-kernel, Douglas Anderson

On Fri, Aug 16, 2019 at 03:04:11PM -0700, Matthias Kaehlcke wrote:
> On Fri, Aug 16, 2019 at 10:13:38PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > Please Cc led mailing lists on led issues.
> 
> sorry for missing this
> 
> > On Tue 2019-08-13 12:11:44, Matthias Kaehlcke wrote:
> > > The LED behavior of some Ethernet PHYs is configurable. Add an
> > > optional 'leds' subnode with a child node for each LED to be
> > > configured. The binding aims to be compatible with the common
> > > LED binding (see devicetree/bindings/leds/common.txt).
> > > 
> > > A LED can be configured to be:
> > > 
> > > - 'on' when a link is active, some PHYs allow configuration for
> > >   certain link speeds
> > >   speeds
> > > - 'off'
> > > - blink on RX/TX activity, some PHYs allow configuration for
> > >   certain link speeds
> > > 
> > > For the configuration to be effective it needs to be supported by
> > > the hardware and the corresponding PHY driver.
> > > 
> > > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > 
> > > @@ -173,5 +217,20 @@ examples:
> > >              reset-gpios = <&gpio1 4 1>;
> > >              reset-assert-us = <1000>;
> > >              reset-deassert-us = <2000>;
> > > +
> > > +            leds {
> > > +                #address-cells = <1>;
> > > +                #size-cells = <0>;
> > > +
> > > +                led@0 {
> > > +                    reg = <0>;
> > > +                    linux,default-trigger = "phy-link-1g";
> > > +                };
> > 
> > Because this affects us.
> > 
> > Is the LED software controllable?
> 
> it might be for certain PHYs, integration with the LED framework is
> not part of this series.
> 
> > Or can it do limited subset of triggers you listed?
> 
> it depends on the PHY. The one in this series (RTL8211E) only supports
> a limited subset of the listed triggers.

Hi Pavel

At the moment, there is no integration with the LED
subsystem. However, i would like to be prepared for it in the
future. It will also require some extensions to the LED subsystem.
These triggers are hardware triggers. We would need to add support for
LED trigger offload to the hardware, not have Linux blink the LED in
software. Plus we need per LED triggers, not only global triggers.
Most Ethernet PHYs also allow on/off state to be set, so they could be
software controllable, and support the generic triggers Linux has.

It has been on my mind to do this for a while, but i've never had the
time. It should also be applicable to other subsystems which have
hardware to blink LEDs. Some serial ports can control LEDs for Rx/Tx
and flow control. Maybe disk/RAID controllers are configuration how
they blink there LEDs?

      Andrew

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-17 14:05       ` Pavel Machek
@ 2019-08-19  0:37         ` Andrew Lunn
  2019-10-07 11:02           ` Pavel Machek
  0 siblings, 1 reply; 24+ messages in thread
From: Andrew Lunn @ 2019-08-19  0:37 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Matthias Kaehlcke, jacek.anaszewski, linux-leds, dmurphy,
	David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

> Yes, I believe the integration is neccessary. Using same binding is
> neccessary for that, but not sufficient. For example, we need
> compatible trigger names, too.

Hi Pavel

Please could you explain what you mean by compatible trigger names?

> So... I'd really like to see proper integration is possible before we
> merge this.

Please let me turn that around. What do you see as being impossible at
the moment? What do we need to convince you about?

    Thanks
	Andrew

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-16 22:39         ` Doug Anderson
@ 2019-08-23 19:58           ` Florian Fainelli
  2019-08-26 18:40             ` Matthias Kaehlcke
  0 siblings, 1 reply; 24+ messages in thread
From: Florian Fainelli @ 2019-08-23 19:58 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Matthias Kaehlcke, Pavel Machek, David S . Miller, Rob Herring,
	Mark Rutland, Andrew Lunn, Heiner Kallweit, netdev, devicetree,
	LKML

On 8/16/19 3:39 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Aug 16, 2019 at 3:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 8/16/19 2:27 PM, Matthias Kaehlcke wrote:
>>> On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
>>>> On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
>>>>> Add a .config_led hook which is called by the PHY core when
>>>>> configuration data for a PHY LED is available. Each LED can be
>>>>> configured to be solid 'off, solid 'on' for certain (or all)
>>>>> link speeds or to blink on RX/TX activity.
>>>>>
>>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>>>
>>>> THis really needs to go through the LED subsystem,
>>>
>>> Sorry, I used what get_maintainers.pl threw at me, I should have
>>> manually cc-ed the LED list.
>>>
>>>> and use the same userland interfaces as the rest of the system.
>>>
>>> With the PHY maintainers we discussed to define a binding that is
>>> compatible with that of the LED one, to have the option to integrate
>>> it with the LED subsystem later. The integration itself is beyond the
>>> scope of this patchset.
>>>
>>> The PHY LED configuration is a low priority for the project I'm
>>> working on. I wanted to make an attempt to upstream it and spent
>>> already significantly more time on it than planned, if integration
>>> with the LED framework now is a requirement please consider this
>>> series abandonded.
>>
>> While I have an appreciation for how hard it can be to work in a
>> corporate environment while doing upstream first and working with
>> virtually unbounded goals (in time or scope) due to maintainers and
>> reviewers, that kind of statement can hinder your ability to establish
>> trust with peers in the community as it can be read as take it or leave it.
> 
> You think so?  I feel like Matthias is simply expressing the reality
> of the situation here and I'd rather see a statement like this posted
> than the series just silently dropped.  Communication is good.
> 
> In general on Chrome OS we don't spent lots of time tweaking with
> Ethernet and even less time tweaking with Ethernet on ARM boards where
> you might need a binding like this, so it's pretty hard to justify up
> the management chain spending massive amounts of resources on it.  In
> this case we have two existing ARM boards which we're trying to uprev
> from 3.14 to 4.19 which were tweaking the Ethernet driver in some
> downstream code.  We thought it would be nice to try to come up with a
> solution that could land upstream, which is usually what we try to do
> in these cases.
> 
> Normally if there is some major architecture needed that can't fit in
> the scope of a project, we would do a downstream solution for the
> project and then fork off the task (maybe by a different Engineer or a
> contractor) to get a solution that can land upstream.  ...but in this
> case it seems hard to justify because it's unlikely we would need it
> again anytime remotely soon.
> 
> So I guess the alternatives to what Matthias did would have been:
> 
> A) Don't even try to upstream.  Seems worse.  At least this way
> there's something a future person can start from and the discussion is
> rolling.
> 
> B) Keep spending tons of time on something even though management
> doesn't want him to.  Seems worse.
> 
> C) Spend his nights and weekends working on this.  Seems worse.
> 
> D) Silently stop working on it without saying "I'm going to stop".  Seems worse.
> 
> ...unless you have a brilliant "E)" I think what Matthias did here is
> exactly right.

I must apologize for making that statement since it was not fair to
Matthias, and he has been clear about how much time he can spend on that
specific, please accept my apologies for that.

Having had many recent encounters with various people not driving
projects to completion lately (not specifically within Linux), it looks
like I am overly sensitive about flagging words and patch status that
may fall within that lexicon. The choice of word is what triggered me.

> 
> BTW: I'm giving a talk on this topic next week at ELC [1].  If you're
> going to be there feel free to attend.  ...or just read the slides if
> not.

I wish I could be there but that was not possible this year.

> 
> 
>> The LED subsystem integration can definitively come in later from my 2
>> cents perspective and this patch series as it stands is valuable and
>> avoids inventing new bindings.
> 
> If something like this series can land and someone can later try to
> make the situation better then I think that would be awesome.  I don't
> think Matthias is saying "I won't spin" or "I won't take feedback".
> He's just expressing that he can't keep working on this indefinitely.
> 
> 
> 
> [1] https://ossna19.sched.com/event/PVSV/how-chrome-os-works-with-upstream-linux-douglas-anderson-google
> 
> -Doug
> 


-- 
Florian

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-23 19:58           ` Florian Fainelli
@ 2019-08-26 18:40             ` Matthias Kaehlcke
  0 siblings, 0 replies; 24+ messages in thread
From: Matthias Kaehlcke @ 2019-08-26 18:40 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Doug Anderson, Pavel Machek, David S . Miller, Rob Herring,
	Mark Rutland, Andrew Lunn, Heiner Kallweit, netdev, devicetree,
	LKML

On Fri, Aug 23, 2019 at 12:58:09PM -0700, Florian Fainelli wrote:
> On 8/16/19 3:39 PM, Doug Anderson wrote:
> > Hi,
> > 
> > On Fri, Aug 16, 2019 at 3:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> On 8/16/19 2:27 PM, Matthias Kaehlcke wrote:
> >>> On Fri, Aug 16, 2019 at 10:13:42PM +0200, Pavel Machek wrote:
> >>>> On Tue 2019-08-13 12:11:47, Matthias Kaehlcke wrote:
> >>>>> Add a .config_led hook which is called by the PHY core when
> >>>>> configuration data for a PHY LED is available. Each LED can be
> >>>>> configured to be solid 'off, solid 'on' for certain (or all)
> >>>>> link speeds or to blink on RX/TX activity.
> >>>>>
> >>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>>>
> >>>> THis really needs to go through the LED subsystem,
> >>>
> >>> Sorry, I used what get_maintainers.pl threw at me, I should have
> >>> manually cc-ed the LED list.
> >>>
> >>>> and use the same userland interfaces as the rest of the system.
> >>>
> >>> With the PHY maintainers we discussed to define a binding that is
> >>> compatible with that of the LED one, to have the option to integrate
> >>> it with the LED subsystem later. The integration itself is beyond the
> >>> scope of this patchset.
> >>>
> >>> The PHY LED configuration is a low priority for the project I'm
> >>> working on. I wanted to make an attempt to upstream it and spent
> >>> already significantly more time on it than planned, if integration
> >>> with the LED framework now is a requirement please consider this
> >>> series abandonded.
> >>
> >> While I have an appreciation for how hard it can be to work in a
> >> corporate environment while doing upstream first and working with
> >> virtually unbounded goals (in time or scope) due to maintainers and
> >> reviewers, that kind of statement can hinder your ability to establish
> >> trust with peers in the community as it can be read as take it or leave it.
> > 
> > You think so?  I feel like Matthias is simply expressing the reality
> > of the situation here and I'd rather see a statement like this posted
> > than the series just silently dropped.  Communication is good.
> > 
> > In general on Chrome OS we don't spent lots of time tweaking with
> > Ethernet and even less time tweaking with Ethernet on ARM boards where
> > you might need a binding like this, so it's pretty hard to justify up
> > the management chain spending massive amounts of resources on it.  In
> > this case we have two existing ARM boards which we're trying to uprev
> > from 3.14 to 4.19 which were tweaking the Ethernet driver in some
> > downstream code.  We thought it would be nice to try to come up with a
> > solution that could land upstream, which is usually what we try to do
> > in these cases.
> > 
> > Normally if there is some major architecture needed that can't fit in
> > the scope of a project, we would do a downstream solution for the
> > project and then fork off the task (maybe by a different Engineer or a
> > contractor) to get a solution that can land upstream.  ...but in this
> > case it seems hard to justify because it's unlikely we would need it
> > again anytime remotely soon.
> > 
> > So I guess the alternatives to what Matthias did would have been:
> > 
> > A) Don't even try to upstream.  Seems worse.  At least this way
> > there's something a future person can start from and the discussion is
> > rolling.
> > 
> > B) Keep spending tons of time on something even though management
> > doesn't want him to.  Seems worse.
> > 
> > C) Spend his nights and weekends working on this.  Seems worse.
> > 
> > D) Silently stop working on it without saying "I'm going to stop".  Seems worse.
> > 
> > ...unless you have a brilliant "E)" I think what Matthias did here is
> > exactly right.
> 
> I must apologize for making that statement since it was not fair to
> Matthias, and he has been clear about how much time he can spend on that
> specific, please accept my apologies for that.
> 
> Having had many recent encounters with various people not driving
> projects to completion lately (not specifically within Linux), it looks
> like I am overly sensitive about flagging words and patch status that
> may fall within that lexicon. The choice of word is what triggered me.

No worries, I understand that it can be frustrating if you repeatedly
experience that projects remain unfinished.

Hopefully this series can be revived eventually when somebody finds
the time to work on the integration with the LED framework.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
  2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
  2019-08-13 19:54   ` Andrew Lunn
  2019-08-16 20:13   ` Pavel Machek
@ 2019-08-27 16:12   ` Rob Herring
  2 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2019-08-27 16:12 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: David S . Miller, Mark Rutland, Andrew Lunn, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

On Tue, Aug 13, 2019 at 12:11:44PM -0700, Matthias Kaehlcke wrote:
> The LED behavior of some Ethernet PHYs is configurable. Add an
> optional 'leds' subnode with a child node for each LED to be
> configured. The binding aims to be compatible with the common
> LED binding (see devicetree/bindings/leds/common.txt).
> 
> A LED can be configured to be:
> 
> - 'on' when a link is active, some PHYs allow configuration for
>   certain link speeds
>   speeds
> - 'off'
> - blink on RX/TX activity, some PHYs allow configuration for
>   certain link speeds
> 
> For the configuration to be effective it needs to be supported by
> the hardware and the corresponding PHY driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v6:
> - none
> 
> Changes in v5:
> - renamed triggers from 'phy_link_<speed>_active' to 'phy-link-<speed>'
> - added entries for 'phy-link-<speed>-activity'
> - added 'phy-link' and 'phy-link-activity' for triggers with any link
>   speed
> - added entry for trigger 'none'
> 
> Changes in v4:
> - patch added to the series
> ---
>  .../devicetree/bindings/net/ethernet-phy.yaml | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index f70f18ff821f..98ba320f828b 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -153,6 +153,50 @@ properties:
>        Delay after the reset was deasserted in microseconds. If
>        this property is missing the delay will be skipped.
>  
> +patternProperties:
> +  "^leds$":
> +    type: object
> +    description:
> +      Subnode with configuration of the PHY LEDs.

#address-cells and #size-cells needed.

> +
> +    patternProperties:
> +      "^led@[0-9]+$":

Need to allow for the case of 1 LED which doesn't need a unit-address 
nor reg.

> +        type: object
> +        description:
> +          Subnode with the configuration of a single PHY LED.
> +
> +    properties:
> +      reg:
> +        description:
> +          The ID number of the LED, typically corresponds to a hardware ID.
> +        $ref: "/schemas/types.yaml#/definitions/uint32"

Standard properties already have a type definition. What's needed is 
'maxItems: 1'.

> +
> +      linux,default-trigger:
> +        description:
> +          This parameter, if present, is a string specifying the trigger
> +          assigned to the LED. Supported triggers are:
> +            "none" - LED will be solid off
> +            "phy-link" - LED will be solid on when a link is active
> +            "phy-link-10m" - LED will be solid on when a 10Mb/s link is active
> +            "phy-link-100m" - LED will be solid on when a 100Mb/s link is active
> +            "phy-link-1g" - LED will be solid on when a 1Gb/s link is active
> +            "phy-link-10g" - LED will be solid on when a 10Gb/s link is active
> +            "phy-link-activity" - LED will be on when link is active and blink
> +                                  off with activity.
> +            "phy-link-10m-activity" - LED will be on when 10Mb/s link is active
> +                                      and blink off with activity.
> +            "phy-link-100m-activity" - LED will be on when 100Mb/s link is
> +                                       active and blink off with activity.
> +            "phy-link-1g-activity" - LED will be on when 1Gb/s link is active
> +                                     and blink off with activity.
> +            "phy-link-10g-activity" - LED will be on when 10Gb/s link is active
> +                                      and blink off with activity.

These strings all need to be in an enum.

The led binding is moving away from linux,default-trigger to 'function' 
and 'color' properties. You probably want to do that here.

> +
> +        $ref: "/schemas/types.yaml#/definitions/string"
> +
> +    required:
> +      - reg
> +
>  required:
>    - reg
>  
> @@ -173,5 +217,20 @@ examples:
>              reset-gpios = <&gpio1 4 1>;
>              reset-assert-us = <1000>;
>              reset-deassert-us = <2000>;
> +
> +            leds {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                led@0 {
> +                    reg = <0>;
> +                    linux,default-trigger = "phy-link-1g";
> +                };
> +
> +                led@1 {
> +                    reg = <1>;
> +                    linux,default-trigger = "phy-link-100m-activity";
> +                };
> +            };
>          };
>      };
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
  2019-08-19  0:37         ` Andrew Lunn
@ 2019-10-07 11:02           ` Pavel Machek
  0 siblings, 0 replies; 24+ messages in thread
From: Pavel Machek @ 2019-10-07 11:02 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Matthias Kaehlcke, jacek.anaszewski, linux-leds, dmurphy,
	David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
	Heiner Kallweit, netdev, devicetree, linux-kernel,
	Douglas Anderson

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Mon 2019-08-19 02:37:57, Andrew Lunn wrote:
> > Yes, I believe the integration is neccessary. Using same binding is
> > neccessary for that, but not sufficient. For example, we need
> > compatible trigger names, too.
> 
> Hi Pavel
> 
> Please could you explain what you mean by compatible trigger names?

Well, you attempted to put trigger names in device tree. That means
those names should work w.r.t. LED subsystem, too.

> > So... I'd really like to see proper integration is possible before we
> > merge this.
> 
> Please let me turn that around. What do you see as being impossible at
> the moment? What do we need to convince you about?

That locking requirements are compatible, that triggers you invented
can be implemented by LED subsystem, ...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, back to index

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13 19:11 [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E Matthias Kaehlcke
2019-08-13 19:11 ` [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration Matthias Kaehlcke
2019-08-13 19:54   ` Andrew Lunn
2019-08-16 20:13   ` Pavel Machek
2019-08-16 22:04     ` Matthias Kaehlcke
2019-08-19  0:29       ` Andrew Lunn
2019-08-27 16:12   ` Rob Herring
2019-08-13 19:11 ` [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT Matthias Kaehlcke
2019-08-13 19:56   ` Andrew Lunn
2019-08-13 19:11 ` [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages Matthias Kaehlcke
2019-08-13 20:09   ` Andrew Lunn
2019-08-13 19:11 ` [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E Matthias Kaehlcke
2019-08-13 20:14   ` Andrew Lunn
2019-08-13 20:46     ` Matthias Kaehlcke
2019-08-16 20:13   ` Pavel Machek
2019-08-16 21:27     ` Matthias Kaehlcke
2019-08-16 22:12       ` Florian Fainelli
2019-08-16 22:39         ` Doug Anderson
2019-08-23 19:58           ` Florian Fainelli
2019-08-26 18:40             ` Matthias Kaehlcke
2019-08-16 22:40         ` Matthias Kaehlcke
2019-08-17 14:05       ` Pavel Machek
2019-08-19  0:37         ` Andrew Lunn
2019-10-07 11:02           ` Pavel Machek

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox