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]

Date:   Wed, 22 Feb 2017 14:22:51 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
Cc:     Chen-Yu Tsai <wens@...e.org>, 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,

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

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