[PATCH net-next v1 0/3] Add support for LEDs on Marvell PHYs

[PATCH net-next v1 0/3] Add support for LEDs on Marvell PHYs

[Marek Behún]

Hi,

after 4 RFC versions I am now sending these patches for real.

The code applies on net-next.

Changes since RFC v4:
- added device-tree binding documentation 
- the OF code now checks for linux,default-hw-mode property so that
  default HW mode can be set in device tree (like linux,default-trigger)
  (this was suggested by Andrew)
- the OF code also checks for enable-active-high and led-open-drain
  properties, and the marvell PHY driver now understands uses these
  settings when initializing the LEDs
- the LED operations were moved to their own struct phy_device_led_ops
- a new member was added into struct phy_device: phyindex. This is an
  incrementing integer, new for each registered phy_device. This is used
  for a simple naming scheme for the devicename part of a LED, as was
  suggested in a discussion by Andrew and Pavel. A PHY controlled LED
  now has a name in form:
    phy%i:color:function
  When a PHY is attached to a netdevice, userspace can control available
  PHY controlled LEDs via /sys/class/net/<ifname>/phydev/leds/
- legacy LED configuration in Marvell PHY driver (in function
  marvell_config_led) now writes only to registers which do not
  correspond to any registered LED

Changes since RFC v3:
- addressed some of Andrew's suggestions
- phy_hw_led_mode.c renamed to phy_led.c
- the DT reading code is now also generic, moved to phy_led.c and called
  from phy_probe
- the function registering the phydev-hw-mode trigger is now called from
  phy_device.c function phy_init before registering genphy drivers
- PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS

Changes since RFC v2:
- to share code with other drivers which may want to also offer PHY HW
  control of LEDs some of the code was refactored and now resides in
  phy_hw_led_mode.c. This code is compiled in when config option
  LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW control
  of LEDs should depend on this option.
- the "hw-control" trigger is renamed to "phydev-hw-mode" and is
  registered by the code in phy_hw_led_mode.c
- the "hw_control" sysfs file is renamed to "hw_mode"
- struct phy_driver is extended by three methods to support PHY HW LED
  control
- I renamed the various HW control modes offeret by Marvell PHYs to
  conform to other Linux mode names, for example the "1000/100/10/else"
  mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was renamed
  to "rx" (this is the name of the mode in netdev trigger).

Marek Behún (3):
  dt-bindings: net: ethernet-phy: add description for PHY LEDs
  net: phy: add API for LEDs controlled by ethernet PHY chips
  net: phy: marvell: add support for LEDs controlled by Marvell PHYs

 .../devicetree/bindings/net/ethernet-phy.yaml |  31 ++
 drivers/net/phy/Kconfig                       |   4 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/marvell.c                     | 309 +++++++++++++++++-
 drivers/net/phy/phy_device.c                  |  28 +-
 drivers/net/phy/phy_led.c                     | 224 +++++++++++++
 include/linux/phy.h                           |  91 ++++++
 7 files changed, 679 insertions(+), 9 deletions(-)
 create mode 100644 drivers/net/phy/phy_led.c

-- 
2.26.2

[PATCH net-next v1 1/3] dt-bindings: net: ethernet-phy: add description for PHY LEDs

[Marek Behún]

Document binding for LEDs connected to and controlled by ethernet PHY
chips.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
---
 .../devicetree/bindings/net/ethernet-phy.yaml | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index a9e547ac79051..11839265cc2f9 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -174,6 +174,37 @@ properties:
       PHY's that have configurable TX internal delays. If this property is
       present then the PHY applies the TX delay.
 
+patternProperties:
+  "^led@[0-9a-f]+$":
+    type: object
+    description:
+      This node represents a LED device connected to the PHY chip.
+    $ref: /schemas/leds/common.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+
+      enable-active-high:
+        description:
+          Polarity of LED is active high. If missing, assumed default is active low.
+        type: boolean
+
+      led-open-drain:
+        description:
+          LED pin is open drain type. If missing, assumed false.
+        type: boolean
+
+      linux,default-hw-mode:
+        description:
+          This parameter, if present, specifies the default HW triggering mode of the LED
+          when LED trigger is set to `phydev-hw-mode`.
+          Available values are specific per PHY chip and per LED.
+        $ref: /schemas/types.yaml#definitions/string
+
+    required:
+      - reg
+
 required:
   - reg
 
