libsensors for linux 2.6

libsensors for linux 2.6

[Jean Delvare]

> I broke down and started integrating Danny's patch.
> stay tuned for progress...

Just one or two questions, since I haven't been watching the thread very
carefully - no time for this, sorry.

You might have seen my recent post about sysfs file naming conventions,
with questions about how to handle values that relate to more than one
sensor, relative hysteresis values and the such. The goal of this naming
convention is to have a common, standardized interface with libs and/or
user-space programs. The fact that libsensors has specific code for each
and every chipset is a problem. With a sysfs interface where each file
name can only represent one thing, this shouldn't be needed anymore.
This basically means that libsensors as we know it should *not* be
needed for Linux 2.6.

Of course, we would still need a way to tweak values, set limits, set
labels and ignore values. The source for this is in our sensors.conf
file. That should almost be the only common part between the old and the
new library. This make me wonder if this is a good idea to try having
only one library for both kernel branches. Why don't we start a 2.6
dedicated library? This would be libsensors.so.3.

That said, this would be better if we could all agree of the sysfs file
naming convention first. Please post your comments on the other thread.
Greg KH and Mark Hoffman already have clarified a number of things and
some points look OK now, but there are still some problems left.

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

libsensors for linux 2.6

[Mark Studebaker]

I think I'll stay on this thread.

