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

[Chen-Yu Tsai <wens@csie.org>]

Hi,

On Wed, Feb 22, 2017 at 2:19 AM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> 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).

I guess this mostly applies for development boards.

Swapping out the battery would also affect design capacity, which may be
exported to userspace via the power supply / fuel gauge device? What then?

> What I can suggest is the following:
>  - set the max constant charge current and the default constant charge
> current from the DT property,

OK.

>  - allow the user to change the constant charge current via sysfs within
> minimal-DT value range,

You mean the "current" max constant charge current known to the driver, right?

>  - 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.

Yeah that sounds like a good proposal. If you want to be really safe,
you could make the option to override the maximum a sysfs option? It
is possible warning messages aren't immediately noticed, such as when
the user is running X.

Reminds me of the anti foot-shooting option in FreeBSD.

Regards
ChenYu


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