[PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Shem Multinymous]

This resubmission addresses all the comments I've received on LKML,
except where said otherwise in reply. Other changes are very minor.

This patch series does three related things:

- Introduces a new driver, thinkpad_ec, for the ThinkPad embedded 
  controller (additional non-ACPI interface).
- Changes the existing hdaps driver to use thinkpad_ec instead of
  direct (and incorrect) port access.
- Several fixes and improvements to the hdaps driver.

Applies to git head.

Most of the patches in this series depend on previous ones.
Any point in this series will compile and was briefly tested (this
required introducing some code that gets overwritten by later patches).
The *combination* of all these patches was extensively tested as part
of the out-of-tree tp_smapi package [1], and verified to work on dozens
of ThinkPad models (thousands of sf.net downloads and no unresolved
bugs). Note that the tp_smapi package includes another driver, to
be submitted later, which also depends the thinkpad_ec driver
introduced here.

For some context and the necessity for a separate thinkpad_ec module,
see LKML thread "tp_smapi conflict with IDE, hdaps" on Dec. 2005.

I would like to thank the many testers and contributors who helped
in the development, and in particular Henrique de Moraes Holschuh,
ThinkWiki.org users Whoopie and Spiney, and Pavel Machek.

[1] http://thinkwiki.org/wiki/tp_smapi


Summary:

 drivers/firmware/Kconfig       |    3 
 drivers/firmware/Makefile      |    1 
 drivers/firmware/thinkpad_ec.c |  465 ++++++++++++++++++
 drivers/hwmon/Kconfig          |    1 
 drivers/hwmon/hdaps.c          |  784 ++++++++++++++++++-------------
 include/linux/thinkpad_ec.h    |   47 +
 6 files changed, 990 insertions(+), 311 deletions(-)

[PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access

[Shem Multinymous]

The embedded controller on ThinkPad laptops has a non-standard interface
at IO ports 0x1600-0x161F (mapped to LCP channel 3 of the H8S chip).
The interface provides various system management services (currently 
known: battery information and accelerometer readouts). This driver
provides access and mutual exclusion for the EC interface.

The mainline hdaps driver already uses this hardware interface (in an 
incorrect and unsafe way), and will be converted to use this module in
the following patches. Another driver using this module, tp_smapi, will 
be submitted later.

The Kconfig entry is set to tristate and will be selected by hdaps and
(eventually) tp_smapi, since thinkpad_ec does nothing by itself.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---

This depends on dmi-decode-and-save-oem-string-information.patch 
pending in -mm ("[PATCH] DMI: Decode and save OEM String information" 
on LKML).

 drivers/firmware/Kconfig       |    3 
 drivers/firmware/Makefile      |    1 
 drivers/firmware/thinkpad_ec.c |  465 +++++++++++++++++++++++++++++++++++++++++
 include/linux/thinkpad_ec.h    |   47 ++++
 4 files changed, 516 insertions(+)


--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -84,4 +84,7 @@ config DCDBAS
 	  Say Y or M here to enable the driver for use by Dell systems
 	  management software such as Dell OpenManage.
 
+config THINKPAD_EC
+	tristate
+
 endmenu
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_EFI_VARS)		+= efivars.o
 obj-$(CONFIG_EFI_PCDP)		+= pcdp.o
 obj-$(CONFIG_DELL_RBU)          += dell_rbu.o
 obj-$(CONFIG_DCDBAS)		+= dcdbas.o
+obj-$(CONFIG_THINKPAD_EC)       += thinkpad_ec.o
--- /dev/null
+++ b/drivers/firmware/thinkpad_ec.c
@@ -0,0 +1,465 @@
+/*
+ *  thinkpad_ec.c - coordinate access to ThinkPad-specific hardware resources
+ *
+ *  The embedded controller on ThinkPad laptops has a non-standard interface
+ *  at IO ports 0x1600-0x161F (mapped to LCP channel 3 of the H8S chip).
+ *  The interface provides various system management services (currently
+ *  known: battery information and accelerometer readouts). This driver
+ *  provides access and mutual exclusion for the EC interface.
+ *  For information about the LPC protocol and terminology, see:
+ *  "H8S/2104B Group Hardware Manual",
+ * http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf
+ *
+ *  Copyright (C) 2006 Shem Multinymous <multinymous@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/dmi.h>
+#include <linux/ioport.h>
+#include <linux/delay.h>
+#include <linux/thinkpad_ec.h>
+#include <linux/jiffies.h>
+#include <asm/io.h>
+
+#define TP_VERSION "0.28"
+
+MODULE_AUTHOR("Shem Multinymous");
+MODULE_DESCRIPTION("ThinkPad embedded controller hardware access");
+MODULE_VERSION(TP_VERSION);
+MODULE_LICENSE("GPL");
+
+/* IO ports used by embedded controller LPC channel 3: */
+#define TPC_BASE_PORT 0x1600
+#define TPC_NUM_PORTS 0x20
+#define TPC_STR3_PORT 0x1604  /* Reads H8S EC register STR3 */
+#define TPC_TWR0_PORT  0x1610 /* Mapped to H8S EC register TWR0MW/SW  */
+#define TPC_TWR15_PORT 0x161F /* Mapped to H8S EC register TWR15. */
+  /* (and port TPC_TWR0_PORT+i is mapped to H8S reg TWRi for 0<i<16) */
+
+/* H8S STR3 status flags (see "H8S/2104B Group Hardware Manual" p.549) */
+#define H8S_STR3_IBF3B 0x80  /* Bidi. Data Register Input Buffer Full */
+#define H8S_STR3_OBF3B 0x40  /* Bidi. Data Register Output Buffer Full */
+#define H8S_STR3_MWMF  0x20  /* Master Write Mode Flag */
+#define H8S_STR3_SWMF  0x10  /* Slave Write Mode Flag */
+#define H8S_STR3_MASK  0xF0  /* All bits we care about in STR3 */
+
+/* Timeouts and retries */
+#define TPC_READ_RETRIES    150
+#define TPC_READ_NDELAY     500
+#define TPC_REQUEST_RETRIES 100
+#define TPC_REQUEST_NDELAY   10
+#define TPC_PREFETCH_TIMEOUT   (HZ/10)  /* invalidate prefetch after 0.1sec */
+
+/* A few macros for printk()ing: */
+#define MSG_FMT(fmt, args...) \
+  "thinkpad_ec: %s: " fmt "\n", __func__, ## args
+#define REQ_FMT(msg, code) \
+  MSG_FMT("%s: (0x%02x:0x%02x)->0x%02x", \
+          msg, args->val[0x0], args->val[0xF], code)
+
+/* State of request prefetching: */
+static u8 prefetch_arg0, prefetch_argF;           /* Args of last prefetch */
+static u64 prefetch_jiffies;                      /* time of prefetch, or: */
+#define TPC_PREFETCH_NONE   INITIAL_JIFFIES       /* - No prefetch */
+#define TPC_PREFETCH_JUNK   (INITIAL_JIFFIES+1)   /* - Ignore prefetch */
+
+/* Locking: */
+
+static DECLARE_MUTEX(thinkpad_ec_mutex);
+
+/**
+ * thinkpad_ec_lock - get lock on the ThinkPad EC
+ *
+ * Get exclusive lock for accesing the ThinkPad embedded controller LPC3
+ * interface. Returns 0 iff lock acquired.
+ */
+int thinkpad_ec_lock(void)
+{
+	int ret;
+	ret = down_interruptible(&thinkpad_ec_mutex);
+	return ret;
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_lock);
+
+/**
+ * thinkpad_ec_try_lock - try getting lock on the ThinkPad EC
+ *
+ * Try getting an exclusive lock for accesing the ThinkPad embedded
+ * controller LPC3. Returns immediately if lock is not available; neither
+ * blocks nor sleeps. Returns 0 iff lock acquired .
+ */
+int thinkpad_ec_try_lock(void)
+{
+	return down_trylock(&thinkpad_ec_mutex);
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_try_lock);
+
+/**
+ * thinkpad_ec_unlock - release lock on ThinkPad EC
+ *
+ * Release a previously acquired exclusive lock on the ThinkPad ebmedded
+ * controller LPC3 interface.
+ */
+void thinkpad_ec_unlock(void)
+{
+ 	up(&thinkpad_ec_mutex);
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_unlock);
+
+/* Tell embedded controller to prepare a row */
+static int thinkpad_ec_request_row(const struct thinkpad_ec_row *args)
+{
+	u8 str3;
+	int i;
+
+	/* EC protocol requires write to TWR0 (function code): */
+	if (!(args->mask & 0x0001)) {
+		printk(KERN_ERR MSG_FMT("bad args->mask=0x%02x", args->mask));
+		return -EINVAL;
+	}
+
+	/* Check initial STR3 status: */
+	str3 = inb(TPC_STR3_PORT) & H8S_STR3_MASK;
+	if (str3 & H8S_STR3_OBF3B) { /* data already pending */
+		inb(TPC_TWR15_PORT); /* marks end of previous transaction */
+		if (prefetch_jiffies == TPC_PREFETCH_NONE)
+			printk(KERN_WARNING
+			       REQ_FMT("readout already pending", str3));
+		return -EBUSY; /* EC will be ready in a few usecs */
+	} else if (str3 == H8S_STR3_SWMF) { /* busy with previous request */
+		if (prefetch_jiffies == TPC_PREFETCH_NONE)
+			printk(KERN_WARNING
+			       REQ_FMT("EC handles previous request", str3));
+		return -EBUSY; /* data will be pending in a few usecs */
+	} else if (str3 != 0x00) { /* unexpected status? */
+		printk(KERN_WARNING REQ_FMT("bad initial STR3", str3));
+		return -EIO;
+	}
+
+	/* Send TWR0MW: */
+	outb(args->val[0], TPC_TWR0_PORT);
+	str3 = inb(TPC_STR3_PORT) & H8S_STR3_MASK;
+	if (str3 != H8S_STR3_MWMF) { /* not accepted? */
+		printk(KERN_WARNING REQ_FMT("arg0 rejected", str3));
+		return -EIO;
+	}
+
+	/* Send TWR1 through TWR14: */
+	for (i=1; i<TP_CONTROLLER_ROW_LEN-1; i++)
+		if ((args->mask>>i)&1)
+			outb(args->val[i], TPC_TWR0_PORT+i);
+
+	/* Send TWR15 (default to 0x01). This marks end of command. */
+	outb((args->mask & 0x8000) ? args->val[0xF] : 0x01, TPC_TWR15_PORT);
+
+	/* Wait until EC starts writing its reply (~60ns on average).
+	 * Releasing locks before this happens may cause an EC hang
+	 * due to firmware bug!
+	 */
+	for (i=0; i<TPC_REQUEST_RETRIES; i++) {
+		str3 = inb(TPC_STR3_PORT) & H8S_STR3_MASK;
+		if (str3 & H8S_STR3_SWMF) /* EC started replying */
+			return 0;
+		else if (str3==(H8S_STR3_IBF3B|H8S_STR3_MWMF) ||
+		         str3==0x00) /* normal progress, wait it out */
+			ndelay(TPC_REQUEST_NDELAY);
+		else { /* weird EC status */
+			printk(KERN_WARNING
+			       REQ_FMT("bad end STR3", str3));
+			return -EIO;
+		}
+	}
+	printk(KERN_WARNING REQ_FMT("EC is mysteriously silent", str3));
+	return -EIO;
+}
+
+/* Read current row data from the controller, assuming it's already
+ * requested.
+ */
+static int thinkpad_ec_read_data(struct thinkpad_ec_row *data)
+{
+	int i;
+	u8 str3 = inb(TPC_STR3_PORT) & H8S_STR3_MASK;
+	/* Once we make a request, STR3 assumes the sequence of values listed
+         * in the following 'if' as it reads the request and writes its data.
+	 * It takes about a few dozen nanosecs total, with very high variance.
+	 */
+	if (str3==(H8S_STR3_IBF3B|H8S_STR3_MWMF) ||
+	    str3==0x00 ||   /* the 0x00 is indistinguishable from idle EC! */
+	    str3==H8S_STR3_SWMF )
+		return -EBUSY; /* not ready yet */
+	/* Finally, the EC signals output buffer full: */
+	if (str3 != (H8S_STR3_OBF3B|H8S_STR3_SWMF)) {
+		printk(KERN_WARNING
+		       MSG_FMT("bad initial STR3 (0x%02x)", str3));
+		return -EIO;
+	}
+
+	/* Read first byte (signals start of read transactions): */
+	data->val[0] = inb(TPC_TWR0_PORT);
+	/* Optionally read 14 more bytes: */
+	for (i=1; i<TP_CONTROLLER_ROW_LEN-1; i++)
+		if ((data->mask >> i)&1)
+			data->val[i] = inb(TPC_TWR0_PORT+i);
+	/* Read last byte from 0x161F (signals end of read transaction): */
+	data->val[0xF] = inb(TPC_TWR15_PORT);
+
+	/* Readout still pending? */
+	str3 = inb(TPC_STR3_PORT) & H8S_STR3_MASK;
+	if (str3 & H8S_STR3_OBF3B)
+		printk(KERN_WARNING
+		       MSG_FMT("OBF3B=1 after read (0x%02x)", str3));
+	return 0;
+}
+
+/* Is the given row currently prefetched?
+ * To keep things simple we compare only the first and last args;
+ * in practice this suffices                                        .*/
+static int thinkpad_ec_is_row_fetched(const struct thinkpad_ec_row *args)
+{
+	return (prefetch_jiffies != TPC_PREFETCH_NONE) &&
+	       (prefetch_jiffies != TPC_PREFETCH_JUNK) &&
+	       (prefetch_arg0 == args->val[0]) &&
+	       (prefetch_argF == args->val[0xF]) &&
+	       (get_jiffies_64() < prefetch_jiffies + TPC_PREFETCH_TIMEOUT);
+}
+
+/**
+ * thinkpad_ec_read_row - request and read data from ThinkPad EC
+ * @args Input register arguments
+ * @data Output register values
+ *
+ * Read a data row from the ThinkPad embedded controller LPC3 interface.
+ * Does fetching and retrying if needed. The row args are specified by
+ * 16 byte arguments, some of which may be missing (but the first is
+ * mandatory). These are given in @args->val[], where @args->val[i] is
+ * used iff (@args->mask>>i)&1). The rows's data is stored in @data->val[],
+ * but is only guaranteed to be valid for indices corresponding to set
+ * bit in @data->mask. That is, if (@data->mask>>i)&1==0 then @data->val[i]
+ * may not be filled (to save time).
+ *
+ * Returns -EBUSY on transient error and -EIO on abnormal condition.
+ * Caller must hold controller lock.
+ */
+int thinkpad_ec_read_row(const struct thinkpad_ec_row *args,
+                           struct thinkpad_ec_row *data)
+{
+	int retries, ret;
+
+	if (thinkpad_ec_is_row_fetched(args))
+		goto read_row; /* already requested */
+
+	/* Request the row */
+	for (retries=0; retries<TPC_READ_RETRIES; ++retries) {
+		ret = thinkpad_ec_request_row(args);
+		if (!ret)
+			goto read_row;
+		if (ret != -EBUSY)
+			break;
+		ndelay(TPC_READ_NDELAY);
+	}
+	printk(KERN_ERR REQ_FMT("failed requesting row", ret));
+	goto out;
+
+read_row:
+	/* Read the row's data */
+	for (retries=0; retries<TPC_READ_RETRIES; ++retries) {
+		ret = thinkpad_ec_read_data(data);
+		if (!ret)
+			goto out;
+		if (ret!=-EBUSY)
+			break;
+		ndelay(TPC_READ_NDELAY);
+	}
+
+	printk(KERN_ERR REQ_FMT("failed waiting for data", ret));
+
+out:
+	prefetch_jiffies = TPC_PREFETCH_JUNK;
+	return ret;
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_read_row);
+
+/**
+ * thinkpad_ec_try_read_row - try reading prefetched data from ThinkPad EC
+ * @args Input register arguments
+ * @data Output register values
+ *
+ * Try reading a data row from the ThinkPad embedded controller LPC3
+ * interface, if this raw was recently prefetched using
+ * thinkpad_ec_prefetch_row(). Does not fetch, retry or block.
+ * The parameters have the same meaning as in thinkpad_ec_read_row().
+ *
+ * Returns -EBUSY is data not ready and -ENODATA if row not prefetched.
+ * Caller must hold controller lock.
+ */
+int thinkpad_ec_try_read_row(const struct thinkpad_ec_row *args,
+                               struct thinkpad_ec_row *data)
+{
+	int ret;
+	if (!thinkpad_ec_is_row_fetched(args)) {
+		ret = -ENODATA;
+	} else {
+		ret = thinkpad_ec_read_data(data);
+		if (!ret)
+			prefetch_jiffies = TPC_PREFETCH_NONE; /* eaten up */
+	}
+	return ret;
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_try_read_row);
+
+/**
+ * thinkpad_ec_prefetch_row - prefetch data from ThinkPad EC
+ * @args Input register arguments
+ *
+ * Prefetch a data row from the ThinkPad embedded controller LCP3
+ * interface. A subsequent call to thinkpad_ec_read_row() with the
+ * same arguments will be faster, and a subsequent call to
+ * thinkpad_ec_try_read_row() stands a good chance of succeeding if
+ * done neither too soon nor too late. See
+ * thinkpad_ec_read_row() for the meaning of @args.
+ *
+ * Returns -EBUSY on transient error and -EIO on abnormal condition.
+ * Caller must hold controller lock.
+ */
+int thinkpad_ec_prefetch_row(const struct thinkpad_ec_row *args)
+{
+	int ret;
+ 	ret = thinkpad_ec_request_row(args);
+	if (ret) {
+		prefetch_jiffies = TPC_PREFETCH_JUNK;
+	} else {
+		prefetch_jiffies = get_jiffies_64();
+		prefetch_arg0 = args->val[0x0];
+		prefetch_argF = args->val[0xF];
+	}
+	return ret;
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_prefetch_row);
+
+/**
+ * thinkpad_ec_invalidate - invalidate prefetched ThinkPad EC data
+ *
+ * Invalidate the data prefetched via thinkpad_ec_prefetch_row() from the
+ * ThinkPad embedded controller LPC3 interface.
+ * Must be called before unlocking by any code that accesses the controller
+ * ports directly.
+ */
+void thinkpad_ec_invalidate(void)
+{
+	prefetch_jiffies = TPC_PREFETCH_JUNK;
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_invalidate);
+
+
+/*** Checking for EC hardware ***/
+
+/* thinkpad_ec_test:
+ * Ensure the EC LPC3 channel really works on this machine by making
+ * an arbitrary harmless EC request and seeing if the EC follows protocol.
+ * This test writes to IO ports, so execute only after checking DMI.
+ */
+static int thinkpad_ec_test(void)
+{
+	int ret;
+	const struct thinkpad_ec_row args = /* battery 0 basic status */
+	  { .mask=0x8001, .val={0x01,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0x00} };
+	struct thinkpad_ec_row data = { .mask = 0x0000 };
+	ret = thinkpad_ec_lock();
+	if (ret)
+		return ret;
+	ret = thinkpad_ec_read_row(&args, &data);
+	thinkpad_ec_unlock();
+	return ret;
+}
+
+/* Search all DMI device names of a given type for a substring */
+static int __init dmi_find_substring(int type, const char *substr)
+{
+	struct dmi_device *dev = NULL;
+	while ((dev = dmi_find_device(type, NULL, dev))) {
+		if (strstr(dev->name, substr))
+			return 1;
+	}
+	return 0;
+}
+
+#define TP_DMI_MATCH(vendor,model)	{		\
+	.ident = vendor " " model,			\
+	.matches = {					\
+		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
+		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
+	}						\
+}
+
+/* Check DMI for existence of ThinkPad embedded controller */
+static int __init check_dmi_for_ec(void)
+{
+	/* A few old models that have a good EC but don't report it in DMI */
+	struct dmi_system_id tp_whitelist[] = {
+		TP_DMI_MATCH("IBM","ThinkPad A30"),
+		TP_DMI_MATCH("IBM","ThinkPad T23"),
+		TP_DMI_MATCH("IBM","ThinkPad X24"),
+		{ .ident = NULL }
+	};
+	return dmi_find_substring(DMI_DEV_TYPE_OEM_STRING,
+	                          "IBM ThinkPad Embedded Controller") ||
+	       dmi_check_system(tp_whitelist);
+}
+
+/*** Init and cleanup ***/
+
+static int __init thinkpad_ec_init(void)
+{
+	if (!check_dmi_for_ec()) {
+		printk(KERN_WARNING "thinkpad_ec: no ThinkPad embedded controller!\n");
+		return -ENODEV;
+	}
+
+	if (!request_region(TPC_BASE_PORT, TPC_NUM_PORTS,
+	                    "thinkpad_ec")) {
+		printk(KERN_ERR "thinkpad_ec: cannot claim io ports %#x-%#x\n",
+		       TPC_BASE_PORT,
+		       TPC_BASE_PORT + TPC_NUM_PORTS -1);
+		return -ENXIO;
+	}
+	prefetch_jiffies = TPC_PREFETCH_JUNK;
+	if (thinkpad_ec_test()) {
+		printk(KERN_ERR "thinkpad_ec: initial ec test failed\n");
+		release_region(TPC_BASE_PORT, TPC_NUM_PORTS);
+		return -ENXIO;
+	}
+	printk(KERN_INFO "thinkpad_ec: thinkpad_ec " TP_VERSION " loaded.\n");
+	return 0;
+}
+
+static void __exit thinkpad_ec_exit(void)
+{
+	release_region(TPC_BASE_PORT, TPC_NUM_PORTS);
+	printk(KERN_INFO "thinkpad_ec: unloaded.\n");
+}
+
+module_init(thinkpad_ec_init);
+module_exit(thinkpad_ec_exit);
--- /dev/null
+++ b/include/linux/thinkpad_ec.h
@@ -0,0 +1,47 @@
+/*
+ *  thinkpad_ec.h - interface to ThinkPad embedded controller LPC3 functions
+ *
+ *  Copyright (C) 2005 Shem Multinymous <multinymous@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef _THINKPAD_EC_H
+#define _THINKPAD_EC_H
+
+#ifdef __KERNEL__
+
+#define TP_CONTROLLER_ROW_LEN 16
+
+/* EC transactions input and output (possibly partial) vectors of 16 bytes. */
+struct thinkpad_ec_row {
+	u16 mask; /* bitmap of which entries of val[] are meaningful */
+	u8 val[TP_CONTROLLER_ROW_LEN];
+};
+
+extern int thinkpad_ec_lock(void);
+extern int thinkpad_ec_try_lock(void);
+extern void thinkpad_ec_unlock(void);
+
+extern int thinkpad_ec_read_row(const struct thinkpad_ec_row *args,
+                                struct thinkpad_ec_row *data);
+extern int thinkpad_ec_try_read_row(const struct thinkpad_ec_row *args,
+                                    struct thinkpad_ec_row *mask);
+extern int thinkpad_ec_prefetch_row(const struct thinkpad_ec_row *args);
+extern void thinkpad_ec_invalidate(void);
+
+
+#endif /* __KERNEL */
+#endif /* _THINKPAD_EC_H */

