Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660153 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with  current draw Pavel Machek <pavel@ucw.cz> - 2017-06-07 22:00 +0200
    Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with  current draw Paul Kocialkowski <contact@paulk.fr> - 2017-06-08 12:10 +0200
      Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with  current draw Sebastian Reichel <sre@kernel.org> - 2017-06-08 21:30 +0200
        Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with  current draw Paul Kocialkowski <contact@paulk.fr> - 2017-06-09 08:20 +0200
          Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with  current draw Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-06-13 14:20 +0200

#1660153 — Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with current draw

[Multipart message — attachments visible in raw view] — view raw

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

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

> > 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.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1661018

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/

[toc] | [prev] | [next] | [standalone]

#1661607

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Thu, Jun 08, 2017 at 01:08:52PM +0300, Paul Kocialkowski wrote:
> > > 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.

+1 for extension and update of documentation. If its known, that
the battery is trickle charged, it should report FULL. No need
to annoy people by constantly updating the status. Think of it
being mapped directly to a status LED. Of course the CURRENT/ENERGY
properties should still be updated, so that anyone interested in
the details can see them.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1661957

[Multipart message — attachments visible in raw view] — view raw

Le jeudi 08 juin 2017 à 21:27 +0200, Sebastian Reichel a écrit :
> Hi,
> 
> On Thu, Jun 08, 2017 at 01:08:52PM +0300, Paul Kocialkowski wrote:
> > > > 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.
> 
> +1 for extension and update of documentation. If its known, that
> the battery is trickle charged, it should report FULL. No need
> to annoy people by constantly updating the status. Think of it
> being mapped directly to a status LED. Of course the CURRENT/ENERGY
> properties should still be updated, so that anyone interested in
> the details can see them.

Agreed. Do ou think there is a need to start a specific discussion about
various scenarios and how to handle them or do shall we just use common sense
here?

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

[toc] | [prev] | [next] | [standalone]

#1664786

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Jun 09, 2017 at 09:16:01AM +0300, Paul Kocialkowski wrote:
> Le jeudi 08 juin 2017 à 21:27 +0200, Sebastian Reichel a écrit :
> > Hi,
> > 
> > On Thu, Jun 08, 2017 at 01:08:52PM +0300, Paul Kocialkowski wrote:
> > > > > 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.
> > 
> > +1 for extension and update of documentation. If its known, that
> > the battery is trickle charged, it should report FULL. No need
> > to annoy people by constantly updating the status. Think of it
> > being mapped directly to a status LED. Of course the CURRENT/ENERGY
> > properties should still be updated, so that anyone interested in
> > the details can see them.
> 
> Agreed. Do ou think there is a need to start a specific discussion about
> various scenarios and how to handle them or do shall we just use common sense
> here?

I suggest to use common sense and if anything is unclear send a
documentation patch, so that we have a basis for discussion.

-- Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web