[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max,

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max,

[Jim Cromie]

implement separate min, max, alarms for each voltage input,
and min, max, crit alarms for temps.

Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
---
$ diffstat hwmon-pc87360-separate-alarms.patch
 pc87360.c |  181 
++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 141 insertions(+), 40 deletions(-)

patch is against 19-rc1, you're accepting separate-alarms patches for 
2.6.20 now ?

It passes this exersise:

#!/bin/bash
sensors -s
sensors
cd /sys/class/hwmon/hwmon0/device
function set_check {
    local lim=$1
    local setting=$2
    local want=$3
    echo $setting > $lim
    sleep 2
    local in=`cat ${lim}_alarm`
    [ $in = $want ] && echo ok setting $lim $setting alarm ${lim}_alarm $in
}
# in8_input is 1477
set_check in8_min 1500 1
set_check in8_min 1400 0
set_check in8_max 1400 1
set_check in8_max 1500 0
# temp3_input is 99000
set_check temp3_min 101000 1
set_check temp3_min 95000 0
set_check temp3_max 95000 1
set_check temp3_max 101000 0
set_check temp3_crit 95000 1
set_check temp3_crit 101000 0

The need for 'sleep 2' above made me think to reset data->valid when any 
setter callback is called.
I didnt add it, but it seems the right thing to do.  My preference is to 
add it into a forthcoming
2D-callback patch.



diff -ruNp -X dontdiff -X exclude-diffs 19rc1-0/drivers/hwmon/pc87360.c alarms-only/drivers/hwmon/pc87360.c
--- 19rc1-0/drivers/hwmon/pc87360.c	2006-10-05 20:17:51.000000000 -0600
+++ alarms-only/drivers/hwmon/pc87360.c	2006-10-08 14:43:01.000000000 -0600
@@ -144,15 +144,14 @@ static inline u8 PWM_TO_REG(int val, int
  * Voltage registers and conversions
  */
 
+#define PC87365_REG_VID			0x06
 #define PC87365_REG_IN_CONVRATE		0x07
 #define PC87365_REG_IN_CONFIG		0x08
+/* per-channel (0-13) registers */
+#define PC87365_REG_IN_STATUS		0x0A
 #define PC87365_REG_IN			0x0B
-#define PC87365_REG_IN_MIN		0x0D
 #define PC87365_REG_IN_MAX		0x0C
-#define PC87365_REG_IN_STATUS		0x0A
-#define PC87365_REG_IN_ALARMS1		0x00
-#define PC87365_REG_IN_ALARMS2		0x01
-#define PC87365_REG_VID			0x06
+#define PC87365_REG_IN_MIN		0x0D
 
 #define IN_FROM_REG(val,ref)		(((val) * (ref) + 128) / 256)
 #define IN_TO_REG(val,ref)		((val) < 0 ? 0 : \
@@ -205,7 +204,6 @@ struct pc87360_data {
 	u8 in_max[14];		/* Register value */
 	u8 in_crit[3];		/* Register value */
 	u8 in_status[14];	/* Register value */
-	u16 in_alarms;		/* Register values, combined, masked */
 	u8 vid_conf;		/* Configuration register value */
 	u8 vrm;
 	u8 vid;			/* Register value */
@@ -215,7 +213,6 @@ struct pc87360_data {
 	s8 temp_max[3];		/* Register value */
 	s8 temp_crit[3];	/* Register value */
 	u8 temp_status[3];	/* Register value */
-	u8 temp_alarms;		/* Register value, masked */
 };
 
 /*
@@ -495,7 +492,9 @@ static struct sensor_device_attribute in
 	&in_input[X].dev_attr.attr,	\
 	&in_status[X].dev_attr.attr,	\
 	&in_min[X].dev_attr.attr,	\
-	&in_max[X].dev_attr.attr
+	&in_max[X].dev_attr.attr,	\
+	&in_min_alarm[X].dev_attr.attr,	\
+	&in_max_alarm[X].dev_attr.attr
 
 static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -518,12 +517,45 @@ static ssize_t set_vrm(struct device *de
 }
 static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
 
-static ssize_t show_in_alarms(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
 {
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 4) >> 2); 
+}
+static struct sensor_device_attribute in_max_alarm[] = {
+	SENSOR_ATTR(in0_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 0),
+	SENSOR_ATTR(in1_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 1),
+	SENSOR_ATTR(in2_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 2),
+	SENSOR_ATTR(in3_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 3),
+	SENSOR_ATTR(in4_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 4),
+	SENSOR_ATTR(in5_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 5),
+	SENSOR_ATTR(in6_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 6),
+	SENSOR_ATTR(in7_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 7),
+	SENSOR_ATTR(in8_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 8),
+	SENSOR_ATTR(in9_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 9),
+	SENSOR_ATTR(in10_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 10),
+};
+
+static ssize_t show_in_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct pc87360_data *data = pc87360_update_device(dev);
-	return sprintf(buf, "%u\n", data->in_alarms);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 2) >> 1);
 }
-static DEVICE_ATTR(alarms_in, S_IRUGO, show_in_alarms, NULL);
+static struct sensor_device_attribute in_min_alarm[] = {
+	SENSOR_ATTR(in0_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 0),
+	SENSOR_ATTR(in1_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 1),
+	SENSOR_ATTR(in2_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 2),
+	SENSOR_ATTR(in3_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 2),
+	SENSOR_ATTR(in4_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 4),
+	SENSOR_ATTR(in5_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 5),
+	SENSOR_ATTR(in6_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 6),
+	SENSOR_ATTR(in7_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 7),
+	SENSOR_ATTR(in8_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 8),
+	SENSOR_ATTR(in9_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 9),
+	SENSOR_ATTR(in10_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 10),
+};
 
 static struct attribute *pc8736x_vin_attr_array[] = {
 	VIN_UNIT_ATTRS(0),
@@ -539,7 +571,6 @@ static struct attribute *pc8736x_vin_att
 	VIN_UNIT_ATTRS(10),
 	&dev_attr_cpu0_vid.attr,
 	&dev_attr_vrm.attr,
-	&dev_attr_alarms_in.attr,
 	NULL
 };
 static const struct attribute_group pc8736x_vin_group = {
@@ -664,12 +695,60 @@ static struct sensor_device_attribute th
 		    show_therm_crit, set_therm_crit, 2+11),
 };
 
+static ssize_t show_therm_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 2) >> 1); 
+}
+static struct sensor_device_attribute therm_min_alarm[] = {
+	SENSOR_ATTR(temp4_min_alarm, S_IRUGO,
+		    show_therm_min_alarm, NULL, 0+11),
+	SENSOR_ATTR(temp5_min_alarm, S_IRUGO,
+		    show_therm_min_alarm, NULL, 1+11),
+	SENSOR_ATTR(temp6_min_alarm, S_IRUGO,
+		    show_therm_min_alarm, NULL, 2+11),
+};
+
+static ssize_t show_therm_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 4) >> 2); 
+}
+static struct sensor_device_attribute therm_max_alarm[] = {
+	SENSOR_ATTR(temp4_max_alarm, S_IRUGO,
+		    show_therm_max_alarm, NULL, 0+11),
+	SENSOR_ATTR(temp5_max_alarm, S_IRUGO,
+		    show_therm_max_alarm, NULL, 1+11),
+	SENSOR_ATTR(temp6_max_alarm, S_IRUGO,
+		    show_therm_max_alarm, NULL, 2+11),
+};
+
+static ssize_t show_therm_crit_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 8) >> 3); 
+}
+static struct sensor_device_attribute therm_crit_alarm[] = {
+	SENSOR_ATTR(temp4_crit_alarm, S_IRUGO,
+		    show_therm_crit_alarm, NULL, 0+11),
+	SENSOR_ATTR(temp5_crit_alarm, S_IRUGO,
+		    show_therm_crit_alarm, NULL, 1+11),
+	SENSOR_ATTR(temp6_crit_alarm, S_IRUGO,
+		    show_therm_crit_alarm, NULL, 2+11),
+};
+
 #define THERM_UNIT_ATTRS(X) \
 	&therm_input[X].dev_attr.attr,	\
 	&therm_status[X].dev_attr.attr,	\
 	&therm_min[X].dev_attr.attr,	\
 	&therm_max[X].dev_attr.attr,	\
-	&therm_crit[X].dev_attr.attr
+	&therm_crit[X].dev_attr.attr,	\
+	&therm_min_alarm[X].dev_attr.attr, \
+	&therm_max_alarm[X].dev_attr.attr, \
+	&therm_crit_alarm[X].dev_attr.attr
 
 static struct attribute * pc8736x_therm_attr_array[] = {
 	THERM_UNIT_ATTRS(0),
@@ -792,26 +871,56 @@ static struct sensor_device_attribute te
 		    show_temp_crit, set_temp_crit, 2),
 };
 
-static ssize_t show_temp_alarms(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t show_temp_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
 {
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct pc87360_data *data = pc87360_update_device(dev);
-	return sprintf(buf, "%u\n", data->temp_alarms);
+	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 2) >> 1);
 }
-static DEVICE_ATTR(alarms_temp, S_IRUGO, show_temp_alarms, NULL);
+static struct sensor_device_attribute temp_min_alarm[] = {
+	SENSOR_ATTR(temp1_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 0),
+	SENSOR_ATTR(temp2_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 1),
+	SENSOR_ATTR(temp3_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 2),
+};
+
+static ssize_t show_temp_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 4) >> 2);
+}
+static struct sensor_device_attribute temp_max_alarm[] = {
+	SENSOR_ATTR(temp1_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 0),
+	SENSOR_ATTR(temp2_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 1),
+	SENSOR_ATTR(temp3_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 2),
+};
+
+static ssize_t show_temp_crit_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 8) >> 3);
+}
+static struct sensor_device_attribute temp_crit_alarm[] = {
+	SENSOR_ATTR(temp1_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 2),
+	SENSOR_ATTR(temp2_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 2),
+	SENSOR_ATTR(temp3_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 2),
+};
 
 #define TEMP_UNIT_ATTRS(X) \
 	&temp_input[X].dev_attr.attr,	\
 	&temp_status[X].dev_attr.attr,	\
 	&temp_min[X].dev_attr.attr,	\
 	&temp_max[X].dev_attr.attr,	\
-	&temp_crit[X].dev_attr.attr
+	&temp_crit[X].dev_attr.attr,	\
+	&temp_min_alarm[X].dev_attr.attr, \
+	&temp_max_alarm[X].dev_attr.attr, \
+	&temp_crit_alarm[X].dev_attr.attr
 
 static struct attribute * pc8736x_temp_attr_array[] = {
 	TEMP_UNIT_ATTRS(0),
 	TEMP_UNIT_ATTRS(1),
 	TEMP_UNIT_ATTRS(2),
-	/* include the few miscellaneous atts here */
-	&dev_attr_alarms_temp.attr,
 	NULL
 };
 static const struct attribute_group pc8736x_temp_group = {
@@ -1032,21 +1141,23 @@ static int pc87360_detect(struct i2c_ada
 
 	/* create device attr-files for varying sysfs groups */
 
-	if (data->tempnr) {
-		for (i = 0; i < data->tempnr; i++) {
-			if ((err = device_create_file(dev,
+	for (i = 0; i < data->tempnr; i++) {
+		if ((err = device_create_file(dev,
 					&temp_input[i].dev_attr))
-			    || (err = device_create_file(dev,
+		    || (err = device_create_file(dev,
 					&temp_min[i].dev_attr))
-			    || (err = device_create_file(dev,
+		    || (err = device_create_file(dev,
 					&temp_max[i].dev_attr))
-			    || (err = device_create_file(dev,
+		    || (err = device_create_file(dev,
 					&temp_crit[i].dev_attr))
-			    || (err = device_create_file(dev,
-					&temp_status[i].dev_attr)))
-				goto ERROR3;
-		}
-		if ((err = device_create_file(dev, &dev_attr_alarms_temp)))
+		    || (err = device_create_file(dev,
+					&temp_status[i].dev_attr))
+		    || (err = device_create_file(dev,
+					&temp_min_alarm[i].dev_attr))
+		    || (err = device_create_file(dev,
+					&temp_max_alarm[i].dev_attr))
+		    || (err = device_create_file(dev,
+					&temp_crit_alarm[i].dev_attr)))
 			goto ERROR3;
 	}
 
@@ -1367,11 +1478,6 @@ static struct pc87360_data *pc87360_upda
 			}
 		}
 		if (data->innr) {
-			data->in_alarms = pc87360_read_value(data, LD_IN,
-					  NO_BANK, PC87365_REG_IN_ALARMS1)
-					| ((pc87360_read_value(data, LD_IN,
-					    NO_BANK, PC87365_REG_IN_ALARMS2)
-					    & 0x07) << 8);
 			data->vid = (data->vid_conf & 0xE0) ?
 				    pc87360_read_value(data, LD_IN,
 				    NO_BANK, PC87365_REG_VID) : 0x1F;
@@ -1403,11 +1509,6 @@ static struct pc87360_data *pc87360_upda
 						     PC87365_REG_TEMP_CRIT);
 			}
 		}
-		if (data->tempnr) {
-			data->temp_alarms = pc87360_read_value(data, LD_TEMP,
-					    NO_BANK, PC87365_REG_TEMP_ALARMS)
-					    & 0x3F;
-		}
 
 		data->last_updated = jiffies;
 		data->valid = 1;

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,

[Jean Delvare]

Hi Jim,

> implement separate min, max, alarms for each voltage input,
> and min, max, crit alarms for temps.

What about the fans?

> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
> ---
> $ diffstat hwmon-pc87360-separate-alarms.patch
>  pc87360.c |  181 
> ++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 141 insertions(+), 40 deletions(-)
> 
> patch is against 19-rc1, you're accepting separate-alarms patches for 
> 2.6.20 now ?

Yes, I am even calling for them. I've done lm63, lm83, lm90 and f71805f
in 2.6.19 already, abituguru was already OK, and I have a patch pending
for w83627hf. All other drivers will need to be worked on, the sooner
the better.

> It passes this exersise:
> 
> #!/bin/bash
> sensors -s
> sensors
> cd /sys/class/hwmon/hwmon0/device
> function set_check {
>     local lim=$1
>     local setting=$2
>     local want=$3
>     echo $setting > $lim
>     sleep 2
>     local in=`cat ${lim}_alarm`
>     [ $in = $want ] && echo ok setting $lim $setting alarm ${lim}_alarm $in
> }
> # in8_input is 1477
> set_check in8_min 1500 1
> set_check in8_min 1400 0
> set_check in8_max 1400 1
> set_check in8_max 1500 0
> # temp3_input is 99000
> set_check temp3_min 101000 1
> set_check temp3_min 95000 0
> set_check temp3_max 95000 1
> set_check temp3_max 101000 0
> set_check temp3_crit 95000 1
> set_check temp3_crit 101000 0
> 
> The need for 'sleep 2' above made me think to reset data->valid when any 
> setter callback is called.

Hmm, I guess it makes some sense, but OTOH you don't know how much time
the hardware will take to trigger the new alarms if needed. So you may
still get a wrong reading if you rush, which means you'll still need to
sleep. There doesn't seem to be much benefit in practice.

> I didnt add it, but it seems the right thing to do.  My preference is to 
> add it into a forthcoming 2D-callback patch.

No, the preference is a different patch for every unrelated change.

> diff -ruNp -X dontdiff -X exclude-diffs 19rc1-0/drivers/hwmon/pc87360.c alarms-only/drivers/hwmon/pc87360.c
> --- 19rc1-0/drivers/hwmon/pc87360.c	2006-10-05 20:17:51.000000000 -0600
> +++ alarms-only/drivers/hwmon/pc87360.c	2006-10-08 14:43:01.000000000 -0600
> @@ -144,15 +144,14 @@ static inline u8 PWM_TO_REG(int val, int
>   * Voltage registers and conversions
>   */
>  
> +#define PC87365_REG_VID			0x06
>  #define PC87365_REG_IN_CONVRATE		0x07
>  #define PC87365_REG_IN_CONFIG		0x08
> +/* per-channel (0-13) registers */
> +#define PC87365_REG_IN_STATUS		0x0A
>  #define PC87365_REG_IN			0x0B
> -#define PC87365_REG_IN_MIN		0x0D
>  #define PC87365_REG_IN_MAX		0x0C
> -#define PC87365_REG_IN_STATUS		0x0A
> -#define PC87365_REG_IN_ALARMS1		0x00
> -#define PC87365_REG_IN_ALARMS2		0x01
> -#define PC87365_REG_VID			0x06
> +#define PC87365_REG_IN_MIN		0x0D

Please don't move things around that way, it hurts readbility. The
defines were sorted according to functionality, on purpose.

> -static ssize_t show_temp_alarms(struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t show_temp_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
>  {
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct pc87360_data *data = pc87360_update_device(dev);
> -	return sprintf(buf, "%u\n", data->temp_alarms);
> +	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 2) >> 1);
>  }
> -static DEVICE_ATTR(alarms_temp, S_IRUGO, show_temp_alarms, NULL);

No, it's not correct. We do not want to remove the old all-in-one alarm
files, it would break compatibility with lm-sensors. Even lm-sensors
2.10.1, which was released 2 weeks ago, doesn't support the new alarm
model yet!

So for now we are only adding the individual files. Then we teach
libsensors how to use them. Then, _much later_, we will drop the legacy
all-in-one alarm files.

As a nice side effect it will make your patch more readable :)

Thanks,
-- 
Jean Delvare

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,

[Jim Cromie]

implement separate min, max, alarms for each voltage input,
and min, max, crit alarms for temps.


Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
---

$ diffstat pc-set/hwmon-pc87360-separate-alarms.patch
 pc87360.c |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 139 insertions(+), 5 deletions(-)



> What about the fans?
>   
Current driver has no existing alarms on the fans, and my mobo has no 
fans either.
So adding them is a- more risky cuz its untestable here, b - a new feature.
I'll look at it later.

> Hmm, I guess it makes some sense, but OTOH you don't know how much time
> the hardware will take to trigger the new alarms if needed. So you may
> still get a wrong reading if you rush, which means you'll still need to
> sleep. There doesn't seem to be much benefit in practice.
>
>   
>> I didnt add it, but it seems the right thing to do.  My preference is to 
>> add it into a forthcoming 2D-callback patch.
>>     
>
> No, the preference is a different patch for every unrelated change.
>
>   
Did I say into ?  I meant after ! ( into the 5 setter-callbacks  ;-)
> Please don't move things around that way, it hurts readbility. The
> defines were sorted according to functionality, on purpose.
>
>   
ack.  dropped that chunk.
> We do not want to remove the old all-in-one alarm
> files, it would break compatibility with lm-sensors. Even lm-sensors
> 2.10.1, which was released 2 weeks ago, doesn't support the new alarm
> model yet!
>
> So for now we are only adding the individual files. Then we teach
> libsensors how to use them. Then, _much later_, we will drop the legacy
> all-in-one alarm files.
>
> As a nice side effect it will make your patch more readable :)
>
>   
Ack.

> Thanks,
>   
thank you.


diff -ruNp -X dontdiff -X exclude-diffs 19rc1-0/drivers/hwmon/pc87360.c 19rc1-1/drivers/hwmon/pc87360.c
--- 19rc1-0/drivers/hwmon/pc87360.c	2006-10-05 20:17:51.000000000 -0600
+++ 19rc1-1/drivers/hwmon/pc87360.c	2006-10-10 09:00:29.000000000 -0600
@@ -495,7 +495,9 @@ static struct sensor_device_attribute in
 	&in_input[X].dev_attr.attr,	\
 	&in_status[X].dev_attr.attr,	\
 	&in_min[X].dev_attr.attr,	\
-	&in_max[X].dev_attr.attr
+	&in_max[X].dev_attr.attr,	\
+	&in_min_alarm[X].dev_attr.attr,	\
+	&in_max_alarm[X].dev_attr.attr
 
 static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -525,6 +527,46 @@ static ssize_t show_in_alarms(struct dev
 }
 static DEVICE_ATTR(alarms_in, S_IRUGO, show_in_alarms, NULL);
 
+static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 4) >> 2); 
+}
+static struct sensor_device_attribute in_max_alarm[] = {
+	SENSOR_ATTR(in0_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 0),
+	SENSOR_ATTR(in1_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 1),
+	SENSOR_ATTR(in2_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 2),
+	SENSOR_ATTR(in3_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 3),
+	SENSOR_ATTR(in4_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 4),
+	SENSOR_ATTR(in5_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 5),
+	SENSOR_ATTR(in6_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 6),
+	SENSOR_ATTR(in7_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 7),
+	SENSOR_ATTR(in8_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 8),
+	SENSOR_ATTR(in9_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 9),
+	SENSOR_ATTR(in10_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 10),
+};
+
+static ssize_t show_in_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 2) >> 1);
+}
+static struct sensor_device_attribute in_min_alarm[] = {
+	SENSOR_ATTR(in0_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 0),
+	SENSOR_ATTR(in1_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 1),
+	SENSOR_ATTR(in2_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 2),
+	SENSOR_ATTR(in3_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 2),
+	SENSOR_ATTR(in4_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 4),
+	SENSOR_ATTR(in5_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 5),
+	SENSOR_ATTR(in6_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 6),
+	SENSOR_ATTR(in7_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 7),
+	SENSOR_ATTR(in8_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 8),
+	SENSOR_ATTR(in9_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 9),
+	SENSOR_ATTR(in10_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 10),
+};
+
 static struct attribute *pc8736x_vin_attr_array[] = {
 	VIN_UNIT_ATTRS(0),
 	VIN_UNIT_ATTRS(1),
@@ -664,12 +706,60 @@ static struct sensor_device_attribute th
 		    show_therm_crit, set_therm_crit, 2+11),
 };
 
