[PATCH] Add support to control PHY LEDs

[PATCH] Add support to control PHY LEDs

[Vishal Thanki]

Hi all,

We had a requirement to disable the PHY link LEDs during system
standby mode without turning off the ethernet module. To achieve that,
I have prepared the sysfs interface turn on/off the PHY link LEDs.

Please suggest if there is any better way to achieve this.

Thanks,
Vishal

Vishal Thanki (1):
  net: phy: at803x: Add support to control PHY LEDs

 drivers/net/phy/Kconfig  |  7 +++++
 drivers/net/phy/at803x.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

-- 
2.4.3

[PATCH] net: phy: at803x: Add support to control PHY LEDs

[Vishal Thanki]

The LEDs can be turned on and off by a sysfs interface
now. Write 0 to "led_enable" file to turn off the LEDs
and write 1 to turn on. The support is experimental
and can be enabled by kernel configuration option.

Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
---
 drivers/net/phy/Kconfig  |  7 +++++
 drivers/net/phy/at803x.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 075a4cc..16f78cf 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -24,6 +24,13 @@ config AT803X_PHY
 	---help---
 	  Currently supports the AT8030 and AT8035 model
 
+config AT803X_PHY_LED_CONTROL
+	depends on AT803X_PHY
+	bool "Support for PHY LED control (Experimental)"
+	---help---
+	  This option enables a sysfs interface to turn on and off the link
+	  LEDs attached to phy.
+
 config AMD_PHY
 	tristate "Drivers for the AMD PHYs"
 	---help---
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 1e901c7..a606d52 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -59,12 +59,19 @@
 #define ATH8031_PHY_ID 0x004dd074
 #define ATH8035_PHY_ID 0x004dd072
 
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+#define LED_OFF		0xF71F
+#endif
+
 MODULE_DESCRIPTION("Atheros 803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
 struct at803x_priv {
 	bool phy_reset:1;
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+	int led_status;
+#endif
 	struct gpio_desc *gpiod_reset;
 };
 
@@ -267,6 +274,42 @@ done:
 	return 0;
 }
 
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+static ssize_t at803x_led_get(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int read = phy_read(phydev, AT803X_LED_CONTROL);
+
+	read = (read == LED_OFF) ? 0 : 1;
+	return sprintf(buf, "%x\n", read);
+}
+
+static ssize_t at803x_led_set(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	struct at803x_priv *priv = phydev->priv;
+	int val;
+
+	if (!strncmp(buf, "0", 1)) {
+		priv->led_status = phy_read(phydev, AT803X_LED_CONTROL);
+		val = LED_OFF;
+	} else if (!strncmp(buf, "1", 1)) {
+		val = priv->led_status;
+	} else {
+		return -EINVAL;
+	}
+
+	phy_write(phydev, AT803X_LED_CONTROL, val);
+
+	return count;
+}
+
+static DEVICE_ATTR(led_enable, 0600, at803x_led_get, at803x_led_set);
+#endif
+
 static int at803x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -285,12 +328,31 @@ static int at803x_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+	dev_set_drvdata(dev, phydev);
+	return device_create_file(dev, &dev_attr_led_enable);
+#else
 	return 0;
+#endif
+}
+
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+static void at803x_remove(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	device_remove_file(dev, &dev_attr_led_enable);
 }
+#endif
 
 static int at803x_config_init(struct phy_device *phydev)
 {
 	int ret;
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+	struct at803x_priv *priv;
+
+	priv = phydev->priv;
+	priv->led_status = phy_read(phydev, AT803X_LED_CONTROL);
+#endif
 
 	ret = genphy_config_init(phydev);
 	if (ret < 0)
@@ -396,6 +458,9 @@ static struct phy_driver at803x_driver[] = {
 	.flags			= PHY_HAS_INTERRUPT,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+	.remove			= at803x_remove,
+#endif
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 }, {
@@ -414,6 +479,9 @@ static struct phy_driver at803x_driver[] = {
 	.flags			= PHY_HAS_INTERRUPT,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+	.remove			= at803x_remove,
+#endif
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 }, {
@@ -432,6 +500,9 @@ static struct phy_driver at803x_driver[] = {
 	.flags			= PHY_HAS_INTERRUPT,
 	.config_aneg		= genphy_config_aneg,
 	.read_status		= genphy_read_status,
+#ifdef CONFIG_AT803X_PHY_LED_CONTROL
+	.remove			= at803x_remove,
+#endif
 	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
 } };
-- 
2.4.3

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Andrew Lunn]

On Thu, Mar 17, 2016 at 02:59:07PM +0100, Vishal Thanki wrote:
> The LEDs can be turned on and off by a sysfs interface
> now. Write 0 to "led_enable" file to turn off the LEDs
> and write 1 to turn on. The support is experimental
> and can be enabled by kernel configuration option.
> 
> Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
> ---
>  drivers/net/phy/Kconfig  |  7 +++++
>  drivers/net/phy/at803x.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++

Hi Vishal

This solution seems specific to the at803. You should be thinking of a
generic solution that all PHYs can use. eg add a new callback function
to phy_driver, and put the sysfs handling code in phylib.

   Andrew

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Florian Fainelli]

