Skip to contentSkip to conversation

ACPI: battery: add power_{now, avg} properties to power_class

RFC PATCH In review Alexey Starikovskiy 3 participants 8 messages Dec 15, 2008 – Dec 16, 2008

Subsystems (2)
ACPI Power supply
lore
Share & tools
lore

Alexey Starikovskiy

author
#0 lore raw

RFC PATCH 1/1 ACPI: battery: add power_{now, avg} properties to power_class

ACPI has smart batteries, which work in units of energy and measure rate of (dis)charge as power, thus it is not appropriate to export it as a current_now.

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c             |    3 ++-
 drivers/acpi/sbs.c                 |    6 ++++--
 drivers/power/power_supply_sysfs.c |    2 ++
 include/linux/power_supply.h       |    2 ++
 4 files changed, 10 insertions(+), 3 deletions(-)
drivers/acpi/battery.c +2 -1
index 1423b0c..88f1fb5 100644
@@ -173,6 +173,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = battery->current_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:

@@ -223,7 +224,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_NOW,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
drivers/acpi/sbs.c +4 -2
index 6050ce4..994c04e 100644
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;

@@ -291,8 +293,8 @@ static enum power_supply_property sbs_energy_battery_props[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CURRENT_AVG,
+	POWER_SUPPLY_PROP_POWER_NOW,
+	POWER_SUPPLY_PROP_POWER_AVG,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
drivers/power/power_supply_sysfs.c +2 -0
index 23ae846..7aa8b4e 100644
@@ -93,6 +93,8 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(voltage_avg),
 	POWER_SUPPLY_ATTR(current_now),
 	POWER_SUPPLY_ATTR(current_avg),
+	POWER_SUPPLY_ATTR(power_now),
+	POWER_SUPPLY_ATTR(power_avg),
 	POWER_SUPPLY_ATTR(charge_full_design),
 	POWER_SUPPLY_ATTR(charge_empty_design),
 	POWER_SUPPLY_ATTR(charge_full),
include/linux/power_supply.h +2 -0
index f9348cb..105a707 100644
@@ -72,6 +72,8 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_VOLTAGE_AVG,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
+	POWER_SUPPLY_PROP_POWER_NOW,
+	POWER_SUPPLY_PROP_POWER_AVG,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
#1 lore raw

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 235

Quoted 13 lines · Alexey Starikovskiy
drivers/acpi/sbs.c +2 -0
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;

Excuse me if I am talking nonsense (I have looked over just the patch, not the entire file), but how can that be correct? It is either power or current, it cannot be both, so the CURRENT case should be dropped. And if it is power, why have fields named current_now... or is ipscale() a voltage, and not a scaling factor?

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
author
#2 lore raw
Quoted 10 lines · Henrique de Moraes Holschuh
Quoted 9 lines · Alexey Starikovskiy
drivers/acpi/sbs.c +1 -0
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
Quoted 8 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 235

Quoted 4 lines
drivers/acpi/sbs.c +1 -0
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;

Excuse me if I am talking nonsense (I have looked over just the patch, not the entire file), but how can that be correct? It is either power or current, it cannot be both, so the CURRENT case should be dropped.

file name under /sys depends on property, so if we want some variable to
be named as current_now, it should be returned by case POWER_SUPPLY_PROP_CURRENT_NOW. Then we register with power_supply, we say which set of properties (either charge or energy) we support, so for one battery we will receive request for either CURRENT_AVG or POWER_AVG, not both.

Quoted 2 lines

And if it is power, why have fields named current_now... or is ipscale() a voltage, and not a scaling factor?

ipscale stands for I/P scaling, as opposed to V scaling -- it depends on units returned by actual battery. All energy/charge fields are reused, so battery->current_now contains either power_now or current_now from battery.

#3 lore raw
Quoted 19 lines · Alexey Starikovskiy
Quoted 10 lines · Henrique de Moraes Holschuh
Quoted 9 lines · Alexey Starikovskiy
drivers/acpi/sbs.c +1 -0
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
Quoted 8 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 235

Quoted 4 lines
drivers/acpi/sbs.c +1 -0
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;

Excuse me if I am talking nonsense (I have looked over just the patch, not the entire file), but how can that be correct? It is either power or current, it cannot be both, so the CURRENT case should be dropped.

Quoted 8 lines

file name under /sys depends on property, so if we want some variable to
be named as current_now, it should be returned by case POWER_SUPPLY_PROP_CURRENT_NOW. Then we register with power_supply, we say which set of properties (either charge or energy) we support, so for one battery we will receive request for either CURRENT_AVG or POWER_AVG, not both.

Quoted 2 lines

And if it is power, why have fields named current_now... or is ipscale() a voltage, and not a scaling factor?

ipscale stands for I/P scaling, as opposed to V scaling -- it depends on units returned by actual battery. All energy/charge fields are reused, so battery->current_now contains either power_now or current_now from battery.

I see. But that's a loaded-spring trap waiting for the unaware. Can it be called something else that is neutral re. power or current, pretty please? even "current_or_power_now" would be less confusing...

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
#4 lore raw
Quoted 24 lines · Henrique de Moraes Holschuh
Quoted 23 lines · Alexey Starikovskiy
Quoted 10 lines · Henrique de Moraes Holschuh
Quoted 9 lines · Alexey Starikovskiy
drivers/acpi/sbs.c +1 -0
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
Quoted 8 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 235

Quoted 4 lines
drivers/acpi/sbs.c +1 -0
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;
 		break;

Excuse me if I am talking nonsense (I have looked over just the patch, not the entire file), but how can that be correct? It is either power or current, it cannot be both, so the CURRENT case should be dropped.

file name under /sys depends on property, so if we want some variable to
be named as current_now, it should be returned by case POWER_SUPPLY_PROP_CURRENT_NOW. Then we register with power_supply, we say which set of properties (either charge or energy) we support, so for one battery we will receive request for either CURRENT_AVG or POWER_AVG, not both.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

And if it is power, why have fields named current_now... or is ipscale() a voltage, and not a scaling factor?

ipscale stands for I/P scaling, as opposed to V scaling -- it depends on units returned by actual battery. All energy/charge fields are reused, so battery->current_now contains either power_now or current_now from battery.

I see. But that's a loaded-spring trap waiting for the unaware. Can it be called something else that is neutral re. power or current, pretty please? even "current_or_power_now" would be less confusing...

As I said in my recent reply to Alex, I'd prefer it if there were separate fields called 'power_now' and 'power_avg' under 'battery'.

Then, there won't be any confustion and people reading the source will clearly understand what is what.

Thanks,
Rafael

#5 lore raw
In reply to Alexey Starikovskiy

Hi,

Quoted 40 lines · Alexey Starikovskiy

ACPI has smart batteries, which work in units of energy and measure rate of (dis)charge as power, thus it is not appropriate to export it as a current_now.

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c             |    3 ++-
 drivers/acpi/sbs.c                 |    6 ++++--
 drivers/power/power_supply_sysfs.c |    2 ++
 include/linux/power_supply.h       |    2 ++
 4 files changed, 10 insertions(+), 3 deletions(-)
drivers/acpi/battery.c +2 -1
index 1423b0c..88f1fb5 100644
@@ -173,6 +173,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = battery->current_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:

@@ -223,7 +224,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_NOW,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
drivers/acpi/sbs.c +0 -0
index 6050ce4..994c04e 100644

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 229

Quoted 6 lines
drivers/acpi/sbs.c +1 -0
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;

Please introduce another field called 'battery->power_now' for this purpose. Otherwise, confusion is guaranteed to ensue.

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 234

Quoted 5 lines
drivers/acpi/sbs.c +1 -0
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;

Same here.

Also, as per our IRC conversation, I'd like 'current_now' to be reported even if energy units are used, at least for some time, to give a chance to the user land to switch to 'power_now' and 'power_avg' without pain.

You can put information about that into Documentation/feature-removal-schedule.txt and say that after 2.6.29 'current_now' will no longer be reported when energy units are used.

Thanks,
Rafael

author
#6 lore raw
Quoted 44 lines · Rafael J. Wysocki

Hi,

Quoted 41 lines · Alexey Starikovskiy

ACPI has smart batteries, which work in units of energy and measure rate of (dis)charge as power, thus it is not appropriate to export it as a current_now.

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c             |    3 ++-
 drivers/acpi/sbs.c                 |    6 ++++--
 drivers/power/power_supply_sysfs.c |    2 ++
 include/linux/power_supply.h       |    2 ++
 4 files changed, 10 insertions(+), 3 deletions(-)
drivers/acpi/battery.c +2 -1
index 1423b0c..88f1fb5 100644
@@ -173,6 +173,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = battery->current_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:

@@ -223,7 +224,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_NOW,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
drivers/acpi/sbs.c +0 -0
index 6050ce4..994c04e 100644
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
Quoted 8 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 229

Quoted 5 lines
drivers/acpi/sbs.c +1 -0
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;

Please introduce another field called 'battery->power_now' for this purpose. Otherwise, confusion is guaranteed to ensue.

How about 'battery->rate_now' for both cases? All other fields are re-used as well, and it did not cause any confusion. Probably no-one ever looked at this code.

Quoted 9 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 234

Quoted 6 lines
drivers/acpi/sbs.c +1 -0
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;

Same here.

Quoted 3 lines

Also, as per our IRC conversation, I'd like 'current_now' to be reported even if energy units are used, at least for some time, to give a chance to the user land to switch to 'power_now' and 'power_avg' without pain.

To clarify, you suggest that "current_now" will report same value as "power_now" in case of energy units?

Quoted 4 lines

You can put information about that into Documentation/feature-removal-schedule.txt and say that after 2.6.29 'current_now' will no longer be reported when energy units are used.

Sounds good.

Thanks,
Alex.

Quoted 3 lines

Thanks,
Rafael

#7 lore raw
Quoted 53 lines · Alexey Starikovskiy
Quoted 44 lines · Rafael J. Wysocki

Hi,

Quoted 41 lines · Alexey Starikovskiy

ACPI has smart batteries, which work in units of energy and measure rate of (dis)charge as power, thus it is not appropriate to export it as a current_now.

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/battery.c             |    3 ++-
 drivers/acpi/sbs.c                 |    6 ++++--
 drivers/power/power_supply_sysfs.c |    2 ++
 include/linux/power_supply.h       |    2 ++
 4 files changed, 10 insertions(+), 3 deletions(-)
drivers/acpi/battery.c +2 -1
index 1423b0c..88f1fb5 100644
@@ -173,6 +173,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = battery->current_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:

@@ -223,7 +224,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_NOW,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
drivers/acpi/sbs.c +0 -0
index 6050ce4..994c04e 100644
@@ -224,10 +224,12 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 				acpi_battery_vscale(battery) * 1000;
Quoted 8 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 229

Quoted 5 lines
drivers/acpi/sbs.c +1 -0
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_POWER_NOW:
 		val->intval = abs(battery->current_now) *
 				acpi_battery_ipscale(battery) * 1000;

Please introduce another field called 'battery->power_now' for this purpose. Otherwise, confusion is guaranteed to ensue.

Quoted 3 lines

How about 'battery->rate_now' for both cases? All other fields are re-used as well, and it did not cause any confusion. Probably no-one ever looked at this code.

Well, they are called 'capacity_*', which doesn't imply any particular units, while 'current_*' does.

Anyway, 'battery->rate_now' would be better than 'battery->current_now'. What about 'battery->capacity_flow'?

Quoted 10 lines
Quoted 10 lines

drivers/acpi/sbs.c static int acpi_sbs_battery_get_property(struct power_supply *psy, · line 234

Quoted 7 lines
drivers/acpi/sbs.c +1 -0
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
+	case POWER_SUPPLY_PROP_POWER_AVG:
 		val->intval = abs(battery->current_avg) *
 				acpi_battery_ipscale(battery) * 1000;

Same here.

Quoted 5 lines
Quoted 3 lines

Also, as per our IRC conversation, I'd like 'current_now' to be reported even if energy units are used, at least for some time, to give a chance to the user land to switch to 'power_now' and 'power_avg' without pain.

To clarify, you suggest that "current_now" will report same value as "power_now" in case of energy units?

Yes, with a comment why that is so (ie. because of the userland doing stupid things).

Quoted 4 lines
Quoted 3 lines

You can put information about that into Documentation/feature-removal-schedule.txt and say that after 2.6.29 'current_now' will no longer be reported when energy units are used.

Sounds good.

OK

Thanks,
Rafael