+static ssize_t show_therm_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 2) >> 1); 
+}
+static struct sensor_device_attribute therm_min_alarm[] = {
+	SENSOR_ATTR(temp4_min_alarm, S_IRUGO,
+		    show_therm_min_alarm, NULL, 0+11),
+	SENSOR_ATTR(temp5_min_alarm, S_IRUGO,
+		    show_therm_min_alarm, NULL, 1+11),
+	SENSOR_ATTR(temp6_min_alarm, S_IRUGO,
+		    show_therm_min_alarm, NULL, 2+11),
+};
+
+static ssize_t show_therm_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 4) >> 2); 
+}
+static struct sensor_device_attribute therm_max_alarm[] = {
+	SENSOR_ATTR(temp4_max_alarm, S_IRUGO,
+		    show_therm_max_alarm, NULL, 0+11),
+	SENSOR_ATTR(temp5_max_alarm, S_IRUGO,
+		    show_therm_max_alarm, NULL, 1+11),
+	SENSOR_ATTR(temp6_max_alarm, S_IRUGO,
+		    show_therm_max_alarm, NULL, 2+11),
+};
+
+static ssize_t show_therm_crit_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 8) >> 3); 
+}
+static struct sensor_device_attribute therm_crit_alarm[] = {
+	SENSOR_ATTR(temp4_crit_alarm, S_IRUGO,
+		    show_therm_crit_alarm, NULL, 0+11),
+	SENSOR_ATTR(temp5_crit_alarm, S_IRUGO,
+		    show_therm_crit_alarm, NULL, 1+11),
+	SENSOR_ATTR(temp6_crit_alarm, S_IRUGO,
+		    show_therm_crit_alarm, NULL, 2+11),
+};
+
 #define THERM_UNIT_ATTRS(X) \
 	&therm_input[X].dev_attr.attr,	\
 	&therm_status[X].dev_attr.attr,	\
 	&therm_min[X].dev_attr.attr,	\
 	&therm_max[X].dev_attr.attr,	\
