Some thoughts about the sysfs interface (repost)

Some thoughts about the sysfs interface (repost)

[Jean Delvare]

Hi all,

(Same player, try again, *with* the patch, that time.)

I had to update the doc/developers/proc document for the lm83 driver (it
has four temperature channels). The same change will be needed when the
driver makes it into Linux 2.6. Greg, attached is a patch I suggest you
apply at the same time you'll apply my lm83 patch to Linux-2.6.0.

Some points I'd like to discuss about:

1* Most entries are described using sentences like "Fixed point value in
form XXXXX and should be divided by 1000 to get degrees Celsius". I
don't understand the "fixed point" thing, nor do I understand the
informational value of "in form XXXXX". Wouldn't it be much clearer to
write "Integer value, thousandth of degrees Celsius"?

2* temp_min[1-3] reads: "Temperature min or hysteresis value.". This is
a Bad Idea (TM) IMHO. One very interesting point in the sysfs interface
over the procfs one is that it could be easily be made chip-independant
(libsensors and sensors could then be drastically simplified). If the
file is named temp_minN, it has to be a min temp. Be it a hysteresis
one, let's call the file temp_hystN instead. It's in no way harder to do
it that way, and it will be way easier for the interface users
(includind ourselves) if we stick to the one name, one use philosophy.

3* One additional note about hysteresis temperatures (and critical
temperatures also). Some chips have several temperature channels, but a
single hysteresis and/or critical temperature register, common to all
channels. For example, the LM83 has four temperature channels, four max
values, but one single critical value. The ADM1032 has two temperature
channels, two min values, two max values, two critical temperatures, but
a single, relative hysteresis value that applies to both critical
values.

I propose that chips that values that are common to all channels simply
have no digit appended. That's how I made it in the lm83 driver.

Now, for the hysteresis, that's more complicated, since not only it may
apply to one or more channels, but it can also happen to one or more
limit, and be relative or absolute. I think that hysteresis files should
be named after the limit they are an hysteresis for. In the case of the
lm90 driver, we would have the following files:
  temp1_input
  temp2_input
  temp1_min
  temp2_min
  temp1_max
  temp2_max
  temp1_crit
  temp2_crit
  temp_crit_hyst
Note that the digit is also omitted, since the value is common to both
channels. An hysteresis that would apply to all temperatures of one
channel would be named "temp1_hyst". An hysteresis that would apply to
more than one channel and more than one limit for each channel would be
called temp_hyst. I think you get the idea.

Remains the problem of relative hyst vs. absolute hyst temperature. The
docs say we should prefer absolute, but in the case of the lm90 driver,
it isn't possible (because the hysteresis value applies to two critical
limits that can have different values). The only thing I can think of is
having different names again. For example, relative hysteresis would be
appended _rhyst while absolute ones would be appended _ahyst (or simply
_hyst if we want to provide some kind of backward compatibility, or if
we consider that absolute values are the default).

A very different approach would be to simulate as much values as we
need, regardless of how many registers there are. In the case of the
lm83 driver, we would have temp1_crit and temp2_crit instead of
temp_crit, and both files would access the same register. The advantage
of this method is that all future chips will necessarily be supportable,
which isn't the case with my first proposal. We could imagine a chip
that has four temperature channels, one critical limit for channels 1
and 2, and one critical limit for channels 3 and 4. That's something my
first model doesn't handle, while the second does. I can say how likely
it is to see such a chip someday. All other scenarios I can think of at
the moment are OK with that first model. The drawbacks of the second
method are: we break the one-one link between sysfs and the hardware
(which helps a lot to understand how a chip works, IMHO), we create more
sysfs entries, and writing to a register might have side effects to the
other registers (might be already happening in some cases anyway).
Another good point is that it solves the relative hysteresis problem
mentioned above.

Other possibilities may exist (might involve symlinks for example). It
would be great to come to a decision before going on with porting chip
drivers, methinks.

