linux-kernel - Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with current draw

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:   Thu, 08 Jun 2017 13:08:52 +0300
From:   Paul Kocialkowski <contact@...lk.fr>
To:     Pavel Machek <pavel@....cz>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Pali Rohár <pali.rohar@...il.com>,
        "Andrew F . Davis" <afd@...com>,
        Sebastian Reichel <sre@...nel.org>,
        Chris Lapa <chris@...a.com.au>,
        Matt Ranostay <mranostay@...il.com>
Subject: Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with
 current draw

Hey,

On Wed, 2017-06-07 at 21:50 +0200, Pavel Machek wrote:
> Hi!
> 
> > On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:
> > > > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> > > > comm
> > > > it/?
> > > > h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
> > > 
> > > Is there some documentation that explains what different power supply
> > > statuses mean? Because without that, we can have long and useless
> > > discussions.
> > 
> > Well, I couldn't really find much except the following from Documentation/
> > (which is not that helpful, and the BATTERY_STATUS_* don't seem to exist
> > anymore):
> > 
> > " STATUS - this attribute represents operating status (charging, full,
> > discharging (i.e. powering a load), etc.). This corresponds to
> > BATTERY_STATUS_* values, as defined in battery.h. "
> > 
> > Generally speaking, I think the question to be asked is what information
> > users
> > will be interested in in each scenario we have to consider.
> 
> Hmm. We really should add some documentation :-(.

Maybe we should start a new thread about this to give it more visibility.
That way, PM maintainers could weigh-in and share thoughts.

I definitely agree there is a need to clarify what we want to report to
userspace given the various scenarios we've been discussing.

> > > If you have 40Wh battery, and you are charging it with 1mW, I don't
> > > believe you should be indicating "charging". That battery is
> > > full. Yes, even full batteries are sometimes charged with very low
> > > currents to keep them full.
> > 
> > That makes sense. Note that this patch was however designed to solve the
> > problem
> > the other way round: my device will report full battery when the PSU was
> > disconnected and that it is, in fact, drawing significant current.
> 
> That is documented / correct behaviour sometimes. Thinkpad batteries
> have thresholds -- lets say 100% and 95%. They charge battery to full
> (as expected), but then they won't start charging battery again unless
> it drops below 95%. So you can have "battery full, charger
> disconnected" state.
> 
> [Design like this prolongs longevity of li-ion batteries.]

That is definitely good to know.

> > > And I'm not sure what this is supposed to do, but its quite strange
> > > code.
> > 
> > Could you comment on what is strange about it? This function corrects the
> > status
> > based on the current flow as explained through this thread.
> > 
> > > +static int sbs_status_correct(struct i2c_client *client, int *intval)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = sbs_read_word_data(client, sbs_data[REG_CURRENT].addr);
> > > +	if (ret < 0)
> > > +	   return ret;
> > > +
> > > +	ret = (s16)ret;
> 
> The last line ... is strange.

The trick here is that sbs_read_word_data will return a negative value (on 32
bits) when an error (say, I/O related) happens, but the current (returned
directly by the call) can also have a legit negative value (current draw).

However, the current value is stored on 16 bytes, so the trick is the use the
upper 2 remaining bytes for error reporting and if there's no error, cast the
value to a signed 16-bit value to get the (legit) signed current value.

-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/