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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c3b21cd4-6d1d-db6a-b1ef-ed119f683035@free-electrons.com>
Date:   Tue, 21 Feb 2017 19:19:39 +0100
From:   Quentin Schulz <quentin.schulz@...e-electrons.com>
To:     Chen-Yu Tsai <wens@...e.org>
Cc:     knaack.h@....de, Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Sebastian Reichel <sre@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Lee Jones <lee.jones@...aro.org>, linux-iio@...r.kernel.org,
        devicetree <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "open list:THERMAL" <linux-pm@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Icenowy Zheng <icenowy@...c.xyz>,
        Bruno Prémont <bonbons@...ux-vserver.org>
Subject: Re: [PATCH v3 13/18] power: supply: add battery driver for AXP20X and
 AXP22X PMICs

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@...e-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@...e-electrons.com>
>> Acked-by: Jonathan Cameron <jic23@...nel.org>
>> Acked-by: Maxime Ripard <maxime.ripard@...e-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@...e.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