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

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

[Marek Behún]

Hi,

this is v3 of my RFC adding support for LEDs connected to Marvell PHYs.

Please note that if you want to test this, you still need to first apply
the patch adding the LED private triggers support from Pavel's tree.
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d

Changes since 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

Marek Behún (2):
  net: phy: add API for LEDs controlled by PHY HW
  net: phy: marvell: add support for PHY LEDs via LED class

 drivers/net/phy/Kconfig           |  10 +
 drivers/net/phy/Makefile          |   1 +
 drivers/net/phy/marvell.c         | 364 ++++++++++++++++++++++++++++++
 drivers/net/phy/phy_hw_led_mode.c |  96 ++++++++
 include/linux/phy.h               |  15 ++
 5 files changed, 486 insertions(+)
 create mode 100644 drivers/net/phy/phy_hw_led_mode.c

-- 
2.26.2

[PATCH RFC leds + net-next v3 1/2] net: phy: add API for LEDs controlled by PHY HW

[Marek Behún]

Many PHYs support various HW control modes for LEDs connected directly
to them.

This code adds a new private LED trigger called phydev-hw-mode. When
this trigger is enabled for a LED, the various HW control modes which
the PHY supports for given LED can be get/set via hw_mode sysfs file.

A PHY driver wishing to utilize this API needs to register the LEDs on
its own and set the .trigger_type member of LED classdev to
&phy_hw_led_trig_type. It also needs to implement the methods
.led_iter_hw_mode, .led_set_hw_mode and .led_get_hw_mode in struct
phydev.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/Kconfig           |  9 +++
 drivers/net/phy/Makefile          |  1 +
 drivers/net/phy/phy_hw_led_mode.c | 96 +++++++++++++++++++++++++++++++
 include/linux/phy.h               | 15 +++++
 4 files changed, 121 insertions(+)
 create mode 100644 drivers/net/phy/phy_hw_led_mode.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index dd20c2c27c2f..ffea11f73acd 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -283,6 +283,15 @@ config LED_TRIGGER_PHY
 		<Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link
 		for any speed known to the PHY.
 
+config LED_TRIGGER_PHY_HW
+	bool "Support HW LED control modes"
+	depends on LEDS_TRIGGERS
+	help
+	  Many PHYs can control blinking of LEDs connected directly to them.
+	  This adds a special LED trigger called phydev-hw-mode. When enabled,
+	  the various control modes supported by the PHY on given LED can be
+	  chosen via hw_mode sysfs file.
+
 
 comment "MII PHY device drivers"
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index d84bab489a53..fd0253ab8097 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_LED_TRIGGER_PHY_HW)	+= phy_hw_led_mode.o
 
 obj-$(CONFIG_PHYLINK)		+= phylink.o
 obj-$(CONFIG_PHYLIB)		+= libphy.o
