From patchwork Wed Jul 1 20:19:16 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Hector Martin X-Patchwork-Id: 33563 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n61KJMsV012417 for ; Wed, 1 Jul 2009 20:19:22 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751700AbZGAUTR (ORCPT ); Wed, 1 Jul 2009 16:19:17 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751807AbZGAUTR (ORCPT ); Wed, 1 Jul 2009 16:19:17 -0400 Received: from marcansoft.com ([80.68.93.119]:53814 "EHLO smtp.marcansoft.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751700AbZGAUTQ (ORCPT ); Wed, 1 Jul 2009 16:19:16 -0400 Received: from [192.168.3.171] (85.Red-88-24-251.staticIP.rima-tde.net [88.24.251.85]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by smtp.marcansoft.com (Postfix) with ESMTPSA id 5A8961E8004; Wed, 1 Jul 2009 22:19:18 +0200 (CEST) Message-ID: <4A4BC4C4.1030407@marcansoft.com> Date: Wed, 01 Jul 2009 22:19:16 +0200 From: Hector Martin User-Agent: Thunderbird 2.0.0.21 (X11/20090530) MIME-Version: 1.0 To: Alexey Starikovskiy CC: Andrew Morton , lenb@kernel.org, linux-acpi@vger.kernel.org Subject: Re: [PATCH] Work around negative s16 battery current on Acer References: <4A4BA8F9.9070803@marcansoft.com> <20090701112958.1d24e06f.akpm@linux-foundation.org> <4A4BAD0C.9020302@suse.de> In-Reply-To: <4A4BAD0C.9020302@suse.de> Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org 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. --- 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