[lm-sensors] [PATCH v3 1/4] hwmon: (tmp401) Simplification and cleanup

[Guenter Roeck <linux@roeck-us.net>]

Use two-dimensional array pointing to registers
Merge temperature and limit access functions into a single function
Return error codes from I2C reads
Use DIV_ROUND_CLOSEST for rounding operations and improve rounding

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: Improve rounding
    Fix spelling error in commit log
    Fix rounding error when writing critical limit

v2: Fix spelling error in commit log, and clarify that only i2c reads
    return errors
    Mark unused array entries by using 0 instead of 0x00 for improved
    readability
    Fix rounding error when writing critical limit
    Don't rename reg to regval in store_temp
    Use local client variable in store_temp

 drivers/hwmon/tmp401.c |  371 ++++++++++++++++++------------------------------
 1 file changed, 137 insertions(+), 234 deletions(-)

diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index f9c492e..4d306b2 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -58,21 +58,32 @@ enum chips { tmp401, tmp411, tmp431 };
 #define TMP401_MANUFACTURER_ID_REG		0xFE
 #define TMP401_DEVICE_ID_REG			0xFF
 
-static const u8 TMP401_TEMP_MSB[2]			= { 0x00, 0x01 };
-static const u8 TMP401_TEMP_LSB[2]			= { 0x15, 0x10 };
-static const u8 TMP401_TEMP_LOW_LIMIT_MSB_READ[2]	= { 0x06, 0x08 };
-static const u8 TMP401_TEMP_LOW_LIMIT_MSB_WRITE[2]	= { 0x0C, 0x0E };
-static const u8 TMP401_TEMP_LOW_LIMIT_LSB[2]		= { 0x17, 0x14 };
-static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_READ[2]	= { 0x05, 0x07 };
-static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[2]	= { 0x0B, 0x0D };
-static const u8 TMP401_TEMP_HIGH_LIMIT_LSB[2]		= { 0x16, 0x13 };
-/* These are called the THERM limit / hysteresis / mask in the datasheet */
-static const u8 TMP401_TEMP_CRIT_LIMIT[2]		= { 0x20, 0x19 };
-
-static const u8 TMP411_TEMP_LOWEST_MSB[2]		= { 0x30, 0x34 };
-static const u8 TMP411_TEMP_LOWEST_LSB[2]		= { 0x31, 0x35 };
-static const u8 TMP411_TEMP_HIGHEST_MSB[2]		= { 0x32, 0x36 };
-static const u8 TMP411_TEMP_HIGHEST_LSB[2]		= { 0x33, 0x37 };
+static const u8 TMP401_TEMP_MSB_READ[6][2] = {
+	{ 0x00, 0x01 },	/* temp */
+	{ 0x06, 0x08 },	/* low limit */
+	{ 0x05, 0x07 },	/* high limit */
+	{ 0x20, 0x19 },	/* therm (crit) limit */
+	{ 0x30, 0x34 },	/* lowest */
+	{ 0x32, 0x36 },	/* highest */
+};
+
+static const u8 TMP401_TEMP_MSB_WRITE[6][2] = {
+	{ 0, 0 },	/* temp (unused) */
+	{ 0x0C, 0x0E },	/* low limit */
+	{ 0x0B, 0x0D },	/* high limit */
+	{ 0x20, 0x19 },	/* therm (crit) limit */
+	{ 0x30, 0x34 },	/* lowest */
+	{ 0x32, 0x36 },	/* highest */
+};
+
+static const u8 TMP401_TEMP_LSB[6][2] = {
+	{ 0x15, 0x10 },	/* temp */
+	{ 0x17, 0x14 },	/* low limit */
+	{ 0x16, 0x13 },	/* high limit */
+	{ 0, 0 },	/* therm (crit) limit (unused) */
+	{ 0x31, 0x35 },	/* lowest */
+	{ 0x33, 0x37 },	/* highest */
+};
 
 /* Flags */
 #define TMP401_CONFIG_RANGE			BIT(2)
