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

[Andrew Lunn <andrew@lunn.ch>]

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@gmail.com>
> ---
>  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 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.

    Andrew