| Message ID | 84c0fa67-2f03-4474-aa75-914d65d88dd0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: trigger: panic: simplify led_trigger_set_panic | expand |
On Sat, 09 Dec 2023, Heiner Kallweit wrote: > I don't see why we iterate over all triggers to find the panic trigger. > We *are* the panic trigger. Therefore we also know that the panic > trigger doesn't have an activate() hook. So we can simplify the code > significantly. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/leds/trigger/ledtrig-panic.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) I would like to take a little advice from the author here who should have been Cc:ed. In case there is information we're missing or overseeing. > diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c > index 72297a845..c646a3cbc 100644 > --- a/drivers/leds/trigger/ledtrig-panic.c > +++ b/drivers/leds/trigger/ledtrig-panic.c > @@ -21,24 +21,15 @@ static struct led_trigger *trigger; > */ > static void led_trigger_set_panic(struct led_classdev *led_cdev) > { > - struct led_trigger *trig; > + if (led_cdev->trigger) > + list_del(&led_cdev->trig_list); > + list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs); > > - list_for_each_entry(trig, &trigger_list, next_trig) { > - if (strcmp("panic", trig->name)) > - continue; > - if (led_cdev->trigger) > - list_del(&led_cdev->trig_list); > - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); > + /* Avoid the delayed blink path */ > + led_cdev->blink_delay_on = 0; > + led_cdev->blink_delay_off = 0; > > - /* Avoid the delayed blink path */ > - led_cdev->blink_delay_on = 0; > - led_cdev->blink_delay_off = 0; > - > - led_cdev->trigger = trig; > - if (trig->activate) > - trig->activate(led_cdev); > - break; > - } > + led_cdev->trigger = trigger; > } > > static int led_trigger_panic_notifier(struct notifier_block *nb, > -- > 2.43.0 >
On 12/9/23 23:54, Heiner Kallweit wrote: > I don't see why we iterate over all triggers to find the panic trigger. > We *are* the panic trigger. Therefore we also know that the panic > trigger doesn't have an activate() hook. So we can simplify the code > significantly. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/leds/trigger/ledtrig-panic.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c > index 72297a845..c646a3cbc 100644 > --- a/drivers/leds/trigger/ledtrig-panic.c > +++ b/drivers/leds/trigger/ledtrig-panic.c > @@ -21,24 +21,15 @@ static struct led_trigger *trigger; > */ > static void led_trigger_set_panic(struct led_classdev *led_cdev) > { > - struct led_trigger *trig; > + if (led_cdev->trigger) > + list_del(&led_cdev->trig_list); > + list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs); > > - list_for_each_entry(trig, &trigger_list, next_trig) { > - if (strcmp("panic", trig->name)) > - continue; > - if (led_cdev->trigger) > - list_del(&led_cdev->trig_list); > - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); > + /* Avoid the delayed blink path */ > + led_cdev->blink_delay_on = 0; > + led_cdev->blink_delay_off = 0; > > - /* Avoid the delayed blink path */ > - led_cdev->blink_delay_on = 0; > - led_cdev->blink_delay_off = 0; > - > - led_cdev->trigger = trig; > - if (trig->activate) > - trig->activate(led_cdev); > - break; > - } > + led_cdev->trigger = trigger; > } > > static int led_trigger_panic_notifier(struct notifier_block *nb, Looks good to me. Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
On Tue, 16 Jan 2024, Jacek Anaszewski wrote: > On 12/9/23 23:54, Heiner Kallweit wrote: > > I don't see why we iterate over all triggers to find the panic trigger. > > We *are* the panic trigger. Therefore we also know that the panic > > trigger doesn't have an activate() hook. So we can simplify the code > > significantly. > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > --- > > drivers/leds/trigger/ledtrig-panic.c | 23 +++++++---------------- > > 1 file changed, 7 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c > > index 72297a845..c646a3cbc 100644 > > --- a/drivers/leds/trigger/ledtrig-panic.c > > +++ b/drivers/leds/trigger/ledtrig-panic.c > > @@ -21,24 +21,15 @@ static struct led_trigger *trigger; > > */ > > static void led_trigger_set_panic(struct led_classdev *led_cdev) > > { > > - struct led_trigger *trig; > > + if (led_cdev->trigger) > > + list_del(&led_cdev->trig_list); > > + list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs); > > - list_for_each_entry(trig, &trigger_list, next_trig) { > > - if (strcmp("panic", trig->name)) > > - continue; > > - if (led_cdev->trigger) > > - list_del(&led_cdev->trig_list); > > - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); > > + /* Avoid the delayed blink path */ > > + led_cdev->blink_delay_on = 0; > > + led_cdev->blink_delay_off = 0; > > - /* Avoid the delayed blink path */ > > - led_cdev->blink_delay_on = 0; > > - led_cdev->blink_delay_off = 0; > > - > > - led_cdev->trigger = trig; > > - if (trig->activate) > > - trig->activate(led_cdev); > > - break; > > - } > > + led_cdev->trigger = trigger; > > } > > static int led_trigger_panic_notifier(struct notifier_block *nb, > > Looks good to me. > > Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> Thanks Jacek. Always helpful.
On Sat, 09 Dec 2023 23:54:51 +0100, Heiner Kallweit wrote: > I don't see why we iterate over all triggers to find the panic trigger. > We *are* the panic trigger. Therefore we also know that the panic > trigger doesn't have an activate() hook. So we can simplify the code > significantly. > > Applied, thanks! [1/1] leds: trigger: panic: simplify led_trigger_set_panic commit: 682e98564ffb67989a77002844767318d366a9ee -- Lee Jones [李琼斯]
diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c index 72297a845..c646a3cbc 100644 --- a/drivers/leds/trigger/ledtrig-panic.c +++ b/drivers/leds/trigger/ledtrig-panic.c @@ -21,24 +21,15 @@ static struct led_trigger *trigger; */ static void led_trigger_set_panic(struct led_classdev *led_cdev) { - struct led_trigger *trig; + if (led_cdev->trigger) + list_del(&led_cdev->trig_list); + list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs); - list_for_each_entry(trig, &trigger_list, next_trig) { - if (strcmp("panic", trig->name)) - continue; - if (led_cdev->trigger) - list_del(&led_cdev->trig_list); - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); + /* Avoid the delayed blink path */ + led_cdev->blink_delay_on = 0; + led_cdev->blink_delay_off = 0; - /* Avoid the delayed blink path */ - led_cdev->blink_delay_on = 0; - led_cdev->blink_delay_off = 0; - - led_cdev->trigger = trig; - if (trig->activate) - trig->activate(led_cdev); - break; - } + led_cdev->trigger = trigger; } static int led_trigger_panic_notifier(struct notifier_block *nb,
I don't see why we iterate over all triggers to find the panic trigger. We *are* the panic trigger. Therefore we also know that the panic trigger doesn't have an activate() hook. So we can simplify the code significantly. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/leds/trigger/ledtrig-panic.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-)