[GIT PATCH v2] thinkpad-acpi changes for the merge window (part 2)

[GIT PATCH v2] thinkpad-acpi changes for the merge window (part 2)

[Henrique de Moraes Holschuh]

(resending, I missed one patch in the series. Sorry about that)

Len,                                                                                                                                                  

Here is the second batch of thinkpad-acpi changes for the merge window (target:
2.6.24).  There will probably be a third batch later, I am waiting some reports
from testers.

This batch improves brightness support on Lenovo ThinkPads, and also disables
said support by default in any notebook that seems to do well without it :-)

Please merge this batch to acpi-test.

As usual, the patch set is available at:
git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git for-upstream/acpi-test

Shortlog of the above branch:

Henrique de Moraes Holschuh (13):
  (already submitted)
      ACPI: thinkpad-acpi: make room for more features in tp_features bitfield
      ACPI: thinkpad-acpi: issue EV_SYNC after EV_SWITCH
      ACPI: thinkpad-acpi: add mutex-based locking to input device event send path
      ACPI: thinkpad-acpi: keep track of module state
      ACPI: thinkpad-acpi: check version of hot key firmware
      ACPI: thinkpad-acpi: dequeue all pending hot key events at once (v2.2)
      ACPI: thinkpad-acpi: fix regression on HKEY LID event handling
      ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it (v2)
      ACPI: thinkpad-acpi: duplicate driver attributes to new hwmon pdrv
  (new)
      ACPI: thinkpad-acpi: skip blanks before the data when parsing sysfs
      ACPI: thinkpad-acpi: support 16 levels of brightness (v2)
      ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it
      ACPI: thinkpad-acpi: bump up version to 0.17

Thanks.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

[PATCH 1/4] ACPI: thinkpad-acpi: skip blanks before the data when parsing sysfs

[Henrique de Moraes Holschuh]

Skip blanks not just at the tail of sysfs writes, but also at the head.

Signed-off-by: Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
---
 drivers/misc/thinkpad_acpi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 81693b4..37891a8 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -709,6 +709,8 @@ static int parse_strtoul(const char *buf,
 {
 	char *endp;
 
+	while (*buf && isspace(*buf))
+		buf++;
 	*value = simple_strtoul(buf, &endp, 0);
 	while (*endp && isspace(*endp))
 		endp++;
-- 
1.5.3.2


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[PATCH 2/4] ACPI: thinkpad-acpi: support 16 levels of brightness (v2)

[Henrique de Moraes Holschuh]

Lenovo ThinkPads with ACPI Video brightness control often have 16
brightness levels in EC, and not just eight levels like older ThinkPads.

We detect the number of brightness levels by the presence of a _BCL method
with 18 entries (16 levels plus the on-battery and on-ac recommended
levels).  If _BCL is not there, we assume eight levels (T60, for example).
Otherwise we assume sixteen levels (T61, X61, etc).

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 Documentation/thinkpad-acpi.txt |   61 +++++++++++++++-----------
 drivers/misc/thinkpad_acpi.c    |   93 ++++++++++++++++++++++++++++++++++----
 drivers/misc/thinkpad_acpi.h    |    3 +-
 3 files changed, 120 insertions(+), 37 deletions(-)

diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
index 3b95bba..4db98b3 100644
--- a/Documentation/thinkpad-acpi.txt
+++ b/Documentation/thinkpad-acpi.txt
@@ -923,19 +923,26 @@ sysfs backlight device "thinkpad_screen"
 This feature allows software control of the LCD brightness on ThinkPad
 models which don't have a hardware brightness slider.
 
-It has some limitations: the LCD backlight cannot be actually turned on or off
-by this interface, and in many ThinkPad models, the "dim while on battery"
-functionality will be enabled by the BIOS when this interface is used, and
-cannot be controlled.
-
-The backlight control has eight levels, ranging from 0 to 7.  Some of the
-levels may not be distinct.
-
-There are two interfaces to the firmware for brightness control, EC and CMOS.
-To select which one should be used, use the brightness_mode module parameter:
-brightness_mode=1 selects EC mode, brightness_mode=2 selects CMOS mode,
-brightness_mode=3 selects both EC and CMOS.  The driver tries to autodetect
-which interface to use.
+It has some limitations: the LCD backlight cannot be actually turned on or
+off by this interface, and in many ThinkPad models, the "dim while on
+battery" functionality will be enabled by the BIOS when this interface is
+used, and cannot be controlled.
+
+On IBM (and some of the earlier Lenovo) ThinkPads, the backlight control
+has eight brightness levels, ranging from 0 to 7.  Some of the levels
+may not be distinct.  Later Lenovo models that implement the ACPI
+display backlight brightness control methods have 16 levels, ranging
+from 0 to 15.
+
+There are two interfaces to the firmware for direct brightness control,
+EC and CMOS.  To select which one should be used, use the
+brightness_mode module parameter: brightness_mode=1 selects EC mode,
+brightness_mode=2 selects CMOS mode, brightness_mode=3 selects both EC
+and CMOS.  The driver tries to autodetect which interface to use.
+
+When display backlight brightness controls are available through the
+standard ACPI interface, it is best to use it instead of this direct
+ThinkPad-specific interface.
 
 Procfs notes:
 
@@ -947,11 +954,11 @@ Procfs notes:
 
 Sysfs notes:
 
-The interface is implemented through the backlight sysfs class, which is poorly
-documented at this time.
+The interface is implemented through the backlight sysfs class, which is
+poorly documented at this time.
 
-Locate the thinkpad_screen device under /sys/class/backlight, and inside it
-there will be the following attributes:
+Locate the thinkpad_screen device under /sys/class/backlight, and inside
+it there will be the following attributes:
 
 	max_brightness:
 		Reads the maximum brightness the hardware can be set to.
@@ -961,17 +968,19 @@ there will be the following attributes:
 		Reads what brightness the screen is set to at this instant.
 
 	brightness:
-		Writes request the driver to change brightness to the given
-		value.  Reads will tell you what brightness the driver is trying
-		to set the display to when "power" is set to zero and the display
-		has not been dimmed by a kernel power management event.
+		Writes request the driver to change brightness to the
+		given value.  Reads will tell you what brightness the
+		driver is trying to set the display to when "power" is set
+		to zero and the display has not been dimmed by a kernel
+		power management event.
 
 	power:
-		power management mode, where 0 is "display on", and 1 to 3 will
-		dim the display backlight to brightness level 0 because
-		thinkpad-acpi cannot really turn the backlight off.  Kernel
-		power management events can temporarily increase the current
-		power management level, i.e. they can dim the display.
+		power management mode, where 0 is "display on", and 1 to 3
+		will dim the display backlight to brightness level 0
+		because thinkpad-acpi cannot really turn the backlight
+		off.  Kernel power management events can temporarily
+		increase the current power management level, i.e. they can
+		dim the display.
 
 
 Volume control -- /proc/acpi/ibm/volume
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 37891a8..2a573db 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -3114,6 +3114,68 @@ static struct backlight_ops ibm_backlight_data = {
 
 static struct mutex brightness_mutex;
 
+static int __init tpacpi_query_bcl_levels(acpi_handle handle)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	int rc;
+
+	if (ACPI_SUCCESS(acpi_evaluate_object(handle, NULL, NULL, &buffer))) {
+		obj = (union acpi_object *)buffer.pointer;
+		if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+			printk(IBM_ERR "Invalid _BCL data\n");
+			rc = 0;
+		} else {
+			if (obj->package.count >= 2)
+				rc = obj->package.count - 2;
+			else
+				rc = 0;
+		}
+	} else {
+		return 0;
+	}
+
+	kfree(buffer.pointer);
+	return rc;
+}
+
+static acpi_status __init brightness_find_bcl(acpi_handle handle, u32 lvl,
+					void *context, void **rv)
+{
+	char name[ACPI_PATH_SEGMENT_LENGTH];
+	struct acpi_buffer buffer = { sizeof(name), &name };
+
+	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer)) &&
+	    !strncmp("_BCL", name, sizeof(name) - 1)) {
+		if (tpacpi_query_bcl_levels(handle) == 16) {
+			*rv = handle;
+			return AE_CTRL_TERMINATE;
+		} else {
+			return AE_OK;
+		}
+	} else {
+		return AE_OK;
+	}
+}
+
+static int __init brightness_check_levels(void)
+{
+	int status;
+	void *found_node = NULL;
+
+	if (!vid_handle) {
+		IBM_ACPIHANDLE_INIT(vid);
+	}
+	if (!vid_handle)
+		return 0;
+
+	/* Search for a _BCL method with 16 levels */
+	status = acpi_walk_namespace(ACPI_TYPE_METHOD, vid_handle, 3,
+					brightness_find_bcl, NULL, &found_node);
+
+	return (ACPI_SUCCESS(status) && found_node != NULL);
+}
+
 static int __init brightness_init(struct ibm_init_struct *iibm)
 {
 	int b;
@@ -3135,10 +3197,17 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 	if (brightness_mode > 3)
 		return -EINVAL;
 
+	tp_features.bright_16levels =
+			thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO &&
+			brightness_check_levels();
+
 	b = brightness_get(NULL);
 	if (b < 0)
 		return 1;
 
+	if (tp_features.bright_16levels)
+		printk(IBM_INFO "detected a 16-level brightness capable ThinkPad\n");
+
 	ibm_backlight_device = backlight_device_register(
 					TPACPI_BACKLIGHT_DEV_NAME, NULL, NULL,
 					&ibm_backlight_data);
@@ -3148,7 +3217,8 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 	}
 	vdbg_printk(TPACPI_DBG_INIT, "brightness is supported\n");
 
