ThinkPad embedded controller and hdaps drivers (version 2)
Subsystems (2)
cover ThinkPad embedded controller and hdaps drivers (version 2)
This resubmission addresses all the comments I've received on LKML, except where said otherwise in reply. Other changes are very minor.
This patch series does three related things:
- Introduces a new driver, thinkpad_ec, for the ThinkPad embedded controller (additional non-ACPI interface).
- Changes the existing hdaps driver to use thinkpad_ec instead of direct (and incorrect) port access.
- Several fixes and improvements to the hdaps driver.
Applies to git head.
Most of the patches in this series depend on previous ones. Any point in this series will compile and was briefly tested (this required introducing some code that gets overwritten by later patches). The *combination* of all these patches was extensively tested as part of the out-of-tree tp_smapi package [1], and verified to work on dozens of ThinkPad models (thousands of sf.net downloads and no unresolved bugs). Note that the tp_smapi package includes another driver, to be submitted later, which also depends the thinkpad_ec driver introduced here.
For some context and the necessity for a separate thinkpad_ec module, see LKML thread "tp_smapi conflict with IDE, hdaps" on Dec. 2005.
I would like to thank the many testers and contributors who helped in the development, and in particular Henrique de Moraes Holschuh, ThinkWiki.org users Whoopie and Spiney, and Pavel Machek.
[1] http://thinkwiki.org/wiki/tp_smapi
Summary:
drivers/firmware/Kconfig | 3 drivers/firmware/Makefile | 1 drivers/firmware/thinkpad_ec.c | 465 ++++++++++++++++++ drivers/hwmon/Kconfig | 1 drivers/hwmon/hdaps.c | 784 ++++++++++++++++++------------- include/linux/thinkpad_ec.h | 47 + 6 files changed, 990 insertions(+), 311 deletions(-)
PATCH 1/12 thinkpad_ec: New driver for ThinkPad embedded controller access
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.
---
This depends on dmi-decode-and-save-oem-string-information.patch pending in -mm ("[PATCH] DMI: Decode and save OEM String information" on LKML).
drivers/firmware/Kconfig | 3 drivers/firmware/Makefile | 1 drivers/firmware/thinkpad_ec.c | 465 +++++++++++++++++++++++++++++++++++++++++ include/linux/thinkpad_ec.h | 47 ++++ 4 files changed, 516 insertions(+)
drivers/firmware/Kconfig +3 -0
@@ -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
drivers/firmware/Makefile +1 -0
@@ -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
drivers/firmware/thinkpad_ec.c new file +465 -0
465 lines not on this page · raw message
include/linux/thinkpad_ec.h new file +47 -0
47 lines not on this page · raw message
PATCH 2/12 hdaps: Use thinkpad_ec instead of direct port access
In reply to Shem MultinymousThe hdaps module currently talks to the ThinkPad embedded controller via direct IO port access, and does so incorrectly and with insufficient IO checking. Beyond bad readouts, this could in principle, trigger known EC firmware bugs, thereby causing a firmware hang and possible hardware damage.
The just-introduced thinkpad_ec provides a safe, correct way to access these EC functions. This patch changes hdaps to use that. This also enables other drivers (e.g., the soon-to-be-submitted tp_smapi module) to access the EC without collisions.
As you can see from the comments at the beginning of the patch, the old driver got some stuff wrong about the meaning of EC readouts. This patch preserves the old broken behavior; it will be fixed in a later patch.
The one-line
{ ABORT_INIT("check4"); goto bad; }statements in hdaps_init_device() are a coding style violation, but make the init code much easier to read than otherwise.
--- drivers/hwmon/Kconfig | 1 drivers/hwmon/hdaps.c | 314 ++++++++++++++++++-------------------------------- 2 files changed, 116 insertions(+), 199 deletions(-)
drivers/hwmon/hdaps.c +115 -199
@@ -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(voistatic 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 instatic int hdaps_read_pair (unsigned int port1,unsigned int port2,int *val1,int *val2) { -int ret; - -down (&hdaps_sem); +int ret =thinkpad_ec_lock (); +if (ret) +return ret; ret =__hdaps_read_pair (port1, port2, val1, val2); -up (&hdaps_sem); - +thinkpad_ec_unlock ();return ret; }
@@ -209,77 +131,79 @@ static int hdaps_read_pair(unsigned int* hdaps_device_init - initialize the accelerometer. Returns zero on success * and negative error code on failure. Can sleep. */ +#define ABORT_INIT (msg) printk(KERN_ERR "hdaps init failed at: %s\n", msg)static int hdaps_device_init (void ) { -int total, ret = -ENXIO ; - -down (&hdaps_sem); - -outb (0x13 ,0x1610 ); -outb (0x01 ,0x161f ); -if (__wait_latch (0x161f ,0x00 )) - goto out; - -/* -* Most ThinkPads return 0x01. -* -* Others--namely the R50p, T41p, and T42p--return 0x03. These laptops -* have "inverted" axises. -* -* The 0x02 value occurs when the chip has been previously initialized. -*/ -if (__check_latch (0x1611 ,0x03 ) && -__check_latch (0x1611 ,0x02 ) && -__check_latch (0x1611 ,0x01 )) - goto out; - -printk (KERN_DEBUG "hdaps: initial latch check good (0x%02x).\n" , -__get_latch (0x1611 )); +int ret; +struct thinkpad_ec_row args, data; +u8 mode; -outb (0x17 ,0x1610 ); -outb (0x81 ,0x1611 ); -outb (0x01 ,0x161f ); -if (__wait_latch (0x161f ,0x00 )) - goto out; -if (__wait_latch (0x1611 ,0x00 )) - goto out; -if (__wait_latch (0x1612 ,0x60 )) - goto out; -if (__wait_latch (0x1613 ,0x00 )) - goto out; -outb (0x14 ,0x1610 ); -outb (0x01 ,0x1611 ); -outb (0x01 ,0x161f ); -if (__wait_latch (0x161f ,0x00 )) - goto out; -outb (0x10 ,0x1610 ); -outb (0xc8 ,0x1611 ); -outb (0x00 ,0x1612 ); -outb (0x02 ,0x1613 ); -outb (0x01 ,0x161f ); -if (__wait_latch (0x161f ,0x00 )) - goto out; -if (__device_refresh_sync ()) - goto out; -if (__wait_latch (0x1611 ,0x00 )) - goto out; + ret =thinkpad_ec_lock (); +if (ret) +return ret; -/* we have done our dance, now let's wait for the applause */ -for (total =INIT_TIMEOUT_MSECS ; total >0 ; total -=INIT_WAIT_MSECS ) { -int x, y; - -/* a read of the device helps push it into action */ -__hdaps_read_pair (HDAPS_PORT_XPOS ,HDAPS_PORT_YPOS , &x, &y); -if (!__wait_latch (0x1611 ,0x02 )) { - ret =0 ; -break ; - } + args.val[0x0 ] =0x13 ; + args.val[0xF ] =0x01 ; + args.mask=0x8001 ; + data.mask=0x8002 ; +if (thinkpad_ec_read_row (&args, &data)) + {ABORT_INIT ("read1" ); goto bad; } +if (data.val[0xF ]!=0x00 ) + {ABORT_INIT ("check1" ); goto bad; } + mode = data.val[0x1 ]; + +printk (KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n" , mode); +if (mode==0x00 ) + {ABORT_INIT ("accelerometer not available" ); goto bad; } + + args.val[0x0 ] =0x17 ; + args.val[0x1 ] =0x81 ; + args.val[0xF ] =0x01 ; + args.mask =0x8003 ; + data.mask =0x800E ; +if (thinkpad_ec_read_row (&args, &data)) + {ABORT_INIT ("read2" ); goto bad; } +if (data.val[0x1 ]!=0x00 || + data.val[0x2 ]!=0x60 || + data.val[0x3 ]!=0x00 || + data.val[0xF ]!=0x00 ) + {ABORT_INIT ("check2" ); goto bad; } + + args.val[0x0 ] =0x14 ; + args.val[0x1 ] =0x01 ; + args.val[0xF ] =0x01 ; + args.mask =0x8003 ; + data.mask =0x8000 ; +if (thinkpad_ec_read_row (&args, &data)) + {ABORT_INIT ("read3" ); goto bad; } +if (data.val[0xF ]!=0x00 ) + {ABORT_INIT ("check3" ); goto bad; } + + args.val[0x0 ] =0x10 ; + args.val[0x1 ] =0xc8 ; + args.val[0x2 ] =0x00 ; + args.val[0x3 ] =0x02 ; + args.val[0xF ] =0x01 ; + args.mask =0x800F ; + data.mask =0x8000 ; +if (thinkpad_ec_read_row (&args, &data)) + {ABORT_INIT ("read4" ); goto bad; } +if (data.val[0xF ]!=0x00 ) + {ABORT_INIT ("check4" ); goto bad; } -msleep (INIT_WAIT_MSECS ); - } +thinkpad_ec_invalidate (); +udelay (200 ); -out: -up (&hdaps_sem); +/* Just prefetch instead of reading, to avoid ~1sec delay on load */ + ret =thinkpad_ec_prefetch_row (&ec_accel_args); +if (ret) + {ABORT_INIT ("initial prefetch failed" ); goto bad; } + goto good; +bad: +thinkpad_ec_invalidate (); + ret = -ENXIO ; +good: +thinkpad_ec_unlock ();return ret; }
@@ -317,7 +241,7 @@ static struct platform_driver hdaps_driv*/ static void hdaps_calibrate (void ) { -__hdaps_read_pair (HDAPS_PORT_XPOS ,HDAPS_PORT_YPOS , &rest_x, &rest_y); +__hdaps_read_pair (EC_ACCEL_IDX_XPOS1 ,EC_ACCEL_IDX_YPOS1 , &rest_x, &rest_y); }static void hdaps_mousedev_poll (unsigned long unused)
@@ -325,12 +249,12 @@ static void hdaps_mousedev_poll(unsignedint 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(unsignedmod_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 du8 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 du8 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(strstruct 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" ); }
drivers/hwmon/Kconfig +1 -0
@@ -494,6 +494,7 @@ config SENSORS_W83627EHF config SENSORS_HDAPS tristate "IBM Hard Drive Active Protection System (hdaps)" depends on HWMON && INPUT && X86 + select THINKPAD_EC default n help This driver provides support for the IBM Hard Drive Active Protection
PATCH 3/12 hdaps: Unify and cache hdaps readouts
In reply to Shem MultinymousThe current hdaps driver queries the hardware on (almost) any sysfs read, reading just the information it needs and discarding the rest This is inefficient, because every hardware query actually gives all information. It also means we're losing data, because readouts are offered by the hardware at a constant rate and each query "eats up" a readout. It also results in unnecessarily complex code.
This patch moves all hardware value reading+parsing to a single function, __hdaps_update(). All values are cached, and easily referenced afterwards. This function is still invoked on every sysfs read. This will be fixed in a later patch.
There's a minor race condition here (and and also in the original code):
the global int pos_x, rest_x etc. could be updated while an attribute's show_* function is reading them. This will be fixed in a future patch.
--- drivers/hwmon/hdaps.c | 173 ++++++++++++++++++++++++++------------------------ 1 file changed, 90 insertions(+), 83 deletions(-)
drivers/hwmon/hdaps.c +90 -83
@@ -63,73 +63,89 @@ static struct timer_list hdaps_timer;static struct platform_device *pdev;static struct input_dev *hdaps_idev;static unsigned int hdaps_invert; + +/* Latest state readout */ +static int pos_x, pos_y;/* position */ +static int var_x, var_y;/* variance (what is this?) */ +static u8 temp1, temp2;/* temperatures */ static u8 km_activity; -static int rest_x; -static int rest_y; +static int rest_x, rest_y;/* calibrated rest position */ -/* -* hdaps_readb_one - reads a byte from a single I/O port, placing the value in -* the given pointer. Returns zero on success or a negative error on failure. -* Can sleep. -*/ -static int hdaps_readb_one (unsigned int port,u8 *val) +/* Some models require an axis transformation to the standard reprsentation */ +void transform_axes (int *x,int *y) { -int ret; -struct thinkpad_ec_row data; - - ret =thinkpad_ec_lock (); -if (ret) -return ret; - data.mask = (1 << port); - ret =thinkpad_ec_read_row (&ec_accel_args, &data); - *val = data.val[port]; -thinkpad_ec_unlock (); -return ret; +if (hdaps_invert) { + *x = -*x; + *y = -*y; + } } -/* __hdaps_read_pair - internal lockless helper for hdaps_read_pair(). */ -static int __hdaps_read_pair (unsigned int port1,unsigned int port2, -int *x,int *y) +/** +* __hdaps_update - query current state, with locks already acquired +* @fast: if nonzero, do one quick attempt without retries. +* +* Query current accelerometer state and update global state variables. +* Also prefetches the next query. Caller must hold controller lock. +*/ +static int __hdaps_update (int fast) { -int ret; +/* Read data: */ struct thinkpad_ec_row data; +int ret; - data.mask = (3 << port1) | (3 << port2) | (1 <<EC_ACCEL_IDX_KMACT ); - ret =thinkpad_ec_read_row (&ec_accel_args, &data); + data.mask = (1 <<EC_ACCEL_IDX_KMACT ) | + (3 <<EC_ACCEL_IDX_YPOS1 ) | (3 <<EC_ACCEL_IDX_XPOS1 ) | + (3 <<EC_ACCEL_IDX_YPOS2 ) | (3 <<EC_ACCEL_IDX_XPOS2 ) | + (1 <<EC_ACCEL_IDX_TEMP1 ) | (1 <<EC_ACCEL_IDX_TEMP2 ); +if (fast) + ret =thinkpad_ec_try_read_row (&ec_accel_args, &data); +else + ret =thinkpad_ec_read_row (&ec_accel_args, &data); +thinkpad_ec_prefetch_row (&ec_accel_args);/* Prefetch even if error */ if (ret)return ret; - *x = *(s16 *)(data.val+port1); - *y = *(s16 *)(data.val+port2); +/* Parse position data: */ + pos_x = *(s16 *)(data.val+EC_ACCEL_IDX_XPOS1 ); + pos_y = *(s16 *)(data.val+EC_ACCEL_IDX_YPOS1 ); +transform_axes (&pos_x, &pos_y); + +/* Parse so-called "variance" data: */ + var_x = *(s16 *)(data.val+EC_ACCEL_IDX_XPOS2 ); + var_y = *(s16 *)(data.val+EC_ACCEL_IDX_YPOS2 ); +transform_axes (&var_x, &var_y); + km_activity = data.val[EC_ACCEL_IDX_KMACT ]; -/* if hdaps_invert is set, negate the two values */ -if (hdaps_invert) { - *x = -*x; - *y = -*y; - } + temp1 = data.val[EC_ACCEL_IDX_TEMP1 ]; + temp2 = data.val[EC_ACCEL_IDX_TEMP2 ];return 0 ; } -/* -* hdaps_read_pair - reads the values from a pair of ports, placing the values -* in the given pointers. Returns zero on success. Can sleep. +/** +* hdaps_update - acquire locks and query current state +* +* Query current accelerometer state and update global state variables. +* Also prefetches the next query. +* Does its own locking. */ -static int hdaps_read_pair (unsigned int port1,unsigned int port2, -int *val1,int *val2) +static int hdaps_update (void ) {int ret =thinkpad_ec_lock ();if (ret)return ret; - ret =__hdaps_read_pair (port1, port2, val1, val2); + ret =__hdaps_update (0 );thinkpad_ec_unlock ();return ret; } -/* -* hdaps_device_init - initialize the accelerometer. Returns zero on success -* and negative error code on failure. Can sleep. +/** +* hdaps_device_init - initialize the accelerometer. +* +* Call several embedded controller functions to test and initialize the +* accelerometer. +* Returns zero on success and negative error code on failure. Can sleep. */ #define ABORT_INIT (msg) printk(KERN_ERR "hdaps init failed at: %s\n", msg)static int hdaps_device_init (void )
@@ -236,35 +252,43 @@ static struct platform_driver hdaps_driv }, }; -/* -* hdaps_calibrate - Set our "resting" values. Callers must hold hdaps_sem. +/** +* hdaps_calibrate - set our "resting" values. +* Does its own locking. */ static void hdaps_calibrate (void ) { -__hdaps_read_pair (EC_ACCEL_IDX_XPOS1 ,EC_ACCEL_IDX_YPOS1 , &rest_x, &rest_y); +hdaps_update (); + rest_x = pos_x; + rest_y = pos_y; } +/* Timer handler for updating the input device. Runs in softirq context, +* so avoid lenghty or blocking operations. +*/ static void hdaps_mousedev_poll (unsigned long unused) { -int x, y; +int ret;/* Cannot sleep. Try nonblockingly. If we fail, try again later. */ -if (thinkpad_ec_try_lock ()) { -mod_timer (&hdaps_timer,jiffies +HDAPS_POLL_PERIOD ); +if (thinkpad_ec_try_lock ()) + goto keep_active; + + ret =__hdaps_update (1 );/* fast update, we're in softirq context */ +thinkpad_ec_unlock (); +/* Any of "successful", "not yet ready" and "not prefetched"? */ +if (ret!=0 && ret!=-EBUSY && ret!=-ENODATA ) { +printk (KERN_ERR +"hdaps: poll failed, disabling mousedev updates\n" );return ; } -if (__hdaps_read_pair (EC_ACCEL_IDX_XPOS1 ,EC_ACCEL_IDX_YPOS1 , &x, &y)) - goto out; - -input_report_abs (hdaps_idev,ABS_X , x - rest_x); -input_report_abs (hdaps_idev,ABS_Y , y - rest_y); +keep_active: +/* Even if we failed now, pos_x,y may have been updated earlier: */ +input_report_abs (hdaps_idev,ABS_X , pos_x - rest_x); +input_report_abs (hdaps_idev,ABS_Y , pos_y - rest_y);input_sync (hdaps_idev); -mod_timer (&hdaps_timer, jiffies +HDAPS_POLL_PERIOD ); - -out: -thinkpad_ec_unlock (); }
@@ -273,51 +297,37 @@ out:static ssize_t hdaps_position_show (struct device *dev,struct device_attribute *attr,char *buf) { -int ret, x, y; - - ret =hdaps_read_pair (EC_ACCEL_IDX_XPOS1 ,EC_ACCEL_IDX_YPOS1 , &x, &y); +int ret =hdaps_update ();if (ret)return ret; - -return sprintf (buf,"(%d,%d)\n" , x, y); +return sprintf (buf,"(%d,%d)\n" , pos_x, pos_y); }static ssize_t hdaps_variance_show (struct device *dev,struct device_attribute *attr,char *buf) { -int ret, x, y; - - ret =hdaps_read_pair (EC_ACCEL_IDX_XPOS2 ,EC_ACCEL_IDX_YPOS2 , &x, &y); +int ret =hdaps_update ();if (ret)return ret; - -return sprintf (buf,"(%d,%d)\n" , x, y); +return sprintf (buf,"(%d,%d)\n" , var_x, var_y); }static ssize_t hdaps_temp1_show (struct device *dev,struct device_attribute *attr,char *buf) { -u8 temp; -int ret; - - ret =hdaps_readb_one (EC_ACCEL_IDX_TEMP1 , &temp); -if (ret <0 ) +int ret =hdaps_update (); +if (ret)return ret; - -return sprintf (buf,"%u\n" , temp); +return sprintf (buf,"%u\n" , temp1); }static ssize_t hdaps_temp2_show (struct device *dev,struct device_attribute *attr,char *buf) { -u8 temp; -int ret; - - ret =hdaps_readb_one (EC_ACCEL_IDX_TEMP2 , &temp); -if (ret <0 ) +int ret =hdaps_update (); +if (ret)return ret; - -return sprintf (buf,"%u\n" , temp); +return sprintf (buf,"%u\n" , temp2); } static ssize_t hdaps_keyboard_activity_show(struct device *dev,
@@ -344,10 +354,7 @@ static ssize_t hdaps_calibrate_store(strstruct device_attribute *attr, const char *buf, size_t count) { - if (thinkpad_ec_lock()) - return -EIO;hdaps_calibrate (); -thinkpad_ec_unlock ();return count; }
PATCH 4/12 hdaps: Correct readout and remove nonsensical attributes
In reply to Shem MultinymousThe current hdaps driver got some details wrong about the result of hardware queries.
First, it fails to check a couple of status values.
Second, it turns out that the hardware will return up to two readouts:
the latest one and also the previous one if it was missed (host didn't poll fast enough). The current driver wrongly interprets the latter as a "variance". We have no use for that previous readout, so it should be ignored. The corresponding sysfs attributes, "temp2" and "variance" are bogus and thus removed.
This patch adds proper status checking, and removes the "variance" and "temp2" sysfs attributes which refer to the old readout.
--- drivers/hwmon/hdaps.c | 75 +++++++++++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 40 deletions(-)
drivers/hwmon/hdaps.c +35 -40
@@ -49,12 +49,16 @@ static const struct thinkpad_ec_row ec_a#define EC_ACCEL_IDX_TEMP2 0xb/* device temperature in Celsius */ #define EC_ACCEL_IDX_QUEUED 0xc/* Number of queued readouts left */ #define EC_ACCEL_IDX_KMACT 0xd/* keyboard or mouse activity */ +#define EC_ACCEL_IDX_RETVAL 0xf/* command return value, good=0x00 */ #define KEYBD_MASK 0x20/* set if keyboard activity */ #define MOUSE_MASK 0x40/* set if mouse activity */ #define KEYBD_ISSET (n) (!! (n & KEYBD_MASK))/* keyboard used? */ #define MOUSE_ISSET (n) (!! (n & MOUSE_MASK))/* mouse used? */ +#define READ_TIMEOUT_MSECS 100/* wait this long for device read */ +#define RETRY_MSECS 3/* retry delay */ +#define HDAPS_POLL_PERIOD (HZ/20)/* poll for input every 1/20s */ #define HDAPS_INPUT_FUZZ 4/* input event threshold */ #define HDAPS_INPUT_FLAT 4
@@ -66,8 +70,7 @@ static unsigned int hdaps_invert;/* Latest state readout */ static int pos_x, pos_y;/* position */ -static int var_x, var_y;/* variance (what is this?) */ -static u8 temp1, temp2;/* temperatures */ +static int temperature;/* temperature */ static u8 km_activity;static int rest_x, rest_y;/* calibrated rest position */
@@ -93,10 +96,9 @@ static int __hdaps_update(int fast)struct thinkpad_ec_row data;int ret; - data.mask = (1 <<EC_ACCEL_IDX_KMACT ) | + data.mask = (1 <<EC_ACCEL_IDX_READOUTS ) | (1 <<EC_ACCEL_IDX_KMACT ) | (3 <<EC_ACCEL_IDX_YPOS1 ) | (3 <<EC_ACCEL_IDX_XPOS1 ) | - (3 <<EC_ACCEL_IDX_YPOS2 ) | (3 <<EC_ACCEL_IDX_XPOS2 ) | - (1 <<EC_ACCEL_IDX_TEMP1 ) | (1 <<EC_ACCEL_IDX_TEMP2 ); + (1 <<EC_ACCEL_IDX_TEMP1 ) | (1 <<EC_ACCEL_IDX_RETVAL );if (fast) ret =thinkpad_ec_try_read_row (&ec_accel_args, &data);else
@@ -105,20 +107,24 @@ static int __hdaps_update(int fast)if (ret)return ret; +/* Check status: */ +if (data.val[EC_ACCEL_IDX_RETVAL ] !=0x00 ) { +printk (KERN_WARNING "hdaps: read RETVAL=0x%02x\n" , + data.val[EC_ACCEL_IDX_RETVAL ]); +return -EIO ; + } + +if (data.val[EC_ACCEL_IDX_READOUTS ] <1 ) +return -EBUSY ;/* no pending readout, try again later */ +/* Parse position data: */ pos_x = *(s16 *)(data.val+EC_ACCEL_IDX_XPOS1 ); pos_y = *(s16 *)(data.val+EC_ACCEL_IDX_YPOS1 );transform_axes (&pos_x, &pos_y); -/* Parse so-called "variance" data: */ - var_x = *(s16 *)(data.val+EC_ACCEL_IDX_XPOS2 ); - var_y = *(s16 *)(data.val+EC_ACCEL_IDX_YPOS2 ); -transform_axes (&var_x, &var_y); - km_activity = data.val[EC_ACCEL_IDX_KMACT ]; - temp1 = data.val[EC_ACCEL_IDX_TEMP1 ]; - temp2 = data.val[EC_ACCEL_IDX_TEMP2 ]; + temperature = data.val[EC_ACCEL_IDX_TEMP1 ];return 0 ; }
@@ -128,15 +134,25 @@ static int __hdaps_update(int fast)* * Query current accelerometer state and update global state variables. * Also prefetches the next query. +* Retries until timeout if the accelerometer is not in ready status (common). * Does its own locking. */ static int hdaps_update (void ) { -int ret =thinkpad_ec_lock (); -if (ret) -return ret; - ret =__hdaps_update (0 ); -thinkpad_ec_unlock (); +int total, ret; +for (total=0 ; total<READ_TIMEOUT_MSECS ; total+=RETRY_MSECS ) { + ret =thinkpad_ec_lock (); +if (ret) +return ret; + ret =__hdaps_update (0 ); +thinkpad_ec_unlock (); + +if (!ret) +return 0 ; +if (ret != -EBUSY ) +break ; +msleep (RETRY_MSECS ); + }return ret; }
@@ -303,31 +319,13 @@ static ssize_t hdaps_position_show(strucreturn sprintf (buf,"(%d,%d)\n" , pos_x, pos_y); } -static ssize_t hdaps_variance_show (struct device *dev, -struct device_attribute *attr,char *buf) -{ -int ret =hdaps_update (); -if (ret) -return ret; -return sprintf (buf,"(%d,%d)\n" , var_x, var_y); -} -static ssize_t hdaps_temp1_show (struct device *dev,struct device_attribute *attr,char *buf) {int ret =hdaps_update ();if (ret)return ret; -return sprintf (buf,"%u\n" , temp1); -} - -static ssize_t hdaps_temp2_show (struct device *dev, -struct device_attribute *attr,char *buf) -{ -int ret =hdaps_update (); -if (ret) -return ret; -return sprintf (buf,"%u\n" , temp2); +return sprintf (buf,"%d\n" , temperature); } static ssize_t hdaps_keyboard_activity_show(struct device *dev,
@@ -380,9 +378,8 @@ static ssize_t hdaps_invert_store(struct } static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL); -static DEVICE_ATTR(variance, 0444, hdaps_variance_show, NULL); static DEVICE_ATTR(temp1, 0444, hdaps_temp1_show, NULL); -static DEVICE_ATTR(temp2, 0444, hdaps_temp2_show, NULL); +/* "temp1" instead of "temperature" is hwmon convention */ static DEVICE_ATTR(keyboard_activity, 0444, hdaps_keyboard_activity_show, NULL); static DEVICE_ATTR(mouse_activity, 0444, hdaps_mouse_activity_show, NULL); static DEVICE_ATTR(calibrate, 0644, hdaps_calibrate_show,hdaps_calibrate_store);
@@ -390,9 +387,7 @@ static DEVICE_ATTR(invert, 0644, hdaps_istatic struct attribute *hdaps_attributes[] = { &dev_attr_position.attr, - &dev_attr_variance.attr, &dev_attr_temp1.attr, - &dev_attr_temp2.attr, &dev_attr_keyboard_activity.attr, &dev_attr_mouse_activity.attr, &dev_attr_calibrate.attr,
PATCH 5/12 hdaps: Remember keyboard and mouse activity
In reply to Shem MultinymousWhen 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.
--- drivers/hwmon/hdaps.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
drivers/hwmon/hdaps.c +23 -6
@@ -53,8 +53,6 @@ static const struct thinkpad_ec_row ec_a#define KEYBD_MASK 0x20/* set if keyboard activity */ #define MOUSE_MASK 0x40/* set if mouse activity */ -#define KEYBD_ISSET (n) (!! (n & KEYBD_MASK))/* keyboard used? */ -#define MOUSE_ISSET (n) (!! (n & MOUSE_MASK))/* mouse used? */ #define READ_TIMEOUT_MSECS 100/* wait this long for device read */ #define RETRY_MSECS 3/* retry delay */
@@ -62,6 +60,7 @@ static const struct thinkpad_ec_row ec_a#define HDAPS_POLL_PERIOD (HZ/20)/* poll for input every 1/20s */ #define HDAPS_INPUT_FUZZ 4/* input event threshold */ #define HDAPS_INPUT_FLAT 4 +#define KMACT_REMEMBER_PERIOD (HZ/10)/* keyboard/mouse persistance */ static struct timer_list hdaps_timer;static struct platform_device *pdev;
@@ -71,9 +70,12 @@ static unsigned int hdaps_invert;/* Latest state readout */ static int pos_x, pos_y;/* position */ static int temperature;/* temperature */ -static u8 km_activity;static int rest_x, rest_y;/* calibrated rest position */ +/* Last time we saw keyboard and mouse activity: */ +static u64 last_keyboard_jiffies =INITIAL_JIFFIES ; +static u64 last_mouse_jiffies =INITIAL_JIFFIES ; +/* Some models require an axis transformation to the standard reprsentation */ void transform_axes (int *x,int *y) {
@@ -122,7 +124,14 @@ static int __hdaps_update(int fast) pos_y = *(s16 *)(data.val+EC_ACCEL_IDX_YPOS1 );transform_axes (&pos_x, &pos_y); - km_activity = data.val[EC_ACCEL_IDX_KMACT ]; +/* Keyboard and mouse activity status is cleared as soon as it's read, +* so applications will eat each other's events. Thus we remember any +* event for KMACT_REMEMBER_PERIOD jiffies. +*/ +if (data.val[EC_ACCEL_IDX_KMACT ] &KEYBD_MASK ) + last_keyboard_jiffies =get_jiffies_64 (); +if (data.val[EC_ACCEL_IDX_KMACT ] &MOUSE_MASK ) + last_mouse_jiffies =get_jiffies_64 (); temperature = data.val[EC_ACCEL_IDX_TEMP1 ];
@@ -332,14 +341,22 @@ static ssize_t hdaps_keyboard_activity_sstruct device_attribute *attr, char *buf) { - return sprintf(buf, "%u\n", KEYBD_ISSET(km_activity)); + int ret = hdaps_update(); +if (ret) +return ret; +return sprintf (buf,"%u\n" , +get_jiffies_64 () < last_keyboard_jiffies +KMACT_REMEMBER_PERIOD ); }static ssize_t hdaps_mouse_activity_show (struct device *dev,struct device_attribute *attr,char *buf) { -return sprintf (buf,"%u\n" ,MOUSE_ISSET (km_activity)); +int ret =hdaps_update (); +if (ret) +return ret; +return sprintf (buf,"%u\n" , +get_jiffies_64 () < last_mouse_jiffies +KMACT_REMEMBER_PERIOD ); } static ssize_t hdaps_calibrate_show(struct device *dev,
PATCH 6/12 hdaps: Limit hardware query rate
In reply to Shem MultinymousThe 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.
--- drivers/hwmon/hdaps.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/hwmon/hdaps.c +13 -6
@@ -57,7 +57,7 @@ static const struct thinkpad_ec_row ec_a#define READ_TIMEOUT_MSECS 100/* wait this long for device read */ #define RETRY_MSECS 3/* retry delay */ -#define HDAPS_POLL_PERIOD (HZ/20)/* poll for input every 1/20s */ +#define HDAPS_POLL_PERIOD (HZ/50)/* poll for input every 1/50s */ #define HDAPS_INPUT_FUZZ 4/* input event threshold */ #define HDAPS_INPUT_FLAT 4#define KMACT_REMEMBER_PERIOD (HZ/10)/* keyboard/mouse persistance */
@@ -67,10 +67,11 @@ static struct platform_device *pdev;static struct input_dev *hdaps_idev;static unsigned int hdaps_invert; -/* Latest state readout */ -static int pos_x, pos_y;/* position */ -static int temperature;/* temperature */ -static int rest_x, rest_y;/* calibrated rest position */ +/* Latest state readout: */ +static int pos_x, pos_y;/* position */ +static int temperature;/* temperature */ +static int stale_readout =1 ;/* last read invalid */ +static int rest_x, rest_y;/* calibrated rest position */ /* Last time we saw keyboard and mouse activity: */ static u64 last_keyboard_jiffies =INITIAL_JIFFIES ;
@@ -135,6 +136,7 @@ static int __hdaps_update(int fast) temperature = data.val[EC_ACCEL_IDX_TEMP1 ]; + stale_readout =0 ;return 0 ; }
@@ -149,6 +151,8 @@ static int __hdaps_update(int fast)static int hdaps_update (void ) {int total, ret; +if (!stale_readout)/* already updated recently? */ +return 0 ;for (total=0 ; total<READ_TIMEOUT_MSECS ; total+=RETRY_MSECS ) { ret =thinkpad_ec_lock ();if (ret)
@@ -244,6 +248,7 @@ bad:thinkpad_ec_invalidate (); ret = -ENXIO ; good: + stale_readout =1 ;thinkpad_ec_unlock ();return ret; }
@@ -295,6 +300,8 @@ static void hdaps_mousedev_poll(unsigned {int ret; + stale_readout =1 ; +/* Cannot sleep. Try nonblockingly. If we fail, try again later. */ if (thinkpad_ec_try_lock ()) goto keep_active;
@@ -304,7 +311,7 @@ static void hdaps_mousedev_poll(unsigned/* Any of "successful", "not yet ready" and "not prefetched"? */ if (ret!=0 && ret!=-EBUSY && ret!=-ENODATA ) {printk (KERN_ERR -"hdaps: poll failed, disabling mousedev updates\n" ); +"hdaps: poll failed, disabling updates\n" );return ; }
Re: [PATCH 06/12] hdaps: Limit hardware query rate
Hi!
Quoted 2 lines
The polling rate is increased to 50Hz, as needed by the hdaps daemon. A later patch makes this configurable.
So you have a daemon that will actually protect the harddrive? Do you have an url? That would be great, particulary for poor users of misdesigned x41 (that seem to kill disk every 6 months without this kind of protection...)
Pavel
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 06/12] hdaps: Limit hardware query rate
Quoted 9 lines · On Thu, 10 Aug 2006 21:26:47 +0000 Pavel Machek <[email protected]> wrote:
Hi!
Quoted 2 lines
The polling rate is increased to 50Hz, as needed by the hdaps daemon. A later patch makes this configurable.
So you have a daemon that will actually protect the harddrive? Do you have an url? That would be great, particulary for poor users of misdesigned x41 (that seem to kill disk every 6 months without this kind of protection...)
There is a hdapsd. Look on
http://www.thinkwiki.org/wiki/How_to_protect_the_harddisk_through_APS
regards
Evgeni
Signature · 2 lines
^^^ | Evgeni -SargentD- Golov ([email protected]) d(O_o)b | PGP-Key-ID: 0xAC15B50C
Quoted 1 line
-|-< | WWW: http://www.die-welt.net ICQ: 54116744
/ \ | IRC: #sod @ irc.german-freakz.net
PATCH 7/12 hdaps: delay calibration to first hardware query
In reply to Shem MultinymousThe 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.
--- drivers/hwmon/hdaps.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/hwmon/hdaps.c +11 -4
@@ -66,6 +66,7 @@ static struct timer_list hdaps_timer;static struct platform_device *pdev;static struct input_dev *hdaps_idev;static unsigned int hdaps_invert; +static int needs_calibration;/* Latest state readout: */ static int pos_x, pos_y;/* position */
@@ -137,6 +138,12 @@ static int __hdaps_update(int fast) temperature = data.val[EC_ACCEL_IDX_TEMP1 ]; stale_readout =0 ; +if (needs_calibration) { + rest_x = pos_x; + rest_y = pos_y; + needs_calibration =0 ; + } +return 0 ; }
@@ -288,9 +295,9 @@ static struct platform_driver hdaps_driv*/ static void hdaps_calibrate (void ) { + needs_calibration =1 ;hdaps_update (); - rest_x = pos_x; - rest_y = pos_y; +/* If that fails, the mousedev poll will take care of things later. */ }/* Timer handler for updating the input device. Runs in softirq context,
@@ -520,8 +527,8 @@ static int __init hdaps_init(void) goto out_group; } -/* initial calibrate for the input device */ -hdaps_calibrate (); +/* calibration for the input device (deferred to avoid delay) */ + needs_calibration =1 ;/* initialize the input class */ hdaps_idev->name ="hdaps" ;
PATCH 8/12 hdaps: Add explicit hardware configuration functions
In reply to Shem MultinymousThis 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.
--- drivers/hwmon/hdaps.c | 205 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 159 insertions(+), 46 deletions(-)
drivers/hwmon/hdaps.c +159 -46
@@ -57,7 +57,6 @@ static const struct thinkpad_ec_row ec_a#define READ_TIMEOUT_MSECS 100/* wait this long for device read */ #define RETRY_MSECS 3/* retry delay */ -#define HDAPS_POLL_PERIOD (HZ/50)/* poll for input every 1/50s */ #define HDAPS_INPUT_FUZZ 4/* input event threshold */ #define HDAPS_INPUT_FLAT 4#define KMACT_REMEMBER_PERIOD (HZ/10)/* keyboard/mouse persistance */
@@ -68,6 +67,13 @@ static struct input_dev *hdaps_idev;static unsigned int hdaps_invert;static int needs_calibration; +/* Configuration: */ +static int sampling_rate =50 ;/* Sampling rate */ +static int oversampling_ratio =5 ;/* Ratio between our sampling rate and +* EC accelerometer sampling rate */ +static int running_avg_filter_order =2 ;/* EC running average filter order */ +static int fake_data_mode;/* Enable EC fake data mode? */ +/* Latest state readout: */ static int pos_x, pos_y;/* position */ static int temperature;/* temperature */
@@ -177,6 +183,143 @@ static int hdaps_update(void) }/** +* hdaps_set_power - enable or disable power to the accelerometer. +* Returns zero on success and negative error code on failure. Can sleep. +*/ +static int hdaps_set_power (int on) +{ +struct thinkpad_ec_row args = + { .mask=0x0003 , .val={0x14 , on?0x01 :0x00 } }; +struct thinkpad_ec_row data = { .mask =0x8000 }; +int ret =thinkpad_ec_read_row (&args, &data); +if (ret) +return ret; +if (data.val[0xF ]!=0x00 ) +return -EIO ; +return 0 ; +} + +/** +* hdaps_set_fake_data_mode - enable or disable EC test mode +* EC test mode fakes accelerometer data using an incrementing counter. +* Returns zero on success and negative error code on failure. Can sleep. +*/ +static int hdaps_set_fake_data_mode (int on) +{ +struct thinkpad_ec_row args = + { .mask=0x0007 , .val={0x17 ,0x83 , on?0x01 :0x00 } }; +struct thinkpad_ec_row data = { .mask =0x8000 }; +int ret =thinkpad_ec_read_row (&args, &data); +if (ret) +return ret; +if (data.val[0xF ]!=0x00 ) { +printk (KERN_WARNING "failed setting hdaps fake data to %d\n" , + on); +return -EIO ; + } +printk (KERN_DEBUG "hdaps: fake_data_mode set to %d\n" , on); +return 0 ; +} + +/** +* hdaps_set_ec_config - set accelerometer parameters. +* @ec_rate: embedded controller sampling rate +* @order: embedded controller running average filter order +* (Normally we have @ec_rate = sampling_rate * oversampling_ratio.) +* Returns zero on success and negative error code on failure. Can sleep. +*/ +static int hdaps_set_ec_config (int ec_rate,int order) +{ +struct thinkpad_ec_row args = { .mask=0x000F , + .val={0x10 , (u8 )ec_rate, (u8)(ec_rate>>8 ), order} }; +struct thinkpad_ec_row data = { .mask =0x8000 }; +int ret =thinkpad_ec_read_row (&args, &data); +printk (KERN_DEBUG "hdaps: setting ec_rate=%d, filter_order=%d\n" , + ec_rate, order); +if (ret) +return ret; +if (data.val[0xF ]==0x03 ) { +printk (KERN_WARNING "hdaps: config param out of range\n" ); +return -EINVAL ; + } +if (data.val[0xF ]==0x06 ) { +printk (KERN_WARNING "hdaps: config change already pending\n" ); +return -EBUSY ; + } +if (data.val[0xF ]!=0x00 ) { +printk (KERN_WARNING "hdaps: config change error, ret=%d\n" , + data.val[0xF ]); +return -EIO ; + } +return 0 ; +} + +/** +* hdaps_get_ec_config - get accelerometer parameters. +* @ec_rate: embedded controller sampling rate +* @order: embedded controller running average filter order +* Returns zero on success and negative error code on failure. Can sleep. +*/ +static int hdaps_get_ec_config (int *ec_rate,int *order) +{ +const struct thinkpad_ec_row args = + { .mask=0x0003 , .val={0x17 ,0x82 } }; +struct thinkpad_ec_row data = { .mask =0x801F }; +int ret =thinkpad_ec_read_row (&args, &data); +if (ret) +return ret; +if (data.val[0xF ]!=0x00 ) +return -EIO ; +if (!(data.val[0x1 ] &0x01 )) +return -ENXIO ;/* accelerometer polling not enabled */ +if (data.val[0x1 ] &0x02 ) +return -EBUSY ;/* config change in progress, retry later */ + *ec_rate = data.val[0x2 ] | ((int )(data.val[0x3 ]) <<8 ); + *order = data.val[0x4 ]; +return 0 ; +} + +/** +* hdaps_get_ec_mode - get EC accelerometer mode +* Returns zero on success and negative error code on failure. Can sleep. +*/ +static int hdaps_get_ec_mode (u8 *mode) +{ +const struct thinkpad_ec_row args = { .mask=0x0001 , .val={0x13 } }; +struct thinkpad_ec_row data = { .mask =0x8002 }; +int ret =thinkpad_ec_read_row (&args, &data); +if (ret) +return ret; +if (data.val[0xF ]!=0x00 ) { +printk (KERN_WARNING +"accelerometer not implemented (0x%02x)\n" , + data.val[0xF ]); +return -EIO ; + } + *mode = data.val[0x1 ]; +return 0 ; +} + +/** +* hdaps_check_ec - checks something about the EC. +* Follows the clean-room spec for HDAPS; we don't know what it means. +* Returns zero on success and negative error code on failure. Can sleep. +*/ +static int __init hdaps_check_ec (u8 *mode) +{ +const struct thinkpad_ec_row args = + { .mask=0x0003 , .val={0x17 ,0x81 } }; +struct thinkpad_ec_row data = { .mask =0x800E }; +int ret =thinkpad_ec_read_row (&args, &data); +if (ret) +return ret; +if (data.val[0x1 ]!=0x00 || data.val[0x2 ]!=0x60 || + data.val[0x3 ]!=0x00 || data.val[0xF ]!=0x00 ) +return -EIO ; +return 0 ; +} + +/** * hdaps_device_init - initialize the accelerometer. * * Call several embedded controller functions to test and initialize the
@@ -187,61 +330,31 @@ static int hdaps_update(void)static int hdaps_device_init (void ) {int ret; -struct thinkpad_ec_row args, data;u8 mode; ret =thinkpad_ec_lock ();if (ret)return ret; - args.val[0x0 ] =0x13 ; - args.val[0xF ] =0x01 ; - args.mask=0x8001 ; - data.mask=0x8002 ; -if (thinkpad_ec_read_row (&args, &data)) - {ABORT_INIT ("read1" ); goto bad; } -if (data.val[0xF ]!=0x00 ) - {ABORT_INIT ("check1" ); goto bad; } - mode = data.val[0x1 ]; +if (hdaps_get_ec_mode (&mode)) + {ABORT_INIT ("hdaps_get_ec_mode failed" ); goto bad; }printk (KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n" , mode);if (mode==0x00 ) {ABORT_INIT ("accelerometer not available" ); goto bad; } - args.val[0x0 ] =0x17 ; - args.val[0x1 ] =0x81 ; - args.val[0xF ] =0x01 ; - args.mask =0x8003 ; - data.mask =0x800E ; -if (thinkpad_ec_read_row (&args, &data)) - {ABORT_INIT ("read2" ); goto bad; } -if (data.val[0x1 ]!=0x00 || - data.val[0x2 ]!=0x60 || - data.val[0x3 ]!=0x00 || - data.val[0xF ]!=0x00 ) - {ABORT_INIT ("check2" ); goto bad; } - - args.val[0x0 ] =0x14 ; - args.val[0x1 ] =0x01 ; - args.val[0xF ] =0x01 ; - args.mask =0x8003 ; - data.mask =0x8000 ; -if (thinkpad_ec_read_row (&args, &data)) - {ABORT_INIT ("read3" ); goto bad; } -if (data.val[0xF ]!=0x00 ) - {ABORT_INIT ("check3" ); goto bad; } +if (hdaps_check_ec (&mode)) + {ABORT_INIT ("hdaps_check_ec failed" ); goto bad; } - args.val[0x0 ] =0x10 ; - args.val[0x1 ] =0xc8 ; - args.val[0x2 ] =0x00 ; - args.val[0x3 ] =0x02 ; - args.val[0xF ] =0x01 ; - args.mask =0x800F ; - data.mask =0x8000 ; -if (thinkpad_ec_read_row (&args, &data)) - {ABORT_INIT ("read4" ); goto bad; } -if (data.val[0xF ]!=0x00 ) - {ABORT_INIT ("check4" ); goto bad; } +if (hdaps_set_power (1 )) + {ABORT_INIT ("hdaps_set_power failed" ); goto bad; } + +if (hdaps_set_ec_config (sampling_rate*oversampling_ratio, + running_avg_filter_order)) + {ABORT_INIT ("hdaps_set_ec_config failed" ); goto bad; } + +if (hdaps_set_fake_data_mode (fake_data_mode)) + {ABORT_INIT ("hdaps_set_fake_data_mode failed" ); goto bad; }thinkpad_ec_invalidate ();udelay (200 );
@@ -327,7 +440,7 @@ keep_active:input_report_abs (hdaps_idev,ABS_X , pos_x - rest_x);input_report_abs (hdaps_idev,ABS_Y , pos_y - rest_y);input_sync (hdaps_idev); -mod_timer (&hdaps_timer, jiffies +HDAPS_POLL_PERIOD ); +mod_timer (&hdaps_timer, jiffies +HZ /sampling_rate); }
@@ -544,7 +657,7 @@ static int __init hdaps_init(void)/* start up our timer for the input device */ init_timer (&hdaps_timer); hdaps_timer.function = hdaps_mousedev_poll; - hdaps_timer.expires = jiffies +HDAPS_POLL_PERIOD ; + hdaps_timer.expires = jiffies +HZ /sampling_rate;add_timer (&hdaps_timer);printk (KERN_INFO "hdaps: driver successfully loaded.\n" );
PATCH 9/12 hdaps: Add new sysfs attributes
In reply to Shem MultinymousThis 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.
--- drivers/hwmon/hdaps.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+)
drivers/hwmon/hdaps.c +107 -0
@@ -521,6 +521,99 @@ static ssize_t hdaps_invert_store(structreturn count; } +static ssize_t hdaps_sampling_rate_show ( +struct device *dev,struct device_attribute *attr,char *buf) +{ +return sprintf (buf,"%d\n" , sampling_rate); +} + +static ssize_t hdaps_sampling_rate_store ( +struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +int rate, ret; +if (sscanf (buf,"%d" , &rate) !=1 || rate>HZ || rate<0 ) { +printk (KERN_WARNING +"must have 0<input_sampling_rate<=HZ=%d\n" ,HZ ); +return -EINVAL ; + } + ret =hdaps_set_ec_config (rate*oversampling_ratio, + running_avg_filter_order); +if (ret) +return ret; + sampling_rate = rate; +return count; +} + +static ssize_t hdaps_oversampling_ratio_show ( +struct device *dev,struct device_attribute *attr,char *buf) +{ +int ec_rate, order; +int ret =hdaps_get_ec_config (&ec_rate, &order); +if (ret) +return ret; +return sprintf (buf,"%u\n" , ec_rate / sampling_rate); +} + +static ssize_t hdaps_oversampling_ratio_store ( +struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +int ratio, ret; +if (sscanf (buf,"%d" , &ratio) !=1 || ratio<1 ) +return -EINVAL ; + ret =hdaps_set_ec_config (sampling_rate*ratio, + running_avg_filter_order); +if (ret) +return ret; + oversampling_ratio = ratio; +return count; +} + +static ssize_t hdaps_running_avg_filter_order_show ( +struct device *dev,struct device_attribute *attr,char *buf) +{ +int rate, order; +int ret =hdaps_get_ec_config (&rate, &order); +if (ret) +return ret; +return sprintf (buf,"%u\n" , order); +} + +static ssize_t hdaps_running_avg_filter_order_store ( +struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +int order, ret; +if (sscanf (buf,"%d" , &order) !=1 ) +return -EINVAL ; + ret =hdaps_set_ec_config (sampling_rate*oversampling_ratio, order); +if (ret) +return ret; + running_avg_filter_order = order; +return count; +} + +static ssize_t hdaps_fake_data_mode_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +int on, ret; +if (sscanf (buf,"%d" , &on) !=1 || on<0 || on>1 ) +return -EINVAL ; + ret =hdaps_set_fake_data_mode (on); +if (ret) +return ret; + fake_data_mode = on; +return count; +} + +static ssize_t hdaps_fake_data_mode_show ( +struct device *dev,struct device_attribute *attr,char *buf) +{ +return sprintf (buf,"%d\n" , fake_data_mode); +} + static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL); static DEVICE_ATTR(temp1, 0444, hdaps_temp1_show, NULL);/* "temp1" instead of "temperature" is hwmon convention */
@@ -528,6 +621,16 @@ static DEVICE_ATTR(keyboard_activity, 04 static DEVICE_ATTR(mouse_activity, 0444, hdaps_mouse_activity_show, NULL); static DEVICE_ATTR(calibrate, 0644, hdaps_calibrate_show,hdaps_calibrate_store); static DEVICE_ATTR(invert, 0644, hdaps_invert_show, hdaps_invert_store); +static DEVICE_ATTR(sampling_rate, 0644, + hdaps_sampling_rate_show, hdaps_sampling_rate_store); +static DEVICE_ATTR(oversampling_ratio, 0644, + hdaps_oversampling_ratio_show, + hdaps_oversampling_ratio_store); +static DEVICE_ATTR(running_avg_filter_order, 0644, + hdaps_running_avg_filter_order_show, + hdaps_running_avg_filter_order_store); +static DEVICE_ATTR(fake_data_mode, 0644, + hdaps_fake_data_mode_show, hdaps_fake_data_mode_store);static struct attribute *hdaps_attributes[] = { &dev_attr_position.attr,
@@ -536,6 +639,10 @@ static struct attribute *hdaps_attribute &dev_attr_mouse_activity.attr, &dev_attr_calibrate.attr, &dev_attr_invert.attr, + &dev_attr_sampling_rate.attr, + &dev_attr_oversampling_ratio.attr, + &dev_attr_running_avg_filter_order.attr, + &dev_attr_fake_data_mode.attr, NULL, };
PATCH 10/12 hdaps: Power off accelerometer on suspend and unload
In reply to Shem MultinymousThis 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.
--- drivers/hwmon/hdaps.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
drivers/hwmon/hdaps.c +26 -0
@@ -373,6 +373,23 @@ good:return ret; } +/** +* hdaps_device_shutdown - power off the accelerometer +* Returns nonzero on failure. Can sleep. +*/ +static int hdaps_device_shutdown (void ) +{ +int ret; + ret =hdaps_set_power (0 ); +if (ret) { +printk (KERN_WARNING "hdaps: cannot power off\n" ); +return ret; + } + ret =hdaps_set_ec_config (0 ,1 ); +if (ret) +printk (KERN_WARNING "hdaps: cannot stop EC sampling\n" ); +return ret; +}/* Device model stuff */
@@ -388,6 +405,12 @@ static int hdaps_probe(struct platform_dreturn 0 ; } +static int hdaps_suspend (struct platform_device *dev,pm_message_t state) +{ +hdaps_device_shutdown ();/* ignore errors, effect is negligible */ +return 0 ; +} +static int hdaps_resume (struct platform_device *dev) {return hdaps_device_init ();
@@ -395,6 +418,7 @@ static int hdaps_resume(struct platform_static struct platform_driver hdaps_driver = { .probe = hdaps_probe, + .suspend = hdaps_suspend, .resume = hdaps_resume, .driver = { .name ="hdaps" ,
@@ -776,6 +800,7 @@ out_device:platform_device_unregister (pdev); out_driver:platform_driver_unregister (&hdaps_driver); +hdaps_device_shutdown (); out:printk (KERN_WARNING "hdaps: driver init failed (ret=%d)!\n" , ret);return ret;
@@ -785,6 +810,7 @@ static void __exit hdaps_exit(void) {del_timer_sync (&hdaps_timer);input_unregister_device (hdaps_idev); +hdaps_device_shutdown ();/* ignore errors, effect is negligible */ sysfs_remove_group (&pdev->dev.kobj, &hdaps_attribute_group);platform_device_unregister (pdev);platform_driver_unregister (&hdaps_driver);
PATCH 11/12 hdaps: Stop polling timer when suspended
In reply to Shem MultinymousThis 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.
--- drivers/hwmon/hdaps.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/hwmon/hdaps.c +11 -6
@@ -407,13 +407,19 @@ static int hdaps_probe(struct platform_dstatic int hdaps_suspend (struct platform_device *dev,pm_message_t state) { +/* Don't do hdaps polls until resume re-initializes the sensor. */ +del_timer_sync (&hdaps_timer);hdaps_device_shutdown ();/* ignore errors, effect is negligible */ return 0 ; }static int hdaps_resume (struct platform_device *dev) { -return hdaps_device_init (); +int ret =hdaps_device_init (); +if (ret) +return ret; +mod_timer (&hdaps_timer, jiffies +HZ /sampling_rate); +return 0 ; }static struct platform_driver hdaps_driver = {
@@ -751,6 +757,9 @@ static int __init hdaps_init(void) goto out; } +/* Init timer before platform_driver_register, in case of suspend */ +init_timer (&hdaps_timer); + hdaps_timer.function = hdaps_mousedev_poll; ret =platform_driver_register (&hdaps_driver);if (ret) goto out;
@@ -785,11 +794,7 @@ static int __init hdaps_init(void)input_register_device (hdaps_idev); -/* start up our timer for the input device */ -init_timer (&hdaps_timer); - hdaps_timer.function = hdaps_mousedev_poll; - hdaps_timer.expires = jiffies +HZ /sampling_rate; -add_timer (&hdaps_timer); +mod_timer (&hdaps_timer, jiffies +HZ /sampling_rate);printk (KERN_INFO "hdaps: driver successfully loaded.\n" );return 0 ;
PATCH 12/12 hdaps: Simplify whitelist
In reply to Shem MultinymousThe hdaps driver can now reliably detect accelerometer hardware, as a free bonus from switch to thinkpad_ec. The whitelist is thus needed only for overriding the default axis configuratrion. This patch simplifies the whitelist to reflect this. Behavior on previously working models is completely unaffected, and new models will work automatically (though not necessarily with correct axis configuration).
--- drivers/hwmon/hdaps.c | 66 ++++++++++---------------------------------------- 1 file changed, 14 insertions(+), 52 deletions(-)
drivers/hwmon/hdaps.c +14 -52
@@ -683,79 +683,41 @@ static struct attribute_group hdaps_attr/* Module stuff */ -/* hdaps_dmi_match - found a match. return one, short-circuiting the hunt. */ -static int hdaps_dmi_match (struct dmi_system_id *id) -{ -printk (KERN_INFO "hdaps: %s detected.\n" , id->ident); -return 1 ; -} -/* hdaps_dmi_match_invert - found an inverted match. */ static int hdaps_dmi_match_invert (struct dmi_system_id *id) { hdaps_invert =1 ; -printk (KERN_INFO "hdaps: inverting axis readings.\n" ); -return hdaps_dmi_match (id); -} - -#define HDAPS_DMI_MATCH_NORMAL (model) { \ - .ident = "IBM " model, \ - .callback = hdaps_dmi_match, \ - .matches = { \ - DMI_MATCH(DMI_BOARD_VENDOR, "IBM"), \ - DMI_MATCH(DMI_PRODUCT_VERSION, model) \ - } \ +printk (KERN_INFO "hdaps: %s detected, inverting axes\n" , + id->ident); +return 1 ; } -#define HDAPS_DMI_MATCH_INVERT (model) { \ - .ident = "IBM " model, \ +#define HDAPS_DMI_MATCH_INVERT (vendor,model) { \ + .ident = vendor " " model, \ .callback = hdaps_dmi_match_invert, \ .matches = { \ - DMI_MATCH(DMI_BOARD_VENDOR, "IBM"), \ + DMI_MATCH(DMI_BOARD_VENDOR, vendor), \ DMI_MATCH(DMI_PRODUCT_VERSION, model) \ } \ } -#define HDAPS_DMI_MATCH_LENOVO (model) { \ - .ident = "Lenovo " model, \ - .callback = hdaps_dmi_match_invert, \ - .matches = { \ - DMI_MATCH(DMI_BOARD_VENDOR, "LENOVO"), \ - DMI_MATCH(DMI_PRODUCT_VERSION, model) \ - } \ -} -static int __init hdaps_init (void ) {int ret; -/* Note that HDAPS_DMI_MATCH_NORMAL("ThinkPad T42") would match +/* List of models with abnormal axis configuration. +Note that HDAPS_DMI_MATCH_NORMAL("ThinkPad T42") would match "ThinkPad T42p", so the order of the entries matters */ struct dmi_system_id hdaps_whitelist[] = { -HDAPS_DMI_MATCH_NORMAL ("ThinkPad H" ), -HDAPS_DMI_MATCH_INVERT ("ThinkPad R50p" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad R50" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad R51" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad R52" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad H" ),/* R52 (1846AQG) */ -HDAPS_DMI_MATCH_INVERT ("ThinkPad T41p" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad T41" ), -HDAPS_DMI_MATCH_INVERT ("ThinkPad T42p" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad T42" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad T43" ), -HDAPS_DMI_MATCH_LENOVO ("ThinkPad T60p" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad X40" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad X41" ), -HDAPS_DMI_MATCH_LENOVO ("ThinkPad X60" ), -HDAPS_DMI_MATCH_NORMAL ("ThinkPad Z60m" ), +HDAPS_DMI_MATCH_INVERT ("IBM" ,"ThinkPad R50p" ), +HDAPS_DMI_MATCH_INVERT ("IBM" ,"ThinkPad T41p" ), +HDAPS_DMI_MATCH_INVERT ("IBM" ,"ThinkPad T42p" ), +HDAPS_DMI_MATCH_INVERT ("LENOVO" ,"ThinkPad T60p" ), +HDAPS_DMI_MATCH_INVERT ("LENOVO" ,"ThinkPad X60" ), { .ident =NULL } }; -if (!dmi_check_system (hdaps_whitelist)) { -printk (KERN_WARNING "hdaps: supported laptop not found!\n" ); - ret = -ENODEV ; - goto out; - } +dmi_check_system (hdaps_whitelist);/* default to normal axes */ /* Init timer before platform_driver_register, in case of suspend */ init_timer (&hdaps_timer);
Quoted 2 lines · Shem Multinymous
This resubmission addresses all the comments I've received on LKML, except where said otherwise in reply. Other changes are very minor.
I reviewed this second patchset will sign-off once in this email, for all 12 patches in the patchset.
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
Robert Love
Robert Love
Acked-byQuoted 7 lines · Robert Love
I reviewed this second patchset will sign-off once in this email, for all 12 patches in the patchset.
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
Signed-off-by Robert Love <[email protected]>
This should be Ack-ed by as I was not involved in the code's development; I only performed a cursory review, in which the code looks good.
Robert Love
Quoted 2 lines · On Thu, 10 Aug 2006 09:46:47 -0400 "Robert Love" <[email protected]> wrote:
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
This situation is still a concern. From where did this additional register information come?
Was it reverse-engineered? If so, by whom and how can we satisfy ourselves of this?
Was it from published documents?
Was it improperly obtained from NDA'ed documentation?
Presumably the answer to the third question will be "no", but if challenged, how can we defend this assertion?
So hm. We're setting precedent here and we need Linus around to resolve this. Perhaps we can ask "Shem" to reveal his true identity to Linus (and maybe me) privately and then we proceed on that basis. The rule could be "each of the Signed-off-by:ers should know the identity of the others".
Quoted 16 lines · Andrew Morton
Quoted 2 lines · On Thu, 10 Aug 2006 09:46:47 -0400 "Robert Love" <[email protected]> wrote:
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
This situation is still a concern. From where did this additional register information come?
Was it reverse-engineered? If so, by whom and how can we satisfy ourselves of this?
Was it from published documents?
Was it improperly obtained from NDA'ed documentation?
Quoted 7 lines
Presumably the answer to the third question will be "no", but if challenged, how can we defend this assertion?
So hm. We're setting precedent here and we need Linus around to resolve this. Perhaps we can ask "Shem" to reveal his true identity to Linus (and maybe me) privately and then we proceed on that basis. The rule could be "each of the Signed-off-by:ers should know the identity of the others".
For what it's worth, I'm not going to be handling these patches at all (normally the hwmon patches go to Linus through Jean and then through me.)
If the original developer does not want to work in the open like the rest of us, I can respect that, but unfortunatly I can't accept the risk of accepting their code.
And no, this is not "been beaten over the head by IP lawyers for three years about risks like this" portion of me talking, although that side does have a lot he could say about this situation...
thanks,
greg k-h
All,
Quoted 20 lines · Andrew Morton
Quoted 19 lines
Quoted 2 lines · On Thu, 10 Aug 2006 09:46:47 -0400 "Robert Love" <[email protected]> wrote:
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
This situation is still a concern. From where did this additional register information come?
Was it reverse-engineered? If so, by whom and how can we satisfy ourselves of this?
Was it from published documents?
Was it improperly obtained from NDA'ed documentation?
Presumably the answer to the third question will be "no", but if challenged, how can we defend this assertion?
Quoted 8 lines
Quoted 4 lines
So hm. We're setting precedent here and we need Linus around to resolve this. Perhaps we can ask "Shem" to reveal his true identity to Linus (and maybe me) privately and then we proceed on that basis. The rule could be "each of the Signed-off-by:ers should know the identity of the others".
For what it's worth, I'm not going to be handling these patches at all (normally the hwmon patches go to Linus through Jean and then through me.)
I said it in private before, and I say it publicly again: I am not handling these patches either. I don't even want to read them.
Quoted 7 lines
If the original developer does not want to work in the open like the rest of us, I can respect that, but unfortunatly I can't accept the risk of accepting their code.
And no, this is not "been beaten over the head by IP lawyers for three years about risks like this" portion of me talking, although that side does have a lot he could say about this situation...
As far as I am concerned, even the real name of the person who wrote and sent these patches wouldn't be enough for me to take them. I would ask for an explanation of how that person got access to information about the HDAPS which even the original author of the driver didn't know about. And I would ask for proofs of that explanation.
All this is very unlikely to happen as I understand it, and anyway it's all too late. Regardless of its technical merits, this patchset has too much legal uncertainty attached to it by now. I'm maintaining the hwmon subsystem on my own time and money, it's painful and unrewarding enough as it is without adding legal hazard into the picture.
Signature · 1 line
Jean Delvare
Quoted 42 lines · Jean Delvare
All,
Quoted 20 lines · Andrew Morton
Quoted 19 lines
Quoted 2 lines · On Thu, 10 Aug 2006 09:46:47 -0400 "Robert Love" <[email protected]> wrote:
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
This situation is still a concern. From where did this additional register information come?
Was it reverse-engineered? If so, by whom and how can we satisfy ourselves of this?
Was it from published documents?
Was it improperly obtained from NDA'ed documentation?
Presumably the answer to the third question will be "no", but if challenged, how can we defend this assertion?
Quoted 8 lines
Quoted 4 lines
So hm. We're setting precedent here and we need Linus around to resolve this. Perhaps we can ask "Shem" to reveal his true identity to Linus (and maybe me) privately and then we proceed on that basis. The rule could be "each of the Signed-off-by:ers should know the identity of the others".
For what it's worth, I'm not going to be handling these patches at all (normally the hwmon patches go to Linus through Jean and then through me.)
I said it in private before, and I say it publicly again: I am not handling these patches either. I don't even want to read them.
Quoted 7 lines
If the original developer does not want to work in the open like the rest of us, I can respect that, but unfortunatly I can't accept the risk of accepting their code.
And no, this is not "been beaten over the head by IP lawyers for three years about risks like this" portion of me talking, although that side does have a lot he could say about this situation...
Quoted 6 lines
As far as I am concerned, even the real name of the person who wrote and sent these patches wouldn't be enough for me to take them. I would ask for an explanation of how that person got access to information about the HDAPS which even the original author of the driver didn't know about. And I would ask for proofs of that explanation.
As the person who wrote the initial version of the driver (not the only one, other people wrote versions at the same time, but mine was the one that was the basis for what Robert took over and what got merged in mainline) I'd also be interrested in where this person got his/her info from. I looked long and hard for docs and send lots of emails to IBM and others requesting info (unsuccessfully), all I could ever find was http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html
which is then what I used to write the initial driver from.
Quoted 6 lines
All this is very unlikely to happen as I understand it, and anyway it's all too late. Regardless of its technical merits, this patchset has too much legal uncertainty attached to it by now. I'm maintaining the hwmon subsystem on my own time and money, it's painful and unrewarding enough as it is without adding legal hazard into the picture.
Maybe if the person behind these patches could do the following
1) Come out with his/her real name.
2) Provide detailed information about what sources of info these patches are based on.
3) Provide names and contact info for people at IBM/Lenovo/the company who made the gyroscope (can't remember who they are) etc etc, so that we can contact them and verify these patches are based on stuff that's OK.
then we could accept the patches...
Signature · 3 lines
Jesper Juhl <[email protected]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html
Hi,
Quoted 1 line · Jesper Juhl
1) Come out with his/her real name.
This won't happen anytime soon (I hope the people who will inevitably reveal it will respect my wish to keep that private). But I'm working with Linus, Andrew and a few other developers to find a workable alternative, such as entrusting my full identity with them.
Quoted 2 lines
2) Provide detailed information about what sources of info these patches are based on.
I have done so.
Quoted 4 lines
3) Provide names and contact info for people at IBM/Lenovo/the company who made the gyroscope (can't remember who they are) etc etc, so that we can contact them and verify these patches are based on stuff that's OK.
This is ludicrous. I have no idea who these people are, nor any way to reach them, nor any reason to do so (other than to ask them nicely to release their specs properly next time and stop wasting everybody's time). Since when do we have to ask vendor's permission to run Linux on their hardware?!
BTW, the consensus on Google is that the accelerometer is a ADXL320 by Analog Devices, whose specs are at [1], but the driver doesn't care or know about this, since it's talking to the H8S embedded controller which *drives* the accelerometer.
Shem
Hi Jean,
Quoted 5 lines · Jean Delvare
I would
ask for an explanation of how that person got access to information about the HDAPS which even the original author of the driver didn't know about. And I would ask for proofs of that explanation. All this is very unlikely to happen as I understand it,
The mystery is rather simpler and less sinister than you imply, and I've already explained it before (e.g., in the first few lines of the code you say you don't want to read). But let me explain again in detail:
The original author of the APS spec [1] reversed-engineered the Windows driver, so he only saw what that driver was doing under nominal conditions. Jesper Juhl, the original author the mainline hdaps code, closely followed that spec (a prudent thing to do if you know nothing about the hardware), so was subject to the same limitation. However, once someone took apart his ThinkPad and uncovered the embedded controller chip [2], we got the underlying EC hardware specs [3]. Contrasting the LPC protocol in [3] with the APS spec [1] and mainline code showed they're indeed very similar, confirming the guess (and giving us the IO port offsets). This was the main breakthrough. The rest is detailed in my previous post.
Shem
[1]http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html [2]http://thinkwiki.org/wiki/Image:T43p-H8S2161.jpg [3]http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf
Hi!
Quoted 5 lines
Quoted 2 lines
Patches look great and I am glad someone has apparently better access to hardware specs than I did.
This situation is still a concern. From where did this additional register information come?
Well, I do not see much additional register information:
1/12: Seems to be based on
http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf
(and says so in the opening comment)
2/12: Seems to only rename already-known data.. plus adds some knowledge of embedded controller, probably from pdf above.
3/12: seems to refactor existing code.
4/12: queued events are embedded controller's feature, I guess, so this should be easy to guess.
5/12: pure linux side code.
6/12: pure linux side code.
7/12: pure linux side code.
8/12: this adds names for things we knew already. Again, this looks like easy enough to guess. "hdaps_check_ec - checks something about the EC" ... does not look like it came from leaked info.
(Is this what people have biggest problem with? 1-7 are still very nice cleanups...)
9/12: pure linux side code.
10/12: pure linux side code.
11/12: pure linux side code.
12/12: improved whitelist. Trivial after discovery that some bit tells you if accelerometer is there.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 7 lines · Andrew Morton
This situation is still a concern. From where did this additional register information come?
Was it reverse-engineered? If so, by whom and how can we satisfy ourselves of this?
Was it from published documents?
Here's a more detailed explanation:
All the low level LPC register access is publicly documented in the manual of the embedded controller. See [1] and in particular [2]. The submitted thinkpad_ec code just follows these specs (very defensively with and a lot of extra status checks, because some EC hangs were reported in older versions). The only remaining information is about the accelerometer-specific commands implemented by the firmware; the public APS spec [3] and the mainline code based on this already contain most of the this information, and a few corrections and extentions were gleaned from the reverse-engineered the firmware code [4]. If case you're wondering about the "opaque" function, hdaps_check_ec(), then note that it's just code from the original hdaps driver (following [3]) that's translated to use thinkpad_ec instead of direct IO port access.
Quoted 1 line
Was it improperly obtained from NDA'ed documentation?
Absolutely not. I've never signed any NDA remotely related to this. (and why I do so when the above sources already contain all the needed information?)
BTW, I can't help wondering: do you have a similarly detailed account for an appreciable fraction of the driver code in mainline?
Quoted 3 lines
So hm. We're setting precedent here and we need Linus around to resolve this. Perhaps we can ask "Shem" to reveal his true identity to Linus (and maybe me) privately and then we proceed on that basis.
Sure, we can do this. Actually I've alredy e-mailed Linus to this effect several days ago, before realizing he's off-line.
Quoted 1 line
"each of the Signed-off-by:ers should know the identity of the others".
How following the DCO's chain-of-trust model?
Documentation/SubmittingPatches +1 -1
@@ -296,7 +296,7 @@ can certify the below: (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified - it. + it, and the legal identity of that person is known to me. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all
Shem
[1]http://thinkwiki.org/wiki/Renesas_H8S/2161BV and in particular [2]http://documentation.renesas.com/eng/products/mpumcu/rej09b0300_2140bhm.pdf [3]http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html [4]http://forum.thinkpads.com/viewtopic.php?t=20958