[PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Vishal Thanki]

Hi all,

Based on suggestions from Andrew and Florian, I have made some changes
to expose the ethernet PHY LEDs using kernel LED subsystem. The following
ALPHA patchset introduces two new LED triggers:

1) <phydev>-eth-phy-link:
To monitor the PHY Link status

2) <phydev>-eth-phy-activity:
To monitor the PHY activity status. This trigger may still some more work
because as of now it just takes decision to set the trigger based on
PHY state machine and triggers the blink_set with delay_on and delay_off
parameters set to 0.

Please provide the review comments so that I can work on this patchset to
make it complete.

Thanks,
Vishal

Vishal Thanki (3):
  net: phy: Add ethernet PHY LED triggers
  net: phy: at8030: Expose the Link and Activity LEDs
  led: at8030: Add LED driver for AT8030 ethernet PHY

 drivers/leds/Kconfig         |   7 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-at803x.c   | 158 +++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/Kconfig      |   7 ++
 drivers/net/phy/Makefile     |   1 +
 drivers/net/phy/at803x.c     |  55 ++++++++++++++-
 drivers/net/phy/phy.c        |  20 +++++-
 drivers/net/phy/phy_device.c |   4 ++
 drivers/net/phy/phy_led.c    |  70 +++++++++++++++++++
 drivers/net/phy/phy_led.h    |  37 ++++++++++
 include/linux/leds.h         |   1 +
 include/linux/phy.h          |   6 ++
 include/linux/phy/at803x.h   |  45 ++++++++++++
 13 files changed, 409 insertions(+), 3 deletions(-)
 create mode 100644 drivers/leds/leds-at803x.c
 create mode 100644 drivers/net/phy/phy_led.c
 create mode 100644 drivers/net/phy/phy_led.h
 create mode 100644 include/linux/phy/at803x.h

-- 
2.4.3

[PATCH 1/3] net: phy: Add ethernet PHY LED triggers

[Vishal Thanki]

Add the LED triggers for ethernet PHY link and activity
status. The triggers are set mainly based on the
PHY state machine.

Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
---
 drivers/net/phy/Kconfig      |  7 +++++
 drivers/net/phy/Makefile     |  1 +
 drivers/net/phy/phy.c        | 20 +++++++++++--
 drivers/net/phy/phy_device.c |  4 +++
 drivers/net/phy/phy_led.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_led.h    | 37 +++++++++++++++++++++++
 include/linux/leds.h         |  1 +
 include/linux/phy.h          |  6 ++++
 8 files changed, 144 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/phy_led.c
 create mode 100644 drivers/net/phy/phy_led.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 6dad9a9..15712da 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -271,6 +271,13 @@ config MDIO_BCM_IPROC
 	  This module provides a driver for the MDIO busses found in the
 	  Broadcom iProc SoC's.
 
+config ETH_PHY_LED
+	bool "Ethernet PHY LED trigger support (Experimental)"
+	depends on LEDS_TRIGGERS
+	help
+	  This feature enables the Ethernet PHY LED triggers for PHY link
+	  and data transfer activities.
+
 endif # PHYLIB
 
 config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index fcdbb92..76ebd83 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -44,3 +44,4 @@ obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
+obj-$(CONFIG_ETH_PHY_LED)	+= phy_led.o
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 5590b9c..e422ff6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -35,6 +35,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/atomic.h>
+#include "phy_led.h"
 
 #include <asm/irq.h>
 
@@ -831,15 +832,15 @@ void phy_state_machine(struct work_struct *work)
 
 	switch (phydev->state) {
 	case PHY_DOWN:
+		phy_trig_led_link(phydev, 0);
 	case PHY_STARTING:
 	case PHY_READY:
 	case PHY_PENDING:
 		break;
 	case PHY_UP:
 		needs_aneg = true;
-
 		phydev->link_timeout = PHY_AN_TIMEOUT;
-
+		phy_trig_led_link(phydev, 1);
 		break;
 	case PHY_AN:
 		err = phy_read_status(phydev);
@@ -849,6 +850,7 @@ void phy_state_machine(struct work_struct *work)
 		/* If the link is down, give up on negotiation for now */
 		if (!phydev->link) {
 			phydev->state = PHY_NOLINK;
+			phy_trig_led_link(phydev, 0);
 			netif_carrier_off(phydev->attached_dev);
 			phydev->adjust_link(phydev->attached_dev);
 			break;
@@ -862,6 +864,7 @@ void phy_state_machine(struct work_struct *work)
 		/* If AN is done, we're running */
 		if (err > 0) {
 			phydev->state = PHY_RUNNING;
+			phy_trig_led_act(phydev);
 			netif_carrier_on(phydev->attached_dev);
 			phydev->adjust_link(phydev->attached_dev);
 
@@ -889,6 +892,7 @@ void phy_state_machine(struct work_struct *work)
 				}
 			}
 			phydev->state = PHY_RUNNING;
+			phy_trig_led_act(phydev);
 			netif_carrier_on(phydev->attached_dev);
 			phydev->adjust_link(phydev->attached_dev);
 		}
