[PATCH v2 0/4] hwmon: (adt7475) Add error handling for update function

[PATCH v2 0/4] hwmon: (adt7475) Add error handling for update function

[Tokunori Ikegami]

Currently the device update function does not care the I2C SMBus error.
This causes a spurious sensor warining detection by sensor application.
To prevent this issue add error handling for the update function.
Also change the show functions to return error pointer value.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org

Tokunori Ikegami (4):
  hwmon: (adt7475) Split device update function to measure and limits
  hwmon: (adt7475) Change valid parameter to bool type
  hwmon: (adt7475) Change update functions to add error handling
  hwmon: (adt7475) Change show functions to return error data correctly

 drivers/hwmon/adt7475.c | 346 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 250 insertions(+), 96 deletions(-)

-- 
2.16.1

[PATCH v2 1/4] hwmon: (adt7475) Split device update function to measure and limits

[Tokunori Ikegami]

The function has the measure update part and limits and settings part.
And those parts can be split so split them for a maintainability.
Also not necessary to read the limits more than once so change to update once.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
Changes since v1:
- Change to update the limits only once.

 drivers/hwmon/adt7475.c | 202 +++++++++++++++++++++++++-----------------------
 1 file changed, 107 insertions(+), 95 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 9ef84998c7f3..42f48c6843c5 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -194,7 +194,7 @@ struct adt7475_data {
 	struct mutex lock;
 
 	unsigned long measure_updated;
-	unsigned long limits_updated;
+	bool limits_updated;
 	char valid;
 
 	u8 config4;
@@ -1658,123 +1658,135 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
 	}
 }
 
-static struct adt7475_data *adt7475_update_device(struct device *dev)
+static void adt7475_update_measure(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adt7475_data *data = i2c_get_clientdata(client);
 	u16 ext;
 	int i;
 
-	mutex_lock(&data->lock);
+	data->alarms = adt7475_read(REG_STATUS2) << 8;
+	data->alarms |= adt7475_read(REG_STATUS1);
+
+	ext = (adt7475_read(REG_EXTEND2) << 8) |
+		adt7475_read(REG_EXTEND1);
+	for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
+		if (!(data->has_voltage & (1 << i)))
+			continue;
+		data->voltage[INPUT][i] =
+			(adt7475_read(VOLTAGE_REG(i)) << 2) |
+			((ext >> (i * 2)) & 3);
+	}
 
-	/* Measurement values update every 2 seconds */
-	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
-	    !data->valid) {
-		data->alarms = adt7475_read(REG_STATUS2) << 8;
-		data->alarms |= adt7475_read(REG_STATUS1);
-
-		ext = (adt7475_read(REG_EXTEND2) << 8) |
-			adt7475_read(REG_EXTEND1);
-		for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
-			if (!(data->has_voltage & (1 << i)))
-				continue;
-			data->voltage[INPUT][i] =
-				(adt7475_read(VOLTAGE_REG(i)) << 2) |
-				((ext >> (i * 2)) & 3);
-		}
+	for (i = 0; i < ADT7475_TEMP_COUNT; i++)
+		data->temp[INPUT][i] =
+			(adt7475_read(TEMP_REG(i)) << 2) |
+			((ext >> ((i + 5) * 2)) & 3);
 
-		for (i = 0; i < ADT7475_TEMP_COUNT; i++)
-			data->temp[INPUT][i] =
-				(adt7475_read(TEMP_REG(i)) << 2) |
-				((ext >> ((i + 5) * 2)) & 3);
+	if (data->has_voltage & (1 << 5)) {
+		data->alarms |= adt7475_read(REG_STATUS4) << 24;
+		ext = adt7475_read(REG_EXTEND3);
+		data->voltage[INPUT][5] = adt7475_read(REG_VTT) << 2 |
+			((ext >> 4) & 3);
+	}
 
-		if (data->has_voltage & (1 << 5)) {
-			data->alarms |= adt7475_read(REG_STATUS4) << 24;
-			ext = adt7475_read(REG_EXTEND3);
-			data->voltage[INPUT][5] = adt7475_read(REG_VTT) << 2 |
-				((ext >> 4) & 3);
-		}
+	for (i = 0; i < ADT7475_TACH_COUNT; i++) {
+		if (i == 3 && !data->has_fan4)
+			continue;
+		data->tach[INPUT][i] =
+			adt7475_read_word(client, TACH_REG(i));
+	}
 
