* [lm-sensors] Problem with:
@ 2008-12-15 12:51 Hans de Goede
2008-12-15 13:24 ` Jean Delvare
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Hans de Goede @ 2008-12-15 12:51 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
I just realised there is an issue with the
hwmon-f71882fg-11-separate-max-crit-alarm-and-beep.patch
patch.
It removes the temp#_alarm attributes (replacing them by temp#_max_alarm),
which is fine for lm_sensors v3, but will cause a problem with lm_sensors v2,
the "sensors" command from lm_sensors v2 will now give failed to read data for
temp# for all 3 temps :(
So it looks like we are sorta stuck with our past mistake of not doing separate
alarms.
We could do separate alarms like this:
temp#_alarm
temp#_crit_alarm
This will work with both libsensors and with "sensors" from both v2 and v3.
Regards,
Hans
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] Problem with: 2008-12-15 12:51 [lm-sensors] Problem with: Hans de Goede @ 2008-12-15 13:24 ` Jean Delvare 2008-12-16 10:07 ` Jean Delvare ` (3 subsequent siblings) 4 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2008-12-15 13:24 UTC (permalink / raw) To: lm-sensors Hi Hans, On Mon, 15 Dec 2008 13:51:13 +0100, Hans de Goede wrote: > I just realised there is an issue with the > hwmon-f71882fg-11-separate-max-crit-alarm-and-beep.patch > patch. > > It removes the temp#_alarm attributes (replacing them by temp#_max_alarm), > which is fine for lm_sensors v3, but will cause a problem with lm_sensors v2, > the "sensors" command from lm_sensors v2 will now give failed to read data for > temp# for all 3 temps :( Indeed. I admit I didn't think about lm_sensors v2 when reviewing your patch. This is probably a good reason to _not_ add support for new chips to lm_sensors v2. Owners of such recent hardware should really use lm_sensors v3 anyway. > So it looks like we are sorta stuck with our past mistake of not doing separate > alarms. > > We could do separate alarms like this: > temp#_alarm > temp#_crit_alarm > > This will work with both libsensors and with "sensors" from both v2 and v3. It's a bit confusing due to the asymmetry, but I admit it should work fine in practice. Feel free to send an updated patch doing this. Including an explanation of why we do this, so that someone reading the code in 3 or 4 years can understand the idea behind it. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] Problem with: 2008-12-15 12:51 [lm-sensors] Problem with: Hans de Goede 2008-12-15 13:24 ` Jean Delvare @ 2008-12-16 10:07 ` Jean Delvare 2008-12-16 13:40 ` Hans de Goede ` (2 subsequent siblings) 4 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2008-12-16 10:07 UTC (permalink / raw) To: lm-sensors Hans, On Mon, 15 Dec 2008 14:24:48 +0100, Jean Delvare wrote: > Hi Hans, > > On Mon, 15 Dec 2008 13:51:13 +0100, Hans de Goede wrote: > > I just realised there is an issue with the > > hwmon-f71882fg-11-separate-max-crit-alarm-and-beep.patch > > patch. > > > > It removes the temp#_alarm attributes (replacing them by temp#_max_alarm), > > which is fine for lm_sensors v3, but will cause a problem with lm_sensors v2, > > the "sensors" command from lm_sensors v2 will now give failed to read data for > > temp# for all 3 temps :( > > Indeed. I admit I didn't think about lm_sensors v2 when reviewing your > patch. This is probably a good reason to _not_ add support for new > chips to lm_sensors v2. Owners of such recent hardware should really > use lm_sensors v3 anyway. > > > So it looks like we are sorta stuck with our past mistake of not doing separate > > alarms. > > > > We could do separate alarms like this: > > temp#_alarm > > temp#_crit_alarm > > > > This will work with both libsensors and with "sensors" from both v2 and v3. > > It's a bit confusing due to the asymmetry, but I admit it should work > fine in practice. Feel free to send an updated patch doing this. > Including an explanation of why we do this, so that someone reading the > code in 3 or 4 years can understand the idea behind it. Do you have an update for this patch, please? I'd like to get all f71882fg/f8000 patches sorted out today. Thanks, -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] Problem with: 2008-12-15 12:51 [lm-sensors] Problem with: Hans de Goede 2008-12-15 13:24 ` Jean Delvare 2008-12-16 10:07 ` Jean Delvare @ 2008-12-16 13:40 ` Hans de Goede 2008-12-16 14:57 ` Jean Delvare 2008-12-16 15:10 ` Hans de Goede 4 siblings, 0 replies; 6+ messages in thread From: Hans de Goede @ 2008-12-16 13:40 UTC (permalink / raw) To: lm-sensors [-- Attachment #1: Type: text/plain, Size: 1577 bytes --] Jean Delvare wrote: > Hans, > > On Mon, 15 Dec 2008 14:24:48 +0100, Jean Delvare wrote: >> Hi Hans, >> >> On Mon, 15 Dec 2008 13:51:13 +0100, Hans de Goede wrote: >>> I just realised there is an issue with the >>> hwmon-f71882fg-11-separate-max-crit-alarm-and-beep.patch >>> patch. >>> >>> It removes the temp#_alarm attributes (replacing them by temp#_max_alarm), >>> which is fine for lm_sensors v3, but will cause a problem with lm_sensors v2, >>> the "sensors" command from lm_sensors v2 will now give failed to read data for >>> temp# for all 3 temps :( >> Indeed. I admit I didn't think about lm_sensors v2 when reviewing your >> patch. This is probably a good reason to _not_ add support for new >> chips to lm_sensors v2. Owners of such recent hardware should really >> use lm_sensors v3 anyway. >> >>> So it looks like we are sorta stuck with our past mistake of not doing separate >>> alarms. >>> >>> We could do separate alarms like this: >>> temp#_alarm >>> temp#_crit_alarm >>> >>> This will work with both libsensors and with "sensors" from both v2 and v3. >> It's a bit confusing due to the asymmetry, but I admit it should work >> fine in practice. Feel free to send an updated patch doing this. >> Including an explanation of why we do this, so that someone reading the >> code in 3 or 4 years can understand the idea behind it. > > Do you have an update for this patch, please? I'd like to get all > f71882fg/f8000 patches sorted out today. > Yes I have (for more then a day) I guess I somehow forgot to send it. Its attached now. Regards, Hans [-- Attachment #2: hwmon-f71882fg-11-separate-max-crit-alarm-and-beep-v2.patch --] [-- Type: text/plain, Size: 3843 bytes --] While studying the datasheets for adding F8000 support, I noticed that the F718x2 has separate alarms (and beep control) for its max and crit limits. We keep the temp#_alarm attributes as they are, even though it would be more logical to rename them to temp#_max_alarm. Because lm_sensors v2 depends on them. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- linux/drivers/hwmon/f71882fg.c.10-applied 2008-12-14 15:25:01.000000000 +0100 +++ linux/drivers/hwmon/f71882fg.c 2008-12-15 15:37:37.000000000 +0100 @@ -270,42 +270,56 @@ store_temp_max, 0, 1), SENSOR_ATTR_2(temp1_max_hyst, S_IRUGO|S_IWUSR, show_temp_max_hyst, store_temp_max_hyst, 0, 1), + /* Should really be temp1_max_alarm, but older versions did not handle + the max and crit alarms separately and lm_sensors v2 depends on the + presence of temp#_alarm files. The same goes for temp2/3 _alarm. */ + SENSOR_ATTR_2(temp1_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 1), + SENSOR_ATTR_2(temp1_max_beep, S_IRUGO|S_IWUSR, show_temp_beep, + store_temp_beep, 0, 1), SENSOR_ATTR_2(temp1_crit, S_IRUGO|S_IWUSR, show_temp_crit, store_temp_crit, 0, 1), SENSOR_ATTR_2(temp1_crit_hyst, S_IRUGO, show_temp_crit_hyst, NULL, 0, 1), + SENSOR_ATTR_2(temp1_crit_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 5), + SENSOR_ATTR_2(temp1_crit_beep, S_IRUGO|S_IWUSR, show_temp_beep, + store_temp_beep, 0, 5), SENSOR_ATTR_2(temp1_type, S_IRUGO, show_temp_type, NULL, 0, 1), - SENSOR_ATTR_2(temp1_beep, S_IRUGO|S_IWUSR, show_temp_beep, - store_temp_beep, 0, 1), - SENSOR_ATTR_2(temp1_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 1), SENSOR_ATTR_2(temp1_fault, S_IRUGO, show_temp_fault, NULL, 0, 1), SENSOR_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, 0, 2), SENSOR_ATTR_2(temp2_max, S_IRUGO|S_IWUSR, show_temp_max, store_temp_max, 0, 2), SENSOR_ATTR_2(temp2_max_hyst, S_IRUGO|S_IWUSR, show_temp_max_hyst, store_temp_max_hyst, 0, 2), + /* Should be temp2_max_alarm, see temp1_alarm note */ + SENSOR_ATTR_2(temp2_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 2), + SENSOR_ATTR_2(temp2_max_beep, S_IRUGO|S_IWUSR, show_temp_beep, + store_temp_beep, 0, 2), SENSOR_ATTR_2(temp2_crit, S_IRUGO|S_IWUSR, show_temp_crit, store_temp_crit, 0, 2), SENSOR_ATTR_2(temp2_crit_hyst, S_IRUGO, show_temp_crit_hyst, NULL, 0, 2), + SENSOR_ATTR_2(temp2_crit_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 6), + SENSOR_ATTR_2(temp2_crit_beep, S_IRUGO|S_IWUSR, show_temp_beep, + store_temp_beep, 0, 6), SENSOR_ATTR_2(temp2_type, S_IRUGO, show_temp_type, NULL, 0, 2), - SENSOR_ATTR_2(temp2_beep, S_IRUGO|S_IWUSR, show_temp_beep, - store_temp_beep, 0, 2), - SENSOR_ATTR_2(temp2_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 2), SENSOR_ATTR_2(temp2_fault, S_IRUGO, show_temp_fault, NULL, 0, 2), SENSOR_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, 0, 3), SENSOR_ATTR_2(temp3_max, S_IRUGO|S_IWUSR, show_temp_max, store_temp_max, 0, 3), SENSOR_ATTR_2(temp3_max_hyst, S_IRUGO|S_IWUSR, show_temp_max_hyst, store_temp_max_hyst, 0, 3), + /* Should be temp3_max_alarm, see temp1_alarm note */ + SENSOR_ATTR_2(temp3_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 3), + SENSOR_ATTR_2(temp3_max_beep, S_IRUGO|S_IWUSR, show_temp_beep, + store_temp_beep, 0, 3), SENSOR_ATTR_2(temp3_crit, S_IRUGO|S_IWUSR, show_temp_crit, store_temp_crit, 0, 3), SENSOR_ATTR_2(temp3_crit_hyst, S_IRUGO, show_temp_crit_hyst, NULL, 0, 3), + SENSOR_ATTR_2(temp3_crit_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 7), + SENSOR_ATTR_2(temp3_crit_beep, S_IRUGO|S_IWUSR, show_temp_beep, + store_temp_beep, 0, 7), SENSOR_ATTR_2(temp3_type, S_IRUGO, show_temp_type, NULL, 0, 3), - SENSOR_ATTR_2(temp3_beep, S_IRUGO|S_IWUSR, show_temp_beep, - store_temp_beep, 0, 3), - SENSOR_ATTR_2(temp3_alarm, S_IRUGO, show_temp_alarm, NULL, 0, 3), SENSOR_ATTR_2(temp3_fault, S_IRUGO, show_temp_fault, NULL, 0, 3), }; [-- Attachment #3: Type: text/plain, Size: 153 bytes --] _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] Problem with: 2008-12-15 12:51 [lm-sensors] Problem with: Hans de Goede ` (2 preceding siblings ...) 2008-12-16 13:40 ` Hans de Goede @ 2008-12-16 14:57 ` Jean Delvare 2008-12-16 15:10 ` Hans de Goede 4 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2008-12-16 14:57 UTC (permalink / raw) To: lm-sensors On Tue, 16 Dec 2008 14:40:07 +0100, Hans de Goede wrote: > Jean Delvare wrote: > > Do you have an update for this patch, please? I'd like to get all > > f71882fg/f8000 patches sorted out today. > > Yes I have (for more then a day) I guess I somehow forgot to send it. > > Its attached now. Applied, thanks. Note that I added a compatibility quirk to lm-sensors 2.10.8 so that it looks for temp#_max_alarm files if they exist. So maybe we can clean up the f71882fg driver in 1 year or so. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] Problem with: 2008-12-15 12:51 [lm-sensors] Problem with: Hans de Goede ` (3 preceding siblings ...) 2008-12-16 14:57 ` Jean Delvare @ 2008-12-16 15:10 ` Hans de Goede 4 siblings, 0 replies; 6+ messages in thread From: Hans de Goede @ 2008-12-16 15:10 UTC (permalink / raw) To: lm-sensors Jean Delvare wrote: > On Tue, 16 Dec 2008 14:40:07 +0100, Hans de Goede wrote: >> Jean Delvare wrote: >>> Do you have an update for this patch, please? I'd like to get all >>> f71882fg/f8000 patches sorted out today. >> Yes I have (for more then a day) I guess I somehow forgot to send it. >> >> Its attached now. > > Applied, thanks. > > Note that I added a compatibility quirk to lm-sensors 2.10.8 so that it > looks for temp#_max_alarm files if they exist. Good, thanks! > So maybe we can clean up > the f71882fg driver in 1 year or so. Lets hope so. Regards, Hans _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-12-16 15:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-12-15 12:51 [lm-sensors] Problem with: Hans de Goede 2008-12-15 13:24 ` Jean Delvare 2008-12-16 10:07 ` Jean Delvare 2008-12-16 13:40 ` Hans de Goede 2008-12-16 14:57 ` Jean Delvare 2008-12-16 15:10 ` Hans de Goede
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.