[PATCH 02/12] hdaps: Use thinkpad_ec instead of direct port access

[Shem Multinymous]

The hdaps module currently talks to the ThinkPad embedded controller via direct
IO port access, and does so incorrectly and with insufficient IO checking.
Beyond bad readouts, this could in principle, trigger known EC firmware bugs,
thereby causing a firmware hang and possible hardware damage.

The just-introduced thinkpad_ec provides a safe, correct way to access these
EC functions. This patch changes hdaps to use that. This also enables other
drivers (e.g., the soon-to-be-submitted tp_smapi module) to access the EC
without collisions.

As you can see from the comments at the beginning of the patch, the old
driver got some stuff wrong about the meaning of EC readouts. This patch
preserves the old broken behavior; it will be fixed in a later patch.

The one-line 
		{ ABORT_INIT("check4"); goto bad; }
statements in hdaps_init_device() are a coding style violation, but make
the init code much easier to read than otherwise.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/Kconfig |    1 
 drivers/hwmon/hdaps.c |  314 ++++++++++++++++++--------------------------------
 2 files changed, 116 insertions(+), 199 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -33,31 +33,28 @@
 #include <linux/module.h>
 #include <linux/timer.h>
 #include <linux/dmi.h>
-#include <asm/io.h>
+#include <linux/thinkpad_ec.h>
 
-#define HDAPS_LOW_PORT		0x1600	/* first port used by hdaps */
-#define HDAPS_NR_PORTS		0x30	/* number of ports: 0x1600 - 0x162f */
-
-#define HDAPS_PORT_STATE	0x1611	/* device state */
-#define HDAPS_PORT_YPOS		0x1612	/* y-axis position */
-#define	HDAPS_PORT_XPOS		0x1614	/* x-axis position */
-#define HDAPS_PORT_TEMP1	0x1616	/* device temperature, in Celsius */
-#define HDAPS_PORT_YVAR		0x1617	/* y-axis variance (what is this?) */
-#define HDAPS_PORT_XVAR		0x1619	/* x-axis variance (what is this?) */
-#define HDAPS_PORT_TEMP2	0x161b	/* device temperature (again?) */
-#define HDAPS_PORT_UNKNOWN	0x161c	/* what is this? */
-#define HDAPS_PORT_KMACT	0x161d	/* keyboard or mouse activity */
-
-#define STATE_FRESH		0x50	/* accelerometer data is fresh */
+/* Embedded controller accelerometer read command and its result: */
+static const struct thinkpad_ec_row ec_accel_args =
+	{ .mask=0x0001, .val={0x11} };
+#define EC_ACCEL_IDX_READOUTS	0x1	/* readouts included in this read */
+					/* First readout, if READOUTS>=1: */
+#define EC_ACCEL_IDX_YPOS1	0x2	/*   y-axis position word */
+#define EC_ACCEL_IDX_XPOS1	0x4	/*   x-axis position word */
+#define EC_ACCEL_IDX_TEMP1	0x6	/*   device temperature in Celsius */
+					/* Second readout, if READOUTS>=2: */
+#define EC_ACCEL_IDX_XPOS2	0x7	/*   y-axis position word */
+#define EC_ACCEL_IDX_YPOS2	0x9	/*   x-axis pisition word */
+#define EC_ACCEL_IDX_TEMP2	0xb	/*   device temperature in Celsius */
+#define EC_ACCEL_IDX_QUEUED	0xc	/* Number of queued readouts left */
+#define EC_ACCEL_IDX_KMACT	0xd	/* keyboard or mouse activity */
 
 #define KEYBD_MASK		0x20	/* set if keyboard activity */
 #define MOUSE_MASK		0x40	/* set if mouse activity */
 #define KEYBD_ISSET(n)		(!! (n & KEYBD_MASK))	/* keyboard used? */
 #define MOUSE_ISSET(n)		(!! (n & MOUSE_MASK))	/* mouse used? */
 
-#define INIT_TIMEOUT_MSECS	4000	/* wait up to 4s for device init ... */
-#define INIT_WAIT_MSECS		200	/* ... in 200ms increments */
-
 #define HDAPS_POLL_PERIOD	(HZ/20)	/* poll for input every 1/20s */
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
@@ -70,79 +67,6 @@ static u8 km_activity;
 static int rest_x;
 static int rest_y;
 
-static DECLARE_MUTEX(hdaps_sem);
-
-/*
- * __get_latch - Get the value from a given port.  Callers must hold hdaps_sem.
- */
-static inline u8 __get_latch(u16 port)
-{
-	return inb(port) & 0xff;
-}
-
-/*
- * __check_latch - Check a port latch for a given value.  Returns zero if the
- * port contains the given value.  Callers must hold hdaps_sem.
- */
-static inline int __check_latch(u16 port, u8 val)
-{
-	if (__get_latch(port) == val)
-		return 0;
-	return -EINVAL;
-}
-
-/*
- * __wait_latch - Wait up to 100us for a port latch to get a certain value,
- * returning zero if the value is obtained.  Callers must hold hdaps_sem.
- */
-static int __wait_latch(u16 port, u8 val)
-{
-	unsigned int i;
-
-	for (i = 0; i < 20; i++) {
-		if (!__check_latch(port, val))
-			return 0;
-		udelay(5);
-	}
-
-	return -EIO;
-}
-
-/*
- * __device_refresh - request a refresh from the accelerometer.  Does not wait
- * for refresh to complete.  Callers must hold hdaps_sem.
- */
-static void __device_refresh(void)
-{
-	udelay(200);
-	if (inb(0x1604) != STATE_FRESH) {
-		outb(0x11, 0x1610);
-		outb(0x01, 0x161f);
-	}
-}
-
-/*
- * __device_refresh_sync - request a synchronous refresh from the
- * accelerometer.  We wait for the refresh to complete.  Returns zero if
- * successful and nonzero on error.  Callers must hold hdaps_sem.
- */
-static int __device_refresh_sync(void)
-{
-	__device_refresh();
-	return __wait_latch(0x1604, STATE_FRESH);
-}
-
-/*
- * __device_complete - indicate to the accelerometer that we are done reading
- * data, and then initiate an async refresh.  Callers must hold hdaps_sem.
- */
-static inline void __device_complete(void)
-{
-	inb(0x161f);
-	inb(0x1604);
-	__device_refresh();
-}
-
 /*
  * hdaps_readb_one - reads a byte from a single I/O port, placing the value in
  * the given pointer.  Returns zero on success or a negative error on failure.
@@ -151,19 +75,15 @@ static inline void __device_complete(voi
 static int hdaps_readb_one(unsigned int port, u8 *val)
 {
 	int ret;
+	struct thinkpad_ec_row data;
 
-	down(&hdaps_sem);
-
-	/* do a sync refresh -- we need to be sure that we read fresh data */
-	ret = __device_refresh_sync();
+	ret = thinkpad_ec_lock();
 	if (ret)
-		goto out;
-
-	*val = inb(port);
-	__device_complete();
-
-out:
-	up(&hdaps_sem);
+		return ret;
+	data.mask = (1 << port);
+	ret = thinkpad_ec_read_row(&ec_accel_args, &data);
+	*val = data.val[port];
+	thinkpad_ec_unlock();
 	return ret;
 }
 