@@ -119,13 +130,8 @@ struct tmp401_data {
 	/* register values */
 	u8 status;
 	u8 config;
-	u16 temp[2];
-	u16 temp_low[2];
-	u16 temp_high[2];
-	u8 temp_crit[2];
+	u16 temp[6][2];
 	u8 temp_crit_hyst;
-	u16 temp_lowest[2];
-	u16 temp_highest[2];
 };
 
 /*
@@ -139,31 +145,10 @@ static int tmp401_register_to_temp(u16 reg, u8 config)
 	if (config & TMP401_CONFIG_RANGE)
 		temp -= 64 * 256;
 
-	return (temp * 625 + 80) / 160;
-}
-
-static u16 tmp401_temp_to_register(long temp, u8 config)
-{
-	if (config & TMP401_CONFIG_RANGE) {
-		temp = clamp_val(temp, -64000, 191000);
-		temp += 64000;
-	} else
-		temp = clamp_val(temp, 0, 127000);
-
-	return (temp * 160 + 312) / 625;
+	return DIV_ROUND_CLOSEST(temp * 125, 32);
 }
 
-static int tmp401_crit_register_to_temp(u8 reg, u8 config)
-{
-	int temp = reg;
-
-	if (config & TMP401_CONFIG_RANGE)
-		temp -= 64;
-
-	return temp * 1000;
-}
-
-static u8 tmp401_crit_temp_to_register(long temp, u8 config)
+static u16 tmp401_temp_to_register(long temp, u8 config, int zbits)
 {
 	if (config & TMP401_CONFIG_RANGE) {
 		temp = clamp_val(temp, -64000, 191000);
@@ -171,113 +156,93 @@ static u8 tmp401_crit_temp_to_register(long temp, u8 config)
 	} else
 		temp = clamp_val(temp, 0, 127000);
 
-	return (temp + 500) / 1000;
+	return DIV_ROUND_CLOSEST(temp * (1 << (8 - zbits)), 1000) << zbits;
 }
 
-static struct tmp401_data *tmp401_update_device_reg16(
-	struct i2c_client *client, struct tmp401_data *data)
+static int tmp401_update_device_reg16(struct i2c_client *client,
+				      struct tmp401_data *data)
 {
-	int i;
-
-	for (i = 0; i < 2; i++) {
-		/*
-		 * High byte must be read first immediately followed
-		 * by the low byte
-		 */
-		data->temp[i] = i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_MSB[i]) << 8;
-		data->temp[i] |= i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_LSB[i]);
-		data->temp_low[i] = i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_LOW_LIMIT_MSB_READ[i]) << 8;
-		data->temp_low[i] |= i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_LOW_LIMIT_LSB[i]);
-		data->temp_high[i] = i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_HIGH_LIMIT_MSB_READ[i]) << 8;
-		data->temp_high[i] |= i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_HIGH_LIMIT_LSB[i]);
-		data->temp_crit[i] = i2c_smbus_read_byte_data(client,
-			TMP401_TEMP_CRIT_LIMIT[i]);
-
-		if (data->kind = tmp411) {
-			data->temp_lowest[i] = i2c_smbus_read_byte_data(client,
-				TMP411_TEMP_LOWEST_MSB[i]) << 8;
-			data->temp_lowest[i] |= i2c_smbus_read_byte_data(
-				client, TMP411_TEMP_LOWEST_LSB[i]);
-
-			data->temp_highest[i] = i2c_smbus_read_byte_data(
-				client, TMP411_TEMP_HIGHEST_MSB[i]) << 8;
-			data->temp_highest[i] |= i2c_smbus_read_byte_data(
-				client, TMP411_TEMP_HIGHEST_LSB[i]);
+	int i, j, val;
+	int num_regs = data->kind = tmp411 ? 6 : 4;
+
+	for (i = 0; i < 2; i++) {			/* local / rem1 */
+		for (j = 0; j < num_regs; j++) {	/* temp / low / ... */
+			/*
+			 * High byte must be read first immediately followed
+			 * by the low byte
+			 */
+			val = i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_MSB_READ[j][i]);
+			if (val < 0)
+				return val;
+			data->temp[j][i] = val << 8;
+			if (j = 3)		/* crit is msb only */
+				continue;
+			val = i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_LSB[j][i]);
+			if (val < 0)
+				return val;
+			data->temp[j][i] |= val;
 		}
 	}
-	return data;
+	return 0;
 }
 
 static struct tmp401_data *tmp401_update_device(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct tmp401_data *data = i2c_get_clientdata(client);
+	struct tmp401_data *ret = data;
+	int val;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		data->status = i2c_smbus_read_byte_data(client, TMP401_STATUS);
-		data->config = i2c_smbus_read_byte_data(client,
-						TMP401_CONFIG_READ);
-		tmp401_update_device_reg16(client, data);
-
-		data->temp_crit_hyst = i2c_smbus_read_byte_data(client,
-						TMP401_TEMP_CRIT_HYST);
+		val = i2c_smbus_read_byte_data(client, TMP401_STATUS);
+		if (val < 0) {
+			ret = ERR_PTR(val);
+			goto abort;
+		}
+		data->status = val;
+		val = i2c_smbus_read_byte_data(client, TMP401_CONFIG_READ);
+		if (val < 0) {
+			ret = ERR_PTR(val);
+			goto abort;
+		}
+		data->config = val;
+		val = tmp401_update_device_reg16(client, data);
+		if (val < 0) {
+			ret = ERR_PTR(val);
+			goto abort;
+		}
+		val = i2c_smbus_read_byte_data(client, TMP401_TEMP_CRIT_HYST);
+		if (val < 0) {
+			ret = ERR_PTR(val);
+			goto abort;
+		}
+		data->temp_crit_hyst = val;
 
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}
 
