Re: [PATCH RFC leds + net-next 0/3] Add support for LEDs on Marvell PHYs

From: Andrew Lunn <andrew@lunn.ch>
To: "Marek Behún" <marek.behun@nic.cz>
Cc: linux-leds@vger.kernel.org, "Pavel Machek" <pavel@ucw.cz>,
	jacek.anaszewski@gmail.com, "Dan Murphy" <dmurphy@ti.com>,
	"Ondřej Jirman" <megous@megous.com>,
	netdev@vger.kernel.org, "Russell King" <linux@armlinux.org.uk>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Gregory Clement" <gregory.clement@bootlin.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC leds + net-next 0/3] Add support for LEDs on Marvell PHYs
Date: Thu, 16 Jul 2020 20:56:47 +0200	[thread overview]
Message-ID: <20200716185647.GA1308244@lunn.ch> (raw)
In-Reply-To: <20200716171730.13227-1-marek.behun@nic.cz>

On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote:
> Hello,
> 
> this RFC series should apply on both net-next/master and Pavel's
> linux-leds/for-master tree.
> 
> This adds support for LED's connected to some Marvell PHYs.
> 
> LEDs are specified via device-tree. Example:

Hi Marek

I've been playing with something similar, off and on, mostly off.

Take a look at

https://github.com/lunn/linux v5.4-rc6-hw-led-triggers

The binding i have is pretty much the same, since we are both
following the common LED binding. I see no problems with this.

> This is achieved by extending the LED trigger API with LED-private triggers.
> The proposal for this is based on work by Ondrej and Pavel.

So what i did here was allow triggers to be registered against a
specific LED. The /sys/class/leds/<LED>/trigger lists both the generic
triggers and the triggers for this specific LED. Phylib can then
register a trigger for each blink reason that specific LED can
perform. Which does result in a lot of triggers. Especially when you
start talking about a 10 port switch each with 2 LEDs.

I still have some open issues...

1) Polarity. It would be nice to be able to configure the polarity of
the LED in the bindings.

2) PHY LEDs which are not actually part of the PHY. Most of the
Marvell Ethernet switches have inbuilt PHYs, which are driven by the
Marvell PHY driver. The Marvell PHY driver has no idea the PHY is
inside a switch, it is just a PHY.  However, the LEDs are not
controlled via PHY registers, but Switch registers. So the switch
driver is going to end up controlling these LEDs. It would be good to
be able to share as much code as possible, keep the naming consistent,
and keep the user API the same.

3) Some PHYs cannot control the LEDs independently. Or they have modes
which configure two or more LEDs. The Marvell PHYs are like
this. There are something like ~10 blink modes which are
independent. And then there are 4 modes which control multiple LEDs.
There is no simple way to support this with Linux LEDs which assume
the LEDs are fully independent. I suspect we simply cannot support
these combined modes.

As a PHY maintainer, i would like to see a solution which makes use of
Linux LEDs. I don't really care who's code it is, and feel free to
borrow my code, or ideas, or ignore it.

      Andrew