[GIT PATCH v3] thinkpad-acpi changes for the merge window (part 1)

[GIT PATCH v3] thinkpad-acpi changes for the merge window (part 1)

[Henrique de Moraes Holschuh]

Len,

Here is the third version of the first batch of changes for thinkpad-acpi,
targetted at the next merge window.   They're mostly non-critical fixes.

I have found a stupid bug on v2 of patch 6, and fixed it.  Sorry about
that.  I have also added two new patches, that were going to be sent as
part 2, but I might as well just send them in right now.

Patches 8 and 9 *fix* a big ABI problem with the way thinkpad-acpi was
exporting the hwmon attributes, which precluded it from working with
lm-sensors(!).  Since I am already touching the ABI, it also prepares
the thinkpad-acpi sensors ABI for a module split (so I can shunt all the
hwmon code to a new module later, and userspace won't even notice).

Please merge this batch to acpi-test.  It replaces previous versions of
"thinkpad-acpi changes for the merge window (part 1)".

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 (9):
      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.1)
      ACPI: thinkpad-acpi: fix regression on HKEY LID event handling
      ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it
      ACPI: thinkpad-acpi: duplicate driver attributes to new hwmon pdrv

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/9] ACPI: thinkpad-acpi: make room for more features in tp_features bitfield

[Henrique de Moraes Holschuh]

