* [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,
2006-10-09 1:27 [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max, Jim Cromie
@ 2006-10-10 9:57 ` Jean Delvare
2006-10-10 18:42 ` Jim Cromie
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2006-10-10 9:57 UTC (permalink / raw)
To: lm-sensors
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,
2006-10-09 1:27 [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max, Jim Cromie
2006-10-10 9:57 ` [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, Jean Delvare
@ 2006-10-10 18:42 ` Jim Cromie
2006-10-11 13:28 ` Jean Delvare
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jim Cromie @ 2006-10-10 18:42 UTC (permalink / raw)
To: lm-sensors
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)))
^ permalink raw reply [flat|nested] 7+ messages in thread* [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,
2006-10-09 1:27 [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max, Jim Cromie
2006-10-10 9:57 ` [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, Jean Delvare
2006-10-10 18:42 ` Jim Cromie
@ 2006-10-11 13:28 ` Jean Delvare
2006-10-11 22:04 ` Jim Cromie
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2006-10-11 13:28 UTC (permalink / raw)
To: lm-sensors
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,
2006-10-09 1:27 [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max, Jim Cromie
` (2 preceding siblings ...)
2006-10-11 13:28 ` Jean Delvare
@ 2006-10-11 22:04 ` Jim Cromie
2006-10-12 9:37 ` Jean Delvare
2006-10-12 18:41 ` Jim Cromie
5 siblings, 0 replies; 7+ messages in thread
From: Jim Cromie @ 2006-10-11 22:04 UTC (permalink / raw)
To: lm-sensors
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 ;-)
^ permalink raw reply [flat|nested] 7+ messages in thread* [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,
2006-10-09 1:27 [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max, Jim Cromie
` (3 preceding siblings ...)
2006-10-11 22:04 ` Jim Cromie
@ 2006-10-12 9:37 ` Jean Delvare
2006-10-12 18:41 ` Jim Cromie
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2006-10-12 9:37 UTC (permalink / raw)
To: lm-sensors
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min,
2006-10-09 1:27 [lm-sensors] [patch 1.6.19-rc1+] hwmon/pc87360: separate {min, max, Jim Cromie
` (4 preceding siblings ...)
2006-10-12 9:37 ` Jean Delvare
@ 2006-10-12 18:41 ` Jim Cromie
5 siblings, 0 replies; 7+ messages in thread
From: Jim Cromie @ 2006-10-12 18:41 UTC (permalink / raw)
To: lm-sensors
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..
^ permalink raw reply [flat|nested] 7+ messages in thread