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

[Marek Behún]

On Mon, 8 Nov 2021 16:16:13 +0100
Ansuel Smith <ansuelsmth@xxxxxxxxx> wrote:

> On Mon, Nov 08, 2021 at 03:04:23PM +0100, Andrew Lunn wrote:
> > > +static inline int led_trigger_offload(struct led_classdev *led_cdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!led_cdev->trigger_offload)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	ret = led_cdev->trigger_offload(led_cdev, true);
> > > +	led_cdev->offloaded = !ret;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
> > > +{
> > > +	if (!led_cdev->trigger_offload)
> > > +		return;
> > > +
> > > +	if (led_cdev->offloaded) {
> > > +		led_cdev->trigger_offload(led_cdev, false);
> > > +		led_cdev->offloaded = false;
> > > +	}
> > > +}
> > > +#endif  
> > 
> > I think there should be two calls into the cdev driver, not this
> > true/false parameter. trigger_offload_start() and
> > trigger_offload_stop().
> >   
> 
> To not add too much function to the struct, can we introduce one
> function that both enable and disable the hw mode?

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