@@ -900,6 +904,8 @@ void phy_state_machine(struct work_struct *work)
 
 		if (phydev->link) {
 			phydev->state = PHY_RUNNING;
+			phy_trig_led_act(phydev);
+			phy_trig_led_link(phydev, 1);
 			netif_carrier_on(phydev->attached_dev);
 		} else {
 			if (0 == phydev->link_timeout--)
@@ -929,9 +935,12 @@ void phy_state_machine(struct work_struct *work)
 
 		if (phydev->link) {
 			phydev->state = PHY_RUNNING;
+			phy_trig_led_act(phydev);
+			phy_trig_led_link(phydev, 1);
 			netif_carrier_on(phydev->attached_dev);
 		} else {
 			phydev->state = PHY_NOLINK;
+			phy_trig_led_link(phydev, 0);
 			netif_carrier_off(phydev->attached_dev);
 		}
 
@@ -948,6 +957,7 @@ void phy_state_machine(struct work_struct *work)
 			phydev->adjust_link(phydev->attached_dev);
 			do_suspend = true;
 		}
+		led_trigger_event(phydev->phy_act, LED_OFF);
 		break;
 	case PHY_RESUMING:
 		if (AUTONEG_ENABLE == phydev->autoneg) {
@@ -965,9 +975,12 @@ void phy_state_machine(struct work_struct *work)
 
 				if (phydev->link) {
 					phydev->state = PHY_RUNNING;
+					phy_trig_led_link(phydev, 1);
+					phy_trig_led_act(phydev);
 					netif_carrier_on(phydev->attached_dev);
 				} else	{
 					phydev->state = PHY_NOLINK;
+					phy_trig_led_link(phydev, 0);
 				}
 				phydev->adjust_link(phydev->attached_dev);
 			} else {
@@ -981,9 +994,12 @@ void phy_state_machine(struct work_struct *work)
 
 			if (phydev->link) {
 				phydev->state = PHY_RUNNING;
+				phy_trig_led_link(phydev, 1);
+				phy_trig_led_act(phydev);
 				netif_carrier_on(phydev->attached_dev);
 			} else	{
 				phydev->state = PHY_NOLINK;
+				phy_trig_led_link(phydev, 0);
 			}
 			phydev->adjust_link(phydev->attached_dev);
 		}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e551f3a..fafcc3c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -34,6 +34,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include "phy_led.h"
 
 #include <asm/irq.h>
 
@@ -1592,6 +1593,8 @@ static int phy_probe(struct device *dev)
 	of_set_phy_supported(phydev);
 	phydev->advertising = phydev->supported;
 
+	phy_led_init(phydev);
+
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;
 
