diff mbox

Work around negative s16 battery current on Acer

Message ID 4A4BA8F9.9070803@marcansoft.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Hector Martin July 1, 2009, 6:20 p.m. UTC 
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.

Comments

Alexey Starikovskiy July 1, 2009, 6:27 p.m. UTC | #1 
Hector Martin пишет:
> 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.
> 
> 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Morton July 1, 2009, 6:29 p.m. UTC | #2 
On Wed, 01 Jul 2009 20:20:41 +0200
Hector Martin <hector@marcansoft.com> 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
> @@ -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);
> @@ -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?

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexey Starikovskiy July 1, 2009, 6:38 p.m. UTC | #3 
Andrew Morton пишет:
> On Wed, 01 Jul 2009 20:20:41 +0200
> Hector Martin <hector@marcansoft.com> 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
>> @@ -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);
>> @@ -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?
> 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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