diff --git a/drivers/net/phy/phy_hw_led_mode.c b/drivers/net/phy/phy_hw_led_mode.c
new file mode 100644
index 000000000000..b4c2f25266a5
--- /dev/null
+++ b/drivers/net/phy/phy_hw_led_mode.c
@@ -0,0 +1,96 @@
+// 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/phy.h>
+
+static void phy_hw_led_trig_deactivate(struct led_classdev *cdev)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	int ret;
+
+	ret = phydev->drv->led_set_hw_mode(phydev, cdev, NULL);
+	if (ret < 0) {
+		phydev_err(phydev, "failed deactivating HW mode on LED %s\n", cdev->name);
+		return;
+	}
+}
+
+static ssize_t hw_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *cdev = led_trigger_get_led(dev);
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	const char *mode, *cur_mode;
+	void *iter = NULL;
+	int len = 0;
+
+	cur_mode = phydev->drv->led_get_hw_mode(phydev, cdev);
+
+	for (mode = phydev->drv->led_iter_hw_mode(phydev, cdev, &iter);
+	     mode;
+	     mode = phydev->drv->led_iter_hw_mode(phydev, cdev, &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';
+
+	return len;
+}
+
+static ssize_t hw_mode_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			     size_t count)
+{
+	struct led_classdev *cdev = led_trigger_get_led(dev);
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	int ret;
+
+	ret = phydev->drv->led_set_hw_mode(phydev, cdev, buf);
+	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",
+	.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);
+
+static int __init phy_led_triggers_init(void)
+{
+	return led_trigger_register(&phy_hw_led_trig);
+}
+
+module_init(phy_led_triggers_init);
+
+static void __exit phy_led_triggers_exit(void)
+{
+	led_trigger_unregister(&phy_hw_led_trig);
+}
+
+module_exit(phy_led_triggers_exit);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0403eb799913..3abaed18f63d 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>
@@ -736,6 +737,14 @@ 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);
+
+#if IS_ENABLED(CONFIG_LED_TRIGGER_PHY_HW)
+	/* PHY LED HW modes support */
+	const char *(*led_iter_hw_mode)(struct phy_device *dev, struct led_classdev *cdev,
+					void **	iter);
+	int (*led_set_hw_mode)(struct phy_device *dev, struct led_classdev *cdev, const char *mode);
+	const char *(*led_get_hw_mode)(struct phy_device *dev, struct led_classdev *cdev);
+#endif
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
@@ -747,6 +756,12 @@ struct phy_driver {
 #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
 #define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)
 
+#if IS_ENABLED(CONFIG_LED_TRIGGER_PHY_HW)
+/* PHY LED HW mode private trigger */
+extern struct led_hw_trigger_type phy_hw_led_trig_type;
+extern struct led_trigger phy_hw_led_trig;
+#endif
+
 /* A Structure for boards to register fixups with the PHY Lib */
 struct phy_fixup {
 	struct list_head list;
-- 
2.26.2

Re: [PATCH RFC leds + net-next v3 1/2] net: phy: add API for LEDs controlled by PHY HW

[Pavel Machek]

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

Hi!

> Many PHYs support various HW control modes for LEDs connected directly
> to them.
> 
> This code adds a new private LED trigger called phydev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> the PHY supports for given LED can be get/set via hw_mode sysfs file.
> 
> A PHY driver wishing to utilize this API needs to register the LEDs on
> its own and set the .trigger_type member of LED classdev to
> &phy_hw_led_trig_type. It also needs to implement the methods
> .led_iter_hw_mode, .led_set_hw_mode and .led_get_hw_mode in struct
> phydev.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Nothing too wrong.

New sysfs file will require documentation.

Plus I wonder: should we have single hw_mode file? It seems many
different "bits" fit inside. Would it be possible to split it further,
and have bits saying:

"I want the LED to be on if link is 10Mbps".
"I want the LED to be on if link is 100Mbps".
"I want the LED to be on if link is 1000Mbps".
"I want the LED to blink on tx".
"I want the LED to blink on rx".

?

+       { "1Gbps/100Mbps/10Mbps",       { 0x2,  -1,  -1,  -1,  -1,
+       { "1Gbps",                      { 0x7,  -1,  -1,  -1,  -1,
+       { "100Mbps-fiber",              {  -1, 0x5,  -1,  -1,  -1,
+       { "100Mbps-10Mbps",             {  -1, 0x5,  -1,  -1,  -1,
+       { "1Gbps-100Mbps",              {  -1, 0x6,  -1,  -1,  -1,
+       { "1Gbps-10Mbps",               {  -1,  -1, 0x6, 0x6,  -1,
+       { "100Mbps",                    {  -1, 0x7,  -1,  -1,  -1,
+       { "10Mbps",                     {  -1,  -1, 0x7,  -1,  -1,

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 v3 1/2] net: phy: add API for LEDs controlled by PHY HW

[Marek Behun]

On Sat, 25 Jul 2020 11:21:24 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > Many PHYs support various HW control modes for LEDs connected directly
> > to them.
> > 
> > This code adds a new private LED trigger called phydev-hw-mode. When
> > this trigger is enabled for a LED, the various HW control modes which
> > the PHY supports for given LED can be get/set via hw_mode sysfs file.
> > 
> > A PHY driver wishing to utilize this API needs to register the LEDs on
> > its own and set the .trigger_type member of LED classdev to
> > &phy_hw_led_trig_type. It also needs to implement the methods
> > .led_iter_hw_mode, .led_set_hw_mode and .led_get_hw_mode in struct
> > phydev.
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>  
> 
> Nothing too wrong.
> 
> New sysfs file will require documentation.
> 
> Plus I wonder: should we have single hw_mode file? It seems many
> different "bits" fit inside. Would it be possible to split it further,
> and have bits saying:
> 
> "I want the LED to be on if link is 10Mbps".
> "I want the LED to be on if link is 100Mbps".
> "I want the LED to be on if link is 1000Mbps".
> "I want the LED to blink on tx".
> "I want the LED to blink on rx".
> 
> ?

I don't think this is possible. Only specific combinations are possible
on Marvell PHYs. There is no HW control mode which would do, for
example:
  - ON when linked to 100Mbps
  - BLINK when receive

PHYs from other vendors can different mode sets.

Marek

> +       { "1Gbps/100Mbps/10Mbps",       { 0x2,  -1,  -1,  -1,  -1,
> +       { "1Gbps",                      { 0x7,  -1,  -1,  -1,  -1,
> +       { "100Mbps-fiber",              {  -1, 0x5,  -1,  -1,  -1,
> +       { "100Mbps-10Mbps",             {  -1, 0x5,  -1,  -1,  -1,
> +       { "1Gbps-100Mbps",              {  -1, 0x6,  -1,  -1,  -1,
> +       { "1Gbps-10Mbps",               {  -1,  -1, 0x6, 0x6,  -1,
> +       { "100Mbps",                    {  -1, 0x7,  -1,  -1,  -1,
> +       { "10Mbps",                     {  -1,  -1, 0x7,  -1,  -1,
> 
> Best regards,
> 									Pavel
> 

Re: [PATCH RFC leds + net-next v3 1/2] net: phy: add API for LEDs controlled by PHY HW

[Andrew Lunn]

On Fri, Jul 24, 2020 at 06:46:02PM +0200, Marek Behún wrote:
> Many PHYs support various HW control modes for LEDs connected directly
> to them.
> 
> This code adds a new private LED trigger called phydev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> the PHY supports for given LED can be get/set via hw_mode sysfs file.
> 
> A PHY driver wishing to utilize this API needs to register the LEDs on
> its own and set the .trigger_type member of LED classdev to
> &phy_hw_led_trig_type. It also needs to implement the methods
> .led_iter_hw_mode, .led_set_hw_mode and .led_get_hw_mode in struct
> phydev.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/net/phy/Kconfig           |  9 +++
>  drivers/net/phy/Makefile          |  1 +
>  drivers/net/phy/phy_hw_led_mode.c | 96 +++++++++++++++++++++++++++++++
>  include/linux/phy.h               | 15 +++++
>  4 files changed, 121 insertions(+)
>  create mode 100644 drivers/net/phy/phy_hw_led_mode.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index dd20c2c27c2f..ffea11f73acd 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -283,6 +283,15 @@ config LED_TRIGGER_PHY
>  		<Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link
>  		for any speed known to the PHY.
>  
> +config LED_TRIGGER_PHY_HW
> +	bool "Support HW LED control modes"
> +	depends on LEDS_TRIGGERS
> +	help
> +	  Many PHYs can control blinking of LEDs connected directly to them.
> +	  This adds a special LED trigger called phydev-hw-mode. When enabled,
> +	  the various control modes supported by the PHY on given LED can be
> +	  chosen via hw_mode sysfs file.
> +
>  
>  comment "MII PHY device drivers"
>  
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index d84bab489a53..fd0253ab8097 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_LED_TRIGGER_PHY_HW)	+= phy_hw_led_mode.o
>  
>  obj-$(CONFIG_PHYLINK)		+= phylink.o
>  obj-$(CONFIG_PHYLIB)		+= libphy.o
> diff --git a/drivers/net/phy/phy_hw_led_mode.c b/drivers/net/phy/phy_hw_led_mode.c
> new file mode 100644
> index 000000000000..b4c2f25266a5
> --- /dev/null
> +++ b/drivers/net/phy/phy_hw_led_mode.c
> @@ -0,0 +1,96 @@
> +// 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/phy.h>
> +
> +static void phy_hw_led_trig_deactivate(struct led_classdev *cdev)
> +{
> +	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
> +	int ret;
> +
> +	ret = phydev->drv->led_set_hw_mode(phydev, cdev, NULL);
> +	if (ret < 0) {
> +		phydev_err(phydev, "failed deactivating HW mode on LED %s\n", cdev->name);
> +		return;
> +	}

The core holds the phydev mutex when calling into the driver. There
are a few exceptions, but it would be good if all the LED calls into
the driver also held the mutex.

> +}
> +
> +static ssize_t hw_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *cdev = led_trigger_get_led(dev);
> +	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
> +	const char *mode, *cur_mode;
> +	void *iter = NULL;
> +	int len = 0;

Reverse christmas tree.  

> +static int __init phy_led_triggers_init(void)
> +{
> +	return led_trigger_register(&phy_hw_led_trig);
> +}
> +
> +module_init(phy_led_triggers_init);
> +
> +static void __exit phy_led_triggers_exit(void)
> +{
> +	led_trigger_unregister(&phy_hw_led_trig);
> +}
> +
> +module_exit(phy_led_triggers_exit);

It is a bit of a surprise to find the module init/exit calls here, and
not in phy.c. I think they should be moved.

    Andrew

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

[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.

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

This patch 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, nor are LED polarity settings.

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

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index ffea11f73acd..5428a8af26d2 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -462,6 +462,7 @@ config LXT_PHY
 
 config MARVELL_PHY
 	tristate "Marvell PHYs"
+	depends on LED_TRIGGER_PHY_HW
 	help
 	  Currently has a driver for the 88E1011S
 
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd092..08cd79a1aa8d 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");
@@ -271,10 +279,19 @@ static struct marvell_hw_stat marvell_hw_stats[] = {
 	{ "phy_receive_errors_fiber", 1, 21, 16},
 };
 
+struct marvell_phy_led {
+	struct led_classdev cdev;
+	u8 idx;
+};
+
+#define to_marvell_phy_led(l)	container_of(l, struct marvell_phy_led, cdev)
+
 struct marvell_priv {
 	u64 stats[ARRAY_SIZE(marvell_hw_stats)];
 	char *hwmon_name;
 	struct device *hwmon_dev;
+	struct marvell_phy_led leds[MARVELL_PHY_MAX_LEDS];
+	u32 led_flags;
 	bool cable_test_tdr;
 	u32 first;
 	u32 last;
@@ -662,6 +679,333 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+enum {
+	L1V0_RECV		= BIT(0),
+	L1V0_COPPER		= BIT(1),
+	L1V5_100_FIBER		= BIT(2),
+	L1V5_100_10		= BIT(3),
+	L2V2_INIT		= BIT(4),
+	L2V2_PTP		= BIT(5),
+	L2V2_DUPLEX		= BIT(6),
+	L3V0_FIBER		= BIT(7),
+	L3V0_LOS		= BIT(8),
+	L3V5_TRANS		= BIT(9),
+	L3V7_FIBER		= BIT(10),
+	L3V7_DUPLEX		= BIT(11),
+};
+
+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, }, 0 },
+	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
+	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },
+	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
+	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
+	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
+	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
+	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
+	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
+	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
+	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
+	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
+	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
+	{ "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, }, 0 },
+	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
+	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
+	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
+	{ "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, }, 0 },
+	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
+	{ "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 },
+	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
+	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
+};
+
+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, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER),
+	LED(1121R, 3, L1V5_100_10),
+	LED(1240,  6, L3V5_TRANS),
+	LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
+	LED(1510,  3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
+	LED(1545,  6, 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_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+	struct phy_device *phydev = to_phy_device(cdev->dev->parent);
+	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
+	u8 val;
+
+	/* don't do anything if HW control is enabled */
+	if (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->idx, val);
+}
+
+static inline bool is_valid_led_mode(struct marvell_priv *priv, struct marvell_phy_led *led,
+				     const struct marvell_led_mode_info *mode)
+{
+	return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags));
+}
+
+static const char *marvell_led_iter_hw_mode(struct phy_device *phydev, struct led_classdev *cdev,
+					    void **iter)
+{
+	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
+	const struct marvell_led_mode_info *mode = *iter;
+	struct marvell_priv *priv = phydev->priv;
+
+	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(priv, 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 led_classdev *cdev,
+				   const char *name)
+{
+	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
+	struct marvell_priv *priv = phydev->priv;
+	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(priv, led, mode))
+			continue;
+
+		if (sysfs_streq(name, mode->name))
+			return marvell_led_set_regval(phydev, led->idx, mode->regval[led->idx]);
+	}
+
+	return -EINVAL;
+}
+
+static const char *marvell_led_get_hw_mode(struct phy_device *phydev, struct led_classdev *cdev)
+{
+	struct marvell_phy_led *led = to_marvell_phy_led(cdev);
+	struct marvell_priv *priv = phydev->priv;
+	const struct marvell_led_mode_info *mode;
+	int i, regval;
+
+	regval = marvell_led_get_regval(phydev, led->idx);
+	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(priv, led, mode))
+			continue;
+
+		if (mode->regval[led->idx] == regval)
+			return mode->name;
+	}
+
+	return NULL;
+}
+
+static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds)
+{
+	struct marvell_priv *priv = phydev->priv;
+	struct led_init_data init_data = {};
+	struct marvell_phy_led *led;
+	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 >= nleds) {
+		phydev_err(phydev,
+			   "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n",
+			   np, reg, nleds - 1);
+		return -EINVAL;
+	}
+
+	led = &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 (led->cdev.max_brightness) {
+		phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np);
+		return -EEXIST;
+	}
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking = marvell_led_brightness_set;
+	led->cdev.trigger_type = &phy_hw_led_trig_type;
+	led->idx = reg;
+
+	of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
+
+	err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->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 marvell_priv *priv = phydev->priv;
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *leds, *led;
+	const struct marvell_leds_info *info = NULL;
+	int i;
+
+	/* some families don't support LED control in this driver yet */
+	if (!phydev->drv->led_set_hw_mode)
+		return;
+
+	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;
+
+	priv->led_flags = info->flags;
+
+#if 0
+	/*
+	 * TODO: here priv->led_flags should be changed so that hw_control values
+	 * for unsupported modes won't be shown. This cannot be deduced from
+	 * family only: for example the 88E1510 family contains 88E1510 which
+	 * does not support fiber, but also 88E1512, which supports fiber.
+	 */
+	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
+	}
+#endif
+
+	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, info->nleds);
+	}
+}
+
 static void marvell_config_led(struct phy_device *phydev)
 {
 	u16 def_config;
@@ -692,6 +1036,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)
@@ -2656,6 +3002,9 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2717,6 +3066,9 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1318S,
@@ -2796,6 +3148,9 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1116R,
@@ -2844,6 +3199,9 @@ 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_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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
@@ -2896,6 +3254,9 @@ 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_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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2964,6 +3325,9 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1548P,
-- 
2.26.2

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

[Pavel Machek]

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

Hi!

> +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> +	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, 0 },
> +	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> +	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },

is this "1Gbps-10Mbps"?

> +	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> +	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> +	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
> +	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
> +	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
> +	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
> +	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
> +	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
> +	{ "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, }, 0 },
> +	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
> +	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
> +	{ "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, }, 0 },
> +	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
> +	{ "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 },
> +	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
> +	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
> +};

Certainly more documentation will be required here, what "ptp" setting
does, for example, is not very obvious to me.

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 v3 2/2] net: phy: marvell: add support for PHY LEDs via LED class

[Marek Behun]

On Sat, 25 Jul 2020 11:23:39 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> > +	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, 0 },
> > +	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> > +	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },  
> 
> is this "1Gbps-10Mbps"?

Most of these modes mean "ON on event", eg
  "link" means ON when link up, else OFF. "
  "tx" means ON on when transmitting, else OFF
  "act" means ON when activity, else OFF
  "copper" means ON when copper link up, else OFF
but some are blinking modes
  "blink-act" means BLINK when activity, else OFF

Some modes can do ON and BLINK, these have one '/' in their name
  "link/act" means ON when link up, BLINK on activity, else OFF
  "link/rx" means ON when link up, BLINK on receive, else OFF

there is one mode, "1Gbps/100Mbps/10Mbps", which behaves differently:
  blinks 3 times when linked on 1Gbps
  blinks 2 times when linked on 100Mbps
  blinks 1 time when linked on 10Mbps
(and this blinking is repeating, ie blinks 3 times, pause, blinks 3 times,
pause)

Some modes are disjunctive:
  "100Mbps-fiber" means ON when linked on 100Mbps or via fiber, else OFF


> 
> > +	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> > +	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> > +	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
> > +	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
> > +	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
> > +	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
> > +	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
> > +	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
> > +	{ "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, }, 0 },
> > +	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
> > +	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
> > +	{ "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, }, 0 },
> > +	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
> > +	{ "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 },
> > +	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
> > +	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
> > +};  
> 
> Certainly more documentation will be required here, what "ptp" setting
> does, for example, is not very obvious to me.

"ptp" means it will light up when the PTP functionality is enabled on
the PHY and a PTP packet is received.

> Best regards,
> 									Pavel

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

[Andrew Lunn]

On Sat, Jul 25, 2020 at 11:34:50AM +0200, Marek Behun wrote:
> On Sat, 25 Jul 2020 11:23:39 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Hi!
> > 
> > > +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> > > +	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, 0 },
> > > +	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> > > +	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },  
> > 
> > is this "1Gbps-10Mbps"?
> 
> Most of these modes mean "ON on event", eg
>   "link" means ON when link up, else OFF. "
>   "tx" means ON on when transmitting, else OFF
>   "act" means ON when activity, else OFF
>   "copper" means ON when copper link up, else OFF
> but some are blinking modes
>   "blink-act" means BLINK when activity, else OFF
> 
> Some modes can do ON and BLINK, these have one '/' in their name
>   "link/act" means ON when link up, BLINK on activity, else OFF
>   "link/rx" means ON when link up, BLINK on receive, else OFF
> 
> there is one mode, "1Gbps/100Mbps/10Mbps", which behaves differently:
>   blinks 3 times when linked on 1Gbps
>   blinks 2 times when linked on 100Mbps
>   blinks 1 time when linked on 10Mbps
> (and this blinking is repeating, ie blinks 3 times, pause, blinks 3 times,
> pause)
> 
> Some modes are disjunctive:
>   "100Mbps-fiber" means ON when linked on 100Mbps or via fiber, else OFF

Hi Marek

I would be good to added this to the sysfs documentation.

> > > +	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> > > +	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> > > +	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
> > > +	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
> > > +	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
> > > +	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },

To be consistent these four probably should have the blink- prefix. Or
it could be /tx, /rx ?

> > > +	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > > +	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
> > > +	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > > +	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
> > > +	{ "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, }, 0 },
> > > +	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
> > > +	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
> > > +	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
> > > +	{ "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, }, 0 },
> > > +	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
> > > +	{ "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 },
> > > +	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
> > > +	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
> > > +};  
> > 
> > Certainly more documentation will be required here, what "ptp" setting
> > does, for example, is not very obvious to me.
> 
> "ptp" means it will light up when the PTP functionality is enabled on
> the PHY and a PTP packet is received.

Does hi-z mean off? In the implementation i did, i did not list off
and on as triggers. I instead used them for untriggered
brightness. That allowed the software triggers to work, so i had the
PHY blinking the heartbeat etc. But i had to make it optional, since a
quick survey of datasheets suggested not all PHYs support simple
on/off control.

Something beyond the scope of this patchset is implementing etHool -p

       -p --identify
              Initiates adapter-specific action intended to enable an operator to
	      easily identify the adapter by sight. Typically this involves  blink‐
              ing one or more LEDs on the specific network port.

If we have software controlled on/off, then a software trigger seems
like i good way to do this.

     Andrew

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

[Marek Behun]

On Sat, 25 Jul 2020 17:03:42 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> Does hi-z mean off? In the implementation i did, i did not list off
> and on as triggers. I instead used them for untriggered
> brightness. That allowed the software triggers to work, so i had the
> PHY blinking the heartbeat etc. But i had to make it optional, since a
> quick survey of datasheets suggested not all PHYs support simple
> on/off control.

I don't actually know what hi-z means, but enabling it disabled the LED.
But there is another register value for OFF...

> Something beyond the scope of this patchset is implementing etHool -p
> 
>        -p --identify
>               Initiates adapter-specific action intended to enable an operator to
> 	      easily identify the adapter by sight. Typically this involves  blink‐
>               ing one or more LEDs on the specific network port.
> 
> If we have software controlled on/off, then a software trigger seems
> like i good way to do this.

I'll look into this.

Marek

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

[Andrew Lunn]

On Sat, Jul 25, 2020 at 07:39:50PM +0200, Marek Behun wrote:
> On Sat, 25 Jul 2020 17:03:42 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Does hi-z mean off? In the implementation i did, i did not list off
> > and on as triggers. I instead used them for untriggered
> > brightness. That allowed the software triggers to work, so i had the
> > PHY blinking the heartbeat etc. But i had to make it optional, since a
> > quick survey of datasheets suggested not all PHYs support simple
> > on/off control.
> 
> I don't actually know what hi-z means, but enabling it disabled the LED.
> But there is another register value for OFF...

Can the pin be used for other things? Maybe it needs to be configured
for high impedance when it is used as a shared interrupt?  If it does
not seem like a useful LED setting, i would drop it for the moment.

> > Something beyond the scope of this patchset is implementing etHool -p
> > 
> >        -p --identify
> >               Initiates adapter-specific action intended to enable an operator to
> > 	      easily identify the adapter by sight. Typically this involves  blink‐
> >               ing one or more LEDs on the specific network port.
> > 
> > If we have software controlled on/off, then a software trigger seems
> > like i good way to do this.
> 
> I'll look into this.

No rush. As i said, out of scope for this patchset. Just keep it in
mind. It should be something we can implement once in phylib, and then
all phy drivers which have LED support get it for free.

    Andrew

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

[Andrew Lunn]

On Fri, Jul 24, 2020 at 06:46:03PM +0200, Marek Behún wrote:
> 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.
> 
> The code reads LEDs definitions from the device-tree node of the PHY.
> 
> This patch 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, nor are LED polarity settings.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/net/phy/Kconfig   |   1 +
>  drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 365 insertions(+)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index ffea11f73acd..5428a8af26d2 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -462,6 +462,7 @@ config LXT_PHY
>  
>  config MARVELL_PHY
>  	tristate "Marvell PHYs"
> +	depends on LED_TRIGGER_PHY_HW

Does it really depend on it? I think the driver will work fine without
it, just the LED control will be missing.

It is really a policy question. Cable test is always available, there
is no Kconfig'ury to stop it getting built. Is LED support really big
so that somebody might want to disable it? I think not. So lets just
enable it all the time.

>  	help
>  	  Currently has a driver for the 88E1011S
>  

> +enum {
> +	L1V0_RECV		= BIT(0),
> +	L1V0_COPPER		= BIT(1),
> +	L1V5_100_FIBER		= BIT(2),
> +	L1V5_100_10		= BIT(3),
> +	L2V2_INIT		= BIT(4),
> +	L2V2_PTP		= BIT(5),
> +	L2V2_DUPLEX		= BIT(6),
> +	L3V0_FIBER		= BIT(7),
> +	L3V0_LOS		= BIT(8),
> +	L3V5_TRANS		= BIT(9),
> +	L3V7_FIBER		= BIT(10),
> +	L3V7_DUPLEX		= BIT(11),

Maybe also add COMMON?

> +};
> +
> +struct marvell_led_mode_info {
> +	const char *name;
> +	s8 regval[MARVELL_PHY_MAX_LEDS];
> +	u32 flags;

Maybe give the enum a name, and use it here? It can be quite hard
tracking the meaning of flags in this code. Here, it is an indication
of an individual feature.

> +};
> +
> +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> +	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, 0 },

Replace flags 0 with COMMON?

> +	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> +	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> +	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> +	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
> +	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
> +	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
> +	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
> +	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
> +	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
> +	{ "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, }, 0 },
> +	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
> +	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
> +	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
> +	{ "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, }, 0 },
> +	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
> +	{ "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 },
> +	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
> +	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
> +};
> +
> +struct marvell_leds_info {
> +	u32 family;
> +	int nleds;
> +	u32 flags;

Here flags is a combination of all the features this specific PHY
supports.

> +};
> +
> +#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, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER),
> +	LED(1121R, 3, L1V5_100_10),
> +	LED(1240,  6, L3V5_TRANS),
> +	LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
> +	LED(1510,  3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
> +	LED(1545,  6, L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
> +};
> +
> +static inline int marvell_led_reg(int led)

No inline functions in C code. Let the compiler decide.

> +{
> +	switch (led) {
> +	case 0 ... 3:
> +		return MII_PHY_LED_CTRL;
> +	case 4 ... 5:
> +		return MII_PHY_LED45_CTRL;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static inline bool is_valid_led_mode(struct marvell_priv *priv, struct marvell_phy_led *led,
> +				     const struct marvell_led_mode_info *mode)

Same here, and anywhere else you might of used inline.

> +{
> +	return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags));

If you have COMMON, this gets simpler.

> +}
> +

> +static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds)
> +{
> +	struct marvell_priv *priv = phydev->priv;
> +	struct led_init_data init_data = {};
> +	struct marvell_phy_led *led;
> +	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 >= nleds) {
> +		phydev_err(phydev,
> +			   "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n",
> +			   np, reg, nleds - 1);
> +		return -EINVAL;
> +	}
> +
> +	led = &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

Code getting committed should not be using #if 0. Is the needed code
in the LED tree? Do we want to consider a stable branch of the LED
tree which DaveM can pull into net-next? Or do you want to wait until
the next merge cycle?

> +
> +	init_data.fwnode = &np->fwnode;
> +	init_data.devname_mandatory = true;
> +	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";

This we need to think about. Are you running this on a system with
systemd? Does the interface have a name like enp2s0? Does the LED get
registered before or after systemd renames it from eth0 to enp2s0?

> +
> +	if (led->cdev.max_brightness) {
> +		phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np);
> +		return -EEXIST;
> +	}
> +
> +	led->cdev.max_brightness = 1;
> +	led->cdev.brightness_set_blocking = marvell_led_brightness_set;
> +	led->cdev.trigger_type = &phy_hw_led_trig_type;
> +	led->idx = reg;
> +
> +	of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
> +
> +	err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
> +	if (err < 0) {
> +		phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err);
> +		return err;
> +	}

I don't like having the DT binding in the driver. We want all PHY
drivers to use the same binding, and not feel free to implement
whatever they want in their own driver. These are all standard
properties which all PHY drivers should be using. So lets move it into
phylib core. That also allows us to specify the properties in DT,
maybe just pulling in the core LED yaml stuff.

> +
> +	return 0;
> +}
> +
> +static void marvell_register_leds(struct phy_device *phydev)
> +{
> +	struct marvell_priv *priv = phydev->priv;
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *leds, *led;
> +	const struct marvell_leds_info *info = NULL;
> +	int i;

Reverse Christmas tree.

> +
> +	/* some families don't support LED control in this driver yet */
> +	if (!phydev->drv->led_set_hw_mode)
> +		return;
> +
> +	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;
> +
> +	priv->led_flags = info->flags;
> +
> +#if 0
> +	/*
> +	 * TODO: here priv->led_flags should be changed so that hw_control values
> +	 * for unsupported modes won't be shown. This cannot be deduced from
> +	 * family only: for example the 88E1510 family contains 88E1510 which
> +	 * does not support fiber, but also 88E1512, which supports fiber.
> +	 */
> +	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> +	}
> +#endif
> +
> +	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, info->nleds);
> +	}
> +}
> +

  Andrew

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

[Marek Behun]

On Sat, 25 Jul 2020 19:23:18 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Jul 24, 2020 at 06:46:03PM +0200, Marek Behún wrote:
> > 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.
> > 
> > The code reads LEDs definitions from the device-tree node of the PHY.
> > 
> > This patch 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, nor are LED polarity settings.
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/net/phy/Kconfig   |   1 +
> >  drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 365 insertions(+)
> > 
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index ffea11f73acd..5428a8af26d2 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -462,6 +462,7 @@ config LXT_PHY
> >  
> >  config MARVELL_PHY
> >  	tristate "Marvell PHYs"
> > +	depends on LED_TRIGGER_PHY_HW  
> 
> Does it really depend on it? I think the driver will work fine without
> it, just the LED control will be missing.
> 
> It is really a policy question. Cable test is always available, there
> is no Kconfig'ury to stop it getting built. Is LED support really big
> so that somebody might want to disable it? I think not. So lets just
> enable it all the time.

OK

> >  	help
> >  	  Currently has a driver for the 88E1011S
> >    
> 
> > +enum {
> > +	L1V0_RECV		= BIT(0),
> > +	L1V0_COPPER		= BIT(1),
> > +	L1V5_100_FIBER		= BIT(2),
> > +	L1V5_100_10		= BIT(3),
> > +	L2V2_INIT		= BIT(4),
> > +	L2V2_PTP		= BIT(5),
> > +	L2V2_DUPLEX		= BIT(6),
> > +	L3V0_FIBER		= BIT(7),
> > +	L3V0_LOS		= BIT(8),
> > +	L3V5_TRANS		= BIT(9),
> > +	L3V7_FIBER		= BIT(10),
> > +	L3V7_DUPLEX		= BIT(11),  
> 
> Maybe also add COMMON?

These are meant to be interpreted as flag bits, one LED can have
multiple flags. Most time in kernel bit fields use 0, not a constant,
to express no flag.

> > +};
> > +
> > +struct marvell_led_mode_info {
> > +	const char *name;
> > +	s8 regval[MARVELL_PHY_MAX_LEDS];
> > +	u32 flags;  
> 
> Maybe give the enum a name, and use it here? It can be quite hard
> tracking the meaning of flags in this code. Here, it is an indication
> of an individual feature.

led_flags?

> 
> > +};
> > +
> > +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> > +	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, 0 },  
> 
> Replace flags 0 with COMMON?
> 
> > +	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, 0 },
> > +	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, 0 },
> > +	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, 0 },
> > +	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, 0 },
> > +	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
> > +	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, 0 },
> > +	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
> > +	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
> > +	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, 0 },
> > +	{ "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, }, 0 },
> > +	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, 0 },
> > +	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, 0 },
> > +	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, 0 },
> > +	{ "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, }, 0 },
> > +	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, 0 },
> > +	{ "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 },
> > +	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, 0 },
> > +	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, 0 },
> > +};
> > +
> > +struct marvell_leds_info {
> > +	u32 family;
> > +	int nleds;
> > +	u32 flags;  
> 
> Here flags is a combination of all the features this specific PHY
> supports.
> 
> > +};
> > +
> > +#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, L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS | L3V7_FIBER),
> > +	LED(1121R, 3, L1V5_100_10),
> > +	LED(1240,  6, L3V5_TRANS),
> > +	LED(1340S, 6, L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
> > +	LED(1510,  3, L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
> > +	LED(1545,  6, L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
> > +};
> > +
> > +static inline int marvell_led_reg(int led)  
> 
> No inline functions in C code. Let the compiler decide.

OK

> 
> > +{
> > +	switch (led) {
> > +	case 0 ... 3:
> > +		return MII_PHY_LED_CTRL;
> > +	case 4 ... 5:
> > +		return MII_PHY_LED45_CTRL;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}  
> 
> > +static inline bool is_valid_led_mode(struct marvell_priv *priv, struct marvell_phy_led *led,
> > +				     const struct marvell_led_mode_info *mode)  
> 
> Same here, and anywhere else you might of used inline.
> 
> > +{
> > +	return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags));  
> 
> If you have COMMON, this gets simpler.

How? I need to check whether this specific LED mode has that specific
flag bit...

> > +}
> > +  
> 
> > +static int marvell_register_led(struct phy_device *phydev, struct device_node *np, int nleds)
> > +{
> > +	struct marvell_priv *priv = phydev->priv;
> > +	struct led_init_data init_data = {};
> > +	struct marvell_phy_led *led;
> > +	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 >= nleds) {
> > +		phydev_err(phydev,
> > +			   "LED node %pOF 'reg' property too large (%u, PHY supports max %u)\n",
> > +			   np, reg, nleds - 1);
> > +		return -EINVAL;
> > +	}
> > +
> > +	led = &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  
> 
> Code getting committed should not be using #if 0. Is the needed code
> in the LED tree? Do we want to consider a stable branch of the LED
> tree which DaveM can pull into net-next? Or do you want to wait until
> the next merge cycle?

That's why this is RFC. But yes, I would like to have this merged for
5.9, so maybe we should ask Dave. Is this common? Do we also need to
tell Pavel or how does this work?

> > +
> > +	init_data.fwnode = &np->fwnode;
> > +	init_data.devname_mandatory = true;
> > +	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";  
> 
> This we need to think about. Are you running this on a system with
> systemd? Does the interface have a name like enp2s0? Does the LED get
> registered before or after systemd renames it from eth0 to enp2s0?

Yes, well, this should be discussed also with LED guys. I don't suppose
that renaming the sysfs symlink on interface rename event is
appropriate, but who knows?
The interfaces are platform specific, on mvebu. They aren't connected
via PCI, so their names remain eth0, eth1 ...

> > +
> > +	if (led->cdev.max_brightness) {
> > +		phydev_err(phydev, "LED node %pOF 'reg' property collision with another LED\n", np);
> > +		return -EEXIST;
> > +	}
> > +
> > +	led->cdev.max_brightness = 1;
> > +	led->cdev.brightness_set_blocking = marvell_led_brightness_set;
> > +	led->cdev.trigger_type = &phy_hw_led_trig_type;
> > +	led->idx = reg;
> > +
> > +	of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
> > +
> > +	err = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);
> > +	if (err < 0) {
> > +		phydev_err(phydev, "Cannot register LED %pOF: %i\n", np, err);
> > +		return err;
> > +	}  
> 
> I don't like having the DT binding in the driver. We want all PHY
> drivers to use the same binding, and not feel free to implement
> whatever they want in their own driver. These are all standard
> properties which all PHY drivers should be using. So lets move it into
> phylib core. That also allows us to specify the properties in DT,
> maybe just pulling in the core LED yaml stuff.
> 

I also want this code to be generalized somehow so that it can be
reused. The problem is that I want to have support for DUAL mode, which
is Marvell specific, and a DUAL LED needs to be defined in device tree.

> > +
> > +	return 0;
> > +}
> > +
> > +static void marvell_register_leds(struct phy_device *phydev)
> > +{
> > +	struct marvell_priv *priv = phydev->priv;
> > +	struct device_node *node = phydev->mdio.dev.of_node;
> > +	struct device_node *leds, *led;
> > +	const struct marvell_leds_info *info = NULL;
> > +	int i;  
> 
> Reverse Christmas tree.
> 
> > +
> > +	/* some families don't support LED control in this driver yet */
> > +	if (!phydev->drv->led_set_hw_mode)
> > +		return;
> > +
> > +	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;
> > +
> > +	priv->led_flags = info->flags;
> > +
> > +#if 0
> > +	/*
> > +	 * TODO: here priv->led_flags should be changed so that hw_control values
> > +	 * for unsupported modes won't be shown. This cannot be deduced from
> > +	 * family only: for example the 88E1510 family contains 88E1510 which
> > +	 * does not support fiber, but also 88E1512, which supports fiber.
> > +	 */
> > +	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> > +	}
> > +#endif
> > +
> > +	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, info->nleds);
> > +	}
> > +}
> > +  
> 
>   Andrew

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

