[lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on alarms

[lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on alarms

[Engelmayer Christian]

From: Christian Engelmayer <christian.engelmayer@frequentis.com>

Added fan limit alarm 'max_alarm' to the alarm section.

Signed-off-by: Christian Engelmayer <christian.engelmayer@frequentis.com>
---
No longer mentioning non standard gpio alarms after clarification by the
maintainer.

--- Documentation/hwmon/sysfs-interface.orig	2009-05-21 23:03:27.000000000
+0200
+++ Documentation/hwmon/sysfs-interface	2009-05-25 22:08:13.000000000 +0200
@@ -389,6 +389,7 @@ OR
 in[0-*]_min_alarm
 in[0-*]_max_alarm
 fan[1-*]_min_alarm
+fan[1-*]_max_alarm
 temp[1-*]_min_alarm
 temp[1-*]_max_alarm
 temp[1-*]_crit_alarm


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

Re: [lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on

[Hans de Goede]

On 05/25/2009 10:15 PM, Engelmayer Christian wrote:
> From: Christian Engelmayer<christian.engelmayer@frequentis.com>
>
> Added fan limit alarm 'max_alarm' to the alarm section.
>

Looks good to me now,

Acked-by: Hans de Goede <hdegoede@redhat.com>


> Signed-off-by: Christian Engelmayer<christian.engelmayer@frequentis.com>
> ---
> No longer mentioning non standard gpio alarms after clarification by the
> maintainer.
>
> --- Documentation/hwmon/sysfs-interface.orig	2009-05-21 23:03:27.000000000
> +0200
> +++ Documentation/hwmon/sysfs-interface	2009-05-25 22:08:13.000000000 +0200
> @@ -389,6 +389,7 @@ OR
>   in[0-*]_min_alarm
>   in[0-*]_max_alarm
>   fan[1-*]_min_alarm
> +fan[1-*]_max_alarm
>   temp[1-*]_min_alarm
>   temp[1-*]_max_alarm
>   temp[1-*]_crit_alarm
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>


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

Re: [lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on

[Jean Delvare]

On Tue, 26 May 2009 11:01:26 +0200, Hans de Goede wrote:
> On 05/25/2009 10:15 PM, Engelmayer Christian wrote:
> > From: Christian Engelmayer<christian.engelmayer@frequentis.com>
> >
> > Added fan limit alarm 'max_alarm' to the alarm section.
> >
> 
> Looks good to me now,
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

I've applied the patch now. Please note that I did have to edit it
manually so that it would apply. Christian, please fix/setup your
e-mail client so that it doesn't wrap long lines.

One thing I am curious about is how we can have fan[1-*]_max_alarm
while we do not have fan[1-*]_max according to our documentation. Some
drivers still seem to implement this feature (dme1737, adt7470,
applesmc) so we should document it, whoulsn't we?

But the max6650 driver, which is what Christian is working on, does not
have this feature, it doesn't even have fan1_min as far as I can see.
This makes me curious about what the fan1_max_alarm and fan1_min_alarm
flags are supposed to mean for the MAX6650/1 device? Apparently not what
Documentation/hwmon/sysfs-interface says... which would be kind of
problematic.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on

[Engelmayer Christian]

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: Tuesday, May 26, 2009 11:16 AM
> To: Hans de Goede
> Cc: Engelmayer Christian; lm-sensors@lm-sensors.org
> Subject: Re: [lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on
alarms
> 
> On Tue, 26 May 2009 11:01:26 +0200, Hans de Goede wrote:
> > On 05/25/2009 10:15 PM, Engelmayer Christian wrote:
> > > From: Christian Engelmayer<christian.engelmayer@frequentis.com>
> > >
> > > Added fan limit alarm 'max_alarm' to the alarm section.
> > >
> >
> > Looks good to me now,
> >
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> I've applied the patch now. Please note that I did have to edit it
> manually so that it would apply. Christian, please fix/setup your
> e-mail client so that it doesn't wrap long lines.

Sorry for the inconvenience. I can see my mails being altered, although the
client settings would be correct. I will fix it asap.

> 
> One thing I am curious about is how we can have fan[1-*]_max_alarm
> while we do not have fan[1-*]_max according to our documentation. Some
> drivers still seem to implement this feature (dme1737, adt7470,
> applesmc) so we should document it, whoulsn't we?

Seems consequential.

> 
> But the max6650 driver, which is what Christian is working on, does not
> have this feature, it doesn't even have fan1_min as far as I can see.
> This makes me curious about what the fan1_max_alarm and fan1_min_alarm
> flags are supposed to mean for the MAX6650/1 device? Apparently not what
> Documentation/hwmon/sysfs-interface says... which would be kind of
> problematic.

I understood the interface that the min/max alarms shall be raised when eg.
a fan spins not fast enough/too fast, whereas the minimum/maximum limits are
taken from the _min/_max limit files.

The max6650 driver does not implement fan1_min/fan1_max because the chip does
not provide support for configurable limits. The chip raises the affected
alarms in case the desired speed cannot be attained in closed-loop mode. The
limits are a direct result of the minimum/maximum output values supported by
the DAC. eg. minimum alarm is raised in case the largest available voltage
has
been applied across the fan, which means that the fan is unable to spin as
fast as the desired speed.

That's my reasoning for choosing the mapping to
fan1_min_alarm/fan1_max_alarm,
although the limits are preset by the hardware and not configurable. Is that
a too generous interpretation of the interface to You?

> 
> --
> Jean Delvare

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

Re: [lm-sensors] [PATCH v2 1/1] hwmon: Updated documentation on

[Jean Delvare]

Hi Christian,

On Tue, 26 May 2009 12:06:36 +0200, Engelmayer Christian wrote:
> > -----Original Message-----
> > From: Jean Delvare [mailto:khali@linux-fr.org]
> > (...)
> > One thing I am curious about is how we can have fan[1-*]_max_alarm
> > while we do not have fan[1-*]_max according to our documentation. Some
> > drivers still seem to implement this feature (dme1737, adt7470,
> > applesmc) so we should document it, whoulsn't we?
> 
> Seems consequential.

I've updated your patch.

For completeness, amongst the 3 drivers I listed above, only adt7470
implements fan[1-*]_max the way it is supposed to. For dme1737, the
value is used to calibrate the speed monitoring logic. For applesmc,
I'm not sure (I don't have any datasheet) but the fact that the
attribute is read-only suggests that it exposes the physical/nominal
maximum speed of the fan, rather than a user-selected speed limit.

This can probably be explained by the fact that the maximum fan speed
limits aren't terribly useful. Your system is at risk if a fan stalls,
but not if it spins very fast (it can be very noisy then, but that's
another issue.) The only indirect indication about a hardware problem,
is when the system is thermo-regulated, a fast-spinning fan suggests a
very high temperature. But this condition is much better detected with
a thermal sensor, of course.

As a matter of fact, libsensors doesn't even support fan[1-*]_max
attributes (if someone wants to implement that... go ahead.)

> > But the max6650 driver, which is what Christian is working on, does not
> > have this feature, it doesn't even have fan1_min as far as I can see.
> > This makes me curious about what the fan1_max_alarm and fan1_min_alarm
> > flags are supposed to mean for the MAX6650/1 device? Apparently not what
> > Documentation/hwmon/sysfs-interface says... which would be kind of
> > problematic.
> 
> I understood the interface that the min/max alarms shall be raised when eg.
> a fan spins not fast enough/too fast, whereas the minimum/maximum limits are
> taken from the _min/_max limit files.

This is correct.

> The max6650 driver does not implement fan1_min/fan1_max because the chip does
> not provide support for configurable limits. The chip raises the affected
> alarms in case the desired speed cannot be attained in closed-loop mode. The
> limits are a direct result of the minimum/maximum output values supported by
> the DAC. eg. minimum alarm is raised in case the largest available voltage
> has
> been applied across the fan, which means that the fan is unable to spin as
> fast as the desired speed.

This is a somewhat different condition. We do not have a dedicated sysfs
interface for this yet. Only a few supported devices/drivers implement
the fan speed target mode.

> That's my reasoning for choosing the mapping to
> fan1_min_alarm/fan1_max_alarm,
> although the limits are preset by the hardware and not configurable. Is that
> a too generous interpretation of the interface to You?

I'm not sure. On the one hand, these conditions aren't that different
from user-defined speed limits, and adding specific code to libsensors
and monitoring applications might be considered overkill. Both are
about the fan speed being above or below some limits. But on the other
hand, my reluctance comes from the fact that some chips actually
implement both limit types, with separate alarm flags for each (even
though I don't think any driver currently exposes those), and I'm not
sure how to make that fit in the current interface.

Opinions are welcome. In the meantime, I have no objection to you using
the current attribute names for the max6650 driver.

Don't you think it would make sense to expose the fan1_min and fan1_max
values which determine when the alarm flags will be raised? I presume
they can be computed from the target speed value.

-- 
Jean Delvare

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