hwmon: sysfs API updates
in [Kernel]
|
From: Jean Delvare on 5 Jul 2010 03:20 Hi Guenter, On Sun, 4 Jul 2010 21:10:18 -0700, Guenter Roeck wrote: > Signed-off-by: Guenter Roeck <guenter.roeck(a)ericsson.com> > --- > Documentation/hwmon/sysfs-interface | 37 +++++++++++++++++++++++++++++----- > 1 files changed, 31 insertions(+), 6 deletions(-) As usual, I don't have the time to review the code, but I'd like to at least comment on the sysfs interface changes: > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface > index d4e2917..2dcec0f 100644 > --- a/Documentation/hwmon/sysfs-interface > +++ b/Documentation/hwmon/sysfs-interface > @@ -421,11 +421,12 @@ power[1-*]_accuracy Accuracy of the power meter. > Unit: Percent > RO > > -power[1-*]_alarm 1 if the system is drawing more power than the > - cap allows; 0 otherwise. A poll notification is > - sent to this file when the power use exceeds the > - cap. This file only appears if the cap is known > - to be enforced by hardware. > +power[1-*]_alarm 1 if the system is drawing more power than cap > + or max allows; 0 otherwise. A poll notification > + is sent to this file when the power use exceeds > + the cap or max limit. If only cap is supported, > + this file only appears if the cap is known to be > + enforced by hardware. > RO > > power[1-*]_cap If power use rises above this limit, the > @@ -450,6 +451,18 @@ power[1-*]_cap_min Minimum cap that can be set. > Unit: microWatt > RO > > +power[1-*]_max Maximum power. > + Unit: microWatt > + RW > + > +power[1-*]_crit Critical maximum power. > + If power rises to or above this limit, the > + system will take drastic action to reduce power > + consumption, such as a system shutdown. At the > + very least, a power fault will be generated. > + Unit: microWatt > + RO Why RO and not RW as every other limit file? > + > ********** > * Energy * > ********** > @@ -471,8 +484,14 @@ limit-related alarms, not both. The driver should just reflect the hardware > implementation. > > in[0-*]_alarm > +in[0-*]_crit_alarm > +curr[1-*]_alarm > +curr[1-*]_crit_alarm > +power[1-*]_alarm > +power[1-*]_crit_alarm > fan[1-*]_alarm > temp[1-*]_alarm > +temp[1-*]_crit_alarm > Channel alarm > 0: no alarm > 1: alarm The limit-specific alarms (*_crit_alarm) go in the second section, below. And as a matter of fact, you've already added some of them there... > @@ -482,10 +501,17 @@ OR > > in[0-*]_min_alarm > in[0-*]_max_alarm > +in[0-*]_lcrit_alarm > +in[0-*]_crit_alarm > +curr[1-*]_lcrit_alarm > +curr[1-*]_crit_alarm No _min and _max alarm for curr? > +power[1-*]_min_alarm > +power[1-*]_max_alarm > fan[1-*]_min_alarm > fan[1-*]_max_alarm > temp[1-*]_min_alarm > temp[1-*]_max_alarm > +temp[1-*]_lcrit_alarm > temp[1-*]_crit_alarm > Limit alarm > 0: no alarm > @@ -497,7 +523,6 @@ to notify open diodes, unconnected fans etc. where the hardware > supports it. When this boolean has value 1, the measurement for that > channel should not be trusted. > > -in[0-*]_fault I've removed it already in a separate patch, so your patch won't apply if you try to remove it again. > fan[1-*]_fault > temp[1-*]_fault > Input fault condition In general, I'm happy with the proposed changes. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Guenter Roeck on 5 Jul 2010 17:50 Hi Jean, I noticed I did not copy the list with my reply, so here are are again, with a couple of additional comments. On Mon, Jul 05, 2010 at 03:18:57AM -0400, Jean Delvare wrote: > Hi Guenter, > > On Sun, 4 Jul 2010 21:10:18 -0700, Guenter Roeck wrote: > > Signed-off-by: Guenter Roeck <guenter.roeck(a)ericsson.com> > > --- > > Documentation/hwmon/sysfs-interface | 37 +++++++++++++++++++++++++++++----- > > 1 files changed, 31 insertions(+), 6 deletions(-) > > As usual, I don't have the time to review the code, but I'd like to at > least comment on the sysfs interface changes: > I appreciate any feedback I can get. > > > > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface > > index d4e2917..2dcec0f 100644 > > --- a/Documentation/hwmon/sysfs-interface > > +++ b/Documentation/hwmon/sysfs-interface > > @@ -421,11 +421,12 @@ power[1-*]_accuracy Accuracy of the power meter. > > Unit: Percent > > RO > > > > -power[1-*]_alarm 1 if the system is drawing more power than the > > - cap allows; 0 otherwise. A poll notification is > > - sent to this file when the power use exceeds the > > - cap. This file only appears if the cap is known > > - to be enforced by hardware. > > +power[1-*]_alarm 1 if the system is drawing more power than cap > > + or max allows; 0 otherwise. A poll notification > > + is sent to this file when the power use exceeds > > + the cap or max limit. If only cap is supported, > > + this file only appears if the cap is known to be > > + enforced by hardware. > > RO > > > > power[1-*]_cap If power use rises above this limit, the > > @@ -450,6 +451,18 @@ power[1-*]_cap_min Minimum cap that can be set. > > Unit: microWatt > > RO > > > > +power[1-*]_max Maximum power. > > + Unit: microWatt > > + RW > > + > > +power[1-*]_crit Critical maximum power. > > + If power rises to or above this limit, the > > + system will take drastic action to reduce power > > + consumption, such as a system shutdown. At the > > + very least, a power fault will be generated. > > + Unit: microWatt > > + RO > > Why RO and not RW as every other limit file? > Cut-and-paste error. I'll fix it. > > + > > ********** > > * Energy * > > ********** > > @@ -471,8 +484,14 @@ limit-related alarms, not both. The driver should just reflect the hardware > > implementation. > > > > in[0-*]_alarm > > +in[0-*]_crit_alarm > > +curr[1-*]_alarm > > +curr[1-*]_crit_alarm > > +power[1-*]_alarm > > +power[1-*]_crit_alarm > > fan[1-*]_alarm > > temp[1-*]_alarm > > +temp[1-*]_crit_alarm > > Channel alarm > > 0: no alarm > > 1: alarm > > The limit-specific alarms (*_crit_alarm) go in the second section, > below. And as a matter of fact, you've already added some of them > there... > Ok, makes sense. > > @@ -482,10 +501,17 @@ OR > > > > in[0-*]_min_alarm > > in[0-*]_max_alarm > > +in[0-*]_lcrit_alarm > > +in[0-*]_crit_alarm > > +curr[1-*]_lcrit_alarm > > +curr[1-*]_crit_alarm > > No _min and _max alarm for curr? > Oversight. pmbus devices don't support currX_min and currX_min_alarm, only currX_lcrit and currX_lcrit_alarm. The ltc4245 driver already supports currX_max_alarm, though, so I'll add both for consistency. The ltc4245 driver only supports currX_max_alarm, not currX_min_alarm, though. Wonder if that should be changed to currX_alarm, to more closely follow the API. Let me know and I'll submit a patch if needed. > > +power[1-*]_min_alarm > > +power[1-*]_max_alarm > > fan[1-*]_min_alarm > > fan[1-*]_max_alarm > > temp[1-*]_min_alarm > > temp[1-*]_max_alarm > > +temp[1-*]_lcrit_alarm > > temp[1-*]_crit_alarm > > Limit alarm > > 0: no alarm > > @@ -497,7 +523,6 @@ to notify open diodes, unconnected fans etc. where the hardware > > supports it. When this boolean has value 1, the measurement for that > > channel should not be trusted. > > > > -in[0-*]_fault > > I've removed it already in a separate patch, so your patch won't apply > if you try to remove it again. > Ok, good point. I'll take it out. > > fan[1-*]_fault > > temp[1-*]_fault > > Input fault condition > > In general, I'm happy with the proposed changes. > Thanks a lot for your time! Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on 6 Jul 2010 03:30 On Mon, 5 Jul 2010 14:39:53 -0700, Guenter Roeck wrote: > Hi Jean, > > I noticed I did not copy the list with my reply, so here are are again, > with a couple of additional comments. > > On Mon, Jul 05, 2010 at 03:18:57AM -0400, Jean Delvare wrote: > > On Sun, 4 Jul 2010 21:10:18 -0700, Guenter Roeck wrote: > > > @@ -482,10 +501,17 @@ OR > > > > > > in[0-*]_min_alarm > > > in[0-*]_max_alarm > > > +in[0-*]_lcrit_alarm > > > +in[0-*]_crit_alarm > > > +curr[1-*]_lcrit_alarm > > > +curr[1-*]_crit_alarm > > > > No _min and _max alarm for curr? > > > Oversight. pmbus devices don't support currX_min and currX_min_alarm, only > currX_lcrit and currX_lcrit_alarm. The ltc4245 driver already supports > currX_max_alarm, though, so I'll add both for consistency. > > The ltc4245 driver only supports currX_max_alarm, not currX_min_alarm, though. > Wonder if that should be changed to currX_alarm, to more closely follow the API. > Let me know and I'll submit a patch if needed. Both should be defined and supported, and the driver author chooses what fits his/her chip best. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Pages: 1 Prev: linux-next: Tree for July 5 Next: compal-laptop/fujitsu-laptop/msi-laptop: make dmi_check_cb |