I2C: lm80 driver improvement (From Aurelien)

PATCH In review Shawn Starr 4 participants 13 messages Jan 26, 2005 – May 19, 2005

lore

Share & tools

lore

Shawn Starr

author

2005-01-26 07:49 UTC #0 lore raw

PATCH 1/1 I2C: lm80 driver improvement (From Aurelien)

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 <[email protected]>

Signed-off-by Shawn Starr <[email protected]>

drivers/i2c/chips/lm80.c 2005-01-26 02:41:00.000000000 -0500 renamed from drivers/i2c/chips/lm80.c 2005-01-26 02:04:38.000000000 -0500 +13 -6

@@ -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);

Shawn Starr

author

2005-01-26 07:57 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Shawn Starr <[email protected]>

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

drivers/i2c/chips/lm80.c +13 -6

@@ -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);

Greg KH

2005-01-26 16:46 UTC #2 lore raw

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

drivers/i2c/chips/lm80.c SENSORS_INSMOD_1(lm80); · line 75

Quoted 4 lines · Shawn Starr

static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div) {
drivers/i2c/chips/lm80.c +1 -1
-	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

Shawn Starr

author

2005-01-26 23:15 UTC #3 lore raw

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

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

Quoted 10 lines · --- Greg KH

Quoted 1 line · Shawn Starr
static inline unsigned char FAN_TO_REG(unsigned
rpm, unsigned div)
Review
Quoted 3 lines
{
(quoted diff) +1 -1
-	if (rpm == 0)
+	if (rpm <= 0)
As was pointed out, this doesn't make any sense.

Quoted 6 lines

Care to redo the patch?
thanks,
greg k-h

drivers/i2c/chips/lm80.c +12 -5

@@ -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);

Greg KH

2005-01-27 09:07 UTC #4 lore raw

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

Quoted 3 lines · Shawn Starr

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

Shawn Starr

author

2005-01-27 16:58 UTC #5 lore raw

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

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

Approved-by Greg KH <[email protected]>

Signed-off-by Sytse Wielinga

<[email protected]>

Signed-off-by Aurelien Jarno <[email protected]>

Signed-off-by Shawn Starr <[email protected]>

Quoted 6 lines · --- Greg KH

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

Quoted 8 lines

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

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

Approved-by Greg KH <[email protected]>

Signed-off-by Sytse Wielinga <[email protected]>

Signed-off-by Aurelien Jarno <[email protected]>

Signed-off-by Shawn Starr <[email protected]>

drivers/i2c/chips/lm80.c +12 -5

@@ -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);

Greg KH

2005-01-29 23:28 UTC #6 lore raw

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

Quoted 2 lines · Shawn Starr

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.

Quoted 1 line

Approved-by Greg KH <[email protected]>

I have not "approved it" yet at all.

Quoted 4 lines

Signed-off-by: Sytse Wielinga
<[email protected]>
Signed-off-by Aurelien Jarno <[email protected]>
Signed-off-by Shawn Starr <[email protected]>

You have line wrap here too :(

Quoted 7 lines

Quoted 6 lines · --- Greg KH
Quoted 3 lines · Shawn Starr
Here is the corrected fix, yeah that didn't make
sense.
3AM isn't a good time to send patches I guess :-)

Quoted 8 lines

Quoted 8 lines
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

Shawn Starr

author

2005-01-30 02:09 UTC #7 lore raw

PATCH 1/1 I2C: lm80 driver improvement - again...

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

Signed-off-by Sytse Wielinga <[email protected]>

Signed-off-by Aurelien Jarno <[email protected]>

Signed-off-by Shawn Starr <[email protected]>

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

Approved-by Greg KH <[email protected]>

Signed-off-by Sytse Wielinga <[email protected]>

Signed-off-by Aurelien Jarno <[email protected]>

Signed-off-by Shawn Starr <[email protected]>

drivers/i2c/chips/lm80.c +12 -5

@@ -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);

Greg KH

Applied

2005-02-01 08:56 UTC #8 lore raw

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

Quoted 5 lines · Shawn Starr

Description: Cleanup some cluttered macros, add error checking for fan divisor value set.
Signed-off-by Sytse Wielinga <[email protected]>
Signed-off-by Aurelien Jarno <[email protected]>
Signed-off-by Shawn Starr <[email protected]>

Applied, thanks.

greg k-h

Shawn Starr

author

2005-05-19 06:25 UTC #9 lore raw

PATCH 1/1 I2C: lm80 driver improvement - again...

In reply to Shawn Starr

The patch is not yet Approved by Greg..

Quoted 6 lines · Shawn Starr

Description: Cleanup some cluttered macros, add error checking for fan divisor value set.
Signed-off-by Sytse Wielinga <[email protected]>
Signed-off-by Aurelien Jarno <[email protected]>
Signed-off-by Shawn Starr <[email protected]>

Aurélien Jarno

2005-01-27 02:49 UTC #10 lore raw

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

In reply to Greg KH

Quoted 11 lines · Greg KH

drivers/i2c/chips/lm80.c SENSORS_INSMOD_1(lm80); · line 75
Quoted 4 lines · Shawn Starr
static inline unsigned char FAN_TO_REG(unsigned rpm, unsigned div) {
drivers/i2c/chips/lm80.c +1 -1
-	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

Signature · 4 lines

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

Sytse Wielinga

2005-01-26 14:06 UTC #11 lore raw

In reply to Shawn Starr

Quoted 4 lines · Shawn Starr

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

Sytse Wielinga

2005-01-26 14:21 UTC #12 lore raw

Quoted 2 lines · 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