-	ibm_backlight_device->props.max_brightness = 7;
+	ibm_backlight_device->props.max_brightness =
+				(tp_features.bright_16levels)? 15 : 7;
 	ibm_backlight_device->props.brightness = b;
 	backlight_update_status(ibm_backlight_device);
 
@@ -3184,13 +3254,14 @@ static int brightness_get(struct backlight_device *bd)
 	if (brightness_mode & 1) {
 		if (!acpi_ec_read(brightness_offset, &lec))
 			return -EIO;
-		lec &= 7;
+		lec &= (tp_features.bright_16levels)? 0x0f : 0x07;
 		level = lec;
 	};
 	if (brightness_mode & 2) {
 		lcmos = (nvram_read_byte(TP_NVRAM_ADDR_BRIGHTNESS)
 			 & TP_NVRAM_MASK_LEVEL_BRIGHTNESS)
 			>> TP_NVRAM_POS_LEVEL_BRIGHTNESS;
+		lcmos &= (tp_features.bright_16levels)? 0x0f : 0x07;
 		level = lcmos;
 	}
 
@@ -3211,7 +3282,7 @@ static int brightness_set(int value)
 	int cmos_cmd, inc, i, res;
 	int current_value;
 
-	if (value > 7)
+	if (value > ((tp_features.bright_16levels)? 15 : 7))
 		return -EINVAL;
 
 	res = mutex_lock_interruptible(&brightness_mutex);
@@ -3227,7 +3298,7 @@ static int brightness_set(int value)
 	cmos_cmd = value > current_value ?
 			TP_CMOS_BRIGHTNESS_UP :
 			TP_CMOS_BRIGHTNESS_DOWN;
-	inc = value > current_value ? 1 : -1;
+	inc = (value > current_value)? 1 : -1;
 
 	res = 0;
 	for (i = current_value; i != value; i += inc) {
@@ -3256,10 +3327,11 @@ static int brightness_read(char *p)
 	if ((level = brightness_get(NULL)) < 0) {
 		len += sprintf(p + len, "level:\t\tunreadable\n");
 	} else {
-		len += sprintf(p + len, "level:\t\t%d\n", level & 0x7);
+		len += sprintf(p + len, "level:\t\t%d\n", level);
 		len += sprintf(p + len, "commands:\tup, down\n");
 		len += sprintf(p + len, "commands:\tlevel <level>"
-			       " (<level> is 0-7)\n");
+			       " (<level> is 0-%d)\n",
+			       (tp_features.bright_16levels) ? 15 : 7);
 	}
 
 	return len;
@@ -3270,18 +3342,19 @@ static int brightness_write(char *buf)
 	int level;
 	int new_level;
 	char *cmd;
