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

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

[Marek Behún]

Hello,

this RFC series should apply on both net-next/master and Pavel's
linux-leds/for-master tree.

This adds support for LED's connected to some Marvell PHYs.

LEDs are specified via device-tree. Example:

ethernet-phy@1 {
	reg = <1>;

	leds {
		led@0 {
			reg = <0>;
			color = <LED_COLOR_ID_GREEN>;
			function = LED_FUNCTION_LINK;
			linux,default-trigger = "hw:1000/100/10/nolink";
		};

		led@1 {
			reg = <1>;
			color = <LED_COLOR_ID_YELLOW>;
			function = LED_FUNCTION_ACTIVITY;
			linux,default-trigger = "hw:act/noact";
		};
	};
};

Since Marvell PHYs can control the LEDs themselves in various modes,
we need to be able to switch between them or disable them.

This is achieved by extending the LED trigger API with LED-private triggers.
The proposal for this is based on work by Ondrej and Pavel, but after writing
this support for Marvell PHYs I am not very happy how this turned out:
- this LED-private triggers API works by registering triggers with specific
  private trigger type. If this trigger type is defined for a trigger, only
  those LEDs will be able to set this trigger which also have this trigger type.
  (Both structs led_classdev and led_trigger have member trigger_type)
- on Marvell PHYs each LED can have up to 8 different triggers
- currently the driver supports up to 6 LEDs, since at least 88E1340 support
  6 LEDs
- almost every LED supports some mode which is not supported by at least one
  other LED
- this leads to the following dillema:
  1. either we support one trigger type across the driver, but then the
     /sys/class/leds/<LED>/trigger file will also list HW triggers not
     supported by this specific LED,
  2. or we add 6 trigger types, each different one LED, and register up to
     8 HW triggers for each trigger type, which results in up to 48 triggers
     per this driver.
  In this proposal alternative 1 is taken and when unsupported HW trigger
  is requested by writing to /sys/class/leds/<LED>/trigger file, the write
  system call returns -EOPNOTSUPP.
- therefore I think that this is not the correct way how to implement
  LED-private triggers, and instead an approach as desribed in [1].
  What do you people think?

Marek

Marek Behún (3):
  leds: trigger: add support for LED-private device triggers
  leds: trigger: return error value if .activate() failed
  net: phy: marvell: add support for PHY LEDs via LED class

 drivers/leds/led-triggers.c |  32 ++--
 drivers/net/phy/Kconfig     |   7 +
 drivers/net/phy/marvell.c   | 307 +++++++++++++++++++++++++++++++++++-
 include/linux/leds.h        |  10 ++
 4 files changed, 346 insertions(+), 10 deletions(-)

-- 
2.26.2

[PATCH RFC leds + net-next 1/3] leds: trigger: add support for LED-private device triggers

[Marek Behún]

Some LED controllers may come with an internal HW triggering mechanism
for the LED and the ability to switch between SW control and the
internal HW control. This includes most PHYs, various ethernet switches,
the Turris Omnia LED controller or AXP20X PMIC.

This adds support for registering such triggers.

This code is based on work by Pavel Machek <pavel@ucw.cz> and
Ondřej Jirman <megous@megous.com>.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/leds/led-triggers.c | 26 ++++++++++++++++++++------
 include/linux/leds.h        | 10 ++++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 79e30d2cb7a5..81e758d5a048 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
 
  /* Used by LED Class */
 
+static inline bool
+trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
+{
+	return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
+}
+
 ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 			  struct bin_attribute *bin_attr, char *buf,
 			  loff_t pos, size_t count)
