[RFC,4/5] ACPI video: support reversed _BCL method

Commit Message

Zhang, Rui March 10, 2009, 8:03 a.m. UTC

Subject: support reversed _BCL method in ACPI video driver
From: Zhang Rui <[e-mail]>

The brightness levels returned by _BCL package are in a reversed order
on some laptops.
http://bugzilla.kernel.org/show_bug.cgi?id=12037
http://bugzilla.kernel.org/show_bug.cgi?id=12302
http://bugzilla.kernel.org/show_bug.cgi?id=12235

sort the _BCL packge in case it's reversed.

Signed-off-by: Zhang Rui <[e-mail]>
---
 drivers/acpi/video.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)



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

Comments

Matthew Garrett March 10, 2009, 5:08 p.m. UTC | #1

On Tue, Mar 10, 2009 at 04:03:31PM +0800, Zhang Rui wrote:
> +	} else if (max_level != br->levels[count - 1]) {
> +		ACPI_ERROR((AE_INFO,
> +			    "Brightness levels in _BCL is in a mess\n"));
> +		goto out_free_levels;

Again, I don't think failing here is the best thing - I think bizarre 
behaviour is preferable to entirely unworking.

Matthew Garrett March 10, 2009, 5:11 p.m. UTC | #2

On Tue, Mar 10, 2009 at 04:03:31PM +0800, Zhang Rui wrote:
> sort the _BCL packge in case it's reversed.

What code actually uses this? A later patch seems to check it in the 
indexed case, but I can't find a reference to using it in the 
non-indexed case.

Zhang, Rui March 11, 2009, 1:42 a.m. UTC | #3

On Wed, 2009-03-11 at 01:11 +0800, Matthew Garrett wrote:
> On Tue, Mar 10, 2009 at 04:03:31PM +0800, Zhang Rui wrote:
> > sort the _BCL packge in case it's reversed.
> 
> What code actually uses this? A later patch seems to check it in the 
> indexed case, but I can't find a reference to using it in the 
> non-indexed case.
> 
It's just used in the indexed case.

In bug http://bugzilla.kernel.org/show_bug.cgi?id=12249

                 Name (PCTG, Package (0x10)
                {
                    0x64,
                    0x5A,
                    0x55,
                    0x50,
                    0x4B,
                    0x46,
                    0x41,
                    0x3C,
                    0x37,
                    0x32,
                    0x2D,
                    0x28,
                    0x23,
                    0x1E,
                    0x19,
                    0x14
                })

                    Method (_BCL, 0, NotSerialized)
                    {
                        Return (PCTG)
                    }
                    Method (_BQC, 0, NotSerialized)
                    {
                        Return (LBTN)
                    }

device->brightness->levels[] = {0x64, 0x5A, 0x14, 0x19,..., 0x5a, 0x64};

The index value returned by _BQC is the index in the PCTG package.
if the _BCL package is in a reversed order, we can not use this index to
get the value in device->brightness->levels[].
For example, if the current brightness is 0x64, _BQC returns 0 in this
case, but we can not translate this to value 0x14, we need a flag to
tell us that _BCL is reversed.

But in the non-indexed case, _BQC always returns a percentage that can
be used directly by ACPI video driver, i.e. we don't need to parse
device->brightness->levels to get the real value. so we don't need to
check the device->brightness->flags._BCL_reversed bit.

thanks,
rui

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

Matthew Garrett March 11, 2009, 1:45 a.m. UTC | #4

On Wed, Mar 11, 2009 at 09:42:55AM +0800, Zhang Rui wrote:

> The index value returned by _BQC is the index in the PCTG package.
> if the _BCL package is in a reversed order, we can not use this index to
> get the value in device->brightness->levels[].
> For example, if the current brightness is 0x64, _BQC returns 0 in this
> case, but we can not translate this to value 0x14, we need a flag to
> tell us that _BCL is reversed.
> 
> But in the non-indexed case, _BQC always returns a percentage that can
> be used directly by ACPI video driver, i.e. we don't need to parse
> device->brightness->levels to get the real value. so we don't need to
> check the device->brightness->flags._BCL_reversed bit.

Ah, of course. Sorry, I should have got that. Thanks for the 
explanation!

Acked-by: Matthew Garrett <[e-mail]>

Patch

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -170,6 +170,7 @@  struct acpi_video_device_cap {
 
 struct acpi_video_brightness_flags {
 	u8 _BCL_no_ac_battery_levels:1;	/* no AC/Battery levels in _BCL */
+	u8 _BCL_reversed:1;		/* _BCL package is in a reversed order*/
 };
 
 struct acpi_video_device_brightness {
@@ -746,9 +747,16 @@  acpi_video_init_brightness(struct acpi_v
 		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);
+	/* Check if the _BCL package is in a reversed order */
+	if (max_level == br->levels[2]) {
+		br->flags._BCL_reversed = 1;
+		sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
+			acpi_video_cmp_level, NULL);
+	} else if (max_level != br->levels[count - 1]) {
+		ACPI_ERROR((AE_INFO,
+			    "Brightness levels in _BCL is in a mess\n"));
+		goto out_free_levels;
+	}
 
 	br->count = count;
 	device->brightness = br;