+	int max_level = (tp_features.bright_16levels) ? 15 : 7;
 
 	while ((cmd = next_cmd(&buf))) {
 		if ((level = brightness_get(NULL)) < 0)
 			return level;
-		level &= 7;
 
 		if (strlencmp(cmd, "up") == 0) {
-			new_level = level == 7 ? 7 : level + 1;
+			new_level = level == (max_level)?
+						max_level : level + 1;
 		} else if (strlencmp(cmd, "down") == 0) {
-			new_level = level == 0 ? 0 : level - 1;
+			new_level = level == 0? 0 : level - 1;
 		} else if (sscanf(cmd, "level %d", &new_level) == 1 &&
-			   new_level >= 0 && new_level <= 7) {
+			   new_level >= 0 && new_level <= max_level) {
 			/* new_level set */
 		} else
 			return -EINVAL;
diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
index c5fdd68..de0f343 100644
--- a/drivers/misc/thinkpad_acpi.h
+++ b/drivers/misc/thinkpad_acpi.h
@@ -84,7 +84,7 @@
 
 /* ThinkPad CMOS NVRAM constants */
 #define TP_NVRAM_ADDR_BRIGHTNESS       0x5e
-#define TP_NVRAM_MASK_LEVEL_BRIGHTNESS 0x07
+#define TP_NVRAM_MASK_LEVEL_BRIGHTNESS 0x0f
 #define TP_NVRAM_POS_LEVEL_BRIGHTNESS 0
 
 #define onoff(status,bit) ((status) & (1 << (bit)) ? "on" : "off")
@@ -246,6 +246,7 @@ static struct {
 	u32 hotkey_wlsw:1;
 	u32 light:1;
 	u32 light_status:1;
+	u32 bright_16levels:1;
 	u32 wan:1;
 	u32 fan_ctrl_status_undef:1;
 	u32 input_device_registered:1;
-- 
1.5.3.2

[PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Henrique de Moraes Holschuh]

If we detect the presence of _BCL on a Lenovo ThinkPad, it means there is
support for ACPI standard backlight brightness control.  In that case, it
is probably best to not make the ThinkPad-specific backlight interface
available.

Provide a Kconfig option (default disabled) to make the ThinkPad-specific
backlight always available, even if there is generic ACPI backlight
support in the DSDT.

Signed-off-by: Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
---
 Documentation/thinkpad-acpi.txt |    5 ++++-
 drivers/misc/Kconfig            |   17 +++++++++++++++++
 drivers/misc/thinkpad_acpi.c    |   20 ++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
index 4db98b3..7ff0183 100644
--- a/Documentation/thinkpad-acpi.txt
+++ b/Documentation/thinkpad-acpi.txt
@@ -942,7 +942,10 @@ and CMOS.  The driver tries to autodetect which interface to use.
 
 When display backlight brightness controls are available through the
 standard ACPI interface, it is best to use it instead of this direct
-ThinkPad-specific interface.
+ThinkPad-specific interface.  In that case, the driver will not export
+the ThinkPad-specific interface by default, although this can be changed
+at compile time, using the CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
+Kconfig option.
 
 Procfs notes:
 
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 73e248f..bd8504a 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -177,6 +177,23 @@ config THINKPAD_ACPI_DEBUG
 
 	  If you are not sure, say N here.
 
+config THINKPAD_ACPI_BACKLIGHT_DESIRED
+	bool "Always load backlight support"
+	depends on THINKPAD_ACPI
+	default n
+	---help---
+	  By default, if the ThinkPad supports the standard ACPI display
+	  brightness control, the thinkpad-specific backlight support will
+	  not be loaded.  This avoids duplicate backlight interfaces
+	  controlling the same LCD, as the ACPI video driver will also publish
+	  a backlight interface to control the display brightness.
+
+	  However, if you prefer to always have the thinkpad-specific backlight
+	  device available, even when it will just be a duplicate of the
+	  standard ACPI backlight device, say Y here.
+
+	  If you are not sure, say N here.
+
 config THINKPAD_ACPI_DOCK
 	bool "Legacy Docking Station Support"
 	depends on THINKPAD_ACPI
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 2a573db..29f521b 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -3114,6 +3114,10 @@ static struct backlight_ops ibm_backlight_data = {
 
 static struct mutex brightness_mutex;
 
+#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
+static int brightness_acpi_detected __initdata;
+#endif
+
 static int __init tpacpi_query_bcl_levels(acpi_handle handle)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -3147,6 +3151,9 @@ static acpi_status __init brightness_find_bcl(acpi_handle handle, u32 lvl,
 
 	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer)) &&
 	    !strncmp("_BCL", name, sizeof(name) - 1)) {
+#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
+		brightness_acpi_detected = 1;
+#endif
 		if (tpacpi_query_bcl_levels(handle) == 16) {
 			*rv = handle;
 			return AE_CTRL_TERMINATE;
@@ -3197,10 +3204,23 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 	if (brightness_mode > 3)
 		return -EINVAL;
 
+#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
+	brightness_acpi_detected = 0;
+#endif
+
 	tp_features.bright_16levels =
 			thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO &&
 			brightness_check_levels();
 
+#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
+	if (brightness_acpi_detected) {
+		dbg_printk(TPACPI_DBG_INIT,
+			   "ACPI standard backlight brightness control "
+			   "available, aborting...\n");
+		return 1;
+	}
+#endif
+
 	b = brightness_get(NULL);
 	if (b < 0)
 		return 1;
-- 
1.5.3.2


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[PATCH 4/4] ACPI: thinkpad-acpi: bump up version to 0.17

[Henrique de Moraes Holschuh]

The lm-sensors 3.0.0/libsensors4 compatibility changes are reason enough to
bump up the version string.  Do it.

Signed-off-by: Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
---
 Documentation/thinkpad-acpi.txt |    4 ++--
 drivers/misc/thinkpad_acpi.c    |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
index 7ff0183..812ae95 100644
--- a/Documentation/thinkpad-acpi.txt
+++ b/Documentation/thinkpad-acpi.txt
@@ -1,7 +1,7 @@
 		     ThinkPad ACPI Extras Driver
 
-                            Version 0.16
-                          August 2nd, 2007
+                            Version 0.17
+                         October 04th, 2007
 
                Borislav Deianov <borislav-iA+eEnwkJgzk1uMJSBkQmQ@public.gmane.org>
              Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 29f521b..352d235 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -21,7 +21,7 @@
  *  02110-1301, USA.
  */
 
-#define IBM_VERSION "0.16"
+#define IBM_VERSION "0.17"
 #define TPACPI_SYSFS_VERSION 0x020000
 
 /*
-- 
1.5.3.2


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [ibm-acpi-devel] [PATCH 2/4] ACPI: thinkpad-acpi: support 16 levels of brightness (v2)

[Thomas Renninger]

On Mon, 2007-10-08 at 10:12 -0300, Henrique de Moraes Holschuh wrote:
> Lenovo ThinkPads with ACPI Video brightness control often have 16
> brightness levels in EC, and not just eight levels like older ThinkPads.
> 
> We detect the number of brightness levels by the presence of a _BCL method
> with 18 entries (16 levels plus the on-battery and on-ac recommended
> levels).  If _BCL is not there, we assume eight levels (T60, for example).
> Otherwise we assume sixteen levels (T61, X61, etc).
This is broken by design, pls don't add this to a mainline kernel.

Correct is: If there is _BCL (and other video functions) don't touch any
video or brightness functionality.

Thinkpad_acpi should try to register for the acpi device with the
LNXVIDEO HID. If the probe/add function there is invoked, it can be sure
that video module tries to load and is handling the stuff in a defined
way and thinkpad_acpi should get its hands off.
AFAIK this would be the first case where two drivers register for the
same device, not sure whether it works out of the box. If the passed
device's driver_data or anything else is not touched, it should.

Here some more arguments from a Lenovo engineer (he doesn't want to have
his mail public, I ask whether I can pass his email to you privately
Henrique...):

-----------------------------------------------------------
ThinkPad has implemented _BCL/_BCM/_BQC method for Vista. 
These brightness control methods are not used on Windows XP. 

-------------
with Brightness Control method
- Press Fn+Home/End 
- ASL: Notify (video, 0x86/0x87) ( Increase/Decrease Brightness)
- OS: Call _BCM    <------ ??? Does this work?
- ASL: Indicate brightness change in config space of IGD. 
- Intel 965GM driver : Change the brightness setting 

--------------
without brightness control method
- Press Fn+Home/End 
- ASL: Indicate brightness change in config space of IGD. 
- Intel 965GM driver : Change the brightness setting    <--- I had
assumed the driver did nothing here. 

Now, I don't understand which point the brightness control is broken. 
Does Suse call _BCM , but not handle Notify (video, 0x86/0x87)
(Increase/Decrease Brightness) ?

> On which systems or more specific from what bios version of these
> models on should these acpi functions be used?

The BIOSs/Machine Type/Model/Support are too complicated. 
Why don't you use assume BIOS supports _BCL/_BCM/_BQC if these methods
are found in ACPI name space ?

T61/T61p/R61  -  Brightness control method : Any version 

                        BIOS                      EC
  1.22-1.06         1.22  (7LET52WW)          1.06  (7KHT22WW)
  1.21-1.06         1.21  (7LET51WW)          1.06  (7KHT22WW)
  1.14-1.06         1.14  (7LET44WW)          1.06  (7KHT22WW)
  1.10-1.06         1.10  (7LET40WW)          1.06  (7KHT22WW)
  1.09-1.05         1.09  (7LET39WW)          1.05  (7KHT21WW)
  1.07-1.04         1.07  (7LET37WW)          1.04  (7KHT20WW)
  1.07              1.07  (7LET37WW)          1.03  (7KHT19WW) 

R61 14.1 Wide, 15.4 Wide Brightness control method : Any version 
  1.22-1.06         1.22  (7KET72WW)          1.06  (7KHT22WW)
  1.21-1.06         1.21  (7KET71WW)          1.06  (7KHT22WW)
  1.14-1.06         1.14  (7KET64WW)          1.06  (7KHT22WW)
  1.10-1.06         1.10  (7KET60WW)          1.06  (7KHT22WW)
  1.09-1.05         1.09  (7KET59WW)          1.05  (7KHT21WW)
  1.07-1.04         1.07  (7KET57WW)          1.04  (7KHT20WW)
  1.07              1.07  (7KET57WW)          1.03  (7KHT19WW) 


R61 15.1 Normal screen Brightness control method : Any version 
  1.07              1.07  (7QET25WW)          1.00  (7QHT15WW)
  1.04              1.04  (7QET22WW)          1.00  (7QHT15WW)
T60/T60p -  Brightness control method : 79ETC1WW or later
                   BIOS            EC 
  2.17         2.17   (79ETD7WW)    1.07   (79HT50WW)
  2.16         2.16   (79ETD6WW)    1.07   (79HT50WW)
  2.15         2.15   (79ETD5WW)    1.07   (79HT50WW)
  2.14         2.14   (79ETD4WW)    1.07   (79HT50WW)
  2.13         2.13   (79ETD3WW)    1.07   (79HT50WW)
  2.12         2.12   (79ETD2WW)    1.07   (79HT50WW)
  2.11         2.11   (79ETD1WW)    1.07   (79HT50WW)
  2.09         2.09   (79ETC9WW)    1.07   (79HT50WW)    <--------
(New) 
Vista support
  2.07         2.07   (79ETC7WW)    1.07   (79HT50WW)
  2.06         2.06   (79ETC6WW)    1.07   (79HT50WW)
  2.05         2.05   (79ETC5WW)    1.07   (79HT50WW)
  2.04         2.04   (79ETC4WW)    1.07   (79HT50WW)
  2.03         2.03   (79ETC3WW)    1.07   (79HT50WW)
  2.01         2.01   (79ETC1WW)    1.07   (79HT50WW)    <--------- 
Brightness control method

--------------------
  1.11         1.11   (79ET67WW)    1.05b  (79HT48WW)
  1.09         1.09a  (79ET65WW)    1.05b  (79HT48WW)
  1.07         1.07   (79ET62WW)    1.04   (79HT45WW)
  1.06         1.06   (79ET61WW)    1.04   (79HT45WW)
  1.05         1.05a  (79ET60WW)    1.02   (79HT43WW)
  1.04         1.04   (79ET58WW)    1.02   (79HT43WW)
  1.03         1.03   (79ET57WW)    1.02   (79HT43WW)
  1.02         1.02   (79ET56WW)    1.01   (79HT42WW)
  1.01         1.01a  (79ET55WW)    1.01   (79HT42WW)
  1.00         1.00h  (79ET52WW)    1.00c  (79HT39WW)

T60/60p Wide screen model - Brightness control method : Any version

  1.11              1.11  (7IET30WW)          1.07  (79HT50WW)
  1.10              1.10  (7IET29WW)          1.07  (79HT50WW)
  1.09              1.09  (7IET28WW)          1.07  (79HT50WW)
  1.08              1.08  (7IET27WW)          1.07  (79HT50WW)
  1.07              1.07  (7IET26WW)          1.07  (79HT50WW)
  1.06              1.06  (7IET25WW)          1.07  (79HT50WW)
  1.04              1.04  (7IET23WW)          1.07  (79HT50WW)
  na                  1.03  (7IET22WW)          1.07  (79HT50WW)
  1.01              1.01  (7IET20WW)          1.07  (79HT50WW)
  1.00              1.00  (7IET19WW)          1.07  (79HT50WW) 


X61/X61s -  Brightness control method : Any version
  1.06              1.06  (7NET25WW)          1.02  (7MHT24WW)
  1.05              1.05  (7NET24WW)          1.02  (7MHT24WW)
  1.03              1.03  (7NET22WW)          1.02  (7MHT24WW)
  1.02              1.02  (7NET21WW)          1.01  (7MHT23WW)


X61 Tablet  Brightness control method : Any version
  1.04              1.04  (7SET18WW)          1.02  (7RHT16WW)
  1.03              1.03  (7SET17WW)          1.01  (7RHT15WW) 

X60/X60s -  Brightness control method : 7BETB9WW or later
  2.13         2.13  (7BETD2WW)     1.13  (7BHT40WW)
  2.12         2.12  (7BETD1WW)     1.13  (7BHT40WW)
  2.11         2.11  (7BETD0WW)     1.13  (7BHT40WW)
  2.10         2.10  (7BETC9WW)     1.13  (7BHT40WW)
  2.09         2.09  (7BETC8WW)     1.10  (7BHT37WW)
  2.07         2.07  (7BETC6WW)     1.10  (7BHT37WW)
  2.06         2.06  (7BETC5WW)     1.10  (7BHT37WW)   <-- (New) Vista 
support
  2.05         2.05  (7BETC4WW)     1.10  (7BHT37WW)
  2.04         2.04  (7BETC3WW)     1.09  (7BHT36WW)
  2.03         2.03  (7BETC2WW)     1.09  (7BHT36WW)
  2.02         2.02  (7BETC1WW)     1.09  (7BHT36WW)
  2.00f        2.00f (7BETB9WW)     1.08  (7BHT35WW)  <--Brightness 
control method

--------------------------------
  1.10         1.10  (7BET50WW)     1.08  (7BHT35WW)
  1.09         1.09  (7BET49WW)     1.07  (7BHT34WW)
  1.06         1.06  (7BET46WW)     1.04  (7BHT31WW)
  1.03         1.05  (7BET45WW)     1.04  (7BHT31WW)
  1.01         1.04  (7BET44WW)     1.02  (7BHT29WW)
  1.00         1.03  (7BET43WW)     1.01  (7BHT28WW)


X60 Tablet -  Brightness control method : Any version
  1.03         1.03  (7JET18WW)     1.03  (7JHT12WW)
  1.04         1.04  (7JET19WW)     1.03  (7JHT12WW)
  1.05         1.05  (7JET20WW)     1.03  (7JHT12WW)   <--(New) Vista 
support
  1.07         1.07  (7JET22WW)     1.03  (7JHT12WW)
  1.08         1.08  (7JET23WW)     1.03  (7JHT12WW)
  1.09         1.09  (7JET24WW)     1.04  (7JHT13WW)
  1.10         1.10  (7JET25WW)     1.04  (7JHT13WW)


R61e  -  Brightness control method : Any version
1.03-1.06         1.03  (7PET16WW)          1.06  (7KHT22WW)

R60 (MT 9455/9457/9459)  -  Brightness control method : 7CETB6WW or
later
  2.19         2.19  (7CETC9WW)     1.10  (7CHT22WW)
  2.18         2.18  (7CETC8WW)     1.10  (7CHT22WW)
  2.17         2.17  (7CETC7WW)     1.09  (7CHT21WW)
  2.16         2.16  (7CETC6WW)     1.09  (7CHT21WW)
  2.14         2.14  (7CETC4WW)     1.09  (7CHT21WW)
  2.11         2.11  (7CETC1WW)     1.09  (7CHT21WW)  <--(New) Vista 
support 
  2.10         2.10  (7CETC0WW)     1.09  (7CHT21WW)
  2.08         2.08  (7CETB8WW)     1.09  (7CHT21WW)
  2.07         2.07  (7CETB7WW)     1.09  (7CHT21WW)
  2.06         2.06  (7CETB6WW)     1.09  (7CHT21WW)  <--------- 
Brightness control method

--------------------------------
  1.05         1.05  (7CET50WW)     1.07  (7CHT19WW)
  1.04-01      1.04  (7CET49WW)     1.04  (7CHT16WW)
  1.04         1.04  (7CET49WW)     1.02  (7CHT14WW)
  1.00         1.02  (7CET47WW)     1.02  (7CHT14WW)

R60e  -  Brightness control method :  7EETB6WW or later
  2.17         2.17  (7EETC7WW)     1.07  (7EHT15WW)
  2.16         2.16  (7EETC6WW)     1.07  (7EHT15WW)
  2.15         2.15  (7EETC5WW)     1.07  (7EHT15WW)
  2.14         2.14  (7EETC4WW)     1.07  (7EHT15WW)
  2.12         2.09  (7EETC2WW)     1.07  (7EHT15WW)
  2.09         2.09  (7EETB9WW)     1.07  (7EHT15WW)  <--(New) Vista 
support 
  2.07         2.07  (7EETB7WW)     1.07  (7EHT15WW)
  2.06         2.06  (7EETB6WW)     1.07  (7EHT15WW)  <--------- 
Brightness control method

--------------------------------
  1.04         1.04  (7EET18WW)     1.05  (7EHT13WW) 
  1.03         1.03  (7EET17WW)     1.02  (7EHT10WW)
  1.00         1.02  (7EET16WW)     1.02  (7EHT10WW)

# Z60m/Z60t/Z61m/Z61e/Z61t/Z61p don't support Brightness Control method.
-----------------------------------------------------------


    Thomas

> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  Documentation/thinkpad-acpi.txt |   61 +++++++++++++++-----------
>  drivers/misc/thinkpad_acpi.c    |   93 ++++++++++++++++++++++++++++++++++----
>  drivers/misc/thinkpad_acpi.h    |    3 +-
>  3 files changed, 120 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
> index 3b95bba..4db98b3 100644
> --- a/Documentation/thinkpad-acpi.txt
> +++ b/Documentation/thinkpad-acpi.txt
> @@ -923,19 +923,26 @@ sysfs backlight device "thinkpad_screen"
>  This feature allows software control of the LCD brightness on ThinkPad
>  models which don't have a hardware brightness slider.
>  
> -It has some limitations: the LCD backlight cannot be actually turned on or off
> -by this interface, and in many ThinkPad models, the "dim while on battery"
> -functionality will be enabled by the BIOS when this interface is used, and
> -cannot be controlled.
> -
> -The backlight control has eight levels, ranging from 0 to 7.  Some of the
> -levels may not be distinct.
> -
> -There are two interfaces to the firmware for brightness control, EC and CMOS.
> -To select which one should be used, use the brightness_mode module parameter:
> -brightness_mode=1 selects EC mode, brightness_mode=2 selects CMOS mode,
> -brightness_mode=3 selects both EC and CMOS.  The driver tries to autodetect
> -which interface to use.
> +It has some limitations: the LCD backlight cannot be actually turned on or
> +off by this interface, and in many ThinkPad models, the "dim while on
> +battery" functionality will be enabled by the BIOS when this interface is
> +used, and cannot be controlled.
> +
> +On IBM (and some of the earlier Lenovo) ThinkPads, the backlight control
> +has eight brightness levels, ranging from 0 to 7.  Some of the levels
> +may not be distinct.  Later Lenovo models that implement the ACPI
> +display backlight brightness control methods have 16 levels, ranging
> +from 0 to 15.
> +
> +There are two interfaces to the firmware for direct brightness control,
> +EC and CMOS.  To select which one should be used, use the
> +brightness_mode module parameter: brightness_mode=1 selects EC mode,
> +brightness_mode=2 selects CMOS mode, brightness_mode=3 selects both EC
> +and CMOS.  The driver tries to autodetect which interface to use.
> +
> +When display backlight brightness controls are available through the
> +standard ACPI interface, it is best to use it instead of this direct
> +ThinkPad-specific interface.
>  
>  Procfs notes:
>  
> @@ -947,11 +954,11 @@ Procfs notes:
>  
>  Sysfs notes:
>  
> -The interface is implemented through the backlight sysfs class, which is poorly
> -documented at this time.
> +The interface is implemented through the backlight sysfs class, which is
> +poorly documented at this time.
>  
> -Locate the thinkpad_screen device under /sys/class/backlight, and inside it
> -there will be the following attributes:
> +Locate the thinkpad_screen device under /sys/class/backlight, and inside
> +it there will be the following attributes:
>  
>  	max_brightness:
>  		Reads the maximum brightness the hardware can be set to.
> @@ -961,17 +968,19 @@ there will be the following attributes:
>  		Reads what brightness the screen is set to at this instant.
>  
>  	brightness:
> -		Writes request the driver to change brightness to the given
> -		value.  Reads will tell you what brightness the driver is trying
> -		to set the display to when "power" is set to zero and the display
> -		has not been dimmed by a kernel power management event.
> +		Writes request the driver to change brightness to the
> +		given value.  Reads will tell you what brightness the
> +		driver is trying to set the display to when "power" is set
> +		to zero and the display has not been dimmed by a kernel
> +		power management event.
>  
>  	power:
> -		power management mode, where 0 is "display on", and 1 to 3 will
> -		dim the display backlight to brightness level 0 because
> -		thinkpad-acpi cannot really turn the backlight off.  Kernel
> -		power management events can temporarily increase the current
> -		power management level, i.e. they can dim the display.
> +		power management mode, where 0 is "display on", and 1 to 3
> +		will dim the display backlight to brightness level 0
> +		because thinkpad-acpi cannot really turn the backlight
> +		off.  Kernel power management events can temporarily
> +		increase the current power management level, i.e. they can
> +		dim the display.
>  
> 
>  Volume control -- /proc/acpi/ibm/volume
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index 37891a8..2a573db 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -3114,6 +3114,68 @@ static struct backlight_ops ibm_backlight_data = {
>  
>  static struct mutex brightness_mutex;
>  
> +static int __init tpacpi_query_bcl_levels(acpi_handle handle)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	int rc;
> +
> +	if (ACPI_SUCCESS(acpi_evaluate_object(handle, NULL, NULL, &buffer))) {
> +		obj = (union acpi_object *)buffer.pointer;
> +		if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> +			printk(IBM_ERR "Invalid _BCL data\n");
> +			rc = 0;
> +		} else {
> +			if (obj->package.count >= 2)
> +				rc = obj->package.count - 2;
> +			else
> +				rc = 0;
> +		}
> +	} else {
> +		return 0;
> +	}
> +
> +	kfree(buffer.pointer);
> +	return rc;
> +}
> +
> +static acpi_status __init brightness_find_bcl(acpi_handle handle, u32 lvl,
> +					void *context, void **rv)
> +{
> +	char name[ACPI_PATH_SEGMENT_LENGTH];
> +	struct acpi_buffer buffer = { sizeof(name), &name };
> +
> +	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer)) &&
> +	    !strncmp("_BCL", name, sizeof(name) - 1)) {
> +		if (tpacpi_query_bcl_levels(handle) == 16) {
> +			*rv = handle;
> +			return AE_CTRL_TERMINATE;
> +		} else {
> +			return AE_OK;
> +		}
> +	} else {
> +		return AE_OK;
> +	}
> +}
> +
> +static int __init brightness_check_levels(void)
> +{
> +	int status;
> +	void *found_node = NULL;
> +
> +	if (!vid_handle) {
> +		IBM_ACPIHANDLE_INIT(vid);
> +	}
> +	if (!vid_handle)
> +		return 0;
> +
> +	/* Search for a _BCL method with 16 levels */
> +	status = acpi_walk_namespace(ACPI_TYPE_METHOD, vid_handle, 3,
> +					brightness_find_bcl, NULL, &found_node);
> +
> +	return (ACPI_SUCCESS(status) && found_node != NULL);
> +}
> +
>  static int __init brightness_init(struct ibm_init_struct *iibm)
>  {
>  	int b;
> @@ -3135,10 +3197,17 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
>  	if (brightness_mode > 3)
>  		return -EINVAL;
>  
> +	tp_features.bright_16levels =
> +			thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO &&
> +			brightness_check_levels();
> +
>  	b = brightness_get(NULL);
>  	if (b < 0)
>  		return 1;
>  
> +	if (tp_features.bright_16levels)
> +		printk(IBM_INFO "detected a 16-level brightness capable ThinkPad\n");
> +
>  	ibm_backlight_device = backlight_device_register(
>  					TPACPI_BACKLIGHT_DEV_NAME, NULL, NULL,
>  					&ibm_backlight_data);
> @@ -3148,7 +3217,8 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
>  	}
>  	vdbg_printk(TPACPI_DBG_INIT, "brightness is supported\n");
>  
> -	ibm_backlight_device->props.max_brightness = 7;
> +	ibm_backlight_device->props.max_brightness =
> +				(tp_features.bright_16levels)? 15 : 7;
>  	ibm_backlight_device->props.brightness = b;
>  	backlight_update_status(ibm_backlight_device);
>  
> @@ -3184,13 +3254,14 @@ static int brightness_get(struct backlight_device *bd)
>  	if (brightness_mode & 1) {
>  		if (!acpi_ec_read(brightness_offset, &lec))
>  			return -EIO;
> -		lec &= 7;
> +		lec &= (tp_features.bright_16levels)? 0x0f : 0x07;
>  		level = lec;
>  	};
>  	if (brightness_mode & 2) {
>  		lcmos = (nvram_read_byte(TP_NVRAM_ADDR_BRIGHTNESS)
>  			 & TP_NVRAM_MASK_LEVEL_BRIGHTNESS)
>  			>> TP_NVRAM_POS_LEVEL_BRIGHTNESS;
> +		lcmos &= (tp_features.bright_16levels)? 0x0f : 0x07;
>  		level = lcmos;
>  	}
>  
> @@ -3211,7 +3282,7 @@ static int brightness_set(int value)
>  	int cmos_cmd, inc, i, res;
>  	int current_value;
>  
> -	if (value > 7)
> +	if (value > ((tp_features.bright_16levels)? 15 : 7))
>  		return -EINVAL;
>  
>  	res = mutex_lock_interruptible(&brightness_mutex);
> @@ -3227,7 +3298,7 @@ static int brightness_set(int value)
>  	cmos_cmd = value > current_value ?
>  			TP_CMOS_BRIGHTNESS_UP :
>  			TP_CMOS_BRIGHTNESS_DOWN;
> -	inc = value > current_value ? 1 : -1;
> +	inc = (value > current_value)? 1 : -1;
>  
>  	res = 0;
>  	for (i = current_value; i != value; i += inc) {
> @@ -3256,10 +3327,11 @@ static int brightness_read(char *p)
>  	if ((level = brightness_get(NULL)) < 0) {
>  		len += sprintf(p + len, "level:\t\tunreadable\n");
>  	} else {
> -		len += sprintf(p + len, "level:\t\t%d\n", level & 0x7);
> +		len += sprintf(p + len, "level:\t\t%d\n", level);
>  		len += sprintf(p + len, "commands:\tup, down\n");
>  		len += sprintf(p + len, "commands:\tlevel <level>"
> -			       " (<level> is 0-7)\n");
> +			       " (<level> is 0-%d)\n",
> +			       (tp_features.bright_16levels) ? 15 : 7);
>  	}
>  
>  	return len;
> @@ -3270,18 +3342,19 @@ static int brightness_write(char *buf)
>  	int level;
>  	int new_level;
>  	char *cmd;
> +	int max_level = (tp_features.bright_16levels) ? 15 : 7;
>  
>  	while ((cmd = next_cmd(&buf))) {
>  		if ((level = brightness_get(NULL)) < 0)
>  			return level;
> -		level &= 7;
>  
>  		if (strlencmp(cmd, "up") == 0) {
> -			new_level = level == 7 ? 7 : level + 1;
> +			new_level = level == (max_level)?
> +						max_level : level + 1;
>  		} else if (strlencmp(cmd, "down") == 0) {
> -			new_level = level == 0 ? 0 : level - 1;
> +			new_level = level == 0? 0 : level - 1;
>  		} else if (sscanf(cmd, "level %d", &new_level) == 1 &&
> -			   new_level >= 0 && new_level <= 7) {
> +			   new_level >= 0 && new_level <= max_level) {
>  			/* new_level set */
>  		} else
>  			return -EINVAL;
> diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
> index c5fdd68..de0f343 100644
> --- a/drivers/misc/thinkpad_acpi.h
> +++ b/drivers/misc/thinkpad_acpi.h
> @@ -84,7 +84,7 @@
>  
>  /* ThinkPad CMOS NVRAM constants */
>  #define TP_NVRAM_ADDR_BRIGHTNESS       0x5e
> -#define TP_NVRAM_MASK_LEVEL_BRIGHTNESS 0x07
> +#define TP_NVRAM_MASK_LEVEL_BRIGHTNESS 0x0f
>  #define TP_NVRAM_POS_LEVEL_BRIGHTNESS 0
>  
>  #define onoff(status,bit) ((status) & (1 << (bit)) ? "on" : "off")
> @@ -246,6 +246,7 @@ static struct {
>  	u32 hotkey_wlsw:1;
>  	u32 light:1;
>  	u32 light_status:1;
> +	u32 bright_16levels:1;
>  	u32 wan:1;
>  	u32 fan_ctrl_status_undef:1;
>  	u32 input_device_registered:1;

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Thomas Renninger]

On Mon, 2007-10-08 at 10:12 -0300, Henrique de Moraes Holschuh wrote:
> If we detect the presence of _BCL on a Lenovo ThinkPad, it means there is
> support for ACPI standard backlight brightness control.  In that case, it
> is probably best to not make the ThinkPad-specific backlight interface
> available.
> 
> Provide a Kconfig option (default disabled) to make the ThinkPad-specific
> backlight always available, even if there is generic ACPI backlight
> support in the DSDT.
IMO a config variable that compiles out brightness control totally makes
more sense (and video, is there any functionality in ThinkPad acpi
driver that does not get supported by the video driver?).

    Thomas
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> ---
>  Documentation/thinkpad-acpi.txt |    5 ++++-
>  drivers/misc/Kconfig            |   17 +++++++++++++++++
>  drivers/misc/thinkpad_acpi.c    |   20 ++++++++++++++++++++
>  3 files changed, 41 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
> index 4db98b3..7ff0183 100644
> --- a/Documentation/thinkpad-acpi.txt
> +++ b/Documentation/thinkpad-acpi.txt
> @@ -942,7 +942,10 @@ and CMOS.  The driver tries to autodetect which interface to use.
>  
>  When display backlight brightness controls are available through the
>  standard ACPI interface, it is best to use it instead of this direct
> -ThinkPad-specific interface.
> +ThinkPad-specific interface.  In that case, the driver will not export
> +the ThinkPad-specific interface by default, although this can be changed
> +at compile time, using the CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
> +Kconfig option.
>  
>  Procfs notes:
>  
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 73e248f..bd8504a 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -177,6 +177,23 @@ config THINKPAD_ACPI_DEBUG
>  
>  	  If you are not sure, say N here.
>  
> +config THINKPAD_ACPI_BACKLIGHT_DESIRED
> +	bool "Always load backlight support"
> +	depends on THINKPAD_ACPI
> +	default n
> +	---help---
> +	  By default, if the ThinkPad supports the standard ACPI display
> +	  brightness control, the thinkpad-specific backlight support will
> +	  not be loaded.  This avoids duplicate backlight interfaces
> +	  controlling the same LCD, as the ACPI video driver will also publish
> +	  a backlight interface to control the display brightness.
> +
> +	  However, if you prefer to always have the thinkpad-specific backlight
> +	  device available, even when it will just be a duplicate of the
> +	  standard ACPI backlight device, say Y here.
> +
> +	  If you are not sure, say N here.
> +
>  config THINKPAD_ACPI_DOCK
>  	bool "Legacy Docking Station Support"
>  	depends on THINKPAD_ACPI
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index 2a573db..29f521b 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -3114,6 +3114,10 @@ static struct backlight_ops ibm_backlight_data = {
>  
>  static struct mutex brightness_mutex;
>  
> +#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
> +static int brightness_acpi_detected __initdata;
> +#endif
> +
>  static int __init tpacpi_query_bcl_levels(acpi_handle handle)
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -3147,6 +3151,9 @@ static acpi_status __init brightness_find_bcl(acpi_handle handle, u32 lvl,
>  
>  	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer)) &&
>  	    !strncmp("_BCL", name, sizeof(name) - 1)) {
> +#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
> +		brightness_acpi_detected = 1;
> +#endif
>  		if (tpacpi_query_bcl_levels(handle) == 16) {
>  			*rv = handle;
>  			return AE_CTRL_TERMINATE;
> @@ -3197,10 +3204,23 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
>  	if (brightness_mode > 3)
>  		return -EINVAL;
>  
> +#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
> +	brightness_acpi_detected = 0;
> +#endif
> +
>  	tp_features.bright_16levels =
>  			thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO &&
>  			brightness_check_levels();
>  
> +#ifndef CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED
> +	if (brightness_acpi_detected) {
> +		dbg_printk(TPACPI_DBG_INIT,
> +			   "ACPI standard backlight brightness control "
> +			   "available, aborting...\n");
> +		return 1;
> +	}
> +#endif
> +
>  	b = brightness_get(NULL);
>  	if (b < 0)
>  		return 1;
> -- 
> 1.5.3.2

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Matthew Garrett]

On Tue, Oct 09, 2007 at 08:21:14AM +0200, Thomas Renninger wrote:

> IMO a config variable that compiles out brightness control totally makes
> more sense (and video, is there any functionality in ThinkPad acpi
> driver that does not get supported by the video driver?).

No, older Thinkpads don't implement the video extension. It needs to be 
handled at runtime.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Thomas Renninger]

On Tue, 2007-10-09 at 08:59 +0100, Matthew Garrett wrote:
> On Tue, Oct 09, 2007 at 08:21:14AM +0200, Thomas Renninger wrote:
> 
> > IMO a config variable that compiles out brightness control totally makes
> > more sense (and video, is there any functionality in ThinkPad acpi
> > driver that does not get supported by the video driver?).
> 
> No, older Thinkpads don't implement the video extension. It needs to be 
> handled at runtime.

Why?
If you have a recent Lenovo you don't need all this compiled in and do
not set it. Otherwise you add it.

   Thomas

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Matthew Garrett]

On Tue, Oct 09, 2007 at 10:25:51AM +0200, Thomas Renninger wrote:
> On Tue, 2007-10-09 at 08:59 +0100, Matthew Garrett wrote:
> > On Tue, Oct 09, 2007 at 08:21:14AM +0200, Thomas Renninger wrote:
> > 
> > > IMO a config variable that compiles out brightness control totally makes
> > > more sense (and video, is there any functionality in ThinkPad acpi
> > > driver that does not get supported by the video driver?).
> > 
> > No, older Thinkpads don't implement the video extension. It needs to be 
> > handled at runtime.
> 
> Why?
> If you have a recent Lenovo you don't need all this compiled in and do
> not set it. Otherwise you add it.

If you have a recent Lenovo you don't need to worry about the extra few 
hundred bytes of code this is going to take. There's no point in 
microoptimising.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Thomas Renninger]