@@ -171,14 +91,17 @@ out:
 static int __hdaps_read_pair(unsigned int port1, unsigned int port2,
 			     int *x, int *y)
 {
-	/* do a sync refresh -- we need to be sure that we read fresh data */
-	if (__device_refresh_sync())
-		return -EIO;
+	int ret;
+	struct thinkpad_ec_row data;
 
-	*y = inw(port2);
-	*x = inw(port1);
-	km_activity = inb(HDAPS_PORT_KMACT);
-	__device_complete();
+	data.mask = (3 << port1) | (3 << port2) | (1 << EC_ACCEL_IDX_KMACT);
+	ret = thinkpad_ec_read_row(&ec_accel_args, &data);
+	if (ret)
+		return ret;
+
+	*x = *(s16*)(data.val+port1);
+	*y = *(s16*)(data.val+port2);
+	km_activity = data.val[EC_ACCEL_IDX_KMACT];
 
 	/* if hdaps_invert is set, negate the two values */
 	if (hdaps_invert) {
@@ -196,12 +119,11 @@ static int __hdaps_read_pair(unsigned in
 static int hdaps_read_pair(unsigned int port1, unsigned int port2,
 			   int *val1, int *val2)
 {
-	int ret;
-
-	down(&hdaps_sem);
+	int ret = thinkpad_ec_lock();
+	if (ret)
+		return ret;
 	ret = __hdaps_read_pair(port1, port2, val1, val2);
-	up(&hdaps_sem);
-
+	thinkpad_ec_unlock();
 	return ret;
 }
 
@@ -209,77 +131,79 @@ static int hdaps_read_pair(unsigned int 
  * hdaps_device_init - initialize the accelerometer.  Returns zero on success
  * and negative error code on failure.  Can sleep.
  */
+#define ABORT_INIT(msg) printk(KERN_ERR "hdaps init failed at: %s\n", msg)
 static int hdaps_device_init(void)
 {
-	int total, ret = -ENXIO;
-
-	down(&hdaps_sem);
-
-	outb(0x13, 0x1610);
-	outb(0x01, 0x161f);
-	if (__wait_latch(0x161f, 0x00))
-		goto out;
-
-	/*
-	 * Most ThinkPads return 0x01.
-	 *
-	 * Others--namely the R50p, T41p, and T42p--return 0x03.  These laptops
-	 * have "inverted" axises.
-	 *
-	 * The 0x02 value occurs when the chip has been previously initialized.
-	 */
-	if (__check_latch(0x1611, 0x03) &&
-		     __check_latch(0x1611, 0x02) &&
-		     __check_latch(0x1611, 0x01))
-		goto out;
-
-	printk(KERN_DEBUG "hdaps: initial latch check good (0x%02x).\n",
-	       __get_latch(0x1611));
+	int ret;
+	struct thinkpad_ec_row args, data;
+	u8 mode;
 
-	outb(0x17, 0x1610);
-	outb(0x81, 0x1611);
-	outb(0x01, 0x161f);
-	if (__wait_latch(0x161f, 0x00))
-		goto out;
-	if (__wait_latch(0x1611, 0x00))
-		goto out;
-	if (__wait_latch(0x1612, 0x60))
-		goto out;
-	if (__wait_latch(0x1613, 0x00))
-		goto out;
-	outb(0x14, 0x1610);
-	outb(0x01, 0x1611);
-	outb(0x01, 0x161f);
-	if (__wait_latch(0x161f, 0x00))
-		goto out;
-	outb(0x10, 0x1610);
-	outb(0xc8, 0x1611);
-	outb(0x00, 0x1612);
-	outb(0x02, 0x1613);
-	outb(0x01, 0x161f);
-	if (__wait_latch(0x161f, 0x00))
-		goto out;
-	if (__device_refresh_sync())
-		goto out;
-	if (__wait_latch(0x1611, 0x00))
-		goto out;
+	ret = thinkpad_ec_lock();
+	if (ret)
+		return ret;
 
-	/* we have done our dance, now let's wait for the applause */
-	for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
-		int x, y;
-
-		/* a read of the device helps push it into action */
-		__hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &x, &y);
-		if (!__wait_latch(0x1611, 0x02)) {
-			ret = 0;
-			break;
-		}
+	args.val[0x0] = 0x13;
+	args.val[0xF] = 0x01;
+	args.mask=0x8001;
+	data.mask=0x8002;
+	if (thinkpad_ec_read_row(&args, &data))
+		{ ABORT_INIT("read1"); goto bad; }
+	if (data.val[0xF]!=0x00)
+		{ ABORT_INIT("check1"); goto bad; }
+	mode = data.val[0x1];
+
+	printk(KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n", mode);
+	if (mode==0x00)
+		{ ABORT_INIT("accelerometer not available"); goto bad; }
+
+	args.val[0x0] = 0x17;
+	args.val[0x1] = 0x81;
+	args.val[0xF] = 0x01;
+	args.mask = 0x8003;
+	data.mask = 0x800E;
+	if (thinkpad_ec_read_row(&args, &data))
+		{ ABORT_INIT("read2"); goto bad; }
+	if (data.val[0x1]!=0x00 ||
+	    data.val[0x2]!=0x60 ||
+	    data.val[0x3]!=0x00 ||
+	    data.val[0xF]!=0x00)
+		{ ABORT_INIT("check2"); goto bad; }
+
+	args.val[0x0] = 0x14;
+	args.val[0x1] = 0x01;
+	args.val[0xF] = 0x01;
+	args.mask = 0x8003;
+	data.mask = 0x8000;
+	if (thinkpad_ec_read_row(&args, &data))
+		{ ABORT_INIT("read3"); goto bad; }
+	if (data.val[0xF]!=0x00)
+		{ ABORT_INIT("check3"); goto bad; }
+
+	args.val[0x0] = 0x10;
+	args.val[0x1] = 0xc8;
+	args.val[0x2] = 0x00;
+	args.val[0x3] = 0x02;
+	args.val[0xF] = 0x01;
+	args.mask = 0x800F;
+	data.mask = 0x8000;
+	if (thinkpad_ec_read_row(&args, &data))
+		{ ABORT_INIT("read4"); goto bad; }
+	if (data.val[0xF]!=0x00)
+		{ ABORT_INIT("check4"); goto bad; }
 
-		msleep(INIT_WAIT_MSECS);
-	}
+	thinkpad_ec_invalidate();
+	udelay(200);
 
-out:
-	up(&hdaps_sem);
+	/* Just prefetch instead of reading, to avoid ~1sec delay on load */
+	ret = thinkpad_ec_prefetch_row(&ec_accel_args);
+	if (ret)
+		{ ABORT_INIT("initial prefetch failed"); goto bad; }
+	goto good;
+bad:
+	thinkpad_ec_invalidate();
+	ret = -ENXIO;
+good:
+	thinkpad_ec_unlock();
 	return ret;
 }
 
@@ -317,7 +241,7 @@ static struct platform_driver hdaps_driv
  */
 static void hdaps_calibrate(void)
 {
-	__hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &rest_x, &rest_y);
+	__hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &rest_x, &rest_y);
 }
 
 static void hdaps_mousedev_poll(unsigned long unused)
@@ -325,12 +249,12 @@ static void hdaps_mousedev_poll(unsigned
 	int x, y;
 
 	/* Cannot sleep.  Try nonblockingly.  If we fail, try again later. */
-	if (down_trylock(&hdaps_sem)) {
+	if (thinkpad_ec_try_lock()) {
 		mod_timer(&hdaps_timer,jiffies + HDAPS_POLL_PERIOD);
 		return;
 	}
 
-	if (__hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &x, &y))
+	if (__hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &x, &y))
 		goto out;
 
 	input_report_abs(hdaps_idev, ABS_X, x - rest_x);
@@ -340,7 +264,7 @@ static void hdaps_mousedev_poll(unsigned
 	mod_timer(&hdaps_timer, jiffies + HDAPS_POLL_PERIOD);
 
 out:
-	up(&hdaps_sem);
+	thinkpad_ec_unlock();
 }
 
 
@@ -351,7 +275,7 @@ static ssize_t hdaps_position_show(struc
 {
 	int ret, x, y;
 
-	ret = hdaps_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &x, &y);
+	ret = hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &x, &y);
 	if (ret)
 		return ret;
 
@@ -363,7 +287,7 @@ static ssize_t hdaps_variance_show(struc
 {
 	int ret, x, y;
 
-	ret = hdaps_read_pair(HDAPS_PORT_XVAR, HDAPS_PORT_YVAR, &x, &y);
+	ret = hdaps_read_pair(EC_ACCEL_IDX_XPOS2, EC_ACCEL_IDX_YPOS2, &x, &y);
 	if (ret)
 		return ret;
 
@@ -376,7 +300,7 @@ static ssize_t hdaps_temp1_show(struct d
 	u8 temp;
 	int ret;
 
-	ret = hdaps_readb_one(HDAPS_PORT_TEMP1, &temp);
+	ret = hdaps_readb_one(EC_ACCEL_IDX_TEMP1, &temp);
 	if (ret < 0)
 		return ret;
 
@@ -389,7 +313,7 @@ static ssize_t hdaps_temp2_show(struct d
 	u8 temp;
 	int ret;
 
-	ret = hdaps_readb_one(HDAPS_PORT_TEMP2, &temp);
+	ret = hdaps_readb_one(EC_ACCEL_IDX_TEMP2, &temp);
 	if (ret < 0)
 		return ret;
 
@@ -420,10 +344,10 @@ static ssize_t hdaps_calibrate_store(str
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
 {
-	down(&hdaps_sem);
+	if (thinkpad_ec_lock())
+		return -EIO;
 	hdaps_calibrate();
-	up(&hdaps_sem);
-
+	thinkpad_ec_unlock();
 	return count;
 }
 
@@ -550,14 +474,9 @@ static int __init hdaps_init(void)
 		goto out;
 	}
 
-	if (!request_region(HDAPS_LOW_PORT, HDAPS_NR_PORTS, "hdaps")) {
-		ret = -ENXIO;
-		goto out;
-	}
-
 	ret = platform_driver_register(&hdaps_driver);
 	if (ret)
-		goto out_region;
+		goto out;
 
 	pdev = platform_device_register_simple("hdaps", -1, NULL, 0);
 	if (IS_ERR(pdev)) {
@@ -604,8 +523,6 @@ out_device:
 	platform_device_unregister(pdev);
 out_driver:
 	platform_driver_unregister(&hdaps_driver);
-out_region:
-	release_region(HDAPS_LOW_PORT, HDAPS_NR_PORTS);
 out:
 	printk(KERN_WARNING "hdaps: driver init failed (ret=%d)!\n", ret);
 	return ret;
@@ -618,7 +535,6 @@ static void __exit hdaps_exit(void)
 	sysfs_remove_group(&pdev->dev.kobj, &hdaps_attribute_group);
 	platform_device_unregister(pdev);
 	platform_driver_unregister(&hdaps_driver);
-	release_region(HDAPS_LOW_PORT, HDAPS_NR_PORTS);
 
 	printk(KERN_INFO "hdaps: driver unloaded.\n");
 }
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -494,6 +494,7 @@ config SENSORS_W83627EHF
 config SENSORS_HDAPS
 	tristate "IBM Hard Drive Active Protection System (hdaps)"
 	depends on HWMON && INPUT && X86
+	select THINKPAD_EC
 	default n
 	help
 	  This driver provides support for the IBM Hard Drive Active Protection

[PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Shem Multinymous]

The current hdaps driver queries the hardware on (almost) any sysfs
read, reading just the information it needs and discarding the rest
This is inefficient, because every hardware query actually gives all
information. It also means we're losing data, because readouts are
offered by the hardware at a constant rate and each query "eats up"
a readout. It also results in unnecessarily complex code.

This patch moves all hardware value reading+parsing to a single
function, __hdaps_update(). All values are cached, and easily
referenced afterwards. This function is still invoked on every sysfs
read. This will be fixed in a later patch.

There's a minor race condition here (and and also in the original code):
the global int pos_x, rest_x etc. could be updated while an attribute's
show_* function is reading them. This will be fixed in a future patch.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |  173 ++++++++++++++++++++++++++------------------------
 1 file changed, 90 insertions(+), 83 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -63,73 +63,89 @@ static struct timer_list hdaps_timer;
 static struct platform_device *pdev;
 static struct input_dev *hdaps_idev;
 static unsigned int hdaps_invert;
+
+/* Latest state readout */
+static int pos_x, pos_y;   /* position */
+static int var_x, var_y;   /* variance (what is this?) */
+static u8 temp1, temp2;    /* temperatures */
 static u8 km_activity;
-static int rest_x;
-static int rest_y;
+static int rest_x, rest_y; /* calibrated rest position */
 
-/*
- * hdaps_readb_one - reads a byte from a single I/O port, placing the value in
- * the given pointer.  Returns zero on success or a negative error on failure.
- * Can sleep.
- */
-static int hdaps_readb_one(unsigned int port, u8 *val)
+/* Some models require an axis transformation to the standard reprsentation */
+void transform_axes(int *x, int *y)
 {
-	int ret;
-	struct thinkpad_ec_row data;
-
-	ret = thinkpad_ec_lock();
-	if (ret)
-		return ret;
-	data.mask = (1 << port);
-	ret = thinkpad_ec_read_row(&ec_accel_args, &data);
-	*val = data.val[port];
-	thinkpad_ec_unlock();
-	return ret;
+	if (hdaps_invert) {
+		*x = -*x;
+		*y = -*y;
+	}
 }
 
-/* __hdaps_read_pair - internal lockless helper for hdaps_read_pair(). */
-static int __hdaps_read_pair(unsigned int port1, unsigned int port2,
-			     int *x, int *y)
+/**
+ * __hdaps_update - query current state, with locks already acquired
+ * @fast: if nonzero, do one quick attempt without retries.
+ *
+ * Query current accelerometer state and update global state variables.
+ * Also prefetches the next query. Caller must hold controller lock.
+ */
+static int __hdaps_update(int fast)
 {
-	int ret;
+	/* Read data: */
 	struct thinkpad_ec_row data;
+	int ret;
 
-	data.mask = (3 << port1) | (3 << port2) | (1 << EC_ACCEL_IDX_KMACT);
-	ret = thinkpad_ec_read_row(&ec_accel_args, &data);
+	data.mask = (1 << EC_ACCEL_IDX_KMACT)    |
+	            (3 << EC_ACCEL_IDX_YPOS1)    | (3 << EC_ACCEL_IDX_XPOS1) |
+	            (3 << EC_ACCEL_IDX_YPOS2)    | (3 << EC_ACCEL_IDX_XPOS2) |
+	            (1 << EC_ACCEL_IDX_TEMP1)    | (1 << EC_ACCEL_IDX_TEMP2);
+	if (fast)
+		ret = thinkpad_ec_try_read_row(&ec_accel_args, &data);
+	else
+		ret = thinkpad_ec_read_row(&ec_accel_args, &data);
+	thinkpad_ec_prefetch_row(&ec_accel_args); /* Prefetch even if error */
 	if (ret)
 		return ret;
 
-	*x = *(s16*)(data.val+port1);
-	*y = *(s16*)(data.val+port2);
+	/* Parse position data: */
+	pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1);
+	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1);
+	transform_axes(&pos_x, &pos_y);
+
+	/* Parse so-called "variance" data: */
+	var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2);
+	var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2);
+	transform_axes(&var_x, &var_y);
+
 	km_activity = data.val[EC_ACCEL_IDX_KMACT];
 
-	/* if hdaps_invert is set, negate the two values */
-	if (hdaps_invert) {
-		*x = -*x;
-		*y = -*y;
-	}
+	temp1 = data.val[EC_ACCEL_IDX_TEMP1];
+	temp2 = data.val[EC_ACCEL_IDX_TEMP2];
 
 	return 0;
 }
 
-/*
- * hdaps_read_pair - reads the values from a pair of ports, placing the values
- * in the given pointers.  Returns zero on success.  Can sleep.
+/**
+ * hdaps_update - acquire locks and query current state
+ *
+ * Query current accelerometer state and update global state variables.
+ * Also prefetches the next query.
+ * Does its own locking.
  */
-static int hdaps_read_pair(unsigned int port1, unsigned int port2,
-			   int *val1, int *val2)
+static int hdaps_update(void)
 {
 	int ret = thinkpad_ec_lock();
 	if (ret)
 		return ret;
-	ret = __hdaps_read_pair(port1, port2, val1, val2);
+	ret = __hdaps_update(0);
 	thinkpad_ec_unlock();
 	return ret;
 }
 
-/*
- * hdaps_device_init - initialize the accelerometer.  Returns zero on success
- * and negative error code on failure.  Can sleep.
+/**
+ * hdaps_device_init - initialize the accelerometer.
+ *
+ * Call several embedded controller functions to test and initialize the
+ * accelerometer.
+ * Returns zero on success and negative error code on failure. Can sleep.
  */
 #define ABORT_INIT(msg) printk(KERN_ERR "hdaps init failed at: %s\n", msg)
 static int hdaps_device_init(void)
@@ -236,35 +252,43 @@ static struct platform_driver hdaps_driv
 	},
 };
 