Everyone let me know your opinion about the topic, should you have one.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
-------------- next part --------------
--- linux-2.6.0-test9/Documentation/i2c/sysfs-interface	Sun Nov  9 11:13:32 2003
+++ linux-2.6.0-test9/Documentation/i2c/sysfs-interface.lm83	Sun Nov  9 12:40:48 2003
@@ -144,7 +144,7 @@
 		Integers 1,2,3, or thermistor Beta value (3435)
 		Read/Write.
 
-temp_max[1-3]	Temperature max value.
+temp_max[1-4]	Temperature max value.
 		Fixed point value in form XXXXX and should be divided by
 		1000 to get degrees Celsius.
 		Read/Write value.
@@ -156,13 +156,22 @@
 		NOT a delta from the max value.
 		Read/Write value.
 
-temp_input[1-3] Temperature input value.
+temp_input[1-4] Temperature input value.
+		Fixed point value in form XXXXX and should be divided by
+		1000 to get degrees Celsius.
 		Read only value.
 
+temp_crit	Temperature critical value, typically greater that all
+		temp_max values.
+		Fixed point value in form XXXXX and should be divided by
+		1000 to get degrees Celsius.
+		Common to all temperature channels.
+		Read/Write value.
+
 		If there are multiple temperature sensors, temp_*1 is
 		generally the sensor inside the chip itself, generally
-		reported as "motherboard temperature".  temp_*2 and
-		temp_*3 are generally sensors external to the chip
+		reported as "motherboard temperature".  temp_*2 to
+		temp_*4 are generally sensors external to the chip
 		itself, for example the thermal diode inside the CPU or
 		a thermistor nearby.
 

Some thoughts about the sysfs interface (repost)

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali@linux-fr.org> [2003-11-09 12:51:57 +0100]:

> 1* Most entries are described using sentences like "Fixed point value in
> form XXXXX and should be divided by 1000 to get degrees Celsius". I
> don't understand the "fixed point" thing, nor do I understand the

A fixed point number is not "just" an integer.  It has a whole part and/or
a fractional part.  The location of the split is "fixed" as a matter of
interpretation, nothing more.  E.g. the LM75 temperature reading is in
9 bits, with the LSB representing 0.5 degrees C.

The computer representation of plain integers is an example of fixed point -
it just happens that there is no fractional part.

Here is a link with more formal language, if you're into that:

http://www.cs.tau.ac.il/~nin/mivne99/fp.htm

> informational value of "in form XXXXX". Wouldn't it be much clearer to
> write "Integer value, thousandth of degrees Celsius"?

I like to document fixed point representations this way, e.g.:

/* temp: 0.5C/bit (-55C to +125C) */

> 2* temp_min[1-3] reads: "Temperature min or hysteresis value.". This is
> a Bad Idea (TM) IMHO. One very interesting point in the sysfs interface
> over the procfs one is that it could be easily be made chip-independant
> (libsensors and sensors could then be drastically simplified). If the
> file is named temp_minN, it has to be a min temp. Be it a hysteresis
> one, let's call the file temp_hystN instead. It's in no way harder to do
> it that way, and it will be way easier for the interface users
> (includind ourselves) if we stick to the one name, one use philosophy.

Agreed.

> 3* One additional note about hysteresis temperatures (and critical
> temperatures also). Some chips have several temperature channels, but a
> single hysteresis and/or critical temperature register, common to all
> channels. For example, the LM83 has four temperature channels, four max
> values, but one single critical value. The ADM1032 has two temperature
> channels, two min values, two max values, two critical temperatures, but
> a single, relative hysteresis value that applies to both critical
> values.
> 
> I propose that chips that values that are common to all channels simply
> have no digit appended. That's how I made it in the lm83 driver.

No, I like your other approach better... (1)

> Now, for the hysteresis, that's more complicated, since not only it may
> apply to one or more channels, but it can also happen to one or more
> limit, and be relative or absolute. I think that hysteresis files should

