Re: [PATCH leds v1 4/5] leds: trigger: netdev: support HW offloading

["Marek Behún" <marek.behun@nic.cz>]

On Fri, 28 May 2021 15:50:08 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, May 28, 2021 at 08:45:56AM +0200, Marek Behún wrote:
> > On Thu, 27 May 2021 18:57:17 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >   
> > > On Wed, May 26, 2021 at 08:00:19PM +0200, Marek Behún wrote:  
> > > > Add support for HW offloading of the netdev trigger.
> > > > 
> > > > We need to export the netdev_led_trigger variable so that
> > > > drivers may check whether the LED is set to this trigger.    
> > > 
> > > Without seeing the driver side, it is not obvious to me why this
> > > is needed. Please add the driver changes to this patchset, so we
> > > can fully see how the API works.  
> > 
> > OK, I will send an implementation for leds-turris-omnia with v2.
> > 
> > The idea is that the trigger_offload() method should check which
> > trigger it should offload. A potential LED controller may be
> > configured to link the LED on net activity, or on SATA activity. So
> > the method should do something like this:
> > 
> >   static int my_trigger_offload(struct led_classdev *cdev, bool
> > enable) {
> >     if (!enable)
> >       return my_disable_hw_triggering(cdev);
> > 	
> >     if (cdev->trigger == &netdev_led_trigger)
> >       return my_offload_netdev_triggering(cdev);
> >     else if (cdev->trigger == &blkdev_led_trigger)
> >       return my_offload_blkdev_triggering(cdev);
> >     else
> >       return -EOPNOTSUPP;
> >   }  
> 
> So the hardware driver does not need the contents of the trigger? It
> never manipulates the trigger. Maybe to keep the abstraction cleaner,
> an enum can be added to the trigger to identify it. The code then
> becomes:
> 
> static int my_trigger_offload(struct led_classdev *cdev, bool enable)
> {
> 	if (!enable)
>         	return my_disable_hw_triggering(cdev);
>  	
> 	switch(cdev->trigger->trigger) {
> 	case TRIGGER_NETDEV:
> 	       return my_offload_netdev_triggering(cdev);
> 	case TRIGGER_BLKDEV:
> 	       return my_offload_blkdev_triggering(cdev);
> 	default:
> 	       return -EOPNOTSUPP;
> }	

If we want to avoid exporting the symbol I would rather compare
  !strcmp(cdev->trigger->name, "netdev")
What do you think?