Re: [PATCH] net: phy: leds: Add support for "link" trigger

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/28/2017 08:27 AM, Maciej S. Szmigiero wrote:
> Currently, we create a LED trigger for any link speed known to a PHY.
> These triggers only fire when their exact link speed had been negotiated
> (they aren't cumulative, that is, they don't fire for "their or any higher"
> link speed).
> 
> What we are missing, however, is a trigger which will fire on any link
> speed known to the PHY. Such trigger can then be used for implementing a
> poor man's substitute of the "link" LED on boards that lack it.
> Let's add it.

The use case definitively makes sense, but the implementation a little less.

> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
>  drivers/net/phy/Kconfig            |  7 +++++--
>  drivers/net/phy/phy_led_triggers.c | 19 ++++++++++++++++---
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index cd931cf9dcc2..3bcc2107ad77 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -191,11 +191,14 @@ config LED_TRIGGER_PHY
>  	  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 currently
> -	  supported by the phy, and are of the form:
> +	  supported by the PHY and also a one common "link" trigger as a
> +	  logical-or of all the link speed ones.
> +	  All these triggers are named according to the following pattern:
>  	      <mii bus id>:<phy>:<speed>
>  
>  	  Where speed is in the form:
> -		<Speed in megabits>Mbps or <Speed in gigabits>Gbps
> +		<Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link
> +		for any speed known to the PHY.
>  
>  
>  comment "MII PHY device drivers"
> diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
> index 94ca42e630bb..5b6f1876f514 100644
> --- a/drivers/net/phy/phy_led_triggers.c
> +++ b/drivers/net/phy/phy_led_triggers.c
> @@ -20,7 +20,8 @@ static struct phy_led_trigger *phy_speed_to_led_trigger(struct phy_device *phy,
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < phy->phy_num_led_triggers; i++) {
> +	/* the first (i = 0) trigger is for "any" speed */
> +	for (i = 1; i < phy->phy_num_led_triggers; i++) {
>  		if (phy->phy_led_triggers[i].speed == speed)
>  			return &phy->phy_led_triggers[i];

This looks unnecessary, because existing LED triggers are all relative
to a particular speed, you need to coerce the existing code into
accepting SPEED_UNKNOWN as a hint to mean "link". Just implement "link"
as a different trigger that does not need the speed argument/lookup to
be happening and don't change how the indexing into the array of speed
triggers is done. Just have a separate "link" trigger that is stored
separately from that existing array.

Also, what happens if I set both "link" and a given "speed" and my RJ-45
connector only has two LEDs, and one of them is already used for "activity"?
-- 
Florian