@@ -1608,6 +1611,7 @@ static int phy_remove(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 
 	mutex_lock(&phydev->lock);
+	phy_led_cleanup(phydev);
 	phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
 
diff --git a/drivers/net/phy/phy_led.c b/drivers/net/phy/phy_led.c
new file mode 100644
index 0000000..240355a
--- /dev/null
+++ b/drivers/net/phy/phy_led.c
@@ -0,0 +1,70 @@
+#include <linux/phy.h>
+#include <linux/leds.h>
+
+static void phy_link_trig_activate(struct led_classdev *led)
+{
+	struct phy_device *phydev = led->trigger->data;
+	enum led_brightness value = LED_OFF;
+
+	if (phydev && phydev->state != PHY_DOWN)
+		value = LED_FULL;
+	led_set_brightness(led, value);
+}
+
+static void phy_act_trig_activate(struct led_classdev *led)
+{
+	struct phy_device *phydev = led->trigger->data;
+	unsigned long on = 0, off = 0;
+
+	if (phydev && (phydev->state == PHY_RUNNING ||
+		       phydev->state == PHY_CHANGELINK))
+		led_blink_set(led, &on, &off);
+}
+
+void phy_led_init(struct phy_device *phydev)
+{
+	phydev->phy_link = kzalloc(sizeof(*phydev->phy_link), GFP_KERNEL);
+	if (!phydev->phy_link)
+		return;
+
+	snprintf(phydev->phy_link_name, sizeof(phydev->phy_link_name),
+		 "%x-eth-phy-link", phydev->drv->phy_id);
+	phydev->phy_link->name = phydev->phy_link_name;
+	if (led_trigger_register(phydev->phy_link)) {
+		kfree(phydev->phy_link);
+		phydev->phy_link = NULL;
+	}
+	phydev->phy_link->data = phydev;
+	phydev->phy_link->activate = phy_link_trig_activate;
+
+	phydev->phy_act = kzalloc(sizeof(*phydev->phy_act), GFP_KERNEL);
+	if (!phydev->phy_act) {
+		led_trigger_unregister(phydev->phy_link);
+		kfree(phydev->phy_link);
+		return;
+	}
+	phydev->phy_act->data = phydev;
+	phydev->phy_act->activate = phy_act_trig_activate;
+
+	snprintf(phydev->phy_act_name, sizeof(phydev->phy_act_name),
+		 "%x-eth-phy-activity", phydev->drv->phy_id);
+	phydev->phy_act->name = phydev->phy_act_name;
+	if (led_trigger_register(phydev->phy_act)) {
+		kfree(phydev->phy_act);
+		phydev->phy_act = NULL;
+		return;
+	}
+}
+
+void phy_led_cleanup(struct phy_device *phydev)
+{
+	if (phydev->phy_link) {
+		led_trigger_unregister(phydev->phy_link);
+		kfree(phydev->phy_link);
+	}
+	if (phydev->phy_act) {
+		led_trigger_unregister(phydev->phy_act);
+		kfree(phydev->phy_act);
+	}
+}
+
diff --git a/drivers/net/phy/phy_led.h b/drivers/net/phy/phy_led.h
new file mode 100644
index 0000000..ff5bd98
--- /dev/null
+++ b/drivers/net/phy/phy_led.h
@@ -0,0 +1,37 @@
+#include <linux/leds.h>
+
+#ifdef CONFIG_ETH_PHY_LED
+static inline void phy_trig_led_act(struct phy_device *phydev)
+{
+	unsigned long on = 0, off = 0;
+
+	led_trigger_blink(phydev->phy_act, &on, &off);
+}
+
+static inline void phy_trig_led_link(struct phy_device *phydev, int on)
+{
+	enum led_brightness value = (on) ? LED_FULL : LED_OFF;
+
+	led_trigger_event(phydev->phy_link, value);
+}
+
+void phy_led_init(struct phy_device *phydev);
+void phy_led_cleanup(struct phy_device *phydev);
+#else
+static inline void phy_trig_led_act(struct phy_device *phydev)
+{
+}
+
+static inline void phy_trig_led_link(struct phy_device *phydev, int on)
+{
+}
+
+static inline void phy_led_init(struct phy_device *phydev)
+{
+}
+
+static inline void phy_led_cleanup(struct phy_device *phydev)
+{
+}
+#endif
+
diff --git a/include/linux/leds.h b/include/linux/leds.h
index f203a8f..11ec574 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -233,6 +233,7 @@ struct led_trigger {
 	const char	 *name;
 	void		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
+	void *data;
 
 	/* LEDs under control by this trigger (for simple triggers) */
 	rwlock_t	  leddev_list_lock;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2abd791..94fee77 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -25,6 +25,7 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 #include <linux/mod_devicetable.h>
+#include <linux/leds.h>
 
 #include <linux/atomic.h>
 
@@ -60,6 +61,7 @@
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_HAS_MAGICANEG	0x00000002
 #define PHY_IS_INTERNAL		0x00000004
+#define PHY_HAS_LED_CTRL	0x00000008
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /* Interface Mode definitions */
@@ -424,6 +426,10 @@ struct phy_device {
 	u8 mdix;
 
 	void (*adjust_link)(struct net_device *dev);
+
+	struct led_trigger *phy_link, *phy_act;
+	char phy_link_name[32], phy_act_name[32];
+
 };
 #define to_phy_device(d) container_of(to_mdio_device(d), \
 				      struct phy_device, mdio)
-- 
2.4.3

[PATCH 2/3] net: phy: at8030: Expose the Link and Activity LEDs

[Vishal Thanki]

Expose these LEDs by means of a platform device which
can be controlled by a suitable LED driver.

Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
---
 drivers/net/phy/at803x.c   | 55 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/phy/at803x.h | 45 +++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/phy/at803x.h

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 1e901c7..8fdb5ff 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -18,6 +18,8 @@
 #include <linux/etherdevice.h>
 #include <linux/of_gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/phy/at803x.h>
 
 #define AT803X_INTR_ENABLE			0x12
 #define AT803X_INTR_ENABLE_AUTONEG_ERR		BIT(15)
@@ -34,6 +36,7 @@
 
 #define AT803X_SMART_SPEED			0x14
 #define AT803X_LED_CONTROL			0x18
+#define AT803X_LED_MANUAL_CTRL			0x19
 
 #define AT803X_DEVICE_ADDR			0x03
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
@@ -117,6 +120,46 @@ static inline int at803x_enable_tx_delay(struct phy_device *phydev)
 					AT803X_DEBUG_TX_CLK_DLY_EN);
 }
 
