Re: [PATCH 08/11] leds: trigger: netdev: add support for LED hw control

[Christian Marangi]

On Sun, Apr 30, 2023 at 07:55:13PM +0200, Andrew Lunn wrote:
> On Thu, Apr 27, 2023 at 02:15:38AM +0200, Christian Marangi wrote:
> > Add support for LED hw control for the netdev trigger.
> > 
> > The trigger on calling set_baseline_state to configure a new mode, will
> > do various check to verify if hw control can be used for the requested
> > mode in the validate_requested_mode() function.
> > 
> > It will first check if the LED driver supports hw control for the netdev
> > trigger, then will check if the requested mode are in the trigger mode
> > mask and finally will call hw_control_set() to apply the requested mode.
> > 
> > To use such mode, interval MUST be set to the default value and net_dev
> > MUST be empty. If one of these 2 value are not valid, hw control will
> > never be used and normal software fallback is used.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 52 +++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index 8cd876647a27..61bc19fd0c7a 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -68,6 +68,13 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  	int current_brightness;
> >  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> >  
> > +	/* Already validated, hw control is possible with the requested mode */
> > +	if (trigger_data->hw_control) {
> > +		led_cdev->hw_control_set(led_cdev, trigger_data->mode);
> > +
> > +		return;
> > +	}
> > +
> >  	current_brightness = led_cdev->brightness;
> >  	if (current_brightness)
> >  		led_cdev->blink_brightness = current_brightness;
> > @@ -95,6 +102,51 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
> >  static int validate_requested_mode(struct led_netdev_data *trigger_data,
> >  				   unsigned long mode, bool *can_use_hw_control)
> >  {
> > +	unsigned int interval = atomic_read(&trigger_data->interval);
> > +	unsigned long hw_supported_mode, hw_mode = 0, sw_mode = 0;
> > +	struct led_classdev *led_cdev = trigger_data->led_cdev;
> > +	unsigned long default_interval = msecs_to_jiffies(50);
> > +	bool force_sw = false;
> > +	int i, ret;
> > +
> > +	hw_supported_mode = led_cdev->trigger_supported_flags_mask;
> > +
> 
> > +		if (interval == default_interval && !trigger_data->net_dev &&
> > +		    !force_sw && test_bit(i, &hw_supported_mode))
> > +			set_bit(i, &hw_mode);
> > +		else
> > +			set_bit(i, &sw_mode);
> > +	}
> > +
> 
> > +	/* Check if the requested mode is supported */
> > +	ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode);
> > +	if (ret)
> > +		return ret;
> 
> Hi Christian
> 
> What is the purpose of led_cdev->trigger_supported_flags_mask? I don't
> see why it is needed when you are also going to ask the PHY if it can
> support the specific blink pattern the user is requesting.

The idea is to have a place where a trigger can quickly check the single
mode supported before the entire mode map is validated, but I understand
that this can totally be dropped with some extra code from both trigger
and LED driver.

While refactoring the netdev triger mode validation I notice it was very
handy and simplified the check logic having a mask of the single mode
supported. But this might be not needed for now and we can think of a
better approach later when we will introduce hardware only modes.

> 
> The problem i have with the Marvell PHY, and other PHYs i've looked at
> datasheets for, is that hardware does not work like this. It has a
> collection of blinking modes, which are a mixture of link speeds, rx
> activity, and tx activity. It supports just a subset of all
> possibilities.
> 
> I think this function can be simplified. Simply ask the LED via
> hw_control_is_supported() does it support this mode. If yes, offload
> it, if not use software blinking.

Yep, I will consider dropping it to slim this series even further.

> 
>     Andrew

-- 
	Ansuel