Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Saturday 27 August 2005 13:35, Alexey Dobriyan wrote: > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Sad, 2005-08-27 at 02:29 -0300, Arnaldo Carvalho de Melo wrote: > > 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(). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Sat, Aug 27, 2005 at 01:12:50AM -0500, Dmitry Torokhov wrote: > On Saturday 27 August 2005 00:34, Arnaldo Carvalho de Melo wrote: > > Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu: > > > Andi Kleen wrote: > > > > - 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. > > > > 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; Well, with my usual .config (-O2) and gcc-3.3.5-something it does: textdata bss dec hex filename 3614 3031696561315ed drivers/hwmon/hdaps.o 3678 30316965677162d drivers/hwmon/hdaps.o (unlikely()s removed) Fortunately, there is -Os: textdata bss dec hex filename 3163 30316965162142a drivers/hwmon/hdaps.o 3163 30316965162142a drivers/hwmon/hdaps.o (unlikely()s removed) See? The difference is 64 vs 451 bytes. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Saturday 27 August 2005 00:34, Arnaldo Carvalho de Melo wrote: > Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu: > > Andi Kleen wrote: > > > - 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. > > 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; -- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Saturday 27 August 2005 00:34, Arnaldo Carvalho de Melo wrote: Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu: Andi Kleen wrote: - 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. 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; -- Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Sat, Aug 27, 2005 at 01:12:50AM -0500, Dmitry Torokhov wrote: On Saturday 27 August 2005 00:34, Arnaldo Carvalho de Melo wrote: Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu: Andi Kleen wrote: - 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. 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; Well, with my usual .config (-O2) and gcc-3.3.5-something it does: textdata bss dec hex filename 3614 3031696561315ed drivers/hwmon/hdaps.o 3678 30316965677162d drivers/hwmon/hdaps.o (unlikely()s removed) Fortunately, there is -Os: textdata bss dec hex filename 3163 30316965162142a drivers/hwmon/hdaps.o 3163 30316965162142a drivers/hwmon/hdaps.o (unlikely()s removed) See? The difference is 64 vs 451 bytes. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Sad, 2005-08-27 at 02:29 -0300, Arnaldo Carvalho de Melo wrote: 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(). - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Saturday 27 August 2005 13:35, Alexey Dobriyan wrote: 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu: > Andi Kleen wrote: > > - 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. Hey, even if unlikely was: #define unlikely(x) (x) I'd find it useful :-) - Arnaldo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
Em Fri, Aug 26, 2005 at 07:37:59PM -0400, Robert Love escreveu: > On Fri, 2005-08-26 at 17:44 -0500, Dmitry Torokhov wrote: > > > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
Andi Kleen wrote: > - 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
From: Andi Kleen <[EMAIL PROTECTED]> Date: 27 Aug 2005 04:34:07 +0200 > "David S. Miller" <[EMAIL PROTECTED]> writes: > > > From: Alexey Dobriyan <[EMAIL PROTECTED]> > > Date: Sat, 27 Aug 2005 02:58:48 +0400 > > > > > 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. 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
"David S. Miller" <[EMAIL PROTECTED]> writes: > From: Alexey Dobriyan <[EMAIL PROTECTED]> > Date: Sat, 27 Aug 2005 02:58:48 +0400 > > > 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) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Fri, 2005-08-26 at 17:44 -0500, Dmitry Torokhov wrote: > 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. > input_[un]register_device and del_timer_sync are "long" operations. I > think a semaphore would be better here. I was considering moving all locking to a single semaphore, actually. Robert Love - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
From: Alexey Dobriyan <[EMAIL PROTECTED]> Date: Sat, 27 Aug 2005 02:58:48 +0400 > 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Fri, Aug 26, 2005 at 06:18:45PM -0400, Robert Love wrote:
> Attached patch provides a driver for the IBM Hard Drive Active
> Protection System (hdaps) on top of 2.6.13-rc6-mm2.
> --- linux-2.6.13-rc6-mm2/drivers/hwmon/hdaps.c
> +++ linux/drivers/hwmon/hdaps.c
> +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 ()?
> +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, );
> + if (unlikely(ret < 0))
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On 8/26/05, Robert Love <[EMAIL PROTECTED]> wrote:
> +static void hdaps_calibrate(void)
> +{
> + int x, y, ret;
> +
> + ret = accelerometer_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, ,
> );
> + 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.
> +
> +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", ) != 1)
> + return -EINVAL;
> +
> + spin_lock(_lock);
> + if (enable == 1)
> + hdaps_mousedev_enable();
> + else if (enable == 0)
> + hdaps_mousedev_disable();
> + spin_unlock(_lock);
> +
> + return count;
> +}
input_[un]register_device and del_timer_sync are "long" operations. I
think a semaphore would be better here.
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch] 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
Signed-off-by: Robert Love <[EMAIL PROTECTED]>
MAINTAINERS|7
drivers/hwmon/Kconfig | 17 +
drivers/hwmon/Makefile |1
drivers/hwmon/hdaps.c | 664 +
4 files changed, 689 insertions(+)
diff -urN linux-2.6.13-rc6-mm2/drivers/hwmon/hdaps.c linux/drivers/hwmon/hdaps.c
--- linux-2.6.13-rc6-mm2/drivers/hwmon/hdaps.c 1969-12-31 19:00:00.0
-0500
+++ linux/drivers/hwmon/hdaps.c 2005-08-26 18:17:33.0 -0400
@@ -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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define HDAPS_LOW_PORT 0x1600 /* first port used by hdaps */
+#define HDAPS_HIGH_PORT0x162f /* last port used by hdaps */
+
+#define STATE_FRESH0x50/* 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 */
+#defineHDAPS_PORT_XPOS 0x1612 /* x-axis position */
+#define HDAPS_PORT_YPOS0x1614 /* y-axis position */
+#define HDAPS_PORT_TEMP0x1616 /* device temperature, in
celcius */
+#define HDAPS_PORT_XVAR0x1617 /* x-axis variance (what is
this?) */
+#define HDAPS_PORT_YVAR0x1619 /* 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_MASK0xff/* 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
[patch] 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
Signed-off-by: Robert Love [EMAIL PROTECTED]
MAINTAINERS|7
drivers/hwmon/Kconfig | 17 +
drivers/hwmon/Makefile |1
drivers/hwmon/hdaps.c | 664 +
4 files changed, 689 insertions(+)
diff -urN linux-2.6.13-rc6-mm2/drivers/hwmon/hdaps.c linux/drivers/hwmon/hdaps.c
--- linux-2.6.13-rc6-mm2/drivers/hwmon/hdaps.c 1969-12-31 19:00:00.0
-0500
+++ linux/drivers/hwmon/hdaps.c 2005-08-26 18:17:33.0 -0400
@@ -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_PORT0x162f /* last port used by hdaps */
+
+#define STATE_FRESH0x50/* 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 */
+#defineHDAPS_PORT_XPOS 0x1612 /* x-axis position */
+#define HDAPS_PORT_YPOS0x1614 /* y-axis position */
+#define HDAPS_PORT_TEMP0x1616 /* device temperature, in
celcius */
+#define HDAPS_PORT_XVAR0x1617 /* x-axis variance (what is
this?) */
+#define HDAPS_PORT_YVAR0x1619 /* 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_MASK0xff/* 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
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On 8/26/05, Robert Love [EMAIL PROTECTED] wrote:
+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.
+
+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;
+}
input_[un]register_device and del_timer_sync are long operations. I
think a semaphore would be better here.
--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Fri, Aug 26, 2005 at 06:18:45PM -0400, Robert Love wrote:
Attached patch provides a driver for the IBM Hard Drive Active
Protection System (hdaps) on top of 2.6.13-rc6-mm2.
--- linux-2.6.13-rc6-mm2/drivers/hwmon/hdaps.c
+++ linux/drivers/hwmon/hdaps.c
+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 ()?
+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))
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
From: Alexey Dobriyan [EMAIL PROTECTED] Date: Sat, 27 Aug 2005 02:58:48 +0400 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
On Fri, 2005-08-26 at 17:44 -0500, Dmitry Torokhov wrote: 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. input_[un]register_device and del_timer_sync are long operations. I think a semaphore would be better here. I was considering moving all locking to a single semaphore, actually. Robert Love - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
From: Andi Kleen [EMAIL PROTECTED] Date: 27 Aug 2005 04:34:07 +0200 David S. Miller [EMAIL PROTECTED] writes: From: Alexey Dobriyan [EMAIL PROTECTED] Date: Sat, 27 Aug 2005 02:58:48 +0400 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. 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
David S. Miller [EMAIL PROTECTED] writes: From: Alexey Dobriyan [EMAIL PROTECTED] Date: Sat, 27 Aug 2005 02:58:48 +0400 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) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
Andi Kleen wrote: - 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
Em Fri, Aug 26, 2005 at 07:37:59PM -0400, Robert Love escreveu: On Fri, 2005-08-26 at 17:44 -0500, Dmitry Torokhov wrote: 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] IBM HDAPS accelerometer driver, with probing.
Em Fri, Aug 26, 2005 at 09:06:22PM -0700, Mitchell Blank Jr escreveu: Andi Kleen wrote: - 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. Hey, even if unlikely was: #define unlikely(x) (x) I'd find it useful :-) - Arnaldo - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