On Tue, 2007-10-09 at 09:33 +0100, Matthew Garrett wrote:
> On Tue, Oct 09, 2007 at 10:25:51AM +0200, Thomas Renninger wrote:
> > On Tue, 2007-10-09 at 08:59 +0100, Matthew Garrett wrote:
> > > On Tue, Oct 09, 2007 at 08:21:14AM +0200, Thomas Renninger wrote:
> > > 
> > > > IMO a config variable that compiles out brightness control totally makes
> > > > more sense (and video, is there any functionality in ThinkPad acpi
> > > > driver that does not get supported by the video driver?).
> > > 
> > > No, older Thinkpads don't implement the video extension. It needs to be 
> > > handled at runtime.
> > 
> > Why?
> > If you have a recent Lenovo you don't need all this compiled in and do
> > not set it. Otherwise you add it.
> 
> If you have a recent Lenovo you don't need to worry about the extra few 
> hundred bytes of code this is going to take. There's no point in 
> microoptimising.

I don't care for a CONFIG_THINKPAD_VIDEO config, some people love
microoptimising, going through the kernel config, disabling everything
which their hardware does not support. It also structures a bit the
dozens of functionalities in the thinkpad module.

More important: CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED variable is error
prone and should not get introduced, right?

   Thomas

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Matthew Garrett]