-	&therm_crit[X].dev_attr.attr
+	&therm_crit[X].dev_attr.attr,	\
+	&therm_min_alarm[X].dev_attr.attr, \
+	&therm_max_alarm[X].dev_attr.attr, \
+	&therm_crit_alarm[X].dev_attr.attr
 
 static struct attribute * pc8736x_therm_attr_array[] = {
 	THERM_UNIT_ATTRS(0),
@@ -799,18 +889,56 @@ static ssize_t show_temp_alarms(struct d
 }
 static DEVICE_ATTR(alarms_temp, S_IRUGO, show_temp_alarms, NULL);
 
+static ssize_t show_temp_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 2) >> 1);
+}
+static struct sensor_device_attribute temp_min_alarm[] = {
+	SENSOR_ATTR(temp1_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 0),
+	SENSOR_ATTR(temp2_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 1),
+	SENSOR_ATTR(temp3_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 2),
+};
+
+static ssize_t show_temp_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 4) >> 2);
+}
+static struct sensor_device_attribute temp_max_alarm[] = {
+	SENSOR_ATTR(temp1_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 0),
+	SENSOR_ATTR(temp2_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 1),
+	SENSOR_ATTR(temp3_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 2),
+};
+
+static ssize_t show_temp_crit_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 8) >> 3);
+}
+static struct sensor_device_attribute temp_crit_alarm[] = {
+	SENSOR_ATTR(temp1_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 0),
+	SENSOR_ATTR(temp2_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 1),
+	SENSOR_ATTR(temp3_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 2),
+};
+
 #define TEMP_UNIT_ATTRS(X) \
 	&temp_input[X].dev_attr.attr,	\
 	&temp_status[X].dev_attr.attr,	\
 	&temp_min[X].dev_attr.attr,	\
 	&temp_max[X].dev_attr.attr,	\
