Re: [PATCH] Work around negative s16 battery current on Acer

[Hector Martin <hector@marcansoft.com>]

[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]

Alexey Starikovskiy wrote:
> Andrew Morton пишет:
>> 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.

>> 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:
>=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.


-- 
Hector Martin (hector@marcansoft.com)
Public Key: http://www.marcansoft.com/marcan.asc


[-- Attachment #2: acpi-battery-acer-rate-quirk.patch --]
[-- Type: text/plain, Size: 1514 bytes --]

Signed-off-by: Hector Martin <hector@marcansoft.com>
--- linux/drivers/acpi/battery.c.old	2009-07-01 19:17:33.000000000 +0200
+++ linux/drivers/acpi/battery.c	2009-07-01 19:52:43.000000000 +0200
@@ -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