-- 
2.26.2

[PATCH net-next v1 2/3] net: phy: add API for LEDs controlled by ethernet PHY chips

[Marek Behún]

Many an ethernet PHY supports various HW control modes for LEDs
connected directly to the PHY chip.

This patch adds code for registering such LEDs when described in device
tree and also adds a new private LED trigger called phydev-hw-mode.
When this trigger is enabled for a LED, the various HW control modes
which are supported by the PHY for given LED cat be get/set via hw_mode
sysfs file.

A PHY driver wishing to utilize this API needs to implement all the
methods in the phy_device_led_ops structure.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/Kconfig      |   4 +
 drivers/net/phy/Makefile     |   1 +
 drivers/net/phy/phy_device.c |  28 +++--
 drivers/net/phy/phy_led.c    | 224 +++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  91 ++++++++++++++
 5 files changed, 341 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/phy/phy_led.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 698bea312adc6..6ab8956c49b06 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -61,6 +61,10 @@ config SFP
 	depends on HWMON || HWMON=n
 	select MDIO_I2C
 
+config PHY_LEDS
+	bool
+	default y if LEDS_TRIGGERS
+
 comment "MII PHY device drivers"
 
 config AMD_PHY
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf8..8a54fb30a729d 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -20,6 +20,7 @@ endif
 obj-$(CONFIG_MDIO_DEVRES)	+= mdio_devres.o
 libphy-$(CONFIG_SWPHY)		+= swphy.o
 libphy-$(CONFIG_LED_TRIGGER_PHY)	+= phy_led_triggers.o
+libphy-$(CONFIG_PHY_LEDS)		+= phy_led.o
 
 obj-$(CONFIG_PHYLINK)		+= phylink.o
 obj-$(CONFIG_PHYLIB)		+= libphy.o
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8adfbad0a1e8f..8a28134841ec3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/atomic.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
  */
 int phy_device_register(struct phy_device *phydev)
 {
+	static atomic_t phyindex;
 	int err;
 
 	err = mdiobus_register_device(&phydev->mdio);
@@ -908,6 +910,7 @@ int phy_device_register(struct phy_device *phydev)
 		goto out;
 	}
 
+	phydev->phyindex = atomic_inc_return(&phyindex) - 1;
 	err = device_add(&phydev->mdio.dev);
 	if (err) {
 		phydev_err(phydev, "failed to add\n");
@@ -2909,6 +2912,8 @@ static int phy_probe(struct device *dev)
 				 phydev->supported);
 	}
 
+	of_phy_register_leds(phydev);
+
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;
 
@@ -3040,24 +3045,32 @@ static int __init phy_init(void)
 {
 	int rc;
 
-	rc = mdio_bus_init();
+	rc = phy_leds_init();
 	if (rc)
 		return rc;
 
+	rc = mdio_bus_init();
+	if (rc)
+		goto err_leds;
+
 	ethtool_set_ethtool_phy_ops(&phy_ethtool_phy_ops);
 	features_init();
 
 	rc = phy_driver_register(&genphy_c45_driver, THIS_MODULE);
 	if (rc)
-		goto err_c45;
+		goto err_mdio;
 
 	rc = phy_driver_register(&genphy_driver, THIS_MODULE);
-	if (rc) {
-		phy_driver_unregister(&genphy_c45_driver);
-err_c45:
-		mdio_bus_exit();
-	}
+	if (rc)
+		goto err_c45;
 
+	return 0;
+err_c45:
+	phy_driver_unregister(&genphy_c45_driver);
+err_mdio:
+	mdio_bus_exit();
+err_leds:
+	phy_leds_exit();
 	return rc;
 }
 
@@ -3067,6 +3080,7 @@ static void __exit phy_exit(void)
 	phy_driver_unregister(&genphy_driver);
 	mdio_bus_exit();
 	ethtool_set_ethtool_phy_ops(NULL);
+	phy_leds_exit();
 }
 
 subsys_initcall(phy_init);
