On 30.10.2017 19:36, Florian Fainelli wrote:
> 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.
This way of implementation (overloading SPEED_UNKNOWN) needed least
modification to the driver, but if a purer way is preferred then ok.
> 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"?
One LED can have only one trigger attached AFAIK.
Maciej
