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 <m...@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
