Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 8 Nov 2021 16:19:12 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>, Pavel Machek <pavel@ucw.cz>,
        John Crispin <john@phrozen.org>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [RFC PATCH v2 3/5] leds: trigger: add offload-phy-activity
 trigger
Message-ID: <YYk/8IIhCYUZFcy4@Ansuel-xps.localdomain>
References: <20211108002500.19115-1-ansuelsmth@gmail.com>
 <20211108002500.19115-4-ansuelsmth@gmail.com>
 <YYkxfRrJ8ERaTr5x@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YYkxfRrJ8ERaTr5x@lunn.ch>
Precedence: bulk

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