* Re: [lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function
@ 2008-04-29 4:08 Juerg Haefliger
2008-04-29 6:03 ` Jean Delvare
2008-05-01 4:08 ` Juerg Haefliger
0 siblings, 2 replies; 3+ messages in thread
From: Juerg Haefliger @ 2008-04-29 4:08 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
> 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\x1c28d80f1992240373099d863e4996cdd5d646d0
>
> 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@linux-fr.org>
> Cc: Justin Thiessen <jthiessen@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. Is there a reason not to check for < 2000 before looping?
...juerg
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function
2008-04-29 4:08 [lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function Juerg Haefliger
@ 2008-04-29 6:03 ` Jean Delvare
2008-05-01 4:08 ` Juerg Haefliger
1 sibling, 0 replies; 3+ messages in thread
From: Jean Delvare @ 2008-04-29 6:03 UTC (permalink / raw)
To: lm-sensors
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\x1c28d80f1992240373099d863e4996cdd5d646d0
> >
> > 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@linux-fr.org>
> > Cc: Justin Thiessen <jthiessen@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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function
2008-04-29 4:08 [lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function Juerg Haefliger
2008-04-29 6:03 ` Jean Delvare
@ 2008-05-01 4:08 ` Juerg Haefliger
1 sibling, 0 replies; 3+ messages in thread
From: Juerg Haefliger @ 2008-05-01 4:08 UTC (permalink / raw)
To: lm-sensors
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\x1c28d80f1992240373099d863e4996cdd5d646d0
> > >
> > > 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@linux-fr.org>
> > > Cc: Justin Thiessen <jthiessen@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
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-05-01 4:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-04-29 4:08 [lm-sensors] [PATCH 01/10] hwmon: (lm85) Fix function Juerg Haefliger
2008-04-29 6:03 ` Jean Delvare
2008-05-01 4:08 ` Juerg Haefliger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.