diff --git a/drivers/net/phy/phy_led.c b/drivers/net/phy/phy_led.c
new file mode 100644
index 0000000000000..80d203b41c92e
--- /dev/null
+++ b/drivers/net/phy/phy_led.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* drivers/net/phy/phy_hw_led_mode.c
+ *
+ * PHY HW LED mode trigger
+ *
+ * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz>
+ */
+#include <linux/leds.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+
+int phy_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+	struct phy_device_led *led = led_cdev_to_phy_device_led(cdev);
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->led_ops->led_brightness_set(phydev, led, brightness);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_led_brightness_set);
+
+static int of_phy_register_led(struct phy_device *phydev, struct device_node *np)
+{
+	struct phy_device_led_init_data pdata = {};
+	struct led_init_data init_data = {};
+	struct phy_device_led *led;
+	char devicename[16];
+	u32 reg;
+	int ret;
+
+	ret = of_property_read_u32(np, "reg", &reg);
+	if (ret < 0)
+		return ret;
+
+	led = devm_kzalloc(&phydev->mdio.dev, sizeof(struct phy_device_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->cdev.brightness_set_blocking = phy_led_brightness_set;
+	led->cdev.trigger_type = &phy_hw_led_trig_type;
+	led->addr = reg;
+
+	of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
+	of_property_read_string(np, "linux,default-hw-mode", &led->hw_mode);
+
+	pdata.active_low = !of_property_read_bool(np, "enable-active-high");
+	pdata.open_drain = of_property_read_bool(np, "led-open-drain");
+
+	init_data.fwnode = &np->fwnode;
+	init_data.devname_mandatory = true;
+	snprintf(devicename, sizeof(devicename), "phy%d", phydev->phyindex);
+	init_data.devicename = devicename;
+
+	ret = phydev->led_ops->led_init(phydev, led, &pdata);
+	if (ret < 0)
+		goto err_free;
+
+	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
+	if (ret < 0)
+		goto err_free;
+
+	led->flags |= PHY_DEVICE_LED_REGISTERED;
+
+	return 0;
+err_free:
+	devm_kfree(&phydev->mdio.dev, led);
+	return ret;
+}
+
+int of_phy_register_leds(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *leds, *led;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (!phydev->drv->led_ops)
+		return 0;
+
+	phydev->led_ops = phydev->drv->led_ops;
+
+	leds = of_get_child_by_name(node, "leds");
+	if (!leds)
+		return 0;
+
+	for_each_available_child_of_node(leds, led) {
+		ret = of_phy_register_led(phydev, led);
+		if (ret < 0)
+			phydev_err(phydev, "Nonfatal error: cannot register LED from node %pOFn: %i\n",
+				   led, ret);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_phy_register_leds);
+
+static int phy_hw_led_trig_activate(struct led_classdev *cdev)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct phy_device_led *led;
+	int ret;
+
+	led = led_cdev_to_phy_device_led(cdev);
+
+	if (!led->hw_mode)
+		return 0;
+
+	/* When this trigger is being activated from devm_led_classdev_register_ext, the mutex is
+	 * already locked (in phy_probe), so only lock/unlock it if the LED is already registered.
+	 */
+	if (led->flags & PHY_DEVICE_LED_REGISTERED)
+		mutex_lock(&phydev->lock);
+
+	ret = phydev->led_ops->led_set_hw_mode(phydev, led, led->hw_mode);
+
+	if (led->flags & PHY_DEVICE_LED_REGISTERED)
+		mutex_unlock(&phydev->lock);
+
+	if (ret < 0)
+		phydev_warn(phydev, "could not set HW mode %s on LED %s: %i\n", led->hw_mode,
+			    cdev->name, ret);
+
+	/* don't fail to activate this trigger so that user can write hw_mode file */
+	return 0;
+}
+
+static void phy_hw_led_trig_deactivate(struct led_classdev *cdev)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct phy_device_led *led;
+	int ret;
+
+	led = led_cdev_to_phy_device_led(cdev);
+
+	mutex_lock(&phydev->lock);
+
+	/* store HW mode before deactivation */
+	led->hw_mode = phydev->led_ops->led_get_hw_mode(phydev, led);
+
+	ret = phydev->led_ops->led_set_hw_mode(phydev, led_cdev_to_phy_device_led(cdev), NULL);
+
+	mutex_unlock(&phydev->lock);
+
+	if (ret < 0)
+		phydev_err(phydev, "failed deactivating HW mode on LED %s\n", cdev->name);
+}
+
+static ssize_t hw_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct phy_device *phydev = to_phy_device(dev->parent);
+	const char *mode, *cur_mode;
+	struct phy_device_led *led;
+	void *iter = NULL;
+	int len = 0;
+
+	led = led_cdev_to_phy_device_led(led_trigger_get_led(dev));
+
+	mutex_lock(&phydev->lock);
+
+	cur_mode = phydev->led_ops->led_get_hw_mode(phydev, led);
+
+	for (mode = phydev->led_ops->led_iter_hw_mode(phydev, led, &iter);
+	     mode;
+	     mode = phydev->led_ops->led_iter_hw_mode(phydev, led, &iter)) {
+		bool sel;
+
+		sel = cur_mode && !strcmp(mode, cur_mode);
+
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
+				 sel ? "]" : "");
+	}
+
+	if (buf[len - 1] == ' ')
+		buf[len - 1] = '\n';
+
+	mutex_unlock(&phydev->lock);
+
+	return len;
+}
+
+static ssize_t hw_mode_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			     size_t count)
+{
+	struct phy_device *phydev = to_phy_device(dev->parent);
+	struct phy_device_led *led;
+	int ret;
+
+	led = led_cdev_to_phy_device_led(led_trigger_get_led(dev));
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->led_ops->led_set_hw_mode(phydev, led, buf);
+	mutex_unlock(&phydev->lock);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(hw_mode);
+
+static struct attribute *phy_hw_led_trig_attrs[] = {
+	&dev_attr_hw_mode.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(phy_hw_led_trig);
+
+struct led_hw_trigger_type phy_hw_led_trig_type;
+EXPORT_SYMBOL_GPL(phy_hw_led_trig_type);
+
+struct led_trigger phy_hw_led_trig = {
+	.name		= "phydev-hw-mode",
+	.activate	= phy_hw_led_trig_activate,
+	.deactivate	= phy_hw_led_trig_deactivate,
+	.trigger_type	= &phy_hw_led_trig_type,
+	.groups		= phy_hw_led_trig_groups,
+};
+EXPORT_SYMBOL_GPL(phy_hw_led_trig);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3a09d2bf69ea4..2c651375becd1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -14,6 +14,7 @@
 #include <linux/compiler.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
+#include <linux/leds.h>
 #include <linux/linkmode.h>
 #include <linux/netlink.h>
 #include <linux/mdio.h>
@@ -406,6 +407,7 @@ struct macsec_ops;
 /* phy_device: An instance of a PHY
  *
  * drv: Pointer to the driver for this PHY instance
+ * phyindex: a simple incrementing PHY index
  * phy_id: UID for this device found during discovery
  * c45_ids: 802.3-c45 Device Identifers if is_c45.
  * is_c45:  Set to true if this phy uses clause 45 addressing.
@@ -420,6 +422,7 @@ struct macsec_ops;
  * downshifted_rate: Set true if link speed has been downshifted.
  * state: state of the PHY for management purposes
  * dev_flags: Device-specific flags used by the PHY driver.
+ * led_ops: PHY connected LEDs ops
  * irq: IRQ number of the PHY's interrupt (-1 if none)
  * phy_timer: The timer for handling the state machine
  * sfp_bus_attached: flag indicating whether the SFP bus has been attached
@@ -446,6 +449,8 @@ struct phy_device {
 	/* And management functions */
 	struct phy_driver *drv;
 
+	int phyindex;
+
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
@@ -505,6 +510,10 @@ struct phy_device {
 	struct phy_led_trigger *led_link_trigger;
 #endif
 
+#if IS_ENABLED(CONFIG_PHY_LEDS)
+	const struct phy_device_led_ops *led_ops;
+#endif
+
 	/*
 	 * Interrupt number for this PHY
 	 * -1 means no interrupt
@@ -562,6 +571,85 @@ struct phy_tdr_config {
 };
 #define PHY_PAIR_ALL -1
 
+struct phy_device_led_init_data {
+	bool active_low;
+	bool open_drain;
+};
+
+#define PHY_DEVICE_LED_REGISTERED	BIT(31)
+
+/* PHY LEDs support */
+struct phy_device_led {
+	struct led_classdev cdev;
+	const char *hw_mode;
+	int addr;
+	u32 flags;
+};
+#define led_cdev_to_phy_device_led(l) container_of(l, struct phy_device_led, cdev)
+
+/* struct phy_device_led_ops: Operations on LEDs controlled by PHY chips
+ *
+ * All the following operations must be implemented:
+ * @led_init: Should initialize LED from init_data (and check whether they are correct).
+ *            This should also set led->cdev.max_brightness, for example.
+ * @led_brightness_set: Sets brightness.
+ * @led_iter_hw_mode: Iterates available HW control mode names for this LED.
+ * @led_set_hw_mode: Sets HW control mode to value specified by given name.
+ * @led_get_hw_mode: Returns current HW control mode name.
+ *
+ * This methods must not lock the phy_device lock mutex. When they are called, the mutex
+ * is already locked.
+ */
+struct phy_device_led_ops {
+	int (*led_init)(struct phy_device *dev, struct phy_device_led *led,
+			const struct phy_device_led_init_data *pdata);
+	int (*led_brightness_set)(struct phy_device *dev, struct phy_device_led *led,
+				  enum led_brightness brightness);
+	const char *(*led_iter_hw_mode)(struct phy_device *dev, struct phy_device_led *led,
+					void **iter);
+	int (*led_set_hw_mode)(struct phy_device *dev, struct phy_device_led *led,
+			       const char *mode);
+	const char *(*led_get_hw_mode)(struct phy_device *dev, struct phy_device_led *led);
+};
+
+#if IS_ENABLED(CONFIG_PHY_LEDS)
+
+#define phy_device_led_ops_ptr(s) (s)
+
+int of_phy_register_leds(struct phy_device *phydev);
+
+extern struct led_hw_trigger_type phy_hw_led_trig_type;
+extern struct led_trigger phy_hw_led_trig;
+int phy_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
+
+static inline int phy_leds_init(void)
+{
+	return led_trigger_register(&phy_hw_led_trig);
+}
+
+static inline void phy_leds_exit(void)
+{
+	led_trigger_unregister(&phy_hw_led_trig);
+}
+#else /* !IS_ENABLED(CONFIG_PHY_LEDS) */
+
+#define phy_device_led_ops_ptr(s) NULL
+
+static inline int of_phy_register_leds(struct phy_device *phydev)
+{
+	return 0;
+}
+
+static inline int phy_leds_init(void)
+{
+	return 0;
+}
+
+static inline void phy_leds_exit(void)
+{
+}
+#endif /* !IS_ENABLED(CONFIG_PHY_LEDS) */
+
 /* struct phy_driver: Driver structure for a particular PHY type
  *
  * driver_data: static driver data
@@ -738,6 +826,9 @@ struct phy_driver {
 	int (*set_loopback)(struct phy_device *dev, bool enable);
 	int (*get_sqi)(struct phy_device *dev);
 	int (*get_sqi_max)(struct phy_device *dev);
+
+	/* PHY connected LEDs operations */
+	const struct phy_device_led_ops *led_ops;
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.26.2

[PATCH net-next v1 3/3] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behún]

This patch adds support for controlling the LEDs connected to several
families of Marvell PHYs via the PHY HW LED trigger API. These families
are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
be added.

This patch does not yet add support for compound LED modes. This could
be achieved via the LED multicolor framework.

Settings such as HW blink rate or pulse stretch duration are not yet
supported.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/marvell.c | 309 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 307 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd0920..e0293f309644a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -148,6 +148,13 @@
 #define MII_88E1510_PHY_LED_DEF		0x1177
 #define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE	0x1040
 
+#define MII_PHY_LED_POLARITY_CTRL	17
+#define MII_PHY_LED_TIMER_CTRL		18
+#define MII_PHY_LED45_CTRL		19
+
+#define MII_PHY_LED_CTRL_FORCE_ON	0x9
+#define MII_PHY_LED_CTRL_FORCE_OFF	0x8
+
 #define MII_M1011_PHY_STATUS		0x11
 #define MII_M1011_PHY_STATUS_1000	0x8000
 #define MII_M1011_PHY_STATUS_100	0x4000
@@ -252,6 +259,8 @@
 #define LPA_PAUSE_FIBER		0x180
 #define LPA_PAUSE_ASYM_FIBER	0x100
 
+#define MARVELL_PHY_MAX_LEDS	6
+
 #define NB_FIBER_STATS	1
 
 MODULE_DESCRIPTION("Marvell PHY driver");
@@ -280,6 +289,7 @@ struct marvell_priv {
 	u32 last;
 	u32 step;
 	s8 pair;
+	u16 legacy_led_config_mask;
 };
 
 static int marvell_read_page(struct phy_device *phydev)
@@ -662,8 +672,295 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+#if IS_ENABLED(CONFIG_PHY_LEDS)
+
+enum {
+	COMMON			= BIT(0),
+	L1V0_RECV		= BIT(1),
+	L1V0_COPPER		= BIT(2),
+	L1V5_100_FIBER		= BIT(3),
+	L1V5_100_10		= BIT(4),
+	L2V2_INIT		= BIT(5),
+	L2V2_PTP		= BIT(6),
+	L2V2_DUPLEX		= BIT(7),
+	L3V0_FIBER		= BIT(8),
+	L3V0_LOS		= BIT(9),
+	L3V5_TRANS		= BIT(10),
+	L3V7_FIBER		= BIT(11),
+	L3V7_DUPLEX		= BIT(12),
+};
+
+struct marvell_led_mode_info {
+	const char *name;
+	s8 regval[MARVELL_PHY_MAX_LEDS];
+	u32 flags;
+};
+
+static const struct marvell_led_mode_info marvell_led_mode_info[] = {
+	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, COMMON },
+	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON },
+	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, COMMON },
+	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON },
+	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, COMMON },
+	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
+	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, COMMON },
+	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
+	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
+	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, COMMON },
+	{ "100Mbps-fiber",		{  -1, 0x5,  -1,  -1,  -1,  -1, }, L1V5_100_FIBER },
+	{ "100Mbps-10Mbps",		{  -1, 0x5,  -1,  -1,  -1,  -1, }, L1V5_100_10 },
+	{ "1Gbps-100Mbps",		{  -1, 0x6,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, COMMON },
+	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, COMMON },
+	{ "fiber",			{  -1,  -1,  -1, 0x0,  -1,  -1, }, L3V0_FIBER },
+	{ "fiber",			{  -1,  -1,  -1, 0x7,  -1,  -1, }, L3V7_FIBER },
+	{ "FullDuplex",			{  -1,  -1,  -1, 0x7,  -1,  -1, }, L3V7_DUPLEX },
+	{ "FullDuplex",			{  -1,  -1,  -1,  -1, 0x6, 0x6, }, COMMON },
+	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, COMMON },
+	{ "FullDuplex/collision",	{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_DUPLEX },
+	{ "ptp",			{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_PTP },
+	{ "init",			{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_INIT },
+	{ "los",			{  -1,  -1,  -1, 0x0,  -1,  -1, }, L3V0_LOS },
+	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, COMMON },
+};
+
+struct marvell_leds_info {
+	u32 family;
+	int nleds;
+	u32 flags;
+};
+
+#define LED(fam, n, flg)							\
+	{									\
+		.family = MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E##fam),	\
+		.nleds = (n),							\
+		.flags = (flg),							\
+	}									\
+
+static const struct marvell_leds_info marvell_leds_info[] = {
+	LED(1112,  4, COMMON | L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS |
+		      L3V7_FIBER),
+	LED(1121R, 3, COMMON | L1V5_100_10),
+	LED(1240,  6, COMMON | L3V5_TRANS),
+	LED(1340S, 6, COMMON | L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
+	LED(1510,  3, COMMON | L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
+	LED(1545,  6, COMMON | L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
+};
+
+static inline int marvell_led_reg(int led)
+{
+	switch (led) {
+	case 0 ... 3:
+		return MII_PHY_LED_CTRL;
+	case 4 ... 5:
+		return MII_PHY_LED45_CTRL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int marvell_led_set_regval(struct phy_device *phydev, int led, u16 val)
+{
+	u16 mask;
+	int reg;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val <<= (led % 4) * 4;
+	mask = 0xf << ((led % 4) * 4);
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_get_regval(struct phy_device *phydev, int led)
+{
+	int reg, val;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, reg);
+	if (val < 0)
+		return val;
+
+	val >>= (led % 4) * 4;
+	val &= 0xf;
+
+	return val;
+}
+
+static int marvell_led_set_polarity(struct phy_device *phydev, int led, bool active_low,
+				    bool open_drain)
+{
+	int reg, shift;
+	u16 mask, val;
+
+	switch (led) {
+	case 0 ... 3:
+		reg = MII_PHY_LED_POLARITY_CTRL;
+		break;
+	case 4 ... 5:
+		reg = MII_PHY_LED45_CTRL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = 0;
+	if (!active_low)
+		val |= BIT(0);
+	if (open_drain)
+		val |= BIT(1);
+
+	shift = led * 2;
+	val <<= shift;
+	mask = 0x3 << shift;
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_brightness_set(struct phy_device *phydev, struct phy_device_led *led,
+				      enum led_brightness brightness)
+{
+	u8 val;
+
+	/* don't do anything if HW control is enabled */
+	if (led->cdev.trigger == &phy_hw_led_trig)
+		return 0;
+
+	val = brightness ? MII_PHY_LED_CTRL_FORCE_ON : MII_PHY_LED_CTRL_FORCE_OFF;
+
+	return marvell_led_set_regval(phydev, led->addr, val);
+}
+
+static inline bool is_valid_led_mode(struct phy_device_led *led,
+				     const struct marvell_led_mode_info *mode)
+{
+	return mode->regval[led->addr] != -1 && (led->flags & mode->flags);
+}
+
+static const char *marvell_led_iter_hw_mode(struct phy_device *phydev, struct phy_device_led *led,
+					    void **iter)
+{
+	const struct marvell_led_mode_info *mode = *iter;
+
+	if (!mode)
+		mode = marvell_led_mode_info;
+
+	if (mode - marvell_led_mode_info == ARRAY_SIZE(marvell_led_mode_info))
+		goto end;
+
+	while (!is_valid_led_mode(led, mode)) {
+		++mode;
+		if (mode - marvell_led_mode_info == ARRAY_SIZE(marvell_led_mode_info))
+			goto end;
+	}
+
+	*iter = (void *)(mode + 1);
+	return mode->name;
+end:
+	*iter = NULL;
+	return NULL;
+}
+
+static int marvell_led_set_hw_mode(struct phy_device *phydev, struct phy_device_led *led,
+				   const char *name)
+{
+	const struct marvell_led_mode_info *mode;
+	int i;
+
+	if (!name)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+		mode = &marvell_led_mode_info[i];
+
+		if (!is_valid_led_mode(led, mode))
+			continue;
+
+		if (sysfs_streq(name, mode->name))
+			return marvell_led_set_regval(phydev, led->addr, mode->regval[led->addr]);
+	}
+
+	return -EINVAL;
+}
+
+static const char *marvell_led_get_hw_mode(struct phy_device *phydev, struct phy_device_led *led)
+{
+	const struct marvell_led_mode_info *mode;
+	int i, regval;
+
+	regval = marvell_led_get_regval(phydev, led->addr);
+	if (regval < 0)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+		mode = &marvell_led_mode_info[i];
+
+		if (!is_valid_led_mode(led, mode))
+			continue;
+
+		if (mode->regval[led->addr] == regval)
+			return mode->name;
+	}
+
+	return NULL;
+}
+
+static int marvell_led_init(struct phy_device *phydev, struct phy_device_led *led,
+			    const struct phy_device_led_init_data *pdata)
+{
+	const struct marvell_leds_info *info = NULL;
+	struct marvell_priv *priv = phydev->priv;
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_leds_info); ++i) {
+		if (MARVELL_PHY_FAMILY_ID(phydev->phy_id) == marvell_leds_info[i].family) {
+			info = &marvell_leds_info[i];
+			break;
+		}
+	}
+
+	if (!info)
+		return -EOPNOTSUPP;
+
+	if (led->addr >= info->nleds)
+		return -EINVAL;
+
+	led->flags = info->flags;
+	led->cdev.max_brightness = 1;
+
+	ret = marvell_led_set_polarity(phydev, led->addr, pdata->active_low, pdata->open_drain);
+	if (ret < 0)
+		return ret;
+
+	/* ensure marvell_config_led below does not change settings we have set for this LED */
+	if (led->addr < 3)
+		priv->legacy_led_config_mask &= ~(0xf << (led->addr * 4));
+
+	return 0;
+}
+
+static const struct phy_device_led_ops marvell_led_ops = {
+	.led_init = marvell_led_init,
+	.led_brightness_set = marvell_led_brightness_set,
+	.led_iter_hw_mode = marvell_led_iter_hw_mode,
+	.led_set_hw_mode = marvell_led_set_hw_mode,
+	.led_get_hw_mode = marvell_led_get_hw_mode,
+};
+
+#endif /* IS_ENABLED(CONFIG_PHY_LEDS) */
+
 static void marvell_config_led(struct phy_device *phydev)
 {
+	struct marvell_priv *priv = phydev->priv;
 	u16 def_config;
 	int err;
 
@@ -688,8 +985,9 @@ static void marvell_config_led(struct phy_device *phydev)
 		return;
 	}
 
-	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
-			      def_config);
+	def_config &= priv->legacy_led_config_mask;
+	err = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
+			       priv->legacy_led_config_mask, def_config);
 	if (err < 0)
 		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
 }
@@ -2580,6 +2878,7 @@ static int marvell_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->legacy_led_config_mask = 0xffff;
 	phydev->priv = priv;
 
 	return 0;
@@ -2656,6 +2955,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.led_ops = phy_device_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2717,6 +3017,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.led_ops = phy_device_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1318S,
@@ -2796,6 +3097,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		.led_ops = phy_device_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1116R,
@@ -2844,6 +3146,7 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		.led_ops = phy_device_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
@@ -2896,6 +3199,7 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		.led_ops = phy_device_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2964,6 +3268,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		.led_ops = phy_device_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1548P,
-- 
2.26.2

Re: [PATCH net-next v1 1/3] dt-bindings: net: ethernet-phy: add description for PHY LEDs

[Marek Behun]

Please ignore this patch, still refactoring...

Re: [PATCH net-next v1 1/3] dt-bindings: net: ethernet-phy: add description for PHY LEDs

[Marek Behun]

Please ignore this series, still refactoring...

Re: [PATCH net-next v1 2/3] net: phy: add API for LEDs controlled by ethernet PHY chips

[kernel test robot]

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

Hi "Marek,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Marek-Beh-n/Add-support-for-LEDs-on-Marvell-PHYs/20200908-080520
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 81365af13a5630673c49bfad9b24cf415e9576f6
config: arm-randconfig-r036-20200909 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 8893d0816ccdf8998d2e21b5430e9d6abe7ef465)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: devm_led_classdev_register_ext
   >>> referenced by phy_led.c:63 (drivers/net/phy/phy_led.c:63)
   >>> net/phy/phy_led.o:(of_phy_register_leds) in archive drivers/built-in.a

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28099 bytes --]