-/*
- * hdaps_calibrate - Set our "resting" values.  Callers must hold hdaps_sem.
+/**
+ * hdaps_calibrate - set our "resting" values.
+ * Does its own locking.
  */
 static void hdaps_calibrate(void)
 {
-	__hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &rest_x, &rest_y);
+	hdaps_update();
+	rest_x = pos_x;
+	rest_y = pos_y;
 }
 
+/* Timer handler for updating the input device. Runs in softirq context,
+ * so avoid lenghty or blocking operations.
+ */
 static void hdaps_mousedev_poll(unsigned long unused)
 {
-	int x, y;
+	int ret;
 
 	/* Cannot sleep.  Try nonblockingly.  If we fail, try again later. */
-	if (thinkpad_ec_try_lock()) {
-		mod_timer(&hdaps_timer,jiffies + HDAPS_POLL_PERIOD);
+	if (thinkpad_ec_try_lock())
+		goto keep_active;
+
+	ret = __hdaps_update(1); /* fast update, we're in softirq context */
+	thinkpad_ec_unlock();
+	/* Any of "successful", "not yet ready" and "not prefetched"? */
+	if (ret!=0 && ret!=-EBUSY && ret!=-ENODATA) {
+		printk(KERN_ERR
+		       "hdaps: poll failed, disabling mousedev updates\n");
 		return;
 	}
 
-	if (__hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &x, &y))
-		goto out;
-
-	input_report_abs(hdaps_idev, ABS_X, x - rest_x);
-	input_report_abs(hdaps_idev, ABS_Y, y - rest_y);
+keep_active:
+	/* Even if we failed now, pos_x,y may have been updated earlier: */
+	input_report_abs(hdaps_idev, ABS_X, pos_x - rest_x);
+	input_report_abs(hdaps_idev, ABS_Y, pos_y - rest_y);
 	input_sync(hdaps_idev);
-
 	mod_timer(&hdaps_timer, jiffies + HDAPS_POLL_PERIOD);
-
-out:
-	thinkpad_ec_unlock();
 }
 
 
@@ -273,51 +297,37 @@ out:
 static ssize_t hdaps_position_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
-	int ret, x, y;
-
-	ret = hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &x, &y);
+	int ret = hdaps_update();
 	if (ret)
 		return ret;
-
-	return sprintf(buf, "(%d,%d)\n", x, y);
+	return sprintf(buf, "(%d,%d)\n", pos_x, pos_y);
 }
 
 static ssize_t hdaps_variance_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
-	int ret, x, y;
-
-	ret = hdaps_read_pair(EC_ACCEL_IDX_XPOS2, EC_ACCEL_IDX_YPOS2, &x, &y);
+	int ret = hdaps_update();
 	if (ret)
 		return ret;
-
-	return sprintf(buf, "(%d,%d)\n", x, y);
+	return sprintf(buf, "(%d,%d)\n", var_x, var_y);
 }
 
 static ssize_t hdaps_temp1_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	u8 temp;
-	int ret;
-
-	ret = hdaps_readb_one(EC_ACCEL_IDX_TEMP1, &temp);
-	if (ret < 0)
+	int ret = hdaps_update();
+	if (ret)
 		return ret;
-
-	return sprintf(buf, "%u\n", temp);
+	return sprintf(buf, "%u\n", temp1);
 }
 
 static ssize_t hdaps_temp2_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	u8 temp;
-	int ret;
-
-	ret = hdaps_readb_one(EC_ACCEL_IDX_TEMP2, &temp);
-	if (ret < 0)
+	int ret = hdaps_update();
+	if (ret)
 		return ret;
-
-	return sprintf(buf, "%u\n", temp);
+	return sprintf(buf, "%u\n", temp2);
 }
 
 static ssize_t hdaps_keyboard_activity_show(struct device *dev,
@@ -344,10 +354,7 @@ static ssize_t hdaps_calibrate_store(str
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
 {
-	if (thinkpad_ec_lock())
-		return -EIO;
 	hdaps_calibrate();
-	thinkpad_ec_unlock();
 	return count;
 }
 

[PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes

[Shem Multinymous]

The current hdaps driver got some details wrong about the result of
hardware queries. 

First, it fails to check a couple of status values.

Second, it turns out that the hardware will return up to two readouts:
the latest one and also the previous one if it was missed (host didn't
poll fast enough). The current driver wrongly interprets the latter as
a "variance". We have no use for that previous readout, so it should 
be ignored. The corresponding sysfs attributes, "temp2" and "variance"
are bogus and thus removed.

This patch adds proper status checking, and removes the "variance" and
"temp2" sysfs attributes which refer to the old readout.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   75 +++++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 40 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -49,12 +49,16 @@ static const struct thinkpad_ec_row ec_a
 #define EC_ACCEL_IDX_TEMP2	0xb	/*   device temperature in Celsius */
 #define EC_ACCEL_IDX_QUEUED	0xc	/* Number of queued readouts left */
 #define EC_ACCEL_IDX_KMACT	0xd	/* keyboard or mouse activity */
+#define EC_ACCEL_IDX_RETVAL	0xf	/* command return value, good=0x00 */
 
 #define KEYBD_MASK		0x20	/* set if keyboard activity */
 #define MOUSE_MASK		0x40	/* set if mouse activity */
 #define KEYBD_ISSET(n)		(!! (n & KEYBD_MASK))	/* keyboard used? */
 #define MOUSE_ISSET(n)		(!! (n & MOUSE_MASK))	/* mouse used? */
 
+#define READ_TIMEOUT_MSECS	100	/* wait this long for device read */
+#define RETRY_MSECS		3	/* retry delay */
+
 #define HDAPS_POLL_PERIOD	(HZ/20)	/* poll for input every 1/20s */
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
@@ -66,8 +70,7 @@ static unsigned int hdaps_invert;
 
 /* Latest state readout */
 static int pos_x, pos_y;   /* position */
-static int var_x, var_y;   /* variance (what is this?) */
-static u8 temp1, temp2;    /* temperatures */
+static int temperature;    /* temperature */
 static u8 km_activity;
 static int rest_x, rest_y; /* calibrated rest position */
 
@@ -93,10 +96,9 @@ static int __hdaps_update(int fast)
 	struct thinkpad_ec_row data;
 	int ret;
 
-	data.mask = (1 << EC_ACCEL_IDX_KMACT)    |
+	data.mask = (1 << EC_ACCEL_IDX_READOUTS) | (1 << EC_ACCEL_IDX_KMACT) |
 	            (3 << EC_ACCEL_IDX_YPOS1)    | (3 << EC_ACCEL_IDX_XPOS1) |
-	            (3 << EC_ACCEL_IDX_YPOS2)    | (3 << EC_ACCEL_IDX_XPOS2) |
-	            (1 << EC_ACCEL_IDX_TEMP1)    | (1 << EC_ACCEL_IDX_TEMP2);
+	            (1 << EC_ACCEL_IDX_TEMP1)    | (1 << EC_ACCEL_IDX_RETVAL);
 	if (fast)
 		ret = thinkpad_ec_try_read_row(&ec_accel_args, &data);
 	else
@@ -105,20 +107,24 @@ static int __hdaps_update(int fast)
 	if (ret)
 		return ret;
 
+	/* Check status: */
+	if (data.val[EC_ACCEL_IDX_RETVAL] != 0x00) {
+		printk(KERN_WARNING "hdaps: read RETVAL=0x%02x\n",
+		       data.val[EC_ACCEL_IDX_RETVAL]);
+		return -EIO;
+	}
+
+	if (data.val[EC_ACCEL_IDX_READOUTS] < 1)
+		return -EBUSY; /* no pending readout, try again later */
+
 	/* Parse position data: */
 	pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1);
 	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1);
 	transform_axes(&pos_x, &pos_y);
 
-	/* Parse so-called "variance" data: */
-	var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2);
-	var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2);
-	transform_axes(&var_x, &var_y);
-
 	km_activity = data.val[EC_ACCEL_IDX_KMACT];
 
-	temp1 = data.val[EC_ACCEL_IDX_TEMP1];
-	temp2 = data.val[EC_ACCEL_IDX_TEMP2];
+	temperature = data.val[EC_ACCEL_IDX_TEMP1];
 
 	return 0;
 }
@@ -128,15 +134,25 @@ static int __hdaps_update(int fast)
  *
  * Query current accelerometer state and update global state variables.
  * Also prefetches the next query.
+ * Retries until timeout if the accelerometer is not in ready status (common).
  * Does its own locking.
  */
 static int hdaps_update(void)
 {
-	int ret = thinkpad_ec_lock();
-	if (ret)
-		return ret;
-	ret = __hdaps_update(0);
-	thinkpad_ec_unlock();
+	int total, ret;
+	for (total=0; total<READ_TIMEOUT_MSECS; total+=RETRY_MSECS) {
+		ret = thinkpad_ec_lock();
+		if (ret)
+			return ret;
+		ret = __hdaps_update(0);
+		thinkpad_ec_unlock();
+
+		if (!ret)
+			return 0;
+		if (ret != -EBUSY)
+			break;
+		msleep(RETRY_MSECS);
+	}
 	return ret;
 }
 
@@ -303,31 +319,13 @@ static ssize_t hdaps_position_show(struc
 	return sprintf(buf, "(%d,%d)\n", pos_x, pos_y);
 }
 
-static ssize_t hdaps_variance_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	int ret = hdaps_update();
-	if (ret)
-		return ret;
-	return sprintf(buf, "(%d,%d)\n", var_x, var_y);
-}
-
 static ssize_t hdaps_temp1_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	int ret = hdaps_update();
 	if (ret)
 		return ret;
-	return sprintf(buf, "%u\n", temp1);
-}
-
-static ssize_t hdaps_temp2_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	int ret = hdaps_update();
-	if (ret)
-		return ret;
-	return sprintf(buf, "%u\n", temp2);
+	return sprintf(buf, "%d\n", temperature);
 }
 
 static ssize_t hdaps_keyboard_activity_show(struct device *dev,
@@ -380,9 +378,8 @@ static ssize_t hdaps_invert_store(struct
 }
 
 static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL);
-static DEVICE_ATTR(variance, 0444, hdaps_variance_show, NULL);
 static DEVICE_ATTR(temp1, 0444, hdaps_temp1_show, NULL);
-static DEVICE_ATTR(temp2, 0444, hdaps_temp2_show, NULL);
+  /* "temp1" instead of "temperature" is hwmon convention */
 static DEVICE_ATTR(keyboard_activity, 0444, hdaps_keyboard_activity_show, NULL);
 static DEVICE_ATTR(mouse_activity, 0444, hdaps_mouse_activity_show, NULL);
 static DEVICE_ATTR(calibrate, 0644, hdaps_calibrate_show,hdaps_calibrate_store);
