Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

From: Pavel Machek <pavel@ucw.cz>
To: "Marek Behún" <marek.behun@nic.cz>
Cc: "Andrew Lunn" <andrew@lunn.ch>,
	netdev@vger.kernel.org, linux-leds@vger.kernel.org,
	"Dan Murphy" <dmurphy@ti.com>,
	"Ondřej Jirman" <megous@megous.com>,
	"Russell King" <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org,
	"Matthias Schiffer" <matthias.schiffer@ew.tq-group.com>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs
Date: Thu, 10 Sep 2020 22:23:45 +0200	[thread overview]
Message-ID: <20200910202345.GA18431@ucw.cz> (raw)
In-Reply-To: <20200910161522.3cf3ad63@dellmb.labs.office.nic.cz>

Hi!

> Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
> You can enable/disable either of these (via separate sysfs files). `rx`
> and `tx` blink the LED, `link` turns the LED on if the interface is
> linked.

I wonder if people really need separate rx and tx, but... this sounds
reasonable.

> The phy_led_trigger subsystem works differently. Instead of registering
> one trigger (like netdev) it registers one trigger per PHY device and
> per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
> 100Mbps, 10Mbps it registers 3 triggers:
>   XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps

That is not reasonable.

> I propose that at least these HW modes should be available (and
> documented) for ethernet PHY controlled LEDs:

Ok, and which of these will you actually use?

>   mode to determine link on:
>     - `link`
>   mode for activity (these should blink):
>     - `activity` (both rx and tx), `rx`, `tx`
>   mode for link (on) and activity (blink)
>     - `link/activity`, maybe `link/rx` and `link/tx`
>   mode for every supported speed:
>     - `1Gbps`, `100Mbps`, `10Mbps`, ...
>   mode for every supported cable type:
>     - `copper`, `fiber`, ... (are there others?)

That's ... way too many options.

Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no.

If displaying link only for certain speeds is useful, have link_min
and link_max, specifying values in Mbps? Default would be link_min ==
0, and link_max = 25000, so it would react on any link speed.

Is mode for cable type really useful? Then we should have link_fiber?
yes/no. link_copper? yes/no.

>   mode that allows the user to determine link speed
>     - `speed` (or maybe `linkspeed` ?)
>     - on some Marvell PHYs the speed can be determined by how fast
>       the LED is blinking (ie. 1Gbps blinks with default blinking
>       frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
>       of half blinking frequency of 100Mbps)
>     - on other Marvell PHYs this is instead:
>       1Gpbs blinks 3 times, pause, 3 times, pause, ...
>       100Mpbs blinks 2 times, pause, 2 times, pause, ...
>       10Mpbs blinks 1 time, pause, 1 time, pause, ...
>     - we don't need to differentiate these modes with different names,
>       because the important thing is just that this mode allows the
>       user to determine the speed from how the LED blinks

I'd be very careful. Userspace should know what they are asking
for. I'd propose simply ignoring this feature.

>   mode to just force blinking - `blink`

We already have different support for blinking in LED subsystem. Lets use that.

Best regards,
									Pavel