diff mbox

[RFC,3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

Message ID 1236672209.2820.121.camel@rzhang-dt (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Zhang, Rui March 10, 2009, 8:03 a.m. UTC 
Subject: support _BCL packages that don't export brightness levels when machine is on AC/Battery
From: Zhang Rui <rui.zhang@intel.com>

Many buggy BIOSes don't export the brightness levels when machine
is on AC/Battery in the _BCL method.

Reformat the _BCL package for these laptops:
now the elements in device->brightness->levels[] are like:
levels[0]: brightness level when on AC power.
levels[1]: brightness level when on Battery power.
levels[2]: supported brightness level 1.
levels[3]: supported brightness level 2.
...
levels[n]: supported brightness level n-1.
levels[n + 1]: supported brightness level n.
So if there are n supported brightness levels on this laptop,
we will have n+2 entries in device->brightnes->levels[].

level[0] and level[1] are invalid on the laptops that don't
export the brightness levels on AC/Battery.
Fortunately, we never use these two values at all, even for the
valid ones.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |   37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)



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

Comments

Matthew Garrett March 10, 2009, 5:06 p.m. UTC | #1 
On Tue, Mar 10, 2009 at 04:03:29PM +0800, Zhang Rui wrote:

> +	if (level_ac_battery > 2) {
> +		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
> +		goto out_free_levels;

Exiting here seems a little excessive? I'd just go with a warning and 
carry on. In future maybe we could strip duplicates. Also, s/two/too/.

>  	/* don't sort the first two brightness levels */
>  	sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
>  		acpi_video_cmp_level, NULL);

I think the comment here needs to be clarified - it sounds like you'll 
ignore the first two in the package, even if they're actual values 
rather than the ac and battery ones.

Otherwise:

Acked-by: Matthew Garrett <mjg@redhat.com>
Thomas Renninger March 11, 2009, 1:57 p.m. UTC | #2 
On Tuesday 10 March 2009 09:03:29 Zhang Rui wrote:

> +	/*
> +	 * some buggy BIOS don't export the levels
> +	 * when machine is on AC/Battery in _BCL package.
> +	 * In this case, the first two elements in _BCL packages
> +	 * are also supported brightness levels that OS should take care of.
> +	 */
> +	for (i = 2; i < count; i++)
> +		if (br->levels[i] == br->levels[0] ||
> +		    br->levels[i] == br->levels[1])
> +			level_ac_battery++;
Hmm, I wonder whether this is what Len sees on one of his machine.
Do you remember when I added the patches to distinguish native vs
acpi brightness switching?
IIRC you missed some brightness levels with ACPI?
Wasn't this 6 vs 8, ACPI vs native?
Rui's patches should fix this then.
IIRC it was a panasonic.
Hmm both, panasonic and toshiba drivers seem to register for the
backlight interface unconditionally and miss the check whether the
ACPI video driver also might register for that one.
If above is true, I can add the check for them again.

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

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -168,10 +168,15 @@  struct acpi_video_device_cap {
 	u8 _DSS:1;		/*Device state set */
 };
 
+struct acpi_video_brightness_flags {
+	u8 _BCL_no_ac_battery_levels:1;	/* no AC/Battery levels in _BCL */
+};
+
 struct acpi_video_device_brightness {
 	int curr;
 	int count;
 	int *levels;
+	struct acpi_video_brightness_flags flags;
 };
 
 struct acpi_video_device {
@@ -682,7 +687,7 @@  static int
 acpi_video_init_brightness(struct acpi_video_device *device)
 {
 	union acpi_object *obj = NULL;
-	int i, max_level = 0, count = 0;
+	int i, max_level = 0, count = 0, level_ac_battery = 0;
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br = NULL;
 
@@ -701,7 +706,7 @@  acpi_video_init_brightness(struct acpi_v
 		goto out;
 	}
 
-	br->levels = kmalloc(obj->package.count * sizeof *(br->levels),
+	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
 				GFP_KERNEL);
 	if (!br->levels)
 		goto out_free;
@@ -719,16 +724,36 @@  acpi_video_init_brightness(struct acpi_v
 		count++;
 	}
 
+	/*
+	 * some buggy BIOS don't export the levels
+	 * when machine is on AC/Battery in _BCL package.
+	 * In this case, the first two elements in _BCL packages
+	 * are also supported brightness levels that OS should take care of.
+	 */
+	for (i = 2; i < count; i++)
+		if (br->levels[i] == br->levels[0] ||
+		    br->levels[i] == br->levels[1])
+			level_ac_battery++;
+
+	if (level_ac_battery > 2) {
+		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
+		goto out_free_levels;
+	} else if (level_ac_battery < 2) {
+		level_ac_battery = 2 - level_ac_battery;
+		br->flags._BCL_no_ac_battery_levels = 1;
+		for (i = (count - 1 + level_ac_battery); i >= 2; i--)
+			br->levels[i] = br->levels[i - level_ac_battery];
+		count += level_ac_battery;
+	}
+
 	/* don't sort the first two brightness levels */
 	sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
 		acpi_video_cmp_level, NULL);
 
-	if (count < 2)
-		goto out_free_levels;
-
 	br->count = count;
 	device->brightness = br;
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			  "found %d brightness levels\n", count - 2));
 	kfree(obj);
 	return max_level;