Re: [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity trigger

On Mon, Nov 08, 2021 at 03:17:33PM +0100, Andrew Lunn wrote:

On Mon, Nov 08, 2021 at 01:24:58AM +0100, Ansuel Smith wrote:

quoted

Add Offload Trigger for PHY Activity. This special trigger is used to
configure and expose the different HW trigger that are provided by the
PHY. Each offload trigger can be configured by sysfs and on trigger
activation the offload mode is enabled.

This currently implement these hw triggers:
  - blink_tx: Blink LED on tx packet receive
  - blink_rx: Blink LED on rx packet receive
  - blink_collision: Blink LED on collision detection

When did you last see a collision? Do you really have a 1/2 duplex
link? Just because the PHY can, does not mean we should support
it. Lets restrict this to the most useful modes.

Ok will drop this. In my case (qca8k) I also never see a device using it
so I agree on the fact that should be dropped.

quoted

  - link_10m: Keep LED on with 10m link speed
  - link_100m: Keep LED on with 100m link speed
  - link_1000m: Keep LED on with 1000m link speed
  - half_duplex: Keep LED on with half duplex link
  - full_duplex: Keep LED on with full duplex link
  - linkup_over: Keep LED on with link speed and blink on rx/tx traffic
  - power_on_reset: Keep LED on with switch reset

quoted

  - blink_2hz: Set blink speed at 2hz for every blink event
  - blink_4hz: Set blink speed at 4hz for every blink event
  - blink_8hz: Set blink speed at 8hz for every blink event

These seems like attributes, not blink modes. They need to be
specified somehow differently, or not at all. Do we really need them?

Sorry I didn't update the commit. In sysfs they are exposed as option
like the power_on_reset and linkup_over. So they are option on how the
LED behave on the event.

quoted

  - blink_auto: Set blink speed at 2hz for 10m link speed,
      4hz for 100m and 8hz for 1000m

Another attribute, and one i've not seen any other PHY do.

Yes we can consider dropping this but I think the other 3 should be
keeped.

	Andrew

-- 
	Ansuel