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
