Re: [PATCH 1/3] power: supply: power-supply-leds: Add power_supply_[un]register_led_trigger()

[Andy Shevchenko <andy@kernel.org>]

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