On Tue, Oct 09, 2007 at 11:46:48AM +0200, Thomas Renninger wrote:

> More important: CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED variable is error
> prone and should not get introduced, right?

I agree that I can't see any reason to ever want to use the thinkpad 
backlight functionality if it can be controlled through the standard 
mechanism instead.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Henrique de Moraes Holschuh]

On Tue, 09 Oct 2007, Matthew Garrett wrote:
> On Tue, Oct 09, 2007 at 11:46:48AM +0200, Thomas Renninger wrote:
> 
> > More important: CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED variable is error
> > prone and should not get introduced, right?

Error prone how?  Please expand, because right now I am not inclined to
remove that variable.  It is optional, and disabled by default.  Distros are
not going to enable it unless they have a damn good reason to, and it is not
even something that one can enable at runtime.

Should I make the help text stronger against enabling the option? Is it not
clear enough?

If you look at the patch, you will notice that the backlight changes are
small, and the biggest part of them affects __init code, which I make
exactly *zero* effort to optimize for size: it is basically "free" for the
scenarios I have to bother with (thinkpads).

> backlight functionality if it can be controlled through the standard 
> mechanism instead.

Well, it is a simple matter of removing the Kconfig option and making that
code non-optional.  But I am not adding a remove-video-support Kconfig
option at this time.  It will wait until I break the driver down into
multiple modules.

