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(-)
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 <[email protected]>
---
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 | 449 +++++++++++++++++++++++++++++++++++++++++
include/linux/thinkpad_ec.h | 47 ++++
4 files changed, 500 insertions(+)
diff -dNurp a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
--- a/drivers/firmware/Kconfig 2006-08-05 04:31:21.000000000 +0300
+++ b/drivers/firmware/Kconfig 2006-08-05 04:31:21.000000000 +0300
@@ -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
diff -dNurp a/drivers/firmware/Makefile b/drivers/firmware/Makefile
--- a/drivers/firmware/Makefile 2006-08-05 04:31:21.000000000 +0300
+++ b/drivers/firmware/Makefile 2006-08-05 04:31:21.000000000 +0300
@@ -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
diff -dNurp a/drivers/firmware/thinkpad_ec.c b/drivers/firmware/thinkpad_ec.c
--- a/drivers/firmware/thinkpad_ec.c 1970-01-01 02:00:00.000000000 +0200
+++ b/drivers/firmware/thinkpad_ec.c 2006-08-05 05:33:06.000000000 +0300
@@ -0,0 +1,444 @@
+/*
+ * 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 <[email protected]>
+ *
+ * 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.27"
+
+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 */
+
+/* Module parameters: */
+static int tp_debug = 0;
+module_param_named(debug, tp_debug, int, 0600);
+MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
+
+/* A few macros for printk()ing: */
+#define DPRINTK(fmt, args...) \
+ do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
+#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 exclusive lock for accesing the controller. May sleep.
+ * Returns 0 iff lock acquired .
+ */
+int thinkpad_ec_lock(void)
+{
+ int ret;
+ ret = down_interruptible(&thinkpad_ec_mutex);
+ if (ret)
+ DPRINTK("tp_controller mutex down interrupted: %d\n", ret);
+ return ret;
+}
+
+EXPORT_SYMBOL_GPL(thinkpad_ec_lock);
+
+/* thinkpad_ec_try_lock:
+ * Get exclusive lock for accesing the controller but only if it's available.
+ * Does not block, does not sleep. 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 a previously acquired controller lock.
+ */
+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:
+ * Read a data row from the controller, fetching and retrying if needed.
+ * The row args are specified by 16 byte arguments, some of which may be
+ * missing (but the first and last are 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->maska. 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 read a prefetched row from the controller. Don't fetch or retry.
+ * See thinkpad_ec_read_row above for the meaning of the arguments.
+ * 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 row from the controller. 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 above for the meaning of the arguments.
+ * 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 the prefetched controller data.
+ * 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 for a given type for a substrng */
+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_ERR "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_INFO "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);
diff -dNurp a/include/linux/thinkpad_ec.h b/include/linux/thinkpad_ec.h
--- a/include/linux/thinkpad_ec.h 1970-01-01 02:00:00.000000000 +0200
+++ b/include/linux/thinkpad_ec.h 2006-08-05 05:33:06.000000000 +0300
@@ -0,0 +1,47 @@
+/*
+ * thinkpad_ec.h - interface to ThinkPad embedded controller LPC3 functions
+ *
+ * Copyright (C) 2005 Shem Multinymous <[email protected]>
+ *
+ * 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 */
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.
Signed-off-by: Shem Multinymous <[email protected]>
---
a/drivers/hwmon/hdaps.c | 314 +++++++++++++++++-------------------------------
drivers/hwmon/Kconfig | 1
2 files changed, 116 insertions(+), 199 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/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: %s\n", msg); goto bad; }
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");
+ if (data.val[0xF]!=0x00)
+ ABORT_INIT("check1");
+ mode = data.val[0x1];
+
+ printk(KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n", mode);
+ if (mode==0x00)
+ ABORT_INIT("accelerometer not available");
+
+ 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");
+ if (data.val[0x1]!=0x00 ||
+ data.val[0x2]!=0x60 ||
+ data.val[0x3]!=0x00 ||
+ data.val[0xF]!=0x00)
+ ABORT_INIT("check2");
+
+ 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");
+ if (data.val[0xF]!=0x00)
+ ABORT_INIT("check3");
+
+ 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");
+ if (data.val[0xF]!=0x00)
+ ABORT_INIT("check4");
- 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 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_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
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", which is nonsensical. We have no use for that previous
readout, so it should be ignored.
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 <[email protected]>
---
hdaps.c | 75 +++++++++++++++++++++++++++++-----------------------------------
1 file changed, 34 insertions(+), 41 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/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,12 +70,10 @@ 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 */
-
/* __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.
@@ -83,10 +85,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
@@ -95,18 +96,23 @@ 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) * (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];
- temp1 = data.val[EC_ACCEL_IDX_TEMP1];
- temp2 = data.val[EC_ACCEL_IDX_TEMP2];
+ temperature = data.val[EC_ACCEL_IDX_TEMP1];
return 0;
}
@@ -118,12 +124,20 @@ static int __hdaps_update(int fast)
*/
static int hdaps_update(void)
{
- int ret;
- ret = thinkpad_ec_lock();
- if (ret)
- return ret;
- ret = __hdaps_update(0);
- thinkpad_ec_unlock();
+ int total, ret;
+ for (total=READ_TIMEOUT_MSECS; total>0; 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;
}
@@ -286,31 +300,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,
@@ -363,9 +359,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);
@@ -373,9 +368,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,
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 <[email protected]>
---
hdaps.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/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;
@@ -123,6 +124,8 @@ static int __hdaps_update(int fast)
temperature = data.val[EC_ACCEL_IDX_TEMP1];
+ stale_readout = 0;
+
return 0;
}
@@ -134,6 +137,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=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
ret = thinkpad_ec_lock();
if (ret)
@@ -226,6 +231,7 @@ bad:
thinkpad_ec_invalidate();
ret = -ENXIO;
good:
+ stale_readout = 1;
thinkpad_ec_unlock();
return ret;
}
@@ -276,6 +282,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;
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 <[email protected]>
---
hdaps.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/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;
+
/* __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.
@@ -110,7 +112,14 @@ static int __hdaps_update(int fast)
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);
- 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];
@@ -313,14 +322,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,
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 <[email protected]>
---
hdaps.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 153 insertions(+), 47 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/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 = 0;
+/* 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 = 0; /* Enable EC fake data mode? */
+
/* Latest state readout: */
static int pos_x, pos_y; /* position */
static int temperature; /* temperature */
@@ -162,6 +168,137 @@ 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, which 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
+ * ( sampling_rate * oversampling_ratio )
+ * order - embedded controller running average filter order
+ * 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. Returns zero on success
* and negative error code on failure. Can sleep.
*/
@@ -169,61 +306,30 @@ 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");
- if (data.val[0xF]!=0x00)
- ABORT_INIT("check1");
- mode = data.val[0x1];
-
+ if (hdaps_get_ec_mode(&mode))
+ ABORT_INIT("hdaps_get_ec_mode failed");
printk(KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n", mode);
if (mode==0x00)
ABORT_INIT("accelerometer not available");
- 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");
- if (data.val[0x1]!=0x00 ||
- data.val[0x2]!=0x60 ||
- data.val[0x3]!=0x00 ||
- data.val[0xF]!=0x00)
- ABORT_INIT("check2");
-
- 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");
- if (data.val[0xF]!=0x00)
- ABORT_INIT("check3");
+ if (hdaps_check_ec(&mode))
+ ABORT_INIT("hdaps_check_ec failed");
- 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");
- if (data.val[0xF]!=0x00)
- ABORT_INIT("check4");
+ if (hdaps_set_power(1))
+ ABORT_INIT("hdaps_set_power failed");
+
+ if (hdaps_set_ec_config(sampling_rate*oversampling_ratio,
+ running_avg_filter_order))
+ ABORT_INIT("hdaps_set_ec_config failed");
+
+ if (hdaps_set_fake_data_mode(fake_data_mode))
+ ABORT_INIT("hdaps_set_fake_data_mode failed");
thinkpad_ec_invalidate();
udelay(200);
@@ -308,7 +414,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);
}
@@ -525,7 +631,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");
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 <[email protected]>
---
hdaps.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/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 = 0;
/* Latest state readout: */
static int pos_x, pos_y; /* position */
@@ -125,6 +126,11 @@ 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;
}
@@ -270,9 +276,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,
@@ -502,8 +508,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";
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 <[email protected]>
---
hdaps.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 110 insertions(+)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/drivers/hwmon/hdaps.c
@@ -495,6 +495,102 @@ 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) {
+ printk(KERN_WARNING
+ "fake_data should be 0 or 1\n");
+ 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 */
@@ -502,6 +598,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,
@@ -510,6 +616,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,
};
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 modules is
completely unaffected, and new models will work automatically (though not
necessarily with correct axis configuration).
Signed-off-by: Shem Multinymous <[email protected]>
---
hdaps.c | 66 +++++++++++++---------------------------------------------------
1 file changed, 14 insertions(+), 52 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/drivers/hwmon/hdaps.c
@@ -652,79 +652,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);
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 <[email protected]>
---
hdaps.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/drivers/hwmon/hdaps.c
@@ -374,13 +374,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();
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 = {
@@ -421,7 +427,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 update timer\n");
return;
}
@@ -720,6 +726,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;
@@ -754,11 +763,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;
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
hdaps.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff -up a/drivers/hwmon/hdaps.c a/drivers/hwmon/hdaps.c
--- a/drivers/hwmon/hdaps.c
+++ a/drivers/hwmon/hdaps.c
@@ -348,6 +348,15 @@ good:
return ret;
}
+/*
+ * hdaps_device_shutdown - power off the accelerometer. Can sleep.
+ */
+static void hdaps_device_shutdown(void) {
+ if (hdaps_set_power(0))
+ printk(KERN_WARNING "hdaps: cannot power off\n");
+ if (hdaps_set_ec_config(0, 1))
+ printk(KERN_WARNING "hdaps: cannot stop EC sampling\n");
+}
/* Device model stuff */
@@ -363,6 +372,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();
+ return 0;
+}
+
static int hdaps_resume(struct platform_device *dev)
{
return hdaps_device_init();
@@ -370,6 +385,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",
@@ -762,6 +778,7 @@ static void __exit hdaps_exit(void)
{
del_timer_sync(&hdaps_timer);
input_unregister_device(hdaps_idev);
+ hdaps_device_shutdown();
sysfs_remove_group(&pdev->dev.kobj, &hdaps_attribute_group);
platform_device_unregister(pdev);
platform_driver_unregister(&hdaps_driver);
On Sun, 06 Aug 2006 10:26:46 +0300
Shem Multinymous <[email protected]> wrote:
> Signed-off-by: Shem Multinymous <[email protected]>
This rather defeats the purpose of the DCO. Perhaps one of the other
project members (ie: one who uses a real name) could also sign off the
patches?
Hi,
On 8/6/06, Andrew Morton <[email protected]> wrote:
> On Sun, 06 Aug 2006 10:26:46 +0300
> Shem Multinymous <[email protected]> wrote:
>
> > Signed-off-by: Shem Multinymous <[email protected]>
>
> This rather defeats the purpose of the DCO.
SubmittingPatches declares the purpose of the DCO as "To improve
tracking of who did what, especially with patches that can percolate
to their final resting place in the kernel through several layers of
maintainers."
I indeed certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Practically speaking, this contact address is stable and reliable.
What more is needed that may be realistically expected from a kernel
patch submission?
Shem
On Sun, 6 Aug 2006 12:56:43 +0300
"Shem Multinymous" <[email protected]> wrote:
> What more is needed that may be realistically expected from a kernel
> patch submission?
We who accept the submission would be making a joke of the whole thing if
we accepted the assurances of a person who is concealing his/her identity.
I suggested a simple solution: Perhaps one of the other project members
(ie: one who uses a real name) could also sign off the patches?
Hi,
On 8/6/06, Andrew Morton <[email protected]> wrote:
> I suggested a simple solution: Perhaps one of the other project members
> (ie: one who uses a real name) could also sign off the patches?
Well, I offer this patch under the GPL, so anyone (including you) can do it.
But I would like to be able to submit further patches without "adult
supervision", so I hope you don't mind my pressing the issue:
> > What more is needed that may be realistically expected from a kernel
> > patch submission?
>
> We who accept the submission would be making a joke of the whole thing if
> we accepted the assurances of a person who is concealing his/her identity.
Can you please be more specific? What purpose does this exclusion
serve, that would be realistically achieved otherwise? You already
have a GPL license from the author, and a way to contact and uniquely
identify the author.
Shem
On Sun, Aug 06, 2006 at 01:44:02PM +0300, Shem Multinymous wrote:
> On 8/6/06, Andrew Morton <[email protected]> wrote:
> >I suggested a simple solution: Perhaps one of the other project members
> >(ie: one who uses a real name) could also sign off the patches?
>
> Well, I offer this patch under the GPL, so anyone (including you) can do it.
But who is "I", and how do we know that you really do have the legal
authority to offer the patch under the GPL? *That's* the whole point
of the DCO. It is a lightweight version of what the FSF requires,
which is a legal contract asserting the same, with an ink signature
and (at least in some cases) notarized by a public notary. The
contract has some interesting words in it, including ones where the
signer indemnifies the FSF against any claims for the work that has
been contributed, but it's all there to protect the FSF.
Some have claimed that the FSF approach is too slow, too heavyweight,
and discourages contributions, but there is good legal reasons for why
they ask that of their contributors. I might not be willing to sign
the FSF's contract, but that's a personal matter between me and the
FSF. (And given the FSF's position on the GPLv3 and my current
distaste of the discussion drafts, it's just as well that I don't
contribute code to the FSF.)
The DCO is a more lightweight mechanism, which tries to establish a
legal chain of accountability for patches. But in order to do that,
we need to know who is making the assertion, and a pseudonym defeats
the whole point of the DCO. No, we're not requiring an ink signature,
and no we're not requiring a notary public to check the identity of
the person signing the contract --- but then again, it's not that hard
to fake the driver's license which the notary public checks. The
point is that there is a process, and that we ask for a certain level
of assurance. The fact that the FSF doesn't require DNA samples
doesn't mean that they shouldn't stop doing what they are doing, just
as the fact that we aren't requiring ink signatures and public notary
checks doesn't mean we shouldn't stop doing what we are doing.
> Can you please be more specific? What purpose does this exclusion
> serve, that would be realistically achieved otherwise? You already
> have a GPL license from the author, and a way to contact and uniquely
> identify the author.
For legal reasons, we need a way to to contact and identify the author
in the real world, not just in cyberspace, and a pseudonym doesn't
meet that requirement.
- Ted
On Sun, Aug 06, 2006 at 10:55:51AM -0400, Theodore Tso wrote:
> On Sun, Aug 06, 2006 at 01:44:02PM +0300, Shem Multinymous wrote:
> > Can you please be more specific? What purpose does this exclusion
> > serve, that would be realistically achieved otherwise? You already
> > have a GPL license from the author, and a way to contact and uniquely
> > identify the author.
>
> For legal reasons, we need a way to to contact and identify the author
> in the real world, not just in cyberspace, and a pseudonym doesn't
> meet that requirement.
In that context, even an anonymous mailer like gmail and the like is
questionable. But, I'm sure one get a domain with faked address data
in the whois database.
Where would you draw the line?
On Sun, Aug 06, 2006 at 06:40:13PM +0200, Olaf Hering wrote:
> On Sun, Aug 06, 2006 at 10:55:51AM -0400, Theodore Tso wrote:
> > On Sun, Aug 06, 2006 at 01:44:02PM +0300, Shem Multinymous wrote:
>
> > > Can you please be more specific? What purpose does this exclusion
> > > serve, that would be realistically achieved otherwise? You already
> > > have a GPL license from the author, and a way to contact and uniquely
> > > identify the author.
> >
> > For legal reasons, we need a way to to contact and identify the author
> > in the real world, not just in cyberspace, and a pseudonym doesn't
> > meet that requirement.
>
> In that context, even an anonymous mailer like gmail and the like is
> questionable. But, I'm sure one get a domain with faked address data
> in the whois database.
> Where would you draw the line?
I think that if we have the date, the name, and Gmail has the logs, it
should be enough to get back to the real persons in case of future legal
trouble. Anyway, it's clearly possible that many contributors already
post under pseudonyms which do not designate them in person. After all,
it is already more or less the case for all those with non-latin alphabets.
Perhaps the only thing we're missing is a list of validated mail account
providers ? Anyway, how do we now that someone hosted by local provider X
my be mapped to a physical person ?
Maybe we should only make the difference between "intentionnal anonymity"
and free mail accounts, if this is possible.
Regards,
Willy
On Sun, 6 Aug 2006 18:40:13 +0200
Olaf Hering <[email protected]> wrote:
> On Sun, Aug 06, 2006 at 10:55:51AM -0400, Theodore Tso wrote:
> > On Sun, Aug 06, 2006 at 01:44:02PM +0300, Shem Multinymous wrote:
>
> > > Can you please be more specific? What purpose does this exclusion
> > > serve, that would be realistically achieved otherwise? You already
> > > have a GPL license from the author, and a way to contact and uniquely
> > > identify the author.
> >
> > For legal reasons, we need a way to to contact and identify the author
> > in the real world, not just in cyberspace, and a pseudonym doesn't
> > meet that requirement.
>
> In that context, even an anonymous mailer like gmail and the like is
> questionable. But, I'm sure one get a domain with faked address data
> in the whois database.
> Where would you draw the line?
I have a personal line, and that is when the patch is "substantial". (This
line is only relevant when someone forgot to add the Signed-off-by: and I'm
wondering whether to ask them to send one).
And I'd say this patch series _is_ substantial because it pokes at
registers which might be described in confidential/NDA'ed documentation, or
in ways which might be derived from $OTHER_OS.
On Sun, 2006-08-06 at 13:44 +0300, Shem Multinymous wrote:
> Hi,
>
> On 8/6/06, Andrew Morton <[email protected]> wrote:
> > I suggested a simple solution: Perhaps one of the other project members
> > (ie: one who uses a real name) could also sign off the patches?
>
> Well, I offer this patch under the GPL, so anyone (including you) can do it.
>
> But I would like to be able to submit further patches without "adult
> supervision", so I hope you don't mind my pressing the issue:
>
Open source is all about trust. Person A takes a patch from person B
because person A has trust in B (conditional on the patch meeting a
technical standard). In B's technical ability, in B's intentions, in B's
sincerity, in B's honesty when he says "this is my work and you can use
it because nobody but me has a claim on this".
Using a fake name is not a good way to gain such trust... At all.
Explicitly refusing to say who you really are just lowers the trust even
more, because it gives a strong appearance that something really fishy
is going on.
Personally I would be extremely hesitant to take any patches from anyone
who refuses to give out his real name. Because trust is about a person.
Because trust means not having to hide things that matter. And a persons
identity does matter for taking patches.
--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Hi Ted,
Thanks for the explanation. Point taken, though I can't help parsing it as:
On 8/6/06, Theodore Tso <[email protected]> wrote:
> For legal reasons, we need a way to to contact and identify the author
> in the real world, not just in cyberspace, and a pseudonym doesn't
> meet that requirement.
"We want to be able to sue you if they sue us."
Which is actually not a problem for me (i.e., I don't believe I have
nothing to worry about legally); but I do have other, non-legal
considerations.
> just as the fact that we aren't requiring ink signatures and public notary
> checks doesn't mean we shouldn't stop doing what we are doing.
Understood, but still a bit silly. You have no idea how many of the
2252 people in `git-whatchanged | grep Signed-off-by: | sort | uniq`
gave their legal name, and I doubt you could contact most of them in
the real world without their cooperation (and with my cooperation, you
could contact me too). Heck, some of those email domains don't even
resolve. So this "chain of responsibiliy" is pretty worthless if
someone really tries to inject legally malicious code into mainline,
i.e., you end up blindly trusting people anyway.
BTW, Ted, we actually have met in person. :-)
Shem
On 8/6/06, Andrew Morton <[email protected]> wrote:
> And I'd say this patch series _is_ substantial because it pokes at
> registers which might be described in confidential/NDA'ed documentation, or
> in ways which might be derived from $OTHER_OS.
For what it's worth to you:
I hereby declare that this patch was developed solely based on public
specifications, observation of hardware behavior by trial&eror, and
specifications made available to me in clean-room settings and with no
attached obligations. So this patch is as pure as the mainline hdaps
driver it fixes (and probably purer than many other drivers), and not
a single line of it is a derivative work of $OTHER_OS code.
If it would help inspire trust, you can look at the tp_smapi revision
history on sf.net, where many of those trials and errors are
immortalized.
As for the register poking, I believe all the code in thinkpad_ec.c
logically follows from the publicly available H8S documentation (see
link at the top of the sourcecode), except for one number (base port
"0x1600") which is already given by the mainline hdaps driver.
Shem
Hi Arjan,
On 8/6/06, Arjan van de Ven <[email protected]> wrote:
> Open source is all about trust. Person A takes a patch from person B
> because person A has trust in B (conditional on the patch meeting a
> technical standard). In B's technical ability, in B's intentions, in B's
> sincerity, in B's honesty when he says "this is my work and you can use
> it because nobody but me has a claim on this".
Excellent points.
> Using a fake name is not a good way to gain such trust... At all.
> Explicitly refusing to say who you really are just lowers the trust even
> more, because it gives a strong appearance that something really fishy
> is going on.
Agreed. But this is only a heuristic; maybe in this case nothing fishy
is going on, whereas many nice-looking patches would reek to heaven if
inspected more closely.
So how about exercising judgment? Look up the tp_smapi development
history on sf.net (under project "tpctl"), the relevant posts on
thinkpad-devel, hdaps-devel and even a bit on lkml, and see if this
looks like standard, clean kernel development or a sinister attempt to
steal our precious bolidy fluids.
Shem
On Mon, Aug 07, 2006 at 01:41:27AM +0300, Shem Multinymous wrote:
> On 8/6/06, Arjan van de Ven <[email protected]> wrote:
> >Open source is all about trust. Person A takes a patch from person B
> >because person A has trust in B (conditional on the patch meeting a
> >technical standard). In B's technical ability, in B's intentions, in B's
> >sincerity, in B's honesty when he says "this is my work and you can use
> >it because nobody but me has a claim on this".
>
> Excellent points.
>
> >Using a fake name is not a good way to gain such trust... At all.
> >Explicitly refusing to say who you really are just lowers the trust even
> >more, because it gives a strong appearance that something really fishy
> >is going on.
>
> Agreed. But this is only a heuristic; maybe in this case nothing fishy
> is going on, whereas many nice-looking patches would reek to heaven if
> inspected more closely.
But since something obviously is fishy here, we can't accept them,
sorry.
I suggest trying to resove the other issues that are preventing you from
using your real name, and then resubmit these patches. As it is,
unfortunatly, we are not going to be able to accept this work.
good luck,
greg k-h
On Mon, 2006-08-07 at 01:41 +0300, Shem Multinymous wrote:
> Hi Arjan,
>
> On 8/6/06, Arjan van de Ven <[email protected]> wrote:
> > Open source is all about trust. Person A takes a patch from person B
> > because person A has trust in B (conditional on the patch meeting a
> > technical standard). In B's technical ability, in B's intentions, in B's
> > sincerity, in B's honesty when he says "this is my work and you can use
> > it because nobody but me has a claim on this".
>
> Excellent points.
>
>
> > Using a fake name is not a good way to gain such trust... At all.
> > Explicitly refusing to say who you really are just lowers the trust even
> > more, because it gives a strong appearance that something really fishy
> > is going on.
>
> Agreed. But this is only a heuristic; maybe in this case nothing fishy
> is going on, whereas many nice-looking patches would reek to heaven if
> inspected more closely.
yes it's only a heuristic. But at this point in time your stuff has a
smell to it, especially due to your reluctance to resolve/debunk the
issue. So while you're right it's not always clear cut... in this case I
think it pretty much is until you decide to clear things up. As far as
I'm concerned the ball is in your court; you obviously have the power
and opportunity to quickly resolve this and show there's no fish behind
door number 2. Continuing to hide isn't helping to convince us that
there is no dead sea creature.
--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
On Sunday 06 August 2006 6:08 pm, Shem Multinymous wrote:
> Hi Ted,
>
> Thanks for the explanation. Point taken, though I can't help parsing it as:
>
> On 8/6/06, Theodore Tso <[email protected]> wrote:
> > For legal reasons, we need a way to to contact and identify the author
> > in the real world, not just in cyberspace, and a pseudonym doesn't
> > meet that requirement.
>
> "We want to be able to sue you if they sue us."
>
> Which is actually not a problem for me (i.e., I don't believe I have
> nothing to worry about legally); but I do have other, non-legal
> considerations.
>
> > just as the fact that we aren't requiring ink signatures and public
> > notary checks doesn't mean we shouldn't stop doing what we are doing.
>
> Understood, but still a bit silly. You have no idea how many of the
> 2252 people in `git-whatchanged | grep Signed-off-by: | sort | uniq`
> gave their legal name, and I doubt you could contact most of them in
> the real world without their cooperation (and with my cooperation, you
> could contact me too). Heck, some of those email domains don't even
> resolve. So this "chain of responsibiliy" is pretty worthless if
> someone really tries to inject legally malicious code into mainline,
> i.e., you end up blindly trusting people anyway.
>
> BTW, Ted, we actually have met in person. :-)
>
> Shem
This is where GNU PGP keys can help. If more people used them, as a trust
mechanism it would help people trust people more. Otherwise, what's the point
of these keysignings? :-)
I don't mind providing my PGP key if it helps people recognize I am who I am
via email and signed patches.
Shawn.
On Mon, Aug 07, 2006 at 01:08:40AM +0300, Shem Multinymous wrote:
> Hi Ted,
>
> Thanks for the explanation. Point taken, though I can't help parsing it as:
>
> On 8/6/06, Theodore Tso <[email protected]> wrote:
> >For legal reasons, we need a way to to contact and identify the author
> >in the real world, not just in cyberspace, and a pseudonym doesn't
> >meet that requirement.
>
> "We want to be able to sue you if they sue us."
That would be the FSF ink contract that they ask you to sign, which states:
I hereby represent and warrant that I am the sole copyright holder for the
Work and that I have the right and power to enter into this contract. I
hereby indemnify and hold harmless the Foundation, its officers, employees,
and agents against any and all claims, actions or damages (including
attorney's reasonable fees) asserted by or paid to any party on account of a
breach or alleged breach of the foregoing warranty.
(Free advice from someone who is not a lawyer: if you are _ever_ asked
to sign a contract or agreement which has the language,
"I... indemnify and hold harmless...", and you have any kind of
significant assets, like a house, car, trust fund, etc., run, don't
walk, to your friendly neighborhood lawyer and get some real legal
advice about what your exposure might be. In any case, the rough
translation of the above is, "If anyone ever sues the FSF over code
that you are giving us for free, regardless of whether or not the
claim has any merit or not, you hereby give us permission to turn
around and sue you for any damanges _or_ legal fees that we might have
to pay out." But don't take my word for it, talk to a lawyer first. :-)
As far as the DCO is concerned, at least to my mind, it's so when
someone shows up, we can say, "Hey, your beef is with him, not us."
This might be especially true if the code was allegedly taken from
company X's intellectual property, and it turns out the person who
contributed it was an employee of company X.
And in any case, it's certainly better than the FSF situation, where
the "alleged breach" language means that even if the claims are
totally bogus, and funded by some PIPE-smoking crack fairy, your
assets would still be at risk to the FSF, which wouldn't be the case
if you hadn't signed a contract with such language in it.
> >just as the fact that we aren't requiring ink signatures and public notary
> >checks doesn't mean we shouldn't stop doing what we are doing.
>
> Understood, but still a bit silly. You have no idea how many of the
> 2252 people in `git-whatchanged | grep Signed-off-by: | sort | uniq`
> gave their legal name, and I doubt you could contact most of them in
> the real world without their cooperation (and with my cooperation, you
> could contact me too). Heck, some of those email domains don't even
> resolve. So this "chain of responsibiliy" is pretty worthless if
> someone really tries to inject legally malicious code into mainline,
> i.e., you end up blindly trusting people anyway.
Sure, but if someone really wanted, they could infect malicious code
into FSF's repositories, too. And if we let fear paralyze us, we
wouldn't get anything done at all. But at the same time, by having a
process, such as the DCO, we can claim that we've mad a good faith
attempt to collect a chain of accountability for contributions to the
kernel.
- Ted
P.S. Can you say, why you prefer contribute this from a pseudonym, if
isn't for legal reasons?
Hi!
> > What more is needed that may be realistically expected from a kernel
> > patch submission?
>
> We who accept the submission would be making a joke of the whole thing if
> we accepted the assurances of a person who is concealing his/her identity.
>
> I suggested a simple solution: Perhaps one of the other project members
> (ie: one who uses a real name) could also sign off the patches?
I'm willing to sign off these patches. (In legal sense, anyway. I have
not yet went through them carefully).
Pavel
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> +/* Module parameters: */
> +static int tp_debug = 0;
Static variables do not need initializers.
> +module_param_named(debug, tp_debug, int, 0600);
> +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> +
> +/* A few macros for printk()ing: */
> +#define DPRINTK(fmt, args...) \
> + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
Is not there generic function doing this?
> +/* thinkpad_ec_lock:
> + * Get exclusive lock for accesing the controller. May sleep.
> + * Returns 0 iff lock acquired .
> + */
Linuxdoc?
> +EXPORT_SYMBOL_GPL(thinkpad_ec_lock);
> +EXPORT_SYMBOL_GPL(thinkpad_ec_try_lock);
> +void thinkpad_ec_unlock(void)
> +{
> + up(&thinkpad_ec_mutex);
> +}
> +
Do we need these wrappers? Perhaps just directly exporting the mutex?
> + /* 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) {
I'd write i++ here (and in other loops)... just for consistency with
rest of kernel.
> +/*** 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) {
{ on new line, please.
> +/* Search all DMI device names for a given type for a substrng */
> +static int __init dmi_find_substring(int type, const char *substr) {
same here.
> + struct dmi_device *dev = NULL;
unneeded initializer.
> +static int __init thinkpad_ec_init(void)
> +{
> + if (!check_dmi_for_ec()) {
> + printk(KERN_ERR "thinkpad_ec: no ThinkPad embedded controller!\n");
> + return -ENODEV;
KERN_ERR is little strong here, no?
> + if (!request_region(TPC_BASE_PORT, TPC_NUM_PORTS,
> + "thinkpad_ec"))
> + {
{ on same line, please.
Pavel
--
Thanks for all the (sleeping) penguins.
Hi!
Yes, this looks badly needed.
> 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.
>
> Signed-off-by: Shem Multinymous <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> @@ -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: %s\n", msg); goto bad; }
No.. macro with embedded goto is *evil*.
> + if (thinkpad_ec_read_row(&args, &data))
> + ABORT_INIT("read1");
> + if (data.val[0xF]!=0x00)
> + ABORT_INIT("check1");
!=0 in if is evil...
> + mode = data.val[0x1];
> +
> + printk(KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n", mode);
> + if (mode==0x00)
if !mode ?
Pavel
--
Thanks for all the (sleeping) penguins.
Hi!
> > Signed-off-by: Shem Multinymous <[email protected]>
>
> This rather defeats the purpose of the DCO. Perhaps one of the other
> project members (ie: one who uses a real name) could also sign off the
> patches?
I have communicated with Shem before, and he seems real enough to me.
I am willing to sign-them-off: in DCO sense, and will review them now.
Pavel
--
Thanks for all the (sleeping) penguins.
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 <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> +/* __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.
Hi!
> 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.
Should we perhaps remember time of last activity instead of 0/1? Aha,
that is how it is implemented, but would not time value be better
userland interface, too?
Ok... changing userland interface should be separate patch, anyway.
And should not hdapsd get it from input interface?
Signed-off-by: Pavel Machek <[email protected]>
--
Thanks for all the (sleeping) penguins.
Hi!
> 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", which is nonsensical. We have no use for that previous
> readout, so it should be ignored.
>
> 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 <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> + if (data.val[EC_ACCEL_IDX_READOUTS]<1)
Could we get spaces around '<'?
> + int total, ret;
> + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
Could we go from 0 to timeout, not the other way around?
> 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);
This actually changes userland interface... but that is probably okay.
Pavel
--
Thanks for all the (sleeping) penguins.
Hi Pavel,
Thanks for the sign-offs!
On 8/7/06, Pavel Machek <[email protected]> wrote:
> > +module_param_named(debug, tp_debug, int, 0600);
> > +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> > +
> > +/* A few macros for printk()ing: */
> > +#define DPRINTK(fmt, args...) \
> > + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
>
> Is not there generic function doing this?
None that I found. Many drivers do it this way.
> > +EXPORT_SYMBOL_GPL(thinkpad_ec_lock);
> > +EXPORT_SYMBOL_GPL(thinkpad_ec_try_lock);
> > +void thinkpad_ec_unlock(void)
> > +{
> > + up(&thinkpad_ec_mutex);
> > +}
> > +
>
> Do we need these wrappers? Perhaps just directly exporting the mutex?
Yes, we may end up needing to lock away other systems (ACPI?) that
touch the same ports. Apparently not an issue right now, but could
change with new firmware.
Also, that's what Alan Cox told me to do. :-)
> > + for (i=0; i<TPC_REQUEST_RETRIES; ++i) {
>
> I'd write i++ here (and in other loops)... just for consistency with
> rest of kernel.
OK.
"++i" is the recommended form for C++, got used to that.
> > + struct dmi_device *dev = NULL;
>
> unneeded initializer.
On a local variable?!
> > +static int __init thinkpad_ec_init(void)
> > +{
> > + if (!check_dmi_for_ec()) {
> > + printk(KERN_ERR "thinkpad_ec: no ThinkPad embedded controller!\n");
> > + return -ENODEV;
>
> KERN_ERR is little strong here, no?
Not sure what's the right one. The user tried to load a module and the
module can't do that; I saw some drivers use KERN_ERR some
KERN_WARNING in similar cases. Is there some guideline on choosing
printk levels?
OK on all other points.
Shem
On 8/7/06, Pavel Machek <[email protected]> wrote:
> > +#define ABORT_INIT(msg) { printk(KERN_ERR "hdaps init: %s\n", msg); goto bad; }
>
> No.. macro with embedded goto is *evil*.
OK. But it does makes the init function much longer and harder to read.
> > + if (data.val[0xF]!=0x00)
> > + ABORT_INIT("check1");
>
> !=0 in if is evil...
Sure, when zero is special, like for booleans or integer or pointers.
But this is a status byte value, I don't want to mistreat it just
because all its bits are unset. Otherwise, imagine the non-systematic
mess this will become:
if (data.val[0x1]!=0x00 ||
data.val[0x2]!=0x60 ||
data.val[0x3]!=0x00 ||
data.val[0xF]!=0x00)
ABORT_INIT("check2");
> > + if (mode==0x00)
>
> if !mode ?
Likewise.
Shem
On 8/7/06, Pavel Machek <[email protected]> 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
On 8/7/06, Pavel Machek <[email protected]> wrote:
> > 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.
> And should not hdapsd get it from input interface?
At this point the disk may parked and its queue frozen, and may depend
on the keyboard/mouse activity to be released. So hdapsd mustn't do
anything that involves potential swapping or userspace with unlocked
memory, and the input infrastructure is too large and flexible for
such a guarantee. I guess this is why IBM provided this low-level hook
at the embedded controller level.
Shem
On 2006.08.07 18:13:06 +0300, Shem Multinymous wrote:
> >> + struct dmi_device *dev = NULL;
> >
> >unneeded initializer.
>
> On a local variable?!
You assign a new value to it on the next line, without ever using its
initial value.
Bj?rn
Hi Pavel,
On 8/7/06, Pavel Machek <[email protected]> wrote:
> > + int total, ret;
> > + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
>
> Could we go from 0 to timeout, not the other way around?
Sure.
(That's actually vanilla hdapsd code, moved around...)
> This actually changes userland interface... but that is probably okay.
Those two sysfs attributes were bogus. If anything used them (which I
very much doubt), it's a good thing we broke it.
OK on the rest.
Shem
On 8/7/06, Bj?rn Steinbrink <[email protected]> wrote:
> On 2006.08.07 18:13:06 +0300, Shem Multinymous wrote:
> > >> + struct dmi_device *dev = NULL;
> > >unneeded initializer.
> > On a local variable?!
>
> You assign a new value to it on the next line, without ever using its
> initial value.
The initial value is used in the last parameter to dmi_find_device():
struct dmi_device *dev = NULL;
while ((dev = dmi_find_device(type, NULL, dev))) {
if (strstr(dev->name, substr))
return 1;
}
Shem
On 2006.08.07 19:41:11 +0300, Shem Multinymous wrote:
> On 8/7/06, Bj?rn Steinbrink <[email protected]> wrote:
> >On 2006.08.07 18:13:06 +0300, Shem Multinymous wrote:
> >> >> + struct dmi_device *dev = NULL;
> >> >unneeded initializer.
> >> On a local variable?!
> >
> >You assign a new value to it on the next line, without ever using its
> >initial value.
>
> The initial value is used in the last parameter to dmi_find_device():
>
> struct dmi_device *dev = NULL;
> while ((dev = dmi_find_device(type, NULL, dev))) {
> if (strstr(dev->name, substr))
> return 1;
> }
Sorry, somehow my brain skipped the end of the line.
Bj?rn
On 2006.08.07 19:30:55 +0300, Shem Multinymous wrote:
> Hi Pavel,
>
> On 8/7/06, Pavel Machek <[email protected]> wrote:
> >> + int total, ret;
> >> + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
> >
> >Could we go from 0 to timeout, not the other way around?
>
> Sure.
> (That's actually vanilla hdapsd code, moved around...)
Maybe you could convert that to sth. like this along the way?
int ret;
unsigned long timeout = jiffies + msec_to_jiffies(READ_TIMEOUT_MSECS);
for (;;) {
ret = thinkpad_ec_lock();
if (ret)
return ret;
ret = __hdaps_update(0);
thinkpad_ec_unlock();
if (ret != -EBUSY)
return ret;
if (time_after(timeout, jiffies))
break;
msleep(RETRY_MSECS);
}
return ret;
Rationale: http://lkml.org/lkml/2005/7/14/133 - it's also listed on the
kerneljanitors todo list.
Regards
Bj?rn
On Mon, 7 Aug 2006 13:26:29 +0000
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > > What more is needed that may be realistically expected from a kernel
> > > patch submission?
> >
> > We who accept the submission would be making a joke of the whole thing if
> > we accepted the assurances of a person who is concealing his/her identity.
> >
> > I suggested a simple solution: Perhaps one of the other project members
> > (ie: one who uses a real name) could also sign off the patches?
>
> I'm willing to sign off these patches. (In legal sense, anyway. I have
> not yet went through them carefully).
>
Thanks. So this will amount to Pavel asserting that this code is kosher,
based upon the knowledge which you've gained from participating in this
project.
I'll run that by Linus when he resurfaces.
And I'll duck this version of the patch series due to your code review
comments. Please cc me on version 2.
Hi!
> Thanks for the sign-offs!
No problem.
> >> +module_param_named(debug, tp_debug, int, 0600);
> >> +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> >> +
> >> +/* A few macros for printk()ing: */
> >> +#define DPRINTK(fmt, args...) \
> >> + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
> >
> >Is not there generic function doing this?
>
> None that I found. Many drivers do it this way.
linux/kernel.h : pr_debug() looks similar.
> >> +EXPORT_SYMBOL_GPL(thinkpad_ec_lock);
> >> +EXPORT_SYMBOL_GPL(thinkpad_ec_try_lock);
> >> +void thinkpad_ec_unlock(void)
> >> +{
> >> + up(&thinkpad_ec_mutex);
> >> +}
> >> +
> >
> >Do we need these wrappers? Perhaps just directly exporting the mutex?
>
> Yes, we may end up needing to lock away other systems (ACPI?) that
> touch the same ports. Apparently not an issue right now, but could
> change with new firmware.
> Also, that's what Alan Cox told me to do. :-)
Okay... but do we really need try_lock variant? lock/unlock would be
okay, but what is try_lock semantics when taking multiple locks...?
> >> + struct dmi_device *dev = NULL;
> >
> >unneeded initializer.
>
> On a local variable?!
You were right, but see the other mail.
> >> +static int __init thinkpad_ec_init(void)
> >> +{
> >> + if (!check_dmi_for_ec()) {
> >> + printk(KERN_ERR "thinkpad_ec: no ThinkPad embedded
> >controller!\n");
> >> + return -ENODEV;
> >
> >KERN_ERR is little strong here, no?
>
> Not sure what's the right one. The user tried to load a module and the
> module can't do that; I saw some drivers use KERN_ERR some
> KERN_WARNING in similar cases. Is there some guideline on choosing
> printk levels?
Well, this will also trigger for thinkpad module compiled into kernel,
right?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > >> >> + struct dmi_device *dev = NULL;
> > >> >unneeded initializer.
> > >> On a local variable?!
> > >
> > >You assign a new value to it on the next line, without ever using its
> > >initial value.
> >
> > The initial value is used in the last parameter to dmi_find_device():
> >
> > struct dmi_device *dev = NULL;
> > while ((dev = dmi_find_device(type, NULL, dev))) {
> > if (strstr(dev->name, substr))
> > return 1;
> > }
>
> Sorry, somehow my brain skipped the end of the line.
Ahha, sorry about that, I was blind, too. I thought it is because the
code is ugly, but now I see I was wrong.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > > > What more is needed that may be realistically expected from a kernel
> > > > patch submission?
> > >
> > > We who accept the submission would be making a joke of the whole thing if
> > > we accepted the assurances of a person who is concealing his/her identity.
> > >
> > > I suggested a simple solution: Perhaps one of the other project members
> > > (ie: one who uses a real name) could also sign off the patches?
> >
> > I'm willing to sign off these patches. (In legal sense, anyway. I have
> > not yet went through them carefully).
> >
>
> Thanks. So this will amount to Pavel asserting that this code is kosher,
> based upon the knowledge which you've gained from participating in this
> project.
Yes.
> I'll run that by Linus when he resurfaces.
Thanks.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> >> +#define ABORT_INIT(msg) { printk(KERN_ERR "hdaps init: %s\n", msg);
> >goto bad; }
> >
> >No.. macro with embedded goto is *evil*.
>
> OK. But it does makes the init function much longer and harder to
>read.
If you keep { printk(); goto } on one line... it is technically
codingstyle violation but it is certainly better than evil macro.
> >> + if (data.val[0xF]!=0x00)
> >> + ABORT_INIT("check1");
> >
> >!=0 in if is evil...
>
> Sure, when zero is special, like for booleans or integer or pointers.
> But this is a status byte value, I don't want to mistreat it just
> because all its bits are unset. Otherwise, imagine the non-systematic
> mess this will become:
Okay, I'd only do it in "single" cases. I guess you have point about
unsigned char not being boolean.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Aug 08, 2006 at 01:15:57AM +0200, Pavel Machek wrote:
> Hi!
>
> > Thanks for the sign-offs!
>
> No problem.
>
> > >> +module_param_named(debug, tp_debug, int, 0600);
> > >> +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> > >> +
> > >> +/* A few macros for printk()ing: */
> > >> +#define DPRINTK(fmt, args...) \
> > >> + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
> > >
> > >Is not there generic function doing this?
> >
> > None that I found. Many drivers do it this way.
>
> linux/kernel.h : pr_debug() looks similar.
Use dev_dbg() and friends please instead of rolling your own.
thanks,
greg k-h
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
On Mon 2006-08-07 16:23:30, Greg KH wrote:
> On Tue, Aug 08, 2006 at 01:15:57AM +0200, Pavel Machek wrote:
> > Hi!
> >
> > > Thanks for the sign-offs!
> >
> > No problem.
> >
> > > >> +module_param_named(debug, tp_debug, int, 0600);
> > > >> +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> > > >> +
> > > >> +/* A few macros for printk()ing: */
> > > >> +#define DPRINTK(fmt, args...) \
> > > >> + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
> > > >
> > > >Is not there generic function doing this?
> > >
> > > None that I found. Many drivers do it this way.
> >
> > linux/kernel.h : pr_debug() looks similar.
>
> Use dev_dbg() and friends please instead of rolling your own.
Ahha, okay, dev_dbg() looks even better. (But we have pr_debug in
linux/kernel.h; if it should not be used, comment would be nice).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon 2006-08-07 19:30:55, Shem Multinymous wrote:
> Hi Pavel,
>
> On 8/7/06, Pavel Machek <[email protected]> wrote:
> >> + int total, ret;
> >> + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
> >
> >Could we go from 0 to timeout, not the other way around?
>
> Sure.
> (That's actually vanilla hdapsd code, moved around...)
Thanks for cleaning it up :-).
> >This actually changes userland interface... but that is probably okay.
>
> Those two sysfs attributes were bogus. If anything used them (which I
> very much doubt), it's a good thing we broke it.
Okay, just make sure you note this in the changelog.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Aug 08, 2006 at 01:25:20AM +0200, Pavel Machek wrote:
> On Mon 2006-08-07 16:23:30, Greg KH wrote:
> > On Tue, Aug 08, 2006 at 01:15:57AM +0200, Pavel Machek wrote:
> > > Hi!
> > >
> > > > Thanks for the sign-offs!
> > >
> > > No problem.
> > >
> > > > >> +module_param_named(debug, tp_debug, int, 0600);
> > > > >> +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> > > > >> +
> > > > >> +/* A few macros for printk()ing: */
> > > > >> +#define DPRINTK(fmt, args...) \
> > > > >> + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
> > > > >
> > > > >Is not there generic function doing this?
> > > >
> > > > None that I found. Many drivers do it this way.
> > >
> > > linux/kernel.h : pr_debug() looks similar.
> >
> > Use dev_dbg() and friends please instead of rolling your own.
>
> Ahha, okay, dev_dbg() looks even better. (But we have pr_debug in
> linux/kernel.h; if it should not be used, comment would be nice).
Use pr_debug if you aren't doing driver things. But if you have a
device, please don't use it.
I guess we could add a comment for it, as if anyone would notice it...
thanks,
greg k-h
Hi!
> > On 8/7/06, Pavel Machek <[email protected]> wrote:
> > >> + int total, ret;
> > >> + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
> > >
> > >Could we go from 0 to timeout, not the other way around?
> >
> > Sure.
> > (That's actually vanilla hdapsd code, moved around...)
>
> Maybe you could convert that to sth. like this along the way?
>
> int ret;
> unsigned long timeout = jiffies + msec_to_jiffies(READ_TIMEOUT_MSECS);
> for (;;) {
> ret = thinkpad_ec_lock();
> if (ret)
> return ret;
> ret = __hdaps_update(0);
> thinkpad_ec_unlock();
>
> if (ret != -EBUSY)
> return ret;
[imagine TIMEOUT_MSEC pause here, SMM does its job?]
> if (time_after(timeout, jiffies))
> break;
> msleep(RETRY_MSECS);
> }
> return ret;
>
> Rationale: http://lkml.org/lkml/2005/7/14/133 - it's also listed on the
> kerneljanitors todo list.
Please don't. New variant is _wrong_. Someone should tell
kerneljanitors :-) ... aha, and Linus.
Minimal fix would be to run one more iteration after timeout.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
pr_debug() should not be used from drivers, add comment saying that.
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 181c69c..1b5f238 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -210,6 +210,7 @@ extern enum system_states {
extern void dump_stack(void);
#ifdef DEBUG
+/* If you are writing a driver, please use dev_dbg, instead */
#define pr_debug(fmt,arg...) \
printk(KERN_DEBUG fmt,##arg)
#else
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 7 Aug 2006 13:44:41 +0000 Pavel Machek wrote:
> Hi!
>
> > 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 <[email protected]>
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> > +/* Module parameters: */
> > +static int tp_debug = 0;
>
> Static variables do not need initializers.
>
> > +module_param_named(debug, tp_debug, int, 0600);
> > +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> > +
> > +/* A few macros for printk()ing: */
> > +#define DPRINTK(fmt, args...) \
> > + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
>
> Is not there generic function doing this?
>
> > +/* thinkpad_ec_lock:
> > + * Get exclusive lock for accesing the controller. May sleep.
> > + * Returns 0 iff lock acquired .
> > + */
>
> Linuxdoc?
Yes. Usually known as kernel-doc.
See Documenation/kernel-doc-nano-HOWTO.txt for more info.
Thanks.
---
~Randy
On 8/8/06, Pavel Machek <[email protected]> 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
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
On 8/8/06, Pavel Machek <[email protected]> wrote:
> Okay... but do we really need try_lock variant?
We need a nonlocking, nonsleeping variant to do the query in the timer
function (softirq context).
> but what is try_lock semantics when taking multiple locks...?
Currently, the same as the undelying down_trylock().
> > >> + if (!check_dmi_for_ec()) {
> > >> + printk(KERN_ERR "thinkpad_ec: no ThinkPad embedded
> > >controller!\n");
> > >> + return -ENODEV;
> > >
> > >KERN_ERR is little strong here, no?
> >
> > Not sure what's the right one. The user tried to load a module and the
> > module can't do that; I saw some drivers use KERN_ERR some
> > KERN_WARNING in similar cases. Is there some guideline on choosing
> > printk levels?
>
> Well, this will also trigger for thinkpad module compiled into kernel,
> right?
OK, I'm changing the DMI failure to KERN_WARNING. Subsequent hardware
checks remains KERN_ERR, since failing those after passing the DMI
check really is abnormal (and indicative of danger).
Shem
Hi!
> >Okay... but do we really need try_lock variant?
>
> We need a nonlocking, nonsleeping variant to do the query in the timer
> function (softirq context).
>
> >but what is try_lock semantics when taking multiple locks...?
>
> Currently, the same as the undelying down_trylock().
Okay, I guess this works for me.
> >Well, this will also trigger for thinkpad module compiled into kernel,
> >right?
>
> OK, I'm changing the DMI failure to KERN_WARNING. Subsequent hardware
> checks remains KERN_ERR, since failing those after passing the DMI
> check really is abnormal (and indicative of danger).
Yep, that sounds correct.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 8/8/06, Greg KH <[email protected]> wrote:
> > > >> +module_param_named(debug, tp_debug, int, 0600);
> > > >> +MODULE_PARM_DESC(debug, "Debug level (0=off, 1=on)");
> > > >> +
> > > >> +/* A few macros for printk()ing: */
> > > >> +#define DPRINTK(fmt, args...) \
> > > >> + do { if (tp_debug) printk(KERN_DEBUG fmt, ## args); } while (0)
> > > >
> > > >Is not there generic function doing this?
> > >
> > > None that I found. Many drivers do it this way.
> >
> > linux/kernel.h : pr_debug() looks similar.
>
> Use dev_dbg() and friends please instead of rolling your own.
But both pr_debug() and dev_dbg() do a static "#ifdef DEBUG", not a
runtime check of the 'debug' module paraeter.
Anyway, it turns out I don't really have any interesting debug outputs
anymore, so I'm eliminating DPRINTK() and the 'debug' module
parameter.
I'm still rolling my own printk formatting macro because thinkpad_ec
doesn't have a device for dev_dbg().
Shem
On 8/8/06, Pavel Machek <[email protected]> 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
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
On Mon, Aug 07, 2006 at 08:20:47PM +0200, Bj?rn Steinbrink wrote:
> On 2006.08.07 19:30:55 +0300, Shem Multinymous wrote:
> > Hi Pavel,
> >
> > On 8/7/06, Pavel Machek <[email protected]> wrote:
> > >> + int total, ret;
> > >> + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
> > >
> > >Could we go from 0 to timeout, not the other way around?
> >
> > Sure.
> > (That's actually vanilla hdapsd code, moved around...)
>
> Maybe you could convert that to sth. like this along the way?
>
> int ret;
> unsigned long timeout = jiffies + msec_to_jiffies(READ_TIMEOUT_MSECS);
> for (;;) {
> ret = thinkpad_ec_lock();
> if (ret)
> return ret;
Just in case someone was going to cut and paste, this will return with
the ec_lock taken.
Cheers,
Muli
Hi!
> 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 <[email protected]>
Looks ok.
Signed-off-by: Pavel Machek <[email protected]>
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> @@ -68,6 +67,13 @@ static struct input_dev *hdaps_idev;
> static unsigned int hdaps_invert;
> static int needs_calibration = 0;
Unneccessary initializer.
> +/* 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 = 0; /* Enable EC fake data mode? */
Here too.
> @@ -162,6 +168,137 @@ 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) {
{ on new line, kernel-doc.
> +/*
> + * hdaps_set_fake_data_mode - enable or disable EC test mode, which fakes
> + * accelerometer data using an incrementing counter.
> + * Returns zero on success and negative error code on failure. Can sleep.
> + */
Why do we want to have fake mode? I see it is useful for debugging,
but?
> +/*
> + * 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.
> + */
URL for spec?
What happens when we delete this one?
Pavel
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 modules is
> completely unaffected, and new models will work automatically (though not
> necessarily with correct axis configuration).
>
> Signed-off-by: Shem Multinymous <[email protected]>
Looks ok.
Signed-off-by: Pavel Machek <[email protected]>
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 <[email protected]>
I'd insert fewer blank lines, otherwise patch looks ok to me.
Signed-off-by: Pavel Machek <[email protected]>
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> +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) {
> + printk(KERN_WARNING
> + "fake_data should be 0 or 1\n");
Kill the printk?
Pavel
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 <[email protected]>
Looks ok.
Signed-off-by: Pavel Machek <[email protected]>
--
Thanks for all the (sleeping) penguins.
Hi!
> 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 <[email protected]>
Signed-off-by: Pavel Machek <[email protected]>
> +/*
> + * hdaps_device_shutdown - power off the accelerometer. Can sleep.
> + */
> +static void hdaps_device_shutdown(void) {
{ on newline?
> + if (hdaps_set_power(0))
> + printk(KERN_WARNING "hdaps: cannot power off\n");
> + if (hdaps_set_ec_config(0, 1))
> + printk(KERN_WARNING "hdaps: cannot stop EC sampling\n");
> +}
Maybe propagate error value?
> +static int hdaps_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + hdaps_device_shutdown();
> + return 0;
> +}
> +
--
Thanks for all the (sleeping) penguins.
On Tue 08-08-06 15:22:34, Muli Ben-Yehuda wrote:
> On Mon, Aug 07, 2006 at 08:20:47PM +0200, Bj?rn Steinbrink wrote:
> > On 2006.08.07 19:30:55 +0300, Shem Multinymous wrote:
> > > Hi Pavel,
> > >
> > > On 8/7/06, Pavel Machek <[email protected]> wrote:
> > > >> + int total, ret;
> > > >> + for (total=READ_TIMEOUT_MSECS; total>0; total-=RETRY_MSECS) {
> > > >
> > > >Could we go from 0 to timeout, not the other way around?
> > >
> > > Sure.
> > > (That's actually vanilla hdapsd code, moved around...)
> >
> > Maybe you could convert that to sth. like this along the way?
> >
> > int ret;
> > unsigned long timeout = jiffies + msec_to_jiffies(READ_TIMEOUT_MSECS);
> > for (;;) {
> > ret = thinkpad_ec_lock();
> > if (ret)
> > return ret;
>
> Just in case someone was going to cut and paste, this will return with
> the ec_lock taken.
Well, taking lock failed (hence error return) so I think code is
correct.
--
Thanks for all the (sleeping) penguins.
On Tue, Aug 08, 2006 at 12:56:52PM +0000, Pavel Machek wrote:
> > > ret = thinkpad_ec_lock();
> > > if (ret)
> > > return ret;
> >
> > Just in case someone was going to cut and paste, this will return with
> > the ec_lock taken.
>
> Well, taking lock failed (hence error return) so I think code is
> correct.
Ugh, I missed that - it's called _lock(), but it's actually
down_interruptible(). Why not just get rid of the wrapper and call
down_interruptible() directly? That makes it obvious what's going on.
Cheers,
Muil
On 8/8/06, Pavel Machek <[email protected]> wrote:
> > static int needs_calibration = 0;
> Unneccessary initializer.
OK, though I would prefer to differentiate "initial value is 0" from
"initial value doesn't matter".
> > + * hdaps_set_fake_data_mode - enable or disable EC test mode, which fakes
> > + * accelerometer data using an incrementing counter.
> > + * Returns zero on success and negative error code on failure. Can sleep.
> > + */
>
> Why do we want to have fake mode? I see it is useful for debugging,
> but?
It's useful for debugging userspace too. Apps like the hdapsd daemon
can use it to figure out how often and how regularly they get fresh
readouts, and whether they often miss readouts.
> > +/*
> > + * 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.
> > + */
>
> URL for spec?
http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html, it's at the
top of the original file.
> What happens when we delete this one?
No idea, nor a way to check (on all relevant models). We've always
done it, this patch just does it a bit more explicitly and by
correctly following the H8S LPC protocol.
OK on all other comments to patches 06-08.
Shem
Hi,
On 8/8/06, Pavel Machek <[email protected]> wrote:
> > + if (hdaps_set_power(0))
> > + printk(KERN_WARNING "hdaps: cannot power off\n");
> > + if (hdaps_set_ec_config(0, 1))
> > + printk(KERN_WARNING "hdaps: cannot stop EC sampling\n");
> > +}
>
> Maybe propagate error value?
I could do that, but both callers will still discard it -- the power
draw change is negligible, and it's not interesting enough to abort
either suspend or module unload.
OK on all remaining comments.
I'll wait a bit for additional comments and then post the revised series.
Shem
Hi Muli,
On 8/8/06, Muli Ben-Yehuda <[email protected]> wrote:
> > > > ret = thinkpad_ec_lock();
> > > > if (ret)
> > > > return ret;
> Ugh, I missed that - it's called _lock(), but it's actually
> down_interruptible().
Why is that confusing?
> Why not just get rid of the wrapper and call
> down_interruptible() directly? That makes it obvious what's going on.
We may end up needing to lock away other subsystems (ACPI?) that
touch the same ports. Apparently not an issue right now, but could
change with new firmware. (http://lkml.org/lkml/2006/8/7/147)
Shem
On Tue, Aug 08, 2006 at 04:35:23PM +0300, Shem Multinymous wrote:
> Hi Muli,
>
> On 8/8/06, Muli Ben-Yehuda <[email protected]> wrote:
> >> > > ret = thinkpad_ec_lock();
> >> > > if (ret)
> >> > > return ret;
>
> >Ugh, I missed that - it's called _lock(), but it's actually
> >down_interruptible().
>
> Why is that confusing?
lock() sounds like spin_lock() to me, and spin_lock() can't
fail. Idiomatic code is easier for my brain to parse.
> >Why not just get rid of the wrapper and call
> >down_interruptible() directly? That makes it obvious what's going on.
>
> We may end up needing to lock away other subsystems (ACPI?) that
> touch the same ports. Apparently not an issue right now, but could
> change with new firmware. (http://lkml.org/lkml/2006/8/7/147)
When (if) it becomes necessary to lock away other subsystems, the
wrapper can be easily reintroduced.
Cheers,
Muli
On 8/8/06, Muli Ben-Yehuda <[email protected]> wrote:
> > We may end up needing to lock away other subsystems (ACPI?) that
> > touch the same ports. Apparently not an issue right now, but could
> > change with new firmware. (http://lkml.org/lkml/2006/8/7/147)
>
> When (if) it becomes necessary to lock away other subsystems, the
> wrapper can be easily reintroduced.
It's an exported function, and we already have an out-of-tree module
(awaiting future submission once the dust clears about this series)
which relies on it.
See the LKML discussion, "Subject: tp_smapi conflict with IDE, hdaps",
and especially Alan Cox's sample code for this. (That got me
embarrassed into writing the rest of thinkpad_ec...)
Shem
If the concern is just the naming then change it to end _trylock, that
fits the locking primitives we use for the kernel in general, but its
really just polishing work
On 8/8/06, Alan Cox <[email protected]> wrote:
> If the concern is just the naming then change it to end _trylock
We already have a thinkpad_ec_trylock() for the non-blocking variant.
Shem
On Tue, Aug 08, 2006 at 06:33:25PM +0300, Shem Multinymous wrote:
> On 8/8/06, Alan Cox <[email protected]> wrote:
> >If the concern is just the naming then change it to end _trylock
>
> We already have a thinkpad_ec_trylock() for the non-blocking
> variant.
thinkpad_ec_down_interruptible()?
Cheers,
Muli
On 8/9/06, Muli Ben-Yehuda <[email protected]> wrote:
> > >If the concern is just the naming then change it to end _trylock
> >
> > We already have a thinkpad_ec_trylock() for the non-blocking
> > variant.
>
> thinkpad_ec_down_interruptible()?
Just ran out, of that sir,
If we'll need to lock away ACPI (just matter of time, my guess), we'll
stumble upon the non-interruptible down()s deep inside in the ACPI
code. So we can't guarantee and shouldn't promise it's really
interruptible.
It's not that strange that a function might fail even if it's named
"do_something()", you know. It's down() that forms an exception - it's
so simple we know it can't fail.
Shem
On Wed, Aug 09, 2006 at 12:02:39PM +0300, Shem Multinymous wrote:
> If we'll need to lock away ACPI (just matter of time, my guess), we'll
> stumble upon the non-interruptible down()s deep inside in the ACPI
> code. So we can't guarantee and shouldn't promise it's really
> interruptible.
Ok, I concede :-)
> It's not that strange that a function might fail even if it's named
> "do_something()", you know. It's down() that forms an exception - it's
> so simple we know it can't fail.
>From a maintainer point of view, anything that is non-idiomatic is a
pain. Since there is no idiomatic way to express what you want, you
get to choose the idiom.
Cheeers,
Muli