@@ -390,9 +387,7 @@ static DEVICE_ATTR(invert, 0644, hdaps_i
 
 static struct attribute *hdaps_attributes[] = {
 	&dev_attr_position.attr,
-	&dev_attr_variance.attr,
 	&dev_attr_temp1.attr,
-	&dev_attr_temp2.attr,
 	&dev_attr_keyboard_activity.attr,
 	&dev_attr_mouse_activity.attr,
 	&dev_attr_calibrate.attr,

[PATCH 05/12] hdaps: Remember keyboard and mouse activity

[Shem Multinymous]

When the current hdaps driver is queried for recent keyboard/mouse activity
(which is provided by the hardware for use in disk parking daemons), it
simply returns the last readout. However, every hardware query resets the
activity flag in the hardware, and this is triggered by (almost) any
hdaps sysfs attribute read, so the flag could be reset before it is 
observed and is thus nearly useless.

This patch makes the activities flags persist for 0.1sec, by remembering
when was the last time we saw them set. This gives apps like the hdaps
daemon enough time to poll the flag.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -53,8 +53,6 @@ static const struct thinkpad_ec_row ec_a
 
 #define KEYBD_MASK		0x20	/* set if keyboard activity */
 #define MOUSE_MASK		0x40	/* set if mouse activity */
-#define KEYBD_ISSET(n)		(!! (n & KEYBD_MASK))	/* keyboard used? */
-#define MOUSE_ISSET(n)		(!! (n & MOUSE_MASK))	/* mouse used? */
 
 #define READ_TIMEOUT_MSECS	100	/* wait this long for device read */
 #define RETRY_MSECS		3	/* retry delay */
@@ -62,6 +60,7 @@ static const struct thinkpad_ec_row ec_a
 #define HDAPS_POLL_PERIOD	(HZ/20)	/* poll for input every 1/20s */
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
+#define KMACT_REMEMBER_PERIOD   (HZ/10) /* keyboard/mouse persistance */
 
 static struct timer_list hdaps_timer;
 static struct platform_device *pdev;
@@ -71,9 +70,12 @@ static unsigned int hdaps_invert;
 /* Latest state readout */
 static int pos_x, pos_y;   /* position */
 static int temperature;    /* temperature */
-static u8 km_activity;
 static int rest_x, rest_y; /* calibrated rest position */
 
+/* Last time we saw keyboard and mouse activity: */
+static u64 last_keyboard_jiffies = INITIAL_JIFFIES;
+static u64 last_mouse_jiffies = INITIAL_JIFFIES;
+
 /* Some models require an axis transformation to the standard reprsentation */
 void transform_axes(int *x, int *y)
 {
@@ -122,7 +124,14 @@ static int __hdaps_update(int fast)
 	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1);
 	transform_axes(&pos_x, &pos_y);
 
-	km_activity = data.val[EC_ACCEL_IDX_KMACT];
+	/* Keyboard and mouse activity status is cleared as soon as it's read,
+	 * so applications will eat each other's events. Thus we remember any
+	 * event for KMACT_REMEMBER_PERIOD jiffies.
+	 */
+	if (data.val[EC_ACCEL_IDX_KMACT] & KEYBD_MASK)
+		last_keyboard_jiffies = get_jiffies_64();
+	if (data.val[EC_ACCEL_IDX_KMACT] & MOUSE_MASK)
+		last_mouse_jiffies = get_jiffies_64();
 
 	temperature = data.val[EC_ACCEL_IDX_TEMP1];
 
@@ -332,14 +341,22 @@ static ssize_t hdaps_keyboard_activity_s
 					    struct device_attribute *attr,
 					    char *buf)
 {
-	return sprintf(buf, "%u\n", KEYBD_ISSET(km_activity));
+	int ret = hdaps_update();
+	if (ret)
+		return ret;
+	return sprintf(buf, "%u\n",
+	   get_jiffies_64() < last_keyboard_jiffies + KMACT_REMEMBER_PERIOD);
 }
 
 static ssize_t hdaps_mouse_activity_show(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
 {
-	return sprintf(buf, "%u\n", MOUSE_ISSET(km_activity));
+	int ret = hdaps_update();
+	if (ret)
+		return ret;
+	return sprintf(buf, "%u\n",
+	   get_jiffies_64() < last_mouse_jiffies + KMACT_REMEMBER_PERIOD);
 }
 
 static ssize_t hdaps_calibrate_show(struct device *dev,

[PATCH 06/12] hdaps: Limit hardware query rate

[Shem Multinymous]

The current hdaps driver queries the hardware on (almost) any sysfs read.
Since fresh readouts are genereated by the hardware at a constant rate,
this means apps are eating each other's events. Also, polling multiple
attributes will genereate excessive hardware queries and excessive CPU
load due to the duration of the hardware query transaction.

With this patch, the driver will normally update its cached readouts
only in its timer function (which exists anyway, for the input device).
If that read failed, it will be retried upon the actual sysfs access.
In all cases, query rate is bounded and apps will get reasonably
fresh and usually cached readouts.

The polling rate is increased to 50Hz, as needed by the hdaps daemon.
A later patch makes this configurable.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -57,7 +57,7 @@ static const struct thinkpad_ec_row ec_a
 #define READ_TIMEOUT_MSECS	100	/* wait this long for device read */
 #define RETRY_MSECS		3	/* retry delay */
 
-#define HDAPS_POLL_PERIOD	(HZ/20)	/* poll for input every 1/20s */
+#define HDAPS_POLL_PERIOD	(HZ/50)	/* poll for input every 1/50s */
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
 #define KMACT_REMEMBER_PERIOD   (HZ/10) /* keyboard/mouse persistance */
@@ -67,10 +67,11 @@ static struct platform_device *pdev;
 static struct input_dev *hdaps_idev;
 static unsigned int hdaps_invert;
 
-/* Latest state readout */
-static int pos_x, pos_y;   /* position */
-static int temperature;    /* temperature */
-static int rest_x, rest_y; /* calibrated rest position */
+/* Latest state readout: */
+static int pos_x, pos_y;      /* position */
+static int temperature;       /* temperature */
+static int stale_readout = 1; /* last read invalid */
+static int rest_x, rest_y;    /* calibrated rest position */
 
 /* Last time we saw keyboard and mouse activity: */
 static u64 last_keyboard_jiffies = INITIAL_JIFFIES;
@@ -135,6 +136,7 @@ static int __hdaps_update(int fast)
 
 	temperature = data.val[EC_ACCEL_IDX_TEMP1];
 
+	stale_readout = 0;
 	return 0;
 }
 
@@ -149,6 +151,8 @@ static int __hdaps_update(int fast)
 static int hdaps_update(void)
 {
 	int total, ret;
+	if (!stale_readout) /* already updated recently? */
+		return 0;
 	for (total=0; total<READ_TIMEOUT_MSECS; total+=RETRY_MSECS) {
 		ret = thinkpad_ec_lock();
 		if (ret)
@@ -244,6 +248,7 @@ bad:
 	thinkpad_ec_invalidate();
 	ret = -ENXIO;
 good:
+	stale_readout = 1;
 	thinkpad_ec_unlock();
 	return ret;
 }
@@ -295,6 +300,8 @@ static void hdaps_mousedev_poll(unsigned
 {
 	int ret;
 
+	stale_readout = 1;
+
 	/* Cannot sleep.  Try nonblockingly.  If we fail, try again later. */
 	if (thinkpad_ec_try_lock())
 		goto keep_active;
@@ -304,7 +311,7 @@ static void hdaps_mousedev_poll(unsigned
 	/* Any of "successful", "not yet ready" and "not prefetched"? */
 	if (ret!=0 && ret!=-EBUSY && ret!=-ENODATA) {
 		printk(KERN_ERR
-		       "hdaps: poll failed, disabling mousedev updates\n");
+		       "hdaps: poll failed, disabling updates\n");
 		return;
 	}
 

Re: [PATCH 06/12] hdaps: Limit hardware query rate

[Pavel Machek]

Hi!

> The polling rate is increased to 50Hz, as needed by the hdaps daemon.
> A later patch makes this configurable.

So you have a daemon that will actually protect the harddrive? Do you
have an url? That would be great, particulary for poor users of
misdesigned x41 (that seem to kill disk every 6 months without this
kind of protection...)
							Pavel

-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH 06/12] hdaps: Limit hardware query rate

[Evgeni Golov]

On Thu, 10 Aug 2006 21:26:47 +0000 Pavel Machek <pavel@suse.cz> wrote:

> Hi!
> 
> > The polling rate is increased to 50Hz, as needed by the hdaps
> > daemon. A later patch makes this configurable.
> 
> So you have a daemon that will actually protect the harddrive? Do you
> have an url? That would be great, particulary for poor users of
> misdesigned x41 (that seem to kill disk every 6 months without this
> kind of protection...)

There is a hdapsd. Look on
http://www.thinkwiki.org/wiki/How_to_protect_the_harddisk_through_APS

regards
Evgeni

-- 
   ^^^    | Evgeni -SargentD- Golov (sargentd@die-welt.net)
 d(O_o)b  | PGP-Key-ID: 0xAC15B50C
  >-|-<   | WWW: http://www.die-welt.net   ICQ: 54116744
   / \    | IRC: #sod @ irc.german-freakz.net

[PATCH 07/12] hdaps: delay calibration to first hardware query

[Shem Multinymous]

The hdaps driver currently calibrates its rest position upon
initialization, which can take several seconds on first module load
(and delays the boot process accordingly). This patch delays 
calibration to the first successful hardware query, when the
information is available anyway. Writes to the "calibrate" sysfs
attribute are handled likewise.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -66,6 +66,7 @@ static struct timer_list hdaps_timer;
 static struct platform_device *pdev;
 static struct input_dev *hdaps_idev;
 static unsigned int hdaps_invert;
+static int needs_calibration;
 
 /* Latest state readout: */
 static int pos_x, pos_y;      /* position */
@@ -137,6 +138,12 @@ static int __hdaps_update(int fast)
 	temperature = data.val[EC_ACCEL_IDX_TEMP1];
 
 	stale_readout = 0;
+	if (needs_calibration) {
+		rest_x = pos_x;
+		rest_y = pos_y;
+		needs_calibration = 0;
+	}
+
 	return 0;
 }
 
@@ -288,9 +295,9 @@ static struct platform_driver hdaps_driv
  */
 static void hdaps_calibrate(void)
 {
+	needs_calibration = 1;
 	hdaps_update();
-	rest_x = pos_x;
-	rest_y = pos_y;
+	/* If that fails, the mousedev poll will take care of things later. */
 }
 
 /* Timer handler for updating the input device. Runs in softirq context,
@@ -520,8 +527,8 @@ static int __init hdaps_init(void)
 		goto out_group;
 	}
 
-	/* initial calibrate for the input device */
-	hdaps_calibrate();
+	/* calibration for the input device (deferred to avoid delay) */
+	needs_calibration = 1;
 
 	/* initialize the input class */
 	hdaps_idev->name = "hdaps";

[PATCH 08/12] hdaps: Add explicit hardware configuration functions

[Shem Multinymous]

This adds functions for configuring accelerometer-related hardware 
parameters in the hdaps driver, and changes the init function to
use these functions instead of opaque magic numbers.
The parameters are configured via variables instead of constants
since a later patch will add sysfs attributes for changing them.

A few of these functions aren't used yet, but will be used by later
patches.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |  205 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 159 insertions(+), 46 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -57,7 +57,6 @@ static const struct thinkpad_ec_row ec_a
 #define READ_TIMEOUT_MSECS	100	/* wait this long for device read */
 #define RETRY_MSECS		3	/* retry delay */
 
-#define HDAPS_POLL_PERIOD	(HZ/50)	/* poll for input every 1/50s */
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
 #define KMACT_REMEMBER_PERIOD   (HZ/10) /* keyboard/mouse persistance */
@@ -68,6 +67,13 @@ static struct input_dev *hdaps_idev;
 static unsigned int hdaps_invert;
 static int needs_calibration;
 
+/* Configuration: */
+static int sampling_rate = 50;       /* Sampling rate  */
+static int oversampling_ratio = 5;   /* Ratio between our sampling rate and
+                                      * EC accelerometer sampling rate      */
+static int running_avg_filter_order = 2; /* EC running average filter order */
+static int fake_data_mode;           /* Enable EC fake data mode? */
+
 /* Latest state readout: */
 static int pos_x, pos_y;      /* position */
 static int temperature;       /* temperature */
@@ -177,6 +183,143 @@ static int hdaps_update(void)
 }
 
 /**
+ * hdaps_set_power - enable or disable power to the accelerometer.
+ * Returns zero on success and negative error code on failure.  Can sleep.
+ */
+static int hdaps_set_power(int on)
+{
+	struct thinkpad_ec_row args =
+		{ .mask=0x0003, .val={0x14, on?0x01:0x00} };
+	struct thinkpad_ec_row data = { .mask = 0x8000 };
+	int ret = thinkpad_ec_read_row(&args, &data);
+	if (ret)
+		return ret;
+	if (data.val[0xF]!=0x00)
+		return -EIO;
+	return 0;
+}
+
+/**
+ * hdaps_set_fake_data_mode - enable or disable EC test mode
+ * EC test mode fakes accelerometer data using an incrementing counter.
+ * Returns zero on success and negative error code on failure.  Can sleep.
+ */
+static int hdaps_set_fake_data_mode(int on)
+{
+	struct thinkpad_ec_row args =
+		{ .mask=0x0007, .val={0x17, 0x83, on?0x01:0x00} };
+	struct thinkpad_ec_row data = { .mask = 0x8000 };
+	int ret = thinkpad_ec_read_row(&args, &data);
+	if (ret)
+		return ret;
+	if (data.val[0xF]!=0x00) {
+		printk(KERN_WARNING "failed setting hdaps fake data to %d\n",
+		       on);
+		return -EIO;
+	}
+	printk(KERN_DEBUG "hdaps: fake_data_mode set to %d\n", on);
+	return 0;
+}
+
+/**
+ * hdaps_set_ec_config - set accelerometer parameters.
+ * @ec_rate: embedded controller sampling rate
+ * @order: embedded controller running average filter order
+ * (Normally we have @ec_rate = sampling_rate * oversampling_ratio.)
+ * Returns zero on success and negative error code on failure.  Can sleep.
+ */
+static int hdaps_set_ec_config(int ec_rate, int order)
+{
+	struct thinkpad_ec_row args = { .mask=0x000F,
+		.val={0x10, (u8)ec_rate, (u8)(ec_rate>>8), order} };
+	struct thinkpad_ec_row data = { .mask = 0x8000 };
+	int ret = thinkpad_ec_read_row(&args, &data);
+	printk(KERN_DEBUG "hdaps: setting ec_rate=%d, filter_order=%d\n",
+	       ec_rate, order);
+	if (ret)
+		return ret;
+	if (data.val[0xF]==0x03) {
+		printk(KERN_WARNING "hdaps: config param out of range\n");
+		return -EINVAL;
+	}
+	if (data.val[0xF]==0x06) {
+		printk(KERN_WARNING "hdaps: config change already pending\n");
+		return -EBUSY;
+	}
+	if (data.val[0xF]!=0x00) {
+		printk(KERN_WARNING "hdaps: config change error, ret=%d\n",
+		      data.val[0xF]);
+		return -EIO;
+	}
+	return 0;
+}
+
+/**
+ * hdaps_get_ec_config - get accelerometer parameters.
+ * @ec_rate: embedded controller sampling rate
+ * @order: embedded controller running average filter order
+ * Returns zero on success and negative error code on failure.  Can sleep.
+ */
+static int hdaps_get_ec_config(int *ec_rate, int *order)
+{
+	const struct thinkpad_ec_row args =
+		{ .mask=0x0003, .val={0x17, 0x82} };
+	struct thinkpad_ec_row data = { .mask = 0x801F };
+	int ret = thinkpad_ec_read_row(&args, &data);
+	if (ret)
+		return ret;
+	if (data.val[0xF]!=0x00)
+		return -EIO;
+	if (!(data.val[0x1] & 0x01))
+		return -ENXIO; /* accelerometer polling not enabled */
+	if (data.val[0x1] & 0x02)
+		return -EBUSY; /* config change in progress, retry later */
+	*ec_rate = data.val[0x2] | ((int)(data.val[0x3]) << 8);
+	*order = data.val[0x4];
+	return 0;
+}
+
+/**
+ * hdaps_get_ec_mode - get EC accelerometer mode
+ * Returns zero on success and negative error code on failure.  Can sleep.
+ */
+static int hdaps_get_ec_mode(u8 *mode)
+{
+	const struct thinkpad_ec_row args = { .mask=0x0001, .val={0x13} };
+	struct thinkpad_ec_row data = { .mask = 0x8002 };
+	int ret = thinkpad_ec_read_row(&args, &data);
+	if (ret)
+		return ret;
+	if (data.val[0xF]!=0x00) {
+		printk(KERN_WARNING
+		       "accelerometer not implemented (0x%02x)\n",
+		       data.val[0xF]);
+		return -EIO;
+	}
+	*mode = data.val[0x1];
+	return 0;
+}
+
+/**
+ * hdaps_check_ec - checks something about the EC.
+ * Follows the clean-room spec for HDAPS; we don't know what it means.
+ * Returns zero on success and negative error code on failure.  Can sleep.
+ */
+static int __init hdaps_check_ec(u8 *mode)
+{
+	const struct thinkpad_ec_row args =
+		{ .mask=0x0003, .val={0x17, 0x81} };
+	struct thinkpad_ec_row data = { .mask = 0x800E };
+	int ret = thinkpad_ec_read_row(&args, &data);
+	if (ret)
+		return  ret;
+	if (data.val[0x1]!=0x00 || data.val[0x2]!=0x60 ||
+	    data.val[0x3]!=0x00 || data.val[0xF]!=0x00)
+		return -EIO;
+	return 0;
+}
+
+/**
  * hdaps_device_init - initialize the accelerometer.
  *
  * Call several embedded controller functions to test and initialize the
@@ -187,61 +330,31 @@ static int hdaps_update(void)
 static int hdaps_device_init(void)
 {
 	int ret;
-	struct thinkpad_ec_row args, data;
 	u8 mode;
 
 	ret = thinkpad_ec_lock();
 	if (ret)
 		return ret;
 
-	args.val[0x0] = 0x13;
-	args.val[0xF] = 0x01;
-	args.mask=0x8001;
-	data.mask=0x8002;
-	if (thinkpad_ec_read_row(&args, &data))
-		{ ABORT_INIT("read1"); goto bad; }
-	if (data.val[0xF]!=0x00)
-		{ ABORT_INIT("check1"); goto bad; }
-	mode = data.val[0x1];
+	if (hdaps_get_ec_mode(&mode))
+		{ ABORT_INIT("hdaps_get_ec_mode failed"); goto bad; }
 
 	printk(KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n", mode);
 	if (mode==0x00)
 		{ ABORT_INIT("accelerometer not available"); goto bad; }
 
-	args.val[0x0] = 0x17;
-	args.val[0x1] = 0x81;
-	args.val[0xF] = 0x01;
-	args.mask = 0x8003;
-	data.mask = 0x800E;
-	if (thinkpad_ec_read_row(&args, &data))
-		{ ABORT_INIT("read2"); goto bad; }
-	if (data.val[0x1]!=0x00 ||
-	    data.val[0x2]!=0x60 ||
-	    data.val[0x3]!=0x00 ||
-	    data.val[0xF]!=0x00)
-		{ ABORT_INIT("check2"); goto bad; }
-
-	args.val[0x0] = 0x14;
-	args.val[0x1] = 0x01;
-	args.val[0xF] = 0x01;
-	args.mask = 0x8003;
-	data.mask = 0x8000;
-	if (thinkpad_ec_read_row(&args, &data))
-		{ ABORT_INIT("read3"); goto bad; }
-	if (data.val[0xF]!=0x00)
-		{ ABORT_INIT("check3"); goto bad; }
+	if (hdaps_check_ec(&mode))
+		{ ABORT_INIT("hdaps_check_ec failed"); goto bad; }
 
-	args.val[0x0] = 0x10;
-	args.val[0x1] = 0xc8;
-	args.val[0x2] = 0x00;
-	args.val[0x3] = 0x02;
-	args.val[0xF] = 0x01;
-	args.mask = 0x800F;
-	data.mask = 0x8000;
-	if (thinkpad_ec_read_row(&args, &data))
-		{ ABORT_INIT("read4"); goto bad; }
-	if (data.val[0xF]!=0x00)
-		{ ABORT_INIT("check4"); goto bad; }
+	if (hdaps_set_power(1))
+		{ ABORT_INIT("hdaps_set_power failed"); goto bad; }
+
+	if (hdaps_set_ec_config(sampling_rate*oversampling_ratio,
+	                        running_avg_filter_order))
+		{ ABORT_INIT("hdaps_set_ec_config failed"); goto bad; }
+
+	if (hdaps_set_fake_data_mode(fake_data_mode))
+		{ ABORT_INIT("hdaps_set_fake_data_mode failed"); goto bad; }
 
 	thinkpad_ec_invalidate();
 	udelay(200);
@@ -327,7 +440,7 @@ keep_active:
 	input_report_abs(hdaps_idev, ABS_X, pos_x - rest_x);
 	input_report_abs(hdaps_idev, ABS_Y, pos_y - rest_y);
 	input_sync(hdaps_idev);
-	mod_timer(&hdaps_timer, jiffies + HDAPS_POLL_PERIOD);
+	mod_timer(&hdaps_timer, jiffies + HZ/sampling_rate);
 }
 
 
@@ -544,7 +657,7 @@ static int __init hdaps_init(void)
 	/* start up our timer for the input device */
 	init_timer(&hdaps_timer);
 	hdaps_timer.function = hdaps_mousedev_poll;
-	hdaps_timer.expires = jiffies + HDAPS_POLL_PERIOD;
+	hdaps_timer.expires = jiffies + HZ/sampling_rate;
 	add_timer(&hdaps_timer);
 
 	printk(KERN_INFO "hdaps: driver successfully loaded.\n");

[PATCH 09/12] hdaps: Add new sysfs attributes

[Shem Multinymous]

This patch adds 4 new r/w sysfs attributes to the hdaps driver:

/sys/devices/platform/hdaps/sampling_rate:
  This determines the frequency of hardware queries and input device updates.
  Default=50.
/sys/devices/platform/hdaps/oversampling_ratio:
  When set to X, the embedded controller is told to do physical accelerometer
  measurements at a rate that is X times higher than the rate at which
  the driver reads those measurements (i.e., X*sampling_rate). This
  reduces sample phase difference is, and useful for the running average
  filter (see next). Default=5
/sys/devices/platform/hdaps/running_avg_filter_order:
  When set to X, reported readouts will be the average of the last X physical
  accelerometer measurements. Current firmware allows 1<=X<=8. Setting to a
  high value decreases readout fluctuations. The averaging is handled
  by the embedded controller, so no CPU resources are used. Default=2.
/sys/devices/platform/hdaps/fake_data_mode:
  If set to 1, enables a test mode where the physical accelerometer readouts
  are replaced with an incrementing counter. This is useful for checking the
  regularity of the sampling interval and driver<->userspace communication,
  which is useful for development and testing of the hdapd userspace daemon.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |  107 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -521,6 +521,99 @@ static ssize_t hdaps_invert_store(struct
 	return count;
 }
 
+static ssize_t hdaps_sampling_rate_show(
+	struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", sampling_rate);
+}
+
+static ssize_t hdaps_sampling_rate_store(
+	struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	int rate, ret;
+	if (sscanf(buf, "%d", &rate) != 1 || rate>HZ || rate<0) {
+		printk(KERN_WARNING
+		       "must have 0<input_sampling_rate<=HZ=%d\n", HZ);
+		return -EINVAL;
+	}
+	ret = hdaps_set_ec_config(rate*oversampling_ratio,
+	                          running_avg_filter_order);
+	if (ret)
+		return ret;
+	sampling_rate = rate;
+	return count;
+}
+
+static ssize_t hdaps_oversampling_ratio_show(
+	struct device *dev, struct device_attribute *attr, char *buf)
+{
+	int ec_rate, order;
+	int ret = hdaps_get_ec_config(&ec_rate, &order);
+	if (ret)
+		return ret;
+	return sprintf(buf, "%u\n", ec_rate / sampling_rate);
+}
+
+static ssize_t hdaps_oversampling_ratio_store(
+	struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	int ratio, ret;
+	if (sscanf(buf, "%d", &ratio) != 1 || ratio<1)
+		return -EINVAL;
+	ret = hdaps_set_ec_config(sampling_rate*ratio,
+	                          running_avg_filter_order);
+	if (ret)
+		return ret;
+	oversampling_ratio = ratio;
+	return count;
+}
+
+static ssize_t hdaps_running_avg_filter_order_show(
+	struct device *dev, struct device_attribute *attr, char *buf)
+{
+	int rate, order;
+	int ret = hdaps_get_ec_config(&rate, &order);
+	if (ret)
+		return ret;
+	return sprintf(buf, "%u\n", order);
+}
+
+static ssize_t hdaps_running_avg_filter_order_store(
+	struct device *dev, struct device_attribute *attr,
+	const char *buf, size_t count)
+{
+	int order, ret;
+	if (sscanf(buf, "%d", &order) != 1)
+		return -EINVAL;
+	ret = hdaps_set_ec_config(sampling_rate*oversampling_ratio, order);
+	if (ret)
+		return ret;
+	running_avg_filter_order = order;
+	return count;
+}
+
+static ssize_t hdaps_fake_data_mode_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	int on, ret;
+	if (sscanf(buf, "%d", &on) != 1 || on<0 || on>1)
+		return -EINVAL;
+	ret = hdaps_set_fake_data_mode(on);
+	if (ret)
+		return ret;
+	fake_data_mode = on;
+	return count;
+}
+
+static ssize_t hdaps_fake_data_mode_show(
+	struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", fake_data_mode);
+}
+
 static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL);
 static DEVICE_ATTR(temp1, 0444, hdaps_temp1_show, NULL);
   /* "temp1" instead of "temperature" is hwmon convention */