+abort:
 	mutex_unlock(&data->update_lock);
-
-	return data;
+	return ret;
 }
 
-static ssize_t show_temp_value(struct device *dev,
-	struct device_attribute *devattr, char *buf)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
-
-	return sprintf(buf, "%d\n",
-		tmp401_register_to_temp(data->temp[index], data->config));
-}
-
-static ssize_t show_temp_min(struct device *dev,
-	struct device_attribute *devattr, char *buf)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
-
-	return sprintf(buf, "%d\n",
-		tmp401_register_to_temp(data->temp_low[index], data->config));
-}
-
-static ssize_t show_temp_max(struct device *dev,
-	struct device_attribute *devattr, char *buf)
+static ssize_t show_temp(struct device *dev,
+			 struct device_attribute *devattr, char *buf)
 {
-	int index = to_sensor_dev_attr(devattr)->index;
+	int nr = to_sensor_dev_attr_2(devattr)->nr;
+	int index = to_sensor_dev_attr_2(devattr)->index;
 	struct tmp401_data *data = tmp401_update_device(dev);
 
-	return sprintf(buf, "%d\n",
-		tmp401_register_to_temp(data->temp_high[index], data->config));
-}
-
-static ssize_t show_temp_crit(struct device *dev,
-	struct device_attribute *devattr, char *buf)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 
 	return sprintf(buf, "%d\n",
-			tmp401_crit_register_to_temp(data->temp_crit[index],
-							data->config));
+		tmp401_register_to_temp(data->temp[nr][index], data->config));
 }
 
 static ssize_t show_temp_crit_hyst(struct device *dev,
@@ -286,122 +251,58 @@ static ssize_t show_temp_crit_hyst(struct device *dev,
 	int temp, index = to_sensor_dev_attr(devattr)->index;
 	struct tmp401_data *data = tmp401_update_device(dev);
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	mutex_lock(&data->update_lock);
-	temp = tmp401_crit_register_to_temp(data->temp_crit[index],
-						data->config);
+	temp = tmp401_register_to_temp(data->temp[3][index], data->config);
 	temp -= data->temp_crit_hyst * 1000;
 	mutex_unlock(&data->update_lock);
 
 	return sprintf(buf, "%d\n", temp);
 }
 
-static ssize_t show_temp_lowest(struct device *dev,
-	struct device_attribute *devattr, char *buf)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
-
-	return sprintf(buf, "%d\n",
-		tmp401_register_to_temp(data->temp_lowest[index],
-					data->config));
-}
-
-static ssize_t show_temp_highest(struct device *dev,
-	struct device_attribute *devattr, char *buf)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
-
-	return sprintf(buf, "%d\n",
-		tmp401_register_to_temp(data->temp_highest[index],
-					data->config));
-}
-
 static ssize_t show_status(struct device *dev,
 	struct device_attribute *devattr, char *buf)
 {
 	int mask = to_sensor_dev_attr(devattr)->index;
 	struct tmp401_data *data = tmp401_update_device(dev);
 
-	if (data->status & mask)
-		return sprintf(buf, "1\n");
-	else
-		return sprintf(buf, "0\n");
-}
-
-static ssize_t store_temp_min(struct device *dev, struct device_attribute
-	*devattr, const char *buf, size_t count)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
-	long val;
-	u16 reg;
-
-	if (kstrtol(buf, 10, &val))
-		return -EINVAL;
-
-	reg = tmp401_temp_to_register(val, data->config);
-
-	mutex_lock(&data->update_lock);
-
-	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP401_TEMP_LOW_LIMIT_MSB_WRITE[index], reg >> 8);
-	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP401_TEMP_LOW_LIMIT_LSB[index], reg & 0xFF);
-
-	data->temp_low[index] = reg;
-
-	mutex_unlock(&data->update_lock);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 
-	return count;
+	return sprintf(buf, "%d\n", !!(data->status & mask));
 }
 
-static ssize_t store_temp_max(struct device *dev, struct device_attribute
-	*devattr, const char *buf, size_t count)
+static ssize_t store_temp(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
 {
-	int index = to_sensor_dev_attr(devattr)->index;
+	int nr = to_sensor_dev_attr_2(devattr)->nr;
+	int index = to_sensor_dev_attr_2(devattr)->index;
+	struct i2c_client *client = to_i2c_client(dev);
 	struct tmp401_data *data = tmp401_update_device(dev);
 	long val;
 	u16 reg;
 
-	if (kstrtol(buf, 10, &val))
-		return -EINVAL;
-
-	reg = tmp401_temp_to_register(val, data->config);
-
-	mutex_lock(&data->update_lock);
-
-	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[index], reg >> 8);
-	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP401_TEMP_HIGH_LIMIT_LSB[index], reg & 0xFF);
-
-	data->temp_high[index] = reg;
-
-	mutex_unlock(&data->update_lock);
-
-	return count;
-}
-
-static ssize_t store_temp_crit(struct device *dev, struct device_attribute
-	*devattr, const char *buf, size_t count)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	struct tmp401_data *data = tmp401_update_device(dev);
-	long val;
-	u8 reg;
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 
 	if (kstrtol(buf, 10, &val))
 		return -EINVAL;
 