-	&temp_crit[X].dev_attr.attr
+	&temp_crit[X].dev_attr.attr,	\
+	&temp_min_alarm[X].dev_attr.attr, \
+	&temp_max_alarm[X].dev_attr.attr, \
+	&temp_crit_alarm[X].dev_attr.attr
 
 static struct attribute * pc8736x_temp_attr_array[] = {
 	TEMP_UNIT_ATTRS(0),
 	TEMP_UNIT_ATTRS(1),
 	TEMP_UNIT_ATTRS(2),
-	/* include the few miscellaneous atts here */
 	&dev_attr_alarms_temp.attr,
 	NULL
 };
@@ -1043,7 +1171,13 @@ static int pc87360_detect(struct i2c_ada
 			    || (err = device_create_file(dev,
 					&temp_crit[i].dev_attr))
 			    || (err = device_create_file(dev,
-					&temp_status[i].dev_attr)))
+					&temp_status[i].dev_attr))
+			    || (err = device_create_file(dev,
+					&temp_min_alarm[i].dev_attr))
+			    || (err = device_create_file(dev,
+					&temp_max_alarm[i].dev_attr))
+			    || (err = device_create_file(dev,
+					&temp_crit_alarm[i].dev_attr)))
 				goto ERROR3;
 		}
 		if ((err = device_create_file(dev, &dev_attr_alarms_temp)))

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,

[Jean Delvare]

Hi Jim,

> implement separate min, max, alarms for each voltage input,
> and min, max, crit alarms for temps.
> 
> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
> ---
> 
> $ diffstat pc-set/hwmon-pc87360-separate-alarms.patch
>  pc87360.c |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 139 insertions(+), 5 deletions(-)
> 
> > What about the fans?
>
> Current driver has no existing alarms on the fans, and my mobo has no 
> fans either.

"sensors" display alarms for the fans for all the PC87360 family chips,
so they must exist.

> So adding them is a- more risky cuz its untestable here, b - a new feature.
> I'll look at it later.

This shouldn't be that risky, and we need it anyway. We can't switch
libsensors to the new model until it offers a functionality level
equivalent to the current code for all supported devices.