+static struct at803x_phy_led leds[] = {
+	{
+		.cdev.name = "at803x-link",
+		.id = AT803X_LINK,
+		.reg = AT803X_LED_MANUAL_CTRL,
+	},
+	{
+		.cdev.name = "at803x-act",
+		.id = AT803X_ACT,
+		.reg = AT803X_LED_MANUAL_CTRL,
+	},
+};
+
+static struct at803x_phy_leds led_grp = {
+	.leds = leds,
+	.nr_leds = ARRAY_SIZE(leds),
+};
+
+static struct platform_device *pdev;
+
+static int at803x_led_init(struct phy_device *phydev)
+{
+	led_grp.phydev = phydev;
+
+	pdev = platform_device_alloc("at803x-led", 0);
+	if (!pdev) {
+		pr_info("Failed to allocate LED\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, &led_grp);
+
+	return platform_device_add(pdev);
+}
+
+static void at803x_led_cleanup(struct phy_device *phydev)
+{
+	platform_device_del(pdev);
+}
+
 /* save relevant PHY registers to private copy */
 static void at803x_context_save(struct phy_device *phydev,
 				struct at803x_context *context)
@@ -285,9 +328,18 @@ static int at803x_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	if (phydev->drv->flags & PHY_HAS_LED_CTRL)
+		return at803x_led_init(phydev);
+
 	return 0;
 }
 
+static void at803x_remove(struct phy_device *phydev)
+{
+	if (phydev->drv->flags & PHY_HAS_LED_CTRL)
+		at803x_led_cleanup(phydev);
+}
+
 static int at803x_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -411,11 +463,12 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	.features		= PHY_BASIC_FEATURES,
-	.flags			= PHY_HAS_INTERRUPT,
+	.flags			= PHY_HAS_INTERRUPT | PHY_HAS_LED_CTRL,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
+	.remove			= at803x_remove,
 }, {
 	/* ATHEROS 8031 */
 	.phy_id			= ATH8031_PHY_ID,
diff --git a/include/linux/phy/at803x.h b/include/linux/phy/at803x.h
new file mode 100644
index 0000000..f33bfcf
--- /dev/null
+++ b/include/linux/phy/at803x.h
@@ -0,0 +1,45 @@
+#ifndef _AT803X_H
+#define _AT803X_H
+
+#include <linux/leds.h>
+#include <linux/phy/phy.h>
+#include <linux/workqueue.h>
+#include <linux/spinlock.h>
+
+enum at803x_led_type {
+	AT803X_LINK,
+	AT803X_ACT,
+};
+
+union at803x_led_manual_ctrl {
+	struct {
+		u16 led_tx:2;
+		u16 led_rx:2;
+		u16 res3:2;
+		u16 led_lnk_10_100_ctrl:2;
+		u16 res2:4;
+		u16 led_act_ctrl:1;
+		u16 res1:3;
+
+	} field;
+	u16 value;
+};
+
+struct at803x_phy_led {
+	struct work_struct work;
+	struct led_classdev cdev;
+	enum led_brightness value;
+	union at803x_led_manual_ctrl regval;
+	enum at803x_led_type id;
+	struct at803x_phy_leds *led_grp;
+	spinlock_t lock;
+	u16 reg;
+};
+
+struct at803x_phy_leds {
+	struct phy_device *phydev;
+	struct at803x_phy_led *leds;
+	int nr_leds;
+};
+
+#endif
-- 
2.4.3

Re: [PATCH 2/3] net: phy: at8030: Expose the Link and Activity LEDs

[Andrew Lunn]

On Wed, Mar 23, 2016 at 06:51:39PM +0100, Vishal Thanki wrote:
> Expose these LEDs by means of a platform device which
> can be controlled by a suitable LED driver.

I would expose the leds directly here in this driver, not via a
separate platform device.

	 Andrew

[PATCH 3/3] led: at8030: Add LED driver for AT8030 ethernet PHY

[Vishal Thanki]

This patch adds support for controlling the Link and
Activity LED of AT8030 ethernet PHY through LED subsystem.

Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
---
 drivers/leds/Kconfig       |   7 ++
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-at803x.c | 158 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)
 create mode 100644 drivers/leds/leds-at803x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1f64151..07781ac 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -615,6 +615,13 @@ config LEDS_VERSATILE
 	  This option enabled support for the LEDs on the ARM Versatile
 	  and RealView boards. Say Y to enabled these.
 