@@ -528,6 +621,16 @@ static DEVICE_ATTR(keyboard_activity, 04
 static DEVICE_ATTR(mouse_activity, 0444, hdaps_mouse_activity_show, NULL);
 static DEVICE_ATTR(calibrate, 0644, hdaps_calibrate_show,hdaps_calibrate_store);
 static DEVICE_ATTR(invert, 0644, hdaps_invert_show, hdaps_invert_store);
+static DEVICE_ATTR(sampling_rate, 0644,
+		hdaps_sampling_rate_show, hdaps_sampling_rate_store);
+static DEVICE_ATTR(oversampling_ratio, 0644,
+		   hdaps_oversampling_ratio_show,
+		   hdaps_oversampling_ratio_store);
+static DEVICE_ATTR(running_avg_filter_order, 0644,
+		hdaps_running_avg_filter_order_show,
+		hdaps_running_avg_filter_order_store);
+static DEVICE_ATTR(fake_data_mode, 0644,
+		   hdaps_fake_data_mode_show, hdaps_fake_data_mode_store);
 
 static struct attribute *hdaps_attributes[] = {
 	&dev_attr_position.attr,
@@ -536,6 +639,10 @@ static struct attribute *hdaps_attribute
 	&dev_attr_mouse_activity.attr,
 	&dev_attr_calibrate.attr,
 	&dev_attr_invert.attr,
+	&dev_attr_sampling_rate.attr,
+	&dev_attr_oversampling_ratio.attr,
+	&dev_attr_running_avg_filter_order.attr,
+	&dev_attr_fake_data_mode.attr,
 	NULL,
 };
 

[PATCH 10/12] hdaps: Power off accelerometer on suspend and unload

[Shem Multinymous]

This patch disables accelerometer power and stops its polling by the
embedded controller upon suspend and module unload. The power saving
is negligible, but it's the right thing to do.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -373,6 +373,23 @@ good:
 	return ret;
 }
 
+/**
+ * hdaps_device_shutdown - power off the accelerometer
+ * Returns nonzero on failure. Can sleep.
+ */
+static int hdaps_device_shutdown(void)
+{
+	int ret;
+	ret = hdaps_set_power(0);
+	if (ret) {
+		printk(KERN_WARNING "hdaps: cannot power off\n");
+		return ret;
+	}
+	ret = hdaps_set_ec_config(0, 1);
+	if (ret)
+		printk(KERN_WARNING "hdaps: cannot stop EC sampling\n");
+	return ret;
+}
 
 /* Device model stuff */
 
@@ -388,6 +405,12 @@ static int hdaps_probe(struct platform_d
 	return 0;
 }
 
+static int hdaps_suspend(struct platform_device *dev, pm_message_t state)
+{
+	hdaps_device_shutdown(); /* ignore errors, effect is negligible */
+	return 0;
+}
+
 static int hdaps_resume(struct platform_device *dev)
 {
 	return hdaps_device_init();
@@ -395,6 +418,7 @@ static int hdaps_resume(struct platform_
 
 static struct platform_driver hdaps_driver = {
 	.probe = hdaps_probe,
+	.suspend = hdaps_suspend,
 	.resume = hdaps_resume,
 	.driver	= {
 		.name = "hdaps",
@@ -776,6 +800,7 @@ out_device:
 	platform_device_unregister(pdev);
 out_driver:
 	platform_driver_unregister(&hdaps_driver);
+	hdaps_device_shutdown();
 out:
 	printk(KERN_WARNING "hdaps: driver init failed (ret=%d)!\n", ret);
 	return ret;
@@ -785,6 +810,7 @@ static void __exit hdaps_exit(void)
 {
 	del_timer_sync(&hdaps_timer);
 	input_unregister_device(hdaps_idev);
+	hdaps_device_shutdown(); /* ignore errors, effect is negligible */
 	sysfs_remove_group(&pdev->dev.kobj, &hdaps_attribute_group);
 	platform_device_unregister(pdev);
 	platform_driver_unregister(&hdaps_driver);

[PATCH 11/12] hdaps: Stop polling timer when suspended

[Shem Multinymous]

This patch stops the hdaps driver's polling timer when the module is 
suspended. Accessing a shut-down accelerometer is not harmful, but 
let's avoid it anyway.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -407,13 +407,19 @@ static int hdaps_probe(struct platform_d
 
 static int hdaps_suspend(struct platform_device *dev, pm_message_t state)
 {
+	/* Don't do hdaps polls until resume re-initializes the sensor. */
+	del_timer_sync(&hdaps_timer);
 	hdaps_device_shutdown(); /* ignore errors, effect is negligible */
 	return 0;
 }
 
 static int hdaps_resume(struct platform_device *dev)
 {
-	return hdaps_device_init();
+	int ret = hdaps_device_init();
+	if (ret)
+		return ret;
+	mod_timer(&hdaps_timer, jiffies + HZ/sampling_rate);
+	return 0;
 }
 
 static struct platform_driver hdaps_driver = {
@@ -751,6 +757,9 @@ static int __init hdaps_init(void)
 		goto out;
 	}
 
+	/* Init timer before platform_driver_register, in case of suspend */
+	init_timer(&hdaps_timer);
+	hdaps_timer.function = hdaps_mousedev_poll;
 	ret = platform_driver_register(&hdaps_driver);
 	if (ret)
 		goto out;
@@ -785,11 +794,7 @@ static int __init hdaps_init(void)
 
 	input_register_device(hdaps_idev);
 
-	/* start up our timer for the input device */
-	init_timer(&hdaps_timer);
-	hdaps_timer.function = hdaps_mousedev_poll;
-	hdaps_timer.expires = jiffies + HZ/sampling_rate;
-	add_timer(&hdaps_timer);
+	mod_timer(&hdaps_timer, jiffies + HZ/sampling_rate);
 
 	printk(KERN_INFO "hdaps: driver successfully loaded.\n");
 	return 0;

[PATCH 12/12] hdaps: Simplify whitelist

[Shem Multinymous]

The hdaps driver can now reliably detect accelerometer hardware, as a 
free bonus from switch to thinkpad_ec. The whitelist is thus needed 
only for overriding the default axis configuratrion. This patch simplifies
the whitelist to reflect this. Behavior on previously working models is
completely unaffected, and new models will work automatically (though not
necessarily with correct axis configuration).

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>
---
 drivers/hwmon/hdaps.c |   66 ++++++++++----------------------------------------
 1 file changed, 14 insertions(+), 52 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -683,79 +683,41 @@ static struct attribute_group hdaps_attr
 
 /* Module stuff */
 
-/* hdaps_dmi_match - found a match.  return one, short-circuiting the hunt. */
-static int hdaps_dmi_match(struct dmi_system_id *id)
-{
-	printk(KERN_INFO "hdaps: %s detected.\n", id->ident);
-	return 1;
-}
-
 /* hdaps_dmi_match_invert - found an inverted match. */
 static int hdaps_dmi_match_invert(struct dmi_system_id *id)
 {
 	hdaps_invert = 1;
-	printk(KERN_INFO "hdaps: inverting axis readings.\n");
-	return hdaps_dmi_match(id);
-}
-
-#define HDAPS_DMI_MATCH_NORMAL(model)	{		\
-	.ident = "IBM " model,				\
-	.callback = hdaps_dmi_match,			\
-	.matches = {					\
-		DMI_MATCH(DMI_BOARD_VENDOR, "IBM"),	\
-		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
-	}						\
+	printk(KERN_INFO "hdaps: %s detected, inverting axes\n",
+	       id->ident);
+	return 1;
 }
 
-#define HDAPS_DMI_MATCH_INVERT(model)	{		\
-	.ident = "IBM " model,				\
+#define HDAPS_DMI_MATCH_INVERT(vendor,model)	{	\
+	.ident = vendor " " model,			\
 	.callback = hdaps_dmi_match_invert,		\
 	.matches = {					\
-		DMI_MATCH(DMI_BOARD_VENDOR, "IBM"),	\
+		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
 		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
 	}						\
 }
 
-#define HDAPS_DMI_MATCH_LENOVO(model)   {               \
-        .ident = "Lenovo " model,                       \
-        .callback = hdaps_dmi_match_invert,             \
-        .matches = {                                    \
-                DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"),  \
-                DMI_MATCH(DMI_PRODUCT_VERSION, model)   \
-        }                                               \
-}
-
 static int __init hdaps_init(void)
 {
 	int ret;
 
-	/* Note that HDAPS_DMI_MATCH_NORMAL("ThinkPad T42") would match
+	/* List of models with abnormal axis configuration.
+	   Note that HDAPS_DMI_MATCH_NORMAL("ThinkPad T42") would match
 	  "ThinkPad T42p", so the order of the entries matters */
 	struct dmi_system_id hdaps_whitelist[] = {
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad H"),
-		HDAPS_DMI_MATCH_INVERT("ThinkPad R50p"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad R50"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad R51"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad R52"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad H"),	 /* R52 (1846AQG) */
-		HDAPS_DMI_MATCH_INVERT("ThinkPad T41p"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad T41"),
-		HDAPS_DMI_MATCH_INVERT("ThinkPad T42p"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad T42"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad T43"),
-		HDAPS_DMI_MATCH_LENOVO("ThinkPad T60p"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad X40"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad X41"),
-		HDAPS_DMI_MATCH_LENOVO("ThinkPad X60"),
-		HDAPS_DMI_MATCH_NORMAL("ThinkPad Z60m"),
+		HDAPS_DMI_MATCH_INVERT("IBM","ThinkPad R50p"),
+		HDAPS_DMI_MATCH_INVERT("IBM","ThinkPad T41p"),
+		HDAPS_DMI_MATCH_INVERT("IBM","ThinkPad T42p"),
+		HDAPS_DMI_MATCH_INVERT("LENOVO","ThinkPad T60p"),
+		HDAPS_DMI_MATCH_INVERT("LENOVO","ThinkPad X60"),
 		{ .ident = NULL }
 	};
 
-	if (!dmi_check_system(hdaps_whitelist)) {
-		printk(KERN_WARNING "hdaps: supported laptop not found!\n");
-		ret = -ENODEV;
-		goto out;
-	}
+	dmi_check_system(hdaps_whitelist); /* default to normal axes */
 
 	/* Init timer before platform_driver_register, in case of suspend */
 	init_timer(&hdaps_timer);

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Robert Love]

On 8/10/06, Shem Multinymous <multinymous@gmail.com> wrote:
> This resubmission addresses all the comments I've received on LKML,
> except where said otherwise in reply. Other changes are very minor.

I reviewed this second patchset will sign-off once in this email, for
all 12 patches in the patchset.

Patches look great and I am glad someone has apparently better access
to hardware specs than I did.

Signed-off-by: Robert Love <rml@novell.com>

        Robert Love

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Robert Love]

On 8/10/06, Robert Love <rlove@rlove.org> wrote:

> I reviewed this second patchset will sign-off once in this email, for
> all 12 patches in the patchset.
>
> Patches look great and I am glad someone has apparently better access
> to hardware specs than I did.
>
> Signed-off-by: Robert Love <rml@novell.com>

This should be Ack-ed by as I was not involved in the code's
development; I only performed a cursory review, in which the code
looks good.

Acked-by: Robert Love <rml@novell.com>

       Robert Love

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Andrew Morton]

On Thu, 10 Aug 2006 09:46:47 -0400
"Robert Love" <rlove@rlove.org> wrote:

> Patches look great and I am glad someone has apparently better access
> to hardware specs than I did.

This situation is still a concern.  From where did this additional register
information come?

Was it reverse-engineered?  If so, by whom and how can we satisfy ourselves
of this?

Was it from published documents?

Was it improperly obtained from NDA'ed documentation?

Presumably the answer to the third question will be "no", but if
challenged, how can we defend this assertion?

So hm.  We're setting precedent here and we need Linus around to resolve
this.  Perhaps we can ask "Shem" to reveal his true identity to Linus (and
maybe me) privately and then we proceed on that basis.  The rule could be
"each of the Signed-off-by:ers should know the identity of the others".

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Greg KH]

On Thu, Aug 10, 2006 at 01:18:20PM -0700, Andrew Morton wrote:
> On Thu, 10 Aug 2006 09:46:47 -0400
> "Robert Love" <rlove@rlove.org> wrote:
> 
> > Patches look great and I am glad someone has apparently better access
> > to hardware specs than I did.
> 
> This situation is still a concern.  From where did this additional register
> information come?
> 
> Was it reverse-engineered?  If so, by whom and how can we satisfy ourselves
> of this?
> 
> Was it from published documents?
> 
> Was it improperly obtained from NDA'ed documentation?
> 
> Presumably the answer to the third question will be "no", but if
> challenged, how can we defend this assertion?
> 
> So hm.  We're setting precedent here and we need Linus around to resolve
> this.  Perhaps we can ask "Shem" to reveal his true identity to Linus (and
> maybe me) privately and then we proceed on that basis.  The rule could be
> "each of the Signed-off-by:ers should know the identity of the others".

For what it's worth, I'm not going to be handling these patches at all
(normally the hwmon patches go to Linus through Jean and then through
me.)

If the original developer does not want to work in the open like the
rest of us, I can respect that, but unfortunatly I can't accept the risk
of accepting their code.

And no, this is not "been beaten over the head by IP lawyers for three
years about risks like this" portion of me talking, although that side
does have a lot he could say about this situation...

thanks,

greg k-h

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Jean Delvare]

All,

> On Thu, Aug 10, 2006 at 01:18:20PM -0700, Andrew Morton wrote:
> > On Thu, 10 Aug 2006 09:46:47 -0400
> > "Robert Love" <rlove@rlove.org> wrote:
> > 
> > > Patches look great and I am glad someone has apparently better access
> > > to hardware specs than I did.
> > 
> > This situation is still a concern.  From where did this additional register
> > information come?
> > 
> > Was it reverse-engineered?  If so, by whom and how can we satisfy ourselves
> > of this?
> > 
> > Was it from published documents?
> > 
> > Was it improperly obtained from NDA'ed documentation?
> > 
> > Presumably the answer to the third question will be "no", but if
> > challenged, how can we defend this assertion?
> > 
> > So hm.  We're setting precedent here and we need Linus around to resolve
> > this.  Perhaps we can ask "Shem" to reveal his true identity to Linus (and
> > maybe me) privately and then we proceed on that basis.  The rule could be
> > "each of the Signed-off-by:ers should know the identity of the others".
> 
> For what it's worth, I'm not going to be handling these patches at all
> (normally the hwmon patches go to Linus through Jean and then through
> me.)

I said it in private before, and I say it publicly again: I am not
handling these patches either. I don't even want to read them.

> If the original developer does not want to work in the open like the
> rest of us, I can respect that, but unfortunatly I can't accept the risk
> of accepting their code.
> 
> And no, this is not "been beaten over the head by IP lawyers for three
> years about risks like this" portion of me talking, although that side
> does have a lot he could say about this situation...

As far as I am concerned, even the real name of the person who wrote
and sent these patches wouldn't be enough for me to take them. I would
ask for an explanation of how that person got access to information
about the HDAPS which even the original author of the driver didn't
know about. And I would ask for proofs of that explanation.

All this is very unlikely to happen as I understand it, and anyway it's
all too late. Regardless of its technical merits, this patchset has too
much legal uncertainty attached to it by now. I'm maintaining the hwmon
subsystem on my own time and money, it's painful and unrewarding enough
as it is without adding legal hazard into the picture.

-- 
Jean Delvare

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Jesper Juhl]

