[PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien)

[PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien)

[Shawn Starr]

You know, after seeing that patch, that it just makes sense to add some of those cleanups to the lm80 driver.

Shawn.

lm80-i2c-0-28
Adapter: SMBus PIIX4 adapter at fe00
+5V:       +4.90 V  (min =  +4.74 V, max =  +5.25 V)
VTT:       +1.72 V  (min =  +1.70 V, max =  +2.10 V)
+3.3V:     +3.35 V  (min =  +3.14 V, max =  +3.47 V)
+Vcore:    +1.98 V  (min =  +1.90 V, max =  +2.10 V)
+12V:     +11.30 V  (min = +11.18 V, max = +12.63 V)
-12V:     -11.88 V  (min = -12.60 V, max = -11.39 V)
-5V:       -4.92 V  (min =  -5.25 V, max =  -4.76 V)
fan1:     1679 RPM  (min = 1328 RPM, div = 4)
temp:     +32.38°C (hot: limit = +60°C, hyst = +65°C)
:                  (os:  limit = +54°C, hyst = +56°C)

Driver works with changes applied.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

--- linux-2.6.11-rc2/drivers/i2c/chips/lm80.c 2005-01-26 02:04:38.000000000 -0500
+++ linux-2.6.11-rc2-fixes/drivers/i2c/chips/lm80.c 2005-01-26 02:41:00.000000000 -0500
@@ -72,7 +72,7 @@ SENSORS_INSMOD_1(lm80);
 
 static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div)
 {
- if (rpm == 0)
+ if (rpm <= 0)
   return 255;
  rpm = SENSORS_LIMIT(rpm, 1, 1000000);
  return SENSORS_LIMIT((1350000 + rpm*div / 2) / (rpm*div), 1, 254);
@@ -99,10 +99,7 @@ static inline long TEMP_FROM_REG(u16 tem
 #define TEMP_LIMIT_TO_REG(val)  SENSORS_LIMIT((val)<0?\
      ((val)-500)/1000:((val)+500)/1000,0,255)
 
-#define ALARMS_FROM_REG(val)  (val)
-
 #define DIV_FROM_REG(val)  (1 << (val))
-#define DIV_TO_REG(val)   ((val)==8?3:(val)==4?2:(val)==1?0:1)
 
 /*
  * Client data (each client gets its own)
@@ -269,7 +266,17 @@ static ssize_t set_fan_div(struct device
       DIV_FROM_REG(data->fan_div[nr]));
 
  val = simple_strtoul(buf, NULL, 10);
- data->fan_div[nr] = DIV_TO_REG(val);
+
+ switch (val) {
+ case 1: data->fan_div[nr] = 0; break;
+ case 2: data->fan_div[nr] = 1; break;
+ case 4: data->fan_div[nr] = 2; break;
+ case 8: data->fan_div[nr] = 3; break;
+ default:
+  dev_err(&client->dev, "fan_div value %ld not "
+   "supported. Choose one of 1, 2, 4 or 8!\n", val);
+  return -EINVAL;
+ }
 
  reg = (lm80_read_value(client, LM80_REG_FANDIV) & ~(3 << (2 * (nr + 1))))
      | (data->fan_div[nr] << (2 * (nr + 1)));
@@ -327,7 +334,7 @@ set_temp(os_hyst, temp_os_hyst, LM80_REG
 static ssize_t show_alarms(struct device *dev, char *buf)
 {
  struct lm80_data *data = lm80_update_device(dev);
- return sprintf(buf, "%d\n", ALARMS_FROM_REG(data->alarms));
+ return sprintf(buf, "%u\n", data->alarms);
 }
 
 static DEVICE_ATTR(in0_min, S_IWUSR | S_IRUGO, show_in_min0, set_in_min0);

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien) - Resubmit

[Shawn Starr]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

You know, after seeing that patch, that it just makes sense to add some of
those cleanups to the lm80 driver.

Shawn.

lm80-i2c-0-28
Adapter: SMBus PIIX4 adapter at fe00
+5V:       +4.90 V  (min =  +4.74 V, max =  +5.25 V)
VTT:       +1.72 V  (min =  +1.70 V, max =  +2.10 V)
+3.3V:     +3.35 V  (min =  +3.14 V, max =  +3.47 V)
+Vcore:    +1.98 V  (min =  +1.90 V, max =  +2.10 V)
+12V:     +11.30 V  (min = +11.18 V, max = +12.63 V)
-12V:     -11.88 V  (min = -12.60 V, max = -11.39 V)
-5V:       -4.92 V  (min =  -5.25 V, max =  -4.76 V)
fan1:     1679 RPM  (min = 1328 RPM, div = 4)
temp:     +32.38°C (hot: limit = +60°C, hyst = +65°C)
:                  (os:  limit = +54°C, hyst = +56°C)

Driver works with changes applied.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

* Note to self, dont use Kmail for raw diff files

[-- Attachment #2: lm80-cleanup.diff --]
[-- Type: text/x-diff, Size: 1788 bytes --]

--- linux-2.6.11-rc2/drivers/i2c/chips/lm80.c	2005-01-26 02:04:38.000000000 -0500
+++ linux-2.6.11-rc2-fixes/drivers/i2c/chips/lm80.c	2005-01-26 02:41:00.000000000 -0500
@@ -72,7 +72,7 @@ SENSORS_INSMOD_1(lm80);
 
 static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div)
 {
-	if (rpm == 0)
+	if (rpm <= 0)
 		return 255;
 	rpm = SENSORS_LIMIT(rpm, 1, 1000000);
 	return SENSORS_LIMIT((1350000 + rpm*div / 2) / (rpm*div), 1, 254);
@@ -99,10 +99,7 @@ static inline long TEMP_FROM_REG(u16 tem
 #define TEMP_LIMIT_TO_REG(val)		SENSORS_LIMIT((val)<0?\
 					((val)-500)/1000:((val)+500)/1000,0,255)
 
-#define ALARMS_FROM_REG(val)		(val)
-
 #define DIV_FROM_REG(val)		(1 << (val))
-#define DIV_TO_REG(val)			((val)==8?3:(val)==4?2:(val)==1?0:1)
 
 /*
  * Client data (each client gets its own)
@@ -269,7 +266,17 @@ static ssize_t set_fan_div(struct device
 			   DIV_FROM_REG(data->fan_div[nr]));
 
 	val = simple_strtoul(buf, NULL, 10);
-	data->fan_div[nr] = DIV_TO_REG(val);
+
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
 
 	reg = (lm80_read_value(client, LM80_REG_FANDIV) & ~(3 << (2 * (nr + 1))))
 	    | (data->fan_div[nr] << (2 * (nr + 1)));
@@ -327,7 +334,7 @@ set_temp(os_hyst, temp_os_hyst, LM80_REG
 static ssize_t show_alarms(struct device *dev, char *buf)
 {
 	struct lm80_data *data = lm80_update_device(dev);
-	return sprintf(buf, "%d\n", ALARMS_FROM_REG(data->alarms));
+	return sprintf(buf, "%u\n", data->alarms);
 }
 
 static DEVICE_ATTR(in0_min, S_IWUSR | S_IRUGO, show_in_min0, set_in_min0);

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien) - Resubmit

[Greg KH]

On Wed, Jan 26, 2005 at 02:57:35AM -0500, Shawn Starr wrote:
>  static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div)
>  {
> -	if (rpm == 0)
> +	if (rpm <= 0)

As was pointed out, this doesn't make any sense.

Care to redo the patch?

thanks,

greg k-h

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien) - Resubmit #2

[Shawn Starr]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

Here is the corrected fix, yeah that didn't make
sense.   
3AM isn't a good time to send patches I guess :-)

 --- Greg KH <greg@kroah.com> wrote: 
> On Wed, Jan 26, 2005 at 02:57:35AM -0500, Shawn
> Starr wrote:
> >  static inline unsigned char FAN_TO_REG(unsigned
> rpm, unsigned div)
> >  {
> > -	if (rpm == 0)
> > +	if (rpm <= 0)
> 
> As was pointed out, this doesn't make any sense.
> 
> Care to redo the patch?
> 
> thanks,
> 
> greg k-h
>  

[-- Attachment #2: lm80-fixup-2.diff --]
[-- Type: application/octet-stream, Size: 1517 bytes --]

--- linux-2.6.11-rc2/drivers/i2c/chips/lm80.c	2005-01-26 02:04:38.000000000 -0500
+++ linux-2.6.11-rc2-fixes/drivers/i2c/chips/lm80.c	2005-01-26 12:31:26.000000000 -0500
@@ -99,10 +99,7 @@ static inline long TEMP_FROM_REG(u16 tem
 #define TEMP_LIMIT_TO_REG(val)		SENSORS_LIMIT((val)<0?\
 					((val)-500)/1000:((val)+500)/1000,0,255)
 
-#define ALARMS_FROM_REG(val)		(val)
-
 #define DIV_FROM_REG(val)		(1 << (val))
-#define DIV_TO_REG(val)			((val)==8?3:(val)==4?2:(val)==1?0:1)
 
 /*
  * Client data (each client gets its own)
@@ -269,7 +266,17 @@ static ssize_t set_fan_div(struct device
 			   DIV_FROM_REG(data->fan_div[nr]));
 
 	val = simple_strtoul(buf, NULL, 10);
-	data->fan_div[nr] = DIV_TO_REG(val);
+
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
 
 	reg = (lm80_read_value(client, LM80_REG_FANDIV) & ~(3 << (2 * (nr + 1))))
 	    | (data->fan_div[nr] << (2 * (nr + 1)));
@@ -327,7 +334,7 @@ set_temp(os_hyst, temp_os_hyst, LM80_REG
 static ssize_t show_alarms(struct device *dev, char *buf)
 {
 	struct lm80_data *data = lm80_update_device(dev);
-	return sprintf(buf, "%d\n", ALARMS_FROM_REG(data->alarms));
+	return sprintf(buf, "%u\n", data->alarms);
 }
 
 static DEVICE_ATTR(in0_min, S_IWUSR | S_IRUGO, show_in_min0, set_in_min0);

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien) - Resubmit #2

[Greg KH]

On Wed, Jan 26, 2005 at 12:31:30PM -0500, Shawn Starr wrote:
> Here is the corrected fix, yeah that didn't make
> sense.   
> 3AM isn't a good time to send patches I guess :-)

Care to resend it, with a full description and a Signed-off-by: line so
I can apply it?

thanks,

greg k-h

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement -

[Shawn Starr]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

Description: Cleanup some cluttered macros, add error
checking for fan divisor value set.

Approved-by: Greg KH <greg@kroah.com>
Signed-off-by: Sytse Wielinga
<s.b.wielinga@student.utwente.nl>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

 --- Greg KH <greg@kroah.com> wrote: 
> On Wed, Jan 26, 2005 at 12:31:30PM -0500, Shawn
> Starr wrote:
> > Here is the corrected fix, yeah that didn't make
> > sense.   
> > 3AM isn't a good time to send patches I guess :-)
> 
> Care to resend it, with a full description and a
> Signed-off-by: line so
> I can apply it?
> 
> thanks,
> 
> greg k-h
>  

[-- Attachment #2: lm80-fixup-3.diff --]
[-- Type: application/octet-stream, Size: 1817 bytes --]


Description: Cleanup some cluttered macros, add error checking for fan divisor value set.

Approved-by: Greg KH <greg@kroah.com>
Signed-off-by: Sytse Wielinga <s.b.wielinga@student.utwente.nl>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

--- linux-2.6.11-rc2/drivers/i2c/chips/lm80.c	2005-01-26 02:04:38.000000000 -0500
+++ linux-2.6.11-rc2-fixes/drivers/i2c/chips/lm80.c	2005-01-26 12:31:26.000000000 -0500
@@ -99,10 +99,7 @@ static inline long TEMP_FROM_REG(u16 tem
 #define TEMP_LIMIT_TO_REG(val)		SENSORS_LIMIT((val)<0?\
 					((val)-500)/1000:((val)+500)/1000,0,255)
 
-#define ALARMS_FROM_REG(val)		(val)
-
 #define DIV_FROM_REG(val)		(1 << (val))
-#define DIV_TO_REG(val)			((val)==8?3:(val)==4?2:(val)==1?0:1)
 
 /*
  * Client data (each client gets its own)
@@ -269,7 +266,17 @@ static ssize_t set_fan_div(struct device
 			   DIV_FROM_REG(data->fan_div[nr]));
 
 	val = simple_strtoul(buf, NULL, 10);
-	data->fan_div[nr] = DIV_TO_REG(val);
+
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
 
 	reg = (lm80_read_value(client, LM80_REG_FANDIV) & ~(3 << (2 * (nr + 1))))
 	    | (data->fan_div[nr] << (2 * (nr + 1)));
@@ -327,7 +334,7 @@ set_temp(os_hyst, temp_os_hyst, LM80_REG
 static ssize_t show_alarms(struct device *dev, char *buf)
 {
 	struct lm80_data *data = lm80_update_device(dev);
-	return sprintf(buf, "%d\n", ALARMS_FROM_REG(data->alarms));
+	return sprintf(buf, "%u\n", data->alarms);
 }
 
 static DEVICE_ATTR(in0_min, S_IWUSR | S_IRUGO, show_in_min0, set_in_min0);

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement -

[Greg KH]

On Thu, Jan 27, 2005 at 11:56:46AM -0500, Shawn Starr wrote:
> Description: Cleanup some cluttered macros, add error
> checking for fan divisor value set.

Hm, we'll get there yet.  Your patch was not in a plain text form, so
that I could apply it directly from the email.

> Approved-by: Greg KH <greg@kroah.com>

I have not "approved it" yet at all.

> Signed-off-by: Sytse Wielinga
> <s.b.wielinga@student.utwente.nl>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

You have line wrap here too :(

>  --- Greg KH <greg@kroah.com> wrote: 
> > On Wed, Jan 26, 2005 at 12:31:30PM -0500, Shawn
> > Starr wrote:
> > > Here is the corrected fix, yeah that didn't make
> > > sense.   
> > > 3AM isn't a good time to send patches I guess :-)
> > 
> > Care to resend it, with a full description and a
> > Signed-off-by: line so
> > I can apply it?
> > 
> > thanks,
> > 
> > greg k-h
> >  

What's this "bottom post" stuff in here?

Also, please CC: the sensors mailing list so the developers there can
review the changes.

Can you try it again from scratch?

thanks,

greg k-h

[PATCH 2.6.11-rc2] I2C: lm80 driver improvement - again...

[Shawn Starr]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

Description: Cleanup some cluttered macros, add error checking for fan divisor value set.

Signed-off-by: Sytse Wielinga <s.b.wielinga@student.utwente.nl>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Shawn Starr <shawn.starr@rogers.com>


[-- Attachment #2: lm80-fixup-3.diff --]
[-- Type: text/plain, Size: 1817 bytes --]


Description: Cleanup some cluttered macros, add error checking for fan divisor value set.

Approved-by: Greg KH <greg@kroah.com>
Signed-off-by: Sytse Wielinga <s.b.wielinga@student.utwente.nl>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

--- linux-2.6.11-rc2/drivers/i2c/chips/lm80.c	2005-01-26 02:04:38.000000000 -0500
+++ linux-2.6.11-rc2-fixes/drivers/i2c/chips/lm80.c	2005-01-26 12:31:26.000000000 -0500
@@ -99,10 +99,7 @@ static inline long TEMP_FROM_REG(u16 tem
 #define TEMP_LIMIT_TO_REG(val)		SENSORS_LIMIT((val)<0?\
 					((val)-500)/1000:((val)+500)/1000,0,255)
 
-#define ALARMS_FROM_REG(val)		(val)
-
 #define DIV_FROM_REG(val)		(1 << (val))
-#define DIV_TO_REG(val)			((val)==8?3:(val)==4?2:(val)==1?0:1)
 
 /*
  * Client data (each client gets its own)
@@ -269,7 +266,17 @@ static ssize_t set_fan_div(struct device
 			   DIV_FROM_REG(data->fan_div[nr]));
 
 	val = simple_strtoul(buf, NULL, 10);
-	data->fan_div[nr] = DIV_TO_REG(val);
+
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
 
 	reg = (lm80_read_value(client, LM80_REG_FANDIV) & ~(3 << (2 * (nr + 1))))
 	    | (data->fan_div[nr] << (2 * (nr + 1)));
@@ -327,7 +334,7 @@ set_temp(os_hyst, temp_os_hyst, LM80_REG
 static ssize_t show_alarms(struct device *dev, char *buf)
 {
 	struct lm80_data *data = lm80_update_device(dev);
-	return sprintf(buf, "%d\n", ALARMS_FROM_REG(data->alarms));
+	return sprintf(buf, "%u\n", data->alarms);
 }
 
 static DEVICE_ATTR(in0_min, S_IWUSR | S_IRUGO, show_in_min0, set_in_min0);

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement - again...

[Greg KH]

On Sat, Jan 29, 2005 at 09:08:55PM -0500, Shawn Starr wrote:
> Description: Cleanup some cluttered macros, add error checking for fan divisor value set.
> 
> Signed-off-by: Sytse Wielinga <s.b.wielinga@student.utwente.nl>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Shawn Starr <shawn.starr@rogers.com>

Applied, thanks.

greg k-h

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien) - Resubmit

[Aurélien Jarno]

Greg KH wrote:
> On Wed, Jan 26, 2005 at 02:57:35AM -0500, Shawn Starr wrote:
> 
>> static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div)
>> {
>>-	if (rpm == 0)
>>+	if (rpm <= 0)
> 
> 
> As was pointed out, this doesn't make any sense.
> 
> Care to redo the patch?
Please note that the problem is not present in the lm78 patch as the 
argument is of type int.

Aurelien

-- 
   .''`.  Aurelien Jarno               GPG: 1024D/F1BCDB73
  : :' :  Debian GNU/Linux developer | Electrical Engineer
  `. `'   aurel32@debian.org         | aurelien@aurel32.net
    `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien)

[Sytse Wielinga]

On Wed, Jan 26, 2005 at 02:49:23AM -0500, Shawn Starr wrote:
>  static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div)
>  {
> - if (rpm == 0)
> + if (rpm <= 0)
The rpm parameter is unsigned so this change is useless. The rest makes sense
to me.

BTW, can anyone tell me why the uints in this parameter list are declared as
'unsigned' and not as 'unsigned int'?

    Sytse

Re: [PATCH 2.6.11-rc2] I2C: lm80 driver improvement (From Aurelien)

[Sytse Wielinga]

On Wed, Jan 26, 2005 at 03:05:09PM +0100, I wrote:
> BTW, can anyone tell me why the uints in this parameter list are declared as
> 'unsigned' and not as 'unsigned int'?
$ find /usr/src/linux/ -name \*.c |xargs grep unsigned\ [^icsl] |wc -l
3151

- Gives himself a good smack on the head -

Sorry about that. I suppose it's just used a lot as a shorthand for 'unsigned
int', which is used a lot more in the kernel (the above command finds 22656
occurrences). On a sidenote, however, wouldn't it be nice for things like this
to be consistent throughout the kernel? Don't think I have an opinion on this,
because I don't, but I'd like to see those of the ones who do.

    Sytse