Work around negative s16 battery current on Acer

PATCH In review Hector Martin 3 participants 8 messages Jul 1, 2009 – Jul 2, 2009 ACPI

Share & tools

Hector Martin

author

2009-07-01 18:29 UTC #0 lore raw

PATCH 1/1 Work around negative s16 battery current on Acer

My Acer Aspire 8930G laptop reports the battery current as a 16-bit signed negative when it is charging. It also reports it as 0x10000 when the current is 0. This patch adds a quirk for this which takes the absolute value of the reported current cast to an s16. This is a DSDT bug present in the latest BIOS revision (the EC register is 16 bits signed and the DSDT attempts to take the 16-bit two's complement of this, which works for discharge but not charge. It also breaks zero values because a 32-bit register is used and the high bits aren't thrown away).

I've enabled this for all Acer systems which report in mA units. This should be safe since it won't break compliant systems unless they report a current above 32A, which is insane.

Signature · 2 lines

Hector Martin ([email protected])
Public Key: http://www.marcansoft.com/marcan.asc

Signed-off-by Hector Martin <[email protected]>

drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +18 -0

@@ -84,6 +84,10 @@
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* For buggy DSDTs that report negative 16-bit values for either charging
+ * or discharging and/or report 0 as 65536 due to bad math.
+ */
+#define QUIRK_SIGNED16_CURRENT 0x0001
 
 struct acpi_battery {
 	struct mutex lock;

@@ -111,6 +115,7 @@
 	int state;
 	int power_unit;
 	u8 alarm_present;
+	long quirks;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

@@ -387,6 +392,10 @@
 				 state_offsets, ARRAY_SIZE(state_offsets));
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
+
+	if (battery->quirks & QUIRK_SIGNED16_CURRENT)
+		battery->current_now = abs((s16)battery->current_now);
+
 	return result;
 }

@@ -492,6 +501,14 @@
 }
 #endif
 