[Andrew Lunn]

On Sat, Jul 25, 2020 at 08:02:24PM +0200, Marek Behun wrote:
> On Sat, 25 Jul 2020 19:23:18 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Fri, Jul 24, 2020 at 06:46:03PM +0200, Marek Behún wrote:
> > > 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.
> > > 
> > > The code reads LEDs definitions from the device-tree node of the PHY.
> > > 
> > > This patch 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, nor are LED polarity settings.
> > > 
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  drivers/net/phy/Kconfig   |   1 +
> > >  drivers/net/phy/marvell.c | 364 ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 365 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > > index ffea11f73acd..5428a8af26d2 100644
> > > --- a/drivers/net/phy/Kconfig
> > > +++ b/drivers/net/phy/Kconfig
> > > @@ -462,6 +462,7 @@ config LXT_PHY
> > >  
> > >  config MARVELL_PHY
> > >  	tristate "Marvell PHYs"
> > > +	depends on LED_TRIGGER_PHY_HW  
> > 
> > Does it really depend on it? I think the driver will work fine without
> > it, just the LED control will be missing.
> > 
> > It is really a policy question. Cable test is always available, there
> > is no Kconfig'ury to stop it getting built. Is LED support really big
> > so that somebody might want to disable it? I think not. So lets just
> > enable it all the time.
> 
> OK
> 
> > >  	help
> > >  	  Currently has a driver for the 88E1011S
> > >    
> > 
> > > +enum {
> > > +	L1V0_RECV		= BIT(0),
> > > +	L1V0_COPPER		= BIT(1),
> > > +	L1V5_100_FIBER		= BIT(2),
> > > +	L1V5_100_10		= BIT(3),
> > > +	L2V2_INIT		= BIT(4),
> > > +	L2V2_PTP		= BIT(5),
> > > +	L2V2_DUPLEX		= BIT(6),
> > > +	L3V0_FIBER		= BIT(7),
> > > +	L3V0_LOS		= BIT(8),
> > > +	L3V5_TRANS		= BIT(9),
> > > +	L3V7_FIBER		= BIT(10),
> > > +	L3V7_DUPLEX		= BIT(11),  

        COMMON			= BIT(32),

> > > +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> > > +	{ "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 },
> > > +	{ "hi-z",			{ 0xa, 0xa, 0xa, 0xa, 0xa, 0xa, }, COMMON },
> > > +	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, COMMON },
> > > +};
> > > +