Re: relative vs. absolute... this is a detail of the hardware that
doesn't concern the user.  Whatever the hardware, the driver interface
should use absolute temps. (Oops, then I kept reading...)

> be named after the limit they are an hysteresis for. In the case of the
> lm90 driver, we would have the following files:
>   temp1_input
>   temp2_input
>   temp1_min
>   temp2_min
>   temp1_max
>   temp2_max
>   temp1_crit
>   temp2_crit
>   temp_crit_hyst
> Note that the digit is also omitted, since the value is common to both
> channels. An hysteresis that would apply to all temperatures of one
> channel would be named "temp1_hyst". An hysteresis that would apply to
> more than one channel and more than one limit for each channel would be
> called temp_hyst. I think you get the idea.
> 
> Remains the problem of relative hyst vs. absolute hyst temperature. The
> docs say we should prefer absolute, but in the case of the lm90 driver,
> it isn't possible (because the hysteresis value applies to two critical
> limits that can have different values). The only thing I can think of is

*Sigh*  Sometimes it feels like there is a conspiracy of h/w designers
to break every least little abstraction we s/w people try to make.

So, just make two absolute hyst values which are linked... a change in
one forces a change in the other.  Userland s/w should not assume any
sensors sysfs file to be static anyway.  It might be a little suprising
to the user - just document it as the hardware limitation that it is and
move on.

> A very different approach would be to simulate as much values as we
> need, regardless of how many registers there are. In the case of the
> lm83 driver, we would have temp1_crit and temp2_crit instead of
> temp_crit, and both files would access the same register. The advantage

(1) I like this.  It greatly simplifies the userland side.  Ultimately,
we should be able to add a chip driver and have immediate library support
for free, just by sticking to these conventions.

> of this method is that all future chips will necessarily be supportable,
> which isn't the case with my first proposal. We could imagine a chip
> that has four temperature channels, one critical limit for channels 1
> and 2, and one critical limit for channels 3 and 4. That's something my
> first model doesn't handle, while the second does. I can say how likely
> it is to see such a chip someday. All other scenarios I can think of at
> the moment are OK with that first model. The drawbacks of the second

Agreed.

> method are: we break the one-one link between sysfs and the hardware
> (which helps a lot to understand how a chip works, IMHO), we create more

Yes, but it's either that or make userland more complicated.

> sysfs entries, and writing to a register might have side effects to the
> other registers (might be already happening in some cases anyway).
> Another good point is that it solves the relative hysteresis problem
> mentioned above.

Right, and possibly others.

> Other possibilities may exist (might involve symlinks for example). It
> would be great to come to a decision before going on with porting chip
> drivers, methinks.

Agreed - a little bit of thought now saves a lot of work later.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Some thoughts about the sysfs interface (repost)

[Greg KH]

On Sun, Nov 09, 2003 at 12:51:57PM +0100, Jean Delvare wrote:
> 
> Hi all,
> 
> (Same player, try again, *with* the patch, that time.)
> 
> I had to update the doc/developers/proc document for the lm83 driver (it
> has four temperature channels). The same change will be needed when the
> driver makes it into Linux 2.6. Greg, attached is a patch I suggest you
> apply at the same time you'll apply my lm83 patch to Linux-2.6.0.
> 
> Some points I'd like to discuss about:
> 
> 1* Most entries are described using sentences like "Fixed point value in
> form XXXXX and should be divided by 1000 to get degrees Celsius". I
> don't understand the "fixed point" thing, nor do I understand the
> informational value of "in form XXXXX". Wouldn't it be much clearer to
> write "Integer value, thousandth of degrees Celsius"?

They are pretty much the same thing, right?  Either way is fine with me.

> 2* temp_min[1-3] reads: "Temperature min or hysteresis value.". This is
> a Bad Idea (TM) IMHO. One very interesting point in the sysfs interface
> over the procfs one is that it could be easily be made chip-independant
> (libsensors and sensors could then be drastically simplified). If the
> file is named temp_minN, it has to be a min temp. Be it a hysteresis
> one, let's call the file temp_hystN instead. It's in no way harder to do
> it that way, and it will be way easier for the interface users
> (includind ourselves) if we stick to the one name, one use philosophy.

