Re: [lm-sensors] dynamic chip support in libsensors + generic chip

[Hans de Goede <j.w.r.degoede@hhs.nl>]

Jean Delvare wrote:
> On Fri, 25 May 2007 15:47:50 +0200, Jean Delvare wrote:
>> Here is the patch I have come up with, which fixes both problems and
>> makes the generic code work for my ADM1032. Can you please review it and
>> confirm that it doesn't break anything on your side? Thanks.
> 
> I spoke a bit too fast. There was actually a third problem, alarms
> weren't reported. This is because the ADM1032 has per-limit alarm flags
> and the generic code only supported per-channel alarms for temperatures.
> 
> So here is a more complete patch which fixes all three problems.
> 

First of all many thanks for looking add this and trying it, that is just what 
this code needs, much testing. Or some testing on many different kinds of 
hardware to be even more clear.

I've reviewed your patch here are some notes:


> Index: prog/sensors/chips_generic.c
> =================================> --- prog/sensors/chips_generic.c	(révision 4405)
> +++ prog/sensors/chips_generic.c	(copie de travail)

you add a SENSORS_FEATURE_TEMP_MIN_HYST sensor type, but do not check / do 
anything with it in print_generic_chip_temp()

> @@ -208,24 +197,27 @@
>    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_FAULT) &&
>        TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_FAULT) > 0.5) {
>      printf(" FAULT");
> -  } else if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
> -      TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5) {
> +  } else
> +  if ((TEMP_FEATURE(SENSORS_FEATURE_TEMP_ALARM) && 
> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_ALARM) > 0.5)
> +   || (type = MINMAX &&
> +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MIN_ALARM) && 
> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MIN_ALARM) > 0.5)
> +   || (type = MINMAX &&
> +       TEMP_FEATURE(SENSORS_FEATURE_TEMP_MAX_ALARM) && 
> +       TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_MAX_ALARM) > 0.5)) {
>      printf(" ALARM");
>    }
>    printf("\n");
>    

-Is the type = MINMAX check for printing the alarms necessary, doesn't this 
has the risk of not showing an alarm for some chip with a funky combination of 
temp sysfs entries?

-Shouldn't the code somehow show the difference between a min and a max (and a 
generic) alarm? See how print_generic_chip_in() does this

---

I also tested lm_sensors 3.0.0 with your patch and it still works fine on my 
abit uguru motherboard

Some generic notes not directly related to your patch:

This piece of code in print_generic_chip_temp() can be done much simpler:

   if (type = LIM) {
   } else {
     print_temp_info_real(val, max, min, 0.0, type, 1, 1);
   }

Since lim always is 0.0 (this initalized to this) in the non LIM case, this can 
be written as just:
   print_temp_info_real(val, max, min, lim, type, 1, 1);

Will you change this or shall I?

More in general print_generic_chip_temp(), needs a good review by someone who 
is well aware of all possible tempX sysfs entry combo's (iow not by me).


Also print_generic_chip_in(), has some dangerous and unneeded assumptions, for 
example:
-it assumes that if there is a inX_max that there will also always be a inX_min
-it assumes that if there is no inX_max there will also be no alarms

Shall I fix these or will you?

Somewhat less "unneeded" assumption
-it doesn't check for inX_max_alarm without an inX_min_alarm and visa versa, I
  admit this would be a driver bug, as then the driver should have just a
  inX_alarm, but still we might want to concider this.


Well thats about it. About all the "Shall I fix these or will you?", that is 
not me being lazing, but rather me trying to avoid commit conflicts. Its fine 
if you want me to take care of them, but then I guess I should wait until your 
patch is in svn.

Regards,

Hans

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