> diff -ruNp -X dontdiff -X exclude-diffs 19rc1-0/drivers/hwmon/pc87360.c 19rc1-1/drivers/hwmon/pc87360.c
> --- 19rc1-0/drivers/hwmon/pc87360.c	2006-10-05 20:17:51.000000000 -0600
> +++ 19rc1-1/drivers/hwmon/pc87360.c	2006-10-10 09:00:29.000000000 -0600
> @@ -495,7 +495,9 @@ static struct sensor_device_attribute in
>  	&in_input[X].dev_attr.attr,	\
>  	&in_status[X].dev_attr.attr,	\
>  	&in_min[X].dev_attr.attr,	\
> -	&in_max[X].dev_attr.attr
> +	&in_max[X].dev_attr.attr,	\
> +	&in_min_alarm[X].dev_attr.attr,	\
> +	&in_max_alarm[X].dev_attr.attr
>  
>  static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -525,6 +527,46 @@ static ssize_t show_in_alarms(struct dev
>  }
>  static DEVICE_ATTR(alarms_in, S_IRUGO, show_in_alarms, NULL);
>  
> +static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)

Line too long, please fold. Same for all other callbacks you introduced.

> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 4) >> 2); 
> +}
> +static struct sensor_device_attribute in_max_alarm[] = {
> +	SENSOR_ATTR(in0_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 0),
> +	SENSOR_ATTR(in1_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 1),
> +	SENSOR_ATTR(in2_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 2),
> +	SENSOR_ATTR(in3_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 3),
> +	SENSOR_ATTR(in4_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 4),
> +	SENSOR_ATTR(in5_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 5),
> +	SENSOR_ATTR(in6_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 6),
> +	SENSOR_ATTR(in7_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 7),
> +	SENSOR_ATTR(in8_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 8),
> +	SENSOR_ATTR(in9_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 9),
> +	SENSOR_ATTR(in10_max_alarm, S_IRUGO, show_in_max_alarm, NULL, 10),
> +};
> +
> +static ssize_t show_in_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 2) >> 1);
> +}
> +static struct sensor_device_attribute in_min_alarm[] = {
> +	SENSOR_ATTR(in0_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 0),
> +	SENSOR_ATTR(in1_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 1),
> +	SENSOR_ATTR(in2_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 2),
> +	SENSOR_ATTR(in3_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 2),
> +	SENSOR_ATTR(in4_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 4),
> +	SENSOR_ATTR(in5_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 5),
> +	SENSOR_ATTR(in6_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 6),
> +	SENSOR_ATTR(in7_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 7),
> +	SENSOR_ATTR(in8_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 8),
> +	SENSOR_ATTR(in9_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 9),
> +	SENSOR_ATTR(in10_min_alarm, S_IRUGO, show_in_min_alarm, NULL, 10),
> +};
> +
>  static struct attribute *pc8736x_vin_attr_array[] = {
>  	VIN_UNIT_ATTRS(0),
>  	VIN_UNIT_ATTRS(1),
> @@ -664,12 +706,60 @@ static struct sensor_device_attribute th
>  		    show_therm_crit, set_therm_crit, 2+11),
>  };
>  
> +static ssize_t show_therm_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 2) >> 1); 
> +}

This callback is exactly the same as show_in_min_alarm above, so why
don't you just reuse it? Same for show_therm_max_alarm below and
show_in_max_alarm.

Which made me check the current code, and it appears that
show_therm_input, show_therm_min and show_therm_max are redundant with
show_in_input, show_in_min and show_in_max, respectively, so the code
could be simplified. And this is again true of "set" callbacks, unless
I am overlooking something? The benefit is even bigger here as these
callbacks are larger. Care to submit a patch?

> +static struct sensor_device_attribute therm_min_alarm[] = {
> +	SENSOR_ATTR(temp4_min_alarm, S_IRUGO,
> +		    show_therm_min_alarm, NULL, 0+11),
> +	SENSOR_ATTR(temp5_min_alarm, S_IRUGO,
> +		    show_therm_min_alarm, NULL, 1+11),
> +	SENSOR_ATTR(temp6_min_alarm, S_IRUGO,
> +		    show_therm_min_alarm, NULL, 2+11),
> +};
> +
> +static ssize_t show_therm_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 4) >> 2); 
> +}
> +static struct sensor_device_attribute therm_max_alarm[] = {
> +	SENSOR_ATTR(temp4_max_alarm, S_IRUGO,
> +		    show_therm_max_alarm, NULL, 0+11),
> +	SENSOR_ATTR(temp5_max_alarm, S_IRUGO,
> +		    show_therm_max_alarm, NULL, 1+11),
> +	SENSOR_ATTR(temp6_max_alarm, S_IRUGO,
> +		    show_therm_max_alarm, NULL, 2+11),
> +};
> +
> +static ssize_t show_therm_crit_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->in_status[attr->index] & 8) >> 3); 
> +}
> +static struct sensor_device_attribute therm_crit_alarm[] = {
> +	SENSOR_ATTR(temp4_crit_alarm, S_IRUGO,
> +		    show_therm_crit_alarm, NULL, 0+11),
> +	SENSOR_ATTR(temp5_crit_alarm, S_IRUGO,
> +		    show_therm_crit_alarm, NULL, 1+11),
> +	SENSOR_ATTR(temp6_crit_alarm, S_IRUGO,
> +		    show_therm_crit_alarm, NULL, 2+11),
> +};
> +
>  #define THERM_UNIT_ATTRS(X) \
>  	&therm_input[X].dev_attr.attr,	\
>  	&therm_status[X].dev_attr.attr,	\
>  	&therm_min[X].dev_attr.attr,	\
>  	&therm_max[X].dev_attr.attr,	\
> -	&therm_crit[X].dev_attr.attr
> +	&therm_crit[X].dev_attr.attr,	\
> +	&therm_min_alarm[X].dev_attr.attr, \
> +	&therm_max_alarm[X].dev_attr.attr, \
> +	&therm_crit_alarm[X].dev_attr.attr
>  
>  static struct attribute * pc8736x_therm_attr_array[] = {
>  	THERM_UNIT_ATTRS(0),
> @@ -799,18 +889,56 @@ static ssize_t show_temp_alarms(struct d
>  }
>  static DEVICE_ATTR(alarms_temp, S_IRUGO, show_temp_alarms, NULL);
>  
> +static ssize_t show_temp_min_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 2) >> 1);
> +}
> +static struct sensor_device_attribute temp_min_alarm[] = {
> +	SENSOR_ATTR(temp1_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 0),
> +	SENSOR_ATTR(temp2_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 1),
> +	SENSOR_ATTR(temp3_min_alarm, S_IRUGO, show_temp_min_alarm, NULL, 2),
> +};
> +
> +static ssize_t show_temp_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 4) >> 2);
> +}
> +static struct sensor_device_attribute temp_max_alarm[] = {
> +	SENSOR_ATTR(temp1_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 0),
> +	SENSOR_ATTR(temp2_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 1),
> +	SENSOR_ATTR(temp3_max_alarm, S_IRUGO, show_temp_max_alarm, NULL, 2),
> +};
> +
> +static ssize_t show_temp_crit_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pc87360_data *data = pc87360_update_device(dev);
> +	return sprintf(buf, "%u\n", (data->temp_status[attr->index] & 8) >> 3);
> +}
> +static struct sensor_device_attribute temp_crit_alarm[] = {
> +	SENSOR_ATTR(temp1_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 0),
> +	SENSOR_ATTR(temp2_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 1),
> +	SENSOR_ATTR(temp3_crit_alarm, S_IRUGO, show_temp_crit_alarm, NULL, 2),
> +};
> +
>  #define TEMP_UNIT_ATTRS(X) \
>  	&temp_input[X].dev_attr.attr,	\
>  	&temp_status[X].dev_attr.attr,	\
>  	&temp_min[X].dev_attr.attr,	\
>  	&temp_max[X].dev_attr.attr,	\
> -	&temp_crit[X].dev_attr.attr
> +	&temp_crit[X].dev_attr.attr,	\
> +	&temp_min_alarm[X].dev_attr.attr, \
> +	&temp_max_alarm[X].dev_attr.attr, \
> +	&temp_crit_alarm[X].dev_attr.attr
>  
>  static struct attribute * pc8736x_temp_attr_array[] = {
>  	TEMP_UNIT_ATTRS(0),
>  	TEMP_UNIT_ATTRS(1),
>  	TEMP_UNIT_ATTRS(2),
> -	/* include the few miscellaneous atts here */

This comment can stay for now?

>  	&dev_attr_alarms_temp.attr,
>  	NULL
>  };
> @@ -1043,7 +1171,13 @@ static int pc87360_detect(struct i2c_ada
>  			    || (err = device_create_file(dev,
>  					&temp_crit[i].dev_attr))
>  			    || (err = device_create_file(dev,
> -					&temp_status[i].dev_attr)))
> +					&temp_status[i].dev_attr))
> +			    || (err = device_create_file(dev,
> +					&temp_min_alarm[i].dev_attr))
> +			    || (err = device_create_file(dev,
> +					&temp_max_alarm[i].dev_attr))
> +			    || (err = device_create_file(dev,
> +					&temp_crit_alarm[i].dev_attr)))
>  				goto ERROR3;
>  		}
>  		if ((err = device_create_file(dev, &dev_attr_alarms_temp)))
> 