> > > +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 | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
> > > +};
> > > +

> > > +{
> > > +	return mode->regval[led->idx] != -1 && (!mode->flags || (priv->led_flags & mode->flags));  

This then becomes

return mode->regval[led->idx] != -1 && (priv->led_flags & mode->flags));  

       Andrew

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

[Andrew Lunn]

> > > +#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  
> > 
> > Code getting committed should not be using #if 0. Is the needed code
> > in the LED tree? Do we want to consider a stable branch of the LED
> > tree which DaveM can pull into net-next? Or do you want to wait until
> > the next merge cycle?
> 
> That's why this is RFC. But yes, I would like to have this merged for
> 5.9, so maybe we should ask Dave. Is this common? Do we also need to
> tell Pavel or how does this work?

The Pavel needs to create a stable branch. DaveM then merges that
branch into net-next. Your patches can then be merged. When Linus
pulls the two branches, led and net-next, git sees the exact same
patches twice, and simply drops them from the second pull request.

So you need to ask Pavel and DaveM if they are willing to do this.

> > > +	init_data.fwnode = &np->fwnode;
> > > +	init_data.devname_mandatory = true;
> > > +	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";  
> > 
> > This we need to think about. Are you running this on a system with
> > systemd? Does the interface have a name like enp2s0? Does the LED get
> > registered before or after systemd renames it from eth0 to enp2s0?
> 
> Yes, well, this should be discussed also with LED guys. I don't suppose
> that renaming the sysfs symlink on interface rename event is
> appropriate, but who knows?
> The interfaces are platform specific, on mvebu. They aren't connected
> via PCI, so their names remain eth0, eth1 ...

