Re: [PATCH net-next 5/5] igc: Export LEDs

From: Kurt Kanzenbach <kurt@linutronix.de>
To: "Heiner Kallweit" <hkallweit1@gmail.com>,
	"Marek Behún" <kabel@kernel.org>,
	"Michael Walle" <michael@walle.cc>
Cc: andrew@lunn.ch, anthony.l.nguyen@intel.com,
	bigeasy@linutronix.de, davem@davemloft.net,
	dvorax.fuxbrumer@linux.intel.com, f.fainelli@gmail.com,
	jacek.anaszewski@gmail.com, kuba@kernel.org,
	linux-leds@vger.kernel.org, netdev@vger.kernel.org, pavel@ucw.cz,
	sasha.neftin@intel.com, vinicius.gomes@intel.com,
	vitaly.lifshits@intel.com
Subject: Re: [PATCH net-next 5/5] igc: Export LEDs
Date: Thu, 29 Jul 2021 08:39:35 +0200	[thread overview]
Message-ID: <87k0l9r5co.fsf@kurt> (raw)
In-Reply-To: <25d3e798-09f5-56b5-5764-c60435109dd2@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On Wed Jul 28 2021, Heiner Kallweit wrote:
> Did we come to any conclusion?
>
> My preliminary r8169 implementation now creates the following LED
> names:

Is that implementation somewhere available?

>
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led1-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led1-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led2-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led2-0300
>
> I understood that LEDs should at least be renamed to r8169-0300::link-0
> to link-2 Is this correct? Or do we have to wait with any network LED support
> for a name discussion outcome?
>
> For the different LED modes I defined private hw triggers (using trigger_type
> to make the triggers usable with r8169 LEDs only). The trigger attribute now
> looks like this:
>
> [none] link_10_100 link_1000 link_10_100_1000 link_ACT link_10_100_ACT link_1000_ACT link_10_100_1000_ACT
>
> Nice, or? Issue is just that these trigger names really should be made a
> standard for all network LEDs. I don't care about the exact naming, important
> is just that trigger names are the same, no matter whether it's about a r8169-
> or igc- or whatever network chip controlled LEDs.

Your above trigger definitions are OK for the igc as well. Except it
supports up to 2500 Mbit/s. For igc there's also more triggers available
such as filter activity, paused and spd mode.

However, what about the cross LED settings which are common to all LEDs?
The igc has one attribute to control the blink rate of all three LEDs.

>
> And I don't have a good solution for initialization yet. LED mode is whatever
> BIOS sets, but initial trigger value is "none". I would have to read the
> initial LED control register values, iterate over the triggers to find the
> matching one, and call led_trigger_set() to properly set this trigger as
> current trigger.

Yes, there needs to be a way to determine the default state.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]