[lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function RANGE_TO_REG()

[Juerg Haefliger]

Hi Jean,


> 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.

True.

Acked-by: Juerg Haefliger <juergh at gmail.com>


>  --
>  Jean Delvare
>