But the Marvell driver is used with more than just mvebu. And we need
this generic. There are USB Ethernet dongles which used phylib. They
will get their interfaces renamed to include the MAC address, etc.

It is possible to hook the notifier so we know when an interface is
renamed. We can then either destroy and re-create the LED, or if the
LED framework allows it, rename it.

Or we avoid interface names all together and stick with the phy name,
which is stable. To make it more user friendly, you could create
additional symlinks. We already have /sys/class/net/ethX/phydev
linking into sys/bus/mdio_bus/devices/.. .  We could add
/sys/class/net/ethX/ledY linking into /sys/class/led/...

It would also be possible to teach ethtool about LEDs, so that it
follows the symbolic links, and manipulates the LED class files.

> I also want this code to be generalized somehow so that it can be
> reused. The problem is that I want to have support for DUAL mode, which
> is Marvell specific, and a DUAL LED needs to be defined in device tree.

It sounds like you first need to teach the LED core about dual LEDs
and triggers which affect two LEDs..

   Andrew

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

[Marek Behun]

On Sat, 25 Jul 2020 20:48:46 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > > +#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    
> > > 
> > > Code getting committed should not be using #if 0. Is the needed code
> > > in the LED tree? Do we want to consider a stable branch of the LED
> > > tree which DaveM can pull into net-next? Or do you want to wait until
> > > the next merge cycle?  
> > 
> > That's why this is RFC. But yes, I would like to have this merged for
> > 5.9, so maybe we should ask Dave. Is this common? Do we also need to
> > tell Pavel or how does this work?  
> 
> The Pavel needs to create a stable branch. DaveM then merges that
> branch into net-next. Your patches can then be merged. When Linus
> pulls the two branches, led and net-next, git sees the exact same
> patches twice, and simply drops them from the second pull request.
> 
> So you need to ask Pavel and DaveM if they are willing to do this.
> 
> > > > +	init_data.fwnode = &np->fwnode;
> > > > +	init_data.devname_mandatory = true;
> > > > +	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";    
> > > 
> > > This we need to think about. Are you running this on a system with
> > > systemd? Does the interface have a name like enp2s0? Does the LED get
> > > registered before or after systemd renames it from eth0 to enp2s0?  
> > 
> > Yes, well, this should be discussed also with LED guys. I don't suppose
> > that renaming the sysfs symlink on interface rename event is
> > appropriate, but who knows?
> > The interfaces are platform specific, on mvebu. They aren't connected
> > via PCI, so their names remain eth0, eth1 ...  
> 
> But the Marvell driver is used with more than just mvebu. And we need
> this generic. There are USB Ethernet dongles which used phylib. They
> will get their interfaces renamed to include the MAC address, etc.
> 
> It is possible to hook the notifier so we know when an interface is
> renamed. We can then either destroy and re-create the LED, or if the
> LED framework allows it, rename it.
> 
> Or we avoid interface names all together and stick with the phy name,
> which is stable. To make it more user friendly, you could create
> additional symlinks. We already have /sys/class/net/ethX/phydev
> linking into sys/bus/mdio_bus/devices/.. .  We could add
> /sys/class/net/ethX/ledY linking into /sys/class/led/...
> 
> It would also be possible to teach ethtool about LEDs, so that it
> follows the symbolic links, and manipulates the LED class files.

