Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

From: Andrew Lunn <andrew@lunn.ch>
To: "Marek Behún" <kabel@kernel.org>
Cc: Ansuel Smith <ansuelsmth@gmail.com>,
	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 1/5] leds: trigger: add API for HW offloading of triggers
Date: Mon, 8 Nov 2021 18:46:26 +0100	[thread overview]
Message-ID: <YYliclrZuxG/laIh@lunn.ch> (raw)
In-Reply-To: <20211108171312.0318b960@thinkpad>

> Dear Ansuel,
> 
> what is the purpose of adding trigger_offload() methods to LED, if you
> are not going to add support to offload the netdev trigger? That was
> the entire purpose when I wrote that patch.
> 
> If you just want to create a new trigger that will make the PHY chip do
> the blinking, there is no need at all for the offloading patch.
> 
> And you will also get a NACK from me and also Pavel (LED subsystem
> maintainer).
> 
> The current plan is to:
> - add support for offloading existing LED triggers to HW (LED
>   controllers (PHY chips, for example))
> - make netdev trigger try offloading itself to HW via this new API (if
>   it fails, netdev trigger will blink the LED in SW as it does now)
> - create LED classdevices in a PHY driver that have the offload()
>   methods implemented. The offload method looks at what trigger is
>   being enabled for the LED, and it if it is a netdev trigger with such
>   settings that are possible to offload, it will be offloaded.
> 
>   This whole thing makes use of the existing sysfs ABI.
>   So for example if I do
>     cd /sys/class/net/eth0/phydev/leds/<LED>
>     echo netdev >trigger
>     echo eth0 >device_name
>     echo 1 >rx
>     echo 1 >tx
>   The netdev trigger is activated, and it calls the offload() method.
>   The offload() method is implemented in the PHY driver, and it checks
>   that it can offload these settings (blink on rx/tx), and will enable
>   this.
> - extend netdev trigger to support more settings:
>   - indicate link for specific link modes only (for example 1g, 100m)
>   - ...
> - extend PHY drivers to support offloading of these new settings
> 
> Marek

Hi Marek

The problem here is, you are not making much progress. People are
giving up on you ever getting this done, and doing their own
implementation. Ansuel code is not mature enough yet, it has problems,
but he is responsive, he is dealing with comments, progress is being
made. At some point, it is going to be good enough, and it will get
merged, unless you actual get your code to a point it can be merged.

	Andrew