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

From: Pavel Machek <pavel@ucw.cz>
To: "Marek Behún" <marek.behun@nic.cz>
Cc: linux-leds@vger.kernel.org, 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>,
	"Andrew Lunn" <andrew@lunn.ch>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC leds + net-next v2 0/1] Add support for LEDs on Marvell PHYs
Date: Fri, 24 Jul 2020 12:29:01 +0200	[thread overview]
Message-ID: <20200724102901.qp65rtkxucauglsp@duo.ucw.cz> (raw)
In-Reply-To: <20200723181319.15988-1-marek.behun@nic.cz>

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

On Thu 2020-07-23 20:13:18, Marek Behún wrote:
> Hi,
> 
> this is v2 of my RFC adding support for LEDs connected to Marvell PHYs.
> 
> The LED subsystem patches are not contained:
> - the patch adding support for LED private triggers is already accepted
>   in Pavel Machek's for-next tree.
>   If you want to try this patch on top of net-next, please also apply
>   https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d
> - the other led-trigger patch is not needed in this version of the RFC
> 
> The main difference from v1 is that only one trigger, named
> "hw-control", is added to the /sys/class/leds/<LED>/trigger file.
> 
> When this trigger is activated, another file called "hw_control" is
> created in the /sys/class/leds/<LED>/ directory. This file lists
> available HW control modes for this LED in the same way the trigger
> file does for triggers.
> 
> Example:
> 
>   # cd /sys/class/leds/eth0\:green\:link/
>   # cat trigger
>   [none] hw-control timer oneshot heartbeat ...
>   # echo hw-control >trigger

Make this "hw-phy-mode" or something. hw-control is a bit too generic.

>   # cat trigger
>   none [hw-control] timer oneshot heartbeat ...
>   # cat hw_control
>   link/nolink link/act/nolink 1000/100/10/nolink act/noact
>   blink-act/noact transmit/notransmit copperlink/else [1000/else]
>   force-hi-z force-blink
>   # echo 1000/100/10/nolink >hw_control
>   # cat hw_control
>   link/nolink link/act/nolink [1000/100/10/nolink] act/noact
>   blink-act/noact transmit/notransmit copperlink/else 1000/else
>   force-hi-z force-blink
> 
> The benefit here is that only one trigger is registered via LED API.
> I guess there are other PHY drivers which too support HW controlled
> blinking modes. So of this way of controlling PHY LED HW controlled
> modes is accepted, the code creating the hw-control trigger and
> hw_control file should be made into library code so that it can be
> reused.
> 
> What do you think?

So.. you have 10 of them right now. I guess both hw_control and making
it into the trigger directly is acceptable here.

In future, would you expect having software "1000/100/10/nolink"
triggers I could activate on my scrollock LED (or on GPIO controlled
LEDs) to indicate network activity?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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