Ah, I like this proposal, it makes a lot more sense.  Care to also fix
up the drivers that are in the current 2.6 tree to follow this rule?

> 3* One additional note about hysteresis temperatures (and critical
> temperatures also). Some chips have several temperature channels, but a
> single hysteresis and/or critical temperature register, common to all
> channels. For example, the LM83 has four temperature channels, four max
> values, but one single critical value. The ADM1032 has two temperature
> channels, two min values, two max values, two critical temperatures, but
> a single, relative hysteresis value that applies to both critical
> values.
> 
> I propose that chips that values that are common to all channels simply
> have no digit appended. That's how I made it in the lm83 driver.

Makes sense.

> Now, for the hysteresis, that's more complicated, since not only it may
> apply to one or more channels, but it can also happen to one or more
> limit, and be relative or absolute. I think that hysteresis files should
> be named after the limit they are an hysteresis for. In the case of the
> lm90 driver, we would have the following files:
>   temp1_input
>   temp2_input
>   temp1_min
>   temp2_min
>   temp1_max
>   temp2_max
>   temp1_crit
>   temp2_crit
>   temp_crit_hyst
> Note that the digit is also omitted, since the value is common to both
> channels. An hysteresis that would apply to all temperatures of one
> channel would be named "temp1_hyst". An hysteresis that would apply to
> more than one channel and more than one limit for each channel would be
> called temp_hyst. I think you get the idea.

As you stated, this has some problems.  I think we should probably not
do this for now, and if we have some real compilcated chips in the
future, that we need these changes for, we can revisit it.

Sound ok?

> --- linux-2.6.0-test9/Documentation/i2c/sysfs-interface	Sun Nov  9 11:13:32 2003
> +++ linux-2.6.0-test9/Documentation/i2c/sysfs-interface.lm83	Sun Nov  9 12:40:48 2003

Should I still apply this patch?

thanks,

greg k-h

Some thoughts about the sysfs interface (repost)

[Jean Delvare]

> A fixed point number is not "just" an integer.  It has a whole part
> and/or a fractional part.  The location of the split is "fixed" as a
> matter of interpretation, nothing more.  E.g. the LM75 temperature
> reading is in 9 bits, with the LSB representing 0.5 degrees C.

Thanks for the clarification. I had never seen fixed point arithmerics
that way before. I must have been troubled by the fact there was no
(visible) dot, so it couldn't be anything but an integer.

This doesn't make the "in form XXXXX" clearer. Wouldn't something like
"with a 3 digit fractional part" be better?

The fact that fixed point binary representation cannot easily be
converted to and from fixed point decimal representation adds to (my)
confusion. What motivated the choice of fixed point decimals? Fixed
point binaries would have been faster since this is how chipsets do
work. But I agree it would have been less easily readable from sysfs.
That said, users are not supposed to browse sysfs to get the hardware
monitoring information for their system.

> > informational value of "in form XXXXX". Wouldn't it be much clearer
> > to write "Integer value, thousandth of degrees Celsius"?
> 
> I like to document fixed point representations this way, e.g.:
> 
> /* temp: 0.5C/bit (-55C to +125C) */

Do you suggest we could say something like "temp1_input: 1mC/bit"? Using
"bits" here isn't really natural since we are in fixed point decimals.
This is why speaking of integers in a given unit (millidegree Celcius
here) makes more sense to me. But it might be just me.

> So, just make two absolute hyst values which are linked... a change in
> one forces a change in the other.  Userland s/w should not assume any
> sensors sysfs file to be static anyway.  It might be a little
> suprising to the user - just document it as the hardware limitation
> that it is and move on.

I would prefer it that way too, or at least I do think so at the moment.