On 10/08/06, Jean Delvare <khali@linux-fr.org> wrote:
> All,
>
> > On Thu, Aug 10, 2006 at 01:18:20PM -0700, Andrew Morton wrote:
> > > On Thu, 10 Aug 2006 09:46:47 -0400
> > > "Robert Love" <rlove@rlove.org> wrote:
> > >
> > > > Patches look great and I am glad someone has apparently better access
> > > > to hardware specs than I did.
> > >
> > > This situation is still a concern.  From where did this additional register
> > > information come?
> > >
> > > Was it reverse-engineered?  If so, by whom and how can we satisfy ourselves
> > > of this?
> > >
> > > Was it from published documents?
> > >
> > > Was it improperly obtained from NDA'ed documentation?
> > >
> > > Presumably the answer to the third question will be "no", but if
> > > challenged, how can we defend this assertion?
> > >
> > > So hm.  We're setting precedent here and we need Linus around to resolve
> > > this.  Perhaps we can ask "Shem" to reveal his true identity to Linus (and
> > > maybe me) privately and then we proceed on that basis.  The rule could be
> > > "each of the Signed-off-by:ers should know the identity of the others".
> >
> > For what it's worth, I'm not going to be handling these patches at all
> > (normally the hwmon patches go to Linus through Jean and then through
> > me.)
>
> I said it in private before, and I say it publicly again: I am not
> handling these patches either. I don't even want to read them.
>
> > If the original developer does not want to work in the open like the
> > rest of us, I can respect that, but unfortunatly I can't accept the risk
> > of accepting their code.
> >
> > And no, this is not "been beaten over the head by IP lawyers for three
> > years about risks like this" portion of me talking, although that side
> > does have a lot he could say about this situation...
>
> As far as I am concerned, even the real name of the person who wrote
> and sent these patches wouldn't be enough for me to take them. I would
> ask for an explanation of how that person got access to information
> about the HDAPS which even the original author of the driver didn't
> know about. And I would ask for proofs of that explanation.
>

As the person who wrote the initial version of the driver (not the
only one, other people wrote versions at the same time, but mine was
the one that was the basis for what Robert took over and what got
merged in mainline) I'd also be interrested in where this person got
his/her info from. I looked long and hard for docs and send lots of
emails to IBM and others requesting info (unsuccessfully), all I could
ever find was http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html
 which is then what I used to write the initial driver from.


> All this is very unlikely to happen as I understand it, and anyway it's
> all too late. Regardless of its technical merits, this patchset has too
> much legal uncertainty attached to it by now. I'm maintaining the hwmon
> subsystem on my own time and money, it's painful and unrewarding enough
> as it is without adding legal hazard into the picture.
>
Maybe if the person behind these patches could do the following

1) Come out with his/her real name.
2) Provide detailed information about what sources of info these
patches are based on.
3) Provide names and contact info for people at IBM/Lenovo/the company
who made the gyroscope (can't remember who they are) etc etc, so that
we can contact them and verify these patches are based on stuff that's
OK.

then we could accept the patches...


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Shem Multinymous]

Hi,

On 8/11/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:

> 1) Come out with his/her real name.

This won't happen anytime soon (I hope the people who will inevitably
reveal it will respect my wish to keep that private). But I'm working
with Linus, Andrew and a few other developers to find a workable
alternative, such as entrusting my full identity with them.


> 2) Provide detailed information about what sources of info these
> patches are based on.

I have done so.


> 3) Provide names and contact info for people at IBM/Lenovo/the company
> who made the gyroscope (can't remember who they are) etc etc, so that
> we can contact them and verify these patches are based on stuff that's
> OK.

This is ludicrous. I have no idea who these people are, nor any way to
reach them, nor any reason to do so (other than to ask them nicely to
release their specs properly next time and stop wasting everybody's
time). Since when do we have to ask vendor's permission to run Linux
on their hardware?!

BTW, the consensus on Google is that the accelerometer is a ADXL320 by
Analog Devices, whose specs are at [1], but the driver doesn't care or
know about this, since it's talking to the H8S embedded controller
which *drives* the accelerometer.

  Shem

[1] http://www.analog.com/en/prod/0,2877,ADXL320,00.html

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Shem Multinymous]

Hi Jean,

On 8/11/06, Jean Delvare <khali@linux-fr.org> wrote:
> I would
> ask for an explanation of how that person got access to information
> about the HDAPS which even the original author of the driver didn't
> know about. And I would ask for proofs of that explanation.
> All this is very unlikely to happen as I understand it,

The mystery is rather simpler and less sinister than you imply, and
I've already explained it before (e.g., in the first few lines of the
code you say you don't want to read). But let me explain again in
detail:

The original author of the APS spec [1] reversed-engineered the
Windows driver, so he only saw what that driver was doing under
nominal conditions. Jesper Juhl, the original author the mainline
hdaps code, closely followed that spec (a prudent thing to do if you
know nothing about the hardware), so was subject to the same
limitation. However, once someone took apart his ThinkPad and
uncovered the embedded controller chip [2], we got the underlying EC
hardware specs [3]. Contrasting the LPC protocol in [3] with the APS
spec [1] and mainline code showed they're indeed very similar,
confirming the guess (and giving us the IO port offsets). This was the
main breakthrough. The rest is detailed in my previous post.

  Shem

[1]http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html
[2]http://thinkwiki.org/wiki/Image:T43p-H8S2161.jpg
[3]http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Pavel Machek]

Hi!

> > Patches look great and I am glad someone has apparently better access
> > to hardware specs than I did.
> 
> This situation is still a concern.  From where did this additional register
> information come?

Well, I do not see much additional register information:

1/12: Seems to be based on
http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf
(and says so in the opening comment)

2/12: Seems to only rename already-known data.. plus adds some
knowledge of embedded controller, probably from pdf above.

3/12: seems to refactor existing code.

4/12: queued events are embedded controller's feature, I guess, so
this should be easy to guess.

5/12: pure linux side code.
6/12: pure linux side code.
7/12: pure linux side code.

8/12: this adds names for things we knew already. Again, this looks
like easy enough to guess. "hdaps_check_ec - checks something about the
EC" ... does not look like it came from leaked info.

(Is this what people have biggest problem with? 1-7 are still very
nice cleanups...)

9/12: pure linux side code.
10/12: pure linux side code.
11/12: pure linux side code.

12/12: improved whitelist. Trivial after discovery that some bit tells
you if accelerometer is there.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 00/12] ThinkPad embedded controller and hdaps drivers (version 2)

[Shem Multinymous]

On 8/10/06, Andrew Morton <akpm@osdl.org> wrote:
> This situation is still a concern.  From where did this additional register
> information come?
>
> Was it reverse-engineered?  If so, by whom and how can we satisfy ourselves
> of this?
>
> Was it from published documents?

Here's a more detailed explanation:

All the low level LPC register access is publicly documented in the
manual of the embedded controller. See [1] and in particular [2]. The
submitted thinkpad_ec code just follows these specs (very defensively
with and a lot of extra status checks, because some EC hangs were
reported in older versions). The only remaining information is about
the accelerometer-specific commands implemented by the firmware; the
public APS spec [3] and the mainline code based on this already
contain most of the this information, and a few corrections and
extentions were gleaned from the reverse-engineered the firmware code
[4]. If case you're wondering about the "opaque" function,
hdaps_check_ec(), then note that it's just code from the original
hdaps driver (following [3]) that's translated to use thinkpad_ec
instead of direct IO port access.


> Was it improperly obtained from NDA'ed documentation?

Absolutely not. I've never signed any NDA remotely related to this.
(and why I do so when the above sources already contain all the needed
information?)

BTW, I can't help wondering: do you have a similarly detailed account
for an appreciable fraction of the driver code in mainline?


> So hm.  We're setting precedent here and we need Linus around to resolve
> this.  Perhaps we can ask "Shem" to reveal his true identity to Linus (and
> maybe me) privately and then we proceed on that basis.

Sure, we can do this. Actually I've alredy e-mailed Linus to this
effect several days ago, before realizing he's off-line.


> "each of the Signed-off-by:ers should know the identity of the others".

How following the DCO's chain-of-trust model?

--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -296,7 +296,7 @@ can certify the below:

         (c) The contribution was provided directly to me by some other
             person who certified (a), (b) or (c) and I have not modified
-            it.
+            it, and the legal identity of that person is known to me.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all


  Shem

[1]http://thinkwiki.org/wiki/Renesas_H8S/2161BV and in particular
[2]http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf
[3]http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html
[4]http://forum.thinkpads.com/viewtopic.php?t=20958

[PATCH 00/12] ThinkPad embedded controller and hdaps drivers

[Shem Multinymous]

This patch series does three related things:

- Introduces a new driver, thinkpad_ec, for the ThinkPad embedded 
  controller (additional non-ACPI interface).
- Changes the existing hdaps driver to use thinkpad_ec instead of
  direct (and incorrect) port access.
- Several fixes and improvements to the hdaps driver.

Applies to git head.

Most of the patches in this series depend on previous ones.
Any point in this series will compile and was briefly tested (this
required introducing some code that gets overwritten by later patches).
The *combination* of all these patches was extensively tested as part
of the out-of-kernel tp_smapi package [1], and verified to work on dozens
of ThinkPad models (thousands of sf.net downloads and no unresolved
bugs). Note that the tp_smapi package includes another driver, to
be submitted later, which also depends the thinkpad_ec driver 
introduced here.

For some context and the necessity for a separate thinkpad_ec module,
see LKML thread "tp_smapi conflict with IDE, hdaps" on Dec. 2005.

I would like to thank the many testers and contributors who helped
in the development, and in particular Henrique de Moraes Holschuh
and ThinkWiki.org users Whoopie and Spiney.

[1] http://thinkwiki.org/wiki/tp_smapi
    http://tpctl.sourceforge.net/rel/tp_smapi-0.27.tgz

Summary:

[PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
[PATCH 02/12] hdaps: Use thinkpad_ec instead of direct port access
[PATCH 03/12] hdaps: Unify and cache hdaps readouts
[PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
[PATCH 05/12] hdaps: Remember keyboard and mouse activity
[PATCH 06/12] hdaps: Limit hardware query rate
[PATCH 07/12] hdaps: delay calibration to first hardware query
[PATCH 08/12] hdaps: Add explicit hardware configuration functions
[PATCH 09/12] hdaps: Add new sysfs attributes
[PATCH 10/12] hdaps: Power off accelerometer on suspend and unload
[PATCH 11/12] hdaps: Stop polling timer when suspended
[PATCH 12/12] hdaps: Simplify whitelist

 drivers/hwmon/hdaps.c          |  758 ++++++++++++++++++++++++-----------------
 drivers/firmware/Kconfig       |    3 
 drivers/firmware/Makefile      |    1 
 drivers/firmware/thinkpad_ec.c |  444 ++++++++++++++++++++++++
 drivers/hwmon/Kconfig          |    1 
 include/linux/thinkpad_ec.h    |   47 ++
 6 files changed, 940 insertions(+), 314 deletions(-)

[PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Shem Multinymous]

The current hdaps driver queries the hardware on (almost) any sysfs
read, reading just the information it needs and discarding the rest
This is inefficient, because every hardware query actually gives all 
information. It also means we're losing data, because readouts are
offered by the hardware at a constant rate and each query "eats up"
a readout. It also results in unnecessarily complex code.

This patch moves all hardware value reading+parsing to a single 
function, __hdaps_update(). All values are cached, and easily 
referenced afterwards. This function is still invoked on every sysfs 
read. This will be fixed in a later patch.

Signed-off-by: Shem Multinymous <multinymous@gmail.com>
---
 hdaps.c |  152 +++++++++++++++++++++++++++++-----------------------------------
 1 file changed, 71 insertions(+), 81 deletions(-)

diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/drivers/hwmon/hdaps.c
@@ -63,66 +63,66 @@ static struct timer_list hdaps_timer;
 static struct platform_device *pdev;
 static struct input_dev *hdaps_idev;
 static unsigned int hdaps_invert;
+
+/* Latest state readout */
+static int pos_x, pos_y;   /* position */
+static int var_x, var_y;   /* variance (what is this?) */
+static u8 temp1, temp2;    /* temperatures */
 static u8 km_activity;
-static int rest_x;
-static int rest_y;
+static int rest_x, rest_y; /* calibrated rest position */
 
-/*
- * hdaps_readb_one - reads a byte from a single I/O port, placing the value in
- * the given pointer.  Returns zero on success or a negative error on failure.
- * Can sleep.
+
+/* __hdaps_update - read current state and update global state variables.
+ * Also prefetches the next read, to reduce udelay busy-waiting.
+ * If fast!=0, do one quick attempt without retries.
+ * Caller must hold controller lock. 
  */
-static int hdaps_readb_one(unsigned int port, u8 *val)
+static int __hdaps_update(int fast)
 {
-	int ret;
+	/* Read data: */
 	struct thinkpad_ec_row data;
-
-	ret = thinkpad_ec_lock();
-	if (ret)
-		return ret;
-	data.mask = (1 << port);
-	ret = thinkpad_ec_read_row(&ec_accel_args, &data);
-	*val = data.val[port];
-	thinkpad_ec_unlock();
-	return ret;
-}
-
-/* __hdaps_read_pair - internal lockless helper for hdaps_read_pair(). */
-static int __hdaps_read_pair(unsigned int port1, unsigned int port2,
-			     int *x, int *y)
-{
 	int ret;
-	struct thinkpad_ec_row data;
 
-	data.mask = (3 << port1) | (3 << port2) | (1 << EC_ACCEL_IDX_KMACT);
-	ret = thinkpad_ec_read_row(&ec_accel_args, &data);
+	data.mask = (1 << EC_ACCEL_IDX_KMACT)    |
+	            (3 << EC_ACCEL_IDX_YPOS1)    | (3 << EC_ACCEL_IDX_XPOS1) |
+	            (3 << EC_ACCEL_IDX_YPOS2)    | (3 << EC_ACCEL_IDX_XPOS2) |
+	            (1 << EC_ACCEL_IDX_TEMP1)    | (1 << EC_ACCEL_IDX_TEMP2);
+	if (fast)
+		ret = thinkpad_ec_try_read_row(&ec_accel_args, &data);
+	else
+		ret = thinkpad_ec_read_row(&ec_accel_args, &data);
+	thinkpad_ec_prefetch_row(&ec_accel_args); /* Prefetch even if error */
 	if (ret)
 		return ret;
 
-	*x = *(s16*)(data.val+port1);
-	*y = *(s16*)(data.val+port2);
+	/* Parse position data: */
+	pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1) * (hdaps_invert?-1:1);
+	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1) * (hdaps_invert?-1:1);
+
+	/* Parse so-called "variance" data: */
+	var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2) * (hdaps_invert?-1:1);
+	var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2) * (hdaps_invert?-1:1);
+
 	km_activity = data.val[EC_ACCEL_IDX_KMACT];
 
-	/* if hdaps_invert is set, negate the two values */
-	if (hdaps_invert) {
-		*x = -*x;
-		*y = -*y;
-	}
+	temp1 = data.val[EC_ACCEL_IDX_TEMP1];
+	temp2 = data.val[EC_ACCEL_IDX_TEMP2];
 
 	return 0;
 }
 
-/*
- * hdaps_read_pair - reads the values from a pair of ports, placing the values
- * in the given pointers.  Returns zero on success.  Can sleep.
+/* hdaps_update - read current state and update global state variables.
+ * Also prefetches the next read, to reduce udelay busy-waiting.
+ * Retries until timeout if the accelerometer is not in ready status (common).
+ * Does its own locking.
  */
-static int hdaps_read_pair(unsigned int port1, unsigned int port2,
-			   int *val1, int *val2)
+static int hdaps_update(void)
 {
-	int ret = thinkpad_ec_lock();
+	int ret;
+	ret = thinkpad_ec_lock();
 	if (ret)
 		return ret;
-	ret = __hdaps_read_pair(port1, port2, val1, val2);
+	ret = __hdaps_update(0);
 	thinkpad_ec_unlock();
 	return ret;
 }
@@ -237,34 +237,41 @@ static struct platform_driver hdaps_driv
 };
 
 /*
- * hdaps_calibrate - Set our "resting" values.  Callers must hold hdaps_sem.
+ * hdaps_calibrate - Set our "resting" values. Does its own locking.
  */
 static void hdaps_calibrate(void)
 {
-	__hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &rest_x, &rest_y);
+	hdaps_update();
+	rest_x = pos_x;
+	rest_y = pos_y;
 }
 
+/* Timer handler for updating the input device. Runs in softirq context,
+ * so avoid lenghty or blocking operations.
+ */
 static void hdaps_mousedev_poll(unsigned long unused)
 {
-	int x, y;
+	int ret;
 
 	/* Cannot sleep.  Try nonblockingly.  If we fail, try again later. */
-	if (thinkpad_ec_try_lock()) {
-		mod_timer(&hdaps_timer,jiffies + HDAPS_POLL_PERIOD);
+	if (thinkpad_ec_try_lock())
+		goto keep_active;
+
+	ret = __hdaps_update(1); /* fast update, we're in softirq context */
+	thinkpad_ec_unlock();
+	/* Any of "successful", "not yet ready" and "not prefetched"? */
+	if (ret!=0 && ret!=-EBUSY && ret!=-ENODATA) {
+		printk(KERN_ERR 
+		       "hdaps: poll failed, disabling mousedev updates\n");
 		return;
 	}
 
-	if (__hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &x, &y))
-		goto out;
-
-	input_report_abs(hdaps_idev, ABS_X, x - rest_x);
-	input_report_abs(hdaps_idev, ABS_Y, y - rest_y);
+keep_active:
+	/* Even if we failed now, pos_x,y may have been updated earlier: */
+	input_report_abs(hdaps_idev, ABS_X, pos_x - rest_x);
+	input_report_abs(hdaps_idev, ABS_Y, pos_y - rest_y);
 	input_sync(hdaps_idev);
-
 	mod_timer(&hdaps_timer, jiffies + HDAPS_POLL_PERIOD);
-
-out:
-	thinkpad_ec_unlock();
 }
 
 
@@ -273,51 +280,37 @@ out:
 static ssize_t hdaps_position_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
-	int ret, x, y;
-
-	ret = hdaps_read_pair(EC_ACCEL_IDX_XPOS1, EC_ACCEL_IDX_YPOS1, &x, &y);
+	int ret = hdaps_update();
 	if (ret)
 		return ret;
-
-	return sprintf(buf, "(%d,%d)\n", x, y);
+	return sprintf(buf, "(%d,%d)\n", pos_x, pos_y);
 }
 
 static ssize_t hdaps_variance_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
-	int ret, x, y;
-
-	ret = hdaps_read_pair(EC_ACCEL_IDX_XPOS2, EC_ACCEL_IDX_YPOS2, &x, &y);
+	int ret = hdaps_update();
 	if (ret)
 		return ret;
-
-	return sprintf(buf, "(%d,%d)\n", x, y);
+	return sprintf(buf, "(%d,%d)\n", var_x, var_y);
 }
 
 static ssize_t hdaps_temp1_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	u8 temp;
-	int ret;
-
-	ret = hdaps_readb_one(EC_ACCEL_IDX_TEMP1, &temp);
-	if (ret < 0)
+	int ret = hdaps_update();
+	if (ret)
 		return ret;
-
-	return sprintf(buf, "%u\n", temp);
+	return sprintf(buf, "%u\n", temp1);
 }
 
 static ssize_t hdaps_temp2_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	u8 temp;
-	int ret;
-
-	ret = hdaps_readb_one(EC_ACCEL_IDX_TEMP2, &temp);
-	if (ret < 0)
+	int ret = hdaps_update();
+	if (ret)
 		return ret;
-
-	return sprintf(buf, "%u\n", temp);
+	return sprintf(buf, "%u\n", temp2);
 }
 
 static ssize_t hdaps_keyboard_activity_show(struct device *dev,
@@ -344,10 +337,7 @@ static ssize_t hdaps_calibrate_store(str
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
 {
-	if (thinkpad_ec_lock())
-		return -EIO;
 	hdaps_calibrate();
-	thinkpad_ec_unlock();
 	return count;
 }
 

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Pavel Machek]

Hi!

> The current hdaps driver queries the hardware on (almost) any sysfs
> read, reading just the information it needs and discarding the rest
> This is inefficient, because every hardware query actually gives all 
> information. It also means we're losing data, because readouts are
> offered by the hardware at a constant rate and each query "eats up"
> a readout. It also results in unnecessarily complex code.
> 
> This patch moves all hardware value reading+parsing to a single 
> function, __hdaps_update(). All values are cached, and easily 
> referenced afterwards. This function is still invoked on every sysfs 
> read. This will be fixed in a later patch.
> 
> Signed-off-by: Shem Multinymous <multinymous@gmail.com>
Signed-off-by: Pavel Machek <pavel@suse.cz>

> +/* __hdaps_update - read current state and update global state variables.
> + * Also prefetches the next read, to reduce udelay busy-waiting.
> + * If fast!=0, do one quick attempt without retries.
> + * Caller must hold controller lock. 
>   */

Linuxdoc, please.

> +	/* Parse position data: */
> +	pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1) * (hdaps_invert?-1:1);
> +	pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1) * (hdaps_invert?-1:1);
> +
> +	/* Parse so-called "variance" data: */
> +	var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2) * (hdaps_invert?-1:1);
> +	var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2) * (hdaps_invert?-1:1);

Perhaps hdaps_invert should already have 1/-1 values.

>  {
> -	int ret = thinkpad_ec_lock();
> +	int ret;
> +	ret = thinkpad_ec_lock();

I actually liked the previous version more, and this change does not
really belong here.

						Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Shem Multinymous]

On 8/7/06, Pavel Machek <pavel@suse.cz> wrote:
> > +     /* Parse position data: */
> > +     pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1) * (hdaps_invert?-1:1);
> > +     pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1) * (hdaps_invert?-1:1);
> > +
> > +     /* Parse so-called "variance" data: */
> > +     var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2) * (hdaps_invert?-1:1);
> > +     var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2) * (hdaps_invert?-1:1);
>
> Perhaps hdaps_invert should already have 1/-1 values.

It's also used as a module parameter, which is 0/1 in mainline. I
don't think this is worth extra code.


> >  {
> > -     int ret = thinkpad_ec_lock();
> > +     int ret;
> > +     ret = thinkpad_ec_lock();
>
> I actually liked the previous version more, and this change does not
> really belong here.

(That's a diff artifact, it's a totally different function...)
Changed to the version you like.

  Shem

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Pavel Machek]

Hi!

> >> +     /* Parse position data: */
> >> +     pos_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1) * (hdaps_invert?-1:1);
> >> +     pos_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1) * (hdaps_invert?-1:1);
> >> +
> >> +     /* Parse so-called "variance" data: */
> >> +     var_x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS2) * (hdaps_invert?-1:1);
> >> +     var_y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS2) * (hdaps_invert?-1:1);
> >
> >Perhaps hdaps_invert should already have 1/-1 values.
> 
> It's also used as a module parameter, which is 0/1 in mainline. I
> don't think this is worth extra code.

Okay, so what about ..

#define CONVERT(x) *(s16*)(data.val+x) * (hdaps_invert?-1:1);

...or better inline function?

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

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Shem Multinymous]

On 8/8/06, Pavel Machek <pavel@suse.cz> wrote:
> Okay, so what about ..
>
> #define CONVERT(x) *(s16*)(data.val+x) * (hdaps_invert?-1:1);
>
> ...or better inline function?

Actually, some models require fancier transformations. This was
supposed to be reserved for a future patch, but might as well prepare
the infrastructure:

/* Some models require an axis transformation to the standard reprsentation */
static void transform_axes(int inx, int iny, int *outx, int *outy) {
	*outx = inx * (hdaps_invert?-1:1);
	*outy = iny * (hdaps_invert?-1:1);
}
...
	/* Parse position data: */
	transform_axes(*(s16*)(data.val+EC_ACCEL_IDX_XPOS1),
	               *(s16*)(data.val+EC_ACCEL_IDX_YPOS1), &pos_x, &pos_y);

	/* Parse so-called "variance" data: */
	transform_axes(*(s16*)(data.val+EC_ACCEL_IDX_XPOS2),
	               *(s16*)(data.val+EC_ACCEL_IDX_YPOS2), &var_x, &var_y);


  Shem

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Pavel Machek]

Hi!

> >Okay, so what about ..
> >
> >#define CONVERT(x) *(s16*)(data.val+x) * (hdaps_invert?-1:1);
> >
> >...or better inline function?
> 
> Actually, some models require fancier transformations. This was
> supposed to be reserved for a future patch, but might as well prepare
> the infrastructure:

Certainly better.

> /* Some models require an axis transformation to the standard reprsentation 
> */
> static void transform_axes(int inx, int iny, int *outx, int *outy) {
> 	*outx = inx * (hdaps_invert?-1:1);
> 	*outy = iny * (hdaps_invert?-1:1);
> }
> ...
> 	/* Parse position data: */
> 	transform_axes(*(s16*)(data.val+EC_ACCEL_IDX_XPOS1),
> 	               *(s16*)(data.val+EC_ACCEL_IDX_YPOS1), &pos_x, &pos_y);

You could also do

void transform_axes(int *x, int *y)
{
 	*outx = inx * (hdaps_invert?-1:1);
 	*outy = iny * (hdaps_invert?-1:1);
}
...
 	/* Parse position data: */
	x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1);
	y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1);
 	transform_axes(&x, &y);

...which looks even better to me.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Shem Multinymous]

On 8/8/06, Pavel Machek <pavel@suse.cz> wrote:
>         /* Parse position data: */
>         x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1);
>         y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1);
>         transform_axes(&x, &y);
>
> ...which looks even better to me.

Yes, that's elegant.
But it made me realize there's a race condition here (and and also in
the mainline driver): the global pos_x, rest_x etc. could be updated
while an attribute's show_* function is called. Ugh. I guess I need to
sprinkle spinlocks all over the place.

  Shem

Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts

[Pavel Machek]

Hi!

> >        /* Parse position data: */
> >        x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1);
> >        y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1);
> >        transform_axes(&x, &y);
> >
> >...which looks even better to me.
> 
> Yes, that's elegant.
> But it made me realize there's a race condition here (and and also in
> the mainline driver): the global pos_x, rest_x etc. could be updated
> while an attribute's show_* function is called. Ugh. I guess I need to
> sprinkle spinlocks all over the place.

They are simple integers... so yes, locking is needed, but I'd not
label it as critical. I guess you should get your series done, first.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html