| Message ID | 20170430182727.24412-5-contact@paulk.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi! > The status reported directly by the battery controller is not always > reliable and should be corrected based on the current draw information. > > This implements such a correction with a dedicated function, called > when retrieving the supply status. > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di, > else > status = POWER_SUPPLY_STATUS_DISCHARGING; > } else { > + curr = (int)((s16)curr) * 1000; Umm. > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di, > status = POWER_SUPPLY_STATUS_CHARGING; > } > > + > + if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING) > + status = POWER_SUPPLY_STATUS_FULL; > + > + if (status == POWER_SUPPLY_STATUS_FULL) { > + /* Drawing or providing current when full */ > + if (curr > 0) > + status = POWER_SUPPLY_STATUS_CHARGING; > + else if (curr < 0) > + status = POWER_SUPPLY_STATUS_DISCHARGING; > + } Are you sure this works? On N900, we normally see small currents to/from "full" battery. Should the test be for absolute_value(curr) < something rather than for == 0? What hw did you test it on? Pavel
Hi, Le dimanche 28 mai 2017 à 21:16 +0200, Pavel Machek a écrit : > Hi! > > > The status reported directly by the battery controller is not always > > reliable and should be corrected based on the current draw information. > > > > This implements such a correction with a dedicated function, called > > when retrieving the supply status. > > > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct > > bq27xxx_device_info *di, > > else > > status = POWER_SUPPLY_STATUS_DISCHARGING; > > } else { > > + curr = (int)((s16)curr) * 1000; > > Umm. > > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct > > bq27xxx_device_info *di, > > status = POWER_SUPPLY_STATUS_CHARGING; > > } > > > > + > > + if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING) > > + status = POWER_SUPPLY_STATUS_FULL; > > + > > + if (status == POWER_SUPPLY_STATUS_FULL) { > > + /* Drawing or providing current when full */ > > + if (curr > 0) > > + status = POWER_SUPPLY_STATUS_CHARGING; > > + else if (curr < 0) > > + status = POWER_SUPPLY_STATUS_DISCHARGING; > > + } > > Are you sure this works? On N900, we normally see small currents to/from > "full" battery. In my case, this works perfectly and I am quite surprised of what you're describing. Is it the case when the battery has a PSU connected? I guess I would consider this a hardware issue (leak currents) and we could definitely set some range (in device-tree) to distinguish between full + leak currents and bad reporting from the fuel gauge. That would work well in my case too. > Should the test be for absolute_value(curr) < something rather than for == 0? > > What hw did you test it on? I tested this on nyan Chromebooks (Acer Chromebook 13 and HP Chromebook 11) as well as veyron Chromebooks (Chromebook C201PA) that use a bq27xxx fuel gauge. Cheers!
Hi! > > > The status reported directly by the battery controller is not always > > > reliable and should be corrected based on the current draw information. > > > > > > This implements such a correction with a dedicated function, called > > > when retrieving the supply status. > > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct > > > bq27xxx_device_info *di, > > > else > > > status = POWER_SUPPLY_STATUS_DISCHARGING; > > > } else { > > > + curr = (int)((s16)curr) * 1000; > > > > Umm. As in "two casts in one expression -- too ugly to live". > > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct > > > bq27xxx_device_info *di, > > > status = POWER_SUPPLY_STATUS_CHARGING; > > > } > > > > > > + > > > + if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING) > > > + status = POWER_SUPPLY_STATUS_FULL; > > > + > > > + if (status == POWER_SUPPLY_STATUS_FULL) { > > > + /* Drawing or providing current when full */ > > > + if (curr > 0) > > > + status = POWER_SUPPLY_STATUS_CHARGING; > > > + else if (curr < 0) > > > + status = POWER_SUPPLY_STATUS_DISCHARGING; > > > + } > > > > Are you sure this works? On N900, we normally see small currents to/from > > "full" battery. > > In my case, this works perfectly and I am quite surprised of what you're > describing. Is it the case when the battery has a PSU connected? "PSU"? This is cellphone. It has USB connection and charges from that. It has been charging for long while now, and current_now fluctuates between 20706 and -2856. USB has limitted current, so I guess "draw current from battery if we need more than USB can provide" is quite common. pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now 5355 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now 5355 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now -4105 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now -4105 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now -7675 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now -5712 pavel@n900:~$ #screen on pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now 4641 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now 4641 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now 37842 pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now 16600 pavel@n900:~$ > I guess I would consider this a hardware issue (leak currents) and we could > definitely set some range (in device-tree) to distinguish between full + leak > currents and bad reporting from the fuel gauge. That would work well in my case > too. I'd pass to userspace what the controller reports. Yes, I seldom see "STATUS_FULL" but that may be a problem we need to track down. Pavel
Hi, Le mercredi 31 mai 2017 à 19:32 +0200, Pavel Machek a écrit : > The status reported directly by the battery controller is not always > > > > reliable and should be corrected based on the current draw information. > > > > > > > > This implements such a correction with a dedicated function, called > > > > when retrieving the supply status. > > > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct > > > > bq27xxx_device_info *di, > > > > else > > > > status = POWER_SUPPLY_STATUS_DISCHARGING; > > > > } else { > > > > + curr = (int)((s16)curr) * 1000; > > > > > > Umm. > > As in "two casts in one expression -- too ugly to live". Oh, I had skipped that comment, sorry about that. Yeah I understand your concern. However, this line was mostly inspired by another part of the code, below the following comment: /* Other gauges return signed value */ I think we should fix the first occurence first and then used the fixed syntax in v2 of this patch. What do you think? > > > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct > > > > bq27xxx_device_info *di, > > > > status = POWER_SUPPLY_STATUS_CHARGING; > > > > } > > > > > > > > + > > > > + if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING) > > > > + status = POWER_SUPPLY_STATUS_FULL; > > > > + > > > > + if (status == POWER_SUPPLY_STATUS_FULL) { > > > > + /* Drawing or providing current when full */ > > > > + if (curr > 0) > > > > + status = POWER_SUPPLY_STATUS_CHARGING; > > > > + else if (curr < 0) > > > > + status = POWER_SUPPLY_STATUS_DISCHARGING; > > > > + } > > > > > > Are you sure this works? On N900, we normally see small currents to/from > > > "full" battery. > > > > In my case, this works perfectly and I am quite surprised of what you're > > describing. Is it the case when the battery has a PSU connected? > > "PSU"? This is cellphone. It has USB connection and charges from that. > > It has been charging for long while now, and current_now fluctuates > between 20706 and -2856. USB has limitted current, so I guess "draw > current from battery if we need more than USB can provide" is quite common. Ah right, I had forgotten about the USB current limitation thing. In this case, I guess the battery is never actually full and IMO, it should be reported as such. > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > 5355 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > 5355 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > -4105 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > -4105 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > -7675 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > -5712 > pavel@n900:~$ #screen on > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > 4641 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > 4641 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > 37842 > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > 16600 > pavel@n900:~$ > > > I guess I would consider this a hardware issue (leak currents) and we could > > definitely set some range (in device-tree) to distinguish between full + > > leak > > currents and bad reporting from the fuel gauge. That would work well in my > > case > > too. > > I'd pass to userspace what the controller reports. Yes, I seldom see > "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. What do you think?
Le mercredi 31 mai 2017 à 21:28 +0200, Paul Kocialkowski a écrit : > Hi, > > Le mercredi 31 mai 2017 à 19:32 +0200, Pavel Machek a écrit : > > The status reported directly by the battery controller is not always > > > > > reliable and should be corrected based on the current draw > > > > > information. > > > > > > > > > > This implements such a correction with a dedicated function, called > > > > > when retrieving the supply status. > > > > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct > > > > > bq27xxx_device_info *di, > > > > > else > > > > > status = POWER_SUPPLY_STATUS_DISCHARGING; > > > > > } else { > > > > > + curr = (int)((s16)curr) * 1000; > > > > > > > > Umm. > > > > As in "two casts in one expression -- too ugly to live". > > Oh, I had skipped that comment, sorry about that. Yeah I understand your > concern. However, this line was mostly inspired by another part of the code, > below the following comment: /* Other gauges return signed value */ > > I think we should fix the first occurence first and then used the fixed syntax > in v2 of this patch. What do you think? > > > > > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct > > > > > bq27xxx_device_info *di, > > > > > status = POWER_SUPPLY_STATUS_CHARGING; > > > > > } > > > > > > > > > > + > > > > > + if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING) > > > > > + status = POWER_SUPPLY_STATUS_FULL; > > > > > + > > > > > + if (status == POWER_SUPPLY_STATUS_FULL) { > > > > > + /* Drawing or providing current when full */ > > > > > + if (curr > 0) > > > > > + status = POWER_SUPPLY_STATUS_CHARGING; > > > > > + else if (curr < 0) > > > > > + status = POWER_SUPPLY_STATUS_DISCHARGING; > > > > > + } > > > > > > > > Are you sure this works? On N900, we normally see small currents to/from > > > > "full" battery. > > > > > > In my case, this works perfectly and I am quite surprised of what you're > > > describing. Is it the case when the battery has a PSU connected? > > > > "PSU"? This is cellphone. It has USB connection and charges from that. > > > > It has been charging for long while now, and current_now fluctuates > > between 20706 and -2856. USB has limitted current, so I guess "draw > > current from battery if we need more than USB can provide" is quite common. > > Ah right, I had forgotten about the USB current limitation thing. In this > case, > I guess the battery is never actually full and IMO, it should be reported as > such. > > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > 5355 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > 5355 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > -4105 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > -4105 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > -7675 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > -5712 > > pavel@n900:~$ #screen on > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > 4641 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > 4641 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > 37842 > > pavel@n900:~$ cat /sys/class/power_supply/bq27200-0/current_now > > 16600 > > pavel@n900:~$ > > > > > I guess I would consider this a hardware issue (leak currents) and we > > > could > > > definitely set some range (in device-tree) to distinguish between full + > > > leak > > > currents and bad reporting from the fuel gauge. That would work well in my > > > case > > > too. > > > > I'd pass to userspace what the controller reports. Yes, I seldom see > > "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. > What do you think? [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/? h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
Hi! > > > I'd pass to userspace what the controller reports. Yes, I seldom see > > > "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/commit/? > 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. 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. And I'm not sure what this is supposed to do, but its quite strange code. +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; + Best regards, Pavel
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 > > > > "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; > +
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
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.
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
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?
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
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c index cade00df6162..f7694e775e68 100644 --- a/drivers/power/supply/bq27xxx_battery.c +++ b/drivers/power/supply/bq27xxx_battery.c @@ -1171,8 +1171,22 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di, union power_supply_propval *val) { int status; + int curr; + int flags; + + curr = bq27xxx_read(di, BQ27XXX_REG_AI, false); + if (curr < 0) { + dev_err(di->dev, "error reading current\n"); + return curr; + } if (di->chip == BQ27000 || di->chip == BQ27010) { + flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, true); + if (flags & BQ27000_FLAG_CHGS) { + dev_dbg(di->dev, "negative current!\n"); + curr = -curr; + } + if (di->cache.flags & BQ27000_FLAG_FC) status = POWER_SUPPLY_STATUS_FULL; else if (di->cache.flags & BQ27000_FLAG_CHGS) @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di, else status = POWER_SUPPLY_STATUS_DISCHARGING; } else { + curr = (int)((s16)curr) * 1000; + if (di->cache.flags & BQ27XXX_FLAG_FC) status = POWER_SUPPLY_STATUS_FULL; else if (di->cache.flags & BQ27XXX_FLAG_DSC) @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di, status = POWER_SUPPLY_STATUS_CHARGING; } + + if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING) + status = POWER_SUPPLY_STATUS_FULL; + + if (status == POWER_SUPPLY_STATUS_FULL) { + /* Drawing or providing current when full */ + if (curr > 0) + status = POWER_SUPPLY_STATUS_CHARGING; + else if (curr < 0) + status = POWER_SUPPLY_STATUS_DISCHARGING; + } + if (di->status_retry == 0 && di->status_change_reference != status) { di->status_change_reference = status; power_supply_changed(di->bat);
The status reported directly by the battery controller is not always reliable and should be corrected based on the current draw information. This implements such a correction with a dedicated function, called when retrieving the supply status. Signed-off-by: Paul Kocialkowski <contact@paulk.fr> --- drivers/power/supply/bq27xxx_battery.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)