+config LEDS_AT803X
+	tristate "LED support for the AT803X Ethernet PHY (Experimental)"
+	depends on LEDS_CLASS
+	help
+	  This option enabled support for the LEDs on AT803X ethernet PHY.
+
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index cb2013d..2d1ae65 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
 obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
+obj-$(CONFIG_LEDS_AT803X)		+= leds-at803x.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-at803x.c b/drivers/leds/leds-at803x.c
new file mode 100644
index 0000000..58eb0c4
--- /dev/null
+++ b/drivers/leds/leds-at803x.c
@@ -0,0 +1,158 @@
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/leds.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/phy/at803x.h>
+
+#define LED_TX_NORMAL		0x0
+#define LED_TX_BLINK		0x1
+#define LED_TX_OFF		0x2
+#define LED_TX_ON		0x3
+
+#define LED_RX_NORMAL		0x0
+#define LED_RX_BLINK		0x1
+#define LED_RX_OFF		0x2
+#define LED_RX_ON		0x3
+
+#define LED_ACT_BLINK		0x1
+
+#define LED_LINK_NORMAL		0x0
+#define LED_LINK_BLINK		LED_ACT_BLINK
+#define LED_LINK_OFF		0x2
+#define LED_LINK_ON		0x3
+
+#define LED_ACT_ACTIVE		0x0
+#define LED_ACT_LINK		0x1
+
+static void at803x_led_work(struct work_struct *work)
+{
+	struct at803x_phy_led *led =
+		container_of(work, struct at803x_phy_led, work);
+	union at803x_led_manual_ctrl regval = led->regval;
+
+	phy_write(led->led_grp->phydev, led->reg, regval.value);
+}
+
+static int at803x_blink_set(struct led_classdev *cdev,
+	unsigned long *delay_on,
+	unsigned long *delay_off)
+{
+	struct at803x_phy_led *led =
+		container_of(cdev, struct at803x_phy_led, cdev);
+	union at803x_led_manual_ctrl regval = {.value = 0};
+	int id = led->id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&led->lock, flags);
+	switch (id) {
+	case AT803X_LINK:
+		/* Can't configure LED_LINK to blink */
+		break;
+	case AT803X_ACT:
+		/*
+		 * LED will be ON when link is established
+		 * and it will blink when link is active.
+		 * delay_on/delay_off values are ignored.
+		 */
+		regval.field.led_act_ctrl = LED_ACT_LINK;
+		break;
+	}
+	led->regval = regval;
+
+	schedule_work(&led->work);
+	spin_unlock_irqrestore(&led->lock, flags);
+	return 0;
+}
+
+
+static void at803x_phy_led_brightness_set(struct led_classdev *led_cdev,
+		enum led_brightness value)
+{
+	struct at803x_phy_led *led =
+		container_of(led_cdev, struct at803x_phy_led, cdev);
+	union at803x_led_manual_ctrl regval = led->regval;
+	int id = led->id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&led->lock, flags);
+	switch (id) {
+	case AT803X_LINK:
+		regval.field.led_lnk_10_100_ctrl = value == LED_FULL ?
+			LED_LINK_ON : LED_LINK_OFF;
+		break;
+	case AT803X_ACT:
+		/* LED_ACT status is controlled via LED_TX and LED_RX */
+		regval.field.led_tx = value == LED_FULL ?
+			LED_TX_ON : LED_TX_OFF;
+		regval.field.led_rx = value == LED_FULL ?
+			LED_RX_ON : LED_RX_OFF;
+		break;
+	}
+
+	led->value = value;
+	led->regval = regval;
+	schedule_work(&led->work);
+	spin_unlock_irqrestore(&led->lock, flags);
+}
+
+static int at803x_phy_led_probe(struct platform_device *pdev)
+{
+	int i;
+	int ret;
+	struct at803x_phy_leds *led_grp = platform_get_drvdata(pdev);
+	struct at803x_phy_led *leds = led_grp->leds;
+
+	for (i = 0; i < led_grp->nr_leds; i++) {
+
+		leds[i].cdev.brightness_set = at803x_phy_led_brightness_set;
+		leds[i].cdev.blink_set = at803x_blink_set;
+
+		ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
+		if (ret < 0)
+			goto err;
+
+		leds[i].led_grp = led_grp;
+		leds[i].regval.value = phy_read(led_grp->phydev, leds[i].reg);
+		INIT_WORK(&leds[i].work, at803x_led_work);
+		spin_lock_init(&leds[i].lock);
+
+	}
+
+	return 0;
+
+err:
+	for (i = i - 1; i >= 0; i--)
+		led_classdev_unregister(&leds[i].cdev);
+
+	return ret;
+}
+
+static int at803x_phy_led_remove(struct platform_device *pdev)
+{
+	int i;
+	struct at803x_phy_leds *led_grp = platform_get_drvdata(pdev);
+	struct at803x_phy_led *leds = led_grp->leds;
+
+
+	for (i = 0; i < led_grp->nr_leds; i++)
+		led_classdev_unregister(&leds[i].cdev);
+
+	return 0;
+}
+
+static struct platform_driver at803x_phy_led_driver = {
+	.probe		= at803x_phy_led_probe,
+	.remove		= at803x_phy_led_remove,
+	.driver		= {
+		.name	= "at803x-led",
+	},
+};
+
+module_platform_driver(at803x_phy_led_driver);
+
+MODULE_DESCRIPTION("at803x PHY LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-at803x");
-- 
2.4.3