+static void acpi_battery_quirks(struct acpi_battery *battery)
+{
+	battery->quirks = 0;
+	if (dmi_name_in_vendors("Acer") && battery->power_unit) {
+		battery->quirks |= QUIRK_SIGNED16_CURRENT;
+	}
+}
+
 static int acpi_battery_update(struct acpi_battery *battery)
 {
 	int result, old_present = acpi_battery_present(battery);

@@ -510,6 +527,7 @@
 		result = acpi_battery_get_info(battery);
 		if (result)
 			return result;
+		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
 #ifdef CONFIG_ACPI_SYSFS_POWER

Andrew Morton

2009-07-01 18:31 UTC #1 lore raw

Quoted 38 lines · On Wed, 01 Jul 2009 20:20:41 +0200 Hector Martin <[email protected]> wrote:

My Acer Aspire 8930G laptop reports the battery current as a 16-bit signed negative when it is charging. It also reports it as 0x10000 when the current is 0. This patch adds a quirk for this which takes the absolute value of the reported current cast to an s16. This is a DSDT bug present in the latest BIOS revision (the EC register is 16 bits signed and the DSDT attempts to take the 16-bit two's complement of this, which works for discharge but not charge. It also breaks zero values because a 32-bit register is used and the high bits aren't thrown away).
I've enabled this for all Acer systems which report in mA units. This should be safe since it won't break compliant systems unless they report a current above 32A, which is insane.
...
+++ a/drivers/acpi/battery.c
drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +5 -0
@@ -85,6 +85,10 @@ static const struct acpi_device_id batte
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* For buggy DSDTs that report negative 16-bit values for either charging
+ * or discharging and/or report 0 as 65536 due to bad math.
+ */
+#define QUIRK_SIGNED16_CURRENT 0x0001
 
 struct acpi_battery {
 	struct mutex lock;
@@ -112,6 +116,7 @@ struct acpi_battery {
 	int state;
 	int power_unit;
 	u8 alarm_present;
+	long quirks;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

drivers/acpi/battery.c static int acpi_battery_get_state(struct · line 402

Quoted 8 lines

drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +4 -0

@@ -390,6 +395,10 @@ static int acpi_battery_get_state(struct
 				 state_offsets, ARRAY_SIZE(state_offsets));
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
+
+	if (battery->quirks & QUIRK_SIGNED16_CURRENT)
+		battery->current_now = abs((s16)battery->current_now);
+
 	return result;

acpi_battery has no field `current_now' in 2.6.30 or 2.6.31-rc1. Which kernel version are you patching here?

Also, I wonder if we need a quirk. Is a "negative" value _ever_ correct? If not, could we do the negation unconditionally?

Alexey Starikovskiy

2009-07-01 18:38 UTC #2 lore raw

Andrew Morton пишет:

Quoted 54 lines

Quoted 38 lines · On Wed, 01 Jul 2009 20:20:41 +0200 Hector Martin <[email protected]> wrote:
My Acer Aspire 8930G laptop reports the battery current as a 16-bit signed negative when it is charging. It also reports it as 0x10000 when the current is 0. This patch adds a quirk for this which takes the absolute value of the reported current cast to an s16. This is a DSDT bug present in the latest BIOS revision (the EC register is 16 bits signed and the DSDT attempts to take the 16-bit two's complement of this, which works for discharge but not charge. It also breaks zero values because a 32-bit register is used and the high bits aren't thrown away).
I've enabled this for all Acer systems which report in mA units. This should be safe since it won't break compliant systems unless they report a current above 32A, which is insane.
...
+++ a/drivers/acpi/battery.c
drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +5 -0
@@ -85,6 +85,10 @@ static const struct acpi_device_id batte
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* For buggy DSDTs that report negative 16-bit values for either charging
+ * or discharging and/or report 0 as 65536 due to bad math.
+ */
+#define QUIRK_SIGNED16_CURRENT 0x0001
 
 struct acpi_battery {
 	struct mutex lock;
@@ -112,6 +116,7 @@ struct acpi_battery {
 	int state;
 	int power_unit;
 	u8 alarm_present;
+	long quirks;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
drivers/acpi/battery.c static int acpi_battery_get_state(struct · line 402
Quoted 8 lines
drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +4 -0
@@ -390,6 +395,10 @@ static int acpi_battery_get_state(struct
 				 state_offsets, ARRAY_SIZE(state_offsets));
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
+
+	if (battery->quirks & QUIRK_SIGNED16_CURRENT)
+		battery->current_now = abs((s16)battery->current_now);
+
 	return result;
acpi_battery has no field `current_now' in 2.6.30 or 2.6.31-rc1. Which kernel version are you patching here?

Quoted 3 lines

Also, I wonder if we need a quirk. Is a "negative" value _ever_ correct? If not, could we do the negation unconditionally?

The problem is that the variable is s64 and not "negative" value is related to s16 portion of it.
It's possible to do this only for "current" mode, in "power" mode values of "negative" s16 range may be valid positive values.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hector Martin

author

2009-07-01 20:19 UTC #3 lore raw

Quoted 4 lines · Alexey Starikovskiy

Andrew Morton пишет:
Quoted 3 lines
On Wed, 01 Jul 2009 20:20:41 +0200
acpi_battery has no field `current_now' in 2.6.30 or 2.6.31-rc1. Which kernel version are you patching here?

2.6.29. Guess I got unlucky.

Quoted 7 lines

Quoted 2 lines
Also, I wonder if we need a quirk. Is a "negative" value _ever_ correct? If not, could we do the negation unconditionally?
The problem is that the variable is s64 and not "negative" value is related to s16 portion of it.
It's possible to do this only for "current" mode, in "power" mode values of "negative" s16 range may be valid positive values.

Exactly. This is why I further qualified the quirk so it operates for Acer machines in current (mA) mode only. Currents >32767mA are insane, but powers >32767mW are not.

The value is nominally a signed 32-bit int as follows:

Quoted 1 line

=0: charge current

-1: unknown

(The spec defines this in unsigned terms but the result is the same).

The problem here is that my laptop erroneously reports charging currents as (65536 - current). Interpreting this as a signed 16-bit int yields the correct signed value, which can then be abs()ed.

The only addition that I'd imagine might make sense would be to check for current_now != -1 before applying the 16-bit trick, since 32-bit signed -1 is a valid value that means "unknown current". This does not conflict with 16-bit -1 because the 16-bits are not sign-extended so -1mA reports as 65535, not -1. Attached new patch with this change.

For reference, the DSDT does something like this:

s32 current_now;

current_now = (EC_BAT_CURRENT_HIGH<<8) | EC_BAT_CURRENT_LOW; current_now = (0xFFFF - current_now) + 1;

The "homebrew" two's complement negation attempts to make discharge currents positive but also makes charge currents negative. It's also completely wrong for 32-bit quantities, which is what we're dealing with here.

Signature · 2 lines

Hector Martin ([email protected])
Public Key: http://www.marcansoft.com/marcan.asc

Signed-off-by Hector Martin <[email protected]>

drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +18 -0

@@ -84,6 +84,10 @@
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* For buggy DSDTs that report negative 16-bit values for either charging
+ * or discharging and/or report 0 as 65536 due to bad math.
+ */
+#define QUIRK_SIGNED16_CURRENT 0x0001
 
 struct acpi_battery {
 	struct mutex lock;

@@ -111,6 +115,7 @@
 	int state;
 	int power_unit;
 	u8 alarm_present;
+	long quirks;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

@@ -387,6 +392,10 @@
 				 state_offsets, ARRAY_SIZE(state_offsets));
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
+
+	if ((battery->quirks & QUIRK_SIGNED16_CURRENT) && battery->current_now != -1)
+		battery->current_now = abs((s16)battery->current_now);
+
 	return result;
 }

@@ -492,6 +501,14 @@
 }
 #endif
 
+static void acpi_battery_quirks(struct acpi_battery *battery)
+{
+	battery->quirks = 0;
+	if (dmi_name_in_vendors("Acer") && battery->power_unit) {
+		battery->quirks |= QUIRK_SIGNED16_CURRENT;
+	}
+}
+
 static int acpi_battery_update(struct acpi_battery *battery)
 {
 	int result, old_present = acpi_battery_present(battery);

@@ -510,6 +527,7 @@
 		result = acpi_battery_get_info(battery);
 		if (result)
 			return result;
+		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
 #ifdef CONFIG_ACPI_SYSFS_POWER

Hector Martin

author

2009-07-02 03:51 UTC #4 lore raw

In reply to Andrew Morton

Quoted 2 lines · Andrew Morton

acpi_battery has no field `current_now' in 2.6.30 or 2.6.31-rc1. Which kernel version are you patching here?

Updated patch for 2.6.30 attached. current_now got renamed to rate_now.

Signature · 2 lines

Hector Martin ([email protected])
Public Key: http://www.marcansoft.com/marcan.asc

Signed-off-by Hector Martin <[email protected]">

drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +19 -0

@@ -85,6 +85,10 @@ static const struct acpi_device_id batte
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* For buggy DSDTs that report negative 16-bit values for either charging
+ * or discharging current and/or report 0 as 65536 due to bad math.
+ */
+#define QUIRK_SIGNED16_CURRENT 0x0001
 
 struct acpi_battery {
 	struct mutex lock;

@@ -112,6 +116,7 @@ struct acpi_battery {
 	int state;
 	int power_unit;
 	u8 alarm_present;
+	long quirks;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

@@ -390,6 +395,11 @@ static int acpi_battery_get_state(struct
 				 state_offsets, ARRAY_SIZE(state_offsets));
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
+
+	if ((battery->quirks & QUIRK_SIGNED16_CURRENT) &&
+	    battery->rate_now != -1)
+		battery->rate_now = abs((s16)battery->rate_now);
+
 	return result;
 }

@@ -495,6 +505,14 @@ static void sysfs_remove_battery(struct 
 }
 #endif
 
+static void acpi_battery_quirks(struct acpi_battery *battery)
+{
+	battery->quirks = 0;
+	if (dmi_name_in_vendors("Acer") && battery->power_unit) {
+		battery->quirks |= QUIRK_SIGNED16_CURRENT;
+	}
+}
+
 static int acpi_battery_update(struct acpi_battery *battery)
 {
 	int result, old_present = acpi_battery_present(battery);

@@ -513,6 +531,7 @@ static int acpi_battery_update(struct ac
 		result = acpi_battery_get_info(battery);
 		if (result)
 			return result;
+		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
 #ifdef CONFIG_ACPI_SYSFS_POWER

Andrew Morton

2009-07-02 22:57 UTC #5 lore raw

Quoted 5 lines · On Thu, 02 Jul 2009 05:51:56 +0200 Hector Martin <[email protected]> wrote:

Quoted 2 lines · Andrew Morton
acpi_battery has no field `current_now' in 2.6.30 or 2.6.31-rc1. Which kernel version are you patching here?
Updated patch for 2.6.30 attached. current_now got renamed to rate_now.

The patch has no changelog.

We could go fishing through the mailing list to find the original changelog, but perhaps it is no longer accurate. Or we could type in a new changelog but that also has accuracy problems.

So please always resend the full changelog (possibly after revising it) with each version of a patch.

Hector Martin

author

2009-07-02 23:46 UTC #6 lore raw

Re: [PATCH] Work around negative s16 battery current on Acer (take 3)

Quoted 2 lines · Andrew Morton

So please always resend the full changelog (possibly after revising it) with each version of a patch.

Trying again then. Hope I nailed the right Thunderbird options this time for optimal patch submission.

Acer Aspire 8930G laptops (and possibly others) report the battery current as a 16-bit signed negative when it is charging. It also reports it as 0x10000 when the current is 0. This patch adds a quirk for this which takes the absolute value of the reported current cast to an s16. This is a DSDT bug present in the latest BIOS revision (the EC register is 16 bits signed and the DSDT attempts to take the 16-bit two's complement of this, which works for discharge but not charge. It also breaks zero values because a 32-bit register is used and the high bits aren't thrown away).

I've enabled this for all Acer systems which report in mA units. This should be safe since it won't break compliant systems unless they report a current above 32A, which is insane. The patch also detects the valid 32-bit value -1, which indicates unknown status, and does not attempt the fix in that case (note that this does not conflict with 16-bit -1, which is 65535 as read normally and gets translated to 1mA).

Signed-off-by Hector Martin <[email protected]>

drivers/acpi/battery.c renamed from drivers/acpi/battery.c.old +19 -0

@@ -85,6 +85,10 @@ static const struct acpi_device_id batte
 
 MODULE_DEVICE_TABLE(acpi, battery_device_ids);
 
+/* For buggy DSDTs that report negative 16-bit values for either charging
+ * or discharging current and/or report 0 as 65536 due to bad math.
+ */
+#define QUIRK_SIGNED16_CURRENT 0x0001
 
 struct acpi_battery {
 	struct mutex lock;

@@ -112,6 +116,7 @@ struct acpi_battery {
 	int state;
 	int power_unit;
 	u8 alarm_present;
+	long quirks;
 };
 
 #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);

@@ -390,6 +395,11 @@ static int acpi_battery_get_state(struct
 				 state_offsets, ARRAY_SIZE(state_offsets));
 	battery->update_time = jiffies;
 	kfree(buffer.pointer);
+
+	if ((battery->quirks & QUIRK_SIGNED16_CURRENT) &&
+	    battery->rate_now != -1)
+		battery->rate_now = abs((s16)battery->rate_now);
+
 	return result;
 }

@@ -495,6 +505,14 @@ static void sysfs_remove_battery(struct 
 }
 #endif
 
+static void acpi_battery_quirks(struct acpi_battery *battery)
+{
+	battery->quirks = 0;
+	if (dmi_name_in_vendors("Acer") && battery->power_unit) {
+		battery->quirks |= QUIRK_SIGNED16_CURRENT;
+	}
+}
+
 static int acpi_battery_update(struct acpi_battery *battery)
 {
 	int result, old_present = acpi_battery_present(battery);

@@ -513,6 +531,7 @@ static int acpi_battery_update(struct ac
 		result = acpi_battery_get_info(battery);
 		if (result)
 			return result;
+		acpi_battery_quirks(battery);
 		acpi_battery_init_alarm(battery);
 	}
 #ifdef CONFIG_ACPI_SYSFS_POWER

Signature · 2 lines

Hector Martin ([email protected])
Public Key: http://www.marcansoft.com/marcan.asc

Alexey Starikovskiy

2009-07-01 18:47 UTC #7 lore raw

In reply to Hector Martin

Hector Martin пишет:

Quoted 10 lines

My Acer Aspire 8930G laptop reports the battery current as a 16-bit signed negative when it is charging. It also reports it as 0x10000 when the current is 0. This patch adds a quirk for this which takes the absolute value of the reported current cast to an s16. This is a DSDT bug present in the latest BIOS revision (the EC register is 16 bits signed and the DSDT attempts to take the 16-bit two's complement of this, which works for discharge but not charge. It also breaks zero values because a 32-bit register is used and the high bits aren't thrown away).

Quoted 5 lines

I've enabled this for all Acer systems which report in mA units. This should be safe since it won't break compliant systems unless they report a current above 32A, which is insane.

Could you please use bitfields? Please take a look at drivers/acpi/ec.c as an example.

Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html