IBM HDAPS accelerometer driver, with probing.
PATCH 1/1 IBM HDAPS accelerometer driver, with probing.
Andrew,
Attached patch provides a driver for the IBM Hard Drive Active Protection System (hdaps) on top of 2.6.13-rc6-mm2.
Over the previous post, it contains several fixes and improvements, including a dev->probe() routine and a DMI whitelist.
Robert Love
Driver for the IBM HDAPS
MAINTAINERS | 7 drivers/hwmon/Kconfig | 17 + drivers/hwmon/Makefile | 1 drivers/hwmon/hdaps.c | 664 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 689 insertions(+)
drivers/hwmon/hdaps.c +664 -0
@@ -0,0 +1,664 @@ +/* +* drivers/hwmon/hdaps.c - driver for IBM's Hard Drive Active Protection System +* +* Copyright (C) 2005 Robert Love <[email protected]> +* Copyright (C) 2005 Jesper Juhl <[email protected]> +* +* The HardDisk Active Protection System (hdaps) is present in the IBM ThinkPad +* T41, T42, T43, and R51, at least. It provides a basic two-axis +* accelerometer and other misc. data. +* +* Based on the document by Mark A. Smith available at +* http://www.almaden.ibm.com/cs/people/marksmith/tpaps.html and a lot of trial +* and error. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. +*/ + +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/input.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/timer.h> +#include <linux/dmi.h> +#include <linux/spinlock.h> +#include <asm/io.h> + +#define HDAPS_LOW_PORT 0x1600/* first port used by hdaps */ +#define HDAPS_HIGH_PORT 0x162f/* last port used by hdaps */ + +#define STATE_FRESH 0x50/* accelerometer data is fresh */ + +#define REFRESH_ASYNC 0x00/* do asynchronous refresh */ +#define REFRESH_SYNC 0x01/* do synchronous refresh */ + +#define HDAPS_PORT_STATE 0x1611/* device state */ +#define HDAPS_PORT_XPOS 0x1612/* x-axis position */ +#define HDAPS_PORT_YPOS 0x1614/* y-axis position */ +#define HDAPS_PORT_TEMP 0x1616/* device temperature, in celcius */ +#define HDAPS_PORT_XVAR 0x1617/* x-axis variance (what is this?) */ +#define HDAPS_PORT_YVAR 0x1619/* y-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 HDAPS_READ_MASK 0xff/* some reads have the low 8 bits set */ + +#define KEYBD_MASK 0x20/* set if keyboard activity */ +#define MOUSE_MASK 0x40/* set if mouse activity */ + +#define KEYBD_ISSET (n) (!! (n & KEYBD_MASK)) +#define MOUSE_ISSET (n) (!! (n & MOUSE_MASK)) + +static spinlock_t hdaps_lock =SPIN_LOCK_UNLOCKED ; + + +/* +* __get_latch - Get the value from a given port latch. Callers must hold +* hdaps_lock. +*/ +static inline unsigned short __get_latch (unsigned short port) +{ +return inb (port) &HDAPS_READ_MASK ; +} + +/* +* __check_latch - Check a port latch for a given value. Callers must hold +* hdaps_lock. +*/ +static inline unsigned int __check_latch (unsigned short port,unsigned char val) +{ +if (__get_latch (port) == val) +return 1 ; +return 0 ; +} + +/* +* __wait_latch - Wait up to 100us for a port latch to get a certain value, +* returning nonzero if the value is obtained and zero otherwise. Callers +* must hold hdaps_lock. +*/ +static unsigned int __wait_latch (unsigned short port,unsigned char val) +{ +unsigned int i; + +for (i =0 ; i <20 ; i++) { +if (__check_latch (port, val)) +return 1 ; +udelay (5 ); + } + +return 0 ; +} + +/* +* __request_refresh - Request a refresh from the accelerometer. +* +* If sync is REFRESH_SYNC, we perform a synchronous refresh and will wait for +* the refresh. Returns nonzero if successful or zero on error. +* +* If sync is REFRESH_ASYNC, we merely kick off a new refresh if the device is +* not up-to-date. Always returns true. On the next read from the device, the +* data should be up-to-date but a synchronous wait should be performed to be +* sure. +* +* Callers must hold hdaps_lock. +*/ +static int __request_refresh (int sync) +{ +unsigned char state; + + state =inb (0x1604 ); +if (state ==STATE_FRESH ) +return 1 ; +else { +outb (0x11 ,0x1610 ); +outb (0x01 ,0x161f ); +if (sync ==REFRESH_ASYNC ) +return 1 ; + } + +return __wait_latch (0x1604 ,STATE_FRESH ); +} + +/* +* __tell_accelerometer_done - Indicate to the accelerometer that we are done +* reading data. Callers must hold hdaps_lock. +*/ +static inline void __tell_accelerometer_done (void ) +{ +inb (0x161f ); +inb (0x1604 ); +} + +/* internal lockless helper for accelerometer_readb_one */ +static int __accelerometer_readb_one (unsigned int port,u8 *val) +{ +int ret =0 ; + +/* do a sync refresh - we need to be sure we read fresh data */ +if (unlikely (!__request_refresh (REFRESH_SYNC ))) { + ret = -EIO ; + goto out; + } + + *val =inb (port); + +__tell_accelerometer_done (); + +if (unlikely (!__request_refresh (REFRESH_ASYNC ))) + ret = -EIO ; + +out: +return ret; +} + +/* +* accelerometer_readb_one - reads a byte from a single given I/O port, +* placing the value in the given pointer. Returns zero on success or a +* negative error on failure. +*/ +static int accelerometer_readb_one (unsigned int port,u8 *val) +{ +int ret =0 ; + +spin_lock (&hdaps_lock); + ret =__accelerometer_readb_one (port, val); +spin_unlock (&hdaps_lock); +return ret; +} + +/* +* accelerometer_read_pair - reads the values from a given pair of I/O ports, +* placing the values in the given pointers. Returns zero on success or a +* negative error on failure. +*/ +static int accelerometer_read_pair (unsigned int port1,unsigned int port2, +int *val1,int *val2) +{ +int ret =0 ; + +spin_lock (&hdaps_lock); + +/* do a sync refresh - we need to be sure we read fresh data */ +if (unlikely (!__request_refresh (REFRESH_SYNC ))) { + ret = -EIO ; + goto out; + } + + *val1 =inw (port1); + *val2 =inw (port2); + +__tell_accelerometer_done (); + +if (unlikely (!__request_refresh (REFRESH_ASYNC ))) + ret = -EIO ; + +out: +spin_unlock (&hdaps_lock); +return ret; +} + +#define INIT_TIMEOUT_MSECS 4000/* wait up to 4s for device init ... */ +#define INIT_WAIT_MSECS 200/* ... in 200ms increments */ + +/* initialize the accelerometer */ +static int accelerometer_init (void ) +{ +unsigned int total_msecs =INIT_TIMEOUT_MSECS ; +unsigned int msecs_per_wait =INIT_WAIT_MSECS ; +int ret = -EIO ; + +spin_lock (&hdaps_lock); + +outb (0x13 ,0x1610 ); +outb (0x01 ,0x161f ); +if (unlikely (!__wait_latch (0x161f ,0x00 ))) + goto out; + +/* +* The 0x3 value appears to only work on some thinkpads, such as the +* T42p. Others return 0x1. +* +* The 0x2 value occurs when the chip has been previously initialized. +*/ +if (unlikely (!__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 )); + +outb (0x17 ,0x1610 ); +outb (0x81 ,0x1611 ); +outb (0x01 ,0x161f ); +if (unlikely (!__wait_latch (0x161f ,0x00 ))) + goto out; +if (unlikely (!__wait_latch (0x1611 ,0x00 ))) + goto out; +if (unlikely (!__wait_latch (0x1612 ,0x60 ))) + goto out; +if (unlikely (!__wait_latch (0x1613 ,0x00 ))) + goto out; +outb (0x14 ,0x1610 ); +outb (0x01 ,0x1611 ); +outb (0x01 ,0x161f ); +if (unlikely (!__wait_latch (0x161f ,0x00 ))) + goto out; +outb (0x10 ,0x1610 ); +outb (0xc8 ,0x1611 ); +outb (0x00 ,0x1612 ); +outb (0x02 ,0x1613 ); +outb (0x01 ,0x161f ); +if (unlikely (!__wait_latch (0x161f ,0x00 ))) + goto out; +if (unlikely (!__request_refresh (REFRESH_SYNC ))) + goto out; +if (unlikely (!__wait_latch (0x1611 ,0x00 ))) + goto out; + +/* we have done our dance, now let's wait for the applause */ + ret = -ENXIO ; +while (total_msecs >0 ) { +u8 ignored; + +/* a read of the device helps push it into action */ +__accelerometer_readb_one (HDAPS_PORT_TEMP2 , &ignored); +if (__wait_latch (0x1611 ,0x02 )) { + ret =0 ; +break ; + } +spin_unlock (&hdaps_lock); + +msleep (msecs_per_wait); + total_msecs -= msecs_per_wait; + +spin_lock (&hdaps_lock); + } + +out: +spin_unlock (&hdaps_lock); +return ret; +} + + +/* device class stuff */ + + +static int hdaps_probe (struct device *dev) +{ +int ret; + + ret =accelerometer_init (); +if (unlikely (ret)) + goto out; + +printk (KERN_INFO "hdaps: device initialized.\n" ); +return 0 ; + +out: +printk (KERN_WARNING "hdaps: device probe failed (ret=%d)!\n" , ret); +return ret; +} + +static int hdaps_resume (struct device *dev,u32 level) +{ +if (level ==RESUME_ENABLE ) +return accelerometer_init (); +return 0 ; +} + +static struct device_driver hdaps_driver = { + .name ="hdaps" , + .bus = &platform_bus_type, + .owner =THIS_MODULE , + .probe = hdaps_probe, + .resume = hdaps_resume +}; + + +/* Input device stuff */ + +static struct platform_device *pdev; +static struct input_dev hdaps_idev; +static struct timer_list hdaps_poll_timer; +static unsigned int hdaps_mousedev_threshold =4 ; +static unsigned long hdaps_poll_ms =25 ; +static int hdaps_mousedev_enabled; +static u16 rest_x; +static u16 rest_y; + +static void hdaps_calibrate (void ) +{ +int x, y, ret; + + ret =accelerometer_read_pair (HDAPS_PORT_XPOS ,HDAPS_PORT_YPOS , &x, &y); +if (unlikely (ret)) +return ; + + rest_x = x; + rest_y = y; +} + +static void hdaps_mousedev_poll (unsigned long unused) +{ +int movex, movey, x, y, ret; + + ret =accelerometer_read_pair (HDAPS_PORT_XPOS ,HDAPS_PORT_YPOS , &x, &y); +if (unlikely (ret)) +return ; + + movex = rest_x - x; + movey = rest_y - y; +if (abs (movex) > hdaps_mousedev_threshold) +input_report_rel (&hdaps_idev,REL_Y , movex); +if (abs (movey) > hdaps_mousedev_threshold) +input_report_rel (&hdaps_idev,REL_X , movey); +input_sync (&hdaps_idev); + +mod_timer (&hdaps_poll_timer, jiffies +msecs_to_jiffies (hdaps_poll_ms)); +} + +/* +* hdaps_mousedev_enable - enable the input class device. Caller must hold +* hdaps_lock. +*/ +static void hdaps_mousedev_enable (void ) +{ +/* calibrate the device before enabling */ +hdaps_calibrate (); + +init_input_dev (&hdaps_idev); + hdaps_idev.dev = &pdev->dev; + hdaps_idev.evbit[0 ] =BIT (EV_KEY ) |BIT (EV_REL ); + hdaps_idev.relbit[0 ] =BIT (REL_X ) |BIT (REL_Y ); + hdaps_idev.keybit[LONG (BTN_LEFT )] =BIT (BTN_LEFT ); +input_register_device (&hdaps_idev); + + hdaps_mousedev_enabled =1 ; + +init_timer (&hdaps_poll_timer); + hdaps_poll_timer.function = hdaps_mousedev_poll; + hdaps_poll_timer.expires = jiffies +msecs_to_jiffies (hdaps_poll_ms); +add_timer (&hdaps_poll_timer); + +printk (KERN_INFO "hdaps: input device enabled.\n" ); +} + +/* +* hdaps_mousedev_disable - disable the input class device. Caller must hold +* hdaps_lock. +*/ +static void hdaps_mousedev_disable (void ) +{ +if (!hdaps_mousedev_enabled) +return ; + + hdaps_mousedev_enabled =0 ; + +del_timer_sync (&hdaps_poll_timer); +input_unregister_device (&hdaps_idev); +} + + +/* Sysfs Files */ + +static ssize_t hdaps_position_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int ret, x, y; + + ret =accelerometer_read_pair (HDAPS_PORT_XPOS ,HDAPS_PORT_YPOS , &x, &y); +if (unlikely (ret)) +return ret; + +return sprintf (buf,"(%d,%d)\n" , x, y); +} +static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL); + +static ssize_t hdaps_variance_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int ret, x, y; + + ret =accelerometer_read_pair (HDAPS_PORT_XVAR ,HDAPS_PORT_YVAR , &x, &y); +if (unlikely (ret)) +return ret; + +return sprintf (buf,"(%d,%d)\n" , x, y); +} +static DEVICE_ATTR(variance, 0444, hdaps_variance_show, NULL); + +static ssize_t hdaps_temp_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +u8 temp; +int ret; + + ret =accelerometer_readb_one (HDAPS_PORT_TEMP , &temp); +if (unlikely (ret <0 )) +return ret; + +return sprintf (buf,"%u\n" , temp); +} +static DEVICE_ATTR(temp, 0444, hdaps_temp_show, NULL); + +static ssize_t hdaps_mousedev_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +return sprintf (buf,"%d\n" , hdaps_mousedev_enabled); +} + +static ssize_t hdaps_mousedev_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +int enable; + +if (sscanf (buf,"%d" , &enable) !=1 ) +return -EINVAL ; + +spin_lock (&hdaps_lock); +if (enable ==1 ) +hdaps_mousedev_enable (); +else if (enable ==0 ) +hdaps_mousedev_disable (); +spin_unlock (&hdaps_lock); + +return count; +} + +static DEVICE_ATTR(mousedev, 0644, hdaps_mousedev_show, hdaps_mousedev_store); + +static ssize_t hdaps_calibrate_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +hdaps_calibrate (); +return count; +} +static DEVICE_ATTR(calibrate, 0200, NULL, hdaps_calibrate_store); + +static ssize_t hdaps_threshold_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +return sprintf (buf,"%u\n" , hdaps_mousedev_threshold); +} + +static ssize_t hdaps_threshold_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +unsigned int threshold; + +if (sscanf (buf,"%u" , &threshold) !=1 || threshold ==0 ) +return -EINVAL ; + hdaps_mousedev_threshold = threshold; + +return count; +} + +static DEVICE_ATTR(mousedev_threshold, 0644, hdaps_threshold_show, + hdaps_threshold_store); + +static ssize_t hdaps_poll_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +return sprintf (buf,"%lu\n" , hdaps_poll_ms); +} + +static ssize_t hdaps_poll_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +unsigned int poll; + +if (sscanf (buf,"%u" , &poll) !=1 || poll ==0 ) +return -EINVAL ; + hdaps_poll_ms = poll; + +return count; +} + +static DEVICE_ATTR(mousedev_poll_ms, 0644, hdaps_poll_show, hdaps_poll_store); + + +/* Module stuff */ + +static unsigned int mousedev; +module_param (mousedev, bool,0 ); +MODULE_PARM_DESC (mousedev,"enable the input class device" ); + +static int hdaps_dmi_match (struct dmi_system_id *id) +{ +printk (KERN_INFO "hdaps: %s detected.\n" , id->ident); +return 0 ; +} + +static int __init hdaps_init (void ) +{ +int ret; + +/* +* Currently, DMI_MATCH(...,"pirate") will match "pirate*" so we only +* check for e.g. "ThinkPad T42" and not also "ThinkPad T42p". +*/ +struct dmi_system_id hdaps_whitelist[] = { + { + .ident ="IBM ThinkPad T41" , + .callback = hdaps_dmi_match, + .matches = { +DMI_MATCH (DMI_BOARD_VENDOR ,"IBM" ), +DMI_MATCH (DMI_PRODUCT_VERSION ,"ThinkPad T41" ), + } + }, + { + .ident ="IBM ThinkPad T42" , + .callback = hdaps_dmi_match, + .matches = { +DMI_MATCH (DMI_BOARD_VENDOR ,"IBM" ), +DMI_MATCH (DMI_PRODUCT_VERSION ,"ThinkPad T42" ), + } + }, + { + .ident ="IBM ThinkPad T43" , + .callback = hdaps_dmi_match, + .matches = { +DMI_MATCH (DMI_BOARD_VENDOR ,"IBM" ), +DMI_MATCH (DMI_PRODUCT_VERSION ,"ThinkPad T43" ), + } + }, + { .matches = {DMI_MATCH (DMI_NONE ,NULL ) } } + }; + +if (!dmi_check_system (hdaps_whitelist)) { +printk (KERN_WARNING "hdaps: supported laptop not found!\n" ); + ret = -ENODEV ; + goto out; + } + + ret =driver_register (&hdaps_driver); +if (unlikely (ret)) + goto out; + +#if defined(CONFIG_PNPACPI ) || defined(CONFIG_PNPBIOS ) +/* +* If CONFIG_PNPACPI or CONFIG_PNPBIOS is enabled, the ACPI layer is +* going to claim all motherboard resources. And there is no way for +* hdaps to repossess them--so we don't. +*/ + pdev =platform_device_register_simple ("hdaps" ,-1 ,NULL ,0 ); +#else + { +struct resource res = { .name ="hdaps" , + .start =HDAPS_LOW_PORT , + .end =HDAPS_HIGH_PORT , + .flags =IORESOURCE_IO , + .parent = &ioport_resource }; + + pdev =platform_device_register_simple ("hdaps" ,-1 , &res,1 ); + } +#endif +if (unlikely (IS_ERR (pdev))) { + ret =PTR_ERR (pdev); + goto out_driver; + } + +device_create_file (&pdev->dev, &dev_attr_position); +device_create_file (&pdev->dev, &dev_attr_variance); +device_create_file (&pdev->dev, &dev_attr_temp); +device_create_file (&pdev->dev, &dev_attr_calibrate); +device_create_file (&pdev->dev, &dev_attr_mousedev); +device_create_file (&pdev->dev, &dev_attr_mousedev_threshold); +device_create_file (&pdev->dev, &dev_attr_mousedev_poll_ms); + +if (mousedev) +hdaps_mousedev_enable (); + +printk (KERN_INFO "hdaps: driver initialized.\n" ); +return 0 ; + +out_driver: +driver_unregister (&hdaps_driver); +out: +printk (KERN_WARNING "hdaps: driver init failed (ret=%d)!\n" , ret); +return ret; +} + +static void __exit hdaps_exit(void) +{ +hdaps_mousedev_disable (); + +device_remove_file (&pdev->dev, &dev_attr_mousedev_poll_ms); +device_remove_file (&pdev->dev, &dev_attr_mousedev_threshold); +device_remove_file (&pdev->dev, &dev_attr_mousedev); +device_remove_file (&pdev->dev, &dev_attr_temp); +device_remove_file (&pdev->dev, &dev_attr_calibrate); +device_remove_file (&pdev->dev, &dev_attr_variance); +device_remove_file (&pdev->dev, &dev_attr_position); +platform_device_unregister (pdev); +driver_unregister (&hdaps_driver); + +printk (KERN_INFO "hdaps: driver unloaded successfully.\n" ); +} + +module_init (hdaps_init); +module_exit (hdaps_exit); + +MODULE_AUTHOR ("Robert Love" ); +MODULE_DESCRIPTION ("IBM Hard Drive Active Protection System (HDAPS) driver" ); +MODULE_LICENSE ("GPL" );
drivers/hwmon/Kconfig +17 -0
@@ -411,6 +411,23 @@ This driver can also be built as a module. If so, the module will be called w83627ehf. +config SENSORS_HDAPS + tristate "IBM Hard Drive Active Protection System (hdaps)" + depends on HWMON && INPUT && X86 + default n + help + This driver provides support for the IBM Hard Drive Active Protection + System (hdaps), which provides an accelerometer and other misc. data. + Supported laptops include the IBM ThinkPad T41, T42, T43, and R51. + The accelerometer data is readable via sysfs. + + This driver also provides an input class device, allowing the + laptop to act as a pinball machine-esque mouse. This is off by + default but enabled via sysfs or the module parameter "mousedev". + + Say Y here if you have an applicable laptop and want to experience + the awesome power of hdaps. + config HWMON_DEBUG_CHIP bool "Hardware Monitoring Chip debugging messages" depends on HWMON
drivers/hwmon/Makefile +1 -0
@@ -22,6 +22,7 @@ obj-$(CONFIG_SENSORS_FSCPOS) += fscpos.o obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o +obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o obj-$(CONFIG_SENSORS_IT87) += it87.o obj-$(CONFIG_SENSORS_LM63) += lm63.o obj-$(CONFIG_SENSORS_LM75) += lm75.o
MAINTAINERS +7 -0
@@ -984,6 +984,13 @@ W: http://www.nyx.net/~arobinso S: Maintained +HDAPS +P: Robert Love +M: [email protected] +M: [email protected] +W: http://www.kernel.org/pub/linux/kernel/people/rml/hdaps/ +S: Maintained + HFS FILESYSTEM P: Roman Zippel M: [email protected]
drivers/hwmon/hdaps.c line 355
Quoted 11 lines · Robert Love
drivers/hwmon/hdaps.c +11
-0+static void hdaps_calibrate (void ) +{ +int x, y, ret; + + ret =accelerometer_read_pair (HDAPS_PORT_XPOS ,HDAPS_PORT_YPOS , &x, &y); +if (unlikely (ret)) +return ; + + rest_x = x; + rest_y = y; +}
Is this function used in a hot path to warrant using "unlikely"? There are to many "unlikely" in the code for my taste.
Quoted 9 lines
drivers/hwmon/hdaps.c +9
-0+ +static ssize_t hdaps_mousedev_store (struct device *dev, +struct device_attribute *attr, +const char *buf,size_t count) +{ +int enable; + +if (sscanf (buf,"%d" , &enable) !=1 ) +return -EINVAL ;
drivers/hwmon/hdaps.c line 483
Quoted 10 lines
drivers/hwmon/hdaps.c +10
-0+ +spin_lock (&hdaps_lock); +if (enable ==1 ) +hdaps_mousedev_enable (); +else if (enable ==0 ) +hdaps_mousedev_disable (); +spin_unlock (&hdaps_lock); + +return count; +}
input_[un]register_device and del_timer_sync are "long" operations. I think a semaphore would be better here.
Signature · 1 line
Dmitry
Quoted 2 lines · Dmitry Torokhov
Is this function used in a hot path to warrant using "unlikely"? There are to many "unlikely" in the code for my taste.
unlikely() can result in better, smaller, faster code. and it acts as a nice directive to programmers reading the code.
Quoted 2 lines
input_[un]register_device and
del_timer_syncare "long" operations. I think a semaphore would be better here.
I was considering moving all locking to a single semaphore, actually.
Robert Love
Em Fri, Aug 26, 2005 at 07:37:59PM -0400, Robert Love escreveu:
Quoted 7 lines · Dmitry Torokhov
Quoted 3 lines
Is this function used in a hot path to warrant using "unlikely"? There are to many "unlikely" in the code for my taste.
unlikely()can result in better, smaller, faster code. and it acts as a nice directive to programmers reading the code.
Agreed, keep them :-)
- Arnaldo
Quoted 4 lines · Arnaldo Carvalho de Melo
Quoted 2 lines
unlikely()can result in better, smaller, faster code. and it acts as a nice directive to programmers reading the code.Agreed, keep them :-)
If the unlikely() hints are being used correctly and the compiler is doing the right thing then it ought to be worthwhile, if not then fix the compiler.
Remember however unlikely() does have a code size cost on some processors and a small performance cost so it really has to mean very_unlikely().
drivers/hwmon/hdaps.c line 307
Quoted 12 lines · Robert Love
Attached patch provides a driver for the IBM Hard Drive Active Protection System (hdaps) on top of 2.6.13-rc6-mm2.
drivers/hwmon/hdaps.c +6
-0+static int hdaps_probe (struct device *dev) +{ +int ret; + + ret =accelerometer_init (); +if (unlikely (ret))
What's the point of having unlikely() attached to every possible if ()?
Quoted 8 lines
drivers/hwmon/hdaps.c +8
-0+static ssize_t hdaps_temp_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +u8 temp; +int ret; + + ret =accelerometer_readb_one (HDAPS_PORT_TEMP , &temp); +if (unlikely (ret <0 ))
From: Alexey Dobriyan <[email protected]>
Date: Sat, 27 Aug 2005 02:58:48 +0400
Quoted 1 line
What's the point of having
unlikely()attached to every possible if ()?
If can result in smaller code, for one thing, even if it isn't a performance critical path.
Quoted 7 lines · David S. Miller
From: Alexey Dobriyan <[email protected]>
Date: Sat, 27 Aug 2005 02:58:48 +0400Quoted 1 line
What's the point of having
unlikely()attached to every possible if ()?If can result in smaller code, for one thing, even if it isn't a performance critical path.
Really? At least on x86 it tends to generate bigger code when block reordering is enabled because a jump forward and a jump backward and a possible label alignment are bigger than just a single jump forward. But then it doesn't make that much difference because the compiler does it on its own for every block.
On x86-64 I keep it disabled because:
- it generates bigger code
- it makes the assembly code unreadable
- it doesn't seem to help that much on modern CPUs with good branch prediction and big icaches anyways.
-Andi (who originally introduced likely/unlikely, but regrets it these days because it's far overused and makes code uglier everywhere)
From: Andi Kleen <[email protected]>
Date: 27 Aug 2005 04:34:07 +0200
Quoted 7 lines
Quoted 5 lines · David S. Miller
From: Alexey Dobriyan <[email protected]>
Date: Sat, 27 Aug 2005 02:58:48 +0400Quoted 1 line
What's the point of having
unlikely()attached to every possible if ()?
Quoted 7 lines
Quoted 2 lines
If can result in smaller code, for one thing, even if it isn't a performance critical path.
Really? At least on x86 it tends to generate bigger code when block reordering is enabled because a jump forward and a jump backward and a possible label alignment are bigger than just a single jump forward.
In the cases I've studied on sparc64 it keeps gcc from doing basic block replication in the unlikely paths.
I've only checked gcc-3.4 and earlier, gcc-4.x is just big bloated useless garbage and should be avoided for a couple of years.
Quoted 2 lines · Andi Kleen
- it doesn't seem to help that much on modern CPUs with good branch prediction and big icaches anyways.
Really? I would think that as pipelines get deeper (although that trend seems to have stopped, thankfully) and Icache-miss penalties get relatively larger we'd see unlikely() becoming MORE of a benefit, not less. Storing the used part of a "hot" function in 1 Icacheline instead of 4 seems like an obvious win.
Personally I've never found unlikely() to be ugly; if anything I think it serves as a nice little human-readable comment about whats going on in the control-flow. I guess I'm in the minority on that one, though.
-Mitch
Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu:
Quoted 10 lines
Quoted 2 lines · Andi Kleen
- it doesn't seem to help that much on modern CPUs with good branch prediction and big icaches anyways.
Really? I would think that as pipelines get deeper (although that trend seems to have stopped, thankfully) and Icache-miss penalties get relatively larger we'd see
unlikely()becoming MORE of a benefit, not less. Storing the used part of a "hot" function in 1 Icacheline instead of 4 seems like an obvious win.
Quoted 3 lines
Personally I've never found
unlikely()to be ugly; if anything I think it serves as a nice little human-readable comment about whats going on in the control-flow. I guess I'm in the minority on that one, though.
Hey, even if unlikely was:
#define unlikely(x) (x)
I'd find it useful :-)
- Arnaldo
Quoted 15 lines · Arnaldo Carvalho de Melo
Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu:
Quoted 10 lines
Quoted 2 lines · Andi Kleen
- it doesn't seem to help that much on modern CPUs with good branch prediction and big icaches anyways.
Really? I would think that as pipelines get deeper (although that trend seems to have stopped, thankfully) and Icache-miss penalties get relatively larger we'd see
unlikely()becoming MORE of a benefit, not less. Storing the used part of a "hot" function in 1 Icacheline instead of 4 seems like an obvious win.Quoted 3 lines
Personally I've never found
unlikely()to be ugly; if anything I think it serves as a nice little human-readable comment about whats going on in the control-flow. I guess I'm in the minority on that one, though.
Quoted 6 lines
Hey, even if unlikely was:
#define unlikely(x) (x)
I'd find it useful :-)
Aside from annotating performance-critical sections what other purpose would it carry? It's not like you should not pay attention to teh code in these branches even if the are unlikely to be taken. So if code is not in hot path likely/unlikely just litter the code.
Btw, does it actually generate smaller code for constructs like
if (unlikely(blah)) goto out;
Signature · 1 line
Dmitry
Quoted 28 lines · Dmitry Torokhov
Quoted 15 lines · Arnaldo Carvalho de Melo
Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu:
Quoted 10 lines
Quoted 2 lines · Andi Kleen
- it doesn't seem to help that much on modern CPUs with good branch prediction and big icaches anyways.
Really? I would think that as pipelines get deeper (although that trend seems to have stopped, thankfully) and Icache-miss penalties get relatively larger we'd see
unlikely()becoming MORE of a benefit, not less. Storing the used part of a "hot" function in 1 Icacheline instead of 4 seems like an obvious win.Quoted 3 lines
Personally I've never found
unlikely()to be ugly; if anything I think it serves as a nice little human-readable comment about whats going on in the control-flow. I guess I'm in the minority on that one, though.Quoted 6 lines
Hey, even if unlikely was:
#define unlikely(x) (x)
I'd find it useful :-)
Aside from annotating performance-critical sections what other purpose would it carry? It's not like you should not pay attention to teh code in these branches even if the are unlikely to be taken. So if code is not in hot path likely/unlikely just litter the code.
Quoted 4 lines
Btw, does it actually generate smaller code for constructs like
if (unlikely(blah)) goto out;
Well, with my usual .config (-O2) and gcc-3.3.5-something it does:
text data bss dec hex filename
3614 303 1696 5613 15ed drivers/hwmon/hdaps.o 3678 303 1696 5677 162d drivers/hwmon/hdaps.o (unlikely()s removed)
Fortunately, there is -Os:
text data bss dec hex filename
3163 303 1696 5162 142a drivers/hwmon/hdaps.o 3163 303 1696 5162 142a drivers/hwmon/hdaps.o (unlikely()s removed)
See? The difference is 64 vs 451 bytes.
Quoted 1 line · Alexey Dobriyan
See? The difference is 64 vs 451 bytes.
Disabling unlikely() doesn't make much difference because the compiler generates the probability internally then and reorders anyways (that is why many unlikelys are completely useless
because the default heuristics for them are quite good)
To see a difference you need to compile with -fno-reorder-blocks
-Andi