Re: [PATCH 3/3] led: at8030: Add LED driver for AT8030 ethernet PHY

[Andrew Lunn]

> +static void at803x_led_work(struct work_struct *work)
> +{
> +	struct at803x_phy_led *led =
> +		container_of(work, struct at803x_phy_led, work);
> +	union at803x_led_manual_ctrl regval = led->regval;
> +
> +	phy_write(led->led_grp->phydev, led->reg, regval.value);
> +}
> +

There has recently been a big refactoring of the LED code. You should
no longer need a work queue. The core will do that for you.

   Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Andrew Lunn]

On Wed, Mar 23, 2016 at 06:51:37PM +0100, Vishal Thanki wrote:
> Hi all,
> 
> Based on suggestions from Andrew and Florian, I have made some changes
> to expose the ethernet PHY LEDs using kernel LED subsystem. The following
> ALPHA patchset introduces two new LED triggers:
> 
> 1) <phydev>-eth-phy-link:
> To monitor the PHY Link status
> 
> 2) <phydev>-eth-phy-activity:
> To monitor the PHY activity status. This trigger may still some more work
> because as of now it just takes decision to set the trigger based on
> PHY state machine and triggers the blink_set with delay_on and delay_off
> parameters set to 0.

My suggestion was that the hardware needs to control the LEDs. You
have software doing it. You might be able to do this with the PHY
state machine for link. But activity is never going to be accepted if
software control.

The LED trigger attached to an LED should be used to configure the
hardware to drive the LED as wanted.

The exception to this is when there is no trigger attached, and the
brightness can set the on/off state, if the hardware supports this.

	   Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Vishal Thanki]

Hi,

> My suggestion was that the hardware needs to control the LEDs. You
> have software doing it. You might be able to do this with the PHY
> state machine for link. But activity is never going to be accepted if
> software control.
>
> The LED trigger attached to an LED should be used to configure the
> hardware to drive the LED as wanted.
>

The eth-phy-activity trigger uses the blink_set which I think uses the
hardware acceleration if available. I am not sure how to handles LEDs
which does not have hardware acceleration for this (eth-phy-activity)
trigger.

> The exception to this is when there is no trigger attached, and the
> brightness can set the on/off state, if the hardware supports this.
>
>            Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Andrew Lunn]

On Wed, Mar 23, 2016 at 09:24:00PM +0100, Vishal Thanki wrote:
> Hi,
> 
> > My suggestion was that the hardware needs to control the LEDs. You
> > have software doing it. You might be able to do this with the PHY
> > state machine for link. But activity is never going to be accepted if
> > software control.
> >
> > The LED trigger attached to an LED should be used to configure the
> > hardware to drive the LED as wanted.
> >
> 
> The eth-phy-activity trigger uses the blink_set which I think uses the
> hardware acceleration if available. I am not sure how to handles LEDs
> which does not have hardware acceleration for this (eth-phy-activity)
> trigger.

We want the LED to blink on activity, real packets coming in and
out. The PHY can do this, so let the PHY control the LED. In this
case, the trigger is just mechanism for the user to say what the LED
should be used for. The trigger is not itself controlling the LED, it
has no idea about packets coming and going.

     Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Vishal Thanki]

