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 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
