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

[Hans de Goede <hdegoede@redhat.com>]

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;
>>  }
>