From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752947AbdJ3SgS (ORCPT ); Mon, 30 Oct 2017 14:36:18 -0400 Received: from mail-qt0-f196.google.com ([209.85.216.196]:51829 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752903AbdJ3SgO (ORCPT ); Mon, 30 Oct 2017 14:36:14 -0400 X-Google-Smtp-Source: ABhQp+QMPpjhmuTxnnAn1MjgwKhlqYhkocDNlfTVow8hJt15ORaxZP16ZXTF3rI7Vx4JmImuynEHsw== Subject: Re: [PATCH] net: phy: leds: Add support for "link" trigger To: "Maciej S. Szmigiero" , Andrew Lunn Cc: netdev@vger.kernel.org, linux-kernel References: <65b5c84f-1222-bbc3-24f5-723e07bbb6ed@maciej.szmigiero.name> From: Florian Fainelli Message-ID: <74fd5477-9165-3372-f386-3f46948b79d6@gmail.com> Date: Mon, 30 Oct 2017 11:36:09 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <65b5c84f-1222-bbc3-24f5-723e07bbb6ed@maciej.szmigiero.name> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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: > :: > > Where speed is in the form: > - Mbps or Gbps > + Mbps OR 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