| Message ID | 20240510194012.138192-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | power: supply: power-supply-leds: Add activate() callback to triggers | expand |
On Fri, May 10, 2024 at 09:40:10PM +0200, Hans de Goede wrote: > Add power_supply_[un]register_led_trigger() helper functions. > > The primary goal of this is as a preparation patch for adding an activate > callback to the power-supply LED triggers to ensure that power-supply > LEDs get the correct initial value when the LED gets registered after > the power_supply has been registered (this will use the psy back pointer). > > There also is quite a lot of code duplication in the existing LED trigger > registration in the form of the kasprintf() for the name-template for each > trigger + related error handling. This duplication is removed by these > new helpers. ... > + err = led_trigger_register(&psy_trig->trig); > + if (err) > + goto err_free_name; > +err_free_name: > + kfree(psy_trig->trig.name); > +err_free_trigger: > + kfree(psy_trig); > + return -ENOMEM; Why not ret? ... > +static int power_supply_create_bat_triggers(struct power_supply *psy) > +{ > + int err = 0; > + > + err |= power_supply_register_led_trigger(psy, "%s-charging-or-full", > + &psy->charging_full_trig); > + err |= power_supply_register_led_trigger(psy, "%s-charging", > + &psy->charging_trig); > + err |= power_supply_register_led_trigger(psy, "%s-full", > + &psy->full_trig); > + err |= power_supply_register_led_trigger(psy, "%s-charging-blink-full-solid", > + &psy->charging_blink_full_solid_trig); > + err |= power_supply_register_led_trigger(psy, "%s-charging-orange-full-green", > + &psy->charging_orange_full_green_trig); Why not using the similar approach as you have done in v4l2 CCI? > + if (err) { > + power_supply_remove_bat_triggers(psy); > + /* > + * led_trigger_register() may also return -EEXIST but that should > + * never happen with the dynamically generated psy trigger names. > + */ Maybe this comment should be above and here just return err; (but see above remark). > + return -ENOMEM; > + } > + > + return 0; > }
Hi, On Fri, May 10, 2024 at 09:40:09PM +0200, Hans de Goede wrote: > This series adds an activate callback to the power-supply LED triggers to > ensure that power-supply LEDs get the correct initial value when the LED > gets registered after the power_supply has been registered. > > Patches 1-2 do some refactoring / prep work and patch 3 adds the actual > activate callback. > > This series applies on top of "[PATCH v9 6/7] power: supply: power-supply-leds: > Add charging_orange_full_green trigger for RGB LED": > https://lore.kernel.org/all/20240504164105.114017-7-hdegoede@redhat.com/ Apart from Andy's comments LGTM. -- Sebastian
Hi Andy, Thank you for the review. On 5/10/24 10:04 PM, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 09:40:10PM +0200, Hans de Goede wrote: >> Add power_supply_[un]register_led_trigger() helper functions. >> >> The primary goal of this is as a preparation patch for adding an activate >> callback to the power-supply LED triggers to ensure that power-supply >> LEDs get the correct initial value when the LED gets registered after >> the power_supply has been registered (this will use the psy back pointer). >> >> There also is quite a lot of code duplication in the existing LED trigger >> registration in the form of the kasprintf() for the name-template for each >> trigger + related error handling. This duplication is removed by these >> new helpers. > > ... > >> + err = led_trigger_register(&psy_trig->trig); >> + if (err) >> + goto err_free_name; > > >> +err_free_name: >> + kfree(psy_trig->trig.name); >> +err_free_trigger: >> + kfree(psy_trig); >> + return -ENOMEM; > > Why not ret? Fixed for v2. > ... > >> +static int power_supply_create_bat_triggers(struct power_supply *psy) >> +{ >> + int err = 0; >> + >> + err |= power_supply_register_led_trigger(psy, "%s-charging-or-full", >> + &psy->charging_full_trig); >> + err |= power_supply_register_led_trigger(psy, "%s-charging", >> + &psy->charging_trig); >> + err |= power_supply_register_led_trigger(psy, "%s-full", >> + &psy->full_trig); >> + err |= power_supply_register_led_trigger(psy, "%s-charging-blink-full-solid", >> + &psy->charging_blink_full_solid_trig); >> + err |= power_supply_register_led_trigger(psy, "%s-charging-orange-full-green", >> + &psy->charging_orange_full_green_trig); > > Why not using the similar approach as you have done in v4l2 CCI? That is a good idea I've done that for v2. >> + if (err) { >> + power_supply_remove_bat_triggers(psy); >> + /* >> + * led_trigger_register() may also return -EEXIST but that should >> + * never happen with the dynamically generated psy trigger names. >> + */ > > Maybe this comment should be above and here just return err; (but see above remark). With the err propagation approach from v4l2 CCI this can now simply return err since that now correctly reflects the first error (and any following calls are made no-ops). Regards, Hans > >> + return -ENOMEM; >> + } >> + >> + return 0; >> } >
Hi All, This series adds an activate callback to the power-supply LED triggers to ensure that power-supply LEDs get the correct initial value when the LED gets registered after the power_supply has been registered. Patches 1-2 do some refactoring / prep work and patch 3 adds the actual activate callback. This series applies on top of "[PATCH v9 6/7] power: supply: power-supply-leds: Add charging_orange_full_green trigger for RGB LED": https://lore.kernel.org/all/20240504164105.114017-7-hdegoede@redhat.com/ Regards, Hans Hans de Goede (3): power: supply: power-supply-leds: Add power_supply_[un]register_led_trigger() power: supply: power-supply-leds: Share trig pointer for online and charging_full power: supply: power-supply-leds: Add activate() callback to triggers drivers/power/supply/power_supply_leds.c | 176 +++++++++++++---------- include/linux/power_supply.h | 9 +- 2 files changed, 101 insertions(+), 84 deletions(-)