I will propose rename of the LED name on interface rename event.

> > I also want this code to be generalized somehow so that it can be
> > reused. The problem is that I want to have support for DUAL mode, which
> > is Marvell specific, and a DUAL LED needs to be defined in device tree.  
> 
> It sounds like you first need to teach the LED core about dual LEDs
> and triggers which affect two LEDs..

This is already done. DUAL LEDs will be handled by the multicolor LED
framework which also is already in Pavel's tree. The problem is that if
we make PHY LEDs OF parsing code generic in phy-core, it will also need
to be robust enough to take care of DUAL LEDs OF parsing. That is
something which is Marvell specific. It is possible that some other
vendor also manufactures PHYs with something like that, but the LEDs
on other PHYs may have different maximum value of brightness, or
additional configurations which will need to be in device tree...

But I will try to make it generic and then we will see where it goes...

Marek

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

[Pavel Machek]

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

On Sat 2020-07-25 20:48:46, Andrew Lunn wrote:
> > > > +#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  
> > > 
> > > Code getting committed should not be using #if 0. Is the needed code
> > > in the LED tree? Do we want to consider a stable branch of the LED
> > > tree which DaveM can pull into net-next? Or do you want to wait until
> > > the next merge cycle?
> > 
> > That's why this is RFC. But yes, I would like to have this merged for
> > 5.9, so maybe we should ask Dave. Is this common? Do we also need to
> > tell Pavel or how does this work?
> 
> The Pavel needs to create a stable branch. DaveM then merges that
> branch into net-next. Your patches can then be merged. When Linus
> pulls the two branches, led and net-next, git sees the exact same
> patches twice, and simply drops them from the second pull request.
> 
> So you need to ask Pavel and DaveM if they are willing to do this.

Multicolor should be upstream now, so I believe this is no longer
required?

> > I also want this code to be generalized somehow so that it can be
> > reused. The problem is that I want to have support for DUAL mode, which
> > is Marvell specific, and a DUAL LED needs to be defined in device tree.
> 
> It sounds like you first need to teach the LED core about dual LEDs
> and triggers which affect two LEDs..

Umm. Yes, triggers for controlling both intensity and hue will be
interesting. Suggestions welcome.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]