> (1) I like this.  It greatly simplifies the userland side. 
> Ultimately, we should be able to add a chip driver and have immediate
> library support for free, just by sticking to these conventions.

This is precisely what I am after (and what I believe the sysfs
interface was intended for from the very begnining).

> > The drawbacks of the second
> > method are: we break the one-one link between sysfs and the hardware
> > (which helps a lot to understand how a chip works, IMHO), we create
> > more
> 
> Yes, but it's either that or make userland more complicated.

You got it, there's a choice to make. I believe that a simplified (and
over-standardized) interface with user-land would be great enough to
allow for some sacrifices.

That said, it would obvisouly require some changes to existing drivers.
At least my lm83, not taken a look at the others yet.

Thanks a lot for your insights and opinions.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Some thoughts about the sysfs interface (repost)

[Jean Delvare]

> > 1* Most entries are described using sentences like "Fixed point
> > value in form XXXXX and should be divided by 1000 to get degrees
> > Celsius". I don't understand the "fixed point" thing, nor do I
> > understand the informational value of "in form XXXXX". Wouldn't it
> > be much clearer to write "Integer value, thousandth of degrees
> > Celsius"?
> 
> They are pretty much the same thing, right?  Either way is fine with
> me.

Yes, that means the same (or I wouldn't propose it as a replacement).
But it sound more understandable to me. I might not be a genius, but it
took me some time before I understood what it was supposed to me - and I
am in the sensors business for quite some time now. So I suppose that a
newcomer would feel as confused as I first was. Especially, the "in form
XXXXX" part has no particular sense I can think of. I'd go for either
"fixed point value, 3 digit fractional part, in degrees Celsius", or
"integer value, in thousandth of degrees Celsius", the one you like
better.

> > 2* temp_min[1-3] reads: "Temperature min or hysteresis value.". This
> > is a Bad Idea (TM) IMHO. One very interesting point in the sysfs
> > interface over the procfs one is that it could be easily be made
> > chip-independant(libsensors and sensors could then be drastically
> > simplified). If the file is named temp_minN, it has to be a min
> > temp. Be it a hysteresis one, let's call the file temp_hystN
> > instead. It's in no way harder to do it that way, and it will be way
> > easier for the interface users(includind ourselves) if we stick to
> > the one name, one use philosophy.
> 
> Ah, I like this proposal, it makes a lot more sense.  Care to also fix
> up the drivers that are in the current 2.6 tree to follow this rule?

Greg, Greg... You are soooo predictable ;) OK, will do.

> As you stated, this has some problems.  I think we should probably not
> do this for now, and if we have some real compilcated chips in the
> future, that we need these changes for, we can revisit it.
> 
> Sound ok?

Not really. I'd prefer if we could decide ourselves for an interface we
could stick to for the next 10 years or so (yes, I'm a dreamer). If such
a chip exist someday, we'll have to fix *all* the existing drivers to
match the new interface. Or at least the ones that use either critical,
either hysteresis - and that's most of them.

I would tend to think that Mark Hoffman's point of view should be
considered. It matches one of my proposal, and guarantees that what a
sysfs file does is all in its name. This is a requirement if we want
libsensors to be simplified and free of chipset-dependant code. The only
thing it breaks is the one-one link between sysfs file and hardware
register, but that's not a high price to pay if you consider what we get
in exchange.

> > --- linux-2.6.0-test9/Documentation/i2c/sysfs-interface
> > +++ linux-2.6.0-test9/Documentation/i2c/sysfs-interface.lm83
> 
> Should I still apply this patch?

Yes, that's a different problem. The LM83 has 4 temperature channels
while the maximum specified in the docs is 3. The patch mostly fixes
that. Since it was already applied to our CVS docs, you can apply it on
your side too. That said, it doesn't really make sense before you also
apply the lm83 driver patch, which is delayed if I remember correctly.

If you prefer that I integrate this patch into the one I'll do for the
min vs. hyst thing, I can do that too. But I heard you prefer many small
patches over one big ;) (OMG am I really so evil?)

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/