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

[Ansuel Smith]

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:
> > 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.

> >   - 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
> 
> >   - 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.

> >   - 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