I still want a stronger reason to remove the backlight functionality
possibility on newer thinkpads, though.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 2/4] ACPI: thinkpad-acpi: support 16 levels of brightness (v2)

[Henrique de Moraes Holschuh]

On Tue, 09 Oct 2007, Thomas Renninger wrote:
> On Mon, 2007-10-08 at 10:12 -0300, Henrique de Moraes Holschuh wrote:
> > Lenovo ThinkPads with ACPI Video brightness control often have 16
> > brightness levels in EC, and not just eight levels like older ThinkPads.
> > 
> > We detect the number of brightness levels by the presence of a _BCL method
> > with 18 entries (16 levels plus the on-battery and on-ac recommended
> > levels).  If _BCL is not there, we assume eight levels (T60, for example).
> > Otherwise we assume sixteen levels (T61, X61, etc).
> This is broken by design, pls don't add this to a mainline kernel.

The thinkpad design does not expose the number of levels anywhere else.  It
is either _BCL, or worse (BCLL, a package used by _BLC/_BCM/_BQC in
thinkpads).

> Correct is: If there is _BCL (and other video functions) don't touch any
> video or brightness functionality.

There is a later patch that does just that.  Why is it not enough?

> Thinkpad_acpi should try to register for the acpi device with the
> LNXVIDEO HID. If the probe/add function there is invoked, it can be sure
> that video module tries to load and is handling the stuff in a defined
> way and thinkpad_acpi should get its hands off.

Far more complicated, and in the *specific* case of thinkpad-acpi, where I
know exactly what to poke in 100% of the problem space ("thinkpads where
thinkpad-acpi would handle backlight"), it is probably not the best way to
go about it.

> -----------------------------------------------------------
> ThinkPad has implemented _BCL/_BCM/_BQC method for Vista. 
> These brightness control methods are not used on Windows XP. 

"and Linux did all sort of unspeakable evil things through them not so long
ago"...

And still does even with the latest kernel, I suppose, anywhere people is
feeding back brightness events into thinkpad-acpi's backlight control
through userspace (something that is *not* recommended, and *not* the right
thing to do in *any* thinkpad model IMO).

Granted, some thinkpad models probably require BIOS updates *and* one of the
latest kernels (or a video.c backport), to have proper backlight control.

> Now, I don't understand which point the brightness control is broken. 
> Does Suse call _BCM , but not handle Notify (video, 0x86/0x87)
> (Increase/Decrease Brightness) ?

I am not sure it is broken anymore, actually.  And I don't have the hardware
to test it.

> The BIOSs/Machine Type/Model/Support are too complicated. 
> Why don't you use assume BIOS supports _BCL/_BCM/_BQC if these methods
> are found in ACPI name space ?

That's exactly what I do :p

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Thomas Renninger]

On Tue, 2007-10-09 at 08:14 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 09 Oct 2007, Matthew Garrett wrote:
> > On Tue, Oct 09, 2007 at 11:46:48AM +0200, Thomas Renninger wrote:
> > 
> > > More important: CONFIG_THINKPAD_ACPI_BACKLIGHT_DESIRED variable is error
> > > prone and should not get introduced, right?
> 
> Error prone how?  Please expand, because right now I am not inclined to
> remove that variable.  It is optional, and disabled by default.  Distros are
> not going to enable it unless they have a damn good reason to, and it is not
> even something that one can enable at runtime.
> 
> Should I make the help text stronger against enabling the option? Is it not
> clear enough?
No, all the _BCL poking should vanish.

Fn keys brightness control shouldn't work on any latest thinkpad with
this code. As soon as you invoke _BCL a flag gets set in AML code and
later the AML code (search somewhere in the _QXX functions) checks that
flag and will send a notify to LCD0 (0x86/0x87) and relies on the OS to
lower/increase the brightness through _BCM.

> If you look at the patch, you will notice that the backlight changes are
> small, and the biggest part of them affects __init code, which I make
> exactly *zero* effort to optimize for size: it is basically "free" for the
> scenarios I have to bother with (thinkpads).
>
> > backlight functionality if it can be controlled through the standard 
> > mechanism instead.
> 
> Well, it is a simple matter of removing the Kconfig option and making that
> code non-optional.  But I am not adding a remove-video-support Kconfig
> option at this time.  It will wait until I break the driver down into
> multiple modules.