On Wed, Mar 23, 2016 at 10:10 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Mar 23, 2016 at 09:24:00PM +0100, Vishal Thanki wrote:
>> Hi,
>>
>> > My suggestion was that the hardware needs to control the LEDs. You
>> > have software doing it. You might be able to do this with the PHY
>> > state machine for link. But activity is never going to be accepted if
>> > software control.
>> >
>> > The LED trigger attached to an LED should be used to configure the
>> > hardware to drive the LED as wanted.
>> >
>>
>> The eth-phy-activity trigger uses the blink_set which I think uses the
>> hardware acceleration if available. I am not sure how to handles LEDs
>> which does not have hardware acceleration for this (eth-phy-activity)
>> trigger.
>
> We want the LED to blink on activity, real packets coming in and
> out. The PHY can do this, so let the PHY control the LED. In this
> case, the trigger is just mechanism for the user to say what the LED
> should be used for. The trigger is not itself controlling the LED, it
> has no idea about packets coming and going.
>

Yes, I understand that. But PHY can only control the LEDs attached to
it directly. The at803x led driver configures the PHY to blink the
activity LED based on traffic but I think it is not possible for PHY
to control other LEDs in system, for example some other LEDs in system
controlled only via GPIO. In such cases, putting PHY activity trigger
on the GPIO LEDs would not make sense. Correct me if I am wrong.

Vishal

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Andrew Lunn]

On Wed, Mar 23, 2016 at 10:24:45PM +0100, Vishal Thanki wrote:
> On Wed, Mar 23, 2016 at 10:10 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Mar 23, 2016 at 09:24:00PM +0100, Vishal Thanki wrote:
> >> Hi,
> >>
> >> > My suggestion was that the hardware needs to control the LEDs. You
> >> > have software doing it. You might be able to do this with the PHY
> >> > state machine for link. But activity is never going to be accepted if
> >> > software control.
> >> >
> >> > The LED trigger attached to an LED should be used to configure the
> >> > hardware to drive the LED as wanted.
> >> >
> >>
> >> The eth-phy-activity trigger uses the blink_set which I think uses the
> >> hardware acceleration if available. I am not sure how to handles LEDs
> >> which does not have hardware acceleration for this (eth-phy-activity)
> >> trigger.
> >
> > We want the LED to blink on activity, real packets coming in and
> > out. The PHY can do this, so let the PHY control the LED. In this
> > case, the trigger is just mechanism for the user to say what the LED
> > should be used for. The trigger is not itself controlling the LED, it
> > has no idea about packets coming and going.
> >
> 
> Yes, I understand that. But PHY can only control the LEDs attached to
> it directly. The at803x led driver configures the PHY to blink the
> activity LED based on traffic but I think it is not possible for PHY
> to control other LEDs in system, for example some other LEDs in system
> controlled only via GPIO. In such cases, putting PHY activity trigger
> on the GPIO LEDs would not make sense. Correct me if I am wrong.

Hi Vishal

All correct. Which is why i said in my original email, you need to
extend the LED core to associate triggers to LEDs. You can then
associate the eth-phy-activity trigger to only PHY leds which can
implement that functionality.

drivers/leds/led-triggers.c contains a global list
LIST_HEAD(trigger_list) which triggers get added to using
led_trigger_register(). You could add a second list to the
led_classdev structure, and add an led_trigger_register_to_led()
function which registers a trigger to a specific LED, on its own
trigger list. led_trigger_store() and led_trigger_show() would use
both lists.

     Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Vishal Thanki]

>> >> The eth-phy-activity trigger uses the blink_set which I think uses the
>> >> hardware acceleration if available. I am not sure how to handles LEDs
>> >> which does not have hardware acceleration for this (eth-phy-activity)
>> >> trigger.
>> >
>> > We want the LED to blink on activity, real packets coming in and
>> > out. The PHY can do this, so let the PHY control the LED. In this
>> > case, the trigger is just mechanism for the user to say what the LED
>> > should be used for. The trigger is not itself controlling the LED, it
>> > has no idea about packets coming and going.
>> >
>>
>> Yes, I understand that. But PHY can only control the LEDs attached to
>> it directly. The at803x led driver configures the PHY to blink the
>> activity LED based on traffic but I think it is not possible for PHY
>> to control other LEDs in system, for example some other LEDs in system
>> controlled only via GPIO. In such cases, putting PHY activity trigger
>> on the GPIO LEDs would not make sense. Correct me if I am wrong.
>
> Hi Vishal
>
> All correct. Which is why i said in my original email, you need to
> extend the LED core to associate triggers to LEDs. You can then
> associate the eth-phy-activity trigger to only PHY leds which can
> implement that functionality.
>
> drivers/leds/led-triggers.c contains a global list
> LIST_HEAD(trigger_list) which triggers get added to using
> led_trigger_register(). You could add a second list to the
> led_classdev structure, and add an led_trigger_register_to_led()

