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;
> }
--
With Best Regards,
Andy Shevchenko
