| Message ID | 20230427001541.18704-9-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: introduce new LED hw control APIs | expand |
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@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 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
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; + + /* Check if trigger can use hw control */ + if (!led_trigger_can_hw_control(led_cdev)) + force_sw = true; + + /* Compose a list of mode that can run in hw or sw */ + for (i = 0; i < __TRIGGER_NETDEV_MAX; i++) { + /* Skip checking mode not active */ + if (!test_bit(i, &mode)) + continue; + + /* net_dev is not supported and must be empty for hw control. + * interval must be set to the default value. Any different + * value is rejected if in hw control. + */ + 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); + } + + /* We can't run modes handled by both sw and hw */ + if (sw_mode && hw_mode) + return -EINVAL; + + /* Exit early if we are using software fallback */ + if (sw_mode) + return 0; + + /* Check if the requested mode is supported */ + ret = led_cdev->hw_control_is_supported(led_cdev, hw_mode); + if (ret) + return ret; + + *can_use_hw_control = true; + return 0; }
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(+)