-	reg = tmp401_crit_temp_to_register(val, data->config);
+	reg = tmp401_temp_to_register(val, data->config, nr = 3 ? 8 : 4);
 
 	mutex_lock(&data->update_lock);
 
-	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP401_TEMP_CRIT_LIMIT[index], reg);
-
-	data->temp_crit[index] = reg;
+	i2c_smbus_write_byte_data(client,
+				  TMP401_TEMP_MSB_WRITE[nr][index],
+				  reg >> 8);
+	if (nr != 3) {
+		i2c_smbus_write_byte_data(client,
+					  TMP401_TEMP_LSB[nr][index],
+					  reg & 0xFF);
+	}
+	data->temp[nr][index] = reg;
 
 	mutex_unlock(&data->update_lock);
 
@@ -416,6 +317,9 @@ static ssize_t store_temp_crit_hyst(struct device *dev, struct device_attribute
 	long val;
 	u8 reg;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	if (kstrtol(buf, 10, &val))
 		return -EINVAL;
 
@@ -425,13 +329,12 @@ static ssize_t store_temp_crit_hyst(struct device *dev, struct device_attribute
 		val = clamp_val(val, 0, 127000);
 
 	mutex_lock(&data->update_lock);
-	temp = tmp401_crit_register_to_temp(data->temp_crit[index],
-						data->config);
+	temp = tmp401_register_to_temp(data->temp[3][index], data->config);
 	val = clamp_val(val, temp - 255000, temp);
 	reg = ((temp - val) + 500) / 1000;
 
-	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP401_TEMP_CRIT_HYST, reg);
+	i2c_smbus_write_byte_data(to_i2c_client(dev), TMP401_TEMP_CRIT_HYST,
+				  reg);
 
 	data->temp_crit_hyst = reg;
 
@@ -460,18 +363,18 @@ static ssize_t reset_temp_history(struct device *dev,
 		return -EINVAL;
 	}
 	i2c_smbus_write_byte_data(to_i2c_client(dev),
-		TMP411_TEMP_LOWEST_MSB[0], val);
+				  TMP401_TEMP_MSB_WRITE[5][0], val);
 
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_value, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp_min,
-			  store_temp_min, 0);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max,
-			  store_temp_max, 0);
-static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp_crit,
-			  store_temp_crit, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_min, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 1, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_max, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 2, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_crit, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 3, 0);
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO,
 			  show_temp_crit_hyst, store_temp_crit_hyst, 0);
 static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_status, NULL,
@@ -480,13 +383,13 @@ static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_status, NULL,
 			  TMP401_STATUS_LOCAL_HIGH);
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_status, NULL,
 			  TMP401_STATUS_LOCAL_CRIT);
-static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp_value, NULL, 1);
-static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp_min,
-			  store_temp_min, 1);
-static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp_max,
-			  store_temp_max, 1);
-static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp_crit,
-			  store_temp_crit, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, 0, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 1, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 2, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_crit, S_IWUSR | S_IRUGO, show_temp,
+			    store_temp, 3, 1);
 static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temp_crit_hyst,
 			  NULL, 1);
 static SENSOR_DEVICE_ATTR(temp2_fault, S_IRUGO, show_status, NULL,
@@ -532,10 +435,10 @@ static const struct attribute_group tmp401_group = {
  * minimum and maximum register reset for both the local
  * and remote channels.
  */
-static SENSOR_DEVICE_ATTR(temp1_highest, S_IRUGO, show_temp_highest, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp1_lowest, S_IRUGO, show_temp_lowest, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp2_highest, S_IRUGO, show_temp_highest, NULL, 1);
-static SENSOR_DEVICE_ATTR(temp2_lowest, S_IRUGO, show_temp_lowest, NULL, 1);
+static SENSOR_DEVICE_ATTR_2(temp1_lowest, S_IRUGO, show_temp, NULL, 4, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_highest, S_IRUGO, show_temp, NULL, 5, 0);
+static SENSOR_DEVICE_ATTR_2(temp2_lowest, S_IRUGO, show_temp, NULL, 4, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_highest, S_IRUGO, show_temp, NULL, 5, 1);
 static SENSOR_DEVICE_ATTR(temp_reset_history, S_IWUSR, NULL, reset_temp_history,
 			  0);
 
-- 
1.7.9.7


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors