[PATCH 1/2] drivers: net: phy: at803x: Add missing mdio device id

[PATCH 1/2] drivers: net: phy: at803x: Add missing mdio device id

[Helmut Schaa]

at803x supports Atheros 8030, 8031 and 8035 PHYs. 8031 was missing from
the mdio device id table.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 drivers/net/phy/at803x.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 1f7091b..ac22283 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -217,6 +217,7 @@ module_exit(atheros_exit);
 
 static struct mdio_device_id __maybe_unused atheros_tbl[] = {
 	{ 0x004dd076, 0xffffffef },
+	{ 0x004dd074, 0xffffffef },
 	{ 0x004dd072, 0xffffffef },
 	{ }
 };
-- 
1.7.10.4

[PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[Helmut Schaa]

The AT8035 PHY allows to control the LED PIN behavior from software.
Expose this functionality by registering an LED device.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 drivers/net/phy/Kconfig  |    7 ++++
 drivers/net/phy/at803x.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3a316b3..5e855b7 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -19,6 +19,13 @@ config AT803X_PHY
 	---help---
 	  Currently supports the AT8030 and AT8035 model
 
+config AT803X_LEDS
+	bool "Enable access to PHY connected LEDs"
+	depends on AT803X_PHY && LEDS_CLASS
+	---help---
+	  Select this to be able to control LEDs connected to the
+	  PHY from userspace
+
 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 ac22283..49ef6306 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -16,6 +16,7 @@
 #include <linux/string.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/leds.h>
 
 #define AT803X_INTR_ENABLE			0x12
 #define AT803X_INTR_STATUS			0x13
@@ -31,6 +32,8 @@
 #define AT803X_DEBUG_DATA			0x1E
 #define AT803X_DEBUG_SYSTEM_MODE_CTRL		0x05
 #define AT803X_DEBUG_RGMII_TX_CLK_DLY		BIT(8)
+#define AT8035_LED_CTRL				0x18
+#define AT8035_LED_CTRL_OFF			BIT(15)
 
 MODULE_DESCRIPTION("Atheros 803x PHY driver");
 MODULE_AUTHOR("Matus Ujhelyi");
@@ -152,6 +155,86 @@ static int at803x_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+#ifdef CONFIG_AT803X_LEDS
+
+struct at8035_priv {
+	struct led_classdev led;
+	struct phy_device *phydev;
+	struct work_struct led_work;
+	bool stop;
+};
+
+static void at8035_led_work(struct work_struct *work)
+{
+	struct at8035_priv *priv =
+		container_of(work, struct at8035_priv, led_work);
+	struct phy_device *phydev = priv->phydev;
+	int val;
+
+	if (priv->stop)
+		return;
+
+	val = phy_read(phydev, AT8035_LED_CTRL);
+
+	if (priv->led.brightness > 0)
+		phy_write(phydev, AT8035_LED_CTRL, val & ~AT8035_LED_CTRL_OFF);
+	else
+		phy_write(phydev, AT8035_LED_CTRL, val | AT8035_LED_CTRL_OFF);
+}
+
+static void at8035_led_brightness_set(struct led_classdev *led,
+				      enum led_brightness brightness)
+{
+	struct at8035_priv *priv = container_of(led, struct at8035_priv, led);
+
+	/* MDIO bus can only be used from process context */
+	if (!priv->stop)
+		schedule_work(&priv->led_work);
+}
+
+static int at8035_probe(struct phy_device *phydev)
+{
+	struct at8035_priv *priv;
+	int ret;
+
+	priv = kzalloc(sizeof(struct at8035_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	INIT_WORK(&priv->led_work, at8035_led_work);
+
+	priv->phydev = phydev;
+	priv->led.brightness_set = at8035_led_brightness_set;
+	priv->led.max_brightness = 1;
+	priv->led.name = kasprintf(GFP_KERNEL, "at8035_%u::act", phydev->addr);
+
+	if (!priv->led.name) {
+		kfree(priv);
+		return -ENOMEM;
+	}
+
+	ret = led_classdev_register(&phydev->dev, &priv->led);
+	if (ret) {
+		kfree(priv->led.name);
+		kfree(priv);
+		return -ENOMEM;
+	}
+
+	phydev->priv = priv;
+	return 0;
+}
+
+static void at8035_remove(struct phy_device *phydev)
+{
+	struct at8035_priv *priv = phydev->priv;
+	priv->stop = true;
+	cancel_work_sync(&priv->led_work);
+	led_classdev_unregister(&priv->led);
+	kfree(priv->led.name);
+	kfree(priv);
+}
+#endif
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* ATHEROS 8035 */
@@ -163,6 +246,10 @@ static struct phy_driver at803x_driver[] = {
 	.get_wol	= at803x_get_wol,
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
+#ifdef CONFIG_AT803X_LEDS
+	.probe		= at8035_probe,
+	.remove		= at8035_remove,
+#endif
 	.config_aneg	= &genphy_config_aneg,
 	.read_status	= &genphy_read_status,
 	.driver		= {
-- 
1.7.10.4

Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[Florian Fainelli]

Hello Helmut,

2013/7/11 Helmut Schaa <helmut.schaa@googlemail.com>:
> The AT8035 PHY allows to control the LED PIN behavior from software.
> Expose this functionality by registering an LED device.
>
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>  drivers/net/phy/Kconfig  |    7 ++++
>  drivers/net/phy/at803x.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 3a316b3..5e855b7 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -19,6 +19,13 @@ config AT803X_PHY
>         ---help---
>           Currently supports the AT8030 and AT8035 model
>
> +config AT803X_LEDS
> +       bool "Enable access to PHY connected LEDs"
> +       depends on AT803X_PHY && LEDS_CLASS
> +       ---help---
> +         Select this to be able to control LEDs connected to the
> +         PHY from userspace
> +
>  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 ac22283..49ef6306 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -16,6 +16,7 @@
>  #include <linux/string.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
> +#include <linux/leds.h>
>
>  #define AT803X_INTR_ENABLE                     0x12
>  #define AT803X_INTR_STATUS                     0x13
> @@ -31,6 +32,8 @@
>  #define AT803X_DEBUG_DATA                      0x1E
>  #define AT803X_DEBUG_SYSTEM_MODE_CTRL          0x05
>  #define AT803X_DEBUG_RGMII_TX_CLK_DLY          BIT(8)
> +#define AT8035_LED_CTRL                                0x18
> +#define AT8035_LED_CTRL_OFF                    BIT(15)
>
>  MODULE_DESCRIPTION("Atheros 803x PHY driver");
>  MODULE_AUTHOR("Matus Ujhelyi");
> @@ -152,6 +155,86 @@ static int at803x_config_init(struct phy_device *phydev)
>         return 0;
>  }
>
> +#ifdef CONFIG_AT803X_LEDS
> +
> +struct at8035_priv {
> +       struct led_classdev led;
> +       struct phy_device *phydev;
> +       struct work_struct led_work;
> +       bool stop;
> +};
> +
> +static void at8035_led_work(struct work_struct *work)
> +{
> +       struct at8035_priv *priv =
> +               container_of(work, struct at8035_priv, led_work);
> +       struct phy_device *phydev = priv->phydev;
> +       int val;
> +
> +       if (priv->stop)
> +               return;
> +
> +       val = phy_read(phydev, AT8035_LED_CTRL);
> +
> +       if (priv->led.brightness > 0)
> +               phy_write(phydev, AT8035_LED_CTRL, val & ~AT8035_LED_CTRL_OFF);
> +       else
> +               phy_write(phydev, AT8035_LED_CTRL, val | AT8035_LED_CTRL_OFF);
> +}

I do like this and I believe that there is nothing specific that would
prevent you from making software controllable LEDs from being used
with other PHYs. How about you create a small LED class helper that
PHY drivers supporting controllable LEDs can hook into and provide
their own callback and LED name for setting the LED state?

> +
> +static void at8035_led_brightness_set(struct led_classdev *led,
> +                                     enum led_brightness brightness)
> +{
> +       struct at8035_priv *priv = container_of(led, struct at8035_priv, led);
> +
> +       /* MDIO bus can only be used from process context */
> +       if (!priv->stop)
> +               schedule_work(&priv->led_work);

It seems to me that you could remove the stop boolean if you move
led_classdev_unregister upper, provided that it guarantees it will not
schedule the workqueue.
--
Florian

Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[Helmut Schaa]

On Thu, Jul 11, 2013 at 2:15 PM, Florian Fainelli <florian@openwrt.org> wrote:
> Hello Helmut,
>
> 2013/7/11 Helmut Schaa <helmut.schaa@googlemail.com>:
>> The AT8035 PHY allows to control the LED PIN behavior from software.
>> Expose this functionality by registering an LED device.
>>
>> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
>> ---
>>  drivers/net/phy/Kconfig  |    7 ++++
>>  drivers/net/phy/at803x.c |   87 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 94 insertions(+)
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 3a316b3..5e855b7 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -19,6 +19,13 @@ config AT803X_PHY
>>         ---help---
>>           Currently supports the AT8030 and AT8035 model
>>
>> +config AT803X_LEDS
>> +       bool "Enable access to PHY connected LEDs"
>> +       depends on AT803X_PHY && LEDS_CLASS
>> +       ---help---
>> +         Select this to be able to control LEDs connected to the
>> +         PHY from userspace
>> +
>>  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 ac22283..49ef6306 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/string.h>
>>  #include <linux/netdevice.h>
>>  #include <linux/etherdevice.h>
>> +#include <linux/leds.h>
>>
>>  #define AT803X_INTR_ENABLE                     0x12
>>  #define AT803X_INTR_STATUS                     0x13
>> @@ -31,6 +32,8 @@
>>  #define AT803X_DEBUG_DATA                      0x1E
>>  #define AT803X_DEBUG_SYSTEM_MODE_CTRL          0x05
>>  #define AT803X_DEBUG_RGMII_TX_CLK_DLY          BIT(8)
>> +#define AT8035_LED_CTRL                                0x18
>> +#define AT8035_LED_CTRL_OFF                    BIT(15)
>>
>>  MODULE_DESCRIPTION("Atheros 803x PHY driver");
>>  MODULE_AUTHOR("Matus Ujhelyi");
>> @@ -152,6 +155,86 @@ static int at803x_config_init(struct phy_device *phydev)
>>         return 0;
>>  }
>>
>> +#ifdef CONFIG_AT803X_LEDS
>> +
>> +struct at8035_priv {
>> +       struct led_classdev led;
>> +       struct phy_device *phydev;
>> +       struct work_struct led_work;
>> +       bool stop;
>> +};
>> +
>> +static void at8035_led_work(struct work_struct *work)
>> +{
>> +       struct at8035_priv *priv =
>> +               container_of(work, struct at8035_priv, led_work);
>> +       struct phy_device *phydev = priv->phydev;
>> +       int val;
>> +
>> +       if (priv->stop)
>> +               return;
>> +
>> +       val = phy_read(phydev, AT8035_LED_CTRL);
>> +
>> +       if (priv->led.brightness > 0)
>> +               phy_write(phydev, AT8035_LED_CTRL, val & ~AT8035_LED_CTRL_OFF);
>> +       else
>> +               phy_write(phydev, AT8035_LED_CTRL, val | AT8035_LED_CTRL_OFF);
>> +}
>
> I do like this and I believe that there is nothing specific that would
> prevent you from making software controllable LEDs from being used
> with other PHYs. How about you create a small LED class helper that
> PHY drivers supporting controllable LEDs can hook into and provide
> their own callback and LED name for setting the LED state?

Hmm, nice idea. However, there is not much complexity in the code. The only
thing worth to make generic would be the delayed register access via workqueue.
The rest is just registering the led device :)

I thought about moving the code into the generic phy code and using a new
phy_driver callback for setting the LED state but there might be more then one
LED pin on some PHYs (activity, speed, duplexmode etc.).

>> +
>> +static void at8035_led_brightness_set(struct led_classdev *led,
>> +                                     enum led_brightness brightness)
>> +{
>> +       struct at8035_priv *priv = container_of(led, struct at8035_priv, led);
>> +
>> +       /* MDIO bus can only be used from process context */
>> +       if (!priv->stop)
>> +               schedule_work(&priv->led_work);
>
> It seems to me that you could remove the stop boolean if you move
> led_classdev_unregister upper, provided that it guarantees it will not
> schedule the workqueue.

led_classdev_unregister will try to set brightness to 0 and thus schedule
the work.

Thanks,
Helmut

Re: [PATCH 1/2] drivers: net: phy: at803x: Add missing mdio device id

[David Miller]

From: Helmut Schaa <helmut.schaa@googlemail.com>
Date: Thu, 11 Jul 2013 13:57:34 +0200

> at803x supports Atheros 8030, 8031 and 8035 PHYs. 8031 was missing from
> the mdio device id table.
> 
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

Applied.

Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[David Miller]

From: Helmut Schaa <helmut.schaa@googlemail.com>
Date: Thu, 11 Jul 2013 13:57:35 +0200

> The AT8035 PHY allows to control the LED PIN behavior from software.
> Expose this functionality by registering an LED device.
> 
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

As a new feature, it is not appropriate to submit this change at this
time.  Please resubmit this when the net-next tree opens up again.

Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[Helmut Schaa]

David Miller <davem@davemloft.net> schrieb:

>From: Helmut Schaa <helmut.schaa@googlemail.com>
>Date: Thu, 11 Jul 2013 13:57:35 +0200
>
>> The AT8035 PHY allows to control the LED PIN behavior from software.
>> Expose this functionality by registering an LED device.
>> 
>> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
>
>As a new feature, it is not appropriate to submit this change at this
>time.  Please resubmit this when the net-next tree opens up again.

OK, will do.
Helmut

Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[Florian Fainelli]

2013/7/11 Helmut Schaa <helmut.schaa@googlemail.com>:
>>
>> I do like this and I believe that there is nothing specific that would
>> prevent you from making software controllable LEDs from being used
>> with other PHYs. How about you create a small LED class helper that
>> PHY drivers supporting controllable LEDs can hook into and provide
>> their own callback and LED name for setting the LED state?
>
> Hmm, nice idea. However, there is not much complexity in the code. The only
> thing worth to make generic would be the delayed register access via workqueue.
> The rest is just registering the led device :)
>
> I thought about moving the code into the generic phy code and using a new
> phy_driver callback for setting the LED state but there might be more then one
> LED pin on some PHYs (activity, speed, duplexmode etc.).

I think that the same callback for all LEDs and just take actions
based on which led_classdev pointer we get passed should be enough. I
agree with you that there is not much complexity, but for sure, if we
add support for LEDs to another PHY driver there will be similar stuff
done (registering a LED classdev, etc...) so we might just want to
specialize the led_brightness_set() callback and the number of LEDs.
One thing with moving this closer to the PHY state machine would be to
allow for better integration, like setting the LED brightness to 0
when the link goes down etc...

>
>>> +
>>> +static void at8035_led_brightness_set(struct led_classdev *led,
>>> +                                     enum led_brightness brightness)
>>> +{
>>> +       struct at8035_priv *priv = container_of(led, struct at8035_priv, led);
>>> +
>>> +       /* MDIO bus can only be used from process context */
>>> +       if (!priv->stop)
>>> +               schedule_work(&priv->led_work);
>>
>> It seems to me that you could remove the stop boolean if you move
>> led_classdev_unregister upper, provided that it guarantees it will not
>> schedule the workqueue.
>
> led_classdev_unregister will try to set brightness to 0 and thus schedule
> the work.

Ok, thanks!
--
Florian

Re: [PATCH 2/2] drivers: net: phy: at803x: LED control via led subsystem

[Helmut Schaa]

On Fri, Jul 12, 2013 at 9:19 AM, Florian Fainelli <florian@openwrt.org> wrote:
> 2013/7/11 Helmut Schaa <helmut.schaa@googlemail.com>:
>>>
>>> I do like this and I believe that there is nothing specific that would
>>> prevent you from making software controllable LEDs from being used
>>> with other PHYs. How about you create a small LED class helper that
>>> PHY drivers supporting controllable LEDs can hook into and provide
>>> their own callback and LED name for setting the LED state?
>>
>> Hmm, nice idea. However, there is not much complexity in the code. The only
>> thing worth to make generic would be the delayed register access via workqueue.
>> The rest is just registering the led device :)
>>
>> I thought about moving the code into the generic phy code and using a new
>> phy_driver callback for setting the LED state but there might be more then one
>> LED pin on some PHYs (activity, speed, duplexmode etc.).
>
> I think that the same callback for all LEDs and just take actions
> based on which led_classdev pointer we get passed should be enough. I
> agree with you that there is not much complexity, but for sure, if we
> add support for LEDs to another PHY driver there will be similar stuff
> done (registering a LED classdev, etc...) so we might just want to
> specialize the led_brightness_set() callback and the number of LEDs.
> One thing with moving this closer to the PHY state machine would be to
> allow for better integration, like setting the LED brightness to 0
> when the link goes down etc...

Agreed, let me see if I can come up with a more generic approach.
Thanks,
Helmut