Otherwise it looks OK to me, thanks for working on this. Please
resubmit with the minor issues above addressed and the fan alarms when
you're done with that.

Thanks,
-- 
Jean Delvare

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
>> implement separate min, max, alarms for each voltage input,
>> and min, max, crit alarms for temps.
>>
>> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
>> ---
>>
>> $ diffstat pc-set/hwmon-pc87360-separate-alarms.patch
>>  pc87360.c |  144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 files changed, 139 insertions(+), 5 deletions(-)
>>
>>     
>>> What about the fans?
>>>       
>> Current driver has no existing alarms on the fans, and my mobo has no 
>> fans either.
>>     
>
> "sensors" display alarms for the fans for all the PC87360 family chips,
> so they must exist.
>
>   
>> So adding them is a- more risky cuz its untestable here, b - a new feature.
>> I'll look at it later.
>>     
>
> This shouldn't be that risky, and we need it anyway. We can't switch
> libsensors to the new model until it offers a functionality level
> equivalent to the current code for all supported devices.
>
>   
>> diff -ruNp -X dontdiff -X exclude-diffs 19rc1-0/drivers/hwmon/pc87360.c 19rc1-1/drivers/hwmon/pc87360.c
>> --- 19rc1-0/drivers/hwmon/pc87360.c	2006-10-05 20:17:51.000000000 -0600
>> +++ 19rc1-1/drivers/hwmon/pc87360.c	2006-10-10 09:00:29.000000000 -0600
>> @@ -495,7 +495,9 @@ static struct sensor_device_attribute in
>>  	&in_input[X].dev_attr.attr,	\
>>  	&in_status[X].dev_attr.attr,	\
>>  	&in_min[X].dev_attr.attr,	\
>> -	&in_max[X].dev_attr.attr
>> +	&in_max[X].dev_attr.attr,	\
>> +	&in_min_alarm[X].dev_attr.attr,	\
>> +	&in_max_alarm[X].dev_attr.attr
>>  
>>  static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> @@ -525,6 +527,46 @@ static ssize_t show_in_alarms(struct dev
>>  }
>>  static DEVICE_ATTR(alarms_in, S_IRUGO, show_in_alarms, NULL);
>>  
>> +static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
>>     
>
> Line too long, please fold. Same for all other callbacks you introduced.
>
>   
Ok.  I was under impression that fn-sigs were the sole exception to the 
line-wrap rule,
in that having the full sig on a single line is good for grepping.

I'll guess that folding existing functions should be a separate patch ?
(combined with any other cosmetic changes, later)

>
> This callback is exactly the same as show_in_min_alarm above, so why
> don't you just reuse it? Same for show_therm_max_alarm below and
> show_in_max_alarm.
>
> Which made me check the current code, and it appears that
> show_therm_input, show_therm_min and show_therm_max are redundant with
> show_in_input, show_in_min and show_in_max, respectively, so the code
> could be simplified. And this is again true of "set" callbacks, unless
> I am overlooking something? The benefit is even bigger here as these
> callbacks are larger. Care to submit a patch?
>
>   
I was planning to fold them together in a 2D patch, like so:

static ssize_t show_temp(struct device *dev, struct device_attribute 
*devattr, char *buf)
{
    struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
    struct pc87360_data *data = pc87360_update_device(dev);
    int idx = attr->index;
    unsigned res = -1;

    switch (attr->nr) {
    case FN_TEMP_INPUT:
        res = TEMP_FROM_REG(data->temp[idx]);
        break;
    case FN_TEMP_STATUS:
        res = data->temp_status[idx];
        break;
    case FN_TEMP_MIN:
        res = TEMP_FROM_REG(data->temp_min[idx]);
        break;
    case FN_TEMP_MAX:
        res = TEMP_FROM_REG(data->temp_max[idx]);
        break;
    case FN_TEMP_CRIT:
        res = TEMP_FROM_REG(data->temp_crit[idx]);
        break;
    case FN_TEMP_MIN_ALARM:
        res = (data->temp_status[idx] & 2) >> 1;
        break;
    case FN_TEMP_MAX_ALARM:
        res = (data->temp_status[idx] & 4) >> 2;
        break;
    case FN_TEMP_CRIT_ALARM:
        res = (data->temp_status[idx] & 8) >> 3;
        break;
    default:
        dev_err(dev, "unknown attr fetch\n");
     }
     return sprintf(buf, "%u\n", res);
}

This gets significant shrinkage..
  15902    5160      32   21094    5266 
19rc1mm1-1/drivers/hwmon/pc87360.ko  --- this patch
  14011    5160      32   19203    4b03 