My suggestion:
static int force_brightness;
module_param(force_brightness, bool, 0);

static int do_not_use_brightness;

static dummy_vid_add(struct acpi_device *device){
	do_not_use_brightness = 1;
	return -EINVAL;
}

static const struct acpi_device_id dummy_vid_device_ids[] = {
	{"LNXVIDEO", 0},
	{"", 0},
};
static struct acpi_driver dummy_vid_driver = {
	.ids = dummy_vid_device_ids,
	.ops = {
		.add = dummy_vid_add,
         }
}

static int __init brightness_init(struct ibm_init_struct *iibm)
{
	if (do_not_use_brightness)
		return -EINVAL;
...
}
...
if (!force_brightness){
	acpi_bus_register_driver(&acpi_dummy_vid_driver);
}

I am a bit anxious with letting two drivers match against "LNXVIDEO",
but I don't know why this should not work. Like that the same code
whether video can register or not is processed and you can be sure
whether video should get active or not.
If the double register does not work, a global variable in
drivers/acpi/scan.c here should do it:
		if(!(info->valid & (ACPI_VALID_HID | ACPI_VALID_CID))){
			status = acpi_video_bus_match(device);
			if(ACPI_SUCCESS(status))
				hid = ACPI_VIDEO_HID;

like that we can be sure (also against modifications at this one place)
video will load.
Hmm, maybe the acpi_video_bus_match test should get extended to match
either against the already tested functions or _BCM/_BCL and maybe only
set the variable if brightness funcs are there.
I'd prefer such a test to avoid the walknamespace call but mainly to
have the matching whether video should be used at one central place.

Also a module parameter to force brightness at module load time should
be more convenient for letting people try out both. I don't see the need
for an extra config variable.

    Thomas

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Matthew Garrett]

On Tue, Oct 09, 2007 at 03:29:12PM +0200, Thomas Renninger wrote:
> static const struct acpi_device_id dummy_vid_device_ids[] = {
> 	{"LNXVIDEO", 0},
> 	{"", 0},
> };

No. This will match if any of the video extension is implemented. We 
only want it to match if backlight control is implemented. There are 
plenty of Thinkpads that implement a subset of the video extension but 
still need backlight control to be handled via the Thinkpad-specific 
routes.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Thomas Renninger]

On Tue, 2007-10-09 at 14:34 +0100, Matthew Garrett wrote:
> On Tue, Oct 09, 2007 at 03:29:12PM +0200, Thomas Renninger wrote:
> > static const struct acpi_device_id dummy_vid_device_ids[] = {
> > 	{"LNXVIDEO", 0},
> > 	{"", 0},
> > };
> 
> No. This will match if any of the video extension is implemented. We 
> only want it to match if backlight control is implemented. There are 
> plenty of Thinkpads that implement a subset of the video extension but 
> still need backlight control to be handled via the Thinkpad-specific 
> routes.
Yep, I just realized that :(
Maybe all required funcs (_BCM,_BCL,...) should get checked, but must
not be invoked or I am pretty sure brightness switch through buttons
won't work because the notify handler isn't used.

I wonder how we should make the video module not load then in a sane way
on those, udev rules in userspace would be very dirty..., dmiscan for
ThinkPad in scan.c and only set LNXVIDEO if _BCM,_BCL... are there also,
maybe the latter is acceptable?

   Thomas

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Henrique de Moraes Holschuh]

On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > Error prone how?  Please expand, because right now I am not inclined to
> > remove that variable.  It is optional, and disabled by default.  Distros are
> > not going to enable it unless they have a damn good reason to, and it is not
> > even something that one can enable at runtime.
> > 
> > Should I make the help text stronger against enabling the option? Is it not
> > clear enough?
>
> No, all the _BCL poking should vanish.

I need to *somehow* find out if the thinkpad supports the video extensions.

> Fn keys brightness control shouldn't work on any latest thinkpad with
> this code. As soon as you invoke _BCL a flag gets set in AML code and
> later the AML code (search somewhere in the _QXX functions) checks that
> flag and will send a notify to LCD0 (0x86/0x87) and relies on the OS to
> lower/increase the brightness through _BCM.

Hmm, to just disable backlight (no checking for levels), I can just check
the existence of _BCL (or _BCM, or _BQC) instead of invoking it.

But even if I want to know how many levels, there is a way. I can locate and
retrieve the BCLL package, and count how many items there are in it.  So it
still doesn't kill the possibility of supporting 16 levels.  So the question
about the Kconfig option is still open.

But I am far more inclined to just drop it, now.

Thanks for the head's up about the _BCL interation, btw.  None of the
testers pointed that problem.  Ugly crap, that one.  There is _DOS for the
OS to tell the AML code what it should do with the notifications, I wish
Lenovo didn't add so many weird work-arounds in their AML SDTs, it cause more
problems than not.

> My suggestion:
> static int force_brightness;
> module_param(force_brightness, bool, 0);
> 
> static int do_not_use_brightness;
> 
> static dummy_vid_add(struct acpi_device *device){
> 	do_not_use_brightness = 1;
> 	return -EINVAL;
> }
> 
> static const struct acpi_device_id dummy_vid_device_ids[] = {
> 	{"LNXVIDEO", 0},
> 	{"", 0},
> };
> static struct acpi_driver dummy_vid_driver = {
> 	.ids = dummy_vid_device_ids,
> 	.ops = {
> 		.add = dummy_vid_add,
>          }
> }
> 
> static int __init brightness_init(struct ibm_init_struct *iibm)
> {
> 	if (do_not_use_brightness)
> 		return -EINVAL;
> ...
> }
> ...
> if (!force_brightness){
> 	acpi_bus_register_driver(&acpi_dummy_vid_driver);
> }

What happens if thinkpad-acpi is loaded before the video driver?

> I am a bit anxious with letting two drivers match against "LNXVIDEO",
> but I don't know why this should not work. Like that the same code
> whether video can register or not is processed and you can be sure
> whether video should get active or not.
> If the double register does not work, a global variable in
> drivers/acpi/scan.c here should do it:
> 		if(!(info->valid & (ACPI_VALID_HID | ACPI_VALID_CID))){
> 			status = acpi_video_bus_match(device);
> 			if(ACPI_SUCCESS(status))
> 				hid = ACPI_VIDEO_HID;
> 
> like that we can be sure (also against modifications at this one place)
> video will load.
> Hmm, maybe the acpi_video_bus_match test should get extended to match
> either against the already tested functions or _BCM/_BCL and maybe only
> set the variable if brightness funcs are there.
> I'd prefer such a test to avoid the walknamespace call but mainly to
> have the matching whether video should be used at one central place.

*if* you give me a place to check, which doesn't make thinkpad-acpi depend
on video.c, I will take it, yes.

> Also a module parameter to force brightness at module load time should
> be more convenient for letting people try out both. I don't see the need
> for an extra config variable.

I don't like to add runtime/load-time parameters for stuff that is
deprecated behaviour.  I only do it to support the roll in of a new
interface.  And using backlight from thinkpad-acpi in a thinkpad where
video.c works fine *is* deprecated behaviour (although I didn't say that
anywhere explictly, yet).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Matthew Garrett]

On Tue, Oct 09, 2007 at 03:47:06PM +0200, Thomas Renninger wrote:

> Yep, I just realized that :(
> Maybe all required funcs (_BCM,_BCL,...) should get checked, but must
> not be invoked or I am pretty sure brightness switch through buttons
> won't work because the notify handler isn't used.

I'm not sure I understand. The video driver works fine on Thinkpads, 
including for backlight control on recent ones.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Thomas Renninger]

On Tue, 2007-10-09 at 10:47 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > > Error prone how?  Please expand, because right now I am not inclined to
> > > remove that variable.  It is optional, and disabled by default.  Distros are
> > > not going to enable it unless they have a damn good reason to, and it is not
> > > even something that one can enable at runtime.
> > > 
> > > Should I make the help text stronger against enabling the option? Is it not
> > > clear enough?
> >
> > No, all the _BCL poking should vanish.
> 
> I need to *somehow* find out if the thinkpad supports the video extensions.

Maybe in scan.c:acpi_video_bus_match() we could add a quirk like:

if (ThinkPad){
 if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCM", &h_dummy1)) &&
 ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCL", &h_dummy2))) &&
 return 0;
else
 return -ENODEV;

This would make the video module only load on thinkpads if the
brightness functions are implemented.
Then the double register could work with this one check, or you are
right, it needs to be checked again in the thinkpad module explicitly.

   Thomas

Re: [PATCH 3/4] ACPI: thinkpad-acpi: disable backlight handler if ACPI generic could do it

[Henrique de Moraes Holschuh]

On Tue, 09 Oct 2007, Thomas Renninger wrote:
> On Tue, 2007-10-09 at 10:47 -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > > No, all the _BCL poking should vanish.
> > 
> > I need to *somehow* find out if the thinkpad supports the video extensions.
> 
> Maybe in scan.c:acpi_video_bus_match() we could add a quirk like:
> 
> if (ThinkPad){
>  if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCM", &h_dummy1)) &&
>  ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCL", &h_dummy2))) &&
>  return 0;
> else
>  return -ENODEV;
> 
> This would make the video module only load on thinkpads if the
> brightness functions are implemented.

The video module handles a lot more than just backlight.  I am not going to
make thinkpad-acpi disable the video module in any way.

Output switching is another area where thinkpad-acpi and video duplicate
functionality, and frankly, I have no idea which ones works, and how well,
and in which thinkpads.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

How to distinguish between general ACPI video driver module and brightness/display providing vendor specific ACPI modules

[Thomas Renninger]

On Tue, 2007-10-09 at 11:29 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > On Tue, 2007-10-09 at 10:47 -0300, Henrique de Moraes Holschuh wrote:
> > > On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > > > No, all the _BCL poking should vanish.
> > > 
> > > I need to *somehow* find out if the thinkpad supports the video extensions.
> > 
> > Maybe in scan.c:acpi_video_bus_match() we could add a quirk like:
> > 
> > if (ThinkPad){
> >  if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCM", &h_dummy1)) &&
> >  ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCL", &h_dummy2))) &&
> >  return 0;
> > else
> >  return -ENODEV;
> > 
> > This would make the video module only load on thinkpads if the
> > brightness functions are implemented.
> 
> The video module handles a lot more than just backlight.  I am not going to
> make thinkpad-acpi disable the video module in any way.
> 
> Output switching is another area where thinkpad-acpi and video duplicate
> functionality, and frankly, I have no idea which ones works, and how well,
> and in which thinkpads.
Me too...
So we have the thinkpad_acpi and other vendor specific drivers and the
general video module, for the latter we want to go for (but was/is
rather untested compared to the others).

What about:
We could compile in a global bit field that gets filled in
video_bus_match in scan.c whether all required brightness functions are
available and another bit reserved for whether all needed video/display
functions are available. This can be done at early ACPI parse time
before any module got loaded.
ThinkPad and other vendor specific modules could then not touch video or
brightness by checking this global bit field.
Video serves brightness and/or display switching if required functions
are available by default the others take their hands off then.

Have I overseen something? Does this make sense?

And now the problem:
Vendor specific drivers might better know whether a machine should get
blacklisted and still should be served by the vendor specific module,
again I'd split up brightness and display/video switching functionality.
Blacklisting might be done by module param to find out machines not
working with the general video module without hurting anyone and for
easy testing and more important, automatically via dmi/DSDT or whatever
blacklisting info. *But* the general video driver will just start
working on these functions if loaded first.
I have no nice idea how to solve that.
The only thing that comes to my mind is to enforce somehow that vendor
specific modules are loaded first and then set something like
disable_video_brightness/disable_video_display global ACPI variables
which the general video driver evaluates when it's loaded afterwards and
in turn doesn't touch these functions even if available.
Not sure whether it can be enforced via udev (in kernel by sending the
uevent for LNXVIDEO as the last one, or by even more ugly userspace
rules...).

Maybe the suggestions after "And now the problem" are a bit overdosed
and the part "What about" is enough and we drive well with such an
approach? -> Just let video serve it's functions which it is designed
for and add fixes/workarounds there if they pop up, no need for vendor
specific workarounds, unfortunately also not possible, at least they
must be located in the video driver itself then.

    Thomas

Re: [ibm-acpi-devel] How to distinguish between general ACPI video driver module and brightness/display providing vendor specific ACPI modules

[Thomas Renninger]

On Tue, 2007-10-09 at 22:53 +0200, Thomas Renninger wrote:
> On Tue, 2007-10-09 at 11:29 -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > > On Tue, 2007-10-09 at 10:47 -0300, Henrique de Moraes Holschuh wrote:
> > > > On Tue, 09 Oct 2007, Thomas Renninger wrote:
> > > > > No, all the _BCL poking should vanish.
> > > > 
> > > > I need to *somehow* find out if the thinkpad supports the video extensions.
> > > 
> > > Maybe in scan.c:acpi_video_bus_match() we could add a quirk like:
> > > 
> > > if (ThinkPad){
> > >  if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCM", &h_dummy1)) &&
> > >  ACPI_SUCCESS(acpi_get_handle(device->handle, "_BCL", &h_dummy2))) &&
> > >  return 0;
> > > else
> > >  return -ENODEV;
> > > 
> > > This would make the video module only load on thinkpads if the
> > > brightness functions are implemented.
> > 
> > The video module handles a lot more than just backlight.  I am not going to
> > make thinkpad-acpi disable the video module in any way.
> > 
> > Output switching is another area where thinkpad-acpi and video duplicate
> > functionality, and frankly, I have no idea which ones works, and how well,
> > and in which thinkpads.
> Me too...
> So we have the thinkpad_acpi and other vendor specific drivers and the
> general video module, for the latter we want to go for (but was/is
> rather untested compared to the others).
> 
> What about:
> We could compile in a global bit field that gets filled in
> video_bus_match in scan.c whether all required brightness functions are
> available and another bit reserved for whether all needed video/display
> functions are available. This can be done at early ACPI parse time
> before any module got loaded.
> ThinkPad and other vendor specific modules could then not touch video or
> brightness by checking this global bit field.
> Video serves brightness and/or display switching if required functions
> are available by default the others take their hands off then.

Unfortunately, I don't have much time (at least the next days) to help
here much.

IMO this should work. Like that there is no need in Thinkpad to fiddle
with hardcoded "\\_SB.../_BCL" paths to find out whether brightness is
supported. Just check for a global variable video_funcs_supported ->
take hands off display switching and brightness_funcs_supported -> take
hands off brightness control. This should work out fine for other vendor
specific drivers also.

FYI:
1)
If I read the thinkpad code right, it always relies on that brightness
level can be read through EC at address 0x31?
I have a T60 here where this does not work, I can switch (only in
console) the brightness levels and there is no change in at any ec
address. So this really must use _BCM,_BCL?

2)
I also found in my X logs on a T60:
(WW) NVIDIA(0): Error: Unable to find DOS (Enable/Disable output switching)
(WW) NVIDIA(0):     file path under /proc/acpi/video. NVIDIA X driver will not
(WW) NVIDIA(0):     be able to respond to  display change hotkey events.
Intel graphics drivers and possibly others probably also want to make
use of this.
So I think switching to video driver here (thinkpad must not touch this
then anymore I expect) makes a lot of sense. I wonder how many
people/tools made use of /proc/acpi/ibm/video?

3)
I think I found a bug in thinkpad_acpi:
	if (!*ibm->acpi->handle)
		return 0;
in setup_acpi_notify(..) probably should be:
	if (!ibm->acpi->handle)
		return 0;

   Thomas

Re: [ibm-acpi-devel] How to distinguish between general ACPI video driver module and brightness/display providing vendor specific ACPI modules

[Henrique de Moraes Holschuh]

On Wed, 10 Oct 2007, Thomas Renninger wrote:
> IMO this should work. Like that there is no need in Thinkpad to fiddle
> with hardcoded "\\_SB.../_BCL" paths to find out whether brightness is
> supported. Just check for a global variable video_funcs_supported ->
> take hands off display switching and brightness_funcs_supported -> take
> hands off brightness control. This should work out fine for other vendor
> specific drivers also.

Well, provided that these drives do work.  I am not sure they do on older
thinkpads, etc.

> If I read the thinkpad code right, it always relies on that brightness
> level can be read through EC at address 0x31?

No.  It can get it from NVRAM too in the latest code, and it will do the
right thing on the newer Lenovo BIOSes.

> I have a T60 here where this does not work, I can switch (only in
> console) the brightness levels and there is no change in at any ec
> address. So this really must use _BCM,_BCL?

See above.  But you do need a *very* recent thinkpad-acpi.  I.e. what is in
acpi-test, or what is in my development versions.

But it *is* still better to use _BCM, _BCL, _BQC when available.

> 2)
> I also found in my X logs on a T60:
> (WW) NVIDIA(0): Error: Unable to find DOS (Enable/Disable output switching)
> (WW) NVIDIA(0):     file path under /proc/acpi/video. NVIDIA X driver will not
> (WW) NVIDIA(0):     be able to respond to  display change hotkey events.
> Intel graphics drivers and possibly others probably also want to make
> use of this.
> So I think switching to video driver here (thinkpad must not touch this
> then anymore I expect) makes a lot of sense. I wonder how many
> people/tools made use of /proc/acpi/ibm/video?

Probably not many.  I really, really refrain from touching that because it
is a rats nest of trouble.

But it *does* work for some people, so I am not fine with the idea of just
ripping it off, unless we know video does at least as well on those
thinkpads.

> 3)
> I think I found a bug in thinkpad_acpi:
> 	if (!*ibm->acpi->handle)
> 		return 0;
> in setup_acpi_notify(..) probably should be:
> 	if (!ibm->acpi->handle)
> 		return 0;

I will check that.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [ibm-acpi-devel] How to distinguish between general ACPI video driver module and brightness/display providing vendor specific ACPI modules

[Henrique de Moraes Holschuh]

On Wed, 10 Oct 2007, Henrique de Moraes Holschuh wrote:
> > 3)
> > I think I found a bug in thinkpad_acpi:
> > 	if (!*ibm->acpi->handle)
> > 		return 0;
> > in setup_acpi_notify(..) probably should be:
> > 	if (!ibm->acpi->handle)
> > 		return 0;
> 
> I will check that.

No bug there.  handle is a pointer to the handle, so it has to be
dereferenced.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh