Hi Juerg, On Mon, 28 Apr 2008 21:08:45 -0700, Juerg Haefliger wrote: > > Function RANGE_TO_REG() is broken. For a requested range of 2000 (2 > > degrees C), it will return an index value of 15, i.e. 80.0 degrees C, > > instead of the expected index value of 0. All other values are handled > > properly, just 2000 isn't. > > > > The bug was introduced back in November 2004 by this patch: > > http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=1c28d80f1992240373099d863e4996cdd5d646d0 > > > > While this can be fixed easily with the current code, I'd rather > > rewrite the whole function in a way which is more obviously correct. > > > > Signed-off-by: Jean Delvare <khali at linux-fr.org> > > Cc: Justin Thiessen <jthiessen at penguincomputing.com> > > --- > > Note: this is the same patch as I already sent on April 3rd. > > > > drivers/hwmon/lm85.c | 25 +++++++++++-------------- > > 1 file changed, 11 insertions(+), 14 deletions(-) > > > > --- linux-2.6.25-rc8.orig/drivers/hwmon/lm85.c 2008-04-02 22:20:01.000000000 +0200 > > +++ linux-2.6.25-rc8/drivers/hwmon/lm85.c 2008-04-02 23:10:16.000000000 +0200 > > @@ -192,23 +192,20 @@ static int RANGE_TO_REG( int range ) > > { > > int i; > > > > - if ( range < lm85_range_map[0] ) { > > - return 0 ; > > - } else if ( range > lm85_range_map[15] ) { > > + if (range >= lm85_range_map[15]) > > return 15 ; > > - } else { /* find closest match */ > > - for ( i = 14 ; i >= 0 ; --i ) { > > - if ( range > lm85_range_map[i] ) { /* range bracketed */ > > - if ((lm85_range_map[i+1] - range) < > > - (range - lm85_range_map[i])) { > > - i++; > > - break; > > - } > > - break; > > - } > > + > > + /* Find the closest match */ > > + for (i = 14; i >= 0; --i) { > > + if (range >= lm85_range_map[i]) { > > + if ((lm85_range_map[i + 1] - range) < > > + (range - lm85_range_map[i])) > > + return i + 1; > > + return i; > > } > > } > > - return( i & 0x0f ); > > + > > + return 0; > > } > > #define RANGE_FROM_REG(val) (lm85_range_map[(val)&0x0f]) > > > > This works but is less efficient compared to the original code for range > values < 2000. But it is slightly more efficient for all values between 2000 and 80000. The average efficiency is exactly the same as those of the original code. > Is there a reason not to check for < 2000 before looping? Code size. The < 2000 case is handled just fine by the for loop, so I don't see the point of making the code bigger. Note that performance is hardly an issue here anyway, users aren't going to change this parameter very often. -- Jean Delvare
