[lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[Frans Meulenbroeks]

added error handling so if lm80_update_device fails
an error is returned when reading the value through sysfs
This is closely modeled after the way this is handled in ltc4261

Note: if update gets an error, this error is returned for all
registers and no further reads are done.
If a register is not accessible most likely the whole device
is broken or disconnected and it is definitely interesting to know.

Note2: I added a macro to read and handle errors. This was felt to
be the simplest. If someone feels it should be done using a
subroutine, be my guest.

Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
---
patch is against the hwmon staging tree
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
as retrieved on jan 2, 2012

 drivers/hwmon/lm80.c |   97 ++++++++++++++++++++++++++++++++++---------------
 1 files changed, 67 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/lm80.c b/drivers/hwmon/lm80.c
index 18a0e6c..5283b27 100644
--- a/drivers/hwmon/lm80.c
+++ b/drivers/hwmon/lm80.c
@@ -168,6 +168,8 @@ static ssize_t show_in_##suffix(struct device *dev, struct device_attribute *att
 { \
 	int nr = to_sensor_dev_attr(attr)->index; \
 	struct lm80_data *data = lm80_update_device(dev); \
+	if (IS_ERR(data)) \
+		return PTR_ERR(data); \
 	return sprintf(buf, "%d\n", IN_FROM_REG(data->value[nr])); \
 }
 show_in(min, in_min)
@@ -197,6 +199,8 @@ static ssize_t show_fan_##suffix(struct device *dev, struct device_attribute *at
 { \
 	int nr = to_sensor_dev_attr(attr)->index; \
 	struct lm80_data *data = lm80_update_device(dev); \
+	if (IS_ERR(data)) \
+		return PTR_ERR(data); \
 	return sprintf(buf, "%d\n", FAN_FROM_REG(data->value[nr], \
 		       DIV_FROM_REG(data->fan_div[nr]))); \
 }
@@ -208,6 +212,8 @@ static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
 {
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct lm80_data *data = lm80_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 	return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]));
 }
 
@@ -271,6 +277,8 @@ static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
 static ssize_t show_temp_input1(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct lm80_data *data = lm80_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 	return sprintf(buf, "%ld\n", TEMP_FROM_REG(data->temp));
 }
 
@@ -278,6 +286,8 @@ static ssize_t show_temp_input1(struct device *dev, struct device_attribute *att
 static ssize_t show_temp_##suffix(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
 	struct lm80_data *data = lm80_update_device(dev); \
+	if (IS_ERR(data)) \
+		return PTR_ERR(data); \
 	return sprintf(buf, "%d\n", TEMP_LIMIT_FROM_REG(data->value)); \
 }
 show_temp(hot_max, temp_hot_max);
@@ -308,6 +318,8 @@ static ssize_t show_alarms(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
 	struct lm80_data *data = lm80_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 	return sprintf(buf, "%u\n", data->alarms);
 }
 
@@ -316,6 +328,8 @@ static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
 {
 	int bitnr = to_sensor_dev_attr(attr)->index;
 	struct lm80_data *data = lm80_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 	return sprintf(buf, "%u\n", (data->alarms >> bitnr) & 1);
 }
 
@@ -544,55 +558,78 @@ static void lm80_init_client(struct i2c_client *client)
 	lm80_write_value(client, LM80_REG_CONFIG, 0x01);
 }
 
+/*
+   Note: safe_lm80_read_value is a helper macro for lm80_update_device
+	 It reads a value using lm80_read_value and jumps to abort
+	 in case of an error.
+	 Due to this jump and because it modifies the first arguement
+	 it has to be a macro
+*/
+#define safe_lm80_read_value(var, client, reg) \
+	{ \
+		status = lm80_read_value(client, reg); \
+		if (unlikely(status < 0)) { \
+			dev_dbg(dev, \
+				"LM80: Failed to read value: reg %d, error %d\n", \
+				reg, status); \
+			ret = ERR_PTR(status); \
+			data->valid = 0; \
+			goto abort; \
+		} \
+		else \
+			var = status; \
+	} 
+
 static struct lm80_data *lm80_update_device(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm80_data *data = i2c_get_clientdata(client);
+	struct lm80_data *ret = data;
 	int i;
+	int status;
+	int tmp1, tmp2;
 
 	mutex_lock(&data->update_lock);
 
+	if (!data->valid)
+	{
+		/* last time failed: re-initialize the LM80 chip */
+		lm80_init_client(client);
+	}
 	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
 		dev_dbg(&client->dev, "Starting lm80 update\n");
 		for (i = 0; i <= 6; i++) {
-			data->in[i] -			    lm80_read_value(client, LM80_REG_IN(i));
-			data->in_min[i] -			    lm80_read_value(client, LM80_REG_IN_MIN(i));
-			data->in_max[i] -			    lm80_read_value(client, LM80_REG_IN_MAX(i));
+			safe_lm80_read_value(data->in[i], client, LM80_REG_IN(i));
+			safe_lm80_read_value(data->in_min[i], client, LM80_REG_IN_MIN(i));
+			safe_lm80_read_value(data->in_max[i], client, LM80_REG_IN_MAX(i));
 		}
-		data->fan[0] = lm80_read_value(client, LM80_REG_FAN1);
-		data->fan_min[0] -		    lm80_read_value(client, LM80_REG_FAN_MIN(1));
-		data->fan[1] = lm80_read_value(client, LM80_REG_FAN2);
-		data->fan_min[1] -		    lm80_read_value(client, LM80_REG_FAN_MIN(2));
-
-		data->temp -		    (lm80_read_value(client, LM80_REG_TEMP) << 8) |
-		    (lm80_read_value(client, LM80_REG_RES) & 0xf0);
-		data->temp_os_max -		    lm80_read_value(client, LM80_REG_TEMP_OS_MAX);
-		data->temp_os_hyst -		    lm80_read_value(client, LM80_REG_TEMP_OS_HYST);
-		data->temp_hot_max -		    lm80_read_value(client, LM80_REG_TEMP_HOT_MAX);
-		data->temp_hot_hyst -		    lm80_read_value(client, LM80_REG_TEMP_HOT_HYST);
-
-		i = lm80_read_value(client, LM80_REG_FANDIV);
+		safe_lm80_read_value(data->fan[0], client, LM80_REG_FAN1);
+		safe_lm80_read_value(data->fan_min[0], client, LM80_REG_FAN_MIN(1));
+		safe_lm80_read_value(data->fan[1], client, LM80_REG_FAN2);
+		safe_lm80_read_value(data->fan_min[1], client, LM80_REG_FAN_MIN(2));
+
+		safe_lm80_read_value(tmp1, client, LM80_REG_TEMP);
+		safe_lm80_read_value(tmp2, client, LM80_REG_RES);
+		data->temp = (tmp1 << 8) | (tmp2 ^ 0xf0);
+
+		safe_lm80_read_value(data->temp_os_max, client, LM80_REG_TEMP_OS_MAX);
+		safe_lm80_read_value(data->temp_os_hyst, client, LM80_REG_TEMP_OS_HYST);
+		safe_lm80_read_value(data->temp_hot_max, client, LM80_REG_TEMP_HOT_MAX);
+		safe_lm80_read_value(data->temp_hot_hyst, client, LM80_REG_TEMP_HOT_HYST);
+
+		safe_lm80_read_value(i, client, LM80_REG_FANDIV);
 		data->fan_div[0] = (i >> 2) & 0x03;
 		data->fan_div[1] = (i >> 4) & 0x03;
-		data->alarms = lm80_read_value(client, LM80_REG_ALARM1) +
-		    (lm80_read_value(client, LM80_REG_ALARM2) << 8);
+		safe_lm80_read_value(tmp1, client, LM80_REG_ALARM1);
+		safe_lm80_read_value(tmp2, client, LM80_REG_ALARM2);
+		data->alarms = tmp1 + (tmp2 << 8);
 		data->last_updated = jiffies;
 		data->valid = 1;
 	}
 
