Re: [PATCH v3 13/18] power: supply: add battery driver for AXP20X and AXP22X PMICs

[Quentin Schulz <quentin.schulz@free-electrons.com>]

Hi Chen-Yu,

On 21/02/2017 05:44, Chen-Yu Tsai wrote:
> On Tue, Feb 14, 2017 at 5:41 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>> The X-Powers AXP20X and AXP22X PMICs can have a battery as power supply.
>>
>> This patch adds the battery power supply driver to get various data from
>> the PMIC, such as the battery status (charging, discharging, full,
>> dead), current max limit, current current, battery capacity (in
>> percentage), voltage max and min limits, current voltage and battery
>> capacity (in Ah).
>>
>> This battery driver uses the AXP20X/AXP22X ADC driver as PMIC data
>> provider.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
[...]
>> +static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
>> +                                             int charge_current)
>> +{
>> +       if (axp_batt->axp_id == AXP209_ID)
>> +               charge_current = (charge_current - 300000) / 100000;
>> +       else
>> +               charge_current = (charge_current - 300000) / 150000;
>> +
>> +       if (charge_current > AXP20X_CHRG_CTRL1_TGT_CURR || charge_current < 0)
>> +               return -EINVAL;
> 
> I would add a check or warn if the value to be programmed exceeds that returned
> by power_supply_get_battery_info.

Completely agree on a warning.

> A charge current exceeding the limits of the
> battery is potentially disastrous. The battery may be destroyed or even burst
> into flames and explode, taking the board and anything nearby with it. Otherwise
> 

Yes, I understand. Maybe I'm seeing this ability to set the (max)
constant current charge the wrong way. Here is what I think:
If we limit the max constant charge current with a DT property, it would
require a DT rebuild when changing the battery (i.e. if an end-user
decides to change the battery with a bigger constant charge current, he
has to recompile the DT to change the DT value).

What I can suggest is the following:
 - set the max constant charge current and the default constant charge
current from the DT property,
 - allow the user to change the constant charge current via sysfs within
minimal-DT value range,
 - allow the user to set max constant charge current via sysfs (and
print a warning as well when setting it), then the user can set a higher
constant charge current,

That would require a two steps modification with a printed warning.
"Safer" but does not remove the ability to change the constant charge
current in the case of battery swapping/changing.

> Acked-by: Chen-Yu Tsai <wens@csie.org>
> 
> Speaking of power_supply_get_battery_info, is it merged or ready to be merged?
> 

v7 under way IIRC.

Thanks,
Quentin

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com