-		for (i = 0; i < ADT7475_TACH_COUNT; i++) {
-			if (i == 3 && !data->has_fan4)
-				continue;
-			data->tach[INPUT][i] =
-				adt7475_read_word(client, TACH_REG(i));
-		}
+	/* Updated by hw when in auto mode */
+	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
+		if (i == 1 && !data->has_pwm2)
+			continue;
+		data->pwm[INPUT][i] = adt7475_read(PWM_REG(i));
+	}
 
-		/* Updated by hw when in auto mode */
-		for (i = 0; i < ADT7475_PWM_COUNT; i++) {
-			if (i == 1 && !data->has_pwm2)
-				continue;
-			data->pwm[INPUT][i] = adt7475_read(PWM_REG(i));
-		}
+	if (data->has_vid)
+		data->vid = adt7475_read(REG_VID) & 0x3f;
+}
+
+static void adt7475_update_limits(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	int i;
 
-		if (data->has_vid)
-			data->vid = adt7475_read(REG_VID) & 0x3f;
+	data->config4 = adt7475_read(REG_CONFIG4);
+	data->config5 = adt7475_read(REG_CONFIG5);
 
-		data->measure_updated = jiffies;
+	for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
+		if (!(data->has_voltage & (1 << i)))
+			continue;
+		/* Adjust values so they match the input precision */
+		data->voltage[MIN][i] =
+			adt7475_read(VOLTAGE_MIN_REG(i)) << 2;
+		data->voltage[MAX][i] =
+			adt7475_read(VOLTAGE_MAX_REG(i)) << 2;
 	}
 
-	/* Limits and settings, should never change update every 60 seconds */
-	if (time_after(jiffies, data->limits_updated + HZ * 60) ||
-	    !data->valid) {
-		data->config4 = adt7475_read(REG_CONFIG4);
-		data->config5 = adt7475_read(REG_CONFIG5);
-
-		for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
-			if (!(data->has_voltage & (1 << i)))
-				continue;
-			/* Adjust values so they match the input precision */
-			data->voltage[MIN][i] =
-				adt7475_read(VOLTAGE_MIN_REG(i)) << 2;
-			data->voltage[MAX][i] =
-				adt7475_read(VOLTAGE_MAX_REG(i)) << 2;
-		}
+	if (data->has_voltage & (1 << 5)) {
+		data->voltage[MIN][5] = adt7475_read(REG_VTT_MIN) << 2;
+		data->voltage[MAX][5] = adt7475_read(REG_VTT_MAX) << 2;
+	}
 
-		if (data->has_voltage & (1 << 5)) {
-			data->voltage[MIN][5] = adt7475_read(REG_VTT_MIN) << 2;
-			data->voltage[MAX][5] = adt7475_read(REG_VTT_MAX) << 2;
-		}
+	for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
+		/* Adjust values so they match the input precision */
+		data->temp[MIN][i] =
+			adt7475_read(TEMP_MIN_REG(i)) << 2;
+		data->temp[MAX][i] =
+			adt7475_read(TEMP_MAX_REG(i)) << 2;
+		data->temp[AUTOMIN][i] =
+			adt7475_read(TEMP_TMIN_REG(i)) << 2;
+		data->temp[THERM][i] =
+			adt7475_read(TEMP_THERM_REG(i)) << 2;
+		data->temp[OFFSET][i] =
+			adt7475_read(TEMP_OFFSET_REG(i));
+	}
+	adt7475_read_hystersis(client);
 
-		for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
-			/* Adjust values so they match the input precision */
-			data->temp[MIN][i] =
-				adt7475_read(TEMP_MIN_REG(i)) << 2;
-			data->temp[MAX][i] =
-				adt7475_read(TEMP_MAX_REG(i)) << 2;
-			data->temp[AUTOMIN][i] =
-				adt7475_read(TEMP_TMIN_REG(i)) << 2;
-			data->temp[THERM][i] =
-				adt7475_read(TEMP_THERM_REG(i)) << 2;
-			data->temp[OFFSET][i] =
-				adt7475_read(TEMP_OFFSET_REG(i));
-		}
-		adt7475_read_hystersis(client);
+	for (i = 0; i < ADT7475_TACH_COUNT; i++) {
+		if (i == 3 && !data->has_fan4)
+			continue;
+		data->tach[MIN][i] =
+			adt7475_read_word(client, TACH_MIN_REG(i));
+	}
 
