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