Increase tp_features to 32 bits.  It is too close to running out of room.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/misc/thinkpad_acpi.h |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
index 082a1cb..fa64ded 100644
--- a/drivers/misc/thinkpad_acpi.h
+++ b/drivers/misc/thinkpad_acpi.h
@@ -233,22 +233,22 @@ struct ibm_init_struct {
 
 static struct {
 #ifdef CONFIG_THINKPAD_ACPI_BAY
-	u16 bay_status:1;
-	u16 bay_eject:1;
-	u16 bay_status2:1;
-	u16 bay_eject2:1;
+	u32 bay_status:1;
+	u32 bay_eject:1;
+	u32 bay_status2:1;
+	u32 bay_eject2:1;
 #endif
-	u16 bluetooth:1;
-	u16 hotkey:1;
-	u16 hotkey_mask:1;
-	u16 hotkey_wlsw:1;
-	u16 light:1;
-	u16 light_status:1;
-	u16 wan:1;
-	u16 fan_ctrl_status_undef:1;
-	u16 input_device_registered:1;
-	u16 platform_drv_registered:1;
-	u16 platform_drv_attrs_registered:1;
+	u32 bluetooth:1;
+	u32 hotkey:1;
+	u32 hotkey_mask:1;
+	u32 hotkey_wlsw:1;
+	u32 light:1;
+	u32 light_status:1;
+	u32 wan:1;
+	u32 fan_ctrl_status_undef:1;
+	u32 input_device_registered:1;
+	u32 platform_drv_registered:1;
+	u32 platform_drv_attrs_registered:1;
 } tp_features;
 
 struct thinkpad_id_data {
-- 
1.5.3.1

[PATCH 2/9] ACPI: thinkpad-acpi: issue EV_SYNC after EV_SWITCH

[Henrique de Moraes Holschuh]

We were missing a input_sync on the radio switch event report path. Add it.

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

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 0222bba..8aa0f96 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -1149,9 +1149,11 @@ static void tpacpi_input_send_radiosw(void)
 {
 	int wlsw;
 
-	if (tp_features.hotkey_wlsw && !hotkey_get_wlsw(&wlsw))
+	if (tp_features.hotkey_wlsw && !hotkey_get_wlsw(&wlsw)) {
 		input_report_switch(tpacpi_inputdev,
 				    SW_RADIO, !!wlsw);
+		input_sync(tpacpi_inputdev);
+	}
 }
 
 static void hotkey_notify(struct ibm_struct *ibm, u32 event)
-- 
1.5.3.1

[PATCH 3/9] ACPI: thinkpad-acpi: add mutex-based locking to input device event send path

[Henrique de Moraes Holschuh]

Protect the input device event sending path with a mutex, since hot key
input events are not atomic and require an cohesive event block to be sent
together.

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

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 8aa0f96..0ced9d6 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -519,6 +519,7 @@ static char *next_cmd(char **cmds)
 static struct platform_device *tpacpi_pdev;
 static struct class_device *tpacpi_hwmon;
 static struct input_dev *tpacpi_inputdev;
+static struct mutex tpacpi_inputdev_send_mutex;
 
 
 static int tpacpi_resume_handler(struct platform_device *pdev)
@@ -1131,6 +1132,8 @@ static void tpacpi_input_send_key(unsigned int scancode,
 				  unsigned int keycode)
 {
 	if (keycode != KEY_RESERVED) {
+		mutex_lock(&tpacpi_inputdev_send_mutex);
+
 		input_report_key(tpacpi_inputdev, keycode, 1);
 		if (keycode == KEY_UNKNOWN)
 			input_event(tpacpi_inputdev, EV_MSC, MSC_SCAN,
@@ -1142,6 +1145,8 @@ static void tpacpi_input_send_key(unsigned int scancode,
 			input_event(tpacpi_inputdev, EV_MSC, MSC_SCAN,
 				    scancode);
 		input_sync(tpacpi_inputdev);
+
+		mutex_unlock(&tpacpi_inputdev_send_mutex);
 	}
 }
 
@@ -1149,11 +1154,15 @@ static void tpacpi_input_send_radiosw(void)
 {
 	int wlsw;
 
+	mutex_lock(&tpacpi_inputdev_send_mutex);
+
 	if (tp_features.hotkey_wlsw && !hotkey_get_wlsw(&wlsw)) {
 		input_report_switch(tpacpi_inputdev,
 				    SW_RADIO, !!wlsw);
 		input_sync(tpacpi_inputdev);
 	}
+
+	mutex_unlock(&tpacpi_inputdev_send_mutex);
 }
 
 static void hotkey_notify(struct ibm_struct *ibm, u32 event)
@@ -4737,6 +4746,7 @@ static int __init thinkpad_acpi_module_init(void)
 		thinkpad_acpi_module_exit();
 		return ret;
 	}
+	mutex_init(&tpacpi_inputdev_send_mutex);
 	tpacpi_inputdev = input_allocate_device();
 	if (!tpacpi_inputdev) {
 		printk(IBM_ERR "unable to allocate input device\n");
-- 
1.5.3.1


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH 4/9] ACPI: thinkpad-acpi: keep track of module state

[Henrique de Moraes Holschuh]

Keep track of module state (init, running, exit).  This makes it trivially
easy to avoid running any interrupt handlers, threads, or any other async
activity before we are ready, or when we want to go away.

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

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 0ced9d6..2155139 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -117,6 +117,12 @@ IBM_BIOS_MODULE_ALIAS("K[U,X-Z]");
 
 #define __unused __attribute__ ((unused))
 
+static enum {
+	TPACPI_LIFE_INIT = 0,
+	TPACPI_LIFE_RUNNING,
+	TPACPI_LIFE_EXITING,
+} tpacpi_lifecycle;
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -342,6 +348,9 @@ static void dispatch_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ibm_struct *ibm = data;
 
+	if (tpacpi_lifecycle != TPACPI_LIFE_RUNNING)
+		return;
+
 	if (!ibm || !ibm->acpi || !ibm->acpi->notify)
 		return;
 
@@ -3899,6 +3908,9 @@ static void fan_watchdog_fire(struct work_struct *ignored)
 {
 	int rc;
 
+	if (tpacpi_lifecycle != TPACPI_LIFE_RUNNING)
+		return;
+
 	printk(IBM_NOTICE "fan watchdog: enabling fan\n");
 	rc = fan_set_enable();
 	if (rc < 0) {
@@ -3919,7 +3931,8 @@ static void fan_watchdog_reset(void)
 	if (fan_watchdog_active)
 		cancel_delayed_work(&fan_watchdog_task);
 
-	if (fan_watchdog_maxinterval > 0) {
+	if (fan_watchdog_maxinterval > 0 &&
+	    tpacpi_lifecycle != TPACPI_LIFE_EXITING) {
 		fan_watchdog_active = 1;
 		if (!schedule_delayed_work(&fan_watchdog_task,
 				msecs_to_jiffies(fan_watchdog_maxinterval
@@ -4685,6 +4698,8 @@ static int __init thinkpad_acpi_module_init(void)
 {
 	int ret, i;
 
+	tpacpi_lifecycle = TPACPI_LIFE_INIT;
+
 	/* Parameter checking */
 	if (hotkey_report_mode > 2)
 		return -EINVAL;
@@ -4781,6 +4796,7 @@ static int __init thinkpad_acpi_module_init(void)
 		tp_features.input_device_registered = 1;
 	}
 
+	tpacpi_lifecycle = TPACPI_LIFE_RUNNING;
 	return 0;
 }
 
@@ -4788,6 +4804,8 @@ static void thinkpad_acpi_module_exit(void)
 {
 	struct ibm_struct *ibm, *itmp;
 
+	tpacpi_lifecycle = TPACPI_LIFE_EXITING;
+
 	list_for_each_entry_safe_reverse(ibm, itmp,
 					 &tpacpi_all_drivers,
 					 all_drivers) {
-- 
1.5.3.1

[PATCH 5/9] ACPI: thinkpad-acpi: check version of hot key firmware

[Henrique de Moraes Holschuh]

Check the HKEY firmware version (HKEY.MHKV handler), and refuse to load if
it is unknown.  Use this instead of the presence of HKEY.DHKV to detect hot
key mask capability.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/misc/thinkpad_acpi.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 2155139..9a61140 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -999,6 +999,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
 	int res, i;
 	int status;
+	int hkeyv;
 
 	vdbg_printk(TPACPI_DBG_INIT, "initializing hotkey subdriver\n");
 
@@ -1024,18 +1025,35 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 			return res;
 
 		/* mask not supported on 570, 600e/x, 770e, 770x, A21e, A2xm/p,
-		   A30, R30, R31, T20-22, X20-21, X22-24 */
-		tp_features.hotkey_mask =
-			acpi_evalf(hkey_handle, NULL, "DHKN", "qv");
+		   A30, R30, R31, T20-22, X20-21, X22-24.  Detected by checking
+		   for HKEY interface version 0x100 */
+		if (acpi_evalf(hkey_handle, &hkeyv, "MHKV", "qd")) {
+			if ((hkeyv >> 8) != 1) {
+				printk(IBM_ERR "unknown version of the "
+				       "HKEY interface: 0x%x\n", hkeyv);
+				printk(IBM_ERR "please report this to %s\n",
+				       IBM_MAIL);
+			} else {
+				/*
+				 * MHKV 0x100 in A31, R40, R40e,
+				 * T4x, X31, and later
+				 * */
+				tp_features.hotkey_mask = 1;
+			}
+		}
 
 		vdbg_printk(TPACPI_DBG_INIT, "hotkey masks are %s\n",
 			str_supported(tp_features.hotkey_mask));
 
 		if (tp_features.hotkey_mask) {
-			/* MHKA available in A31, R40, R40e, T4x, X31, and later */
 			if (!acpi_evalf(hkey_handle, &hotkey_all_mask,
-					"MHKA", "qd"))
+					"MHKA", "qd")) {
+				printk(IBM_ERR
+				       "missing MHKA handler, "
+				       "please report this to %s\n",
+				       IBM_MAIL);
 				hotkey_all_mask = 0x080cU; /* FN+F12, FN+F4, FN+F3 */
+			}
 		}
 
 		res = hotkey_get(&hotkey_orig_status, &hotkey_orig_mask);
-- 
1.5.3.1

[PATCH 6/9] ACPI: thinkpad-acpi: dequeue all pending hot key events at once (v2.1)

[Henrique de Moraes Holschuh]

Receive all pending HKEY events at once from a single notification, and don't
complain if the queue is empty.

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

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 9a61140..ab2ab6b 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -1196,9 +1196,30 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 {
 	u32 hkey;
 	unsigned int keycode, scancode;
-	int send_acpi_ev = 0;
+	int send_acpi_ev;
+
+	if (event != 0x80) {
+		printk(IBM_ERR "unknown HKEY notification event %d\n", event);
+		/* forward it to userspace, maybe it knows how to handle it */
+		acpi_bus_generate_netlink_event(ibm->acpi->device->pnp.device_class,
+						ibm->acpi->device->dev.bus_id,
+						event, hkey);
+		return;
+	}
+
+	while (1) {
+		if (!acpi_evalf(hkey_handle, &hkey, "MHKP", "d")) {
+			printk(IBM_ERR "failed to retrieve HKEY event\n");
+			return;
+		}
+
+		if (hkey == 0) {
+			/* queue empty */
+			return;
+		}
+
+		send_acpi_ev = 0;
 
-	if (event == 0x80 && acpi_evalf(hkey_handle, &hkey, "MHKP", "d")) {
 		switch (hkey >> 12) {
 		case 1:
 			/* 0x1000-0x1FFF: key presses */
@@ -1220,8 +1241,8 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 			 * eat up known LID events */
 			if (hkey != 0x5001 && hkey != 0x5002) {
 				printk(IBM_ERR
-					"unknown LID-related hotkey event: 0x%04x\n",
-					hkey);
+				       "unknown LID-related HKEY event: 0x%04x\n",
+				       hkey);
 				send_acpi_ev = 1;
 			}
 			break;
@@ -1240,21 +1261,17 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 			printk(IBM_NOTICE "unhandled HKEY event 0x%04x\n", hkey);
 			send_acpi_ev = 1;
 		}
-	} else {
-		printk(IBM_ERR "unknown hotkey notification event %d\n", event);
-		hkey = 0;
-		send_acpi_ev = 1;
-	}
 
-	/* Legacy events */
-	if (send_acpi_ev || hotkey_report_mode < 2)
-		acpi_bus_generate_proc_event(ibm->acpi->device, event, hkey);
+		/* Legacy events */
+		if (send_acpi_ev || hotkey_report_mode < 2)
+			acpi_bus_generate_proc_event(ibm->acpi->device, event, hkey);
 
-	/* netlink events */
-	if (send_acpi_ev) {
-		acpi_bus_generate_netlink_event(ibm->acpi->device->pnp.device_class,
-						ibm->acpi->device->dev.bus_id,
-						event, hkey);
+		/* netlink events */
+		if (send_acpi_ev) {
+			acpi_bus_generate_netlink_event(ibm->acpi->device->pnp.device_class,
+							ibm->acpi->device->dev.bus_id,
+							event, hkey);
+		}
 	}
 }
 
-- 
1.5.3.1


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH 7/9] ACPI: thinkpad-acpi: fix regression on HKEY LID event handling

[Henrique de Moraes Holschuh]

We were letting ThinkPad-specific LID events through to userspace again,
instead of dropping them.  Fix it.  We don't want to give userspace the
option of not using generic LID handling.

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

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index ab2ab6b..8bd6c83 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -1197,6 +1197,7 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 	u32 hkey;
 	unsigned int keycode, scancode;
 	int send_acpi_ev;
+	int ignore_acpi_ev;
 
 	if (event != 0x80) {
 		printk(IBM_ERR "unknown HKEY notification event %d\n", event);
@@ -1219,6 +1220,7 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 		}
 
 		send_acpi_ev = 0;
+		ignore_acpi_ev = 0;
 
 		switch (hkey >> 12) {
 		case 1:
@@ -1244,6 +1246,8 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 				       "unknown LID-related HKEY event: 0x%04x\n",
 				       hkey);
 				send_acpi_ev = 1;
+			} else {
+				ignore_acpi_ev = 1;
 			}
 			break;
 		case 7:
@@ -1263,11 +1267,12 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 		}
 
 		/* Legacy events */
-		if (send_acpi_ev || hotkey_report_mode < 2)
+		if (!ignore_acpi_ev && (send_acpi_ev || hotkey_report_mode < 2)) {
 			acpi_bus_generate_proc_event(ibm->acpi->device, event, hkey);
+		}
 
 		/* netlink events */
-		if (send_acpi_ev) {
+		if (!ignore_acpi_ev && send_acpi_ev) {
 			acpi_bus_generate_netlink_event(ibm->acpi->device->pnp.device_class,
 							ibm->acpi->device->dev.bus_id,
 							event, hkey);
-- 
1.5.3.1

[PATCH 8/9] ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it

[Henrique de Moraes Holschuh]

Use a separate platform device to attach hwmon attributes and class, and
add a name attribute of "thinkpad_hwmon" to it.  To do it properly, we
also register a new platform driver ("thinkpad_hwmon").

This makes thinkpad-acpi compatible with libsensors4 from lm-sensors, and
the platform driver and device split will make it much easier to separate
hwmon functionality into its own module later on.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: LM Sensors ML <lm-sensors@lm-sensors.org>
---
 Documentation/thinkpad-acpi.txt |   24 +++++++++---
 drivers/misc/thinkpad_acpi.c    |   79 +++++++++++++++++++++++++++++++++------
 drivers/misc/thinkpad_acpi.h    |    6 ++-
 3 files changed, 90 insertions(+), 19 deletions(-)

diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
index 60953d6..029bb43 100644
--- a/Documentation/thinkpad-acpi.txt
+++ b/Documentation/thinkpad-acpi.txt
@@ -105,10 +105,15 @@ The version of thinkpad-acpi's sysfs interface is exported by the driver
 as a driver attribute (see below).
 
 Sysfs driver attributes are on the driver's sysfs attribute space,
-for 2.6.20 this is /sys/bus/platform/drivers/thinkpad_acpi/.
+for 2.6.20 this is /sys/bus/platform/drivers/thinkpad_acpi/ and
+/sys/bus/platform/drivers/thinkpad_hwmon/
 
-Sysfs device attributes are on the driver's sysfs attribute space,
-for 2.6.20 this is /sys/devices/platform/thinkpad_acpi/.
+Sysfs device attributes are on the thinkpad_acpi device sysfs attribute
+space, for 2.6.20 this is /sys/devices/platform/thinkpad_acpi/.
+
+Sysfs device attributes for the sensors and fan are on the
+thinkpad_hwmon device's sysfs attribute space, but you should locate it
+looking for a hwmon device with the name attribute of "thinkpad_hwmon".
 
 Driver version
 --------------
@@ -766,7 +771,7 @@ Temperature sensors
 -------------------
 
 procfs: /proc/acpi/ibm/thermal
-sysfs device attributes: (hwmon) temp*_input
+sysfs device attributes: (hwmon "thinkpad_hwmon") temp*_input
 
 Most ThinkPads include six or more separate temperature sensors but only
 expose the CPU temperature through the standard ACPI methods.  This
@@ -989,7 +994,9 @@ Fan control and monitoring: fan speed, fan enable/disable
 ---------------------------------------------------------
 
 procfs: /proc/acpi/ibm/fan
-sysfs device attributes: (hwmon) fan_input, pwm1, pwm1_enable
+sysfs device attributes: (hwmon "thinkpad_hwmon") fan_input, pwm1,
+			  pwm1_enable
+sysfs hwmon driver attributes: fan_watchdog
 
 NOTE NOTE NOTE: fan control operations are disabled by default for
 safety reasons.  To enable them, the module parameter "fan_control=1"
@@ -1131,7 +1138,7 @@ hwmon device attribute fan1_input:
 	which can take up to two minutes.  May return rubbish on older
 	ThinkPads.
 
-driver attribute fan_watchdog:
+hwmon driver attribute fan_watchdog:
 	Fan safety watchdog timer interval, in seconds.  Minimum is
 	1 second, maximum is 120 seconds.  0 disables the watchdog.
 
@@ -1233,3 +1240,8 @@ Sysfs interface changelog:
 		layer, the radio switch generates input event EV_RADIO,
 		and the driver enables hot key handling by default in
 		the firmware.
+
+0x020000:	ABI fix: added a separate hwmon platform device and
+		driver, which must be located by name (thinkpad_hwmon)
+		and the hwmon class for lm_sensors4 compatibility.
+		Moved all hwmon attributes to this new platform device.
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 8bd6c83..4a85ede 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -22,7 +22,7 @@
  */
 
 #define IBM_VERSION "0.16"
-#define TPACPI_SYSFS_VERSION 0x010000
+#define TPACPI_SYSFS_VERSION 0x020000
 
 /*
  *  Changelog:
@@ -526,6 +526,7 @@ static char *next_cmd(char **cmds)
  ****************************************************************************/
 
 static struct platform_device *tpacpi_pdev;
+static struct platform_device *tpacpi_sensors_pdev;
 static struct class_device *tpacpi_hwmon;
 static struct input_dev *tpacpi_inputdev;
 static struct mutex tpacpi_inputdev_send_mutex;
@@ -553,6 +554,12 @@ static struct platform_driver tpacpi_pdriver = {
 	.resume = tpacpi_resume_handler,
 };
 
+static struct platform_driver tpacpi_hwmon_pdriver = {
+	.driver = {
+		.name = IBM_HWMON_DRVR_NAME,
+		.owner = THIS_MODULE,
+	},
+};
 
 /*************************************************************************
  * thinkpad-acpi driver attributes
@@ -2872,7 +2879,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
 
 	switch(thermal_read_mode) {
 	case TPACPI_THERMAL_TPEC_16:
-		res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+		res = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
 				&thermal_temp_input16_group);
 		if (res)
 			return res;
@@ -2880,7 +2887,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
 	case TPACPI_THERMAL_TPEC_8:
 	case TPACPI_THERMAL_ACPI_TMP07:
 	case TPACPI_THERMAL_ACPI_UPDT:
-		res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+		res = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
 				&thermal_temp_input8_group);
 		if (res)
 			return res;
@@ -2897,13 +2904,13 @@ static void thermal_exit(void)
 {
 	switch(thermal_read_mode) {
 	case TPACPI_THERMAL_TPEC_16:
-		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+		sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
 				   &thermal_temp_input16_group);
 		break;
 	case TPACPI_THERMAL_TPEC_8:
 	case TPACPI_THERMAL_ACPI_TMP07:
 	case TPACPI_THERMAL_ACPI_UPDT:
-		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+		sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
 				   &thermal_temp_input16_group);
 		break;
 	case TPACPI_THERMAL_NONE:
@@ -3686,7 +3693,7 @@ static struct device_attribute dev_attr_fan_fan1_input =
 	__ATTR(fan1_input, S_IRUGO,
 		fan_fan1_input_show, NULL);
 
-/* sysfs fan fan_watchdog (driver) ------------------------------------- */
+/* sysfs fan fan_watchdog (hwmon driver) ------------------------------- */
 static ssize_t fan_fan_watchdog_show(struct device_driver *drv,
 				     char *buf)
 {
@@ -3828,10 +3835,10 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 
 	if (fan_status_access_mode != TPACPI_FAN_NONE ||
 	    fan_control_access_mode != TPACPI_FAN_WR_NONE) {
-		rc = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+		rc = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
 					 &fan_attr_group);
 		if (!(rc < 0))
-			rc = driver_create_file(&tpacpi_pdriver.driver,
+			rc = driver_create_file(&tpacpi_hwmon_pdriver.driver,
 					&driver_attr_fan_watchdog);
 		if (rc < 0)
 			return rc;
@@ -3914,8 +3921,8 @@ static void fan_exit(void)
 	vdbg_printk(TPACPI_DBG_EXIT, "cancelling any pending fan watchdog tasks\n");
 
 	/* FIXME: can we really do this unconditionally? */
-	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &fan_attr_group);
-	driver_remove_file(&tpacpi_pdriver.driver, &driver_attr_fan_watchdog);
+	sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj, &fan_attr_group);
+	driver_remove_file(&tpacpi_hwmon_pdriver.driver, &driver_attr_fan_watchdog);
 
 	cancel_delayed_work(&fan_watchdog_task);
 	flush_scheduled_work();
@@ -4366,6 +4373,19 @@ static struct ibm_struct fan_driver_data = {
  ****************************************************************************
  ****************************************************************************/
 
+/* sysfs name ---------------------------------------------------------- */
+static ssize_t thinkpad_acpi_pdev_name_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n", IBM_HWMON_DRVR_NAME);
+}
+
+static struct device_attribute dev_attr_thinkpad_acpi_pdev_name =
+	__ATTR(name, S_IRUGO, thinkpad_acpi_pdev_name_show, NULL);
+
+/* --------------------------------------------------------------------- */
+
 /* /proc support */
 static struct proc_dir_entry *proc_dir;
 
@@ -4768,12 +4788,20 @@ static int __init thinkpad_acpi_module_init(void)
 
 	ret = platform_driver_register(&tpacpi_pdriver);
 	if (ret) {
-		printk(IBM_ERR "unable to register platform driver\n");
+		printk(IBM_ERR "unable to register main platform driver\n");
 		thinkpad_acpi_module_exit();
 		return ret;
 	}
 	tp_features.platform_drv_registered = 1;
 
+	ret = platform_driver_register(&tpacpi_hwmon_pdriver);
+	if (ret) {
+		printk(IBM_ERR "unable to register hwmon platform driver\n");
+		thinkpad_acpi_module_exit();
+		return ret;
+	}
+	tp_features.sensors_pdrv_registered = 1;
+
 	ret = tpacpi_create_driver_attributes(&tpacpi_pdriver.driver);
 	if (ret) {
 		printk(IBM_ERR "unable to create sysfs driver attributes\n");
@@ -4793,7 +4821,26 @@ static int __init thinkpad_acpi_module_init(void)
 		thinkpad_acpi_module_exit();
 		return ret;
 	}
-	tpacpi_hwmon = hwmon_device_register(&tpacpi_pdev->dev);
+	tpacpi_sensors_pdev = platform_device_register_simple(
+							IBM_HWMON_DRVR_NAME,
+							-1, NULL, 0);
+	if (IS_ERR(tpacpi_sensors_pdev)) {
+		ret = PTR_ERR(tpacpi_sensors_pdev);
+		tpacpi_sensors_pdev = NULL;
+		printk(IBM_ERR "unable to register hwmon platform device\n");
+		thinkpad_acpi_module_exit();
+		return ret;
+	}
+	ret = device_create_file(&tpacpi_sensors_pdev->dev,
+				 &dev_attr_thinkpad_acpi_pdev_name);
+	if (ret) {
+		printk(IBM_ERR
+			"unable to create sysfs hwmon device attributes\n");
+		thinkpad_acpi_module_exit();
+		return ret;
+	}
+	tp_features.sensors_pdev_attrs_registered = 1;
+	tpacpi_hwmon = hwmon_device_register(&tpacpi_sensors_pdev->dev);
 	if (IS_ERR(tpacpi_hwmon)) {
 		ret = PTR_ERR(tpacpi_hwmon);
 		tpacpi_hwmon = NULL;
@@ -4864,12 +4911,20 @@ static void thinkpad_acpi_module_exit(void)
 	if (tpacpi_hwmon)
 		hwmon_device_unregister(tpacpi_hwmon);
 
+	if (tp_features.sensors_pdev_attrs_registered)
+		device_remove_file(&tpacpi_sensors_pdev->dev,
+				   &dev_attr_thinkpad_acpi_pdev_name);
+	if (tpacpi_sensors_pdev)
+		platform_device_unregister(tpacpi_sensors_pdev);
 	if (tpacpi_pdev)
 		platform_device_unregister(tpacpi_pdev);
 
 	if (tp_features.platform_drv_attrs_registered)
 		tpacpi_remove_driver_attributes(&tpacpi_pdriver.driver);
 
+	if (tp_features.sensors_pdrv_registered)
+		platform_driver_unregister(&tpacpi_hwmon_pdriver);
+
 	if (tp_features.platform_drv_registered)
 		platform_driver_unregister(&tpacpi_pdriver);
 
diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
index fa64ded..791b8ca 100644
--- a/drivers/misc/thinkpad_acpi.h
+++ b/drivers/misc/thinkpad_acpi.h
@@ -58,13 +58,14 @@
 
 #define IBM_NAME "thinkpad"
 #define IBM_DESC "ThinkPad ACPI Extras"
-#define IBM_FILE "thinkpad_acpi"
+#define IBM_FILE IBM_NAME "_acpi"
 #define IBM_URL "http://ibm-acpi.sf.net/"
 #define IBM_MAIL "ibm-acpi-devel@lists.sourceforge.net"
 
 #define IBM_PROC_DIR "ibm"
 #define IBM_ACPI_EVENT_PREFIX "ibm"
 #define IBM_DRVR_NAME IBM_FILE
+#define IBM_HWMON_DRVR_NAME IBM_NAME "_hwmon"
 
 #define IBM_LOG IBM_FILE ": "
 #define IBM_ERR	   KERN_ERR    IBM_LOG
@@ -171,6 +172,7 @@ static int parse_strtoul(const char *buf, unsigned long max,
 
 /* Device model */
 static struct platform_device *tpacpi_pdev;
+static struct platform_device *tpacpi_sensors_pdev;
 static struct class_device *tpacpi_hwmon;
 static struct platform_driver tpacpi_pdriver;
 static struct input_dev *tpacpi_inputdev;
@@ -249,6 +251,8 @@ static struct {
 	u32 input_device_registered:1;
 	u32 platform_drv_registered:1;
 	u32 platform_drv_attrs_registered:1;
+	u32 sensors_pdrv_registered:1;
+	u32 sensors_pdev_attrs_registered:1;
 } tp_features;
 
 struct thinkpad_id_data {
-- 
1.5.3.1

[PATCH 9/9] ACPI: thinkpad-acpi: duplicate driver attributes to new hwmon pdrv

[Henrique de Moraes Holschuh]

Thinkpad-acpi has some driver attributes (debug level, sysfs interface
version, etc) that also belong to the new hwmon driver.  Duplicate them
there.

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

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 4a85ede..9f8db98 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -4803,12 +4803,16 @@ static int __init thinkpad_acpi_module_init(void)
 	tp_features.sensors_pdrv_registered = 1;
 
 	ret = tpacpi_create_driver_attributes(&tpacpi_pdriver.driver);
+	if (!ret) {
+		tp_features.platform_drv_attrs_registered = 1;
+		ret = tpacpi_create_driver_attributes(&tpacpi_hwmon_pdriver.driver);
+	}
 	if (ret) {
 		printk(IBM_ERR "unable to create sysfs driver attributes\n");
 		thinkpad_acpi_module_exit();
 		return ret;
 	}
-	tp_features.platform_drv_attrs_registered = 1;
+	tp_features.sensors_pdrv_attrs_registered = 1;
 
 
 	/* Device initialization */
@@ -4919,6 +4923,8 @@ static void thinkpad_acpi_module_exit(void)
 	if (tpacpi_pdev)
 		platform_device_unregister(tpacpi_pdev);
 
+	if (tp_features.sensors_pdrv_attrs_registered)
+		tpacpi_remove_driver_attributes(&tpacpi_hwmon_pdriver.driver);
 	if (tp_features.platform_drv_attrs_registered)
 		tpacpi_remove_driver_attributes(&tpacpi_pdriver.driver);
 
diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
index 791b8ca..c5fdd68 100644
--- a/drivers/misc/thinkpad_acpi.h
+++ b/drivers/misc/thinkpad_acpi.h
@@ -252,6 +252,7 @@ static struct {
 	u32 platform_drv_registered:1;
 	u32 platform_drv_attrs_registered:1;
 	u32 sensors_pdrv_registered:1;
+	u32 sensors_pdrv_attrs_registered:1;
 	u32 sensors_pdev_attrs_registered:1;
 } tp_features;
 
-- 
1.5.3.1


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [GIT PATCH v3] thinkpad-acpi changes for the merge window (part 1)

[Henrique de Moraes Holschuh]

On Sun, 23 Sep 2007, Henrique de Moraes Holschuh wrote:
> I have found a stupid bug on v2 of patch 6, and fixed it.  Sorry about

Today must not be my day.  There is still an error in patch 6, I will send
the batch again in a few moments.  I am *really* sorry about 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

Re: [lm-sensors] [PATCH 8/9] ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it

[Jean Delvare]

Hi Henrique,

On Sun, 23 Sep 2007 11:24:16 -0300, Henrique de Moraes Holschuh wrote:
> Use a separate platform device to attach hwmon attributes and class, and
> add a name attribute of "thinkpad_hwmon" to it.  To do it properly, we
> also register a new platform driver ("thinkpad_hwmon").

The name attribute should really read "thinkpad" not "thinkpad_hwmon",
otherwise "sensors" will present the chip as "thinkpad_hwmon-isa-0000",
its section in /etc/sensors.conf will be named chip "thinkpad_hwmon-*",
etc. That's pretty redundant and unaesthetic.

> This makes thinkpad-acpi compatible with libsensors4 from lm-sensors, and
> the platform driver and device split will make it much easier to separate
> hwmon functionality into its own module later on.

I'm still not convinced that we want a separate device for this. If you
have a namespace issue, it's because the hwmon attributes are created
directly in the device directory (for historical reasons) while they
should be created in the hwmon class device directory. I'd rather work
on fixing this now, than use a temporary workaround as you are
proposing.

> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
> Cc: LM Sensors ML <lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org>

It's pretty pointless to have a mailing list listed here, as the list
itself can't ack nor nack your patch.

> ---
>  Documentation/thinkpad-acpi.txt |   24 +++++++++---
>  drivers/misc/thinkpad_acpi.c    |   79 +++++++++++++++++++++++++++++++++------
>  drivers/misc/thinkpad_acpi.h    |    6 ++-
>  3 files changed, 90 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
> index 60953d6..029bb43 100644
> --- a/Documentation/thinkpad-acpi.txt
> +++ b/Documentation/thinkpad-acpi.txt
> @@ -105,10 +105,15 @@ The version of thinkpad-acpi's sysfs interface is exported by the driver
>  as a driver attribute (see below).
>  
>  Sysfs driver attributes are on the driver's sysfs attribute space,
> -for 2.6.20 this is /sys/bus/platform/drivers/thinkpad_acpi/.
> +for 2.6.20 this is /sys/bus/platform/drivers/thinkpad_acpi/ and
> +/sys/bus/platform/drivers/thinkpad_hwmon/
>  
> -Sysfs device attributes are on the driver's sysfs attribute space,
> -for 2.6.20 this is /sys/devices/platform/thinkpad_acpi/.
> +Sysfs device attributes are on the thinkpad_acpi device sysfs attribute
> +space, for 2.6.20 this is /sys/devices/platform/thinkpad_acpi/.

That's confusing. You keep mentioning version 2.6.20 when your patch
will not make it into mainline before 2.6.24 at best.

> +
> +Sysfs device attributes for the sensors and fan are on the
> +thinkpad_hwmon device's sysfs attribute space, but you should locate it
> +looking for a hwmon device with the name attribute of "thinkpad_hwmon".
>  
>  Driver version
>  --------------
> @@ -766,7 +771,7 @@ Temperature sensors
>  -------------------
>  
>  procfs: /proc/acpi/ibm/thermal
> -sysfs device attributes: (hwmon) temp*_input
> +sysfs device attributes: (hwmon "thinkpad_hwmon") temp*_input
>  
>  Most ThinkPads include six or more separate temperature sensors but only
>  expose the CPU temperature through the standard ACPI methods.  This
> @@ -989,7 +994,9 @@ Fan control and monitoring: fan speed, fan enable/disable
>  ---------------------------------------------------------
>  
>  procfs: /proc/acpi/ibm/fan
> -sysfs device attributes: (hwmon) fan_input, pwm1, pwm1_enable
> +sysfs device attributes: (hwmon "thinkpad_hwmon") fan_input, pwm1,
> +			  pwm1_enable

fan_input is not a valid attribute name, should be fan1_input.

> +sysfs hwmon driver attributes: fan_watchdog
>  
>  NOTE NOTE NOTE: fan control operations are disabled by default for
>  safety reasons.  To enable them, the module parameter "fan_control=1"
> @@ -1131,7 +1138,7 @@ hwmon device attribute fan1_input:
>  	which can take up to two minutes.  May return rubbish on older
>  	ThinkPads.
>  
> -driver attribute fan_watchdog:
> +hwmon driver attribute fan_watchdog:
>  	Fan safety watchdog timer interval, in seconds.  Minimum is
>  	1 second, maximum is 120 seconds.  0 disables the watchdog.
>  
> @@ -1233,3 +1240,8 @@ Sysfs interface changelog:
>  		layer, the radio switch generates input event EV_RADIO,
>  		and the driver enables hot key handling by default in
>  		the firmware.
> +
> +0x020000:	ABI fix: added a separate hwmon platform device and
> +		driver, which must be located by name (thinkpad_hwmon)
> +		and the hwmon class for lm_sensors4 compatibility.

There's no lm_sensors4. It's either lm-sensors 3, or libsensors4. (I
know it's confusing, not my fault.)

> +		Moved all hwmon attributes to this new platform device.
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index 8bd6c83..4a85ede 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -22,7 +22,7 @@
>   */
>  
>  #define IBM_VERSION "0.16"
> -#define TPACPI_SYSFS_VERSION 0x010000
> +#define TPACPI_SYSFS_VERSION 0x020000
>  
>  /*
>   *  Changelog:
> @@ -526,6 +526,7 @@ static char *next_cmd(char **cmds)
>   ****************************************************************************/
>  
>  static struct platform_device *tpacpi_pdev;
> +static struct platform_device *tpacpi_sensors_pdev;
>  static struct class_device *tpacpi_hwmon;
>  static struct input_dev *tpacpi_inputdev;
>  static struct mutex tpacpi_inputdev_send_mutex;
> @@ -553,6 +554,12 @@ static struct platform_driver tpacpi_pdriver = {
>  	.resume = tpacpi_resume_handler,
>  };
>  
> +static struct platform_driver tpacpi_hwmon_pdriver = {
> +	.driver = {
> +		.name = IBM_HWMON_DRVR_NAME,
> +		.owner = THIS_MODULE,
> +	},
> +};
>  
>  /*************************************************************************
>   * thinkpad-acpi driver attributes
> @@ -2872,7 +2879,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>  
>  	switch(thermal_read_mode) {
>  	case TPACPI_THERMAL_TPEC_16:
> -		res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> +		res = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
>  				&thermal_temp_input16_group);
>  		if (res)
>  			return res;
> @@ -2880,7 +2887,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>  	case TPACPI_THERMAL_TPEC_8:
>  	case TPACPI_THERMAL_ACPI_TMP07:
>  	case TPACPI_THERMAL_ACPI_UPDT:
> -		res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> +		res = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
>  				&thermal_temp_input8_group);
>  		if (res)
>  			return res;
> @@ -2897,13 +2904,13 @@ static void thermal_exit(void)
>  {
>  	switch(thermal_read_mode) {
>  	case TPACPI_THERMAL_TPEC_16:
> -		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> +		sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
>  				   &thermal_temp_input16_group);
>  		break;
>  	case TPACPI_THERMAL_TPEC_8:
>  	case TPACPI_THERMAL_ACPI_TMP07:
>  	case TPACPI_THERMAL_ACPI_UPDT:
> -		sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> +		sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
>  				   &thermal_temp_input16_group);
>  		break;
>  	case TPACPI_THERMAL_NONE:
> @@ -3686,7 +3693,7 @@ static struct device_attribute dev_attr_fan_fan1_input =
>  	__ATTR(fan1_input, S_IRUGO,
>  		fan_fan1_input_show, NULL);
>  
> -/* sysfs fan fan_watchdog (driver) ------------------------------------- */
> +/* sysfs fan fan_watchdog (hwmon driver) ------------------------------- */
>  static ssize_t fan_fan_watchdog_show(struct device_driver *drv,
>  				     char *buf)
>  {
> @@ -3828,10 +3835,10 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  
>  	if (fan_status_access_mode != TPACPI_FAN_NONE ||
>  	    fan_control_access_mode != TPACPI_FAN_WR_NONE) {
> -		rc = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> +		rc = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
>  					 &fan_attr_group);
>  		if (!(rc < 0))
> -			rc = driver_create_file(&tpacpi_pdriver.driver,
> +			rc = driver_create_file(&tpacpi_hwmon_pdriver.driver,
>  					&driver_attr_fan_watchdog);
>  		if (rc < 0)
>  			return rc;
> @@ -3914,8 +3921,8 @@ static void fan_exit(void)
>  	vdbg_printk(TPACPI_DBG_EXIT, "cancelling any pending fan watchdog tasks\n");
>  
>  	/* FIXME: can we really do this unconditionally? */
> -	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &fan_attr_group);
> -	driver_remove_file(&tpacpi_pdriver.driver, &driver_attr_fan_watchdog);
> +	sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj, &fan_attr_group);
> +	driver_remove_file(&tpacpi_hwmon_pdriver.driver, &driver_attr_fan_watchdog);
>  
>  	cancel_delayed_work(&fan_watchdog_task);
>  	flush_scheduled_work();
> @@ -4366,6 +4373,19 @@ static struct ibm_struct fan_driver_data = {
>   ****************************************************************************
>   ****************************************************************************/
>  
> +/* sysfs name ---------------------------------------------------------- */
> +static ssize_t thinkpad_acpi_pdev_name_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%s\n", IBM_HWMON_DRVR_NAME);
> +}
> +
> +static struct device_attribute dev_attr_thinkpad_acpi_pdev_name =
> +	__ATTR(name, S_IRUGO, thinkpad_acpi_pdev_name_show, NULL);
> +
> +/* --------------------------------------------------------------------- */
> +
>  /* /proc support */
>  static struct proc_dir_entry *proc_dir;
>  
> @@ -4768,12 +4788,20 @@ static int __init thinkpad_acpi_module_init(void)
>  
>  	ret = platform_driver_register(&tpacpi_pdriver);
>  	if (ret) {
> -		printk(IBM_ERR "unable to register platform driver\n");
> +		printk(IBM_ERR "unable to register main platform driver\n");
>  		thinkpad_acpi_module_exit();
>  		return ret;
>  	}
>  	tp_features.platform_drv_registered = 1;
>  
> +	ret = platform_driver_register(&tpacpi_hwmon_pdriver);
> +	if (ret) {
> +		printk(IBM_ERR "unable to register hwmon platform driver\n");
> +		thinkpad_acpi_module_exit();
> +		return ret;
> +	}
> +	tp_features.sensors_pdrv_registered = 1;
> +
>  	ret = tpacpi_create_driver_attributes(&tpacpi_pdriver.driver);
>  	if (ret) {
>  		printk(IBM_ERR "unable to create sysfs driver attributes\n");
> @@ -4793,7 +4821,26 @@ static int __init thinkpad_acpi_module_init(void)
>  		thinkpad_acpi_module_exit();
>  		return ret;
>  	}
> -	tpacpi_hwmon = hwmon_device_register(&tpacpi_pdev->dev);
> +	tpacpi_sensors_pdev = platform_device_register_simple(
> +							IBM_HWMON_DRVR_NAME,
> +							-1, NULL, 0);

As explained before, libsensors won't pick this device. You'd need to
give it an id of 0 instead of -1. Hmm, I'll see if I can hack something
in libsensors4 until that part of the code is reimplemented in a better
way.

> +	if (IS_ERR(tpacpi_sensors_pdev)) {
> +		ret = PTR_ERR(tpacpi_sensors_pdev);
> +		tpacpi_sensors_pdev = NULL;
> +		printk(IBM_ERR "unable to register hwmon platform device\n");
> +		thinkpad_acpi_module_exit();
> +		return ret;
> +	}
> +	ret = device_create_file(&tpacpi_sensors_pdev->dev,
> +				 &dev_attr_thinkpad_acpi_pdev_name);
> +	if (ret) {
> +		printk(IBM_ERR
> +			"unable to create sysfs hwmon device attributes\n");
> +		thinkpad_acpi_module_exit();
> +		return ret;
> +	}
> +	tp_features.sensors_pdev_attrs_registered = 1;
> +	tpacpi_hwmon = hwmon_device_register(&tpacpi_sensors_pdev->dev);
>  	if (IS_ERR(tpacpi_hwmon)) {
>  		ret = PTR_ERR(tpacpi_hwmon);
>  		tpacpi_hwmon = NULL;
> @@ -4864,12 +4911,20 @@ static void thinkpad_acpi_module_exit(void)
>  	if (tpacpi_hwmon)
>  		hwmon_device_unregister(tpacpi_hwmon);
>  
> +	if (tp_features.sensors_pdev_attrs_registered)
> +		device_remove_file(&tpacpi_sensors_pdev->dev,
> +				   &dev_attr_thinkpad_acpi_pdev_name);
> +	if (tpacpi_sensors_pdev)
> +		platform_device_unregister(tpacpi_sensors_pdev);
>  	if (tpacpi_pdev)
>  		platform_device_unregister(tpacpi_pdev);
>  
>  	if (tp_features.platform_drv_attrs_registered)
>  		tpacpi_remove_driver_attributes(&tpacpi_pdriver.driver);
>  
> +	if (tp_features.sensors_pdrv_registered)
> +		platform_driver_unregister(&tpacpi_hwmon_pdriver);
> +
>  	if (tp_features.platform_drv_registered)
>  		platform_driver_unregister(&tpacpi_pdriver);
>  
> diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
> index fa64ded..791b8ca 100644
> --- a/drivers/misc/thinkpad_acpi.h
> +++ b/drivers/misc/thinkpad_acpi.h
> @@ -58,13 +58,14 @@
>  
>  #define IBM_NAME "thinkpad"
>  #define IBM_DESC "ThinkPad ACPI Extras"
> -#define IBM_FILE "thinkpad_acpi"
> +#define IBM_FILE IBM_NAME "_acpi"
>  #define IBM_URL "http://ibm-acpi.sf.net/"
>  #define IBM_MAIL "ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org"
>  
>  #define IBM_PROC_DIR "ibm"
>  #define IBM_ACPI_EVENT_PREFIX "ibm"
>  #define IBM_DRVR_NAME IBM_FILE
> +#define IBM_HWMON_DRVR_NAME IBM_NAME "_hwmon"
>  
>  #define IBM_LOG IBM_FILE ": "
>  #define IBM_ERR	   KERN_ERR    IBM_LOG
> @@ -171,6 +172,7 @@ static int parse_strtoul(const char *buf, unsigned long max,
>  
>  /* Device model */
>  static struct platform_device *tpacpi_pdev;
> +static struct platform_device *tpacpi_sensors_pdev;
>  static struct class_device *tpacpi_hwmon;
>  static struct platform_driver tpacpi_pdriver;
>  static struct input_dev *tpacpi_inputdev;
> @@ -249,6 +251,8 @@ static struct {
>  	u32 input_device_registered:1;
>  	u32 platform_drv_registered:1;
>  	u32 platform_drv_attrs_registered:1;
> +	u32 sensors_pdrv_registered:1;
> +	u32 sensors_pdev_attrs_registered:1;
>  } tp_features;
>  
>  struct thinkpad_id_data {


-- 
Jean Delvare

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 8/9] ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it

[Jean Delvare]

On Mon, 24 Sep 2007 17:11:28 +0200, Jean Delvare wrote:
> On Sun, 23 Sep 2007 11:24:16 -0300, Henrique de Moraes Holschuh wrote:
> > +	tpacpi_sensors_pdev = platform_device_register_simple(
> > +							IBM_HWMON_DRVR_NAME,
> > +							-1, NULL, 0);
> 
> As explained before, libsensors won't pick this device. You'd need to
> give it an id of 0 instead of -1. Hmm, I'll see if I can hack something
> in libsensors4 until that part of the code is reimplemented in a better
> way.

OK, it's fixed in libsensors4 now, the above code will work OK, no need
to change it.

-- 
Jean Delvare

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [lm-sensors] [PATCH 8/9] ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it

[Henrique de Moraes Holschuh]

On Mon, 24 Sep 2007, Jean Delvare wrote:
> On Sun, 23 Sep 2007 11:24:16 -0300, Henrique de Moraes Holschuh wrote:
> > Use a separate platform device to attach hwmon attributes and class, and
> > add a name attribute of "thinkpad_hwmon" to it.  To do it properly, we
> > also register a new platform driver ("thinkpad_hwmon").
> 
> The name attribute should really read "thinkpad" not "thinkpad_hwmon",
> otherwise "sensors" will present the chip as "thinkpad_hwmon-isa-0000",
> its section in /etc/sensors.conf will be named chip "thinkpad_hwmon-*",
> etc. That's pretty redundant and unaesthetic.

-isa- in platform drivers where there is not even a ISA bus in sight is also
very unaesthetic.

That said, I will change it to "thinkpad", but I still would rather have
thinkpad-platform from a cosmetic point of view, than thinkpad-isa-0001.

> > This makes thinkpad-acpi compatible with libsensors4 from lm-sensors, and
> > the platform driver and device split will make it much easier to separate
> > hwmon functionality into its own module later on.
> 
> I'm still not convinced that we want a separate device for this. If you

It is there for ease of breaking thinkpad-acpi into many modules.  It makes
life easier for me, and also it makes it pretty damn clear to userspace
app writers that they better use the proper ABI (libsensors4) or else.

> have a namespace issue, it's because the hwmon attributes are created
> directly in the device directory (for historical reasons) while they
> should be created in the hwmon class device directory. I'd rather work
> on fixing this now, than use a temporary workaround as you are
> proposing.

As I said, I have now another reason for the split.  The major mess of a
bunch of classes inside the device directory is annoying, but since you want
to fix that I wouldn't move the attributes to another device just for that
reason anymore.

> > Signed-off-by: Henrique de Moraes Holschuh <hmh-N3TV7GIv+o9fyO9Q7EP/yw@public.gmane.org>
> > Cc: LM Sensors ML <lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org>
> 
> It's pretty pointless to have a mailing list listed here, as the list
> itself can't ack nor nack your patch.

People in the list certainly can :-)  But if you'd rather I cc you directly,
I will change it.

> > -Sysfs device attributes are on the driver's sysfs attribute space,
> > -for 2.6.20 this is /sys/devices/platform/thinkpad_acpi/.
> > +Sysfs device attributes are on the thinkpad_acpi device sysfs attribute
> > +space, for 2.6.20 this is /sys/devices/platform/thinkpad_acpi/.
> 
> That's confusing. You keep mentioning version 2.6.20 when your patch
> will not make it into mainline before 2.6.24 at best.

I will update it.

> > -sysfs device attributes: (hwmon) fan_input, pwm1, pwm1_enable
> > +sysfs device attributes: (hwmon "thinkpad_hwmon") fan_input, pwm1,
> > +			  pwm1_enable
> 
> fan_input is not a valid attribute name, should be fan1_input.

Typo, the driver exports it as fan1_input.  Will fix, thanks for noticing
it.

> > +0x020000:	ABI fix: added a separate hwmon platform device and
> > +		driver, which must be located by name (thinkpad_hwmon)
> > +		and the hwmon class for lm_sensors4 compatibility.
> 
> There's no lm_sensors4. It's either lm-sensors 3, or libsensors4. (I
> know it's confusing, not my fault.)

Ok. Will fix.

> > -	tpacpi_hwmon = hwmon_device_register(&tpacpi_pdev->dev);
> > +	tpacpi_sensors_pdev = platform_device_register_simple(
> > +							IBM_HWMON_DRVR_NAME,
> > +							-1, NULL, 0);
> 
> As explained before, libsensors won't pick this device. You'd need to
> give it an id of 0 instead of -1. Hmm, I'll see if I can hack something
> in libsensors4 until that part of the code is reimplemented in a better
> way.

It really doesn't matter to me, as I used a new device.  I could change it
rather easily.  But since other platform drivers might not be in the same
position and it is more semanthically correct to have a device unique when
it IS unique...  IMHO it would be best to just keep it at -1, as you changed
libsensors4 to not care about it already.

-- 
  "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: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [lm-sensors] [PATCH 8/9] ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it

[Jean Delvare]

On Mon, 24 Sep 2007 23:39:58 -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 24 Sep 2007, Jean Delvare wrote:
> > The name attribute should really read "thinkpad" not "thinkpad_hwmon",
> > otherwise "sensors" will present the chip as "thinkpad_hwmon-isa-0000",
> > its section in /etc/sensors.conf will be named chip "thinkpad_hwmon-*",
> > etc. That's pretty redundant and unaesthetic.
> 
> -isa- in platform drivers where there is not even a ISA bus in sight is also
> very unaesthetic.

It's about I/O addressing space, not bus. OK, technically speaking,
recent systems' I/O space follows the LPC standard, not ISA, but for the
user it's essentially the same.

Most applications will only present the prefix of the hardware
monitoring name (i.e. the contents of the name sysfs attribute), so the
rest of the naming syntax isn't that important.

> That said, I will change it to "thinkpad", but I still would rather have
> thinkpad-platform from a cosmetic point of view, than thinkpad-isa-0001.

Great, thanks. thinkpad-platform wouldn't be a valid name for
libsensors, names have to be in the form $prefix-$bus-$address. And
again, there's no easy way for libsensors to tell between an ISA or LPC
device (which have been around for years and named *-isa-*) and a true
platform device, otherwise I would of course do it.

> > I'm still not convinced that we want a separate device for this. If you
> 
> It is there for ease of breaking thinkpad-acpi into many modules.  It makes
> life easier for me, and also it makes it pretty damn clear to userspace
> app writers that they better use the proper ABI (libsensors4) or else.

I don't understand what you mean here, sorry.

> (...)
> > > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > > Cc: LM Sensors ML <lm-sensors@lm-sensors.org>
> > 
> > It's pretty pointless to have a mailing list listed here, as the list
> > itself can't ack nor nack your patch.
> 
> People in the list certainly can :-)  But if you'd rather I cc you directly,
> I will change it.

Actually you'd rather Cc the hwmon subsystem maintainer, if anyone, and
that's not me. But I was really only commenting on the Cc: in the
header of the patch, not the Cc: when sending the patch. Cc'ing the
lm-sensors list when sending your patch was of course correct. Just
writing it down in the patch header doesn't make sense.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 8/9] ACPI: thinkpad-acpi: use a separate platform device for hwmon and name it

[Henrique de Moraes Holschuh]

On Tue, 25 Sep 2007, Jean Delvare wrote:
> > > > Cc: LM Sensors ML <lm-sensors@lm-sensors.org>
> > > 
> > > It's pretty pointless to have a mailing list listed here, as the list
> > > itself can't ack nor nack your patch.
> > 
> > People in the list certainly can :-)  But if you'd rather I cc you directly,
> > I will change it.
> 
> Actually you'd rather Cc the hwmon subsystem maintainer, if anyone, and
> that's not me. But I was really only commenting on the Cc: in the
> header of the patch, not the Cc: when sending the patch. Cc'ing the
> lm-sensors list when sending your patch was of course correct. Just
> writing it down in the patch header doesn't make sense.

That's how I track down whom I need to CC.  These patches often sit on my
queues for a while before I submit them anywhere, so I'd have to actually
place the maintainers *and* the list in the patch headers.  The patches are
sent in patch sets, and often have different CCs on individual patches.

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