This passes the of_node from the bq27xxx i2c battery driver to the
common code, so that it can be registered and provide external supplies
linked with device-tree.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/power/supply/bq27xxx_battery.c | 5 ++++-
drivers/power/supply/bq27xxx_battery_i2c.c | 1 +
include/linux/power/bq27xxx_battery.h | 1 +
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 398801a21b86..6ef95442a918 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1351,7 +1351,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
{
struct power_supply_desc *psy_desc;
- struct power_supply_config psy_cfg = { .drv_data = di, };
+ struct power_supply_config psy_cfg = {};
+
+ psy_cfg.drv_data = di;
+ psy_cfg.of_node = di->of_node;
INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
mutex_init(&di->lock);
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index c68fbc3fe50a..38a0422a4192 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -96,6 +96,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
di->chip = id->driver_data;
di->name = name;
di->bus.read = bq27xxx_battery_i2c_read;
+ di->of_node = client->dev.of_node;
ret = bq27xxx_battery_setup(di);
if (ret)
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index b312bcef53da..94637b77ecbf 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -63,6 +63,7 @@ struct bq27xxx_device_info {
const char *name;
struct bq27xxx_access_methods bus;
struct bq27xxx_reg_cache cache;
+ struct device_node *of_node;
int charge_design_full;
unsigned long last_update;
struct delayed_work work;
--
2.12.2
This uses the managed devices resources version of the
power_supply_register_no_ws function to register the power supply.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/power/supply/bq27xxx_battery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 6ef95442a918..be476e0bc85d 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1371,7 +1371,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
psy_desc->get_property = bq27xxx_battery_get_property;
psy_desc->external_power_changed = bq27xxx_external_power_changed;
- di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
+ di->bat = devm_power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
if (IS_ERR(di->bat)) {
dev_err(di->dev, "failed to register battery\n");
return PTR_ERR(di->bat);
--
2.12.2
This introduces a dedicated status change work to look for power
status change. It is triggered by external power change notifications
and periodically retries detecting a power status change for 5 seconds.
This is largely inspired by a similar mechanism from the sbs-battery
driver.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/power/supply/bq27xxx_battery.c | 38 ++++++++++++++++++++++++++++++++--
include/linux/power/bq27xxx_battery.h | 3 +++
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 926bd58344d9..cade00df6162 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1190,6 +1190,11 @@ static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
status = POWER_SUPPLY_STATUS_CHARGING;
}
+ if (di->status_retry == 0 && di->status_change_reference != status) {
+ di->status_change_reference = status;
+ power_supply_changed(di->bat);
+ }
+
val->intval = status;
return 0;
@@ -1340,12 +1345,38 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
return ret;
}
+static void bq27xxx_status_change(struct work_struct *work)
+{
+ struct bq27xxx_device_info *di =
+ container_of(work, struct bq27xxx_device_info,
+ status_work.work);
+ union power_supply_propval val;
+ int ret;
+
+ bq27xxx_battery_update(di);
+
+ ret = bq27xxx_battery_status(di, &val);
+ if (ret < 0)
+ return;
+
+ if (di->status_change_reference != val.intval) {
+ di->status_change_reference = val.intval;
+ power_supply_changed(di->bat);
+ }
+
+ if (di->status_retry > 0) {
+ di->status_retry--;
+ schedule_delayed_work(&di->status_work, HZ);
+ }
+}
+
static void bq27xxx_external_power_changed(struct power_supply *psy)
{
struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
- cancel_delayed_work_sync(&di->poll_work);
- schedule_delayed_work(&di->poll_work, 0);
+ cancel_delayed_work_sync(&di->status_work);
+ schedule_delayed_work(&di->status_work, HZ);
+ di->status_retry = 5;
}
int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
@@ -1357,8 +1388,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
psy_cfg.of_node = di->of_node;
INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
+ INIT_DELAYED_WORK(&di->status_work, bq27xxx_status_change);
mutex_init(&di->lock);
di->regs = bq27xxx_regs[di->chip];
+ di->status_change_reference = POWER_SUPPLY_STATUS_UNKNOWN;
psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
if (!psy_desc)
@@ -1400,6 +1433,7 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
poll_interval = 0;
cancel_delayed_work_sync(&di->poll_work);
+ cancel_delayed_work_sync(&di->status_work);
power_supply_unregister(di->bat);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 0a9af513165a..16d604681ace 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -67,6 +67,9 @@ struct bq27xxx_device_info {
int charge_design_full;
unsigned long last_update;
struct delayed_work poll_work;
+ struct delayed_work status_work;
+ int status_retry;
+ int status_change_reference;
struct power_supply *bat;
struct list_head list;
struct mutex lock;
--
2.12.2
This renames the work structure member to poll_work, in anticipation
of the introduction of a status_work member used to detect status
changes.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/power/supply/bq27xxx_battery.c | 20 ++++++++++----------
drivers/power/supply/bq27xxx_battery_i2c.c | 2 +-
include/linux/power/bq27xxx_battery.h | 2 +-
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index be476e0bc85d..926bd58344d9 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -769,8 +769,8 @@ static int poll_interval_param_set(const char *val, const struct kernel_param *k
mutex_lock(&bq27xxx_list_lock);
list_for_each_entry(di, &bq27xxx_battery_devices, list) {
- cancel_delayed_work_sync(&di->work);
- schedule_delayed_work(&di->work, 0);
+ cancel_delayed_work_sync(&di->poll_work);
+ schedule_delayed_work(&di->poll_work, 0);
}
mutex_unlock(&bq27xxx_list_lock);
@@ -1126,12 +1126,12 @@ static void bq27xxx_battery_poll(struct work_struct *work)
{
struct bq27xxx_device_info *di =
container_of(work, struct bq27xxx_device_info,
- work.work);
+ poll_work.work);
bq27xxx_battery_update(di);
if (poll_interval > 0)
- schedule_delayed_work(&di->work, poll_interval * HZ);
+ schedule_delayed_work(&di->poll_work, poll_interval * HZ);
}
/*
@@ -1265,8 +1265,8 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
mutex_lock(&di->lock);
if (time_is_before_jiffies(di->last_update + 5 * HZ)) {
- cancel_delayed_work_sync(&di->work);
- bq27xxx_battery_poll(&di->work.work);
+ cancel_delayed_work_sync(&di->poll_work);
+ bq27xxx_battery_poll(&di->poll_work.work);
}
mutex_unlock(&di->lock);
@@ -1344,8 +1344,8 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
{
struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
- cancel_delayed_work_sync(&di->work);
- schedule_delayed_work(&di->work, 0);
+ cancel_delayed_work_sync(&di->poll_work);
+ schedule_delayed_work(&di->poll_work, 0);
}
int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
@@ -1356,7 +1356,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
psy_cfg.drv_data = di;
psy_cfg.of_node = di->of_node;
- INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
+ INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
mutex_init(&di->lock);
di->regs = bq27xxx_regs[di->chip];
@@ -1399,7 +1399,7 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
*/
poll_interval = 0;
- cancel_delayed_work_sync(&di->work);
+ cancel_delayed_work_sync(&di->poll_work);
power_supply_unregister(di->bat);
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 38a0422a4192..1b2ad22190ae 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -103,7 +103,7 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
goto err_failed;
/* Schedule a polling after about 1 min */
- schedule_delayed_work(&di->work, 60 * HZ);
+ schedule_delayed_work(&di->poll_work, 60 * HZ);
i2c_set_clientdata(client, di);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 94637b77ecbf..0a9af513165a 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -66,7 +66,7 @@ struct bq27xxx_device_info {
struct device_node *of_node;
int charge_design_full;
unsigned long last_update;
- struct delayed_work work;
+ struct delayed_work poll_work;
struct power_supply *bat;
struct list_head list;
struct mutex lock;
--
2.12.2
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 <[email protected]>
---
drivers/power/supply/bq27xxx_battery.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
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);
--
2.12.2
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
--
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/
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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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;
> +
--
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/
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
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/
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?
--
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/
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