Re: [PATCH net-next v1 2/3] net: phy: add API for LEDs controlled by ethernet PHY chips

[Pavel Machek]

Hi!

> Many an ethernet PHY supports various HW control modes for LEDs
> connected directly to the PHY chip.
> 
> This patch adds code for registering such LEDs when described in device
> tree and also adds a new private LED trigger called phydev-hw-mode.
> When this trigger is enabled for a LED, the various HW control modes
> which are supported by the PHY for given LED cat be get/set via hw_mode
> sysfs file.
> 
> A PHY driver wishing to utilize this API needs to implement all the
> methods in the phy_device_led_ops structure.
> 
> Signed-off-by: Marek Beh??n <marek.behun@nic.cz>


>  	select MDIO_I2C
>  
> +config PHY_LEDS
> +	bool
> +	default y if LEDS_TRIGGERS
> +
>  comment "MII PHY device drivers"
>  
>  config AMD_PHY

> +/* drivers/net/phy/phy_hw_led_mode.c
> + *

Stale comment.

> +	init_data.fwnode = &np->fwnode;
> +	init_data.devname_mandatory = true;
> +	snprintf(devicename, sizeof(devicename), "phy%d", phydev->phyindex);
> +	init_data.devicename = devicename;
> +
> +	ret = phydev->led_ops->led_init(phydev, led, &pdata);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	led->flags |= PHY_DEVICE_LED_REGISTERED;
> +
> +	return 0;
> +err_free:
> +	devm_kfree(&phydev->mdio.dev, led);
> +	return ret;

devm should take care of freeing, right?

Plus, format comments to 80 colums. checkpatch no longer warns, but rule still exists.

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