-		for (i = 0; i < ADT7475_TACH_COUNT; i++) {
-			if (i == 3 && !data->has_fan4)
-				continue;
-			data->tach[MIN][i] =
-				adt7475_read_word(client, TACH_MIN_REG(i));
-		}
+	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
+		if (i == 1 && !data->has_pwm2)
+			continue;
+		data->pwm[MAX][i] = adt7475_read(PWM_MAX_REG(i));
+		data->pwm[MIN][i] = adt7475_read(PWM_MIN_REG(i));
+		/* Set the channel and control information */
+		adt7475_read_pwm(client, i);
+	}
 
-		for (i = 0; i < ADT7475_PWM_COUNT; i++) {
-			if (i == 1 && !data->has_pwm2)
-				continue;
-			data->pwm[MAX][i] = adt7475_read(PWM_MAX_REG(i));
-			data->pwm[MIN][i] = adt7475_read(PWM_MIN_REG(i));
-			/* Set the channel and control information */
-			adt7475_read_pwm(client, i);
-		}
+	data->range[0] = adt7475_read(TEMP_TRANGE_REG(0));
+	data->range[1] = adt7475_read(TEMP_TRANGE_REG(1));
+	data->range[2] = adt7475_read(TEMP_TRANGE_REG(2));
+}
+
+static struct adt7475_data *adt7475_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7475_data *data = i2c_get_clientdata(client);
 
-		data->range[0] = adt7475_read(TEMP_TRANGE_REG(0));
-		data->range[1] = adt7475_read(TEMP_TRANGE_REG(1));
-		data->range[2] = adt7475_read(TEMP_TRANGE_REG(2));
+	mutex_lock(&data->lock);
 
-		data->limits_updated = jiffies;
-		data->valid = 1;
+	/* Measurement values update every 2 seconds */
+	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
+	    !data->valid) {
+		adt7475_update_measure(dev);
+		data->measure_updated = jiffies;
 	}
 
+	/* Limits and settings, should never change update more than once */
+	if (!data->limits_updated) {
+		adt7475_update_limits(dev);
+		data->limits_updated = true;
+	}
 	mutex_unlock(&data->lock);
 
 	return data;
-- 
2.16.1

[PATCH v2 2/4] hwmon: (adt7475) Change valid parameter to bool type

[Tokunori Ikegami]

Currently the valid paramter is char type but the type is not required.
So change the parameter to boot type correctly.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
Changes since v1:
- Change to rebase on the v2 patch 1.

 drivers/hwmon/adt7475.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 42f48c6843c5..680fc9cecb7b 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -195,7 +195,7 @@ struct adt7475_data {
 
 	unsigned long measure_updated;
 	bool limits_updated;
-	char valid;
+	bool valid;
 
 	u8 config4;
 	u8 config5;
@@ -1780,6 +1780,7 @@ static struct adt7475_data *adt7475_update_device(struct device *dev)
 	    !data->valid) {
 		adt7475_update_measure(dev);
 		data->measure_updated = jiffies;
+		data->valid = true;
 	}
 
 	/* Limits and settings, should never change update more than once */
-- 
2.16.1

[PATCH v2 3/4] hwmon: (adt7475) Change update functions to add error handling

[Tokunori Ikegami]

I2C SMBus is sometimes possible to return error codes.
And at the error case the measurement values are updated incorrectly.
The sensor application sends warning log message and SNMP trap.
To prevent this add error handling into the update functions.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
Changes since v1:
- Move the changes in adt7475_update_device() to patch 4.

 drivers/hwmon/adt7475.c | 187 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 145 insertions(+), 42 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 680fc9cecb7b..74fe5424c394 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1658,114 +1658,217 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
 	}
 }
 
-static void adt7475_update_measure(struct device *dev)
+static int adt7475_update_measure(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adt7475_data *data = i2c_get_clientdata(client);
 	u16 ext;
 	int i;
+	int ret;
 
-	data->alarms = adt7475_read(REG_STATUS2) << 8;
-	data->alarms |= adt7475_read(REG_STATUS1);
+	ret = adt7475_read(REG_STATUS2);
+	if (ret < 0)
+		return ret;
+	data->alarms = ret << 8;
+
+	ret = adt7475_read(REG_STATUS1);
+	if (ret < 0)
+		return ret;
+	data->alarms |= ret;
+
+	ret = adt7475_read(REG_EXTEND2);
+	if (ret < 0)
+		return ret;
+
+	ext = (ret << 8);
+
+	ret = adt7475_read(REG_EXTEND1);
+	if (ret < 0)
+		return ret;
+
+	ext |= ret;
 
-	ext = (adt7475_read(REG_EXTEND2) << 8) |
-		adt7475_read(REG_EXTEND1);
 	for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
 		if (!(data->has_voltage & (1 << i)))
 			continue;
+		ret = adt7475_read(VOLTAGE_REG(i));
+		if (ret < 0)
+			return ret;
 		data->voltage[INPUT][i] =
-			(adt7475_read(VOLTAGE_REG(i)) << 2) |
+			(ret << 2) |
 			((ext >> (i * 2)) & 3);
 	}
 