19rc1mm1-2/drivers/hwmon/pc87360.ko  --- 2D patch

This folding doesnt rely on thermistor measurements being done with voltage
hardware, which is an ideosyncrasy of this particular part, and doesnt 
work for
the temp_ sensors (only the therm ones), so I think its clearer and more 
general this way.


>>  static struct attribute * pc8736x_temp_attr_array[] = {
>>  	TEMP_UNIT_ATTRS(0),
>>  	TEMP_UNIT_ATTRS(1),
>>  	TEMP_UNIT_ATTRS(2),
>> -	/* include the few miscellaneous atts here */
>>     
>
> This comment can stay for now?
>
>   
Its less true than it once was - few is one, and not so miscellaneous -
temp-alarms belongs with temp_*, and comment implies that its something 
of a catch-all. 
At least that was my mis-thinking when I 1st added it.

Or did you mean drop it *later* ?

>>  	&dev_attr_alarms_temp.attr,
>>  	NULL
>>  };
>>
>>     
> Otherwise it looks OK to me, thanks for working on this. Please
> resubmit with the minor issues above addressed and the fan alarms when
> you're done with that.
>
>   
Ok. 2 separate patches, works for me :-)

> Thanks,
>   

btw, thunderbird shows me 7:28 am on this mail, but Id *swear*
I looked at my inbox a dozen times today w/o seeing it.
Any idea why ? / what time did you actually send it ?

thanks
jimc


ps.  got a (link to a good model) patch converting a superio-i2c driver 
to platform_driver ?
It will simplify things if I have something to crib from ;-)

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,

[Jean Delvare]

Hi Jim,

> > > +static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
> >
> > Line too long, please fold. Same for all other callbacks you introduced.
>
> Ok.  I was under impression that fn-sigs were the sole exception
> to the line-wrap rule, in that having the full sig on a single line is
> good for grepping.

No, there is no such exception. Line wrap indeed makes grep'ing more
difficult in some cases, but Linus seems to care about the line length
more than the grep'ability.

> I'll guess that folding existing functions should be a separate patch ?
> (combined with any other cosmetic changes, later)

Yes, please.

> > This callback is exactly the same as show_in_min_alarm above, so why
> > don't you just reuse it? Same for show_therm_max_alarm below and
> > show_in_max_alarm.
> >
> > Which made me check the current code, and it appears that
> > show_therm_input, show_therm_min and show_therm_max are redundant with
> > show_in_input, show_in_min and show_in_max, respectively, so the code
> > could be simplified. And this is again true of "set" callbacks, unless
> > I am overlooking something? The benefit is even bigger here as these
> > callbacks are larger. Care to submit a patch?
>
> I was planning to fold them together in a 2D patch, like so:
> 
> static ssize_t show_temp(struct device *dev, struct device_attribute 
> *devattr, char *buf)
> {
>     struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>     struct pc87360_data *data = pc87360_update_device(dev);
>     int idx = attr->index;
>     unsigned res = -1;
> 
>     switch (attr->nr) {
>     case FN_TEMP_INPUT:
>         res = TEMP_FROM_REG(data->temp[idx]);
>         break;
>     case FN_TEMP_STATUS:
>         res = data->temp_status[idx];
>         break;
>     case FN_TEMP_MIN:
>         res = TEMP_FROM_REG(data->temp_min[idx]);
>         break;
>     case FN_TEMP_MAX:
>         res = TEMP_FROM_REG(data->temp_max[idx]);
>         break;
>     case FN_TEMP_CRIT:
>         res = TEMP_FROM_REG(data->temp_crit[idx]);
>         break;
>     case FN_TEMP_MIN_ALARM:
>         res = (data->temp_status[idx] & 2) >> 1;
>         break;
>     case FN_TEMP_MAX_ALARM:
>         res = (data->temp_status[idx] & 4) >> 2;
>         break;
>     case FN_TEMP_CRIT_ALARM:
>         res = (data->temp_status[idx] & 8) >> 3;
>         break;
>     default:
>         dev_err(dev, "unknown attr fetch\n");
>      }
>      return sprintf(buf, "%u\n", res);

How do you handle negative temperature values with a %u?

> }
> 
> This gets significant shrinkage..
>   15902    5160      32   21094    5266 
> 19rc1mm1-1/drivers/hwmon/pc87360.ko  --- this patch

What is "this patch" exactly?

>   14011    5160      32   19203    4b03 
> 19rc1mm1-2/drivers/hwmon/pc87360.ko  --- 2D patch
> 
> This folding doesnt rely on thermistor measurements being done with voltage
> hardware, which is an ideosyncrasy of this particular part, and doesnt 
> work for the temp_ sensors (only the therm ones), so I think its clearer
> and more general this way.

Actually, thermistor measurements are always being done with voltage
hardware, by design. What may be surprising is that the chip doesn't
attempt to abstract this to make temperatures look more like
temperatures.

As said before, I'm not a fan of this switch/case approach, as it looks
suboptimal from a performance point of view (unless you can prove me
wrong with actual data). I did not object for the vt1211 driver because
it was a new driver, and the author prefered it that way, so why not,
but I see little benefit in converting a perfectly working driver to a
different approach when both approaches have drawbacks and benefits.

> > >  static struct attribute * pc8736x_temp_attr_array[] = {
> > >  	TEMP_UNIT_ATTRS(0),
> > >  	TEMP_UNIT_ATTRS(1),
> > >  	TEMP_UNIT_ATTRS(2),
> > > -	/* include the few miscellaneous atts here */
> >
> > This comment can stay for now?
>
> Its less true than it once was - few is one, and not so miscellaneous -
> temp-alarms belongs with temp_*, and comment implies that its something 
> of a catch-all. 
> At least that was my mis-thinking when I 1st added it.
> 
> Or did you mean drop it *later* ?

No, I'm fine with that. I thought it was a left over from the first
patch attempt, but if you remove it on purpose it's OK.

(Indeed it then becomes a cleanup change without direct relation with
what the patch does. But nevermind.)

> btw, thunderbird shows me 7:28 am on this mail, but Id *swear*
> I looked at my inbox a dozen times today w/o seeing it.
> Any idea why ? / what time did you actually send it ?

Wed, 11 Oct 2006 15:28:19 +0200

Which is indeed 7:28 AM for you. Maybe the mail was delayed on the way,
check the headers.

> ps.  got a (link to a good model) patch converting a superio-i2c driver 
> to platform_driver ?
> It will simplify things if I have something to crib from ;-)

The f71805f and vt1211 drivers are already implemented as platform
drivers so you can start from there. The preliminary pc87427 driver
which I'll submit later this week is a platform driver as well.

For the pc87360 driver I expect an additional difficulty because it
uses up to 3 different I/O areas, each of which may or may not be
there. So you'll have to find a way to carry chip-specific information
from the Super-I/O detection step to the platform device creation, and
from there to the platform device probe function.

You may take a look there:
http://khali.linux-fr.org/devel/linux-2.6/hwmon-f71805f-add-f71872f-support.patch
This is an example of how I used dev.platform_data to carry this
information now that the f71805f driver supports two different chip
types. I guess you'll have to do something similar.

