ThinkPad embedded controller and hdaps drivers
cover ThinkPad embedded controller and hdaps drivers
This patch series does three related things:
- Introduces a new driver, thinkpad_ec, for the ThinkPad embedded controller (additional non-ACPI interface).
- Changes the existing hdaps driver to use thinkpad_ec instead of direct (and incorrect) port access.
- Several fixes and improvements to the hdaps driver.
Applies to git head.
Most of the patches in this series depend on previous ones. Any point in this series will compile and was briefly tested (this required introducing some code that gets overwritten by later patches). The *combination* of all these patches was extensively tested as part of the out-of-kernel tp_smapi package [1], and verified to work on dozens of ThinkPad models (thousands of sf.net downloads and no unresolved bugs). Note that the tp_smapi package includes another driver, to be submitted later, which also depends the thinkpad_ec driver introduced here.
For some context and the necessity for a separate thinkpad_ec module, see LKML thread "tp_smapi conflict with IDE, hdaps" on Dec. 2005.
I would like to thank the many testers and contributors who helped in the development, and in particular Henrique de Moraes Holschuh and ThinkWiki.org users Whoopie and Spiney.
[1] http://thinkwiki.org/wiki/tp_smapi
http://tpctl.sourceforge.net/rel/tp_smapi-0.27.tgz
Summary:
[PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access [PATCH 02/12] hdaps: Use thinkpad_ec instead of direct port access [PATCH 03/12] hdaps: Unify and cache hdaps readouts
[PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes [PATCH 05/12] hdaps: Remember keyboard and mouse activity
[PATCH 06/12] hdaps: Limit hardware query rate
[PATCH 07/12] hdaps: delay calibration to first hardware query [PATCH 08/12] hdaps: Add explicit hardware configuration functions [PATCH 09/12] hdaps: Add new sysfs attributes
[PATCH 10/12] hdaps: Power off accelerometer on suspend and unload [PATCH 11/12] hdaps: Stop polling timer when suspended
[PATCH 12/12] hdaps: Simplify whitelist
drivers/hwmon/hdaps.c | 758 ++++++++++++++++++++++++----------------- drivers/firmware/Kconfig | 3 drivers/firmware/Makefile | 1 drivers/firmware/thinkpad_ec.c | 444 ++++++++++++++++++++++++ drivers/hwmon/Kconfig | 1 include/linux/thinkpad_ec.h | 47 ++ 6 files changed, 940 insertions(+), 314 deletions(-)
PATCH 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 | 449 +++++++++++++++++++++++++++++++++++++++++ include/linux/thinkpad_ec.h | 47 ++++ 4 files changed, 500 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 +444 -0
444 lines not on this page · raw message
include/linux/thinkpad_ec.h +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.
--- a/drivers/hwmon/hdaps.c | 314 +++++++++++++++++------------------------------- drivers/hwmon/Kconfig | 1 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: %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(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_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 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.
--- 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 ; +/* __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_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 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", 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.
--- hdaps.c | 75 +++++++++++++++++++++++++++++----------------------------------- 1 file changed, 34 insertions(+), 41 deletions(-)
drivers/hwmon/hdaps.c +34 -41
@@ -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(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,
@@ -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_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 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.
--- hdaps.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/hwmon/hdaps.c +13 -5
@@ -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;
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.
--- hdaps.c | 152 +++++++++++++++++++++++++++++----------------------------------- 1 file changed, 71 insertions(+), 81 deletions(-)
drivers/hwmon/hdaps.c +71 -81
@@ -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(strstruct device_attribute *attr, const char *buf, size_t count) { - if (thinkpad_ec_lock()) - return -EIO;hdaps_calibrate (); -thinkpad_ec_unlock ();return count; }
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.
--- hdaps.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/hwmon/hdaps.c +10 -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 =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" ;
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.
--- hdaps.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 153 insertions(+), 47 deletions(-)
drivers/hwmon/hdaps.c +153 -47
@@ -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" );
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.
--- hdaps.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/hwmon/hdaps.c +12 -7
@@ -374,13 +374,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 ();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 ;
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 modules is completely unaffected, and new models will work automatically (though not necessarily with correct axis configuration).
--- hdaps.c | 66 +++++++++++++--------------------------------------------------- 1 file changed, 14 insertions(+), 52 deletions(-)
drivers/hwmon/hdaps.c +14 -52
@@ -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);
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.
--- hdaps.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+)
drivers/hwmon/hdaps.c +110 -0
@@ -495,6 +495,102 @@ 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 ) { +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, };
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.
--- hdaps.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/hwmon/hdaps.c +17 -0
@@ -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_dreturn 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);
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousQuoted 1 line · On Sun, 06 Aug 2006 10:26:46 +0300 Shem Multinymous <[email protected]>…
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?
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Hi,
Quoted 6 lines · Andrew Morton
Quoted 1 line · On Sun, 06 Aug 2006 10:26:46 +0300 Shem Multinymous <[email protected]>…
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Quoted 2 lines · On Sun, 6 Aug 2006 12:56:43 +0300 "Shem Multinymous" <[email protected]>…
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?
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Hi,
Quoted 2 lines · Andrew Morton
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:
Quoted 5 lines
Quoted 2 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Quoted 5 lines · Shem Multinymous
Quoted 2 lines · Andrew Morton
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.
Quoted 4 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Quoted 10 lines · Theodore Tso
Quoted 4 lines · Shem Multinymous
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?
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Quoted 8 lines · Olaf Hering
Quoted 7 lines · Theodore Tso
Quoted 4 lines · Shem Multinymous
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.
Quoted 8 lines
Quoted 3 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Olaf HeringQuoted 8 lines · On Sun, 6 Aug 2006 18:40:13 +0200 Olaf Hering <[email protected]> wrote:
Quoted 7 lines · Theodore Tso
Quoted 4 lines · Shem Multinymous
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.
Quoted 8 lines
Quoted 3 lines
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.
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousQuoted 11 lines · Shem Multinymous
Hi,
Quoted 2 lines · Andrew Morton
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.
Signature · 1 line
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Theodore TsoHi Ted,
Thanks for the explanation. Point taken, though I can't help parsing it as:
Quoted 3 lines · Theodore Tso
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.
Quoted 2 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Andrew MortonQuoted 3 lines · Andrew Morton
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Arjan van de VenHi Arjan,
Quoted 5 lines · Arjan van de Ven
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.
Quoted 4 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Quoted 9 lines · Shem Multinymous
Quoted 5 lines · Arjan van de Ven
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.
Quoted 8 lines
Quoted 4 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousQuoted 12 lines · Shem Multinymous
Hi Arjan,
Quoted 5 lines · Arjan van de Ven
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.
Quoted 8 lines
Quoted 4 lines
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.
Signature · 1 line
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Re: [Hdaps-devel] [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousQuoted 27 lines · Shem Multinymous
Hi Ted,
Thanks for the explanation. Point taken, though I can't help parsing it as:
Quoted 3 lines · Theodore Tso
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.
Quoted 2 lines
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 | uniqgave 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.
Quoted 3 lines
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.
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousQuoted 10 lines · Shem Multinymous
Hi Ted,
Thanks for the explanation. Point taken, though I can't help parsing it as:
Quoted 3 lines · Theodore Tso
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.
Quoted 11 lines
Quoted 2 lines
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 | uniqgave 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?
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Andrew MortonHi!
Quoted 8 lines
Quoted 2 lines
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
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Andrew MortonHi!
Quoted 5 lines
Quoted 1 line
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
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousHi!
Quoted 11 lines
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.
Quoted 4 lines
The
Kconfigentry is set to tristate and will be selected by hdaps and (eventually)tp_smapi, sincethinkpad_ecdoes nothing by itself.Signed-off-by Shem Multinymous <[email protected]>
drivers/firmware/thinkpad_ec.c line 69
Quoted 2 lines
drivers/firmware/thinkpad_ec.c +2
-0+/* Module parameters: */ +static int tp_debug =0 ;
Static variables do not need initializers.
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)
Is not there generic function doing this?
Quoted 4 lines
+/*
thinkpad_ec_lock:
+ * Get exclusive lock for accesing the controller. May sleep.
+ * Returns 0 iff lock acquired .
+ */
Linuxdoc?
drivers/firmware/thinkpad_ec.c line 125
Quoted 7 lines
drivers/firmware/thinkpad_ec.c +7
-0+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?
drivers/firmware/thinkpad_ec.c line 178
Quoted 5 lines
drivers/firmware/thinkpad_ec.c +5
-0+/* 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.
drivers/firmware/thinkpad_ec.c line 364
Quoted 8 lines
drivers/firmware/thinkpad_ec.c +8
-0+/*** 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.
drivers/firmware/thinkpad_ec.c line 378
Quoted 2 lines
drivers/firmware/thinkpad_ec.c +2
-0+/* 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.
drivers/firmware/thinkpad_ec.c line 379
Quoted 1 line
drivers/firmware/thinkpad_ec.c +1
-0+struct dmi_device *dev =NULL ;
unneeded initializer.
drivers/firmware/thinkpad_ec.c line 416
Quoted 5 lines
drivers/firmware/thinkpad_ec.c +5
-0+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?
drivers/firmware/thinkpad_ec.c line 421
Quoted 3 lines
drivers/firmware/thinkpad_ec.c +3
-0+if (!request_region (TPC_BASE_PORT ,TPC_NUM_PORTS , +"thinkpad_ec" )) + {
{ on same line, please.
Pavel
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 02/12] hdaps: Use thinkpad_ec instead of direct port access
In reply to Shem MultinymousHi!
Yes, this looks badly needed.
Quoted 5 lines
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]>
drivers/hwmon/hdaps.c static int hdaps_read_pair(unsigned int · line 134
Quoted 5 lines
drivers/hwmon/hdaps.c +1
-0@@ -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*.
drivers/hwmon/hdaps.c static int hdaps_read_pair(unsigned int · line 152
Quoted 4 lines
drivers/hwmon/hdaps.c +4
-0+if (thinkpad_ec_read_row (&args, &data)) +ABORT_INIT ("read1" ); +if (data.val[0xF ]!=0x00 ) +ABORT_INIT ("check1" );
!=0 in if is evil...
drivers/hwmon/hdaps.c static int hdaps_read_pair(unsigned int · line 156
Quoted 4 lines
drivers/hwmon/hdaps.c +4
-0+ mode = data.val[0x1 ]; + +printk (KERN_DEBUG "hdaps: initial mode latch is 0x%02x\n" , mode); +if (mode==0x00 )
if !mode ?
Pavel
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
In reply to Shem MultinymousHi!
Quoted 7 lines
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.
Quoted 6 lines
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]>
Quoted 5 lines
+/*
__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.
drivers/hwmon/hdaps.c static struct timer_list hdaps_timer; · line 104
Quoted 7 lines
drivers/hwmon/hdaps.c +7
-0+/* 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.
drivers/hwmon/hdaps.c static struct timer_list hdaps_timer; · line 122
Quoted 4 lines
{
drivers/hwmon/hdaps.c +2
-1-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
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Shem MultinymousHi!
Quoted 11 lines
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.
Quoted 4 lines
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]>
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 106
Quoted 1 line
drivers/hwmon/hdaps.c +1
-0+if (data.val[EC_ACCEL_IDX_READOUTS ]<1 )
Could we get spaces around '<'?
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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?
drivers/hwmon/hdaps.c static ssize_t hdaps_invert_store(struct · line 366
Quoted 8 lines
static
DEVICE_ATTR(position, 0444, hdaps_position_show, NULL);drivers/hwmon/hdaps.c +1
-2-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
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 05/12] hdaps: Remember keyboard and mouse activity
In reply to Shem MultinymousHi!
Quoted 10 lines
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?
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Pavel MachekHi Pavel,
Thanks for the sign-offs!
Quoted 8 lines · Pavel Machek
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
None that I found. Many drivers do it this way.
Quoted 9 lines
drivers/firmware/thinkpad_ec.c line 125
Quoted 7 lines
drivers/firmware/thinkpad_ec.c +7
-0+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. :-)
Quoted 4 lines
Quoted 1 line
+ 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.
Quoted 3 lines
Quoted 1 line
+ struct
dmi_device*dev = NULL;unneeded initializer.
On a local variable?!
Quoted 7 lines
drivers/firmware/thinkpad_ec.c line 416
Quoted 5 lines
drivers/firmware/thinkpad_ec.c +5
-0+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_ERRis 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
Re: [PATCH 02/12] hdaps: Use thinkpad_ec instead of direct port access
In reply to Pavel MachekQuoted 3 lines · Pavel Machek
Quoted 1 line
+#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.
Quoted 4 lines
drivers/hwmon/hdaps.c static int hdaps_read_pair(unsigned int · line 152
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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");Quoted 3 lines
Quoted 1 line
+ if (mode==0x00)
if !mode ?
Likewise.
Shem
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
In reply to Pavel MachekQuoted 9 lines · Pavel Machek
drivers/hwmon/hdaps.c static struct timer_list hdaps_timer; · line 104
Quoted 7 lines
drivers/hwmon/hdaps.c +7
-0+/* 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_invertshould 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.
Quoted 7 lines
drivers/hwmon/hdaps.c static struct timer_list hdaps_timer; · line 122
Quoted 4 lines
{
drivers/hwmon/hdaps.c +2
-1-int ret =thinkpad_ec_lock (); +int ret; + ret =thinkpad_ec_lock ();I actually liked the previous version more, and this change does not really belong here.
(That's a diff artifact, it's a totally different function...) Changed to the version you like.
Shem
Re: [PATCH 05/12] hdaps: Remember keyboard and mouse activity
In reply to Pavel MachekQuoted 7 lines · Pavel Machek
Quoted 7 lines
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.
Quoted 5 lines
Quoted 3 lines
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousQuoted 5 lines · Shem Multinymous
Quoted 3 lines
Quoted 1 line
+ 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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Pavel MachekHi Pavel,
Quoted 4 lines · Pavel Machek
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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...)
Quoted 1 line
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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Björn SteinbrinkQuoted 7 lines · Björn Steinbrink
Quoted 3 lines · Shem Multinymous
Quoted 2 lines
Quoted 1 line
+ 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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Quoted 9 lines · Shem Multinymous
Quoted 7 lines · Björn Steinbrink
Quoted 3 lines · Shem Multinymous
Quoted 2 lines
Quoted 1 line
+ 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.
Quoted 7 lines
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Shem MultinymousQuoted 10 lines · Shem Multinymous
Hi Pavel,
Quoted 4 lines · Pavel Machek
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Pavel MachekQuoted 5 lines · On Mon, 7 Aug 2006 13:26:29 +0000 Pavel Machek <[email protected]> wrote:
Hi!
Quoted 3 lines
Quoted 2 lines
What more is needed that may be realistically expected from a kernel patch submission?
Quoted 9 lines
Quoted 5 lines
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.
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Shem MultinymousHi!
Quoted 1 line
Thanks for the sign-offs!
No problem.
Quoted 10 lines
Quoted 8 lines
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
None that I found. Many drivers do it this way.
linux/kernel.h : pr_debug() looks similar.
Quoted 10 lines
Quoted 9 lines
drivers/firmware/thinkpad_ec.c line 125
Quoted 7 lines
drivers/firmware/thinkpad_ec.c +7
-0+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?
Quoted 4 lines
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...?
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+ struct
dmi_device*dev = NULL;unneeded initializer.
On a local variable?!
You were right, but see the other mail.
Quoted 5 lines
Quoted 5 lines
drivers/firmware/thinkpad_ec.c
Quoted 4 lines
drivers/firmware/thinkpad_ec.c +4
-0+static int __init thinkpad_ec_init(void) +{ + if (!check_dmi_for_ec()) { + printk(KERN_ERR "thinkpad_ec: no ThinkPad embeddedcontroller!\n");
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
+ return
-ENODEV;
KERN_ERRis 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_ERRsomeKERN_WARNINGin similar cases. Is there some guideline on choosing printk levels?
Well, this will also trigger for thinkpad module compiled into kernel, right?
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Björn SteinbrinkHi!
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
+ 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():
Quoted 7 lines
Quoted 5 lines
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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Andrew MortonHi!
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
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?
Quoted 7 lines
Quoted 3 lines
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.
Quoted 1 line
I'll run that by Linus when he resurfaces.
Thanks.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 02/12] hdaps: Use thinkpad_ec instead of direct port access
In reply to Shem MultinymousHi!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
+#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.
Quoted 9 lines
Quoted 4 lines
drivers/hwmon/hdaps.c static int hdaps_read_pair(unsigned int · line 152
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Pavel MachekQuoted 15 lines · Pavel Machek
Hi!
Quoted 1 line
Thanks for the sign-offs!
No problem.
Quoted 9 lines
Quoted 8 lines
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
Quoted 3 lines
Quoted 1 line
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
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
In reply to Shem MultinymousHi!
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
drivers/hwmon/hdaps.c +7
-0+/* 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 );
Quoted 4 lines
Quoted 1 line
Perhaps
hdaps_invertshould 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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Greg KHQuoted 16 lines · Greg KH
Quoted 15 lines · Pavel Machek
Hi!
Quoted 1 line
Thanks for the sign-offs!
No problem.
Quoted 9 lines
Quoted 8 lines
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Shem MultinymousQuoted 10 lines · Shem Multinymous
Hi Pavel,
Quoted 4 lines · Pavel Machek
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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 :-).
Quoted 4 lines
Quoted 1 line
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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Pavel MachekQuoted 17 lines · Pavel Machek
Quoted 16 lines · Greg KH
Quoted 15 lines · Pavel Machek
Hi!
Quoted 1 line
Thanks for the sign-offs!
No problem.
Quoted 9 lines
Quoted 8 lines
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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 havepr_debuginlinux/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
timeout nonsense [was Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes]
In reply to Björn SteinbrinkHi!
Quoted 13 lines
Quoted 8 lines · Pavel Machek
Quoted 4 lines
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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);
Quoted 9 lines
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?]
Quoted 8 lines
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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Pavel MachekQuoted 34 lines · On Mon, 7 Aug 2006 13:44:41 +0000 Pavel Machek wrote:
Hi!
Quoted 11 lines
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.Quoted 4 lines
The
Kconfigentry is set to tristate and will be selected by hdaps and (eventually)tp_smapi, sincethinkpad_ecdoes nothing by itself.Signed-off-by Shem Multinymous <[email protected]>Signed-off-by Pavel Machek <[email protected]>drivers/firmware/thinkpad_ec.c line 69
Quoted 2 lines
drivers/firmware/thinkpad_ec.c +2
-0+/* Module parameters: */ +static int tp_debug =0 ;Static variables do not need initializers.
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
Quoted 6 lines
Quoted 4 lines
+/*
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
PATCH 1/1 pr_debug() should not be used in drivers
In reply to Greg KHpr_debug() should not be used from drivers, add comment saying that.
include/linux/kernel.h +1 -0
@@ -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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
In reply to Pavel MachekQuoted 5 lines · Pavel Machek
Okay, so what about ..
#define CONVERT(x) *(s16*)(data.val+x) * (hdaps_invert?-1:1);
...or better inline function?
Actually, some models require fancier transformations. This was supposed to be reserved for a future patch, but might as well prepare the infrastructure:
/* Some models require an axis transformation to the standard reprsentation */ static void transform_axes(int inx, int iny, int *outx, int *outy) {
*outx = inx * (hdaps_invert?-1:1); *outy = iny * (hdaps_invert?-1:1); } ... /* Parse position data: */ transform_axes(*(s16*)(data.val+EC_ACCEL_IDX_XPOS1), *(s16*)(data.val+EC_ACCEL_IDX_YPOS1), &pos_x, &pos_y); /* Parse so-called "variance" data: */ transform_axes(*(s16*)(data.val+EC_ACCEL_IDX_XPOS2), *(s16*)(data.val+EC_ACCEL_IDX_YPOS2), &var_x, &var_y);
Shem
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
Hi!
Quoted 9 lines
Quoted 5 lines
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.
Quoted 10 lines
/* Some models require an axis transformation to the standard reprsentation */
static voidtransform_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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Pavel MachekQuoted 1 line · Pavel Machek
Okay... but do we really need
try_lockvariant?
We need a nonlocking, nonsleeping variant to do the query in the timer function (softirq context).
Quoted 1 line
but what is
try_locksemantics when taking multiple locks...?
Currently, the same as the undelying down_trylock().
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!check_dmi_for_ec()) { + printk(KERN_ERR "thinkpad_ec: no ThinkPad embeddedcontroller!\n");
Quoted 1 line
+ return
-ENODEV;
KERN_ERRis little strong here, no?
Quoted 7 lines
Quoted 4 lines
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_ERRsomeKERN_WARNINGin 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
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
Hi!
Quoted 8 lines
Quoted 1 line
Okay... but do we really need
try_lockvariant?We need a nonlocking, nonsleeping variant to do the query in the timer function (softirq context).
Quoted 1 line
but what is
try_locksemantics when taking multiple locks...?Currently, the same as the undelying
down_trylock().
Okay, I guess this works for me.
Quoted 6 lines
Quoted 2 lines
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 remainsKERN_ERR, since failing those after passing the DMI check really is abnormal (and indicative of danger).
Yep, that sounds correct.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 01/12] thinkpad_ec: New driver for ThinkPad embedded controller access
In reply to Greg KHQuoted 9 lines · Greg KH
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
drivers/firmware/thinkpad_ec.c line 75
Quoted 6 lines
drivers/firmware/thinkpad_ec.c +6
-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)Is not there generic function doing this?
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
In reply to Pavel MachekQuoted 6 lines · Pavel Machek
/* Parse position data: */ x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1); y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1); transform_axes(&x, &y);...which looks even better to me.
Yes, that's elegant.
But it made me realize there's a race condition here (and and also in the mainline driver): the global pos_x, rest_x etc. could be updated while an attribute's show_* function is called. Ugh. I guess I need to sprinkle spinlocks all over the place.
Shem
Re: [PATCH 03/12] hdaps: Unify and cache hdaps readouts
Hi!
Quoted 5 lines
Quoted 5 lines
/* Parse position data: */ x = *(s16*)(data.val+EC_ACCEL_IDX_XPOS1); y = *(s16*)(data.val+EC_ACCEL_IDX_YPOS1); transform_axes(&x, &y);
Quoted 7 lines
Quoted 1 line
...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 globalpos_x,rest_xetc. 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
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Björn SteinbrinkQuoted 12 lines · Björn Steinbrink
Quoted 10 lines · Shem Multinymous
Hi Pavel,
Quoted 4 lines · Pavel Machek
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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...)
Quoted 8 lines
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
Re: [PATCH 11/12] hdaps: Stop polling timer when suspended
In reply to Shem MultinymousHi!
Quoted 5 lines
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.
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 08/12] hdaps: Add explicit hardware configuration functions
In reply to Shem MultinymousHi!
Quoted 10 lines
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]>
drivers/hwmon/hdaps.c static struct input_dev *hdaps_idev; · line 68
Quoted 3 lines
drivers/hwmon/hdaps.c +0
-0@@ -68,6 +67,13 @@ static struct input_dev *hdaps_idev;static unsigned int hdaps_invert;static int needs_calibration =0 ;
Unneccessary initializer.
drivers/hwmon/hdaps.c static struct input_dev *hdaps_idev; · line 75
Quoted 6 lines
drivers/hwmon/hdaps.c +6
-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? */
Here too.
drivers/hwmon/hdaps.c static int hdaps_update(void) · line 174
Quoted 8 lines
drivers/hwmon/hdaps.c +4
-0@@ -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.
drivers/hwmon/hdaps.c static int hdaps_update(void) · line 190
Quoted 5 lines
drivers/hwmon/hdaps.c +5
-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. +*/
Why do we want to have fake mode? I see it is useful for debugging, but?
drivers/hwmon/hdaps.c static int hdaps_update(void) · line 287
Quoted 5 lines
drivers/hwmon/hdaps.c +5
-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. +*/
URL for spec?
What happens when we delete this one?
Pavel
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 12/12] hdaps: Simplify whitelist
In reply to Shem MultinymousHi!
Quoted 8 lines
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.
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 06/12] hdaps: Limit hardware query rate
In reply to Shem MultinymousHi!
Quoted 12 lines
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.
Quoted 4 lines
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.
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 09/12] hdaps: Add new sysfs attributes
In reply to Shem MultinymousHi!
Quoted 22 lines
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.
Quoted 1 line
Signed-off-by Shem Multinymous <[email protected]>
drivers/hwmon/hdaps.c static ssize_t hdaps_invert_store(struct · line 578
Quoted 8 lines
drivers/hwmon/hdaps.c +8
-0+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
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 07/12] hdaps: delay calibration to first hardware query
In reply to Shem MultinymousHi!
Quoted 8 lines
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.
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 10/12] hdaps: Power off accelerometer on suspend and unload
In reply to Shem MultinymousHi!
Quoted 5 lines
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]>
drivers/hwmon/hdaps.c good: · line 354
Quoted 4 lines
drivers/hwmon/hdaps.c +4
-0+/* +* hdaps_device_shutdown - power off the accelerometer. Can sleep. +*/ +static void hdaps_device_shutdown (void ) {
{ on newline?
drivers/hwmon/hdaps.c good: · line 359
Quoted 5 lines
drivers/hwmon/hdaps.c +5
-0+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?
Quoted 6 lines
drivers/hwmon/hdaps.c +6
-0+static int hdaps_suspend (struct platform_device *dev,pm_message_t state) +{ +hdaps_device_shutdown (); +return 0 ; +} +
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Muli Ben-YehudaQuoted 22 lines · Muli Ben-Yehuda
Quoted 12 lines · Björn Steinbrink
Quoted 10 lines · Shem Multinymous
Hi Pavel,
Quoted 4 lines · Pavel Machek
drivers/hwmon/hdaps.c static int __hdaps_update(int fast) · line 128
Quoted 2 lines
drivers/hwmon/hdaps.c +2
-0+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...)Quoted 8 lines
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;
Quoted 2 lines
Just in case someone was going to cut and paste, this will return with the
ec_locktaken.
Well, taking lock failed (hence error return) so I think code is correct.
Signature · 1 line
Thanks for all the (sleeping) penguins.
Re: [PATCH 08/12] hdaps: Add explicit hardware configuration functions
In reply to Pavel MachekQuoted 2 lines · Pavel Machek
Quoted 1 line
static int
needs_calibration= 0;Unneccessary initializer.
OK, though I would prefer to differentiate "initial value is 0" from "initial value doesn't matter".
Quoted 7 lines
Quoted 4 lines
+ *
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.
Quoted 7 lines
Quoted 5 lines
+/*
+ *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.
Quoted 1 line
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Pavel MachekQuoted 9 lines · Pavel Machek
Quoted 6 lines
Quoted 3 lines
ret = thinkpad_ec_lock(); if (ret) return ret;Just in case someone was going to cut and paste, this will return with the
ec_locktaken.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
Re: [PATCH 10/12] hdaps: Power off accelerometer on suspend and unload
In reply to Pavel MachekHi,
Quoted 7 lines · Pavel Machek
drivers/hwmon/hdaps.c good: · line 359
Quoted 5 lines
drivers/hwmon/hdaps.c +5
-0+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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
In reply to Muli Ben-YehudaHi Muli,
Quoted 6 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
ret = thinkpad_ec_lock(); if (ret) return ret;Ugh, I missed that - it's called
_lock(), but it's actuallydown_interruptible().
Why is that confusing?
Quoted 2 lines
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
Quoted 11 lines · Shem Multinymous
Hi Muli,
Quoted 6 lines · Muli Ben-Yehuda
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
ret = thinkpad_ec_lock(); if (ret) return ret;Ugh, I missed that - it's called
_lock(), but it's actuallydown_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.
Quoted 6 lines
Quoted 2 lines
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
Quoted 6 lines · Muli Ben-Yehuda
Quoted 3 lines
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
Quoted 1 line · Alan Cox
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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
Quoted 5 lines · Shem Multinymous
Quoted 1 line · Alan Cox
If the concern is just the naming then change it to end
_trylockWe already have a
thinkpad_ec_trylock()for the non-blocking variant.
thinkpad_ec_down_interruptible()?
Cheers,
Muli
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
Quoted 6 lines · Muli Ben-Yehuda
Quoted 4 lines
Quoted 1 line
If the concern is just the naming then change it to end
_trylockWe 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
Re: [PATCH 04/12] hdaps: Correct readout and remove nonsensical attributes
Quoted 4 lines · Shem Multinymous
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 :-)
Quoted 5 lines
It's not that strange that a function might fail even if it's named "
do_something()", you know. It'sdown()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