+abort:
 	mutex_unlock(&data->update_lock);
-
-	return data;
+	return ret;
 }
 
 static int __init sensors_lm80_init(void)
-- 
1.7.0.4


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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 808 bytes --]

Oops:

Forgot to mention this in the commit message but this:

  static struct lm80_data *lm80_update_device(struct device *dev)
>  {
>
> [...]

>
> +       if (!data->valid)
> +       {
> +               /* last time failed: re-initialize the LM80 chip */
> +               lm80_init_client(client);
> +       }
>        if (time_after(jiffies, data->last_updated + 2 * HZ) ||
> !data->valid) {
>

was added to reinitialize the chip if the data is not valid.

This helps that if the device is disconnected temporarily it will give the
proper value again after being connected.

(this is tested with a small fan+LM80 pcb that is connected to the main
system with a connector so the fan can be serviced/replaced if needed
(which was the reason to author this patch in the first place)).

Best regards, Frans.

[-- Attachment #1.2: Type: text/html, Size: 1181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Guenter Roeck]

Hi Frans,

On Mon, Jan 02, 2012 at 06:03:22AM -0500, Frans Meulenbroeks wrote:
> added error handling so if lm80_update_device fails
> an error is returned when reading the value through sysfs
> This is closely modeled after the way this is handled in ltc4261
> 
> Note: if update gets an error, this error is returned for all
> registers and no further reads are done.
> If a register is not accessible most likely the whole device
> is broken or disconnected and it is definitely interesting to know.
> 
> Note2: I added a macro to read and handle errors. This was felt to
> be the simplest. If someone feels it should be done using a
> subroutine, be my guest.
> 
Ok, I'll be your guest ;). See below for reasons.

> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> ---
> patch is against the hwmon staging tree
> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
> as retrieved on jan 2, 2012
> 
>  drivers/hwmon/lm80.c |   97 ++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 67 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/hwmon/lm80.c b/drivers/hwmon/lm80.c
> index 18a0e6c..5283b27 100644
> --- a/drivers/hwmon/lm80.c
> +++ b/drivers/hwmon/lm80.c
> @@ -168,6 +168,8 @@ static ssize_t show_in_##suffix(struct device *dev, struct device_attribute *att
>  { \
>  	int nr = to_sensor_dev_attr(attr)->index; \
>  	struct lm80_data *data = lm80_update_device(dev); \
> +	if (IS_ERR(data)) \
> +		return PTR_ERR(data); \
>  	return sprintf(buf, "%d\n", IN_FROM_REG(data->value[nr])); \
>  }
>  show_in(min, in_min)
> @@ -197,6 +199,8 @@ static ssize_t show_fan_##suffix(struct device *dev, struct device_attribute *at
>  { \
>  	int nr = to_sensor_dev_attr(attr)->index; \
>  	struct lm80_data *data = lm80_update_device(dev); \
> +	if (IS_ERR(data)) \
> +		return PTR_ERR(data); \
>  	return sprintf(buf, "%d\n", FAN_FROM_REG(data->value[nr], \
>  		       DIV_FROM_REG(data->fan_div[nr]))); \
>  }
> @@ -208,6 +212,8 @@ static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
>  {
>  	int nr = to_sensor_dev_attr(attr)->index;
>  	struct lm80_data *data = lm80_update_device(dev);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
>  	return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]));
>  }
>  
> @@ -271,6 +277,8 @@ static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
>  static ssize_t show_temp_input1(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct lm80_data *data = lm80_update_device(dev);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
>  	return sprintf(buf, "%ld\n", TEMP_FROM_REG(data->temp));
>  }
>  
> @@ -278,6 +286,8 @@ static ssize_t show_temp_input1(struct device *dev, struct device_attribute *att
>  static ssize_t show_temp_##suffix(struct device *dev, struct device_attribute *attr, char *buf) \
>  { \
>  	struct lm80_data *data = lm80_update_device(dev); \
> +	if (IS_ERR(data)) \
> +		return PTR_ERR(data); \
>  	return sprintf(buf, "%d\n", TEMP_LIMIT_FROM_REG(data->value)); \
>  }
>  show_temp(hot_max, temp_hot_max);
> @@ -308,6 +318,8 @@ static ssize_t show_alarms(struct device *dev, struct device_attribute *attr,
>  			   char *buf)
>  {
>  	struct lm80_data *data = lm80_update_device(dev);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
>  	return sprintf(buf, "%u\n", data->alarms);
>  }
>  
> @@ -316,6 +328,8 @@ static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
>  {
>  	int bitnr = to_sensor_dev_attr(attr)->index;
>  	struct lm80_data *data = lm80_update_device(dev);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
>  	return sprintf(buf, "%u\n", (data->alarms >> bitnr) & 1);
>  }
>  
> @@ -544,55 +558,78 @@ static void lm80_init_client(struct i2c_client *client)
>  	lm80_write_value(client, LM80_REG_CONFIG, 0x01);
>  }
>  
> +/*
> +   Note: safe_lm80_read_value is a helper macro for lm80_update_device
> +	 It reads a value using lm80_read_value and jumps to abort
> +	 in case of an error.
> +	 Due to this jump and because it modifies the first arguement

s/arguement/argument/

> +	 it has to be a macro

This is exactly the reason why it should not be a macro: It uses two variables
defined outside the macro, it depends on a label defined outside the macro,
and it modifies a variable passed to it. Way too many side effects for a macro,
and just asking for trouble later on.

Sure, I understand you want to limit the changes to lm80_update_device, but you'll have
to bite the bullet here. Just make it

	reg = lm80_read_value(client, <reg>);
	if (reg < 0) {
		ret = ERR_PTR(reg);
		data->valid = 0;
		goto abort;
	}
	data-><regstore> = reg;

I don't think the dev_dbg is really needed, since the error will now be returned
to the user anyway.

> +*/
> +#define safe_lm80_read_value(var, client, reg) \
> +	{ \
> +		status = lm80_read_value(client, reg); \
> +		if (unlikely(status < 0)) { \
> +			dev_dbg(dev, \
> +				"LM80: Failed to read value: reg %d, error %d\n", \
> +				reg, status); \
> +			ret = ERR_PTR(status); \
> +			data->valid = 0; \
> +			goto abort; \
> +		} \
> +		else \
> +			var = status; \

else statement is not needed.
 
> +	} 
> +
>  static struct lm80_data *lm80_update_device(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm80_data *data = i2c_get_clientdata(client);
> +	struct lm80_data *ret = data;
>  	int i;
> +	int status;
> +	int tmp1, tmp2;
>  
>  	mutex_lock(&data->update_lock);
>  
> +	if (!data->valid)
> +	{
> +		/* last time failed: re-initialize the LM80 chip */
> +		lm80_init_client(client);
> +	}

{ } not needed, and at wrong location anyway (CodingStyle, chapter 3). This also duplicates
2st-time initialization. One way to avoid that would be to add a second state variable
(such as "error") to cover this case. Might be worth thinking about. Also, please add
an explanation (from your followup e-mail) describing why you do this.

On a side note, this means the chip will only get (re-)initialized if polled again.
Is that ok for your application ? Not that any alternatives would be easy to implement - a cleaner
solution would be for your system to detect that the device was pulled, unload the driver if that
happens, and reload it once the device is re-inserted. At least this is how we handle it in our
systems.

>  	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
>  		dev_dbg(&client->dev, "Starting lm80 update\n");
>  		for (i = 0; i <= 6; i++) {
> -			data->in[i] > -			    lm80_read_value(client, LM80_REG_IN(i));
> -			data->in_min[i] > -			    lm80_read_value(client, LM80_REG_IN_MIN(i));
> -			data->in_max[i] > -			    lm80_read_value(client, LM80_REG_IN_MAX(i));
> +			safe_lm80_read_value(data->in[i], client, LM80_REG_IN(i));
> +			safe_lm80_read_value(data->in_min[i], client, LM80_REG_IN_MIN(i));
> +			safe_lm80_read_value(data->in_max[i], client, LM80_REG_IN_MAX(i));
>  		}
> -		data->fan[0] = lm80_read_value(client, LM80_REG_FAN1);
> -		data->fan_min[0] > -		    lm80_read_value(client, LM80_REG_FAN_MIN(1));
> -		data->fan[1] = lm80_read_value(client, LM80_REG_FAN2);
> -		data->fan_min[1] > -		    lm80_read_value(client, LM80_REG_FAN_MIN(2));
> -
> -		data->temp > -		    (lm80_read_value(client, LM80_REG_TEMP) << 8) |
> -		    (lm80_read_value(client, LM80_REG_RES) & 0xf0);
> -		data->temp_os_max > -		    lm80_read_value(client, LM80_REG_TEMP_OS_MAX);
> -		data->temp_os_hyst > -		    lm80_read_value(client, LM80_REG_TEMP_OS_HYST);
> -		data->temp_hot_max > -		    lm80_read_value(client, LM80_REG_TEMP_HOT_MAX);
> -		data->temp_hot_hyst > -		    lm80_read_value(client, LM80_REG_TEMP_HOT_HYST);
> -
> -		i = lm80_read_value(client, LM80_REG_FANDIV);
> +		safe_lm80_read_value(data->fan[0], client, LM80_REG_FAN1);
> +		safe_lm80_read_value(data->fan_min[0], client, LM80_REG_FAN_MIN(1));
> +		safe_lm80_read_value(data->fan[1], client, LM80_REG_FAN2);
> +		safe_lm80_read_value(data->fan_min[1], client, LM80_REG_FAN_MIN(2));
> +
> +		safe_lm80_read_value(tmp1, client, LM80_REG_TEMP);
> +		safe_lm80_read_value(tmp2, client, LM80_REG_RES);
> +		data->temp = (tmp1 << 8) | (tmp2 ^ 0xf0);
> +
> +		safe_lm80_read_value(data->temp_os_max, client, LM80_REG_TEMP_OS_MAX);
> +		safe_lm80_read_value(data->temp_os_hyst, client, LM80_REG_TEMP_OS_HYST);
> +		safe_lm80_read_value(data->temp_hot_max, client, LM80_REG_TEMP_HOT_MAX);
> +		safe_lm80_read_value(data->temp_hot_hyst, client, LM80_REG_TEMP_HOT_HYST);
> +
> +		safe_lm80_read_value(i, client, LM80_REG_FANDIV);
>  		data->fan_div[0] = (i >> 2) & 0x03;
>  		data->fan_div[1] = (i >> 4) & 0x03;
> -		data->alarms = lm80_read_value(client, LM80_REG_ALARM1) +
> -		    (lm80_read_value(client, LM80_REG_ALARM2) << 8);
> +		safe_lm80_read_value(tmp1, client, LM80_REG_ALARM1);
> +		safe_lm80_read_value(tmp2, client, LM80_REG_ALARM2);
> +		data->alarms = tmp1 + (tmp2 << 8);
>  		data->last_updated = jiffies;
>  		data->valid = 1;
>  	}
>  
> +abort:
>  	mutex_unlock(&data->update_lock);
> -
> -	return data;
> +	return ret;
>  }
>  
>  static int __init sensors_lm80_init(void)
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Guenter Roeck]