I heavily modified and checked in Danny's patch, with the following
features:
	- Supports both sysfs and /proc (#ifdef SYSFS removed)
	- Requires two additions to struct sensors_chip_feature: 
	  sysfs file name and sysfs magnitude
	  This is the struct in the massive lib/chips.c file.
	  I've only updated w83781d for now for my testing.
	- version advanced to 3.0.0

I did it this way to minimize typing. The sysfs name and magnitude are
optional;
it will use the old name and magnitude if they aren't present.

It's been a long time and nobody had stepped up to even doing this much.
Some simpler libsensors or none at all may be a good long-term goal.
If gkrellm is using sysfs values directly with some sort of
configuration file
perhaps we should look at that as a good example of how to do things.

But I think this approach is good for now. Another hour of modifying
lib/chips.c
and it will be done. But if you're getting ready to change the standard
names
and/or magnitudes I better hold off...
Helpful hint: the more names stay the same as they were before the less
typing there will be (inx, inx_min, inx_max....). Unfortunately
pretty much everything is different now. The reason it's called
"in_input1"
instead of "in1" escapes me.

On the larger point, don't think I'm disagreeing with you.
None of this changes my opinion that libsensors sucks.

One problem I ran into:

For temp_xxx, sysfs_interface says divide by 1000 but actually it's 100
(for w83781d anyway)


mds



Jean Delvare wrote:
> 
> > I broke down and started integrating Danny's patch.
> > stay tuned for progress...
> 
> Just one or two questions, since I haven't been watching the thread very
> carefully - no time for this, sorry.
> 
> You might have seen my recent post about sysfs file naming conventions,
> with questions about how to handle values that relate to more than one
> sensor, relative hysteresis values and the such. The goal of this naming
> convention is to have a common, standardized interface with libs and/or
> user-space programs. The fact that libsensors has specific code for each
> and every chipset is a problem. With a sysfs interface where each file
> name can only represent one thing, this shouldn't be needed anymore.
> This basically means that libsensors as we know it should *not* be
> needed for Linux 2.6.
> 
> Of course, we would still need a way to tweak values, set limits, set
> labels and ignore values. The source for this is in our sensors.conf
> file. That should almost be the only common part between the old and the
> new library. This make me wonder if this is a good idea to try having
> only one library for both kernel branches. Why don't we start a 2.6
> dedicated library? This would be libsensors.so.3.
> 
> That said, this would be better if we could all agree of the sysfs file
> naming convention first. Please post your comments on the other thread.
> Greg KH and Mark Hoffman already have clarified a number of things and
> some points look OK now, but there are still some problems left.
> 
> --
> Jean Delvare
> http://www.ensicaen.ismra.fr/~delvare/

libsensors for linux 2.6

[Jean Delvare]

> But I think this approach is good for now. Another hour of modifying
> lib/chips.c and it will be done. But if you're getting ready to
> change the standard names and/or magnitudes I better hold off...

Not sure how long it will take nor if it will be done now. I would like
it to be decided as soon as possible bug Greg's opinion differs, it
seems.

> Helpful hint: the more names stay the same as they were before the
> less typing there will be (inx, inx_min, inx_max....). Unfortunately
> pretty much everything is different now.

The simple fact that we switched to a one file per value philosophy
makes it fundamentaly different, isn't it?

> The reason it's called "in_input1" instead of "in1" escapes me.

Probably in order to match the above mentioned philosophy. In1 is an
entity, not a value. As an entity, it has several values: measurement
(here called input) and limits. I guess that having all files named
after a common scheme (${entity-name}_${value-name}${N} here) is of
great value when it comes to obtaining a chipset-independant library
and/or user-space tools interface.

That said, I would have named that file in1_input, not in_input1,
because it makes it visualy obvious that all in1_* correspond to the
same entity, while it isn't that obvious (to me at least) that all in_*1
values belong to a family.

> One problem I ran into:
> 
> For temp_xxx, sysfs_interface says divide by 1000 but actually it's
> 100(for w83781d anyway)

I knew it. See my answer to Danny's post earlier in this thread.
After a quick look, the it87 driver seems to be OK.

Greg, can you confirm that the retained unit it the thousandth (and not
hundredth) of degree Celcius?

Mark, are you using a w83781d or similar chip under 2.6.0-testX? If you
do, I would like you to confirm that the hysteresis and over
temperatures are swapped.

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

libsensors for linux 2.6

[Mark Studebaker]

Jean Delvare wrote:
> 
> > But I think this approach is good for now. Another hour of modifying
> > lib/chips.c and it will be done. But if you're getting ready to
> > change the standard names and/or magnitudes I better hold off...
> 
> Not sure how long it will take nor if it will be done now. I would like
> it to be decided as soon as possible bug Greg's opinion differs, it
> seems.
> 
> > Helpful hint: the more names stay the same as they were before the
> > less typing there will be (inx, inx_min, inx_max....). Unfortunately
> > pretty much everything is different now.
> 
> The simple fact that we switched to a one file per value philosophy
> makes it fundamentaly different, isn't it?
> 

But there's an entry in chips.c for each value, not just for each file.
And there is a name for each value too. This was the name presented to
the
userspace tools, not a /proc file name, so it's dangerous to
assume this is a sysfs name... but that's what I did for now, again
to minimize typing. Exceptions are added with the new sysname value.

So for sysfs we open up the file listed in the new struct entry
(sysname)
and use the first (only) value.
And we ignore the other stuff that binds entries together
to form multiple values in one file.

So maybe it's fundamentally different, maybe not. But since there's
already an entry per value,
it's pretty easy to add the sysfs file name. That would be it if only
the magnitudes
were the same as before. Since it isn't, we have to add a sysfs
magnitude.

> > The reason it's called "in_input1" instead of "in1" escapes me.
> 
> Probably in order to match the above mentioned philosophy. In1 is an
> entity, not a value. As an entity, it has several values: measurement
> (here called input) and limits. I guess that having all files named
> after a common scheme (${entity-name}_${value-name}${N} here) is of
> great value when it comes to obtaining a chipset-independant library
> and/or user-space tools interface.
> 
> That said, I would have named that file in1_input, not in_input1,
> because it makes it visualy obvious that all in1_* correspond to the
> same entity, while it isn't that obvious (to me at least) that all in_*1
> values belong to a family.
> 
> > One problem I ran into:
> >
> > For temp_xxx, sysfs_interface says divide by 1000 but actually it's
> > 100(for w83781d anyway)
> 
> I knew it. See my answer to Danny's post earlier in this thread.
> After a quick look, the it87 driver seems to be OK.
> 
> Greg, can you confirm that the retained unit it the thousandth (and not
> hundredth) of degree Celcius?
> 
> Mark, are you using a w83781d or similar chip under 2.6.0-testX? If you
> do, I would like you to confirm that the hysteresis and over
> temperatures are swapped.
> 

I'm running test8 and they don't look swapped to me.

# for i in temp*
> do
> echo $i
> cat $i
> done
temp_input1
3100
temp_input2
3750
temp_input3
0
temp_max1
12700
temp_max2
6000
temp_max3
6000
temp_min1
6000
temp_min2
5000
temp_min3
5000

libsensors for linux 2.6

[Jean Delvare]

> I'm running test8 and they don't look swapped to me.
> 
> # for i in temp*
> > do
> > echo $i
> > cat $i
> > done
> temp_input1
> 3100
> temp_input2
> 3750
> temp_input3
> 0
> temp_max1
> 12700
> temp_max2
> 6000
> temp_max3
> 6000
> temp_min1
> 6000
> temp_min2
> 5000
> temp_min3
> 5000

I misexpressed myself. What I should have said is: *writing* to over and hystsresis values is swapped. Try writing to temp_min1, I'm quite sure that temp_max1 value will change instead.

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

libsensors for linux 2.6

[Mark Studebaker]

looks fine to me...
temp_min1 is 3rd from last...

# cat temp*
3200
3650
0
12700
6000
6000
6000
5000
5000
# echo 7000 > temp_min1
# cat temp*
3200
3650
0
12700
6000
6000
7000
5000
5000


Jean Delvare wrote:
> 
> > I'm running test8 and they don't look swapped to me.
> >
> > # for i in temp*
> > > do
> > > echo $i
> > > cat $i
> > > done
> > temp_input1
> > 3100
> > temp_input2
> > 3750
> > temp_input3
> > 0
> > temp_max1
> > 12700
> > temp_max2
> > 6000
> > temp_max3
> > 6000
> > temp_min1
> > 6000
> > temp_min2
> > 5000
> > temp_min3
> > 5000
> 
> I misexpressed myself. What I should have said is: *writing* to over and hystsresis values is swapped. Try writing to temp_min1, I'm quite sure that temp_max1 value will change instead.
> 
> --
> Jean Delvare
> http://www.ensicaen.ismra.fr/~delvare/

libsensors for linux 2.6

[Jean Delvare]

Mark,

> looks fine to me...
> temp_min1 is 3rd from last...

OK, actually the variables are swapped everywhere (for temp1), so
although it is a mess from a developer's point of view, it doesn't hurt
the user.

Still I would bet that temp2 and temp3 suffer from the problem I
suspected. Writing to temp_max2 would probably change temp_min2's value
instead. Could you confirm?

I will send a modified patch to Greg, that should fix that. Would be
great if you could test it, just to make sure I did not miss a thing
(although I'll double check this time).

Thanks.

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

libsensors for linux 2.6

[Mark Studebaker]

Jean Delvare wrote:
> 
> Mark,
> 
> > looks fine to me...
> > temp_min1 is 3rd from last...
> 
> OK, actually the variables are swapped everywhere (for temp1), so
> although it is a mess from a developer's point of view, it doesn't hurt
> the user.
> 
> Still I would bet that temp2 and temp3 suffer from the problem I
> suspected. Writing to temp_max2 would probably change temp_min2's value
> instead. Could you confirm?
> 

correct, that's what it does, max and min are swapped for 2 and 3.