Hi Andrew,

I still have some questions. Will the phylib call this
led_trigger_register_to_led() function for registering the trigger
instead of calling led_trigger_register()?

> function which registers a trigger to a specific LED, on its own
> trigger list. led_trigger_store() and led_trigger_show() would use
> both lists.

In case of led_trigger_store(), how to stop the non-PHY LEDs to
register themselves from eth-phy-activity trigger. Should there be a
LED class field which distinguishes different LEDs types (GPIO, PHY
etc..) ? And only PHY LEDs have the privilege to register to
eth-phy-activity trigger?

Thanks you very much for your guidance and patience with me.

Regards,
Vishal

>
>      Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Andrew Lunn]

> Hi Andrew,
> 
> I still have some questions. Will the phylib call this
> led_trigger_register_to_led() function for registering the trigger
> instead of calling led_trigger_register()?

Each phy driver needs to call led_classdev_register() for each LED it
has. It then also calls led_trigger_register_to_led() with the
triggers that each LED supports.

> > function which registers a trigger to a specific LED, on its own
> > trigger list. led_trigger_store() and led_trigger_show() would use
> > both lists.
> 
> In case of led_trigger_store(), how to stop the non-PHY LEDs to
> register themselves from eth-phy-activity trigger.

They won't have the option. led_trigger_register_to_led() only
registers the trigger to one specific LED, i.e. adds it to the trigger
list in its led_classdev structure, not the global list of triggers.

     Andrew

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Stephen Hemminger]

On Wed, 23 Mar 2016 18:51:37 +0100
Vishal Thanki <vishalthanki@gmail.com> wrote:

> Hi all,
> 
> Based on suggestions from Andrew and Florian, I have made some changes
> to expose the ethernet PHY LEDs using kernel LED subsystem. The following
> ALPHA patchset introduces two new LED triggers:
> 
> 1) <phydev>-eth-phy-link:
> To monitor the PHY Link status
> 
> 2) <phydev>-eth-phy-activity:
> To monitor the PHY activity status. This trigger may still some more work
> because as of now it just takes decision to set the trigger based on
> PHY state machine and triggers the blink_set with delay_on and delay_off
> parameters set to 0.
> 
> Please provide the review comments so that I can work on this patchset to
> make it complete.
> 
> Thanks,
> Vishal
> 
> Vishal Thanki (3):
>   net: phy: Add ethernet PHY LED triggers
>   net: phy: at8030: Expose the Link and Activity LEDs
>   led: at8030: Add LED driver for AT8030 ethernet PHY
> 
>  drivers/leds/Kconfig         |   7 ++
>  drivers/leds/Makefile        |   1 +
>  drivers/leds/leds-at803x.c   | 158 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/phy/Kconfig      |   7 ++
>  drivers/net/phy/Makefile     |   1 +
>  drivers/net/phy/at803x.c     |  55 ++++++++++++++-
>  drivers/net/phy/phy.c        |  20 +++++-
>  drivers/net/phy/phy_device.c |   4 ++
>  drivers/net/phy/phy_led.c    |  70 +++++++++++++++++++
>  drivers/net/phy/phy_led.h    |  37 ++++++++++
>  include/linux/leds.h         |   1 +
>  include/linux/phy.h          |   6 ++
>  include/linux/phy/at803x.h   |  45 ++++++++++++
>  13 files changed, 409 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/leds/leds-at803x.c
>  create mode 100644 drivers/net/phy/phy_led.c
>  create mode 100644 drivers/net/phy/phy_led.h
>  create mode 100644 include/linux/phy/at803x.h
> 

There already is LED control via ethtool.
It is more important that the existing API (ethtool) still work.

Re: [PATCH 0/3] Control ethernet PHY LEDs via LED subsystem

[Andrew Lunn]

Hi Stephen
 
> There already is LED control via ethtool.
> It is more important that the existing API (ethtool) still work.

I'm having trouble finding this. The man page for ethtool only
mentions -p --identify with respect to LEDs.

I also don't see anything in include/uapi/linux/ethtool.h

Please could you give more specific pointers.

Thanks
	Andrew