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

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