Re: [PATCH 2/2] power: supply: ab8500: Exit maintenance if too low voltage

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 4/19/22 12:26, Matti Vaittinen wrote:

>> --- a/drivers/power/supply/ab8500_chargalg.c
>> +++ b/drivers/power/supply/ab8500_chargalg.c
>> @@ -1514,6 +1514,14 @@ static void ab8500_chargalg_algorithm(struct 
>> ab8500_chargalg *di)
>>               ab8500_chargalg_stop_maintenance_timer(di);
>>               ab8500_chargalg_state_to(di, STATE_MAINTENANCE_B_INIT);
>>           }
>> +        /*
>> +         * This happens if the voltage drops too quickly during
>> +         * maintenance charging, especially in older batteries.
>> +         */
>> +        if (ab8500_chargalg_time_to_restart(di)) {
>> +            ab8500_chargalg_state_to(di, STATE_NORMAL_INIT);
>> +            dev_info(di->dev, "restarted charging from maintenance 
>> state A - battery getting old?\n");
>> +        }
>>           break;
>>       case STATE_MAINTENANCE_B_INIT:
>> @@ -1538,6 +1546,14 @@ static void ab8500_chargalg_algorithm(struct 
>> ab8500_chargalg *di)
>>               ab8500_chargalg_stop_maintenance_timer(di);
>>               ab8500_chargalg_state_to(di, STATE_NORMAL_INIT);
>>           }
>> +        /*
>> +         * This happens if the voltage drops too quickly during
>> +         * maintenance charging, especially in older batteries.
>> +         */
>> +        if (ab8500_chargalg_time_to_restart(di)) {
>> +            ab8500_chargalg_state_to(di, STATE_NORMAL_INIT);
>> +            dev_info(di->dev, "restarted charging from maintenance 
>> state B - battery getting old?\n");
>> +        }
>>           break;
>>       case STATE_TEMP_LOWHIGH_INIT:
> 
> 
> Probably just a matter of taste (like underscores in private function 
> names ;] ) - I would prefer combining the cases for INITs to something 
> like:
> 
>      case STATE_MAINTENANCE_A_INIT:
>      case STATE_MAINTENANCE_B_INIT:
> 
>          mt = power_supply_get_maintenance_charging_setting(bi,
>              (di->charge_state == STATE_MAINTENANCE_B_INIT));
> 
> ...
>          ab8500_chargalg_state_to(di, di->charge_state + 1);
> 
>      break;
> 
> That would slightly reduce the code although at the cost of additional 
> arithmetics. I'm leaving this to you though.

Oh. I was misreading the code. There is fallthrough and not break as the 
actual 'maintenance states' are handled directly after the 'maintenance 
init states'. Well, it seems to me also the actual maintenance states 
could be combined into one case though. But as I wrote - your decision :)

Best Regards
	-- Matti