On Mon, Jan 02, 2012 at 06:03:22AM -0500, Frans Meulenbroeks wrote:
> added error handling so if lm80_update_device fails
> an error is returned when reading the value through sysfs
> This is closely modeled after the way this is handled in ltc4261
> 
> Note: if update gets an error, this error is returned for all
> registers and no further reads are done.
> If a register is not accessible most likely the whole device
> is broken or disconnected and it is definitely interesting to know.
> 
> Note2: I added a macro to read and handle errors. This was felt to
> be the simplest. If someone feels it should be done using a
> subroutine, be my guest.
> 
> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>

Another note: checkpatch.pl reports 2 errors and 10 warnings on this patch.

Guenter

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 4942 bytes --]

Hi Guenter,

Thanks for your replies.

Let me answer all of your replies in one go:

lm75.c: will fix

checkpatch issue: I used checkfile, not checkpatch (actually I was unaware
of checkpatch -f and without -f it does say it is not a unified diff and no
other messages)
checkpatch -f indeed reports errors, I'll give this another stab.

lm80 comments:

2012/1/2 Guenter Roeck <guenter.roeck@ericsson.com>
[...]


> >
> > +/*
> > +   Note: safe_lm80_read_value is a helper macro for lm80_update_device
> > +      It reads a value using lm80_read_value and jumps to abort
> > +      in case of an error.
> > +      Due to this jump and because it modifies the first arguement
>
>        return sprintf(buf, "%d\n", IN_FROM_REG(data->value[nr])); \
> s/arguement/argument/
>
> > +      it has to be a macro
>
> This is exactly the reason why it should not be a macro: It uses two
> variables
> defined outside the macro, it depends on a label defined outside the macro,
> and it modifies a variable passed to it. Way too many side effects for a
> macro,
> and just asking for trouble later on.
>

I am aware of that. That is why I added the comment and kept the macro
close to the function.

>
> Sure, I understand you want to limit the changes to lm80_update_device,
> but you'll have
> to bite the bullet here. Just make it
>
>        reg = lm80_read_value(client, <reg>);
>        if (reg < 0) {
>                ret = ERR_PTR(reg);
>                data->valid = 0;
>                goto abort;
>        }
>        data-><regstore> = reg;
>
> I don't think the dev_dbg is really needed, since the error will now be
> returned
> to the user anyway.
>

I've thought about that before writing the macro. Issue is that this code
snippet will be about 16 times in this file. That does not really make
things better readable and maintainable.

If something like
rv = helperfunc(client, reg1, valptr1);
if (!rv)
   rv = helperfunc(client, reg2, valptr2);
if (!rv)
   rv = helperfunc(client, reg3, valptr3);

is ok, I think I could come up with a small helper func and avoid that
things become too wieldy and unreadable.
Is this acceptable wrt coding style.
(and maybe it has to be if (rv < 0) or so but I hope you get the idea)

>
> > +*/
> > +#define safe_lm80_read_value(var, client, reg) \
> > +     { \
> > +             status = lm80_read_value(client, reg); \
> > +             if (unlikely(status < 0)) { \
> > +                     dev_dbg(dev, \
> > +                             "LM80: Failed to read value: reg %d, error
> %d\n", \
> > +                             reg, status); \
> > +                     ret = ERR_PTR(status); \
> > +                     data->valid = 0; \
> > +                     goto abort; \
> > +             } \
> > +             else \
> > +                     var = status; \
>
> else statement is not needed.
>
OK

>
> > +     }
> > +
> >  static struct lm80_data *lm80_update_device(struct device *dev)
> >  {
> >       struct i2c_client *client = to_i2c_client(dev);
> >       struct lm80_data *data = i2c_get_clientdata(client);
> > +     struct lm80_data *ret = data;
> >       int i;
> > +     int status;
> > +     int tmp1, tmp2;
> >
> >       mutex_lock(&data->update_lock);
> >
> > +     if (!data->valid)
> > +     {
> > +             /* last time failed: re-initialize the LM80 chip */
> > +             lm80_init_client(client);
> > +     }
>
> { } not needed, and at wrong location anyway (CodingStyle, chapter 3).
> This also duplicates
> 2st-time initialization. One way to avoid that would be to add a second
> state variable
> (such as "error") to cover this case. Might be worth thinking about. Also,
> please add
> an explanation (from your followup e-mail) describing why you do this.
>

Will do

>
> On a side note, this means the chip will only get (re-)initialized if
> polled again.
> Is that ok for your application ? Not that any alternatives would be easy
> to implement - a cleaner
> solution would be for your system to detect that the device was pulled,
> unload the driver if that
> happens, and reload it once the device is re-inserted. At least this is
> how we handle it in our
> systems.
>

Hm, yes.
For our app this is ok. I poll the device every second.
And as it stands there is not really a mechanism to detect that the device
was pulled (apart from read operations to the device failing) (at least not
in our system).

In our system the LM80 is with the fan unit and if the fan unit is removed
also the LM80 is disconnected. I2C is of course not hot pluggable, and
actually we state in the documentation that fans are not hot swappable, but
we want to be able to properly deal with the situation of a user does (as
irrespective of what the manual says, someone is going to do this anyway).

If you feel there is a better solution without changing the hardware
interface, I'd like to hear from you as I am always eager to improve things.

Best regards, Frans

[...]

[-- Attachment #1.2: Type: text/html, Size: 6468 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Guenter Roeck]

Hi Frans,

On Tue, 2012-01-03 at 05:32 -0500, Frans Meulenbroeks wrote:
> Hi Guenter,
> 
> Thanks for your replies.
> 
> Let me answer all of your replies in one go:
> 
> lm75.c: will fix
> 
> checkpatch issue: I used checkfile, not checkpatch (actually I was
> unaware of checkpatch -f and without -f it does say it is not a
> unified diff and no other messages)
> checkpatch -f indeed reports errors, I'll give this another stab. 
> 
Where do you get checkfile from ? Doesn't seem to exist, at least not in
the latest kernel.

> lm80 comments:
> 
> 2012/1/2 Guenter Roeck <guenter.roeck@ericsson.com>
> [...]
>  
> 
>         >
>         > +/*
>         > +   Note: safe_lm80_read_value is a helper macro for
>         lm80_update_device
>         > +      It reads a value using lm80_read_value and jumps to
>         abort
>         > +      in case of an error.
>         > +      Due to this jump and because it modifies the first
>         arguement
>         
>         
>                return sprintf(buf, "%d\n",
>         IN_FROM_REG(data->value[nr])); \
>         s/arguement/argument/
>         
>         > +      it has to be a macro
>         
>         
>         This is exactly the reason why it should not be a macro: It
>         uses two variables
>         defined outside the macro, it depends on a label defined
>         outside the macro,
>         and it modifies a variable passed to it. Way too many side
>         effects for a macro,
>         and just asking for trouble later on.
> 
> I am aware of that. That is why I added the comment and kept the macro
> close to the function. 
> 
>         
>         Sure, I understand you want to limit the changes to
>         lm80_update_device, but you'll have
>         to bite the bullet here. Just make it
>         
>                reg = lm80_read_value(client, <reg>);
>                if (reg < 0) {
>                        ret = ERR_PTR(reg);
>                        data->valid = 0;
>                        goto abort;
>                }
>                data-><regstore> = reg;
>         
>         I don't think the dev_dbg is really needed, since the error
>         will now be returned
>         to the user anyway.
> 
> I've thought about that before writing the macro. Issue is that this
> code snippet will be about 16 times in this file. That does not really
> make things better readable and maintainable.
> 
> If something like
> rv = helperfunc(client, reg1, valptr1);
> if (!rv) 
>    rv = helperfunc(client, reg2, valptr2);
> if (!rv) 
>    rv = helperfunc(client, reg3, valptr3);
> 
> is ok, I think I could come up with a small helper func and avoid that
> things become too wieldy and unreadable.
> Is this acceptable wrt coding style.
> (and maybe it has to be if (rv < 0) or so but I hope you get the idea)
> 
How about something like

	rv = helperfunc(client, reg, &val);
	if (rv)
		goto abort;
	...
	goto done;

abort:
	ret = ERR_PTR(rv);
	data->valid = 0;
done:
	mutex_unlock(&data->update_lock);
	return ret;

or

	rv = read(client, reg);
	if (rv < 0)
		goto abort;
	storage = rv;

followed by the rest of the code above, to avoid the helper function.

[ ... ]

> 
>         
>         On a side note, this means the chip will only get
>         (re-)initialized if polled again.
>         Is that ok for your application ? Not that any alternatives
>         would be easy to implement - a cleaner
>         solution would be for your system to detect that the device
>         was pulled, unload the driver if that
>         happens, and reload it once the device is re-inserted. At
>         least this is how we handle it in our
>         systems.
> 
> Hm, yes.
> For our app this is ok. I poll the device every second.
> And as it stands there is not really a mechanism to detect that the
> device was pulled (apart from read operations to the device failing)
> (at least not in our system).
> 
> In our system the LM80 is with the fan unit and if the fan unit is
> removed also the LM80 is disconnected. I2C is of course not hot
> pluggable, and actually we state in the documentation that fans are
> not hot swappable, but we want to be able to properly deal with the
> situation of a user does (as irrespective of what the manual says,
> someone is going to do this anyway).
> 
> If you feel there is a better solution without changing the hardware
> interface, I'd like to hear from you as I am always eager to improve
> things.
> 

Depends on how your polling works. You might do something like

    while (true) {
	error = poll_lm80();
	if (error) {
		scream and log event;
		unload lm80 driver;
		do {
			sleep(1);
			error = read_i2c_register(bus, addr, reg);
			// use i2cget if this is a script
		}  while (error);
		load lm80 driver;
	}
	sleep(1);
    }

Of course that only works if you only have a single lm80 in your system,
and if you build the lm80 driver as module.

Would this work for you ?

Thanks,
Guenter



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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 4814 bytes --]

2012/1/3 Guenter Roeck <guenter.roeck@ericsson.com>

> Hi Frans,
>
> On Tue, 2012-01-03 at 05:32 -0500, Frans Meulenbroeks wrote:
> > Hi Guenter,
> >
> > Thanks for your replies.
> >
> > Let me answer all of your replies in one go:
> >
> > lm75.c: will fix
> >
> > checkpatch issue: I used checkfile, not checkpatch (actually I was
> > unaware of checkpatch -f and without -f it does say it is not a
> > unified diff and no other messages)
> > checkpatch -f indeed reports errors, I'll give this another stab.
> >
> Where do you get checkfile from ? Doesn't seem to exist, at least not in
> the latest kernel.
>

Misremembered the name (and didn't look it up when typing the message).
The script I used was cleanfile not checkfile

(and apologies for the mistakes, I'm still fairly new at the process of
submitting patches; thanks for your patience).

>
> .
> >
> > If something like
> > rv = helperfunc(client, reg1, valptr1);
> > if (!rv)
> >    rv = helperfunc(client, reg2, valptr2);
> > if (!rv)
> >    rv = helperfunc(client, reg3, valptr3);
> >
> > is ok, I think I could come up with a small helper func and avoid that
> > things become too wieldy and unreadable.
> > Is this acceptable wrt coding style.
> > (and maybe it has to be if (rv < 0) or so but I hope you get the idea)
> >
> How about something like
>
>        rv = helperfunc(client, reg, &val);
>        if (rv)
>                goto abort;
>        ...
>        goto done;
>
> abort:
>        ret = ERR_PTR(rv);
>        data->valid = 0;
> done:
>        mutex_unlock(&data->update_lock);
>        return ret;
>
> or
>
>        rv = read(client, reg);
>        if (rv < 0)
>                goto abort;
>        storage = rv;
>
> followed by the rest of the code above, to avoid the helper function.
>

Yeah, this is probably the simplest.
It would requre some temporary vars as there is code form the form temp =
(read(client, reg1) << 8) | read(client, reg2)
but that is no problem.

Having been educated by Edsger "goto considered harmful" Dijkstra, I have
some internal barriers against the use of goto, but  I think in this case I
can warrant it for myself.

>
> [ ... ]
>
> >
> >
> >         On a side note, this means the chip will only get
> >         (re-)initialized if polled again.
> >         Is that ok for your application ? Not that any alternatives
> >         would be easy to implement - a cleaner
> >         solution would be for your system to detect that the device
> >         was pulled, unload the driver if that
> >         happens, and reload it once the device is re-inserted. At
> >         least this is how we handle it in our
> >         systems.
> >
> > Hm, yes.
> > For our app this is ok. I poll the device every second.
> > And as it stands there is not really a mechanism to detect that the
> > device was pulled (apart from read operations to the device failing)
> > (at least not in our system).
> >
> > In our system the LM80 is with the fan unit and if the fan unit is
> > removed also the LM80 is disconnected. I2C is of course not hot
> > pluggable, and actually we state in the documentation that fans are
> > not hot swappable, but we want to be able to properly deal with the
> > situation of a user does (as irrespective of what the manual says,
> > someone is going to do this anyway).
> >
> > If you feel there is a better solution without changing the hardware
> > interface, I'd like to hear from you as I am always eager to improve
> > things.
> >
>
> Depends on how your polling works. You might do something like
>
>    while (true) {
>        error = poll_lm80();
>        if (error) {
>                scream and log event;
>                unload lm80 driver;
>                do {
>                        sleep(1);
>                        error = read_i2c_register(bus, addr, reg);
>                        // use i2cget if this is a script
>                }  while (error);
>                load lm80 driver;
>        }
>        sleep(1);
>    }
>
> Of course that only works if you only have a single lm80 in your system,
> and if you build the lm80 driver as module.
>

I've been pondering about using a module (currnently we don't). Also there
might (and probably will) be two lm80's in the system.

solution could be to unload and reload the driver when I detect the chip is
back again. Then again I'm not 100% sure if read_i2c_register works for me,
as I am on powerpc having this chip mapped in the device tree.
Haven't tried it with an lm sensor, but noticed that mapped gpio chips are
locked by the system and cannot be called using i2c-get


As we poll the device anyway, I guess I'm best off leaving things as they
are. (with reinitialisation if data->valid == 0), taking also into account
that this is not supposed to happen anyway.

Thanks for your suggestions!
Frans

[-- Attachment #1.2: Type: text/html, Size: 6110 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Guenter Roeck]

On Tue, 2012-01-03 at 17:17 -0500, Frans Meulenbroeks wrote:
> 
> 
> 2012/1/3 Guenter Roeck <guenter.roeck@ericsson.com>
>         Hi Frans,
>         
>         On Tue, 2012-01-03 at 05:32 -0500, Frans Meulenbroeks wrote:
>         > Hi Guenter,
>         >
>         > Thanks for your replies.
>         >
>         > Let me answer all of your replies in one go:
>         >
>         > lm75.c: will fix
>         >
>         > checkpatch issue: I used checkfile, not checkpatch (actually
>         I was
>         > unaware of checkpatch -f and without -f it does say it is
>         not a
>         > unified diff and no other messages)
>         > checkpatch -f indeed reports errors, I'll give this another
>         stab.
>         >
>         
>         Where do you get checkfile from ? Doesn't seem to exist, at
>         least not in
>         the latest kernel.
> 
> Misremembered the name (and didn't look it up when typing the
> message).
> The script I used was cleanfile not checkfile
> 
> (and apologies for the mistakes, I'm still fairly new at the process
> of submitting patches; thanks for your patience). 
> 
You are doing pretty well ...

>         
>         .
>         >
>         > If something like
>         > rv = helperfunc(client, reg1, valptr1);
>         > if (!rv)
>         >    rv = helperfunc(client, reg2, valptr2);
>         > if (!rv)
>         >    rv = helperfunc(client, reg3, valptr3);
>         >
>         > is ok, I think I could come up with a small helper func and
>         avoid that
>         > things become too wieldy and unreadable.
>         > Is this acceptable wrt coding style.
>         > (and maybe it has to be if (rv < 0) or so but I hope you get
>         the idea)
>         >
>         
>         How about something like
>         
>                rv = helperfunc(client, reg, &val);
>                if (rv)
>                        goto abort;
>                ...
>                goto done;
>         
>         abort:
>                ret = ERR_PTR(rv);
>                data->valid = 0;
>         done:
>                mutex_unlock(&data->update_lock);
>                return ret;
>         
>         or
>         
>                rv = read(client, reg);
>                if (rv < 0)
>                        goto abort;
>                storage = rv;
>         
>         followed by the rest of the code above, to avoid the helper
>         function.
> 
> Yeah, this is probably the simplest. 
> It would requre some temporary vars as there is code form the form
> temp = (read(client, reg1) << 8) | read(client, reg2)
> but that is no problem.
> 
> Having been educated by Edsger "goto considered harmful" Dijkstra, I
> have some internal barriers against the use of goto, but  I think in
> this case I can warrant it for myself.
> 
See CodingStyle, Chapter 7. Or, in other words: When travelling to Rome,
behave like a Roman ;).

>         
>         [ ... ]
>         
>         >
>         >
>         >         On a side note, this means the chip will only get
>         >         (re-)initialized if polled again.
>         >         Is that ok for your application ? Not that any
>         alternatives
>         >         would be easy to implement - a cleaner
>         >         solution would be for your system to detect that the
>         device
>         >         was pulled, unload the driver if that
>         >         happens, and reload it once the device is
>         re-inserted. At
>         >         least this is how we handle it in our
>         >         systems.
>         >
>         > Hm, yes.
>         > For our app this is ok. I poll the device every second.
>         > And as it stands there is not really a mechanism to detect
>         that the
>         > device was pulled (apart from read operations to the device
>         failing)
>         > (at least not in our system).
>         >
>         > In our system the LM80 is with the fan unit and if the fan
>         unit is
>         > removed also the LM80 is disconnected. I2C is of course not
>         hot
>         > pluggable, and actually we state in the documentation that
>         fans are
>         > not hot swappable, but we want to be able to properly deal
>         with the
>         > situation of a user does (as irrespective of what the manual
>         says,
>         > someone is going to do this anyway).
>         >
>         > If you feel there is a better solution without changing the
>         hardware
>         > interface, I'd like to hear from you as I am always eager to
>         improve
>         > things.
>         >
>         
>         
>         Depends on how your polling works. You might do something like
>         
>            while (true) {
>                error = poll_lm80();
>                if (error) {
>                        scream and log event;
>                        unload lm80 driver;
>                        do {
>                                sleep(1);
>                                error = read_i2c_register(bus, addr,
>         reg);
>                                // use i2cget if this is a script
>                        }  while (error);
>                        load lm80 driver;
>                }
>                sleep(1);
>            }
>         
>         Of course that only works if you only have a single lm80 in
>         your system,
>         and if you build the lm80 driver as module.
> 
> I've been pondering about using a module (currnently we don't). Also
> there might (and probably will) be two lm80's in the system.
> 
> solution could be to unload and reload the driver when I detect the
> chip is back again. Then again I'm not 100% sure if read_i2c_register
> works for me, as I am on powerpc having this chip mapped in the device
> tree.
> Haven't tried it with an lm sensor, but noticed that mapped gpio chips
> are locked by the system and cannot be called using i2c-get
> 
i2cget from i2c-tools has a -f option to force reading registers even if
the device is already open.


> 
> As we poll the device anyway, I guess I'm best off leaving things as
> they are. (with reinitialisation if data->valid = 0), taking also
> into account that this is not supposed to happen anyway.
> 
Guess so, only use an error flag instead of data->valid to avoid
initializing the chip multiple times on startup, and remember to explain
the reason.

Thanks,
Guenter



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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Jean Delvare]

Hi Frans,

On Mon,  2 Jan 2012 12:03:22 +0100, Frans Meulenbroeks wrote:
> added error handling so if lm80_update_device fails
> an error is returned when reading the value through sysfs
> This is closely modeled after the way this is handled in ltc4261
> (...)

Are you really still using  LM80 chips? These are pretty old and I did
not expect anyone to be still using them. And certainly not for new
designs... The device (at least the incarnations that existed in
1998-2000) was not reputed for its reliability...

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 826 bytes --]

2012/1/5 Jean Delvare <khali@linux-fr.org>

> Hi Frans,
>
> On Mon,  2 Jan 2012 12:03:22 +0100, Frans Meulenbroeks wrote:
> > added error handling so if lm80_update_device fails
> > an error is returned when reading the value through sysfs
> > This is closely modeled after the way this is handled in ltc4261
> > (...)
>
> Are you really still using  LM80 chips? These are pretty old and I did
> not expect anyone to be still using them. And certainly not for new
> designs... The device (at least the incarnations that existed in
> 1998-2000) was not reputed for its reliability...
>
> Yes, we still have them and I need to support them (just like LM75).
(acutally we're using LM96080). They are not on my board, but we have
external fan units that contain these chips, and my board needs to support
these fan units.

Frans.

[-- Attachment #1.2: Type: text/html, Size: 1217 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[Jean Delvare]

On Thu, 5 Jan 2012 15:52:41 +0100, Frans Meulenbroeks wrote:
> 2012/1/5 Jean Delvare <khali@linux-fr.org>
> > Are you really still using  LM80 chips? These are pretty old and I did
> > not expect anyone to be still using them. And certainly not for new
> > designs... The device (at least the incarnations that existed in
> > 1998-2000) was not reputed for its reliability...
>
> Yes, we still have them and I need to support them (just like LM75).
> (acutally we're using LM96080). They are not on my board, but we have
> external fan units that contain these chips, and my board needs to support
> these fan units.

Frans, could you please provide a register dump of your LM96080 chip?
I'd like to add that to my collection. You can get such a dump using
the i2c-dev driver + i2cdump (from package i2c-tools.)

Thanks,
-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 1078 bytes --]

2012/1/26 Jean Delvare <khali@linux-fr.org>

> On Thu, 5 Jan 2012 15:52:41 +0100, Frans Meulenbroeks wrote:
> > 2012/1/5 Jean Delvare <khali@linux-fr.org>
> > > Are you really still using  LM80 chips? These are pretty old and I did
> > > not expect anyone to be still using them. And certainly not for new
> > > designs... The device (at least the incarnations that existed in
> > > 1998-2000) was not reputed for its reliability...
> >
> > Yes, we still have them and I need to support them (just like LM75).
> > (acutally we're using LM96080). They are not on my board, but we have
> > external fan units that contain these chips, and my board needs to
> support
> > these fan units.
>
> Frans, could you please provide a register dump of your LM96080 chip?
> I'd like to add that to my collection. You can get such a dump using
> the i2c-dev driver + i2cdump (from package i2c-tools.)
>
> I will try to do so tomorrow.
Please remind me if I haven't done so in a few days as I probably have
forgotten it (I'm currently in a "product-is-about-to-be-released" stress
:-)

Frans

[-- Attachment #1.2: Type: text/html, Size: 1492 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 2950 bytes --]

2012/1/26 Jean Delvare <khali@linux-fr.org>

> On Thu, 5 Jan 2012 15:52:41 +0100, Frans Meulenbroeks wrote:
> > 2012/1/5 Jean Delvare <khali@linux-fr.org>
> > > Are you really still using  LM80 chips? These are pretty old and I did
> > > not expect anyone to be still using them. And certainly not for new
> > > designs... The device (at least the incarnations that existed in
> > > 1998-2000) was not reputed for its reliability...
> >
> > Yes, we still have them and I need to support them (just like LM75).
> > (acutally we're using LM96080). They are not on my board, but we have
> > external fan units that contain these chips, and my board needs to
> support
> > these fan units.
>
> Frans, could you please provide a register dump of your LM96080 chip?
> I'd like to add that to my collection. You can get such a dump using
> the i2c-dev driver + i2cdump (from package i2c-tools.)
>
> Thanks,
>
> Hi Jean,

I hope this is what you are looking for:

root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028# cat
fan1_input
3668
root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028#
i2cdump  1 0x28
No size specified (using byte-data access)
Error: Could not set address to 0x28: Device or resource busy
root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028#
i2cdump -f 1 0x28
No size specified (using byte-data access)
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-1, address 0x28, mode byte
Continue? [Y/n] y
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 01 7f 2d 00 00 3c 18 00 00 00 00 00 00 00 00 00    ??-..<?.........
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
20: 0c 00 00 00 00 00 00 1a 2e ff 00 00 00 00 00 00    ?......?........
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 08    ..............??
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

Not fully sure if this is correct as I had to use -f
Also fan1_input is 3668, which is hex 0e54 but I do not see that value in
the dump.

Feel free to ping me if there is other info you need.

Best regards, Frans

[-- Attachment #1.2: Type: text/html, Size: 3540 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[Jean Delvare]

On Fri, 27 Jan 2012 14:22:00 +0100, Frans Meulenbroeks wrote:
> 2012/1/26 Jean Delvare <khali@linux-fr.org>
> > Frans, could you please provide a register dump of your LM96080 chip?
> > I'd like to add that to my collection. You can get such a dump using
> > the i2c-dev driver + i2cdump (from package i2c-tools.)
> 
> I hope this is what you are looking for:
> 
> root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028# cat
> fan1_input
> 3668
> root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028#
> i2cdump  1 0x28
> No size specified (using byte-data access)
> Error: Could not set address to 0x28: Device or resource busy
> root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028#
> i2cdump -f 1 0x28
> No size specified (using byte-data access)
> WARNING! This program can confuse your I2C bus, cause data loss and worse!
> I will probe file /dev/i2c-1, address 0x28, mode byte
> Continue? [Y/n] y
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 01 7f 2d 00 00 3c 18 00 00 00 00 00 00 00 00 00    ??-..<?.........
> 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 20: 0c 00 00 00 00 00 00 1a 2e ff 00 00 00 00 00 00    ?......?........
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 08    ..............??
> 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 
> Not fully sure if this is correct as I had to use -f

That's perfect, thank you.

> Also fan1_input is 3668, which is hex 0e54 but I do not see that value in
> the dump.

As is the case of most monitoring chips, the LM96080 doesn't measure
the fan speed directly. Instead, it measures how much time it takes to
the fan to spin once, and this time is expressed in units of an
internal clock (22.5 kHz). The relevant register is 0x28, value is 0x2e
or 46. The speed is computed as:

RPM = (22.5k * 60) / (register value * divisor)

Where the divisor is 1, 2, 4 or 8, encoded in register 0x05. In your
case the divisor is 8, which leads to :

22.5 *  1000 * 60 / (46 * 8) = 3668 RPM

As a side note, a divisor of 4 or even 2 would yield better speed
measurement accuracy in your case (i.e. you can set fan1_div to 2 in
your configuration file.)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 3/4] drivers/hwmon/lm80.c: added error handling

[Frans Meulenbroeks]

[-- Attachment #1.1: Type: text/plain, Size: 3649 bytes --]

2012/1/28 Jean Delvare <khali@linux-fr.org>

> On Fri, 27 Jan 2012 14:22:00 +0100, Frans Meulenbroeks wrote:
> > 2012/1/26 Jean Delvare <khali@linux-fr.org>
> > > Frans, could you please provide a register dump of your LM96080 chip?
> > > I'd like to add that to my collection. You can get such a dump using
> > > the i2c-dev driver + i2cdump (from package i2c-tools.)
> >
> > I hope this is what you are looking for:
> >
> > root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028# cat
> > fan1_input
> > 3668
> > root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028#
> > i2cdump  1 0x28
> > No size specified (using byte-data access)
> > Error: Could not set address to 0x28: Device or resource busy
> > root@(none):/sys/devices/e0000000.soc8313/e0003100.i2c/i2c-1/1-0028#
> > i2cdump -f 1 0x28
> > No size specified (using byte-data access)
> > WARNING! This program can confuse your I2C bus, cause data loss and
> worse!
> > I will probe file /dev/i2c-1, address 0x28, mode byte
> > Continue? [Y/n] y
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> > 00: 01 7f 2d 00 00 3c 18 00 00 00 00 00 00 00 00 00    ??-..<?.........
> > 10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > 20: 0c 00 00 00 00 00 00 1a 2e ff 00 00 00 00 00 00    ?......?........
> > 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 08    ..............??
> > 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> > f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> >
> > Not fully sure if this is correct as I had to use -f
>
> That's perfect, thank you.
>
> > Also fan1_input is 3668, which is hex 0e54 but I do not see that value in
> > the dump.
>
> As is the case of most monitoring chips, the LM96080 doesn't measure
> the fan speed directly. Instead, it measures how much time it takes to
> the fan to spin once, and this time is expressed in units of an
> internal clock (22.5 kHz). The relevant register is 0x28, value is 0x2e
> or 46. The speed is computed as:
>
> RPM = (22.5k * 60) / (register value * divisor)
>
> Where the divisor is 1, 2, 4 or 8, encoded in register 0x05. In your
> case the divisor is 8, which leads to :
>
> 22.5 *  1000 * 60 / (46 * 8) = 3668 RPM
>
> As a side note, a divisor of 4 or even 2 would yield better speed
> measurement accuracy in your case (i.e. you can set fan1_div to 2 in
> your configuration file.)
>
> --
>
> Thanks for the explanation.
Actually I guessed it would be something like this, but didn't get to
digging into it.
Wrt the accuracy. Might try that. Then again we do not bother too much as
we mainly want to monitor the fan for working and are not too interested in
the speed; I'm just monitoring if it does not fall below a treshold value
(and if it does raise an alarm indicating that preventive replacement is
needed.

Frans

[-- Attachment #1.2: Type: text/html, Size: 4442 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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