adt7473: fix voltage conversion

PATCH In review Paulius Zaleckas 2 participants 2 messages Apr 22, 2008 – Apr 26, 2008 Hardware monitoring

Share & tools

Paulius Zaleckas

author

2008-04-22 22:54 UTC #0 lore raw

PATCH 1/1 adt7473: fix voltage conversion

Fixes voltage conversion routines.
According to the datasheet voltage is scaled with resistors and value 192 is nominal voltage. 0 is 0V.
Conversion routines taken from the lm85 driver

Signed-of-by Paulius Zaleckas <[email protected]>

drivers/hwmon/adt7473.c +16 ~~-32~~

index 9587869..6eeea6a 100644
@@ -317,37 +317,21 @@ out:
 	return data;
 }
 
-/*
- * On this chip, voltages are given as a count of steps between a minimum
- * and maximum voltage, not a direct voltage.
+/* Conversions. Rounding and limit checking is only done on the TO_REG 
+   variants. Note that you should be a bit careful with which arguments
+   these macros are called: arguments may be evaluated more than once.
  */
-static const int volt_convert_table[][2] = {
-	{2997, 3},
-	{4395, 4},
-};
-
-static int decode_volt(int volt_index, u8 raw)
-{
-	int cmax = volt_convert_table[volt_index][0];
-	int cmin = volt_convert_table[volt_index][1];
-	return ((raw * (cmax - cmin)) / 255) + cmin;
-}
-
-static u8 encode_volt(int volt_index, int cooked)
-{
-	int cmax = volt_convert_table[volt_index][0];
-	int cmin = volt_convert_table[volt_index][1];
-	u8 x;
 
-	if (cooked > cmax)
-		cooked = cmax;
-	else if (cooked < cmin)
-		cooked = cmin;
+/* IN are scaled acording to built-in resistors */
+static int adt7473_scaling[] = {  /* .001 Volts */
+		2250, 3300
+	};
+#define SCALE(val,from,to)		(((val)*(to) + ((from)/2))/(from))
 
-	x = ((cooked - cmin) * 255) / (cmax - cmin);
+#define INS_TO_REG(n,val)	\
+		SENSORS_LIMIT(SCALE(val,adt7473_scaling[n],192),0,255)
 
-	return x;
-}
+#define INS_FROM_REG(n,val)	SCALE((val), 192, adt7473_scaling[n])
 
 static ssize_t show_volt_min(struct device *dev,
 			     struct device_attribute *devattr,

@@ -356,7 +340,7 @@ static ssize_t show_volt_min(struct device *dev,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct adt7473_data *data = adt7473_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       decode_volt(attr->index, data->volt_min[attr->index]));
+		       INS_FROM_REG(attr->index, data->volt_min[attr->index]));
 }
 
 static ssize_t set_volt_min(struct device *dev,

@@ -367,7 +351,7 @@ static ssize_t set_volt_min(struct device *dev,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adt7473_data *data = i2c_get_clientdata(client);
-	int volt = encode_volt(attr->index, simple_strtol(buf, NULL, 10));
+	int volt = INS_TO_REG(attr->index, simple_strtol(buf, NULL, 10));
 
 	mutex_lock(&data->lock);
 	data->volt_min[attr->index] = volt;

@@ -385,7 +369,7 @@ static ssize_t show_volt_max(struct device *dev,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct adt7473_data *data = adt7473_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       decode_volt(attr->index, data->volt_max[attr->index]));
+		       INS_FROM_REG(attr->index, data->volt_max[attr->index]));
 }
 
 static ssize_t set_volt_max(struct device *dev,

@@ -396,7 +380,7 @@ static ssize_t set_volt_max(struct device *dev,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adt7473_data *data = i2c_get_clientdata(client);
-	int volt = encode_volt(attr->index, simple_strtol(buf, NULL, 10));
+	int volt = INS_TO_REG(attr->index, simple_strtol(buf, NULL, 10));
 
 	mutex_lock(&data->lock);
 	data->volt_max[attr->index] = volt;

@@ -414,7 +398,7 @@ static ssize_t show_volt(struct device *dev, struct device_attribute *devattr,
 	struct adt7473_data *data = adt7473_update_device(dev);
 
 	return sprintf(buf, "%d\n",
-		       decode_volt(attr->index, data->volt[attr->index]));
+		       INS_FROM_REG(attr->index, data->volt[attr->index]));
 }
 
 /*

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2008-04-26 13:08 UTC #1 lore raw

Hi Paulius,

Adding Darrick in Cc in case he missed this patch.

Could any of you please send me a dump of an ADT7473 chip for my collection?

Quoted 4 lines · Paulius Zaleckas

Fixes voltage conversion routines.
According to the datasheet voltage is scaled with resistors and value 192 is nominal voltage. 0 is 0V.
Conversion routines taken from the lm85 driver

Together with its horrible coding style :( While copying the logic from the lm85 driver is fine, copying the code itself isn't so. It uses macros where (possibly inline) functions are preferred, had bad indentation, etc...

So, can you please resend a patch that would do only the required changes? Keep decode_volt and encode_volt as the function names, and just change the code inside these functions. This should make for a much nicer patch.

Quoted 2 lines

Signed-of-by Paulius Zaleckas <[email protected]>

Typo: Signed-off-by.

Review:

Quoted 31 lines

drivers/hwmon/adt7473.c +3 ~~-19~~

index 9587869..6eeea6a 100644
@@ -317,37 +317,21 @@ out:
 	return data;
 }
 
-/*
- * On this chip, voltages are given as a count of steps between a minimum
- * and maximum voltage, not a direct voltage.
+/* Conversions. Rounding and limit checking is only done on the TO_REG 
+   variants. Note that you should be a bit careful with which arguments
+   these macros are called: arguments may be evaluated more than once.
  */
-static const int volt_convert_table[][2] = {
-	{2997, 3},
-	{4395, 4},
-};
-
-static int decode_volt(int volt_index, u8 raw)
-{
-	int cmax = volt_convert_table[volt_index][0];
-	int cmin = volt_convert_table[volt_index][1];
-	return ((raw * (cmax - cmin)) / 255) + cmin;
-}
-
-static u8 encode_volt(int volt_index, int cooked)
-{
-	int cmax = volt_convert_table[volt_index][0];
-	int cmin = volt_convert_table[volt_index][1];

drivers/hwmon/adt7473.c out: · line 328

Quoted 10 lines

drivers/hwmon/adt7473.c +4 -5

-	u8 x;
 
-	if (cooked > cmax)
-		cooked = cmax;
-	else if (cooked < cmin)
-		cooked = cmin;
+/* IN are scaled acording to built-in resistors */
+static int adt7473_scaling[] = {  /* .001 Volts */
+		2250, 3300
+	};

Too many tabs.

drivers/hwmon/adt7473.c out: · line 329

Quoted 1 line

drivers/hwmon/adt7473.c +1 -0

+#define SCALE(val,from,to)		(((val)*(to) + ((from)/2))/(from))

Would better be an inline function. There should be spaces around * and /.

drivers/hwmon/adt7473.c out: · line 337

Quoted 12 lines

drivers/hwmon/adt7473.c +3 -3

-	x = ((cooked - cmin) * 255) / (cmax - cmin);
+#define INS_TO_REG(n,val)	\
+		SENSORS_LIMIT(SCALE(val,adt7473_scaling[n],192),0,255)
 
-	return x;
-}
+#define INS_FROM_REG(n,val)	SCALE((val), 192, adt7473_scaling[n])
 
 static ssize_t show_volt_min(struct device *dev,
 			     struct device_attribute *devattr,
(...)

While we're here, I've read the ADT7473 datasheet and it states that bit 5 of configuration register 2 (0x73) disables the internal attenuator for Vccp. In that case 2.250V is no longer the nominal value (register value 192) but the maximum value (virtually register value 256). It would be great if the adt7473 driver could handle this case, otherwise the reported voltage value is incorrect. Maybe something for a next patch?

Thanks,

Signature · 6 lines

Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors