[PATCH 0/5] hwmon: adt7475: Add error handling for update function

[PATCH 0/5] 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 isseu add error handling for the update function.

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

Tokunori Ikegami (5):
  hwmon: adt7475: Split device update function to measure and limits
  hwmon: adt7475: Change read and write functions to return error
  hwmon: adt7475: Change to use adt7475_read macro
  hwmon: adt7475: Change to use adt7475_write macro
  hwmon: adt7475: Change update functions to add error handling

 drivers/hwmon/adt7475.c | 369 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 257 insertions(+), 112 deletions(-)

-- 
2.16.1

[PATCH 1/5] 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.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
 drivers/hwmon/adt7475.c | 214 +++++++++++++++++++++++++++---------------------
 1 file changed, 122 insertions(+), 92 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 9ef84998c7f3..234f725213f9 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1658,119 +1658,149 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
 	}
 }
 
-static struct adt7475_data *adt7475_update_device(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;
 
-	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);
+
+	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_TEMP_COUNT; i++)
-			data->temp[INPUT][i] =
-				(adt7475_read(TEMP_REG(i)) << 2) |
-				((ext >> ((i + 5) * 2)) & 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));
+	}
 
-		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);
-		}
+	/* 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));
+	}
 
-		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));
-		}
+	if (data->has_vid)
+		data->vid = adt7475_read(REG_VID) & 0x3f;
 
-		/* 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));
-		}
+	return 0;
+}
 
-		if (data->has_vid)
-			data->vid = adt7475_read(REG_VID) & 0x3f;
+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;
 
-		data->measure_updated = jiffies;
+	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;
 	}
 
-	/* 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));
+
+	return 0;
+}
 
-		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);
+	int ret;
 
+	mutex_lock(&data->lock);
+
+	/* Measurement values update every 2 seconds */
+	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
+	    !data->valid) {
+		ret = adt7475_update_measure(dev);
+		if (ret < 0) {
+			data->valid = 0;
+			mutex_unlock(&data->lock);
+			return data;
+		}
+		data->measure_updated = jiffies;
+	}
+
+	/* Limits and settings, should never change update every 60 seconds */
+	if (time_after(jiffies, data->limits_updated + HZ * 60) ||
+	    !data->valid) {
+		ret = adt7475_update_limits(dev);
+		if (ret < 0) {
+			data->valid = 0;
+			mutex_unlock(&data->lock);
+			return data;
+		}
 		data->limits_updated = jiffies;
 		data->valid = 1;
 	}
-- 
2.16.1

Re: [PATCH 1/5] hwmon: adt7475: Split device update function to measure and limits

[Guenter Roeck]

On 07/11/2018 06:04 PM, 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.
> 
> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: linux-hwmon@vger.kernel.org
> ---
>   drivers/hwmon/adt7475.c | 214 +++++++++++++++++++++++++++---------------------
>   1 file changed, 122 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index 9ef84998c7f3..234f725213f9 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -1658,119 +1658,149 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
>   	}
>   }
>   
> -static struct adt7475_data *adt7475_update_device(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;
>   
> -	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);
> +
> +	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_TEMP_COUNT; i++)
> -			data->temp[INPUT][i] =
> -				(adt7475_read(TEMP_REG(i)) << 2) |
> -				((ext >> ((i + 5) * 2)) & 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));
> +	}
>   
> -		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);
> -		}
> +	/* 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));
> +	}
>   
> -		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));
> -		}
> +	if (data->has_vid)
> +		data->vid = adt7475_read(REG_VID) & 0x3f;
>   
> -		/* 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));
> -		}
> +	return 0;
> +}
>   
> -		if (data->has_vid)
> -			data->vid = adt7475_read(REG_VID) & 0x3f;
> +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;
>   
> -		data->measure_updated = jiffies;
> +	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;
>   	}
>   
> -	/* 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));
> +
> +	return 0;
> +}
>   
> -		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);
> +	int ret;
>   
> +	mutex_lock(&data->lock);
> +
> +	/* Measurement values update every 2 seconds */
> +	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
> +	    !data->valid) {
> +		ret = adt7475_update_measure(dev);
> +		if (ret < 0) {
> +			data->valid = 0;
> +			mutex_unlock(&data->lock);
> +			return data;

Maybe I am missing something, but I don't see the point of this code.
The errors still won't be returned to the user. The only effect is that
old values will be returned, and the next time around the read operation
will hopefully succeed. That isn't error handling, it is error suppression.

> +		}
> +		data->measure_updated = jiffies;
> +	}
> +
> +	/* Limits and settings, should never change update every 60 seconds */
> +	if (time_after(jiffies, data->limits_updated + HZ * 60) ||
> +	    !data->valid) {

Is there any reason to believe that the limits have to be updated more
than once ? Does the chip have the habit of loosing its configuration ?
If it does, wouldn't it be better to _restore_ the limits to the ones
previously cached instead of silently discarding the configuration ?

> +		ret = adt7475_update_limits(dev);
> +		if (ret < 0) {
> +			data->valid = 0;
> +			mutex_unlock(&data->lock);
> +			return data;
> +		}
>   		data->limits_updated = jiffies;
>   		data->valid = 1;
>   	}
> 

RE: [PATCH 1/5] hwmon: adt7475: Split device update function to measure and limits

[IKEGAMI Tokunori]

Hi Guenter-san,

Yes you are correct.
At first I thought to change the read and write functions to repeat for the error case.
Then I confirmed the lm93 implementation and followed it basically since I thought it is better.
But as you mentioned this is not an error handling so I will change to add the original retry functions.

Regards,
Ikegami

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, July 12, 2018 11:00 AM
> To: IKEGAMI Tokunori; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 1/5] hwmon: adt7475: Split device update function
> to measure and limits
> 
> On 07/11/2018 06:04 PM, 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.
> >
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: linux-hwmon@vger.kernel.org
> > ---
> >   drivers/hwmon/adt7475.c | 214
> +++++++++++++++++++++++++++---------------------
> >   1 file changed, 122 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> > index 9ef84998c7f3..234f725213f9 100644
> > --- a/drivers/hwmon/adt7475.c
> > +++ b/drivers/hwmon/adt7475.c
> > @@ -1658,119 +1658,149 @@ static void adt7475_read_pwm(struct
> i2c_client *client, int index)
> >   	}
> >   }
> >
> > -static struct adt7475_data *adt7475_update_device(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;
> >
> > -	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);
> > +
> > +	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_TEMP_COUNT; i++)
> > -			data->temp[INPUT][i] =
> > -				(adt7475_read(TEMP_REG(i)) << 2) |
> > -				((ext >> ((i + 5) * 2)) & 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));
> > +	}
> >
> > -		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);
> > -		}
> > +	/* 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));
> > +	}
> >
> > -		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));
> > -		}
> > +	if (data->has_vid)
> > +		data->vid = adt7475_read(REG_VID) & 0x3f;
> >
> > -		/* 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));
> > -		}
> > +	return 0;
> > +}
> >
> > -		if (data->has_vid)
> > -			data->vid = adt7475_read(REG_VID) & 0x3f;
> > +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;
> >
> > -		data->measure_updated = jiffies;
> > +	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;
> >   	}
> >
> > -	/* 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));
> > +
> > +	return 0;
> > +}
> >
> > -		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);
> > +	int ret;
> >
> > +	mutex_lock(&data->lock);
> > +
> > +	/* Measurement values update every 2 seconds */
> > +	if (time_after(jiffies, data->measure_updated + HZ * 2) ||
> > +	    !data->valid) {
> > +		ret = adt7475_update_measure(dev);
> > +		if (ret < 0) {
> > +			data->valid = 0;
> > +			mutex_unlock(&data->lock);
> > +			return data;
> 
> Maybe I am missing something, but I don't see the point of this code.
> The errors still won't be returned to the user. The only effect is that
> old values will be returned, and the next time around the read operation
> will hopefully succeed. That isn't error handling, it is error
> suppression.
> 
> > +		}
> > +		data->measure_updated = jiffies;
> > +	}
> > +
> > +	/* Limits and settings, should never change update every 60
> seconds */
> > +	if (time_after(jiffies, data->limits_updated + HZ * 60) ||
> > +	    !data->valid) {
> 
> Is there any reason to believe that the limits have to be updated more
> than once ? Does the chip have the habit of loosing its configuration ?
> If it does, wouldn't it be better to _restore_ the limits to the ones
> previously cached instead of silently discarding the configuration ?
> 
> > +		ret = adt7475_update_limits(dev);
> > +		if (ret < 0) {
> > +			data->valid = 0;
> > +			mutex_unlock(&data->lock);
> > +			return data;
> > +		}
> >   		data->limits_updated = jiffies;
> >   		data->valid = 1;
> >   	}
> >

[PATCH 2/5] hwmon: adt7475: Change read and write functions to return error

[Tokunori Ikegami]

Currently adt7475_read_word and adt7475_write_word do not return error.
So change both functions to return error codes correctly.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
 drivers/hwmon/adt7475.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 234f725213f9..a40eb62ee6b1 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -303,20 +303,34 @@ static inline u16 volt2reg(int channel, long volt, u8 bypass_attn)
 	return clamp_val(reg, 0, 1023) & (0xff << 2);
 }
 
-static u16 adt7475_read_word(struct i2c_client *client, int reg)
+static int adt7475_read_word(struct i2c_client *client, int reg)
 {
-	u16 val;
+	int val, val2;
 
-	val = i2c_smbus_read_byte_data(client, reg);
-	val |= (i2c_smbus_read_byte_data(client, reg + 1) << 8);
+	val = adt7475_read(reg);
+	if (val < 0)
+		return val;
 
-	return val;
+	val2 = adt7475_read(reg + 1);
+	if (val2 < 0)
+		return val2;
+
+	return val | (val2 << 8);
 }
 
-static void adt7475_write_word(struct i2c_client *client, int reg, u16 val)
+static int adt7475_write_word(struct i2c_client *client, int reg, u16 val)
 {
-	i2c_smbus_write_byte_data(client, reg + 1, val >> 8);
-	i2c_smbus_write_byte_data(client, reg, val & 0xFF);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client, reg + 1, val >> 8);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, reg, val & 0xFF);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static ssize_t show_voltage(struct device *dev, struct device_attribute *attr,
-- 
2.16.1

[PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[Tokunori Ikegami]

It shoudl be same as with other functions to use adt7475_read.
So change to use it instead of i2c_smbus_read_byte_data.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
 drivers/hwmon/adt7475.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index a40eb62ee6b1..bad250729e99 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1012,7 +1012,7 @@ static ssize_t pwm_use_point2_pwm_at_crit_store(struct device *dev,
 		return -EINVAL;
 
 	mutex_lock(&data->lock);
-	data->config4 = i2c_smbus_read_byte_data(client, REG_CONFIG4);
+	data->config4 = adt7475_read(REG_CONFIG4);
 	if (val)
 		data->config4 |= CONFIG4_MAXDUTY;
 	else
-- 
2.16.1

Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[Guenter Roeck]

On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> It shoudl be same as with other functions to use adt7475_read.
> So change to use it instead of i2c_smbus_read_byte_data.
> 

I don't see a point in this change. Replacing a function name doesn't make
the code easier to read. If anything, you could consider dropping adt7475_read
and calling i2c_smbus_read_byte_data() directly.

Guenter

> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: linux-hwmon@vger.kernel.org
> ---
>   drivers/hwmon/adt7475.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index a40eb62ee6b1..bad250729e99 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -1012,7 +1012,7 @@ static ssize_t pwm_use_point2_pwm_at_crit_store(struct device *dev,
>   		return -EINVAL;
>   
>   	mutex_lock(&data->lock);
> -	data->config4 = i2c_smbus_read_byte_data(client, REG_CONFIG4);
> +	data->config4 = adt7475_read(REG_CONFIG4);
>   	if (val)
>   		data->config4 |= CONFIG4_MAXDUTY;
>   	else
> 

RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[IKEGAMI Tokunori]

Hi Guenter-san,

Thank you so much for your comments.
Okay now I am thinking to change the adt7475_read macro to a function to repeat for the error case.
If you have any comment about this please let me know.
Anyway I will do send v2 version patches later.

Regards,
Ikegami

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, July 12, 2018 10:50 AM
> To: IKEGAMI Tokunori; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
> 
> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> > It shoudl be same as with other functions to use adt7475_read.
> > So change to use it instead of i2c_smbus_read_byte_data.
> >
> 
> I don't see a point in this change. Replacing a function name doesn't
> make
> the code easier to read. If anything, you could consider dropping
> adt7475_read
> and calling i2c_smbus_read_byte_data() directly.
> 
> Guenter
> 
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: linux-hwmon@vger.kernel.org
> > ---
> >   drivers/hwmon/adt7475.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> > index a40eb62ee6b1..bad250729e99 100644
> > --- a/drivers/hwmon/adt7475.c
> > +++ b/drivers/hwmon/adt7475.c
> > @@ -1012,7 +1012,7 @@ static ssize_t
> pwm_use_point2_pwm_at_crit_store(struct device *dev,
> >   		return -EINVAL;
> >
> >   	mutex_lock(&data->lock);
> > -	data->config4 = i2c_smbus_read_byte_data(client,
> REG_CONFIG4);
> > +	data->config4 = adt7475_read(REG_CONFIG4);
> >   	if (val)
> >   		data->config4 |= CONFIG4_MAXDUTY;
> >   	else
> >

Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[Guenter Roeck]

On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
> Hi Guenter-san,
> 
> Thank you so much for your comments.
> Okay now I am thinking to change the adt7475_read macro to a function to repeat for the error case.
> If you have any comment about this please let me know.

Yes - we should only do this if it is known to be a chip problem.
Patching the chip driver for a board (or i2c controller) problem
is not appropriate.

Guenter

> Anyway I will do send v2 version patches later.
> 
> Regards,
> Ikegami
> 
>> -----Original Message-----
>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
>> Roeck
>> Sent: Thursday, July 12, 2018 10:50 AM
>> To: IKEGAMI Tokunori; Jean Delvare
>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
>>
>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
>>> It shoudl be same as with other functions to use adt7475_read.
>>> So change to use it instead of i2c_smbus_read_byte_data.
>>>
>>
>> I don't see a point in this change. Replacing a function name doesn't
>> make
>> the code easier to read. If anything, you could consider dropping
>> adt7475_read
>> and calling i2c_smbus_read_byte_data() directly.
>>
>> Guenter
>>
>>> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> Cc: linux-hwmon@vger.kernel.org
>>> ---
>>>    drivers/hwmon/adt7475.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
>>> index a40eb62ee6b1..bad250729e99 100644
>>> --- a/drivers/hwmon/adt7475.c
>>> +++ b/drivers/hwmon/adt7475.c
>>> @@ -1012,7 +1012,7 @@ static ssize_t
>> pwm_use_point2_pwm_at_crit_store(struct device *dev,
>>>    		return -EINVAL;
>>>
>>>    	mutex_lock(&data->lock);
>>> -	data->config4 = i2c_smbus_read_byte_data(client,
>> REG_CONFIG4);
>>> +	data->config4 = adt7475_read(REG_CONFIG4);
>>>    	if (val)
>>>    		data->config4 |= CONFIG4_MAXDUTY;
>>>    	else
>>>
> 

RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[IKEGAMI Tokunori]

Hi Guenter-san,

Thank you so much for your comments.

Oh I see and actually we have some I2C error units in our products.
The patch is for the error case mainly.
But I think that sometimes the I2C error is still caused on the normal units also actually.
I am not sure about that the normal units error behavior is caused by the board (I2C controller) or the chip ADT7476A but probably the former but not sure.
So should these patches not be applied to the adt7475 driver as you mentioned?

At first I thought that the I2C SMBus APIs return error codes but adt7475 driver does not check so it should be checked the error codes.
Is this not correct?

To make sure let me confirm above.

Regards,
Ikegami

> -----Original Message-----
> From: linux-hwmon-owner@vger.kernel.org
> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter Roeck
> Sent: Thursday, July 12, 2018 12:42 PM
> To: IKEGAMI Tokunori; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
> 
> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
> > Hi Guenter-san,
> >
> > Thank you so much for your comments.
> > Okay now I am thinking to change the adt7475_read macro to a function
> to repeat for the error case.
> > If you have any comment about this please let me know.
> 
> Yes - we should only do this if it is known to be a chip problem.
> Patching the chip driver for a board (or i2c controller) problem
> is not appropriate.
> 
> Guenter
> 
> > Anyway I will do send v2 version patches later.
> >
> > Regards,
> > Ikegami
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> >> Roeck
> >> Sent: Thursday, July 12, 2018 10:50 AM
> >> To: IKEGAMI Tokunori; Jean Delvare
> >> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> macro
> >>
> >> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> >>> It shoudl be same as with other functions to use adt7475_read.
> >>> So change to use it instead of i2c_smbus_read_byte_data.
> >>>
> >>
> >> I don't see a point in this change. Replacing a function name doesn't
> >> make
> >> the code easier to read. If anything, you could consider dropping
> >> adt7475_read
> >> and calling i2c_smbus_read_byte_data() directly.
> >>
> >> Guenter
> >>
> >>> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> >>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>> Cc: linux-hwmon@vger.kernel.org
> >>> ---
> >>>    drivers/hwmon/adt7475.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> >>> index a40eb62ee6b1..bad250729e99 100644
> >>> --- a/drivers/hwmon/adt7475.c
> >>> +++ b/drivers/hwmon/adt7475.c
> >>> @@ -1012,7 +1012,7 @@ static ssize_t
> >> pwm_use_point2_pwm_at_crit_store(struct device *dev,
> >>>    		return -EINVAL;
> >>>
> >>>    	mutex_lock(&data->lock);
> >>> -	data->config4 = i2c_smbus_read_byte_data(client,
> >> REG_CONFIG4);
> >>> +	data->config4 = adt7475_read(REG_CONFIG4);
> >>>    	if (val)
> >>>    		data->config4 |= CONFIG4_MAXDUTY;
> >>>    	else
> >>>
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[Guenter Roeck]

On 07/11/2018 09:01 PM, IKEGAMI Tokunori wrote:
> Hi Guenter-san,
> 
> Thank you so much for your comments.
> 
> Oh I see and actually we have some I2C error units in our products.
> The patch is for the error case mainly.

What is an "I2C error unit" ?

> But I think that sometimes the I2C error is still caused on the normal units also actually.
> I am not sure about that the normal units error behavior is caused by the board (I2C controller) or the chip ADT7476A but probably the former but not sure.
> So should these patches not be applied to the adt7475 driver as you mentioned?
> 

Drivers are not responsible for handling bad board behavior, so retries
due to controller or board issues should not be handled by the chip driver.
Otherwise we would end up having to sprinkle retries into all i2c drivers
in the kernel.

If the problem is caused by the controller, it should be handled in the
controller driver. If the problem is caused by bad i2c signals, it should
be handled by hardware (or maybe by selecting a different clock speed).

> At first I thought that the I2C SMBus APIs return error codes but adt7475 driver does not check so it should be checked the error codes.
> Is this not correct?
> 

Yes, the driver should check for errors, but it should report all errors
to user space and neither retry nor silently hide/ignore errors (unless
a problem is known to be a chip problem).

Guenter

> To make sure let me confirm above.
> 
> Regards,
> Ikegami
> 
>> -----Original Message-----
>> From: linux-hwmon-owner@vger.kernel.org
>> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter Roeck
>> Sent: Thursday, July 12, 2018 12:42 PM
>> To: IKEGAMI Tokunori; Jean Delvare
>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
>>
>> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
>>> Hi Guenter-san,
>>>
>>> Thank you so much for your comments.
>>> Okay now I am thinking to change the adt7475_read macro to a function
>> to repeat for the error case.
>>> If you have any comment about this please let me know.
>>
>> Yes - we should only do this if it is known to be a chip problem.
>> Patching the chip driver for a board (or i2c controller) problem
>> is not appropriate.
>>
>> Guenter
>>
>>> Anyway I will do send v2 version patches later.
>>>
>>> Regards,
>>> Ikegami
>>>
>>>> -----Original Message-----
>>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
>>>> Roeck
>>>> Sent: Thursday, July 12, 2018 10:50 AM
>>>> To: IKEGAMI Tokunori; Jean Delvare
>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
>> macro
>>>>
>>>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
>>>>> It shoudl be same as with other functions to use adt7475_read.
>>>>> So change to use it instead of i2c_smbus_read_byte_data.
>>>>>
>>>>
>>>> I don't see a point in this change. Replacing a function name doesn't
>>>> make
>>>> the code easier to read. If anything, you could consider dropping
>>>> adt7475_read
>>>> and calling i2c_smbus_read_byte_data() directly.
>>>>
>>>> Guenter
>>>>
>>>>> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
>>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> Cc: linux-hwmon@vger.kernel.org
>>>>> ---
>>>>>     drivers/hwmon/adt7475.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
>>>>> index a40eb62ee6b1..bad250729e99 100644
>>>>> --- a/drivers/hwmon/adt7475.c
>>>>> +++ b/drivers/hwmon/adt7475.c
>>>>> @@ -1012,7 +1012,7 @@ static ssize_t
>>>> pwm_use_point2_pwm_at_crit_store(struct device *dev,
>>>>>     		return -EINVAL;
>>>>>
>>>>>     	mutex_lock(&data->lock);
>>>>> -	data->config4 = i2c_smbus_read_byte_data(client,
>>>> REG_CONFIG4);
>>>>> +	data->config4 = adt7475_read(REG_CONFIG4);
>>>>>     	if (val)
>>>>>     		data->config4 |= CONFIG4_MAXDUTY;
>>>>>     	else
>>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[IKEGAMI Tokunori]

Hi Guenter-san,

Thank you so much for the explanation.
I could understand enough it.
I will abandon these patches for now.

> What is an "I2C error unit" ?

  The error unit is caused by the I2C isolator failure device then bad I2C signals causes the error.
  Our products have been replaced the device to other new device for the manufacturing.
  But still there are many units using the old device and it is possible to be caused the failure behavior in future.
  We tried to change the I2C clock speed on the error unit but the issue was not able to be resolved.
  By the way we have fixed our original user space driver for other feature to retry for the error.

> Yes, the driver should check for errors, but it should report all errors
> to user space and neither retry nor silently hide/ignore errors (unless
> a problem is known to be a chip problem).

  I will do consider this in future.
  But at first I will investigate the cause of another voltage warning issue at first.
  It is not caused by the I2C error but sometimes detected the voltage warning actually.

Regards,
Ikegami

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, July 12, 2018 1:23 PM
> To: IKEGAMI Tokunori; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
> 
> On 07/11/2018 09:01 PM, IKEGAMI Tokunori wrote:
> > Hi Guenter-san,
> >
> > Thank you so much for your comments.
> >
> > Oh I see and actually we have some I2C error units in our products.
> > The patch is for the error case mainly.
> 
> What is an "I2C error unit" ?
> 
> > But I think that sometimes the I2C error is still caused on the normal
> units also actually.
> > I am not sure about that the normal units error behavior is caused by
> the board (I2C controller) or the chip ADT7476A but probably the former
> but not sure.
> > So should these patches not be applied to the adt7475 driver as you
> mentioned?
> >
> 
> Drivers are not responsible for handling bad board behavior, so retries
> due to controller or board issues should not be handled by the chip
> driver.
> Otherwise we would end up having to sprinkle retries into all i2c drivers
> in the kernel.
> 
> If the problem is caused by the controller, it should be handled in the
> controller driver. If the problem is caused by bad i2c signals, it should
> be handled by hardware (or maybe by selecting a different clock speed).
> 
> > At first I thought that the I2C SMBus APIs return error codes but adt7475
> driver does not check so it should be checked the error codes.
> > Is this not correct?
> >
> 
> Yes, the driver should check for errors, but it should report all errors
> to user space and neither retry nor silently hide/ignore errors (unless
> a problem is known to be a chip problem).
> 
> Guenter
> 
> > To make sure let me confirm above.
> >
> > Regards,
> > Ikegami
> >
> >> -----Original Message-----
> >> From: linux-hwmon-owner@vger.kernel.org
> >> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter
> Roeck
> >> Sent: Thursday, July 12, 2018 12:42 PM
> >> To: IKEGAMI Tokunori; Jean Delvare
> >> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> macro
> >>
> >> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
> >>> Hi Guenter-san,
> >>>
> >>> Thank you so much for your comments.
> >>> Okay now I am thinking to change the adt7475_read macro to a function
> >> to repeat for the error case.
> >>> If you have any comment about this please let me know.
> >>
> >> Yes - we should only do this if it is known to be a chip problem.
> >> Patching the chip driver for a board (or i2c controller) problem
> >> is not appropriate.
> >>
> >> Guenter
> >>
> >>> Anyway I will do send v2 version patches later.
> >>>
> >>> Regards,
> >>> Ikegami
> >>>
> >>>> -----Original Message-----
> >>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> >>>> Roeck
> >>>> Sent: Thursday, July 12, 2018 10:50 AM
> >>>> To: IKEGAMI Tokunori; Jean Delvare
> >>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> >> macro
> >>>>
> >>>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> >>>>> It shoudl be same as with other functions to use adt7475_read.
> >>>>> So change to use it instead of i2c_smbus_read_byte_data.
> >>>>>
> >>>>
> >>>> I don't see a point in this change. Replacing a function name doesn't
> >>>> make
> >>>> the code easier to read. If anything, you could consider dropping
> >>>> adt7475_read
> >>>> and calling i2c_smbus_read_byte_data() directly.
> >>>>
> >>>> Guenter
> >>>>
> >>>>> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> >>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>>>> Cc: linux-hwmon@vger.kernel.org
> >>>>> ---
> >>>>>     drivers/hwmon/adt7475.c | 2 +-
> >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> >>>>> index a40eb62ee6b1..bad250729e99 100644
> >>>>> --- a/drivers/hwmon/adt7475.c
> >>>>> +++ b/drivers/hwmon/adt7475.c
> >>>>> @@ -1012,7 +1012,7 @@ static ssize_t
> >>>> pwm_use_point2_pwm_at_crit_store(struct device *dev,
> >>>>>     		return -EINVAL;
> >>>>>
> >>>>>     	mutex_lock(&data->lock);
> >>>>> -	data->config4 = i2c_smbus_read_byte_data(client,
> >>>> REG_CONFIG4);
> >>>>> +	data->config4 = adt7475_read(REG_CONFIG4);
> >>>>>     	if (val)
> >>>>>     		data->config4 |= CONFIG4_MAXDUTY;
> >>>>>     	else
> >>>>>
> >>>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-hwmon"
> >> in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[IKEGAMI Tokunori]

Hi Guenter-san,

Sorry but let me consult one more thing.

Since I have just remembered that there are -EAGAIN and -ETIMEDOUT possible to be returned by I2C driver.
Those were mentioned to be able to be checked by the hwmon adt7475 driver in our company not long ago.
Also I have just confirmed the Documentation/i2c/fault-codes so it seems still correct.

How do you think about this?
Still those errors also should be just returned to user space?

Sorry for many times asking you but if you have a time please let me know your opinion.

Regards,
Ikegami

> -----Original Message-----
> From: IKEGAMI Tokunori
> Sent: Thursday, July 12, 2018 3:23 PM
> To: 'Guenter Roeck'; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
> 
> Hi Guenter-san,
> 
> Thank you so much for the explanation.
> I could understand enough it.
> I will abandon these patches for now.
> 
> > What is an "I2C error unit" ?
> 
>   The error unit is caused by the I2C isolator failure device then bad
> I2C signals causes the error.
>   Our products have been replaced the device to other new device for the
> manufacturing.
>   But still there are many units using the old device and it is possible
> to be caused the failure behavior in future.
>   We tried to change the I2C clock speed on the error unit but the issue
> was not able to be resolved.
>   By the way we have fixed our original user space driver for other
> feature to retry for the error.
> 
> > Yes, the driver should check for errors, but it should report all errors
> > to user space and neither retry nor silently hide/ignore errors (unless
> > a problem is known to be a chip problem).
> 
>   I will do consider this in future.
>   But at first I will investigate the cause of another voltage warning
> issue at first.
>   It is not caused by the I2C error but sometimes detected the voltage
> warning actually.
> 
> Regards,
> Ikegami
> 
> > -----Original Message-----
> > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> > Roeck
> > Sent: Thursday, July 12, 2018 1:23 PM
> > To: IKEGAMI Tokunori; Jean Delvare
> > Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> > Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> macro
> >
> > On 07/11/2018 09:01 PM, IKEGAMI Tokunori wrote:
> > > Hi Guenter-san,
> > >
> > > Thank you so much for your comments.
> > >
> > > Oh I see and actually we have some I2C error units in our products.
> > > The patch is for the error case mainly.
> >
> > What is an "I2C error unit" ?
> >
> > > But I think that sometimes the I2C error is still caused on the normal
> > units also actually.
> > > I am not sure about that the normal units error behavior is caused
> by
> > the board (I2C controller) or the chip ADT7476A but probably the former
> > but not sure.
> > > So should these patches not be applied to the adt7475 driver as you
> > mentioned?
> > >
> >
> > Drivers are not responsible for handling bad board behavior, so retries
> > due to controller or board issues should not be handled by the chip
> > driver.
> > Otherwise we would end up having to sprinkle retries into all i2c
> drivers
> > in the kernel.
> >
> > If the problem is caused by the controller, it should be handled in
> the
> > controller driver. If the problem is caused by bad i2c signals, it
> should
> > be handled by hardware (or maybe by selecting a different clock speed).
> >
> > > At first I thought that the I2C SMBus APIs return error codes but
> adt7475
> > driver does not check so it should be checked the error codes.
> > > Is this not correct?
> > >
> >
> > Yes, the driver should check for errors, but it should report all errors
> > to user space and neither retry nor silently hide/ignore errors (unless
> > a problem is known to be a chip problem).
> >
> > Guenter
> >
> > > To make sure let me confirm above.
> > >
> > > Regards,
> > > Ikegami
> > >
> > >> -----Original Message-----
> > >> From: linux-hwmon-owner@vger.kernel.org
> > >> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter
> > Roeck
> > >> Sent: Thursday, July 12, 2018 12:42 PM
> > >> To: IKEGAMI Tokunori; Jean Delvare
> > >> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> > >> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> > macro
> > >>
> > >> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
> > >>> Hi Guenter-san,
> > >>>
> > >>> Thank you so much for your comments.
> > >>> Okay now I am thinking to change the adt7475_read macro to a function
> > >> to repeat for the error case.
> > >>> If you have any comment about this please let me know.
> > >>
> > >> Yes - we should only do this if it is known to be a chip problem.
> > >> Patching the chip driver for a board (or i2c controller) problem
> > >> is not appropriate.
> > >>
> > >> Guenter
> > >>
> > >>> Anyway I will do send v2 version patches later.
> > >>>
> > >>> Regards,
> > >>> Ikegami
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of
> Guenter
> > >>>> Roeck
> > >>>> Sent: Thursday, July 12, 2018 10:50 AM
> > >>>> To: IKEGAMI Tokunori; Jean Delvare
> > >>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> > >>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use
> adt7475_read
> > >> macro
> > >>>>
> > >>>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> > >>>>> It shoudl be same as with other functions to use adt7475_read.
> > >>>>> So change to use it instead of i2c_smbus_read_byte_data.
> > >>>>>
> > >>>>
> > >>>> I don't see a point in this change. Replacing a function name
> doesn't
> > >>>> make
> > >>>> the code easier to read. If anything, you could consider dropping
> > >>>> adt7475_read
> > >>>> and calling i2c_smbus_read_byte_data() directly.
> > >>>>
> > >>>> Guenter
> > >>>>
> > >>>>> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > >>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > >>>>> Cc: linux-hwmon@vger.kernel.org
> > >>>>> ---
> > >>>>>     drivers/hwmon/adt7475.c | 2 +-
> > >>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> > >>>>> index a40eb62ee6b1..bad250729e99 100644
> > >>>>> --- a/drivers/hwmon/adt7475.c
> > >>>>> +++ b/drivers/hwmon/adt7475.c
> > >>>>> @@ -1012,7 +1012,7 @@ static ssize_t
> > >>>> pwm_use_point2_pwm_at_crit_store(struct device *dev,
> > >>>>>     		return -EINVAL;
> > >>>>>
> > >>>>>     	mutex_lock(&data->lock);
> > >>>>> -	data->config4 = i2c_smbus_read_byte_data(client,
> > >>>> REG_CONFIG4);
> > >>>>> +	data->config4 = adt7475_read(REG_CONFIG4);
> > >>>>>     	if (val)
> > >>>>>     		data->config4 |= CONFIG4_MAXDUTY;
> > >>>>>     	else
> > >>>>>
> > >>>
> > >>
> > >> --
> > >> To unsubscribe from this list: send the line "unsubscribe
> linux-hwmon"
> > >> in
> > >> the body of a message to majordomo@vger.kernel.org
> > >> More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[Guenter Roeck]

On 07/12/2018 06:47 AM, IKEGAMI Tokunori wrote:
> Hi Guenter-san,
> 
> Sorry but let me consult one more thing.
> 
> Since I have just remembered that there are -EAGAIN and -ETIMEDOUT possible to be returned by I2C driver.
> Those were mentioned to be able to be checked by the hwmon adt7475 driver in our company not long ago.
> Also I have just confirmed the Documentation/i2c/fault-codes so it seems still correct.
> 
> How do you think about this?
> Still those errors also should be just returned to user space?
> 

A driver should return the error codes it receives from lower level drivers, ie
in this case the error code from the i2c controller driver. It should not modify
error return codes. In fact, if a driver does modify error return codes,
static analyzers will complain.

Hope this helps,
Guenter

> Sorry for many times asking you but if you have a time please let me know your opinion.
> 
> Regards,
> Ikegami
> 
>> -----Original Message-----
>> From: IKEGAMI Tokunori
>> Sent: Thursday, July 12, 2018 3:23 PM
>> To: 'Guenter Roeck'; Jean Delvare
>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>> Subject: RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
>>
>> Hi Guenter-san,
>>
>> Thank you so much for the explanation.
>> I could understand enough it.
>> I will abandon these patches for now.
>>
>>> What is an "I2C error unit" ?
>>
>>    The error unit is caused by the I2C isolator failure device then bad
>> I2C signals causes the error.
>>    Our products have been replaced the device to other new device for the
>> manufacturing.
>>    But still there are many units using the old device and it is possible
>> to be caused the failure behavior in future.
>>    We tried to change the I2C clock speed on the error unit but the issue
>> was not able to be resolved.
>>    By the way we have fixed our original user space driver for other
>> feature to retry for the error.
>>
>>> Yes, the driver should check for errors, but it should report all errors
>>> to user space and neither retry nor silently hide/ignore errors (unless
>>> a problem is known to be a chip problem).
>>
>>    I will do consider this in future.
>>    But at first I will investigate the cause of another voltage warning
>> issue at first.
>>    It is not caused by the I2C error but sometimes detected the voltage
>> warning actually.
>>
>> Regards,
>> Ikegami
>>
>>> -----Original Message-----
>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
>>> Roeck
>>> Sent: Thursday, July 12, 2018 1:23 PM
>>> To: IKEGAMI Tokunori; Jean Delvare
>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
>> macro
>>>
>>> On 07/11/2018 09:01 PM, IKEGAMI Tokunori wrote:
>>>> Hi Guenter-san,
>>>>
>>>> Thank you so much for your comments.
>>>>
>>>> Oh I see and actually we have some I2C error units in our products.
>>>> The patch is for the error case mainly.
>>>
>>> What is an "I2C error unit" ?
>>>
>>>> But I think that sometimes the I2C error is still caused on the normal
>>> units also actually.
>>>> I am not sure about that the normal units error behavior is caused
>> by
>>> the board (I2C controller) or the chip ADT7476A but probably the former
>>> but not sure.
>>>> So should these patches not be applied to the adt7475 driver as you
>>> mentioned?
>>>>
>>>
>>> Drivers are not responsible for handling bad board behavior, so retries
>>> due to controller or board issues should not be handled by the chip
>>> driver.
>>> Otherwise we would end up having to sprinkle retries into all i2c
>> drivers
>>> in the kernel.
>>>
>>> If the problem is caused by the controller, it should be handled in
>> the
>>> controller driver. If the problem is caused by bad i2c signals, it
>> should
>>> be handled by hardware (or maybe by selecting a different clock speed).
>>>
>>>> At first I thought that the I2C SMBus APIs return error codes but
>> adt7475
>>> driver does not check so it should be checked the error codes.
>>>> Is this not correct?
>>>>
>>>
>>> Yes, the driver should check for errors, but it should report all errors
>>> to user space and neither retry nor silently hide/ignore errors (unless
>>> a problem is known to be a chip problem).
>>>
>>> Guenter
>>>
>>>> To make sure let me confirm above.
>>>>
>>>> Regards,
>>>> Ikegami
>>>>
>>>>> -----Original Message-----
>>>>> From: linux-hwmon-owner@vger.kernel.org
>>>>> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter
>>> Roeck
>>>>> Sent: Thursday, July 12, 2018 12:42 PM
>>>>> To: IKEGAMI Tokunori; Jean Delvare
>>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
>>> macro
>>>>>
>>>>> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
>>>>>> Hi Guenter-san,
>>>>>>
>>>>>> Thank you so much for your comments.
>>>>>> Okay now I am thinking to change the adt7475_read macro to a function
>>>>> to repeat for the error case.
>>>>>> If you have any comment about this please let me know.
>>>>>
>>>>> Yes - we should only do this if it is known to be a chip problem.
>>>>> Patching the chip driver for a board (or i2c controller) problem
>>>>> is not appropriate.
>>>>>
>>>>> Guenter
>>>>>
>>>>>> Anyway I will do send v2 version patches later.
>>>>>>
>>>>>> Regards,
>>>>>> Ikegami
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of
>> Guenter
>>>>>>> Roeck
>>>>>>> Sent: Thursday, July 12, 2018 10:50 AM
>>>>>>> To: IKEGAMI Tokunori; Jean Delvare
>>>>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
>>>>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use
>> adt7475_read
>>>>> macro
>>>>>>>
>>>>>>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
>>>>>>>> It shoudl be same as with other functions to use adt7475_read.
>>>>>>>> So change to use it instead of i2c_smbus_read_byte_data.
>>>>>>>>
>>>>>>>
>>>>>>> I don't see a point in this change. Replacing a function name
>> doesn't
>>>>>>> make
>>>>>>> the code easier to read. If anything, you could consider dropping
>>>>>>> adt7475_read
>>>>>>> and calling i2c_smbus_read_byte_data() directly.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>>> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
>>>>>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>>>>> Cc: linux-hwmon@vger.kernel.org
>>>>>>>> ---
>>>>>>>>      drivers/hwmon/adt7475.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
>>>>>>>> index a40eb62ee6b1..bad250729e99 100644
>>>>>>>> --- a/drivers/hwmon/adt7475.c
>>>>>>>> +++ b/drivers/hwmon/adt7475.c
>>>>>>>> @@ -1012,7 +1012,7 @@ static ssize_t
>>>>>>> pwm_use_point2_pwm_at_crit_store(struct device *dev,
>>>>>>>>      		return -EINVAL;
>>>>>>>>
>>>>>>>>      	mutex_lock(&data->lock);
>>>>>>>> -	data->config4 = i2c_smbus_read_byte_data(client,
>>>>>>> REG_CONFIG4);
>>>>>>>> +	data->config4 = adt7475_read(REG_CONFIG4);
>>>>>>>>      	if (val)
>>>>>>>>      		data->config4 |= CONFIG4_MAXDUTY;
>>>>>>>>      	else
>>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe
>> linux-hwmon"
>>>>> in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at
>> http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro

[IKEGAMI Tokunori]

Hi Guenter-san,

Thank you so much for your help.

> A driver should return the error codes it receives from lower level
> drivers, ie
> in this case the error code from the i2c controller driver. It should
> not modify
> error return codes. In fact, if a driver does modify error return codes,
> static analyzers will complain.

  Noted about this I will try to consider to do this.

Regards,
Ikegami

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, July 12, 2018 11:06 PM
> To: IKEGAMI Tokunori; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read macro
> 
> On 07/12/2018 06:47 AM, IKEGAMI Tokunori wrote:
> > Hi Guenter-san,
> >
> > Sorry but let me consult one more thing.
> >
> > Since I have just remembered that there are -EAGAIN and -ETIMEDOUT
> possible to be returned by I2C driver.
> > Those were mentioned to be able to be checked by the hwmon adt7475 driver
> in our company not long ago.
> > Also I have just confirmed the Documentation/i2c/fault-codes so it
> seems still correct.
> >
> > How do you think about this?
> > Still those errors also should be just returned to user space?
> >
> 
> A driver should return the error codes it receives from lower level
> drivers, ie
> in this case the error code from the i2c controller driver. It should
> not modify
> error return codes. In fact, if a driver does modify error return codes,
> static analyzers will complain.
> 
> Hope this helps,
> Guenter
> 
> > Sorry for many times asking you but if you have a time please let me
> know your opinion.
> >
> > Regards,
> > Ikegami
> >
> >> -----Original Message-----
> >> From: IKEGAMI Tokunori
> >> Sent: Thursday, July 12, 2018 3:23 PM
> >> To: 'Guenter Roeck'; Jean Delvare
> >> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >> Subject: RE: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> macro
> >>
> >> Hi Guenter-san,
> >>
> >> Thank you so much for the explanation.
> >> I could understand enough it.
> >> I will abandon these patches for now.
> >>
> >>> What is an "I2C error unit" ?
> >>
> >>    The error unit is caused by the I2C isolator failure device then
> bad
> >> I2C signals causes the error.
> >>    Our products have been replaced the device to other new device for
> the
> >> manufacturing.
> >>    But still there are many units using the old device and it is
> possible
> >> to be caused the failure behavior in future.
> >>    We tried to change the I2C clock speed on the error unit but the
> issue
> >> was not able to be resolved.
> >>    By the way we have fixed our original user space driver for other
> >> feature to retry for the error.
> >>
> >>> Yes, the driver should check for errors, but it should report all
> errors
> >>> to user space and neither retry nor silently hide/ignore errors
> (unless
> >>> a problem is known to be a chip problem).
> >>
> >>    I will do consider this in future.
> >>    But at first I will investigate the cause of another voltage
> warning
> >> issue at first.
> >>    It is not caused by the I2C error but sometimes detected the voltage
> >> warning actually.
> >>
> >> Regards,
> >> Ikegami
> >>
> >>> -----Original Message-----
> >>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> >>> Roeck
> >>> Sent: Thursday, July 12, 2018 1:23 PM
> >>> To: IKEGAMI Tokunori; Jean Delvare
> >>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> >> macro
> >>>
> >>> On 07/11/2018 09:01 PM, IKEGAMI Tokunori wrote:
> >>>> Hi Guenter-san,
> >>>>
> >>>> Thank you so much for your comments.
> >>>>
> >>>> Oh I see and actually we have some I2C error units in our products.
> >>>> The patch is for the error case mainly.
> >>>
> >>> What is an "I2C error unit" ?
> >>>
> >>>> But I think that sometimes the I2C error is still caused on the normal
> >>> units also actually.
> >>>> I am not sure about that the normal units error behavior is caused
> >> by
> >>> the board (I2C controller) or the chip ADT7476A but probably the
> former
> >>> but not sure.
> >>>> So should these patches not be applied to the adt7475 driver as you
> >>> mentioned?
> >>>>
> >>>
> >>> Drivers are not responsible for handling bad board behavior, so
> retries
> >>> due to controller or board issues should not be handled by the chip
> >>> driver.
> >>> Otherwise we would end up having to sprinkle retries into all i2c
> >> drivers
> >>> in the kernel.
> >>>
> >>> If the problem is caused by the controller, it should be handled in
> >> the
> >>> controller driver. If the problem is caused by bad i2c signals, it
> >> should
> >>> be handled by hardware (or maybe by selecting a different clock
> speed).
> >>>
> >>>> At first I thought that the I2C SMBus APIs return error codes but
> >> adt7475
> >>> driver does not check so it should be checked the error codes.
> >>>> Is this not correct?
> >>>>
> >>>
> >>> Yes, the driver should check for errors, but it should report all
> errors
> >>> to user space and neither retry nor silently hide/ignore errors
> (unless
> >>> a problem is known to be a chip problem).
> >>>
> >>> Guenter
> >>>
> >>>> To make sure let me confirm above.
> >>>>
> >>>> Regards,
> >>>> Ikegami
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: linux-hwmon-owner@vger.kernel.org
> >>>>> [mailto:linux-hwmon-owner@vger.kernel.org] On Behalf Of Guenter
> >>> Roeck
> >>>>> Sent: Thursday, July 12, 2018 12:42 PM
> >>>>> To: IKEGAMI Tokunori; Jean Delvare
> >>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use adt7475_read
> >>> macro
> >>>>>
> >>>>> On 07/11/2018 07:52 PM, IKEGAMI Tokunori wrote:
> >>>>>> Hi Guenter-san,
> >>>>>>
> >>>>>> Thank you so much for your comments.
> >>>>>> Okay now I am thinking to change the adt7475_read macro to a
> function
> >>>>> to repeat for the error case.
> >>>>>> If you have any comment about this please let me know.
> >>>>>
> >>>>> Yes - we should only do this if it is known to be a chip problem.
> >>>>> Patching the chip driver for a board (or i2c controller) problem
> >>>>> is not appropriate.
> >>>>>
> >>>>> Guenter
> >>>>>
> >>>>>> Anyway I will do send v2 version patches later.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Ikegami
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of
> >> Guenter
> >>>>>>> Roeck
> >>>>>>> Sent: Thursday, July 12, 2018 10:50 AM
> >>>>>>> To: IKEGAMI Tokunori; Jean Delvare
> >>>>>>> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> >>>>>>> Subject: Re: [PATCH 3/5] hwmon: adt7475: Change to use
> >> adt7475_read
> >>>>> macro
> >>>>>>>
> >>>>>>> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> >>>>>>>> It shoudl be same as with other functions to use adt7475_read.
> >>>>>>>> So change to use it instead of i2c_smbus_read_byte_data.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I don't see a point in this change. Replacing a function name
> >> doesn't
> >>>>>>> make
> >>>>>>> the code easier to read. If anything, you could consider dropping
> >>>>>>> adt7475_read
> >>>>>>> and calling i2c_smbus_read_byte_data() directly.
> >>>>>>>
> >>>>>>> Guenter
> >>>>>>>
> >>>>>>>> Signed-off-by: Tokunori Ikegami
> <ikegami@allied-telesis.co.jp>
> >>>>>>>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>>>>>>> Cc: linux-hwmon@vger.kernel.org
> >>>>>>>> ---
> >>>>>>>>      drivers/hwmon/adt7475.c | 2 +-
> >>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/hwmon/adt7475.c
> b/drivers/hwmon/adt7475.c
> >>>>>>>> index a40eb62ee6b1..bad250729e99 100644
> >>>>>>>> --- a/drivers/hwmon/adt7475.c
> >>>>>>>> +++ b/drivers/hwmon/adt7475.c
> >>>>>>>> @@ -1012,7 +1012,7 @@ static ssize_t
> >>>>>>> pwm_use_point2_pwm_at_crit_store(struct device *dev,
> >>>>>>>>      		return -EINVAL;
> >>>>>>>>
> >>>>>>>>      	mutex_lock(&data->lock);
> >>>>>>>> -	data->config4 = i2c_smbus_read_byte_data(client,
> >>>>>>> REG_CONFIG4);
> >>>>>>>> +	data->config4 = adt7475_read(REG_CONFIG4);
> >>>>>>>>      	if (val)
> >>>>>>>>      		data->config4 |= CONFIG4_MAXDUTY;
> >>>>>>>>      	else
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>>> --
> >>>>> To unsubscribe from this list: send the line "unsubscribe
> >> linux-hwmon"
> >>>>> in
> >>>>> the body of a message to majordomo@vger.kernel.org
> >>>>> More majordomo info at
> >> http://vger.kernel.org/majordomo-info.html
> >

[PATCH 4/5] hwmon: adt7475: Change to use adt7475_write macro

[Tokunori Ikegami]

As same with adt7475_read it is better to use adt7475_write macro.
So change to use it instead of i2c_smbus_write_byte_data.

Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
 drivers/hwmon/adt7475.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index bad250729e99..31a12ac405ef 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -133,6 +133,10 @@
 
 #define adt7475_read(reg) i2c_smbus_read_byte_data(client, (reg))
 
+/* Macro to write the registers */
+
+#define adt7475_write(reg, val) i2c_smbus_write_byte_data(client, (reg), (val))
+
 /* Macros to easily index the registers */
 
 #define TACH_REG(idx) (REG_TACH_BASE + ((idx) * 2))
@@ -322,11 +326,11 @@ static int adt7475_write_word(struct i2c_client *client, int reg, u16 val)
 {
 	int ret;
 
-	ret = i2c_smbus_write_byte_data(client, reg + 1, val >> 8);
+	ret = adt7475_write(reg + 1, val >> 8);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_write_byte_data(client, reg, val & 0xFF);
+	ret = adt7475_write(reg, val & 0xFF);
 	if (ret < 0)
 		return ret;
 
@@ -381,7 +385,7 @@ static ssize_t set_voltage(struct device *dev, struct device_attribute *attr,
 			reg = REG_VTT_MAX;
 	}
 
-	i2c_smbus_write_byte_data(client, reg,
+	adt7475_write(reg,
 				  data->voltage[sattr->nr][sattr->index] >> 2);
 	mutex_unlock(&data->lock);
 
@@ -534,7 +538,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
 		break;
 	}
 
-	i2c_smbus_write_byte_data(client, reg, out);
+	adt7475_write(reg, out);
 
 	mutex_unlock(&data->lock);
 	return count;
@@ -615,7 +619,7 @@ static ssize_t set_temp_st(struct device *dev, struct device_attribute *attr,
 	data->enh_acoustics[idx] &= ~(0xf << shift);
 	data->enh_acoustics[idx] |= (val << shift);
 
-	i2c_smbus_write_byte_data(client, reg, data->enh_acoustics[idx]);
+	adt7475_write(reg, data->enh_acoustics[idx]);
 
 	mutex_unlock(&data->lock);
 
@@ -683,7 +687,7 @@ static ssize_t set_point2(struct device *dev, struct device_attribute *attr,
 	data->range[sattr->index] &= ~0xF0;
 	data->range[sattr->index] |= val << 4;
 
-	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
+	adt7475_write(TEMP_TRANGE_REG(sattr->index),
 				  data->range[sattr->index]);
 
 	mutex_unlock(&data->lock);
@@ -798,7 +802,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 	}
 
 	data->pwm[sattr->nr][sattr->index] = clamp_val(val, 0, 0xFF);
-	i2c_smbus_write_byte_data(client, reg,
+	adt7475_write(reg,
 				  data->pwm[sattr->nr][sattr->index]);
 	mutex_unlock(&data->lock);
 
@@ -835,7 +839,7 @@ static ssize_t set_stall_disable(struct device *dev,
 	if (val)
 		data->enh_acoustics[0] |= mask;
 
-	i2c_smbus_write_byte_data(client, REG_ENHANCE_ACOUSTICS1,
+	adt7475_write(REG_ENHANCE_ACOUSTICS1,
 				  data->enh_acoustics[0]);
 
 	mutex_unlock(&data->lock);
@@ -894,7 +898,7 @@ static int hw_set_pwm(struct i2c_client *client, int index,
 	data->pwm[CONTROL][index] &= ~0xE0;
 	data->pwm[CONTROL][index] |= (val & 7) << 5;
 
-	i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
+	adt7475_write(PWM_CONFIG_REG(index),
 				  data->pwm[CONTROL][index]);
 
 	return 0;
@@ -983,7 +987,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
 	data->range[sattr->index] &= ~0xf;
 	data->range[sattr->index] |= out;
 
-	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
+	adt7475_write(TEMP_TRANGE_REG(sattr->index),
 				  data->range[sattr->index]);
 
 	mutex_unlock(&data->lock);
@@ -1017,7 +1021,7 @@ static ssize_t pwm_use_point2_pwm_at_crit_store(struct device *dev,
 		data->config4 |= CONFIG4_MAXDUTY;
 	else
 		data->config4 &= ~CONFIG4_MAXDUTY;
-	i2c_smbus_write_byte_data(client, REG_CONFIG4, data->config4);
+	adt7475_write(REG_CONFIG4, data->config4);
 	mutex_unlock(&data->lock);
 
 	return count;
@@ -1642,10 +1646,10 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
 		data->pwm[CONTROL][index] &= ~0xE0;
 		data->pwm[CONTROL][index] |= (7 << 5);
 
-		i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
+		adt7475_write(PWM_CONFIG_REG(index),
 					  data->pwm[INPUT][index]);
 
-		i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
+		adt7475_write(PWM_CONFIG_REG(index),
 					  data->pwm[CONTROL][index]);
 
 		data->pwmctl[index] = 1;
-- 
2.16.1

Re: [PATCH 4/5] hwmon: adt7475: Change to use adt7475_write macro

[Guenter Roeck]

On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> As same with adt7475_read it is better to use adt7475_write macro.
> So change to use it instead of i2c_smbus_write_byte_data.
> 
You don't explain why this would be "better", and I disagree that it is.
On the contrary, it just hides the API function. I don't see why this
would be better, and even more I don't want to encourage others
flooding us with patches to introduce similar code in other drivers
because they think that it is "better".

Guenter

> Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Cc: linux-hwmon@vger.kernel.org
> ---
>   drivers/hwmon/adt7475.c | 30 +++++++++++++++++-------------
>   1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index bad250729e99..31a12ac405ef 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -133,6 +133,10 @@
>   
>   #define adt7475_read(reg) i2c_smbus_read_byte_data(client, (reg))
>   
> +/* Macro to write the registers */
>  > +#define adt7475_write(reg, val) i2c_smbus_write_byte_data(client, (reg), (val))
> +
>   /* Macros to easily index the registers */
>   
>   #define TACH_REG(idx) (REG_TACH_BASE + ((idx) * 2))
> @@ -322,11 +326,11 @@ static int adt7475_write_word(struct i2c_client *client, int reg, u16 val)
>   {
>   	int ret;
>   
> -	ret = i2c_smbus_write_byte_data(client, reg + 1, val >> 8);
> +	ret = adt7475_write(reg + 1, val >> 8);
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = i2c_smbus_write_byte_data(client, reg, val & 0xFF);
> +	ret = adt7475_write(reg, val & 0xFF);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -381,7 +385,7 @@ static ssize_t set_voltage(struct device *dev, struct device_attribute *attr,
>   			reg = REG_VTT_MAX;
>   	}
>   
> -	i2c_smbus_write_byte_data(client, reg,
> +	adt7475_write(reg,
>   				  data->voltage[sattr->nr][sattr->index] >> 2);
>   	mutex_unlock(&data->lock);
>   
> @@ -534,7 +538,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
>   		break;
>   	}
>   
> -	i2c_smbus_write_byte_data(client, reg, out);
> +	adt7475_write(reg, out);
>   
>   	mutex_unlock(&data->lock);
>   	return count;
> @@ -615,7 +619,7 @@ static ssize_t set_temp_st(struct device *dev, struct device_attribute *attr,
>   	data->enh_acoustics[idx] &= ~(0xf << shift);
>   	data->enh_acoustics[idx] |= (val << shift);
>   
> -	i2c_smbus_write_byte_data(client, reg, data->enh_acoustics[idx]);
> +	adt7475_write(reg, data->enh_acoustics[idx]);
>   
>   	mutex_unlock(&data->lock);
>   
> @@ -683,7 +687,7 @@ static ssize_t set_point2(struct device *dev, struct device_attribute *attr,
>   	data->range[sattr->index] &= ~0xF0;
>   	data->range[sattr->index] |= val << 4;
>   
> -	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
> +	adt7475_write(TEMP_TRANGE_REG(sattr->index),
>   				  data->range[sattr->index]);
>   
>   	mutex_unlock(&data->lock);
> @@ -798,7 +802,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
>   	}
>   
>   	data->pwm[sattr->nr][sattr->index] = clamp_val(val, 0, 0xFF);
> -	i2c_smbus_write_byte_data(client, reg,
> +	adt7475_write(reg,
>   				  data->pwm[sattr->nr][sattr->index]);
>   	mutex_unlock(&data->lock);
>   
> @@ -835,7 +839,7 @@ static ssize_t set_stall_disable(struct device *dev,
>   	if (val)
>   		data->enh_acoustics[0] |= mask;
>   
> -	i2c_smbus_write_byte_data(client, REG_ENHANCE_ACOUSTICS1,
> +	adt7475_write(REG_ENHANCE_ACOUSTICS1,
>   				  data->enh_acoustics[0]);
>   
>   	mutex_unlock(&data->lock);
> @@ -894,7 +898,7 @@ static int hw_set_pwm(struct i2c_client *client, int index,
>   	data->pwm[CONTROL][index] &= ~0xE0;
>   	data->pwm[CONTROL][index] |= (val & 7) << 5;
>   
> -	i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
> +	adt7475_write(PWM_CONFIG_REG(index),
>   				  data->pwm[CONTROL][index]);
>   
>   	return 0;
> @@ -983,7 +987,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
>   	data->range[sattr->index] &= ~0xf;
>   	data->range[sattr->index] |= out;
>   
> -	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
> +	adt7475_write(TEMP_TRANGE_REG(sattr->index),
>   				  data->range[sattr->index]);
>   
>   	mutex_unlock(&data->lock);
> @@ -1017,7 +1021,7 @@ static ssize_t pwm_use_point2_pwm_at_crit_store(struct device *dev,
>   		data->config4 |= CONFIG4_MAXDUTY;
>   	else
>   		data->config4 &= ~CONFIG4_MAXDUTY;
> -	i2c_smbus_write_byte_data(client, REG_CONFIG4, data->config4);
> +	adt7475_write(REG_CONFIG4, data->config4);
>   	mutex_unlock(&data->lock);
>   
>   	return count;
> @@ -1642,10 +1646,10 @@ static void adt7475_read_pwm(struct i2c_client *client, int index)
>   		data->pwm[CONTROL][index] &= ~0xE0;
>   		data->pwm[CONTROL][index] |= (7 << 5);
>   
> -		i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
> +		adt7475_write(PWM_CONFIG_REG(index),
>   					  data->pwm[INPUT][index]);
>   
> -		i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
> +		adt7475_write(PWM_CONFIG_REG(index),
>   					  data->pwm[CONTROL][index]);
>   
>   		data->pwmctl[index] = 1;
> 

RE: [PATCH 4/5] hwmon: adt7475: Change to use adt7475_write macro

[IKEGAMI Tokunori]

Hi Guenter-san,

I think to change the patch to repeat for the error case by a adt7475_write function as same with adt7475_read.

Regards,
Ikegami

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: Thursday, July 12, 2018 10:53 AM
> To: IKEGAMI Tokunori; Jean Delvare
> Cc: PACKHAM Chris; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH 4/5] hwmon: adt7475: Change to use adt7475_write
> macro
> 
> On 07/11/2018 06:04 PM, Tokunori Ikegami wrote:
> > As same with adt7475_read it is better to use adt7475_write macro.
> > So change to use it instead of i2c_smbus_write_byte_data.
> >
> You don't explain why this would be "better", and I disagree that it is.
> On the contrary, it just hides the API function. I don't see why this
> would be better, and even more I don't want to encourage others
> flooding us with patches to introduce similar code in other drivers
> because they think that it is "better".
> 
> Guenter
> 
> > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > Cc: linux-hwmon@vger.kernel.org
> > ---
> >   drivers/hwmon/adt7475.c | 30 +++++++++++++++++-------------
> >   1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> > index bad250729e99..31a12ac405ef 100644
> > --- a/drivers/hwmon/adt7475.c
> > +++ b/drivers/hwmon/adt7475.c
> > @@ -133,6 +133,10 @@
> >
> >   #define adt7475_read(reg) i2c_smbus_read_byte_data(client, (reg))
> >
> > +/* Macro to write the registers */
> >  > +#define adt7475_write(reg, val) i2c_smbus_write_byte_data(client,
> (reg), (val))
> > +
> >   /* Macros to easily index the registers */
> >
> >   #define TACH_REG(idx) (REG_TACH_BASE + ((idx) * 2))
> > @@ -322,11 +326,11 @@ static int adt7475_write_word(struct i2c_client
> *client, int reg, u16 val)
> >   {
> >   	int ret;
> >
> > -	ret = i2c_smbus_write_byte_data(client, reg + 1, val >> 8);
> > +	ret = adt7475_write(reg + 1, val >> 8);
> >   	if (ret < 0)
> >   		return ret;
> >
> > -	ret = i2c_smbus_write_byte_data(client, reg, val & 0xFF);
> > +	ret = adt7475_write(reg, val & 0xFF);
> >   	if (ret < 0)
> >   		return ret;
> >
> > @@ -381,7 +385,7 @@ static ssize_t set_voltage(struct device *dev,
> struct device_attribute *attr,
> >   			reg = REG_VTT_MAX;
> >   	}
> >
> > -	i2c_smbus_write_byte_data(client, reg,
> > +	adt7475_write(reg,
> >
> data->voltage[sattr->nr][sattr->index] >> 2);
> >   	mutex_unlock(&data->lock);
> >
> > @@ -534,7 +538,7 @@ static ssize_t set_temp(struct device *dev, struct
> device_attribute *attr,
> >   		break;
> >   	}
> >
> > -	i2c_smbus_write_byte_data(client, reg, out);
> > +	adt7475_write(reg, out);
> >
> >   	mutex_unlock(&data->lock);
> >   	return count;
> > @@ -615,7 +619,7 @@ static ssize_t set_temp_st(struct device *dev,
> struct device_attribute *attr,
> >   	data->enh_acoustics[idx] &= ~(0xf << shift);
> >   	data->enh_acoustics[idx] |= (val << shift);
> >
> > -	i2c_smbus_write_byte_data(client, reg,
> data->enh_acoustics[idx]);
> > +	adt7475_write(reg, data->enh_acoustics[idx]);
> >
> >   	mutex_unlock(&data->lock);
> >
> > @@ -683,7 +687,7 @@ static ssize_t set_point2(struct device *dev,
> struct device_attribute *attr,
> >   	data->range[sattr->index] &= ~0xF0;
> >   	data->range[sattr->index] |= val << 4;
> >
> > -	i2c_smbus_write_byte_data(client,
> TEMP_TRANGE_REG(sattr->index),
> > +	adt7475_write(TEMP_TRANGE_REG(sattr->index),
> >   				  data->range[sattr->index]);
> >
> >   	mutex_unlock(&data->lock);
> > @@ -798,7 +802,7 @@ static ssize_t set_pwm(struct device *dev, struct
> device_attribute *attr,
> >   	}
> >
> >   	data->pwm[sattr->nr][sattr->index] = clamp_val(val, 0, 0xFF);
> > -	i2c_smbus_write_byte_data(client, reg,
> > +	adt7475_write(reg,
> >
> data->pwm[sattr->nr][sattr->index]);
> >   	mutex_unlock(&data->lock);
> >
> > @@ -835,7 +839,7 @@ static ssize_t set_stall_disable(struct device
> *dev,
> >   	if (val)
> >   		data->enh_acoustics[0] |= mask;
> >
> > -	i2c_smbus_write_byte_data(client, REG_ENHANCE_ACOUSTICS1,
> > +	adt7475_write(REG_ENHANCE_ACOUSTICS1,
> >   				  data->enh_acoustics[0]);
> >
> >   	mutex_unlock(&data->lock);
> > @@ -894,7 +898,7 @@ static int hw_set_pwm(struct i2c_client *client,
> int index,
> >   	data->pwm[CONTROL][index] &= ~0xE0;
> >   	data->pwm[CONTROL][index] |= (val & 7) << 5;
> >
> > -	i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
> > +	adt7475_write(PWM_CONFIG_REG(index),
> >   				  data->pwm[CONTROL][index]);
> >
> >   	return 0;
> > @@ -983,7 +987,7 @@ static ssize_t set_pwmfreq(struct device *dev,
> struct device_attribute *attr,
> >   	data->range[sattr->index] &= ~0xf;
> >   	data->range[sattr->index] |= out;
> >
> > -	i2c_smbus_write_byte_data(client,
> TEMP_TRANGE_REG(sattr->index),
> > +	adt7475_write(TEMP_TRANGE_REG(sattr->index),
> >   				  data->range[sattr->index]);
> >
> >   	mutex_unlock(&data->lock);
> > @@ -1017,7 +1021,7 @@ static ssize_t
> pwm_use_point2_pwm_at_crit_store(struct device *dev,
> >   		data->config4 |= CONFIG4_MAXDUTY;
> >   	else
> >   		data->config4 &= ~CONFIG4_MAXDUTY;
> > -	i2c_smbus_write_byte_data(client, REG_CONFIG4,
> data->config4);
> > +	adt7475_write(REG_CONFIG4, data->config4);
> >   	mutex_unlock(&data->lock);
> >
> >   	return count;
> > @@ -1642,10 +1646,10 @@ static void adt7475_read_pwm(struct i2c_client
> *client, int index)
> >   		data->pwm[CONTROL][index] &= ~0xE0;
> >   		data->pwm[CONTROL][index] |= (7 << 5);
> >
> > -		i2c_smbus_write_byte_data(client,
> PWM_CONFIG_REG(index),
> > +		adt7475_write(PWM_CONFIG_REG(index),
> >   					  data->pwm[INPUT][index]);
> >
> > -		i2c_smbus_write_byte_data(client,
> PWM_CONFIG_REG(index),
> > +		adt7475_write(PWM_CONFIG_REG(index),
> >   					  data->pwm[CONTROL][index]);
> >
> >   		data->pwmctl[index] = 1;
> >

[PATCH 5/5] 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: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: linux-hwmon@vger.kernel.org
---
 drivers/hwmon/adt7475.c | 177 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 137 insertions(+), 40 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 31a12ac405ef..00da17d72d0d 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1682,48 +1682,91 @@ static int adt7475_update_measure(struct device *dev)
 	struct adt7475_data *data = i2c_get_clientdata(client);
 	u16 ext;
 	int i;
+	int val, val2;
 
-	data->alarms = adt7475_read(REG_STATUS2) << 8;
-	data->alarms |= adt7475_read(REG_STATUS1);
+	val = adt7475_read(REG_STATUS2);
+	if (val < 0)
+		return val;
+	data->alarms = val << 8;
+
+	val = adt7475_read(REG_STATUS1);
+	if (val < 0)
+		return val;
+	data->alarms |= val;
+
+	val2 = adt7475_read(REG_EXTEND2);
+	if (val2 < 0)
+		return val2;
+
+	val = adt7475_read(REG_EXTEND1);
+	if (val < 0)
+		return val;
+
+	ext = (val2 << 8) | val;
 
-	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;
+		val = adt7475_read(VOLTAGE_REG(i));
+		if (val < 0)
+			return val;
 		data->voltage[INPUT][i] =
-			(adt7475_read(VOLTAGE_REG(i)) << 2) |
+			(val << 2) |
 			((ext >> (i * 2)) & 3);
 	}
 
-	for (i = 0; i < ADT7475_TEMP_COUNT; i++)
+	for (i = 0; i < ADT7475_TEMP_COUNT; i++) {
+		val = adt7475_read(TEMP_REG(i));
+		if (val < 0)
+			return val;
 		data->temp[INPUT][i] =
-			(adt7475_read(TEMP_REG(i)) << 2) |
+			(val << 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 |
+		val = adt7475_read(REG_STATUS4);
+		if (val < 0)
+			return val;
+		data->alarms |= val << 24;
+
+		val = adt7475_read(REG_EXTEND3);
+		if (val < 0)
+			return val;
+		ext = val;
+
+		val = adt7475_read(REG_VTT);
+		if (val < 0)
+			return val;
+		data->voltage[INPUT][5] = val << 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));
+		val = adt7475_read_word(client, TACH_REG(i));
+		if (val < 0)
+			return val;
+		data->tach[INPUT][i] = val;
 	}
 
 	/* 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));
+		val = adt7475_read(PWM_REG(i));
+		if (val < 0)
+			return val;
+		data->pwm[INPUT][i] = val;
 	}
 
-	if (data->has_vid)
-		data->vid = adt7475_read(REG_VID) & 0x3f;
+	if (data->has_vid) {
+		val = adt7475_read(REG_VID);
+		if (val < 0)
+			return val;
+		data->vid = val & 0x3f;
+	}
 
 	return 0;
 }
@@ -1733,59 +1776,113 @@ 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 val;
 
-	data->config4 = adt7475_read(REG_CONFIG4);
-	data->config5 = adt7475_read(REG_CONFIG5);
+	val = adt7475_read(REG_CONFIG4);
+	if (val < 0)
+		return val;
+	data->config4 = val;
+
+	val = adt7475_read(REG_CONFIG5);
+	if (val < 0)
+		return val;
+	data->config5 = val;
 
 	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;
+		val = adt7475_read(VOLTAGE_MIN_REG(i));
+		if (val < 0)
+			return val;
+		data->voltage[MIN][i] = val << 2;
+
+		val = adt7475_read(VOLTAGE_MAX_REG(i));
+		if (val < 0)
+			return val;
+		data->voltage[MAX][i] = val << 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;
+		val = adt7475_read(REG_VTT_MIN);
+		if (val < 0)
+			return val;
+		data->voltage[MIN][5] = val << 2;
+
+		val = adt7475_read(REG_VTT_MAX);
+		if (val < 0)
+			return val;
+		data->voltage[MAX][5] = val << 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));
+		val = adt7475_read(TEMP_MIN_REG(i));
+		if (val < 0)
+			return val;
+		data->temp[MIN][i] = val << 2;
+
+		val = adt7475_read(TEMP_MAX_REG(i));
+		if (val < 0)
+			return val;
+		data->temp[MAX][i] = val << 2;
+
+		val = adt7475_read(TEMP_TMIN_REG(i));
+		if (val < 0)
+			return val;
+		data->temp[AUTOMIN][i] = val << 2;
+
+		val = adt7475_read(TEMP_THERM_REG(i));
+		if (val < 0)
+			return val;
+		data->temp[THERM][i] = val << 2;
+
+		val = adt7475_read(TEMP_OFFSET_REG(i));
+		if (val < 0)
+			return val;
+		data->temp[OFFSET][i] = val;
 	}
 	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));
+		val = adt7475_read_word(client, TACH_MIN_REG(i));
+		if (val < 0)
+			return val;
+		data->tach[MIN][i] = val;
 	}
 
 	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));
+		val = adt7475_read(PWM_MAX_REG(i));
+		if (val < 0)
+			return val;
+		data->pwm[MAX][i] = val;
+
+		val = adt7475_read(PWM_MIN_REG(i));
+		if (val < 0)
+			return val;
+		data->pwm[MIN][i] = val;
 		/* 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));
+	val = adt7475_read(TEMP_TRANGE_REG(0));
+	if (val < 0)
+		return val;
+	data->range[0] = val;
+
+	val = adt7475_read(TEMP_TRANGE_REG(1));
+	if (val < 0)
+		return val;
+	data->range[1] = val;
+
+	val = adt7475_read(TEMP_TRANGE_REG(2));
+	if (val < 0)
+		return val;
+	data->range[2] = val;
 
 	return 0;
 }
-- 
2.16.1