Re: [RFC 3/3] phy,leds: add support for led triggers on phy link state change

2016-09-19 Thread Zach Brown 
On Wed, Sep 14, 2016 at 04:16:15PM -0700, Florian Fainelli wrote:
> On 09/14/2016 02:55 PM, Zach Brown wrote:
> > From: Josh Cartwright 
> > 
> > Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
> > create a set of led triggers for each instantiated PHY device.  There is
> > one LED trigger per link-speed, per-phy.
> > 
> > This allows for a user to configure their system to allow a set of LEDs
> > to represent link state changes on the phy.
> 
> The part that seems to be missing from this changeset (not an issue) is
> how you can "accelerate" the triggers, or rather make sure that the
> trigger configuration potentially calls back into the PHY driver when
> the requested trigger is actually supported by the on-PHY LEDs.
> 
> Other than that, just minor nits here and there.
> 
> > 
> > Signed-off-by: Josh Cartwright 
> > Signed-off-by: Nathan Sullivan 
> > Signed-off-by: Zach Brown 
> > ---
> 
> > +config LED_TRIGGER_PHY
> > +   bool "Support LED triggers for tracking link state"
> > +   depends on LEDS_TRIGGERS
> > +   ---help---
> > + Adds support for a set of LED trigger events per-PHY.  Link
> > + state change will trigger the events, for consumption by an
> > + LED class driver.  There are triggers for each link speed,
> > + and are of the form:
> > +  ::
> > +
> > + Where speed is one of: 10Mb, 100Mb, Gb, 2.5Gb, or 10GbE.
> 
> I would do s/10GbE/10Gb/ just to be consistent with the other speeds, or
> maybe, to avoid too much duplicationg of how we represent speeds, use
> the same set of strings that drivers/net/phy/phy.c::phy_speed_to_str uses.
> 
> 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index c6f6683..3345767 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -936,6 +936,7 @@ void phy_state_machine(struct work_struct *work)
> > phydev->state = PHY_NOLINK;
> > netif_carrier_off(phydev->attached_dev);
> > phydev->adjust_link(phydev->attached_dev);
> > +   phy_led_trigger_change_speed(phydev);
> 
> Since we have essentially two actions to take when performing link state
> changes:
> 
> - call the adjust_link callback
> - call into the LED trigger
> 
> we might consider encapsulating this into a function that could be named
> phy_adjust_link() and does both of these. That would be a preliminary
> patch to this this one.
> 
> >  
> > @@ -345,6 +347,8 @@ struct phy_device *phy_device_create(struct mii_bus 
> > *bus, int addr, int phy_id,
> >  
> > dev->state = PHY_DOWN;
> >  
> > +   phy_led_triggers_register(dev);
> > +
> > mutex_init(>lock);
> > INIT_DELAYED_WORK(>state_queue, phy_state_machine);
> > INIT_WORK(>phy_queue, phy_change);
> 
> Humm, should it be before the PHY state machine workqueue creation or
> after? Just wondering if there is a remote chance we could get an user
> to immediately program a trigger and this could create a problem, maybe
> not so much.
> 
I'm not sure, but I don't think there would be an issue since the interaction
between the triggers and the phy system only goes in one direction. The
triggers don't influence the phy system to my understanding.

> > diff --git a/drivers/net/phy/phy_led_triggers.c 
> > b/drivers/net/phy/phy_led_triggers.c
> > new file mode 100644
> > index 000..6c40414
> > --- /dev/null
> > +++ b/drivers/net/phy/phy_led_triggers.c
> > @@ -0,0 +1,109 @@
> > +/* Copyright (C) 2016 National Instruments Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +#include 
> > +#include 
> > +
> > +void phy_led_trigger_change_speed(struct phy_device *phy)
> > +{
> > +   struct phy_led_trigger *plt;
> > +
> > +   if (!phy->link) {
> > +   if (phy->last_triggered) {
> > +   led_trigger_event(>last_triggered->trigger,
> > + LED_OFF);
> > +   phy->last_triggered = NULL;
> > +   }
> > +   return;
> > +   }
> > +
> > +   switch (phy->speed) {
> > +   case SPEED_10:
> > +   plt = >phy_led_trigger[0];
> > +   break;
> > +   case SPEED_100:
> > +   plt = >phy_led_trigger[1];
> > +   break;
> > +   case SPEED_1000:
> > +   plt = >phy_led_trigger[2];
> > +   break;
> > +   case SPEED_2500:
> > +   plt = >phy_led_trigger[3];
> > +

Re: [RFC 3/3] phy,leds: add support for led triggers on phy link state change

2016-09-14 Thread Florian Fainelli 
On 09/14/2016 02:55 PM, Zach Brown wrote:
> From: Josh Cartwright 
> 
> Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
> create a set of led triggers for each instantiated PHY device.  There is
> one LED trigger per link-speed, per-phy.
> 
> This allows for a user to configure their system to allow a set of LEDs
> to represent link state changes on the phy.

The part that seems to be missing from this changeset (not an issue) is
how you can "accelerate" the triggers, or rather make sure that the
trigger configuration potentially calls back into the PHY driver when
the requested trigger is actually supported by the on-PHY LEDs.

Other than that, just minor nits here and there.

> 
> Signed-off-by: Josh Cartwright 
> Signed-off-by: Nathan Sullivan 
> Signed-off-by: Zach Brown 
> ---

> +config LED_TRIGGER_PHY
> + bool "Support LED triggers for tracking link state"
> + depends on LEDS_TRIGGERS
> + ---help---
> +   Adds support for a set of LED trigger events per-PHY.  Link
> +   state change will trigger the events, for consumption by an
> +   LED class driver.  There are triggers for each link speed,
> +   and are of the form:
> +::
> +
> +   Where speed is one of: 10Mb, 100Mb, Gb, 2.5Gb, or 10GbE.

I would do s/10GbE/10Gb/ just to be consistent with the other speeds, or
maybe, to avoid too much duplicationg of how we represent speeds, use
the same set of strings that drivers/net/phy/phy.c::phy_speed_to_str uses.


> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c6f6683..3345767 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -936,6 +936,7 @@ void phy_state_machine(struct work_struct *work)
>   phydev->state = PHY_NOLINK;
>   netif_carrier_off(phydev->attached_dev);
>   phydev->adjust_link(phydev->attached_dev);
> + phy_led_trigger_change_speed(phydev);

Since we have essentially two actions to take when performing link state
changes:

- call the adjust_link callback
- call into the LED trigger

we might consider encapsulating this into a function that could be named
phy_adjust_link() and does both of these. That would be a preliminary
patch to this this one.

>  
> @@ -345,6 +347,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, 
> int addr, int phy_id,
>  
>   dev->state = PHY_DOWN;
>  
> + phy_led_triggers_register(dev);
> +
>   mutex_init(>lock);
>   INIT_DELAYED_WORK(>state_queue, phy_state_machine);
>   INIT_WORK(>phy_queue, phy_change);

Humm, should it be before the PHY state machine workqueue creation or
after? Just wondering if there is a remote chance we could get an user
to immediately program a trigger and this could create a problem, maybe
not so much.

> diff --git a/drivers/net/phy/phy_led_triggers.c 
> b/drivers/net/phy/phy_led_triggers.c
> new file mode 100644
> index 000..6c40414
> --- /dev/null
> +++ b/drivers/net/phy/phy_led_triggers.c
> @@ -0,0 +1,109 @@
> +/* Copyright (C) 2016 National Instruments Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 
> +#include 
> +
> +void phy_led_trigger_change_speed(struct phy_device *phy)
> +{
> + struct phy_led_trigger *plt;
> +
> + if (!phy->link) {
> + if (phy->last_triggered) {
> + led_trigger_event(>last_triggered->trigger,
> +   LED_OFF);
> + phy->last_triggered = NULL;
> + }
> + return;
> + }
> +
> + switch (phy->speed) {
> + case SPEED_10:
> + plt = >phy_led_trigger[0];
> + break;
> + case SPEED_100:
> + plt = >phy_led_trigger[1];
> + break;
> + case SPEED_1000:
> + plt = >phy_led_trigger[2];
> + break;
> + case SPEED_2500:
> + plt = >phy_led_trigger[3];
> + break;
> + case SPEED_1:
> + plt = >phy_led_trigger[4];
> + break;

We could use a table here indexed by the speed, or have a function that
does phy_speed_to_led_trigger(unsigned int speed) for instance, which
would be more robust to adding other speeds in the future.

> + default:
> + plt = NULL;
> + break;
> + }
> +
> + if (plt != phy->last_triggered) {
> +

[RFC 3/3] phy, leds: add support for led triggers on phy link state change

2016-09-14 Thread Zach Brown 
From: Josh Cartwright 

Create an option CONFIG_LED_TRIGGER_PHY (default n), which will
create a set of led triggers for each instantiated PHY device.  There is
one LED trigger per link-speed, per-phy.

This allows for a user to configure their system to allow a set of LEDs
to represent link state changes on the phy.

Signed-off-by: Josh Cartwright 
Signed-off-by: Nathan Sullivan 
Signed-off-by: Zach Brown 
---
 drivers/net/phy/Kconfig|  12 
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/phy.c  |   8 +++
 drivers/net/phy/phy_device.c   |   4 ++
 drivers/net/phy/phy_led_triggers.c | 109 +
 include/linux/phy.h|   9 +++
 include/linux/phy_led_triggers.h   |  42 ++
 7 files changed, 185 insertions(+)
 create mode 100644 drivers/net/phy/phy_led_triggers.c
 create mode 100644 include/linux/phy_led_triggers.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1c3e07c..4aeaba4 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -15,6 +15,18 @@ if PHYLIB
 config SWPHY
bool
 
+config LED_TRIGGER_PHY
+   bool "Support LED triggers for tracking link state"
+   depends on LEDS_TRIGGERS
+   ---help---
+ Adds support for a set of LED trigger events per-PHY.  Link
+ state change will trigger the events, for consumption by an
+ LED class driver.  There are triggers for each link speed,
+ and are of the form:
+  ::
+
+ Where speed is one of: 10Mb, 100Mb, Gb, 2.5Gb, or 10GbE.
+
 comment "MDIO bus device drivers"
 
 config MDIO_BCM_IPROC
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e58667d..86d12cd 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,6 +2,7 @@
 
 libphy-y   := phy.o phy_device.o mdio_bus.o mdio_device.o
 libphy-$(CONFIG_SWPHY) += swphy.o
+libphy-$(CONFIG_LED_TRIGGER_PHY)   += phy_led_triggers.o
 
 obj-$(CONFIG_PHYLIB)   += libphy.o
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f6683..3345767 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -936,6 +936,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLINK;
netif_carrier_off(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
break;
}
 
@@ -949,6 +950,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 
} else if (0 == phydev->link_timeout--)
needs_aneg = true;
@@ -976,6 +978,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_RUNNING;
netif_carrier_on(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
}
break;
case PHY_FORCING:
@@ -992,6 +995,7 @@ void phy_state_machine(struct work_struct *work)
}
 
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
break;
case PHY_RUNNING:
/* Only register a CHANGE if we are polling and link changed
@@ -1021,6 +1025,7 @@ void phy_state_machine(struct work_struct *work)
}
 
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
 
if (phy_interrupt_is_valid(phydev))
err = phy_config_interrupt(phydev,
@@ -1031,6 +1036,7 @@ void phy_state_machine(struct work_struct *work)
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
do_suspend = true;
}
break;
@@ -1055,6 +1061,7 @@ void phy_state_machine(struct work_struct *work)
phydev->state = PHY_NOLINK;
}
phydev->adjust_link(phydev->attached_dev);
+   phy_led_trigger_change_speed(phydev);
} else {
phydev->state = PHY_AN;
phydev->link_timeout = PHY_AN_TIMEOUT;
@@ -1071,6