Hey,

On Wed, 2017-06-07 at 09:52 +0200, Pavel Machek wrote:

I'd pass to userspace what the controller reports. Yes, I seldom see

quoted

quoted

quoted

"STATUS_FULL" but that may be a problem we need to track down.

The controller is known, from my experience, to not be reliable in that
regard,
so I don't think it makes sense to pass a state that doesn't reflect the
actual
state of charging just because the chip tells us so.

Worst case, we could also have a dt property to enable that kind of fixup
workaround and let every device maintainer decide whether it is relevant
for
their device.

Actually, since a similar fix[0] was accepted in sbs-battery, I'd rather not
make this optional but rather make it the default and perhaps have a dt prop
to
disable it.

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

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.

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

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