-- 
Jean Delvare

[lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
>>>> +static ssize_t show_in_max_alarm(struct device *dev, struct device_attribute *devattr, char *buf)
>>>>         
>>> Line too long, please fold. Same for all other callbacks you introduced.
>>>       
>> Ok.  I was under impression that fn-sigs were the sole exception
>> to the line-wrap rule, in that having the full sig on a single line is
>> good for grepping.
>>     
>
> No, there is no such exception. Line wrap indeed makes grep'ing more
> difficult in some cases, but Linus seems to care about the line length
> more than the grep'ability.
>
>   
Ok, thanks.
>> I'll guess that folding existing functions should be a separate patch ?
>> (combined with any other cosmetic changes, later)
>>     
>
> Yes, please.
>
>   
ok - Im thinking a full cosmetics patch, wrapping all long fn-sigs, 
dropping old comment,
>>> This callback is exactly the same as show_in_min_alarm above, so why
>>> don't you just reuse it? Same for show_therm_max_alarm below and
>>> show_in_max_alarm.
>>>
>>> Which made me check the current code, and it appears that
>>> show_therm_input, show_therm_min and show_therm_max are redundant with
>>> show_in_input, show_in_min and show_in_max, respectively, so the code
>>> could be simplified. And this is again true of "set" callbacks, unless
>>> I am overlooking something? The benefit is even bigger here as these
>>> callbacks are larger. Care to submit a patch?
>>>       
>> I was planning to fold them together in a 2D patch, like so:
>>
>> static ssize_t show_temp(struct device *dev, struct device_attribute 
>> *devattr, char *buf)
>> {
>>     struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>>     struct pc87360_data *data = pc87360_update_device(dev);
>>     int idx = attr->index;
>>     unsigned res = -1;
>>
>>     switch (attr->nr) {
>>     case FN_TEMP_INPUT:
>>         res = TEMP_FROM_REG(data->temp[idx]);
>>         break;
>>     
>>     default:
>>         dev_err(dev, "unknown attr fetch\n");
>>      }
>>      return sprintf(buf, "%u\n", res);
>>     
>
> How do you handle negative temperature values with a %u?
>
>   
Ahh, Id not noticed that b4..
To establish a true baseline, I went back to 2.6.13 (before I started 
hacking on this driver).
All of show_(in|therm)_<foo> use %u, which I also do in the 2D callbacks.
All the show_temp_<foo>s use /"%d\n" /in their respective formats, which 
I didnt do, but will.

FWIW show_temp_status also used %d rather than %u, the latter is better 
since status is flags.
Id prefer to ignore that preexisting sub-optimality (and use userspace 
bug-compatibility argument to support that)
rather than add a case-specific return sprintf() or fmt* indirection.



>> }
>>
>> This gets significant shrinkage..
>>   15902    5160      32   21094    5266 
>> 19rc1mm1-1/drivers/hwmon/pc87360.ko  --- this patch
>>     
>
> What is "this patch" exactly?
>   
the previously sent patch - adding separate alarms.

>   
>>   14011    5160      32   19203    4b03 
>> 19rc1mm1-2/drivers/hwmon/pc87360.ko  --- 2D patch
>>
>> This folding doesnt rely on thermistor measurements being done with voltage
>> hardware, which is an ideosyncrasy of this particular part, and doesnt 
>> work for the temp_ sensors (only the therm ones), so I think its clearer
>> and more general this way.
>>     
>
> Actually, thermistor measurements are always being done with voltage
> hardware, by design. What may be surprising is that the chip doesn't
> attempt to abstract this to make temperatures look more like
> temperatures.
>
> As said before, I'm not a fan of this switch/case approach, as it looks
> suboptimal from a performance point of view (unless you can prove me
> wrong with actual data). I did not object for the vt1211 driver because
> it was a new driver, and the author prefered it that way, so why not,
> but I see little benefit in converting a perfectly working driver to a
> different approach when both approaches have drawbacks and benefits.
>
>   
1st, the size arg:

[jimc at harpo hwmon-stuff]$ size 19rc1mm1-*/drivers/hwmon/pc87360.ko
   text    data     bss     dec     hex filename
  14474    3816      32   18322    4792 19rc1mm1-0/drivers/hwmon/pc87360.ko
  15902    5160      32   21094    5266 19rc1mm1-1/drivers/hwmon/pc87360.ko
  15046    5160      32   20238    4f0e 
19rc1mm1-1-fold/drivers/hwmon/pc87360.ko
  14011    5160      32   19203    4b03 19rc1mm1-2/drivers/hwmon/pc87360.ko

0 - virgin rc1-mm1
1 - separate alarms
1-fold - with in/therm redundancy folded out
2 - 2D callbacks

As is evident, 2D gives 2x the size reduction as in/therm folding,
and I think its clearer.  (also refactoring the status touching cases, 
and sharing
that across in/therm is possible, but of tiny benefit in size, and a 
loss of clarity imho)

As to performance (ie cache performance), the rule-of-thumb is that 
smaller is better
(consider that force_inline is needed, cuz gcc sometimes ignores inline, 
cuz its often a lose).

Beyond that, the access pattern from userspace is to read all the attrs 
from each sensor,
then move to next sensor.  For separate callbacks, this is the worst 
pattern - each separate
callback has opportunity to eject the previous from cache, whereas with 
2D callbacks,
the same function is re-called (more likely to still be in cache), and 
each case is likely to
have separate cache-lines (adjacent code --> adjacent cache-line)

I could derive some value from trying to prove this (and learn to wield 
oprofile well,
or cachegrind - but Im not set up to run a UML/xen-guest kernel, esp on 
my 586-266 8-)
but GregKH has dismissed the cache effects as unimportant, so for now, 
its *much* easier
to talk thru it.  Do you find the previous paragraph at all convincing ?
Is there another aspect of performance that Ive overlooked ?


>>>> -	/* include the few miscellaneous atts here */
>>>>         
>>> This comment can stay for now?
>>>       
>> Its less true than it once was - few is one, and not so miscellaneous -
>> temp-alarms belongs with temp_*, and comment implies that its something 
>> of a catch-all. 
>> At least that was my mis-thinking when I 1st added it.
>>
>> Or did you mean drop it *later* ?
>>     
>
> No, I'm fine with that. I thought it was a left over from the first
> patch attempt, but if you remove it on purpose it's OK.
>
> (Indeed it then becomes a cleanup change without direct relation with
> what the patch does. But nevermind.)
>   
Ok - I can restore it now, and pull it later - in a following 
cosmetics/rewrap patch.

Id rather get your provisional acceptance of this separate-alarms patch,
2- then send my 2D for review (which applies cleanly over this one),
3- then do the rewrap,
4- then add fan_alarms (which dont exist currently BTW - only in_alarms 
and temp_alarms do)
5- then do platform_driver (or maybe isa_driver, since is now in-core)

since 3,4,5 are dependent on how 2 goes, I havent touched them yet..