[GIT PATCH v2] thinkpad-acpi first set of changes for 2.6.33 (v2)

[GIT PATCH v2] thinkpad-acpi first set of changes for 2.6.33 (v2)

[Henrique de Moraes Holschuh]

Len,

This is the second version of the first batch of changes for thinkpad-acpi, for
the merge window.

Patch 4 is already in linux-next through the backlight tree, but it has a merge
problem there, so I have included it on this batch.  I will coordinate with the
backlight maintainer, so that the patch can be merged in mainline through the
acpi tree.

Changes since the last post:
  patch 08 has been refactored as per comments.
  patches 09 and 10 are new.

Please apply to acpi-test.

Thank you!

Shortlog:
Henrique de Moraes Holschuh (10):
      thinkpad-acpi: fix default brightness_mode for R50e/R51
      thinkpad-acpi: preserve rfkill state across suspend/resume
      thinkpad-acpi: fix some version quirks
      thinkpad-acpi: issue backlight class events
      thinkpad-acpi: silence bogus complain during rmmod
      thinkpad-acpi: adopt input device
      thinkpad-acpi: expose module parameters
      thinkpad-acpi: log temperatures on termal alarm (v2)
      thinkpad-acpi: use input_set_capability
      thinkpad-acpi: sync input device EV_SW state directly

-- 
  "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 01/10] thinkpad-acpi: fix default brightness_mode for R50e/R51

[Henrique de Moraes Holschuh]

According to a report, the R50e wants EC-based brightness control,
even if it uses an Intel GPU.  The current driver default was reported
to not work at all.

This bug can be worked around by the "brightness_mode=3" module
parameter.

Change the default of the R50e and R51 2xxx models (which use the same
EC firmware, 1V) to TPACPI_BRGHT_Q_EC, but keep TPACPI_BRGHT_Q_ASK set
for now, as I'd like to get more reports.

This fixes a regression caused by commit
59fe4fe34d7afdf63208124f313be9056feaa2f4,
"thinkpad-acpi: fix incorrect use of TPACPI_BRGHT_MODE_ECNVRAM"

Kernel 2.6.31 also needs this fix.