-	for (i = 0; i < ADT7475_TEMP_COUNT; i++)
+	for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
+		ret = adt7475_read(TEMP_REG(i));
+		if (ret < 0)
+			return ret;
 		data->temp[INPUT][i] =
-			(adt7475_read(TEMP_REG(i)) << 2) |
+			(ret << 2) |
 			((ext >> ((i + 5) * 2)) & 3);
+	}
 
 	if (data->has_voltage & (1 << 5)) {
-		data->alarms |= adt7475_read(REG_STATUS4) << 24;
-		ext = adt7475_read(REG_EXTEND3);
-		data->voltage[INPUT][5] = adt7475_read(REG_VTT) << 2 |
+		ret = adt7475_read(REG_STATUS4);
+		if (ret < 0)
+			return ret;
+		data->alarms |= ret << 24;
+
+		ret = adt7475_read(REG_EXTEND3);
+		if (ret < 0)
+			return ret;
+		ext = ret;
+
+		ret = adt7475_read(REG_VTT);
+		if (ret < 0)
+			return ret;
+		data->voltage[INPUT][5] = ret << 2 |
 			((ext >> 4) & 3);
 	}
 
 	for (i = 0; i < ADT7475_TACH_COUNT; i++) {
 		if (i == 3 && !data->has_fan4)
 			continue;
-		data->tach[INPUT][i] =
-			adt7475_read_word(client, TACH_REG(i));
+		ret = adt7475_read_word(client, TACH_REG(i));
+		if (ret < 0)
+			return ret;
+		data->tach[INPUT][i] = ret;
 	}
 
 	/* Updated by hw when in auto mode */
 	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
 		if (i == 1 && !data->has_pwm2)
 			continue;
-		data->pwm[INPUT][i] = adt7475_read(PWM_REG(i));
+		ret = adt7475_read(PWM_REG(i));
+		if (ret < 0)
+			return ret;
+		data->pwm[INPUT][i] = ret;
 	}
 
-	if (data->has_vid)
-		data->vid = adt7475_read(REG_VID) & 0x3f;
+	if (data->has_vid) {
+		ret = adt7475_read(REG_VID);
+		if (ret < 0)
+			return ret;
+		data->vid = ret & 0x3f;
+	}
+
+	return 0;
 }
 
-static void adt7475_update_limits(struct device *dev)
+static int adt7475_update_limits(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adt7475_data *data = i2c_get_clientdata(client);
 	int i;
+	int ret;
 
-	data->config4 = adt7475_read(REG_CONFIG4);
-	data->config5 = adt7475_read(REG_CONFIG5);
+	ret = adt7475_read(REG_CONFIG4);
+	if (ret < 0)
+		return ret;
+	data->config4 = ret;
+
+	ret = adt7475_read(REG_CONFIG5);
+	if (ret < 0)
+		return ret;
+	data->config5 = ret;
 
 	for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
 		if (!(data->has_voltage & (1 << i)))
 			continue;
 		/* Adjust values so they match the input precision */
-		data->voltage[MIN][i] =
-			adt7475_read(VOLTAGE_MIN_REG(i)) << 2;
-		data->voltage[MAX][i] =
-			adt7475_read(VOLTAGE_MAX_REG(i)) << 2;
+		ret = adt7475_read(VOLTAGE_MIN_REG(i));
+		if (ret < 0)
+			return ret;
+		data->voltage[MIN][i] = ret << 2;
+
+		ret = adt7475_read(VOLTAGE_MAX_REG(i));
+		if (ret < 0)
+			return ret;
+		data->voltage[MAX][i] = ret << 2;
 	}
 
 	if (data->has_voltage & (1 << 5)) {
-		data->voltage[MIN][5] = adt7475_read(REG_VTT_MIN) << 2;
-		data->voltage[MAX][5] = adt7475_read(REG_VTT_MAX) << 2;
+		ret = adt7475_read(REG_VTT_MIN);
+		if (ret < 0)
+			return ret;
+		data->voltage[MIN][5] = ret << 2;
+
+		ret = adt7475_read(REG_VTT_MAX);
+		if (ret < 0)
+			return ret;
+		data->voltage[MAX][5] = ret << 2;
 	}
 
 	for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
 		/* Adjust values so they match the input precision */
-		data->temp[MIN][i] =
-			adt7475_read(TEMP_MIN_REG(i)) << 2;
-		data->temp[MAX][i] =
-			adt7475_read(TEMP_MAX_REG(i)) << 2;
-		data->temp[AUTOMIN][i] =
-			adt7475_read(TEMP_TMIN_REG(i)) << 2;
-		data->temp[THERM][i] =
-			adt7475_read(TEMP_THERM_REG(i)) << 2;
-		data->temp[OFFSET][i] =
-			adt7475_read(TEMP_OFFSET_REG(i));
+		ret = adt7475_read(TEMP_MIN_REG(i));
+		if (ret < 0)
+			return ret;
+		data->temp[MIN][i] = ret << 2;
+
+		ret = adt7475_read(TEMP_MAX_REG(i));
+		if (ret < 0)
+			return ret;
+		data->temp[MAX][i] = ret << 2;
+
+		ret = adt7475_read(TEMP_TMIN_REG(i));
+		if (ret < 0)
+			return ret;
+		data->temp[AUTOMIN][i] = ret << 2;
+
+		ret = adt7475_read(TEMP_THERM_REG(i));
+		if (ret < 0)
+			return ret;
+		data->temp[THERM][i] = ret << 2;
+
+		ret = adt7475_read(TEMP_OFFSET_REG(i));
+		if (ret < 0)
+			return ret;
+		data->temp[OFFSET][i] = ret;
 	}
 	adt7475_read_hystersis(client);
 
 	for (i = 0; i < ADT7475_TACH_COUNT; i++) {
 		if (i == 3 && !data->has_fan4)
 			continue;
-		data->tach[MIN][i] =
-			adt7475_read_word(client, TACH_MIN_REG(i));
+		ret = adt7475_read_word(client, TACH_MIN_REG(i));
+		if (ret < 0)
+			return ret;
+		data->tach[MIN][i] = ret;
 	}
 
 	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
 		if (i == 1 && !data->has_pwm2)
 			continue;
-		data->pwm[MAX][i] = adt7475_read(PWM_MAX_REG(i));
-		data->pwm[MIN][i] = adt7475_read(PWM_MIN_REG(i));
+		ret = adt7475_read(PWM_MAX_REG(i));
+		if (ret < 0)
+			return ret;
+		data->pwm[MAX][i] = ret;
+
+		ret = adt7475_read(PWM_MIN_REG(i));
+		if (ret < 0)
+			return ret;
+		data->pwm[MIN][i] = ret;
 		/* Set the channel and control information */
 		adt7475_read_pwm(client, i);
 	}
 
-	data->range[0] = adt7475_read(TEMP_TRANGE_REG(0));
-	data->range[1] = adt7475_read(TEMP_TRANGE_REG(1));
-	data->range[2] = adt7475_read(TEMP_TRANGE_REG(2));
+	ret = adt7475_read(TEMP_TRANGE_REG(0));
+	if (ret < 0)
+		return ret;
+	data->range[0] = ret;
+
+	ret = adt7475_read(TEMP_TRANGE_REG(1));
+	if (ret < 0)
+		return ret;
+	data->range[1] = ret;
+
+	ret = adt7475_read(TEMP_TRANGE_REG(2));
+	if (ret < 0)
+		return ret;
+	data->range[2] = ret;
+
+	return 0;
 }
 
 static struct adt7475_data *adt7475_update_device(struct device *dev)
-- 
2.16.1

[PATCH v2 4/4] hwmon: (adt7475) Change show functions to return error data correctly

[Tokunori Ikegami]

The update device function was changed to return error pointer value.
So change the show functions using the update function to return error.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
Changes since v1:
- Move the changes in adt7475_update_device() from patch 3.
- Change to rebase on the v2 patch 1.

 drivers/hwmon/adt7475.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 74fe5424c394..bdafc213fdd5 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -326,6 +326,9 @@ static ssize_t show_voltage(struct device *dev, struct device_attribute *attr,
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 	unsigned short val;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	switch (sattr->nr) {
 	case ALARM:
 		return sprintf(buf, "%d\n",
@@ -381,6 +384,9 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 	int out;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	switch (sattr->nr) {
 	case HYSTERSIS:
 		mutex_lock(&data->lock);
@@ -625,6 +631,9 @@ static ssize_t show_point2(struct device *dev, struct device_attribute *attr,
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 	int out, val;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	mutex_lock(&data->lock);
 	out = (data->range[sattr->index] >> 4) & 0x0F;
 	val = reg2temp(data, data->temp[AUTOMIN][sattr->index]);
@@ -683,6 +692,9 @@ static ssize_t show_tach(struct device *dev, struct device_attribute *attr,
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 	int out;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	if (sattr->nr == ALARM)
 		out = (data->alarms >> (sattr->index + 10)) & 1;
 	else
@@ -720,6 +732,9 @@ static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
 	struct adt7475_data *data = adt7475_update_device(dev);
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", data->pwm[sattr->nr][sattr->index]);
 }
 
@@ -729,6 +744,9 @@ static ssize_t show_pwmchan(struct device *dev, struct device_attribute *attr,
 	struct adt7475_data *data = adt7475_update_device(dev);
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", data->pwmchan[sattr->index]);
 }
 
@@ -738,6 +756,9 @@ static ssize_t show_pwmctrl(struct device *dev, struct device_attribute *attr,
 	struct adt7475_data *data = adt7475_update_device(dev);
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", data->pwmctl[sattr->index]);
 }
 
@@ -945,6 +966,9 @@ static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
 	int i = clamp_val(data->range[sattr->index] & 0xf, 0,
 			  ARRAY_SIZE(pwmfreq_table) - 1);
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", pwmfreq_table[i]);
 }
 
@@ -1035,6 +1059,10 @@ static ssize_t cpu0_vid_show(struct device *dev,
 			     struct device_attribute *devattr, char *buf)
 {
 	struct adt7475_data *data = adt7475_update_device(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
 }
 
@@ -1875,20 +1903,30 @@ static struct adt7475_data *adt7475_update_device(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct adt7475_data *data = i2c_get_clientdata(client);
+	int ret;
 
 	mutex_lock(&data->lock);
 
 	/* Measurement values update every 2 seconds */
 	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
 	    !data->valid) {
-		adt7475_update_measure(dev);
+		ret = adt7475_update_measure(dev);
+		if (ret < 0) {
+			data->valid = false;
+			mutex_unlock(&data->lock);
+			return ERR_PTR(ret);
+		}
 		data->measure_updated = jiffies;
 		data->valid = true;
 	}
 
 	/* Limits and settings, should never change update more than once */
 	if (!data->limits_updated) {
-		adt7475_update_limits(dev);
+		ret = adt7475_update_limits(dev);
+		if (ret < 0) {
+			mutex_unlock(&data->lock);
+			return ERR_PTR(ret);
+		}
 		data->limits_updated = true;
 	}
 	mutex_unlock(&data->lock);
-- 
2.16.1

Re: [PATCH v2 1/4] hwmon: (adt7475) Split device update function to measure and limits

[Guenter Roeck]

On Wed, Aug 08, 2018 at 12:19:06AM +0900, Tokunori Ikegami wrote:
> The function has the measure update part and limits and settings part.
> And those parts can be split so split them for a maintainability.
> Also not necessary to read the limits more than once so change to update once.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: linux-hwmon@vger.kernel.org
> ---
> Changes since v1:
> - Change to update the limits only once.
> 
>  drivers/hwmon/adt7475.c | 202 +++++++++++++++++++++++++-----------------------
>  1 file changed, 107 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index 9ef84998c7f3..42f48c6843c5 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -194,7 +194,7 @@ struct adt7475_data {
>  	struct mutex lock;
>  
>  	unsigned long measure_updated;
> -	unsigned long limits_updated;
> +	bool limits_updated;
>  	char valid;
>  
>  	u8 config4;
> @@ -1658,123 +1658,135 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
>  	}
>  }
>  
> -static struct adt7475_data *adt7475_update_device(struct device *dev)
> +static void adt7475_update_measure(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct adt7475_data *data = i2c_get_clientdata(client);
>  	u16 ext;
>  	int i;
>  
> -	mutex_lock(&data->lock);
> +	data->alarms = adt7475_read(REG_STATUS2) << 8;
> +	data->alarms |= adt7475_read(REG_STATUS1);
> +
> +	ext = (adt7475_read(REG_EXTEND2) << 8) |
> +		adt7475_read(REG_EXTEND1);
> +	for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
> +		if (!(data->has_voltage & (1 << i)))
> +			continue;
> +		data->voltage[INPUT][i] =
> +			(adt7475_read(VOLTAGE_REG(i)) << 2) |
> +			((ext >> (i * 2)) & 3);
> +	}
>  
> -	/* Measurement values update every 2 seconds */
> -	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
> -	    !data->valid) {
> -		data->alarms = adt7475_read(REG_STATUS2) << 8;
> -		data->alarms |= adt7475_read(REG_STATUS1);
> -
> -		ext = (adt7475_read(REG_EXTEND2) << 8) |
> -			adt7475_read(REG_EXTEND1);
> -		for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
> -			if (!(data->has_voltage & (1 << i)))
> -				continue;
> -			data->voltage[INPUT][i] =
> -				(adt7475_read(VOLTAGE_REG(i)) << 2) |
> -				((ext >> (i * 2)) & 3);
> -		}
> +	for (i = 0; i < ADT7475_TEMP_COUNT; i++)
> +		data->temp[INPUT][i] =
> +			(adt7475_read(TEMP_REG(i)) << 2) |
> +			((ext >> ((i + 5) * 2)) & 3);
>  
> -		for (i = 0; i < ADT7475_TEMP_COUNT; i++)
> -			data->temp[INPUT][i] =
> -				(adt7475_read(TEMP_REG(i)) << 2) |
> -				((ext >> ((i + 5) * 2)) & 3);
> +	if (data->has_voltage & (1 << 5)) {
> +		data->alarms |= adt7475_read(REG_STATUS4) << 24;
> +		ext = adt7475_read(REG_EXTEND3);
> +		data->voltage[INPUT][5] = adt7475_read(REG_VTT) << 2 |
> +			((ext >> 4) & 3);
> +	}
>  
> -		if (data->has_voltage & (1 << 5)) {
> -			data->alarms |= adt7475_read(REG_STATUS4) << 24;
> -			ext = adt7475_read(REG_EXTEND3);
> -			data->voltage[INPUT][5] = adt7475_read(REG_VTT) << 2 |
> -				((ext >> 4) & 3);
> -		}
> +	for (i = 0; i < ADT7475_TACH_COUNT; i++) {
> +		if (i == 3 && !data->has_fan4)
> +			continue;
> +		data->tach[INPUT][i] =
> +			adt7475_read_word(client, TACH_REG(i));
> +	}
>  
> -		for (i = 0; i < ADT7475_TACH_COUNT; i++) {
> -			if (i == 3 && !data->has_fan4)
> -				continue;
> -			data->tach[INPUT][i] =
> -				adt7475_read_word(client, TACH_REG(i));
> -		}
> +	/* Updated by hw when in auto mode */
> +	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
> +		if (i == 1 && !data->has_pwm2)
> +			continue;
> +		data->pwm[INPUT][i] = adt7475_read(PWM_REG(i));
> +	}
>  
> -		/* Updated by hw when in auto mode */
> -		for (i = 0; i < ADT7475_PWM_COUNT; i++) {
> -			if (i == 1 && !data->has_pwm2)
> -				continue;
> -			data->pwm[INPUT][i] = adt7475_read(PWM_REG(i));
> -		}
> +	if (data->has_vid)
> +		data->vid = adt7475_read(REG_VID) & 0x3f;
> +}
> +
> +static void adt7475_update_limits(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7475_data *data = i2c_get_clientdata(client);
> +	int i;
>  
> -		if (data->has_vid)
> -			data->vid = adt7475_read(REG_VID) & 0x3f;
> +	data->config4 = adt7475_read(REG_CONFIG4);
> +	data->config5 = adt7475_read(REG_CONFIG5);
>  
> -		data->measure_updated = jiffies;
> +	for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
> +		if (!(data->has_voltage & (1 << i)))
> +			continue;
> +		/* Adjust values so they match the input precision */
> +		data->voltage[MIN][i] =
> +			adt7475_read(VOLTAGE_MIN_REG(i)) << 2;
> +		data->voltage[MAX][i] =
> +			adt7475_read(VOLTAGE_MAX_REG(i)) << 2;
>  	}
>  
> -	/* Limits and settings, should never change update every 60 seconds */
> -	if (time_after(jiffies, data->limits_updated + HZ * 60) ||
> -	    !data->valid) {
> -		data->config4 = adt7475_read(REG_CONFIG4);
> -		data->config5 = adt7475_read(REG_CONFIG5);
> -
> -		for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) {
> -			if (!(data->has_voltage & (1 << i)))
> -				continue;
> -			/* Adjust values so they match the input precision */
> -			data->voltage[MIN][i] =
> -				adt7475_read(VOLTAGE_MIN_REG(i)) << 2;
> -			data->voltage[MAX][i] =
> -				adt7475_read(VOLTAGE_MAX_REG(i)) << 2;
> -		}
> +	if (data->has_voltage & (1 << 5)) {
> +		data->voltage[MIN][5] = adt7475_read(REG_VTT_MIN) << 2;
> +		data->voltage[MAX][5] = adt7475_read(REG_VTT_MAX) << 2;
> +	}
>  
> -		if (data->has_voltage & (1 << 5)) {
> -			data->voltage[MIN][5] = adt7475_read(REG_VTT_MIN) << 2;
> -			data->voltage[MAX][5] = adt7475_read(REG_VTT_MAX) << 2;
> -		}
> +	for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
> +		/* Adjust values so they match the input precision */
> +		data->temp[MIN][i] =
> +			adt7475_read(TEMP_MIN_REG(i)) << 2;
> +		data->temp[MAX][i] =
> +			adt7475_read(TEMP_MAX_REG(i)) << 2;
> +		data->temp[AUTOMIN][i] =
> +			adt7475_read(TEMP_TMIN_REG(i)) << 2;
> +		data->temp[THERM][i] =
> +			adt7475_read(TEMP_THERM_REG(i)) << 2;
> +		data->temp[OFFSET][i] =
> +			adt7475_read(TEMP_OFFSET_REG(i));
> +	}
> +	adt7475_read_hystersis(client);
>  
> -		for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
> -			/* Adjust values so they match the input precision */
> -			data->temp[MIN][i] =
> -				adt7475_read(TEMP_MIN_REG(i)) << 2;
> -			data->temp[MAX][i] =
> -				adt7475_read(TEMP_MAX_REG(i)) << 2;
> -			data->temp[AUTOMIN][i] =
> -				adt7475_read(TEMP_TMIN_REG(i)) << 2;
> -			data->temp[THERM][i] =
> -				adt7475_read(TEMP_THERM_REG(i)) << 2;
> -			data->temp[OFFSET][i] =
> -				adt7475_read(TEMP_OFFSET_REG(i));
> -		}
> -		adt7475_read_hystersis(client);
> +	for (i = 0; i < ADT7475_TACH_COUNT; i++) {
> +		if (i == 3 && !data->has_fan4)
> +			continue;
> +		data->tach[MIN][i] =
> +			adt7475_read_word(client, TACH_MIN_REG(i));
> +	}
>  
> -		for (i = 0; i < ADT7475_TACH_COUNT; i++) {
> -			if (i == 3 && !data->has_fan4)
> -				continue;
> -			data->tach[MIN][i] =
> -				adt7475_read_word(client, TACH_MIN_REG(i));
> -		}
> +	for (i = 0; i < ADT7475_PWM_COUNT; i++) {
> +		if (i == 1 && !data->has_pwm2)
> +			continue;
> +		data->pwm[MAX][i] = adt7475_read(PWM_MAX_REG(i));
> +		data->pwm[MIN][i] = adt7475_read(PWM_MIN_REG(i));
> +		/* Set the channel and control information */
> +		adt7475_read_pwm(client, i);
> +	}
>  
> -		for (i = 0; i < ADT7475_PWM_COUNT; i++) {
> -			if (i == 1 && !data->has_pwm2)
> -				continue;
> -			data->pwm[MAX][i] = adt7475_read(PWM_MAX_REG(i));
> -			data->pwm[MIN][i] = adt7475_read(PWM_MIN_REG(i));
> -			/* Set the channel and control information */
> -			adt7475_read_pwm(client, i);
> -		}
> +	data->range[0] = adt7475_read(TEMP_TRANGE_REG(0));
> +	data->range[1] = adt7475_read(TEMP_TRANGE_REG(1));
> +	data->range[2] = adt7475_read(TEMP_TRANGE_REG(2));
> +}
> +
> +static struct adt7475_data *adt7475_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7475_data *data = i2c_get_clientdata(client);
>  
> -		data->range[0] = adt7475_read(TEMP_TRANGE_REG(0));
> -		data->range[1] = adt7475_read(TEMP_TRANGE_REG(1));
> -		data->range[2] = adt7475_read(TEMP_TRANGE_REG(2));
> +	mutex_lock(&data->lock);
>  
> -		data->limits_updated = jiffies;
> -		data->valid = 1;
> +	/* Measurement values update every 2 seconds */
> +	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
> +	    !data->valid) {
> +		adt7475_update_measure(dev);
> +		data->measure_updated = jiffies;
>  	}
>  
> +	/* Limits and settings, should never change update more than once */
> +	if (!data->limits_updated) {
> +		adt7475_update_limits(dev);
> +		data->limits_updated = true;
> +	}

It should now be possible to do this from the probe function.

Thanks,
Guenter

>  	mutex_unlock(&data->lock);
>  
>  	return data;
> -- 
> 2.16.1
>