@@ -50,7 +56,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 
 	down_read(&triggers_list_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
-		if (sysfs_streq(buf, trig->name)) {
+		if (sysfs_streq(buf, trig->name) && trigger_relevant(led_cdev, trig)) {
 			down_write(&led_cdev->trigger_lock);
 			led_trigger_set(led_cdev, trig);
 			up_write(&led_cdev->trigger_lock);
@@ -93,8 +99,12 @@ static int led_trigger_format(char *buf, size_t size,
 				       led_cdev->trigger ? "none" : "[none]");
 
 	list_for_each_entry(trig, &trigger_list, next_trig) {
-		bool hit = led_cdev->trigger &&
-			!strcmp(led_cdev->trigger->name, trig->name);
+		bool hit;
+
+		if (!trigger_relevant(led_cdev, trig))
+			continue;
+
+		hit = led_cdev->trigger && !strcmp(led_cdev->trigger->name, trig->name);
 
 		len += led_trigger_snprintf(buf + len, size - len,
 					    " %s%s%s", hit ? "[" : "",
@@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 	down_read(&triggers_list_lock);
 	down_write(&led_cdev->trigger_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
-		if (!strcmp(led_cdev->default_trigger, trig->name)) {
+		if (!strcmp(led_cdev->default_trigger, trig->name) &&
+		    trigger_relevant(led_cdev, trig)) {
 			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
 			led_trigger_set(led_cdev, trig);
 			break;
@@ -280,7 +291,9 @@ int led_trigger_register(struct led_trigger *trig)
 	down_write(&triggers_list_lock);
 	/* Make sure the trigger's name isn't already in use */
 	list_for_each_entry(_trig, &trigger_list, next_trig) {
-		if (!strcmp(_trig->name, trig->name)) {
+		if (!strcmp(_trig->name, trig->name) &&
+		    (trig->trigger_type == _trig->trigger_type ||
+		     !trig->trigger_type || !_trig->trigger_type)) {
 			up_write(&triggers_list_lock);
 			return -EEXIST;
 		}
@@ -294,7 +307,8 @@ int led_trigger_register(struct led_trigger *trig)
 	list_for_each_entry(led_cdev, &leds_list, node) {
 		down_write(&led_cdev->trigger_lock);
 		if (!led_cdev->trigger && led_cdev->default_trigger &&
-			    !strcmp(led_cdev->default_trigger, trig->name)) {
+		    !strcmp(led_cdev->default_trigger, trig->name) &&
+		    trigger_relevant(led_cdev, trig)) {
 			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
 			led_trigger_set(led_cdev, trig);
 		}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 2451962d1ec5..6a8d6409c993 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -57,6 +57,10 @@ struct led_init_data {
 	bool devname_mandatory;
 };
 
+struct led_hw_trigger_type {
+	int dummy;
+};
+
 struct led_classdev {
 	const char		*name;
 	enum led_brightness	 brightness;
@@ -141,6 +145,9 @@ struct led_classdev {
 	void			*trigger_data;
 	/* true if activated - deactivate routine uses it to do cleanup */
 	bool			activated;
+
+	/* LEDs that have private triggers have this set */
+	struct led_hw_trigger_type	*trigger_type;
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -345,6 +352,9 @@ struct led_trigger {
 	int		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
 
+	/* LED-private triggers have this set */
+	struct led_hw_trigger_type *trigger_type;
+
 	/* LEDs under control by this trigger (for simple triggers) */
 	rwlock_t	  leddev_list_lock;
 	struct list_head  led_cdevs;
-- 
2.26.2

[PATCH RFC leds + net-next 2/3] leds: trigger: return error value if .activate() failed

[Marek Behún]

Currently when the .activate() method fails and returns a negative error
code while writing to the /sys/class/leds/<LED>/trigger file, the write
system call does not inform the user abouth this failure.

This patch fixes this.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/leds/led-triggers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 81e758d5a048..804e0d624f47 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -40,7 +40,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_trigger *trig;
-	int ret = count;
+	int ret;
 
 	mutex_lock(&led_cdev->led_access);
 
@@ -58,7 +58,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 	list_for_each_entry(trig, &trigger_list, next_trig) {
 		if (sysfs_streq(buf, trig->name) && trigger_relevant(led_cdev, trig)) {
 			down_write(&led_cdev->trigger_lock);
-			led_trigger_set(led_cdev, trig);
+			ret = led_trigger_set(led_cdev, trig);
 			up_write(&led_cdev->trigger_lock);
 
 			up_read(&triggers_list_lock);
@@ -71,7 +71,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 
 unlock:
 	mutex_unlock(&led_cdev->led_access);
-	return ret;
+	return ret < 0 ? ret : count;
 }
 EXPORT_SYMBOL_GPL(led_trigger_write);
 
-- 
2.26.2

[PATCH RFC leds + net-next 3/3] net: phy: marvell: add support for PHY LEDs via LED class

[Marek Behún]

This patch adds support for controlling the LEDs connected to Marvell
PHYs via Linux' LED API.

The code reads LEDs definitions from the device-tree node of the PHY.

Since the LEDs can be controlled by hardware, we add LED-private LED
triggers for every possible HW controlled mode.

This does not yet add support for compound LED modes. This could be
achieved via the LED multicolor framework (which is not yet in
upstream).

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/Kconfig   |   7 +
 drivers/net/phy/marvell.c | 307 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 313 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index dd20c2c27c2f..fb75abdb9f24 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -456,6 +456,13 @@ config MARVELL_PHY
 	help
 	  Currently has a driver for the 88E1011S
 
+config MARVELL_PHY_LEDS
+	bool "Support LEDs on Marvell PHYs"
+	depends on MARVELL_PHY && LEDS_TRIGGERS
+	help
+	  This option enables support for controlling LEDs connected to Marvell
+	  PHYs.
+
 config MARVELL_10G_PHY
 	tristate "Marvell Alaska 10Gbit PHYs"
 	help
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd092..066bb0a77840 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -23,6 +23,7 @@
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/spinlock.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -148,6 +149,11 @@
 #define MII_88E1510_PHY_LED_DEF		0x1177
 #define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE	0x1040
 
+#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 +258,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");
@@ -275,6 +283,9 @@ struct marvell_priv {
 	u64 stats[ARRAY_SIZE(marvell_hw_stats)];
 	char *hwmon_name;
 	struct device *hwmon_dev;
+#if IS_ENABLED(CONFIG_MARVELL_PHY_LEDS)
+	struct led_classdev leds[MARVELL_PHY_MAX_LEDS];
+#endif
 	bool cable_test_tdr;
 	u32 first;
 	u32 last;
@@ -662,6 +673,273 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+#if IS_ENABLED(CONFIG_MARVELL_PHY_LEDS)
+
+static struct led_hw_trigger_type marvell_led_trigger_type;
+
+struct marvell_led_trigger_info {
+	s8 regval[MARVELL_PHY_MAX_LEDS];
+};
+
+static const s8 marvell_led_trigger_info[][MARVELL_PHY_MAX_LEDS] = {
+	{ 0x0,  -1, 0x0,  -1,  -1,  -1, },
+	{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, },
+	{ 0x2,  -1,  -1,  -1,  -1,  -1, },
+	{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, },
+	{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, },
+	{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, },
+	{  -1,  -1,  -1,  -1, 0x0, 0x0, },
+	{ 0x6, 0x0,  -1,  -1,  -1,  -1, },
+	{ 0x7,  -1,  -1,  -1,  -1,  -1, },
+	{  -1, 0x2,  -1, 0x2, 0x2, 0x2, },
+	{  -1, 0x5,  -1,  -1,  -1,  -1, },
+	{  -1, 0x6,  -1,  -1,  -1,  -1, },
+	{  -1,  -1, 0x6, 0x6,  -1,  -1, },
+	{  -1, 0x7,  -1,  -1,  -1,  -1, },
+	{  -1,  -1, 0x7,  -1,  -1,  -1, },
+	{  -1,  -1,  -1, 0x0,  -1,  -1, },
+	{  -1,  -1,  -1, 0x7, 0x6, 0x6, },
+	{  -1,  -1,  -1,  -1, 0x7, 0x7, },
+	{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, },
+	{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, },
+};
+
+static int marvell_led_trigger_activate(struct led_classdev *led_cdev);
+static void marvell_led_trigger_deactivate(struct led_classdev *led_cdev);
+
+#define DEF_MARVELL_LED_TRIG(n)						\
+	{								\
+		.name		= (n),					\
+		.activate	= marvell_led_trigger_activate,		\
+		.deactivate	= marvell_led_trigger_deactivate,	\
+		.trigger_type	= &marvell_led_trigger_type,		\
+	}
+
+/*
+ * TODO: Do we want to check LED capabilities based on PHY ID?
+ * For example some support PTP triggering while others have that register
+ * value specified as "Reserved".
+ */
+static struct led_trigger marvell_led_triggers[] = {
+	DEF_MARVELL_LED_TRIG("hw:link/nolink"),
+	DEF_MARVELL_LED_TRIG("hw:link/act/nolink"),
+	DEF_MARVELL_LED_TRIG("hw:1000/100/10/nolink"),
+	DEF_MARVELL_LED_TRIG("hw:act/noact"),
+	DEF_MARVELL_LED_TRIG("hw:blink-act/noact"),
+	DEF_MARVELL_LED_TRIG("hw:transmit/notransmit"),
+	DEF_MARVELL_LED_TRIG("hw:recv/norecv"),
+	DEF_MARVELL_LED_TRIG("hw:copperlink/else"),
+	DEF_MARVELL_LED_TRIG("hw:1000/else"),
+	DEF_MARVELL_LED_TRIG("hw:link/recv/nolink"),
+	DEF_MARVELL_LED_TRIG("hw:100-fiber/else"),
+	DEF_MARVELL_LED_TRIG("hw:1000-100/else"),
+	DEF_MARVELL_LED_TRIG("hw:1000-10/else"),
+	DEF_MARVELL_LED_TRIG("hw:100/else"),
+	DEF_MARVELL_LED_TRIG("hw:10/else"),
+	DEF_MARVELL_LED_TRIG("hw:fiber/else"),
+	DEF_MARVELL_LED_TRIG("hw:full/half"),
+	DEF_MARVELL_LED_TRIG("hw:full/collision/half"),
+	DEF_MARVELL_LED_TRIG("hw:force-hi-z"),
+	DEF_MARVELL_LED_TRIG("hw:force-blink"),
+};
+
+static int marvell_register_led_triggers(void)
+{
+	int i, err;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_triggers); ++i) {
+		err = led_trigger_register(&marvell_led_triggers[i]);
+		if (err)
+			goto fail;
+	}
+
+	return 0;
+fail:
+	for (--i; i >= 0; --i)
+		led_trigger_unregister(&marvell_led_triggers[i]);
+
+	return err;
+}
+
+static void marvell_unregister_led_triggers(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_triggers); ++i)
+		led_trigger_unregister(&marvell_led_triggers[i]);
+}
+
+static int marvell_led_set_regval(struct phy_device *phydev, int led, u16 val)
+{
+	u16 mask;
+	int reg;
+
+	switch (led) {
+	case 0 ... 3:
+		reg = MII_PHY_LED_CTRL;
+		break;
+	case 4 ... 5:
+		reg = MII_PHY_LED45_CTRL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val <<= (led % 4) * 4;
+	mask = 0xf << ((led % 4) * 4);
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL, mask, val);
+}
+
+static inline int marvell_led_index(struct led_classdev *cdev)
+{
+	struct marvell_priv *priv = to_phy_device(cdev->dev->parent)->priv;
+
+	return cdev - &priv->leds[0];
+}
+
+static int _marvell_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness,
+				       bool check_trigger)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct marvell_priv *priv = phydev->priv;
+	int led;
+	u8 val;
+
+	/* don't do anything if one of HW triggers is set */
+	if (check_trigger && &marvell_led_triggers[0] <= cdev->trigger &&
+	    cdev->trigger < &marvell_led_triggers[ARRAY_SIZE(marvell_led_triggers)])
+		return 0;
+
+	led = cdev - &priv->leds[0];
+	val = brightness ? MII_PHY_LED_CTRL_FORCE_ON : MII_PHY_LED_CTRL_FORCE_OFF;
+
+	return marvell_led_set_regval(phydev, marvell_led_index(cdev), val);
+}
+
+static int marvell_led_brightness_set_blocking(struct led_classdev *cdev,
+					       enum led_brightness brightness)
+{
+	return _marvell_led_brightness_set(cdev, brightness, true);
+}
+
+static int marvell_led_trigger_activate(struct led_classdev *cdev)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct marvell_priv *priv = phydev->priv;
+	int trig, led;
+	s8 regval;
+
+	trig = cdev->trigger - &marvell_led_triggers[0];
+	led = cdev - &priv->leds[0];
+
+	regval = marvell_led_trigger_info[trig][led];
+	if (regval < 0)
+		return -EOPNOTSUPP;
+
+	return marvell_led_set_regval(phydev, led, regval);
+}
+
+static void marvell_led_trigger_deactivate(struct led_classdev *cdev)
+{
+	_marvell_led_brightness_set(cdev, cdev->brightness, false);
+}
+
+static int marvell_register_led(struct phy_device *phydev, struct device_node *np)
+{
+	struct marvell_priv *priv = phydev->priv;
+	struct led_init_data init_data = {};
+	struct led_classdev *cdev;
+	u32 reg, color;
+	int err;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	if (err < 0)
+		return err;
+
+	/*
+	 * Maybe we should check here if reg >= nleds, where nleds is number of LEDs of this specific
+	 * PHY.
+	 */
+	if (reg >= MARVELL_PHY_MAX_LEDS) {
+		phydev_err(phydev, "LED node %pOF has invaling 'reg' property %u\n", np, reg);
+		return -EINVAL;
+	}
+
+	cdev = &priv->leds[reg];
+
+	err = of_property_read_u32(np, "color", &color);
+	if (err < 0) {
+		phydev_err(phydev, "LED node %pOF does not specify color\n", np);
+		return -EINVAL;
+	}
+
+#if 0
+	/* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */
+	/* TODO: Support DUAL MODE */
+	if (color == LED_COLOR_ID_MULTI) {
+		phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n",
+			    np);
+		return -ENOTSUPP;
+	}
+#endif
+
+	init_data.fwnode = &np->fwnode;
+	init_data.devname_mandatory = true;
+	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";
+
+	if (cdev->max_brightness) {
+		phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np);
+		return -EEXIST;
+	}
+
+	cdev->max_brightness = 1;
+	cdev->brightness_set_blocking = marvell_led_brightness_set_blocking;
+	cdev->trigger_type = &marvell_led_trigger_type;
+
+	of_property_read_string(np, "linux,default-trigger", &cdev->default_trigger);
+
+	err = devm_led_classdev_register_ext(&phydev->mdio.dev, cdev, &init_data);
+	if (err < 0) {
+		phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static void marvell_register_leds(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *leds, *led;
+
+	leds = of_get_child_by_name(node, "leds");
+	if (!leds)
+		return;
+
+	for_each_available_child_of_node(leds, led) {
+		/* Should this check if some LED registration failed? */
+		marvell_register_led(phydev, led);
+	}
+}
+
+#else /* !IS_ENABLED(CONFIG_MARVELL_PHY_LEDS) */
+
+static inline int marvell_register_led_triggers(void)
+{
+	return 0;
+}
+
+static inline void marvell_unregister_led_triggers(void)
+{
+}
+
+static inline void marvell_register_leds(struct phy_device *phydev)
+{
+}
+
+#endif /* !IS_ENABLED(CONFIG_MARVELL_PHY_LEDS) */
+
 static void marvell_config_led(struct phy_device *phydev)
 {
 	u16 def_config;
@@ -692,6 +970,8 @@ static void marvell_config_led(struct phy_device *phydev)
 			      def_config);
 	if (err < 0)
 		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
+
+	marvell_register_leds(phydev);
 }
 
 static int marvell_config_init(struct phy_device *phydev)
@@ -2989,7 +3269,32 @@ static struct phy_driver marvell_drivers[] = {
 	},
 };
 
-module_phy_driver(marvell_drivers);
+static int __init phy_module_init(void)
+{
+	int ret;
+
+	ret = marvell_register_led_triggers();
+	if (ret < 0)
+		return ret;
+
+	ret = phy_drivers_register(marvell_drivers, ARRAY_SIZE(marvell_drivers), THIS_MODULE);
+	if (ret < 0) {
+		marvell_unregister_led_triggers();
+		return ret;
+	}
+
+	return 0;
+}
+
+module_init(phy_module_init);
+
+static void __exit phy_module_exit(void)
+{
+	phy_drivers_unregister(marvell_drivers, ARRAY_SIZE(marvell_drivers));
+	marvell_unregister_led_triggers();
+}
+
+module_exit(phy_module_exit);
 
 static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ MARVELL_PHY_ID_88E1101, MARVELL_PHY_ID_MASK },
-- 
2.26.2

Re: [PATCH RFC leds + net-next 0/3] Add support for LEDs on Marvell PHYs

[Ondřej Jirman]

Hello,

On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote:
> Hello,
> 
> this RFC series should apply on both net-next/master and Pavel's
> linux-leds/for-master tree.
> 
> This adds support for LED's connected to some Marvell PHYs.
> 
> LEDs are specified via device-tree. Example:
> 
> ethernet-phy@1 {
> 	reg = <1>;
> 
> 	leds {
> 		led@0 {
> 			reg = <0>;
> 			color = <LED_COLOR_ID_GREEN>;
> 			function = LED_FUNCTION_LINK;
> 			linux,default-trigger = "hw:1000/100/10/nolink";
> 		};
> 
> 		led@1 {
> 			reg = <1>;
> 			color = <LED_COLOR_ID_YELLOW>;
> 			function = LED_FUNCTION_ACTIVITY;
> 			linux,default-trigger = "hw:act/noact";
> 		};
> 	};
> };
> 
> Since Marvell PHYs can control the LEDs themselves in various modes,
> we need to be able to switch between them or disable them.
> 
> This is achieved by extending the LED trigger API with LED-private triggers.
> The proposal for this is based on work by Ondrej and Pavel, but after writing
> this support for Marvell PHYs I am not very happy how this turned out:
> - this LED-private triggers API works by registering triggers with specific
>   private trigger type. If this trigger type is defined for a trigger, only
>   those LEDs will be able to set this trigger which also have this trigger type.
>   (Both structs led_classdev and led_trigger have member trigger_type)
> - on Marvell PHYs each LED can have up to 8 different triggers
> - currently the driver supports up to 6 LEDs, since at least 88E1340 support
>   6 LEDs
> - almost every LED supports some mode which is not supported by at least one
>   other LED
> - this leads to the following dillema:
>   1. either we support one trigger type across the driver, but then the
>      /sys/class/leds/<LED>/trigger file will also list HW triggers not
>      supported by this specific LED,
>   2. or we add 6 trigger types, each different one LED, and register up to
>      8 HW triggers for each trigger type, which results in up to 48 triggers
>      per this driver.
>   In this proposal alternative 1 is taken and when unsupported HW trigger
>   is requested by writing to /sys/class/leds/<LED>/trigger file, the write
>   system call returns -EOPNOTSUPP.
> - therefore I think that this is not the correct way how to implement
>   LED-private triggers, and instead an approach as desribed in [1].
>   What do you people think?

This seems easily solvable by using sysfs attributes for specific hw mode
configuration.

This would result in having one private trigger registered with all the special
casing being done in your sysfs *_store/show functions that could be made per LED,
because sysfs functions get a reference to led_classdev.

You can then apply any kind of constraints fairly easily in sysfs interface
for your hw trigger.

See my driver: https://megous.com/git/linux/commit/?h=tbs-a711-5.8&id=a4da688b5d48011777d1e870a7d2b42edc9d144f

I guess unless you really want to have all possible configureations exposed
as differently named triggers via the led/trigger sysfs interface. But as you
see it makes things complicated.

regards,
	o.


> Marek
> 
> Marek Behún (3):
>   leds: trigger: add support for LED-private device triggers
>   leds: trigger: return error value if .activate() failed
>   net: phy: marvell: add support for PHY LEDs via LED class
> 
>  drivers/leds/led-triggers.c |  32 ++--
>  drivers/net/phy/Kconfig     |   7 +
>  drivers/net/phy/marvell.c   | 307 +++++++++++++++++++++++++++++++++++-
>  include/linux/leds.h        |  10 ++
>  4 files changed, 346 insertions(+), 10 deletions(-)
> 
> -- 
> 2.26.2
> 

Re: [PATCH RFC leds + net-next 0/3] Add support for LEDs on Marvell PHYs

[Andrew Lunn]

On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote:
> Hello,
> 
> this RFC series should apply on both net-next/master and Pavel's
> linux-leds/for-master tree.
> 
> This adds support for LED's connected to some Marvell PHYs.
> 
> LEDs are specified via device-tree. Example:

Hi Marek

I've been playing with something similar, off and on, mostly off.

Take a look at

https://github.com/lunn/linux v5.4-rc6-hw-led-triggers

The binding i have is pretty much the same, since we are both
following the common LED binding. I see no problems with this.

> This is achieved by extending the LED trigger API with LED-private triggers.
> The proposal for this is based on work by Ondrej and Pavel.

So what i did here was allow triggers to be registered against a
specific LED. The /sys/class/leds/<LED>/trigger lists both the generic
triggers and the triggers for this specific LED. Phylib can then
register a trigger for each blink reason that specific LED can
perform. Which does result in a lot of triggers. Especially when you
start talking about a 10 port switch each with 2 LEDs.

I still have some open issues...

1) Polarity. It would be nice to be able to configure the polarity of
the LED in the bindings.

2) PHY LEDs which are not actually part of the PHY. Most of the
Marvell Ethernet switches have inbuilt PHYs, which are driven by the
Marvell PHY driver. The Marvell PHY driver has no idea the PHY is
inside a switch, it is just a PHY.  However, the LEDs are not
controlled via PHY registers, but Switch registers. So the switch
driver is going to end up controlling these LEDs. It would be good to
be able to share as much code as possible, keep the naming consistent,
and keep the user API the same.

3) Some PHYs cannot control the LEDs independently. Or they have modes
which configure two or more LEDs. The Marvell PHYs are like
this. There are something like ~10 blink modes which are
independent. And then there are 4 modes which control multiple LEDs.
There is no simple way to support this with Linux LEDs which assume
the LEDs are fully independent. I suspect we simply cannot support
these combined modes.

As a PHY maintainer, i would like to see a solution which makes use of
Linux LEDs. I don't really care who's code it is, and feel free to
borrow my code, or ideas, or ignore it.

      Andrew

Re: [PATCH RFC leds + net-next 2/3] leds: trigger: return error value if .activate() failed

[Pavel Machek]

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

Hi!

> Currently when the .activate() method fails and returns a negative error
> code while writing to the /sys/class/leds/<LED>/trigger file, the write
> system call does not inform the user abouth this failure.
> 
> This patch fixes this.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/leds/led-triggers.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 81e758d5a048..804e0d624f47 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -40,7 +40,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>  	struct led_trigger *trig;
> -	int ret = count;
> +	int ret;
>

Please check the code. AFAICT you need ret = 0 here.

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 --]

Re: [PATCH RFC leds + net-next 1/3] leds: trigger: add support for LED-private device triggers

[Pavel Machek]

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

Hi!

> Some LED controllers may come with an internal HW triggering mechanism
> for the LED and the ability to switch between SW control and the
> internal HW control. This includes most PHYs, various ethernet switches,
> the Turris Omnia LED controller or AXP20X PMIC.
> 
> This adds support for registering such triggers.
> 
> This code is based on work by Pavel Machek <pavel@ucw.cz> and
> Ondřej Jirman <megous@megous.com>.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Looks good to me. I'll likely apply it soon...

Best regards,
								Pavel


> ---
>  drivers/leds/led-triggers.c | 26 ++++++++++++++++++++------
>  include/linux/leds.h        | 10 ++++++++++
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..81e758d5a048 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
>  
>   /* Used by LED Class */
>  
> +static inline bool
> +trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> +{
> +	return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
> +}
> +
>  ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>  			  struct bin_attribute *bin_attr, char *buf,
>  			  loff_t pos, size_t count)
> @@ -50,7 +56,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>  
>  	down_read(&triggers_list_lock);
>  	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		if (sysfs_streq(buf, trig->name)) {
> +		if (sysfs_streq(buf, trig->name) && trigger_relevant(led_cdev, trig)) {
>  			down_write(&led_cdev->trigger_lock);
>  			led_trigger_set(led_cdev, trig);
>  			up_write(&led_cdev->trigger_lock);
> @@ -93,8 +99,12 @@ static int led_trigger_format(char *buf, size_t size,
>  				       led_cdev->trigger ? "none" : "[none]");
>  
>  	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		bool hit = led_cdev->trigger &&
> -			!strcmp(led_cdev->trigger->name, trig->name);
> +		bool hit;
> +
> +		if (!trigger_relevant(led_cdev, trig))
> +			continue;
> +
> +		hit = led_cdev->trigger && !strcmp(led_cdev->trigger->name, trig->name);
>  
>  		len += led_trigger_snprintf(buf + len, size - len,
>  					    " %s%s%s", hit ? "[" : "",
> @@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
>  	down_read(&triggers_list_lock);
>  	down_write(&led_cdev->trigger_lock);
>  	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		if (!strcmp(led_cdev->default_trigger, trig->name)) {
> +		if (!strcmp(led_cdev->default_trigger, trig->name) &&
> +		    trigger_relevant(led_cdev, trig)) {
>  			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
>  			led_trigger_set(led_cdev, trig);
>  			break;
> @@ -280,7 +291,9 @@ int led_trigger_register(struct led_trigger *trig)
>  	down_write(&triggers_list_lock);
>  	/* Make sure the trigger's name isn't already in use */
>  	list_for_each_entry(_trig, &trigger_list, next_trig) {
> -		if (!strcmp(_trig->name, trig->name)) {
> +		if (!strcmp(_trig->name, trig->name) &&
> +		    (trig->trigger_type == _trig->trigger_type ||
> +		     !trig->trigger_type || !_trig->trigger_type)) {
>  			up_write(&triggers_list_lock);
>  			return -EEXIST;
>  		}
> @@ -294,7 +307,8 @@ int led_trigger_register(struct led_trigger *trig)
>  	list_for_each_entry(led_cdev, &leds_list, node) {
>  		down_write(&led_cdev->trigger_lock);
>  		if (!led_cdev->trigger && led_cdev->default_trigger &&
> -			    !strcmp(led_cdev->default_trigger, trig->name)) {
> +		    !strcmp(led_cdev->default_trigger, trig->name) &&
> +		    trigger_relevant(led_cdev, trig)) {
>  			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
>  			led_trigger_set(led_cdev, trig);
>  		}
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..6a8d6409c993 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
>  	bool devname_mandatory;
>  };
>  
> +struct led_hw_trigger_type {
> +	int dummy;
> +};
> +
>  struct led_classdev {
>  	const char		*name;
>  	enum led_brightness	 brightness;
> @@ -141,6 +145,9 @@ struct led_classdev {
>  	void			*trigger_data;
>  	/* true if activated - deactivate routine uses it to do cleanup */
>  	bool			activated;
> +
> +	/* LEDs that have private triggers have this set */
> +	struct led_hw_trigger_type	*trigger_type;
>  #endif
>  
>  #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> @@ -345,6 +352,9 @@ struct led_trigger {
>  	int		(*activate)(struct led_classdev *led_cdev);
>  	void		(*deactivate)(struct led_classdev *led_cdev);
>  
> +	/* LED-private triggers have this set */
> +	struct led_hw_trigger_type *trigger_type;
> +
>  	/* LEDs under control by this trigger (for simple triggers) */
>  	rwlock_t	  leddev_list_lock;
>  	struct list_head  led_cdevs;

-- 
(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 --]

Re: [PATCH RFC leds + net-next 1/3] leds: trigger: add support for LED-private device triggers

[Jacek Anaszewski]

Hi Marek,

On 7/16/20 7:17 PM, Marek Behún wrote:
> Some LED controllers may come with an internal HW triggering mechanism
> for the LED and the ability to switch between SW control and the
> internal HW control. This includes most PHYs, various ethernet switches,
> the Turris Omnia LED controller or AXP20X PMIC.
> 
> This adds support for registering such triggers.
> 
> This code is based on work by Pavel Machek <pavel@ucw.cz> and
> Ondřej Jirman <megous@megous.com>.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>   drivers/leds/led-triggers.c | 26 ++++++++++++++++++++------
>   include/linux/leds.h        | 10 ++++++++++
>   2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2cb7a5..81e758d5a048 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,6 +27,12 @@ LIST_HEAD(trigger_list);
>   
>    /* Used by LED Class */
>   
> +static inline bool
> +trigger_relevant(struct led_classdev *led_cdev, struct led_trigger *trig)
> +{
> +	return !trig->trigger_type || trig->trigger_type == led_cdev->trigger_type;
> +}
> +
>   ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>   			  struct bin_attribute *bin_attr, char *buf,
>   			  loff_t pos, size_t count)
> @@ -50,7 +56,7 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>   
>   	down_read(&triggers_list_lock);
>   	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		if (sysfs_streq(buf, trig->name)) {
> +		if (sysfs_streq(buf, trig->name) && trigger_relevant(led_cdev, trig)) {
>   			down_write(&led_cdev->trigger_lock);
>   			led_trigger_set(led_cdev, trig);
>   			up_write(&led_cdev->trigger_lock);
> @@ -93,8 +99,12 @@ static int led_trigger_format(char *buf, size_t size,
>   				       led_cdev->trigger ? "none" : "[none]");
>   
>   	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		bool hit = led_cdev->trigger &&
> -			!strcmp(led_cdev->trigger->name, trig->name);
> +		bool hit;
> +
> +		if (!trigger_relevant(led_cdev, trig))
> +			continue;
> +
> +		hit = led_cdev->trigger && !strcmp(led_cdev->trigger->name, trig->name);
>   
>   		len += led_trigger_snprintf(buf + len, size - len,
>   					    " %s%s%s", hit ? "[" : "",
> @@ -243,7 +253,8 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
>   	down_read(&triggers_list_lock);
>   	down_write(&led_cdev->trigger_lock);
>   	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		if (!strcmp(led_cdev->default_trigger, trig->name)) {
> +		if (!strcmp(led_cdev->default_trigger, trig->name) &&
> +		    trigger_relevant(led_cdev, trig)) {
>   			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
>   			led_trigger_set(led_cdev, trig);
>   			break;
> @@ -280,7 +291,9 @@ int led_trigger_register(struct led_trigger *trig)
>   	down_write(&triggers_list_lock);
>   	/* Make sure the trigger's name isn't already in use */
>   	list_for_each_entry(_trig, &trigger_list, next_trig) {
> -		if (!strcmp(_trig->name, trig->name)) {
> +		if (!strcmp(_trig->name, trig->name) &&
> +		    (trig->trigger_type == _trig->trigger_type ||
> +		     !trig->trigger_type || !_trig->trigger_type)) {
>   			up_write(&triggers_list_lock);
>   			return -EEXIST;
>   		}
> @@ -294,7 +307,8 @@ int led_trigger_register(struct led_trigger *trig)
>   	list_for_each_entry(led_cdev, &leds_list, node) {
>   		down_write(&led_cdev->trigger_lock);
>   		if (!led_cdev->trigger && led_cdev->default_trigger &&
> -			    !strcmp(led_cdev->default_trigger, trig->name)) {
> +		    !strcmp(led_cdev->default_trigger, trig->name) &&
> +		    trigger_relevant(led_cdev, trig)) {
>   			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
>   			led_trigger_set(led_cdev, trig);
>   		}
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 2451962d1ec5..6a8d6409c993 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -57,6 +57,10 @@ struct led_init_data {
>   	bool devname_mandatory;
>   };
>   
> +struct led_hw_trigger_type {
> +	int dummy;
> +};
> +
>   struct led_classdev {
>   	const char		*name;
>   	enum led_brightness	 brightness;
> @@ -141,6 +145,9 @@ struct led_classdev {
>   	void			*trigger_data;
>   	/* true if activated - deactivate routine uses it to do cleanup */
>   	bool			activated;
> +
> +	/* LEDs that have private triggers have this set */
> +	struct led_hw_trigger_type	*trigger_type;
>   #endif
>   
>   #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
> @@ -345,6 +352,9 @@ struct led_trigger {
>   	int		(*activate)(struct led_classdev *led_cdev);
>   	void		(*deactivate)(struct led_classdev *led_cdev);
>   
> +	/* LED-private triggers have this set */
> +	struct led_hw_trigger_type *trigger_type;
> +
>   	/* LEDs under control by this trigger (for simple triggers) */
>   	rwlock_t	  leddev_list_lock;
>   	struct list_head  led_cdevs;

Looks good to me:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC leds + net-next 1/3] leds: trigger: add support for LED-private device triggers

[Pavel Machek]

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

Hi!

> >This adds support for registering such triggers.
> >
> >This code is based on work by Pavel Machek <pavel@ucw.cz> and
> >Ondřej Jirman <megous@megous.com>.
> >
> >Signed-off-by: Marek Behún <marek.behun@nic.cz>

> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>


Thanks, applied.
							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 --]

Re: [PATCH RFC leds + net-next 0/3] Add support for LEDs on Marvell PHYs

[Marek Behún]

On Thu, 16 Jul 2020 20:56:47 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote:
> > Hello,
> > 
> > this RFC series should apply on both net-next/master and Pavel's
> > linux-leds/for-master tree.
> > 
> > This adds support for LED's connected to some Marvell PHYs.
> > 
> > LEDs are specified via device-tree. Example:  
> 
> Hi Marek
> 
> I've been playing with something similar, off and on, mostly off.
> 
> Take a look at
> 
> https://github.com/lunn/linux v5.4-rc6-hw-led-triggers
> 
> The binding i have is pretty much the same, since we are both
> following the common LED binding. I see no problems with this.
> 
> > This is achieved by extending the LED trigger API with LED-private
> > triggers. The proposal for this is based on work by Ondrej and
> > Pavel.  
> 
> So what i did here was allow triggers to be registered against a
> specific LED. The /sys/class/leds/<LED>/trigger lists both the generic
> triggers and the triggers for this specific LED. Phylib can then
> register a trigger for each blink reason that specific LED can
> perform. Which does result in a lot of triggers. Especially when you
> start talking about a 10 port switch each with 2 LEDs.
> 
> I still have some open issues...
>

Hi Andrew,

Pavel Machek has applied support for LED private triggers yesterday,
see
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d

The way this is handled has this issue - it results in a lot of
triggers, if we want each possible control to have its own trigger in
the sysfs trigger file. But as Ondrej pointed out, we can just register
one "hw-control" device trigger, and have its activation create another
file/files via which the user can select which type of HW control he
wants to activate. Something similar is done in netdev trigger.

I don't like it much, but this is what can be done if we want to
avoid having lots of triggers registered.

> 1) Polarity. It would be nice to be able to configure the polarity of
> the LED in the bindings.

Yes, and also the DUAL mode and everything else.

> 2) PHY LEDs which are not actually part of the PHY. Most of the
> Marvell Ethernet switches have inbuilt PHYs, which are driven by the
> Marvell PHY driver. The Marvell PHY driver has no idea the PHY is
> inside a switch, it is just a PHY.  However, the LEDs are not
> controlled via PHY registers, but Switch registers. So the switch
> driver is going to end up controlling these LEDs. It would be good to
> be able to share as much code as possible, keep the naming consistent,
> and keep the user API the same.

I know about this - in fact I want this solved for Turris MOX, which
has one 1518 PHY and can have up to three switches connected - I want
every LED to be configurable by userspace.

The internal PHY of the switch can be identified in the marvell phy
driver not to register any LEDs. Then the switch LEDs can be controlled
by the switch driver. I don't think we are able to share much of the
code, since the access to these registers is different from the LED
registers in the PHY, and register values are also different. The only
think which could be shared are names of the trigger, I think. But I
will look into this and prepare a patch series that will share as much
code as is reasonable.

> 3) Some PHYs cannot control the LEDs independently. Or they have modes
> which configure two or more LEDs. The Marvell PHYs are like
> this. There are something like ~10 blink modes which are
> independent. And then there are 4 modes which control multiple LEDs.
> There is no simple way to support this with Linux LEDs which assume
> the LEDs are fully independent. I suspect we simply cannot support
> these combined modes.

I know about these modes, I have func specs for several differnet
Marvell PHYs and switches opened and have read about the LED systems.
I intend to do this so that all corner cases are considere.

> As a PHY maintainer, i would like to see a solution which makes use of
> Linux LEDs. I don't really care who's code it is, and feel free to
> borrow my code, or ideas, or ignore it.
> 
>       Andrew

Wait a few days and I shall send another proposal.

Marek