Reported-by: Ferenc Wagner <wferi@niif.hu>
Tested-by: Ferenc Wagner <wferi@niif.hu>
Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: stable@kernel.org
---
 drivers/platform/x86/thinkpad_acpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index a848c7e..9c6d5a9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6123,8 +6123,8 @@ static const struct tpacpi_quirk brightness_quirk_table[] __initconst = {
 
 	/* Models with Intel Extreme Graphics 2 */
 	TPACPI_Q_IBM('1', 'U', TPACPI_BRGHT_Q_NOEC),
-	TPACPI_Q_IBM('1', 'V', TPACPI_BRGHT_Q_ASK|TPACPI_BRGHT_Q_NOEC),
-	TPACPI_Q_IBM('1', 'W', TPACPI_BRGHT_Q_ASK|TPACPI_BRGHT_Q_NOEC),
+	TPACPI_Q_IBM('1', 'V', TPACPI_BRGHT_Q_ASK|TPACPI_BRGHT_Q_EC),
+	TPACPI_Q_IBM('1', 'W', TPACPI_BRGHT_Q_ASK|TPACPI_BRGHT_Q_EC),
 
 	/* Models with Intel GMA900 */
 	TPACPI_Q_IBM('7', '0', TPACPI_BRGHT_Q_NOEC),	/* T43, R52 */
-- 
1.6.5.4

[PATCH 02/10] thinkpad-acpi: preserve rfkill state across suspend/resume

[Henrique de Moraes Holschuh]

Since the rfkill rework in 2.6.31, the driver is always resuming with
the radios disabled.

Change thinkpad-acpi to ask the firmware to resume with the radios in
the last state.  This fixes the Bluetooth and WWAN rfkill switches.

Note that it means we respect the firmware's oddities.  Should the
user toggle the hardware rfkill switch on and off, it might cause the
radios to resume enabled.

UWB is an unknown quantity since it has nowhere the same level of
firmware support (no control over state storage in NVRAM, for
example), and might need further fixing.  Testers welcome.

This change fixes a regression from 2.6.30.

Reported-by: Jerone Young <jerone.young@canonical.com>
Reported-by: Ian Molton <ian.molton@collabora.co.uk>
Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Tested-by: Ian Molton <ian.molton@collabora.co.uk>
Cc: stable@kernel.org
---
 drivers/platform/x86/thinkpad_acpi.c |   32 +++++---------------------------
 1 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9c6d5a9..1ee734c 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3866,15 +3866,6 @@ enum {
 
 #define TPACPI_RFK_BLUETOOTH_SW_NAME	"tpacpi_bluetooth_sw"
 
-static void bluetooth_suspend(pm_message_t state)
-{
-	/* Try to make sure radio will resume powered off */
-	if (!acpi_evalf(NULL, NULL, "\\BLTH", "vd",
-		   TP_ACPI_BLTH_PWR_OFF_ON_RESUME))
-		vdbg_printk(TPACPI_DBG_RFKILL,
-			"bluetooth power down on resume request failed\n");
-}
-
 static int bluetooth_get_status(void)
 {
 	int status;
@@ -3908,10 +3899,9 @@ static int bluetooth_set_status(enum tpacpi_rfkill_state state)
 #endif
 
 	/* We make sure to keep TP_ACPI_BLUETOOTH_RESUMECTRL off */
+	status = TP_ACPI_BLUETOOTH_RESUMECTRL;
 	if (state == TPACPI_RFK_RADIO_ON)
-		status = TP_ACPI_BLUETOOTH_RADIOSSW;
-	else
-		status = 0;
+		status |= TP_ACPI_BLUETOOTH_RADIOSSW;
 
 	if (!acpi_evalf(hkey_handle, NULL, "SBDC", "vd", status))
 		return -EIO;
@@ -4050,7 +4040,6 @@ static struct ibm_struct bluetooth_driver_data = {
 	.read = bluetooth_read,
 	.write = bluetooth_write,
 	.exit = bluetooth_exit,
-	.suspend = bluetooth_suspend,
 	.shutdown = bluetooth_shutdown,
 };
 
@@ -4068,15 +4057,6 @@ enum {
 
 #define TPACPI_RFK_WWAN_SW_NAME		"tpacpi_wwan_sw"
 
-static void wan_suspend(pm_message_t state)
-{
-	/* Try to make sure radio will resume powered off */
-	if (!acpi_evalf(NULL, NULL, "\\WGSV", "qvd",
-		   TP_ACPI_WGSV_PWR_OFF_ON_RESUME))
-		vdbg_printk(TPACPI_DBG_RFKILL,
-			"WWAN power down on resume request failed\n");
-}
-
 static int wan_get_status(void)
 {
 	int status;
@@ -4109,11 +4089,10 @@ static int wan_set_status(enum tpacpi_rfkill_state state)
 	}
 #endif
 
-	/* We make sure to keep TP_ACPI_WANCARD_RESUMECTRL off */
+	/* We make sure to set TP_ACPI_WANCARD_RESUMECTRL */
+	status = TP_ACPI_WANCARD_RESUMECTRL;
 	if (state == TPACPI_RFK_RADIO_ON)
-		status = TP_ACPI_WANCARD_RADIOSSW;
-	else
-		status = 0;
+		status |= TP_ACPI_WANCARD_RADIOSSW;
 
 	if (!acpi_evalf(hkey_handle, NULL, "SWAN", "vd", status))
 		return -EIO;
@@ -4251,7 +4230,6 @@ static struct ibm_struct wan_driver_data = {
 	.read = wan_read,
 	.write = wan_write,
 	.exit = wan_exit,
-	.suspend = wan_suspend,
 	.shutdown = wan_shutdown,
 };
 
-- 
1.6.5.4

[PATCH 03/10] thinkpad-acpi: fix some version quirks

[Henrique de Moraes Holschuh]

Update some of the BIOS/EC version quirks.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/platform/x86/thinkpad_acpi.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 1ee734c..6160813 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1779,7 +1779,7 @@ static const struct tpacpi_quirk tpacpi_bios_version_qtable[] __initconst = {
 
 	TPV_QL1('7', '9',  'E', '3',  '5', '0'), /* T60/p */
 	TPV_QL1('7', 'C',  'D', '2',  '2', '2'), /* R60, R60i */
-	TPV_QL0('7', 'E',  'D', '0'),		 /* R60e, R60i */
+	TPV_QL1('7', 'E',  'D', '0',  '1', '5'), /* R60e, R60i */
 
 	/*      BIOS FW    BIOS VERS  EC FW     EC VERS */
 	TPV_QI2('1', 'W',  '9', '0',  '1', 'V', '2', '8'), /* R50e (1) */
@@ -1795,8 +1795,8 @@ static const struct tpacpi_quirk tpacpi_bios_version_qtable[] __initconst = {
 	TPV_QI1('7', '4',  '6', '4',  '2', '7'), /* X41 (0) */
 	TPV_QI1('7', '5',  '6', '0',  '2', '0'), /* X41t (0) */
 
-	TPV_QL0('7', 'B',  'D', '7'),		 /* X60/s */
-	TPV_QL0('7', 'J',  '3', '0'),		 /* X60t */
+	TPV_QL1('7', 'B',  'D', '7',  '4', '0'), /* X60/s */
+	TPV_QL1('7', 'J',  '3', '0',  '1', '3'), /* X60t */
 
 	/* (0) - older versions lack DMI EC fw string and functionality */
 	/* (1) - older versions known to lack functionality */
-- 
1.6.5.4

[PATCH 04/10] thinkpad-acpi: issue backlight class events

[Henrique de Moraes Holschuh]

Take advantage of the new events capabilities of the backlight class to
notify userspace of backlight changes.

This depends on "backlight: Allow drivers to update the core, and
generate events on changes", by Matthew Garrett.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Richard Purdie <rpurdie@linux.intel.com>
---
 Documentation/laptops/thinkpad-acpi.txt |   51 +++++++-----------------------
 drivers/platform/x86/thinkpad_acpi.c    |   24 ++++++++++++++-
 2 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index aafcaa6..f5056c7 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -460,6 +460,8 @@ event	code	Key		Notes
 				For Lenovo ThinkPads with a new
 				BIOS, it has to be handled either
 				by the ACPI OSI, or by userspace.
+				The driver does the right thing,
+				never mess with this.
 0x1011	0x10	FN+END		Brightness down.  See brightness
 				up for details.
 
@@ -582,46 +584,15 @@ with hotkey_report_mode.
 
 Brightness hotkey notes:
 
-These are the current sane choices for brightness key mapping in
-thinkpad-acpi:
+Don't mess with the brightness hotkeys in a Thinkpad.  If you want
+notifications for OSD, use the sysfs backlight class event support.
 
-For IBM and Lenovo models *without* ACPI backlight control (the ones on
-which thinkpad-acpi will autoload its backlight interface by default,
-and on which ACPI video does not export a backlight interface):
-
-1. Don't enable or map the brightness hotkeys in thinkpad-acpi, as
-   these older firmware versions unfortunately won't respect the hotkey
-   mask for brightness keys anyway, and always reacts to them.  This
-   usually work fine, unless X.org drivers are doing something to block
-   the BIOS.  In that case, use (3) below.  This is the default mode of
-   operation.
-
-2. Enable the hotkeys, but map them to something else that is NOT
-   KEY_BRIGHTNESS_UP/DOWN or any other keycode that would cause
-   userspace to try to change the backlight level, and use that as an
-   on-screen-display hint.
-
-3. IF AND ONLY IF X.org drivers find a way to block the firmware from
-   automatically changing the brightness, enable the hotkeys and map
-   them to KEY_BRIGHTNESS_UP and KEY_BRIGHTNESS_DOWN, and feed that to
-   something that calls xbacklight.  thinkpad-acpi will not be able to
-   change brightness in that case either, so you should disable its
-   backlight interface.
-
-For Lenovo models *with* ACPI backlight control:
-
-1. Load up ACPI video and use that.  ACPI video will report ACPI
-   events for brightness change keys.  Do not mess with thinkpad-acpi
-   defaults in this case.  thinkpad-acpi should not have anything to do
-   with backlight events in a scenario where ACPI video is loaded:
-   brightness hotkeys must be disabled, and the backlight interface is
-   to be kept disabled as well.  This is the default mode of operation.
-
-2. Do *NOT* load up ACPI video, enable the hotkeys in thinkpad-acpi,
-   and map them to KEY_BRIGHTNESS_UP and KEY_BRIGHTNESS_DOWN.  Process
-   these keys on userspace somehow (e.g. by calling xbacklight).
-   The driver will do this automatically if it detects that ACPI video
-   has been disabled.
+The driver will issue KEY_BRIGHTNESS_UP and KEY_BRIGHTNESS_DOWN events
+automatically for the cases were userspace has to do something to
+implement brightness changes.  When you override these events, you will
+either fail to handle properly the ThinkPads that require explicit
+action to change backlight brightness, or the ThinkPads that require
+that no action be taken to work properly.
 
 
 Bluetooth
@@ -1465,3 +1436,5 @@ Sysfs interface changelog:
 		and it is always able to disable hot keys.  Very old
 		thinkpads are properly supported.  hotkey_bios_mask
 		is deprecated and marked for removal.
+
+0x020600:	Marker for backlight change event support.
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 6160813..4406136 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -22,7 +22,7 @@
  */
 
 #define TPACPI_VERSION "0.23"
-#define TPACPI_SYSFS_VERSION 0x020500
+#define TPACPI_SYSFS_VERSION 0x020600
 
 /*
  *  Changelog:
@@ -6073,6 +6073,12 @@ static int brightness_get(struct backlight_device *bd)
 	return status & TP_EC_BACKLIGHT_LVLMSK;
 }
 
+static void tpacpi_brightness_notify_change(void)
+{
+	backlight_force_update(ibm_backlight_device,
+			       BACKLIGHT_UPDATE_HOTKEY);
+}
+
 static struct backlight_ops ibm_backlight_data = {
 	.get_brightness = brightness_get,
 	.update_status  = brightness_update_status,
@@ -6227,6 +6233,12 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 	ibm_backlight_device->props.brightness = b & TP_EC_BACKLIGHT_LVLMSK;
 	backlight_update_status(ibm_backlight_device);
 
+	vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_BRGHT,
+			"brightness: registering brightness hotkeys "
+			"as change notification\n");
+	tpacpi_hotkey_driver_mask_set(hotkey_driver_mask
+				| TP_ACPI_HKEY_BRGHTUP_MASK
+				| TP_ACPI_HKEY_BRGHTDWN_MASK);;
 	return 0;
 }
 
@@ -6303,6 +6315,9 @@ static int brightness_write(char *buf)
 	 * Doing it this way makes the syscall restartable in case of EINTR
 	 */
 	rc = brightness_set(level);
+	if (!rc && ibm_backlight_device)
+		backlight_force_update(ibm_backlight_device,
+					BACKLIGHT_UPDATE_SYSFS);
 	return (rc == -EINTR)? -ERESTARTSYS : rc;
 }
 
@@ -7702,6 +7717,13 @@ static struct ibm_struct fan_driver_data = {
  */
 static void tpacpi_driver_event(const unsigned int hkey_event)
 {
+	if (ibm_backlight_device) {
+		switch (hkey_event) {
+		case TP_HKEY_EV_BRGHT_UP:
+		case TP_HKEY_EV_BRGHT_DOWN:
+			tpacpi_brightness_notify_change();
+		}
+	}
 }
 
 
-- 
1.6.5.4

[PATCH 05/10] thinkpad-acpi: silence bogus complain during rmmod

[Henrique de Moraes Holschuh]

Fix this bogus warning during module shutdown, when
backlight event reporting is enabled:

"thinkpad_acpi: required events 0x00018000 not enabled!"

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/platform/x86/thinkpad_acpi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4406136..86418cd 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2189,7 +2189,8 @@ static int hotkey_mask_set(u32 mask)
 		       fwmask, hotkey_acpi_mask);
 	}
 
-	hotkey_mask_warn_incomplete_mask();
+	if (tpacpi_lifecycle != TPACPI_LIFE_EXITING)
+		hotkey_mask_warn_incomplete_mask();
 
 	return rc;
 }
-- 
1.6.5.4

[PATCH 06/10] thinkpad-acpi: adopt input device

[Henrique de Moraes Holschuh]

Properly init the parent field of the input device.  Thanks to Alan
Jenkins, who noted this problem in a different driver.

Reported-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/platform/x86/thinkpad_acpi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 86418cd..dd8bd07 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8375,6 +8375,7 @@ static int __init thinkpad_acpi_module_init(void)
 						PCI_VENDOR_ID_IBM;
 		tpacpi_inputdev->id.product = TPACPI_HKEY_INPUT_PRODUCT;
 		tpacpi_inputdev->id.version = TPACPI_HKEY_INPUT_VERSION;
+		tpacpi_inputdev->dev.parent = &tpacpi_pdev->dev;
 	}
 	for (i = 0; i < ARRAY_SIZE(ibms_init); i++) {
 		ret = ibm_init(&ibms_init[i]);
-- 
1.6.5.4

[PATCH 07/10] thinkpad-acpi: expose module parameters

[Henrique de Moraes Holschuh]

Export the normal (non-command) module paramenters as mode 0444, so
that they will show up in sysfs.

These parameters must not be changed at runtime as a rule, with very
few exceptions.

Reported-by: Ferenc Wagner <wferi@niif.hu>
Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/platform/x86/thinkpad_acpi.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index dd8bd07..4b96a96 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8116,32 +8116,32 @@ static int __init set_ibm_param(const char *val, struct kernel_param *kp)
 	return -EINVAL;
 }
 
-module_param(experimental, int, 0);
+module_param(experimental, int, 0444);
 MODULE_PARM_DESC(experimental,
 		 "Enables experimental features when non-zero");
 
 module_param_named(debug, dbg_level, uint, 0);
 MODULE_PARM_DESC(debug, "Sets debug level bit-mask");
 
-module_param(force_load, bool, 0);
+module_param(force_load, bool, 0444);
 MODULE_PARM_DESC(force_load,
 		 "Attempts to load the driver even on a "
 		 "mis-identified ThinkPad when true");
 
-module_param_named(fan_control, fan_control_allowed, bool, 0);
+module_param_named(fan_control, fan_control_allowed, bool, 0444);
 MODULE_PARM_DESC(fan_control,
 		 "Enables setting fan parameters features when true");
 
-module_param_named(brightness_mode, brightness_mode, uint, 0);
+module_param_named(brightness_mode, brightness_mode, uint, 0444);
 MODULE_PARM_DESC(brightness_mode,
 		 "Selects brightness control strategy: "
 		 "0=auto, 1=EC, 2=UCMS, 3=EC+NVRAM");
 
-module_param(brightness_enable, uint, 0);
+module_param(brightness_enable, uint, 0444);
 MODULE_PARM_DESC(brightness_enable,
 		 "Enables backlight control when 1, disables when 0");
 
-module_param(hotkey_report_mode, uint, 0);
+module_param(hotkey_report_mode, uint, 0444);
 MODULE_PARM_DESC(hotkey_report_mode,
 		 "used for backwards compatibility with userspace, "
 		 "see documentation");
@@ -8164,25 +8164,25 @@ TPACPI_PARAM(volume);
 TPACPI_PARAM(fan);
 
 #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES
-module_param(dbg_wlswemul, uint, 0);
+module_param(dbg_wlswemul, uint, 0444);
 MODULE_PARM_DESC(dbg_wlswemul, "Enables WLSW emulation");
 module_param_named(wlsw_state, tpacpi_wlsw_emulstate, bool, 0);
 MODULE_PARM_DESC(wlsw_state,
 		 "Initial state of the emulated WLSW switch");
 
-module_param(dbg_bluetoothemul, uint, 0);
+module_param(dbg_bluetoothemul, uint, 0444);
 MODULE_PARM_DESC(dbg_bluetoothemul, "Enables bluetooth switch emulation");
 module_param_named(bluetooth_state, tpacpi_bluetooth_emulstate, bool, 0);
 MODULE_PARM_DESC(bluetooth_state,
 		 "Initial state of the emulated bluetooth switch");
 
-module_param(dbg_wwanemul, uint, 0);
+module_param(dbg_wwanemul, uint, 0444);
 MODULE_PARM_DESC(dbg_wwanemul, "Enables WWAN switch emulation");
 module_param_named(wwan_state, tpacpi_wwan_emulstate, bool, 0);
 MODULE_PARM_DESC(wwan_state,
 		 "Initial state of the emulated WWAN switch");
 
-module_param(dbg_uwbemul, uint, 0);
+module_param(dbg_uwbemul, uint, 0444);
 MODULE_PARM_DESC(dbg_uwbemul, "Enables UWB switch emulation");
 module_param_named(uwb_state, tpacpi_uwb_emulstate, bool, 0);
 MODULE_PARM_DESC(uwb_state,
-- 
1.6.5.4

[PATCH 08/10] thinkpad-acpi: log temperatures on termal alarm (v2)

[Henrique de Moraes Holschuh]

Log temperatures on any of the EC thermal alarms.  It could be
useful to help tracking down what is happening...

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Pavel Machek <pavel@ucw.cz>
---
 drivers/platform/x86/thinkpad_acpi.c |   57 ++++++++++++++++++++++++++-------
 1 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4b96a96..b3e256b 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3546,49 +3546,57 @@ static bool hotkey_notify_usrevent(const u32 hkey,
 	}
 }
 
+static void thermal_dump_all_sensors(void);
+
 static bool hotkey_notify_thermal(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
 {
+	bool known = true;
+
 	/* 0x6000-0x6FFF: thermal alarms */
 	*send_acpi_ev = true;
 	*ignore_acpi_ev = false;
 
 	switch (hkey) {
+	case TP_HKEY_EV_THM_TABLE_CHANGED:
+		printk(TPACPI_INFO
+			"EC reports that Thermal Table has changed\n");
+		/* recommended action: do nothing, we don't have
+		 * Lenovo ATM information */
+		return true;
 	case TP_HKEY_EV_ALARM_BAT_HOT:
 		printk(TPACPI_CRIT
 			"THERMAL ALARM: battery is too hot!\n");
 		/* recommended action: warn user through gui */
-		return true;
+		break;
 	case TP_HKEY_EV_ALARM_BAT_XHOT:
 		printk(TPACPI_ALERT
 			"THERMAL EMERGENCY: battery is extremely hot!\n");
 		/* recommended action: immediate sleep/hibernate */
-		return true;
+		break;
 	case TP_HKEY_EV_ALARM_SENSOR_HOT:
 		printk(TPACPI_CRIT
 			"THERMAL ALARM: "
 			"a sensor reports something is too hot!\n");
 		/* recommended action: warn user through gui, that */
 		/* some internal component is too hot */
-		return true;
+		break;
 	case TP_HKEY_EV_ALARM_SENSOR_XHOT:
 		printk(TPACPI_ALERT
 			"THERMAL EMERGENCY: "
 			"a sensor reports something is extremely hot!\n");
 		/* recommended action: immediate sleep/hibernate */
-		return true;
-	case TP_HKEY_EV_THM_TABLE_CHANGED:
-		printk(TPACPI_INFO
-			"EC reports that Thermal Table has changed\n");
-		/* recommended action: do nothing, we don't have
-		 * Lenovo ATM information */
-		return true;
+		break;
 	default:
 		printk(TPACPI_ALERT
 			 "THERMAL ALERT: unknown thermal alarm received\n");
-		return false;
+		known = false;
 	}
+
+	thermal_dump_all_sensors();
+
+	return known;
 }
 
 static void hotkey_notify(struct ibm_struct *ibm, u32 event)
@@ -5462,8 +5470,11 @@ enum { /* TPACPI_THERMAL_TPEC_* */
 	TP_EC_THERMAL_TMP0 = 0x78,	/* ACPI EC regs TMP 0..7 */
 	TP_EC_THERMAL_TMP8 = 0xC0,	/* ACPI EC regs TMP 8..15 */
 	TP_EC_THERMAL_TMP_NA = -128,	/* ACPI EC sensor not available */
+
+	TPACPI_THERMAL_SENSOR_NA = -128000, /* Sensor not available */
 };
 
+
 #define TPACPI_MAX_THERMAL_SENSORS 16	/* Max thermal sensors supported */
 struct ibm_thermal_sensors_struct {
 	s32 temp[TPACPI_MAX_THERMAL_SENSORS];
@@ -5553,6 +5564,28 @@ static int thermal_get_sensors(struct ibm_thermal_sensors_struct *s)
 	return n;
 }
 
+static void thermal_dump_all_sensors(void)
+{
+	int n, i;
+	struct ibm_thermal_sensors_struct t;
+
+	n = thermal_get_sensors(&t);
+	if (n <= 0)
+		return;
+
+	printk(TPACPI_NOTICE
+		"temperatures (Celsius):");
+
+	for (i = 0; i < n; i++) {
+		if (t.temp[i] != TPACPI_THERMAL_SENSOR_NA)
+			printk(KERN_CONT " %d", (int)(t.temp[i] / 1000));
+		else
+			printk(KERN_CONT " N/A");
+	}
+
+	printk(KERN_CONT "\n");
+}
+
 /* sysfs temp##_input -------------------------------------------------- */
 
 static ssize_t thermal_temp_input_show(struct device *dev,
@@ -5568,7 +5601,7 @@ static ssize_t thermal_temp_input_show(struct device *dev,
 	res = thermal_get_sensor(idx, &value);
 	if (res)
 		return res;
-	if (value == TP_EC_THERMAL_TMP_NA * 1000)
+	if (value == TPACPI_THERMAL_SENSOR_NA)
 		return -ENXIO;
 
 	return snprintf(buf, PAGE_SIZE, "%d\n", value);
-- 
1.6.5.4

[PATCH 09/10] thinkpad-acpi: use input_set_capability

[Henrique de Moraes Holschuh]

Use input_set_capability() instead of set_bit.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/platform/x86/thinkpad_acpi.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index b3e256b..53d6c33 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3348,16 +3348,14 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 			TPACPI_HOTKEY_MAP_SIZE);
 	}
 
-	set_bit(EV_KEY, tpacpi_inputdev->evbit);
-	set_bit(EV_MSC, tpacpi_inputdev->evbit);
-	set_bit(MSC_SCAN, tpacpi_inputdev->mscbit);
+	input_set_capability(tpacpi_inputdev, EV_MSC, MSC_SCAN);
 	tpacpi_inputdev->keycodesize = TPACPI_HOTKEY_MAP_TYPESIZE;
 	tpacpi_inputdev->keycodemax = TPACPI_HOTKEY_MAP_LEN;
 	tpacpi_inputdev->keycode = hotkey_keycode_map;
 	for (i = 0; i < TPACPI_HOTKEY_MAP_LEN; i++) {
 		if (hotkey_keycode_map[i] != KEY_RESERVED) {
-			set_bit(hotkey_keycode_map[i],
-				tpacpi_inputdev->keybit);
+			input_set_capability(tpacpi_inputdev, EV_KEY,
+						hotkey_keycode_map[i]);
 		} else {
 			if (i < sizeof(hotkey_reserved_mask)*8)
 				hotkey_reserved_mask |= 1 << i;
@@ -3365,12 +3363,10 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	}
 
 	if (tp_features.hotkey_wlsw) {
-		set_bit(EV_SW, tpacpi_inputdev->evbit);
-		set_bit(SW_RFKILL_ALL, tpacpi_inputdev->swbit);
+		input_set_capability(tpacpi_inputdev, EV_SW, SW_RFKILL_ALL);
 	}
 	if (tp_features.hotkey_tablet) {
-		set_bit(EV_SW, tpacpi_inputdev->evbit);
-		set_bit(SW_TABLET_MODE, tpacpi_inputdev->swbit);
+		input_set_capability(tpacpi_inputdev, EV_SW, SW_TABLET_MODE);
 	}
 
 	/* Do not issue duplicate brightness change events to
-- 
1.6.5.4

[PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Henrique de Moraes Holschuh]

Before we register the input device, sync the input layer EV_SW state
directly by setting the bitmaps, to avoid issuing a gratuitous event
for the initial state of these switches.

I will propose a clean input layer API for this and change the driver
to use it later, but I'd rather get the driver fix in mainline ASAP.

This fixes some annoyances caused by the interaction with rfkill and
EV_SW SW_RFKILL_ALL events.

Reported-by: Kevin Locke <kevin@kevinlocke.name>
Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/platform/x86/thinkpad_acpi.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 53d6c33..e7ff6c7 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3186,6 +3186,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	int res, i;
 	int status;
 	int hkeyv;
+	bool radiosw_state  = false;
+	bool tabletsw_state = false;
 
 	unsigned long quirks;
 
@@ -3291,6 +3293,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES
 	if (dbg_wlswemul) {
 		tp_features.hotkey_wlsw = 1;
+		radiosw_state = !!tpacpi_wlsw_emulstate;
 		printk(TPACPI_INFO
 			"radio switch emulation enabled\n");
 	} else
@@ -3298,6 +3301,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	/* Not all thinkpads have a hardware radio switch */
 	if (acpi_evalf(hkey_handle, &status, "WLSW", "qd")) {
 		tp_features.hotkey_wlsw = 1;
+		radiosw_state = !!status;
 		printk(TPACPI_INFO
 			"radio switch found; radios are %s\n",
 			enabled(status, 0));
@@ -3309,11 +3313,11 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	/* For X41t, X60t, X61t Tablets... */
 	if (!res && acpi_evalf(hkey_handle, &status, "MHKG", "qd")) {
 		tp_features.hotkey_tablet = 1;
+		tabletsw_state = !!(status & TP_HOTKEY_TABLET_MASK);
 		printk(TPACPI_INFO
 			"possible tablet mode switch found; "
 			"ThinkPad in %s mode\n",
-			(status & TP_HOTKEY_TABLET_MASK)?
-				"tablet" : "laptop");
+			(tabletsw_state) ? "tablet" : "laptop");
 		res = add_to_attr_set(hotkey_dev_attributes,
 				&dev_attr_hotkey_tablet_mode.attr);
 	}
@@ -3364,9 +3368,13 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
 	if (tp_features.hotkey_wlsw) {
 		input_set_capability(tpacpi_inputdev, EV_SW, SW_RFKILL_ALL);
+		if (radiosw_state)
+			__set_bit(SW_RFKILL_ALL, tpacpi_inputdev->sw);
 	}
 	if (tp_features.hotkey_tablet) {
 		input_set_capability(tpacpi_inputdev, EV_SW, SW_TABLET_MODE);
+		if (tabletsw_state)
+			__set_bit(SW_TABLET_MODE, tpacpi_inputdev->sw);
 	}
 
 	/* Do not issue duplicate brightness change events to
@@ -3433,8 +3441,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	tpacpi_inputdev->close = &hotkey_inputdev_close;
 
 	hotkey_poll_setup_safe(true);
-	tpacpi_send_radiosw_update();
-	tpacpi_input_send_tabletsw();
 
 	return 0;
 
-- 
1.6.5.4

Re: [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Dmitry Torokhov]

On Tuesday 08 December 2009 05:36:30 pm Henrique de Moraes Holschuh wrote:
> Before we register the input device, sync the input layer EV_SW state
> directly by setting the bitmaps, to avoid issuing a gratuitous event
> for the initial state of these switches.
> 
> I will propose a clean input layer API for this and change the driver
> to use it later, but I'd rather get the driver fix in mainline ASAP.
> 

Just do input_report_switch() before registering the device, it will do
the right thing.

Input core guarantees (and will continue doing so) that it is safe to
pass events to the device as soon as it was allocated with
input_allocate_device().

-- 
Dmitry

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev

Re: [PATCH 08/10] thinkpad-acpi: log temperatures on termal alarm (v2)

[Pavel Machek]

On Tue 2009-12-08 23:36:28, Henrique de Moraes Holschuh wrote:
> Log temperatures on any of the EC thermal alarms.  It could be
> useful to help tracking down what is happening...
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Pavel Machek <pavel@ucw.cz>

ACK.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Henrique de Moraes Holschuh]

On Tue, 08 Dec 2009, Dmitry Torokhov wrote:
> On Tuesday 08 December 2009 05:36:30 pm Henrique de Moraes Holschuh wrote:
> > Before we register the input device, sync the input layer EV_SW state
> > directly by setting the bitmaps, to avoid issuing a gratuitous event
> > for the initial state of these switches.
> > 
> > I will propose a clean input layer API for this and change the driver
> > to use it later, but I'd rather get the driver fix in mainline ASAP.
> > 
> 
> Just do input_report_switch() before registering the device, it will do
> the right thing.

At device startup, the input core doesn't know the initial state of the
switch. It assumes the switch is "off" (all bits clear in the sw bitmap).

input_report_switch() will call input_event(), which will have a 50% chance
of doing the wrong thing at startup (i.e. issue an event) since it will look
at the state of the sw bitmap to decide whether to issue an event or not.

> Input core guarantees (and will continue doing so) that it is safe to
> pass events to the device as soon as it was allocated with
> input_allocate_device().

Well, there really isn't a transparent way to do this...

Either you change the core to use the first event reported to init the
switch and never issue that first event outside the core (we will also have
to make sure all drivers know they have to issue this initial event), or you
add an API for the drivers to directly inform the core of the initial state
of the switches (I have this already written) and check all drivers that
export EV_SW events to make sure they use the API (I haven't done this 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 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Dmitry Torokhov]

On Wed, Dec 09, 2009 at 02:30:41PM -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 08 Dec 2009, Dmitry Torokhov wrote:
> > On Tuesday 08 December 2009 05:36:30 pm Henrique de Moraes Holschuh wrote:
> > > Before we register the input device, sync the input layer EV_SW state
> > > directly by setting the bitmaps, to avoid issuing a gratuitous event
> > > for the initial state of these switches.
> > > 
> > > I will propose a clean input layer API for this and change the driver
> > > to use it later, but I'd rather get the driver fix in mainline ASAP.
> > > 
> > 
> > Just do input_report_switch() before registering the device, it will do
> > the right thing.
> 
> At device startup, the input core doesn't know the initial state of the
> switch. It assumes the switch is "off" (all bits clear in the sw bitmap).
> 
> input_report_switch() will call input_event(), which will have a 50% chance
> of doing the wrong thing at startup (i.e. issue an event) since it will look
> at the state of the sw bitmap to decide whether to issue an event or not.
> 

It will not propagate events because until you register the device there
won't be any consumers attached to it. So the only thing that will
happen is that it will sync internal state of the device from input core
point of view with the true state of the hardware.

> > Input core guarantees (and will continue doing so) that it is safe to
> > pass events to the device as soon as it was allocated with
> > input_allocate_device().
> 
> Well, there really isn't a transparent way to do this...
> 
> Either you change the core to use the first event reported to init the
> switch and never issue that first event outside the core (we will also have
> to make sure all drivers know they have to issue this initial event), or you
> add an API for the drivers to directly inform the core of the initial state
> of the switches (I have this already written) and check all drivers that
> export EV_SW events to make sure they use the API (I haven't done this yet).

There is already such API, input_event(). See above.

-- 
Dmitry

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev

Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Henrique de Moraes Holschuh]

On Wed, 09 Dec 2009, Dmitry Torokhov wrote:
> > input_report_switch() will call input_event(), which will have a 50% chance
> > of doing the wrong thing at startup (i.e. issue an event) since it will look
> > at the state of the sw bitmap to decide whether to issue an event or not.
> > 
> 
> It will not propagate events because until you register the device there
> won't be any consumers attached to it. So the only thing that will
> happen is that it will sync internal state of the device from input core
> point of view with the true state of the hardware.

Ah, I see.  Cute trick, and yes, that would work just fine.  I will do that,
it certainly beats accessing the sw bitmap directly.

Is it documented anywhere?

Still, please look at the patch below...  Would something like this be a
cleaner API?  It is certainly more obvious, and it is cleaner on the driver
side (one function call does everything, instead of a call to
input_set_capability plus a call to whatever the driver needs to issue the
initial EV_SW event)...

I left the thinkpad-acpi hunk in it to show how it makes things easier for
the driver.

Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Date:   Sun Dec 6 14:04:30 2009 -0200

    input: add API to set initial EV_SW state
    
    Add an API for input drivers to init EV_SW switches.
    
    Change the thinkpad-acpi driver to use the new API.
    
    Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
    Cc: Dmitry Torokhov <dtor@mail.ru>

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 7c237e6..4fcb362 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1388,6 +1388,33 @@ void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int
 EXPORT_SYMBOL(input_set_capability);
 
 /**
+ * input_init_switch - prepare device to handle a certain EV_SW event
+ * @dev: device that is capable of issuing the EV_SW event
+ * @code: event code
+ * @state: initial state for the switch
+ *
+ * This function sets the corresponding bits in the capability bitmap,
+ * switch capability bitmap, and switch state bitmap.
+ */
+void input_init_switch(struct input_dev *dev, unsigned int code, bool state)
+{
+	if (code > SW_MAX) {
+		printk(KERN_ERR
+			"input_init_switch: unknown code %u\n",
+			code);
+		dump_stack();
+		return;
+	}
+
+	input_set_capability(dev, EV_SW, code);
+	if (state)
+		__set_bit(code, dev->sw);
+	else
+		__clear_bit(code, dev->sw);
+}
+EXPORT_SYMBOL(input_init_switch);
+
+/**
  * input_register_device - register device with input core
  * @dev: device to be registered
  *
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index a63ceb1..efa6830 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3365,16 +3365,12 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		}
 	}
 
-	if (tp_features.hotkey_wlsw) {
-		input_set_capability(tpacpi_inputdev, EV_SW, SW_RFKILL_ALL);
-		if (radiosw_state)
-			__set_bit(SW_RFKILL_ALL, tpacpi_inputdev->sw);
-	}
-	if (tp_features.hotkey_tablet) {
-		input_set_capability(tpacpi_inputdev, EV_SW, SW_TABLET_MODE);
-		if (tabletsw_state)
-			__set_bit(SW_TABLET_MODE, tpacpi_inputdev->sw);
-	}
+	if (tp_features.hotkey_wlsw)
+		input_init_switch(tpacpi_inputdev,
+				  SW_RFKILL_ALL, radiosw_state);
+	if (tp_features.hotkey_tablet)
+		input_init_switch(tpacpi_inputdev,
+				  SW_TABLET_MODE, tabletsw_state);
 
 	/* Do not issue duplicate brightness change events to
 	 * userspace */
diff --git a/include/linux/input.h b/include/linux/input.h
index 8b3bc3e..40ecd48 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -1353,6 +1353,8 @@ static inline void input_set_abs_params(struct input_dev *dev, int axis, int min
 int input_get_keycode(struct input_dev *dev, int scancode, int *keycode);
 int input_set_keycode(struct input_dev *dev, int scancode, int keycode);
 
+void input_init_switch(struct input_dev *dev, unsigned int code, bool state);
+
 extern struct class input_class;
 
 /**


-- 
  "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: [GIT PATCH v2] thinkpad-acpi first set of changes for 2.6.33 (v2)

[Len Brown]

1-9 applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Dmitry Torokhov]

On Wed, Dec 09, 2009 at 06:32:01PM -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 09 Dec 2009, Dmitry Torokhov wrote:
> > > input_report_switch() will call input_event(), which will have a 50% chance
> > > of doing the wrong thing at startup (i.e. issue an event) since it will look
> > > at the state of the sw bitmap to decide whether to issue an event or not.
> > > 
> > 
> > It will not propagate events because until you register the device there
> > won't be any consumers attached to it. So the only thing that will
> > happen is that it will sync internal state of the device from input core
> > point of view with the true state of the hardware.
> 
> Ah, I see.  Cute trick, and yes, that would work just fine.  I will do that,
> it certainly beats accessing the sw bitmap directly.
> 
> Is it documented anywhere?

Not explicetiley, no.

> 
> Still, please look at the patch below...  Would something like this be a
> cleaner API?  It is certainly more obvious, and it is cleaner on the driver
> side (one function call does everything, instead of a call to
> input_set_capability plus a call to whatever the driver needs to issue the
> initial EV_SW event)...
> 

Yes, I think it is a good idea. However why don't we change it to:

input_setup_event(dev, type, code, value)
{
	input_set_capability(...);
	input_event(...);
}

So it would work for everything (who knows, maybe down the road some
driver wants to init its ABS axes properly and so on)?

-- 
Dmitry

Re: [GIT PATCH v2] thinkpad-acpi first set of changes for 2.6.33 (v2)

[Henrique de Moraes Holschuh]

On Wed, 09 Dec 2009, Len Brown wrote:
> 1-9 applied to acpi-test

Thanks.  I will respin patch 10, add a trivial patch for bug 13600, and work
on the seq_file patch while I wait for comments on the other stack of 4
patches I sent for review.

I will submit them all to you in a second batch, in a few days.

-- 
  "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] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Henrique de Moraes Holschuh]

On Wed, 09 Dec 2009, Dmitry Torokhov wrote:
> > Still, please look at the patch below...  Would something like this be a
> > cleaner API?  It is certainly more obvious, and it is cleaner on the driver
> > side (one function call does everything, instead of a call to
> > input_set_capability plus a call to whatever the driver needs to issue the
> > initial EV_SW event)...
> > 
> 
> Yes, I think it is a good idea. However why don't we change it to:
> 
> input_setup_event(dev, type, code, value)
> {
> 	input_set_capability(...);
> 	input_event(...);
> }
> 
> So it would work for everything (who knows, maybe down the road some
> driver wants to init its ABS axes properly and so on)?

Well, we already have input_set_abs_params, I'd say that's the correct place
to init the axes... and you'd compile-break any drivers that need love to
properly init the axes, so it is a win-win situation as it would make sure
the patch is feature-complete (i.e. converts all drivers to the new call and
makes sure they all init their abs params properly).

The only other event that needs initialization are the switches, so I'd
argue that we might as well use my proposed patch, which is specific and
more lightweight, and convert the in-tree drivers to it.  A few might be
missing the before-registering input_event...

BTW, looking at input.h, wouldn't it be better to force the init functions
to be run before registering the input device, doing a BUG_ON() if they're
misused?   I'd also suggest a BUG_ON(!dev), or at least an if (!dev) return
-EINVAL; to the top of input_register_device(...)...

-- 
  "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] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Dmitry Torokhov]

On Thu, Dec 10, 2009 at 11:19:18AM -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 09 Dec 2009, Dmitry Torokhov wrote:
> > > Still, please look at the patch below...  Would something like this be a
> > > cleaner API?  It is certainly more obvious, and it is cleaner on the driver
> > > side (one function call does everything, instead of a call to
> > > input_set_capability plus a call to whatever the driver needs to issue the
> > > initial EV_SW event)...
> > > 
> > 
> > Yes, I think it is a good idea. However why don't we change it to:
> > 
> > input_setup_event(dev, type, code, value)
> > {
> > 	input_set_capability(...);
> > 	input_event(...);
> > }
> > 
> > So it would work for everything (who knows, maybe down the road some
> > driver wants to init its ABS axes properly and so on)?
> 
> Well, we already have input_set_abs_params, I'd say that's the correct place
> to init the axes... and you'd compile-break any drivers that need love to
> properly init the axes, so it is a win-win situation as it would make sure
> the patch is feature-complete (i.e. converts all drivers to the new call and
> makes sure they all init their abs params properly).

Right, input_set_abs_params()... No, I don't really want to go through
all touchpad, touchscreen and joystick drivers at once, thank you vey
much ;) Besides, for the vast majority of uses 0 is the proper initial
value for absolute axis.


> The only other event that needs initialization are the switches, so I'd
> argue that we might as well use my proposed patch, which is specific and
> more lightweight, and convert the in-tree drivers to it.  A few might be
> missing the before-registering input_event...
> 

As I think about it even more I think we should leave it asd is. You
should just split setting up the device's capabilities and seeding of
the initial values. Given that you need to re-seed the values upon
resume - and there you do need to use input_event() - just create a
function that queries the hardwware and sends the events and invoke it
once upon registration and also resume hardler.

> BTW, looking at input.h, wouldn't it be better to force the init functions
> to be run before registering the input device, doing a BUG_ON() if they're
> misused?

What function are you referring to? Input devices are in useable state
as returned by input_allocate_device().

>   I'd also suggest a BUG_ON(!dev), or at least an if (!dev) return
> -EINVAL; to the top of input_register_device(...)...
> 

Nah, just let it crash (the same effect as BUG_ON really).

-- 
Dmitry

Re: [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Henrique de Moraes Holschuh]

On Thu, 10 Dec 2009, Dmitry Torokhov wrote:
> Right, input_set_abs_params()... No, I don't really want to go through
> all touchpad, touchscreen and joystick drivers at once, thank you vey
> much ;) Besides, for the vast majority of uses 0 is the proper initial
> value for absolute axis.

Heh.

> > The only other event that needs initialization are the switches, so I'd
> > argue that we might as well use my proposed patch, which is specific and
> > more lightweight, and convert the in-tree drivers to it.  A few might be
> > missing the before-registering input_event...
> 
> As I think about it even more I think we should leave it asd is. You

It needs to be documented somewhere, at least.

> > BTW, looking at input.h, wouldn't it be better to force the init functions
> > to be run before registering the input device, doing a BUG_ON() if they're
> > misused?
> 
> What function are you referring to? Input devices are in useable state

The ones proposed in this subthread, which shouldn't be used after
registering.

> as returned by input_allocate_device().

Unless, of course, someone screws up verifying if input_alocate_device
returned NULL...

> >   I'd also suggest a BUG_ON(!dev), or at least an if (!dev) return
> > -EINVAL; to the top of input_register_device(...)...
> 
> Nah, just let it crash (the same effect as BUG_ON really).

There's a difference, and it is not small if it happens outside of a driver
developer's test kernel.

Not that I'd expect anyone to ship code so broken, that would manage to
input_register_device(NULL), so I don't have any strong feelings on this.

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

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev

Re: [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Dmitry Torokhov]

On Thu, Dec 10, 2009 at 10:46:02PM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 10 Dec 2009, Dmitry Torokhov wrote:
> > Right, input_set_abs_params()... No, I don't really want to go through
> > all touchpad, touchscreen and joystick drivers at once, thank you vey
> > much ;) Besides, for the vast majority of uses 0 is the proper initial
> > value for absolute axis.
> 
> Heh.
> 
> > > The only other event that needs initialization are the switches, so I'd
> > > argue that we might as well use my proposed patch, which is specific and
> > > more lightweight, and convert the in-tree drivers to it.  A few might be
> > > missing the before-registering input_event...
> > 
> > As I think about it even more I think we should leave it asd is. You
> 
> It needs to be documented somewhere, at least.
> 

Something like the patch below?

> > > BTW, looking at input.h, wouldn't it be better to force the init functions
> > > to be run before registering the input device, doing a BUG_ON() if they're
> > > misused?
> > 
> > What function are you referring to? Input devices are in useable state
> 
> The ones proposed in this subthread, which shouldn't be used after
> registering.
> 
> > as returned by input_allocate_device().
> 
> Unless, of course, someone screws up verifying if input_alocate_device
> returned NULL...
> 

Then it will crash way before registering, as soon as they try to set
name, phys and so on.. Just not worh it.

-- 
Dmitry

Input: document use of input_event() function

From: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Signed-off-by: Dmitry Torokhov <dtor-JGs/UdohzUI@public.gmane.org>
---

 drivers/input/input.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)


diff --git a/drivers/input/input.c b/drivers/input/input.c
index e097176..6240c7a 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -302,9 +302,15 @@ static void input_handle_event(struct input_dev *dev,
  * @value: value of the event
  *
  * This function should be used by drivers implementing various input
- * devices. See also input_inject_event().
+ * devices to report input events. See also input_inject_event().
+ *
+ * NOTE: input_event() may be safely used right after input device was
+ * allocated with input_allocate_device(), even before it is registered
+ * with input_register_device(), but the event will not reach any of the
+ * input handlers. Such early invocation of input_event() may be used
+ * to 'seed' initial state of a switch or initial position of absolute
+ * axis, etc.
  */
-
 void input_event(struct input_dev *dev,
 		 unsigned int type, unsigned int code, int value)
 {

------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev

Re: [ibm-acpi-devel] [PATCH 10/10] thinkpad-acpi: sync input device EV_SW state directly

[Henrique de Moraes Holschuh]

On Thu, 10 Dec 2009, Dmitry Torokhov wrote:
> > > > The only other event that needs initialization are the switches, so I'd
> > > > argue that we might as well use my proposed patch, which is specific and
> > > > more lightweight, and convert the in-tree drivers to it.  A few might be
> > > > missing the before-registering input_event...
> > > 
> > > As I think about it even more I think we should leave it asd is. You
> > 
> > It needs to be documented somewhere, at least.
> 
> Something like the patch below?

ACK.

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