On 17/03/16 07:50, Andrew Lunn wrote:
> On Thu, Mar 17, 2016 at 02:59:07PM +0100, Vishal Thanki wrote:
>> The LEDs can be turned on and off by a sysfs interface
>> now. Write 0 to "led_enable" file to turn off the LEDs
>> and write 1 to turn on. The support is experimental
>> and can be enabled by kernel configuration option.
>>
>> Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
>> ---
>>  drivers/net/phy/Kconfig  |  7 +++++
>>  drivers/net/phy/at803x.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Hi Vishal
> 
> This solution seems specific to the at803. You should be thinking of a
> generic solution that all PHYs can use. eg add a new callback function
> to phy_driver, and put the sysfs handling code in phylib.

Indeed, maybe we should be considering using the existing LEDS subsystem
(drivers/leds/*) to expose the PHY LEDs to user-space in a standard way?
-- 
Florian

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Vishal Thanki]

On Thu, Mar 17, 2016 at 5:59 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 17/03/16 07:50, Andrew Lunn wrote:
>> On Thu, Mar 17, 2016 at 02:59:07PM +0100, Vishal Thanki wrote:
>>> The LEDs can be turned on and off by a sysfs interface
>>> now. Write 0 to "led_enable" file to turn off the LEDs
>>> and write 1 to turn on. The support is experimental
>>> and can be enabled by kernel configuration option.
>>>
>>> Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
>>> ---
>>>  drivers/net/phy/Kconfig  |  7 +++++
>>>  drivers/net/phy/at803x.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> Hi Vishal
>>
>> This solution seems specific to the at803. You should be thinking of a
>> generic solution that all PHYs can use. eg add a new callback function
>> to phy_driver, and put the sysfs handling code in phylib.
>
> Indeed, maybe we should be considering using the existing LEDS subsystem
> (drivers/leds/*) to expose the PHY LEDs to user-space in a standard way?
> --

I agree with the idea of making a generic callback in phy_driver so
that other PHYs
can use it. However I think these LEDs are tightly coupled with PHYs and it
would not make much sense to expose them via LED subsystem as they
cannot be controlled individually IMHO.

> Florian

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Andrew Lunn]

> I agree with the idea of making a generic callback in phy_driver so
> that other PHYs
> can use it. However I think these LEDs are tightly coupled with PHYs and it
> would not make much sense to expose them via LED subsystem as they
> cannot be controlled individually IMHO.

They might not be on/off controllable individually, but you can often
set them to show Packet RX, Packet TX, Link, speed, etc, in a
reasonably flexible way. So you could have LED triggers mapping to
these functionalities. The user can then pick the trigger for the LED.

Actually, the data sheet says:

2.3.4 LED Interface

The LED interface can either be controlled by the PHY or controlled
manually, independent of the state of the PHY. Two status LEDs are
available. These can be used to indicate operation speed, and link
status. The LEDs can be programmed to different status functions from
their default value. They can also be controlled directly from the MII
register interface.

So maybe you can control the on/off state.

   Andrew

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Vishal Thanki]

> 
> They might not be on/off controllable individually, but you can often
> set them to show Packet RX, Packet TX, Link, speed, etc, in a
> reasonably flexible way. So you could have LED triggers mapping to
> these functionalities. The user can then pick the trigger for the LED.
> 
> Actually, the data sheet says:
> 
> 2.3.4 LED Interface
> 
> The LED interface can either be controlled by the PHY or controlled
> manually, independent of the state of the PHY. Two status LEDs are
> available. These can be used to indicate operation speed, and link
> status. The LEDs can be programmed to different status functions from
> their default value. They can also be controlled directly from the MII
> register interface.
> 
> So maybe you can control the on/off state.
> 

I need to explore more on LED subsystem. Thank you for your suggestions
and input.

Thanks,
Vishal

>    Andrew

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Vishal Thanki]

Hi,

On Thu, Mar 17, 2016 at 07:07:12PM +0100, Vishal Thanki wrote:
> > 
> > They might not be on/off controllable individually, but you can often
> > set them to show Packet RX, Packet TX, Link, speed, etc, in a
> > reasonably flexible way. So you could have LED triggers mapping to
> > these functionalities. The user can then pick the trigger for the LED.
> > 
> > Actually, the data sheet says:
> > 
> > 2.3.4 LED Interface
> > 
> > The LED interface can either be controlled by the PHY or controlled
> > manually, independent of the state of the PHY. Two status LEDs are
> > available. These can be used to indicate operation speed, and link
> > status. The LEDs can be programmed to different status functions from
> > their default value. They can also be controlled directly from the MII
> > register interface.
> > 
> > So maybe you can control the on/off state.
> > 

I need some more understanding on using the LED subsystem for this task.
Pardon my lack of knowledge, and probably silly questions too.

Here is what I understood when we decide to use the LED subsystem for
exposing PHY LEDs.

1) Have a generic driver under drivers/leds/, for example leds-eth-phy.c
2) Implement a set of phylib APIs which will be used by generic phy led
driver to set the brightness.
3) The phylib APIs will depend on actual PHY driver, at803x.c for
example, (by means of callbacks implemented in PHY driver) to control LEDs.
4) PHY driver can advertise its support for LEDs using the capability
flags.

Please let me know if my understanding is correct.

Thanks,
Vishal

> 

Re: [PATCH] net: phy: at803x: Add support to control PHY LEDs

[Andrew Lunn]

On Mon, Mar 21, 2016 at 06:36:50PM +0100, Vishal Thanki wrote:
> Hi,
> 
> On Thu, Mar 17, 2016 at 07:07:12PM +0100, Vishal Thanki wrote:
> > > 
> > > They might not be on/off controllable individually, but you can often
> > > set them to show Packet RX, Packet TX, Link, speed, etc, in a
> > > reasonably flexible way. So you could have LED triggers mapping to
> > > these functionalities. The user can then pick the trigger for the LED.
> > > 
> > > Actually, the data sheet says:
> > > 
> > > 2.3.4 LED Interface
> > > 
> > > The LED interface can either be controlled by the PHY or controlled
> > > manually, independent of the state of the PHY. Two status LEDs are
> > > available. These can be used to indicate operation speed, and link
> > > status. The LEDs can be programmed to different status functions from
> > > their default value. They can also be controlled directly from the MII
> > > register interface.
> > > 
> > > So maybe you can control the on/off state.
> > > 
> 
> I need some more understanding on using the LED subsystem for this task.
> Pardon my lack of knowledge, and probably silly questions too.
> 
> Here is what I understood when we decide to use the LED subsystem for
> exposing PHY LEDs.
> 
> 1) Have a generic driver under drivers/leds/, for example leds-eth-phy.c
> 2) Implement a set of phylib APIs which will be used by generic phy led
> driver to set the brightness.
> 3) The phylib APIs will depend on actual PHY driver, at803x.c for
> example, (by means of callbacks implemented in PHY driver) to control LEDs.
> 4) PHY driver can advertise its support for LEDs using the capability
> flags.
> 
> Please let me know if my understanding is correct.

LEDs are more than brightness. They can also have triggers.

It would require some rework of the LED core, but you could imagine
being able to associate triggers to specific LEDs. You could have
triggers like "eth-phy-link" and "eth-phy-activity" which the phylib and
phy driver exports. The PHY would export LEDs and say that they can be
used with these triggers. So when for example the "eth-phy-link"
trigger is activated on an LED, the PHY driver programs the hardware
to show the link status on the LED.

The key here is that the hardware is in control of the LED, when the
trigger is used. You don't want software involved when you are
transmitting a million packets per second.

	Andrew

Re: [PATCH] Add support to control PHY LEDs

[Andrew Lunn]

On Thu, Mar 17, 2016 at 02:59:06PM +0100, Vishal Thanki wrote:
> Hi all,
> 
> We had a requirement to disable the PHY link LEDs during system
> standby mode without turning off the ethernet module. To achieve that,
> I have prepared the sysfs interface turn on/off the PHY link LEDs.

Hi Vishal

Could you implement this in at803x_suspend()?

      Andrew