Fujitsu Siemens sensor HERMES

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

after contacting FSC about development documentation for their SM chip, they 
pointed me to Matthias Reineke (m.reineke@email.de), but I didn't get any 
reply from him on my request.

Using google I've found

	http://archives.andrew.net.au/lm-sensors/msg01988.html

which mentions him and some people from lm_sensors. Has there been any 
progress on the HERMES chip?

Can I help contributing (I own a FSC D1564 board)?

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> after contacting FSC about development documentation for their SM
> chip, they pointed me to Matthias Reineke (m.reineke@email.de), but I
> didn't get any reply from him on my request.
> 
> Using google I've found
> 
> 	http://archives.andrew.net.au/lm-sensors/msg01988.html
> 
> which mentions him and some people from lm_sensors. Has there been
> any progress on the HERMES chip?

Matthias was supposed to write a driver for it. I think I remember he
obtained some datasheet from FSC. The problem is, we did not heard about
him since February 2003.

> Can I help contributing (I own a FSC D1564 board)?

We need a datasheet. Without one, there's nothing we can do. If you
could get one from FSC, it would be great. Just in case, note that none
of us developers is willing to sign a NDA for this. Either they send us
a datasheet, or we don't write support for their chip. It's that simple.

Maybe we could contact Joachim.Braeuer at fujitsu-siemens.com and/or
Thomas.Bretthauer at fujitsu-siemens.com, they were our contacts at FSC
at that time, and maybe will help once again. BTW, I'd really like to
hear from Matthias again. Matthias, are you reading us?

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > I'm now waiting for your new patch, fixing my previous remarks, and
> > I'll will commit it to our repository.
> > 
> > Keep up the good work :)
> 
> I'm sorry for the delay, but I was very busy at the company due to 
> perparations for SYSTEMS fair.
> 
> Attached you'll find the updated patch.

Great. Looks really good. I've taken out the sensors-detect part, since
I had already added support for the Hermes some weeks ago (amazing how
we came to the very same function, almost to the byte). I've also fixed
a few typos in the docs, reformatted things a bit to conform to the
project standards (if there is such a thing). I obviously should have
told you that part of the docs is generated from the code (insmod
parameters, /proc files list).

I couldn't test your driver (don't have the hardware) but I read it
quickly and I believe it's ready for integration. I've committed your
work to our CVS repository, and updated our webpage to reflect that.
Good job!

Still there are two things that I would like to hear you about:

1* In the docs, you say that fans have a programmable divider of 1, 2 or
4. I can't see how one could change that divider. Am I missing
something?

2* In the driver itself, the hexadecimal mask 0x9b in fscher_in() would
probably need some comment. The choice of that value isn't obvious.
Others masks are simple enough to be guessed or at least trusted at
first sight, but this one isn't IMHO.

Thanks again.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > Great. Looks really good. I've taken out the sensors-detect part,
> > since I had already added support for the Hermes some weeks ago
> > (amazing how we came to the very same function, almost to the byte).
> > I've also fixed a few typos in the docs, reformatted things a bit to
> > conform to the project standards (if there is such a thing). I
> > obviously should have told you that part of the docs is generated
> > from the code (insmod parameters, /proc files list).
> 
> Please tell me.

Look in prog/doc, you'll find the two scripts there. I used them to
override what you had in your doc file before committing it. That way
we're sure there's no typo in there. This also provides basic code
checking, which can't hurt.

> > Still there are two things that I would like to hear you about:
> > 
> > 1* In the docs, you say that fans have a programmable divider of 1,
> > 2 or 4. I can't see how one could change that divider. Am I missing
> > something?
> 
> RPM value can be devided by 1, 2 or 4 if the ripple pre scaler is set
> to 2, 4 or 8 (see LIMITATIONS).

I'm not sure I get you. Is it because most fans return two ripples by
rotation? Anyway, that fan divisor things have always been over me, I
guess it won't change today.

> > 2* In the driver itself, the hexadecimal mask 0x9b in fscher_in()
> > would probably need some comment. The choice of that value isn't
> > obvious. Others masks are simple enough to be guessed or at least
> > trusted at first sight, but this one isn't IMHO.
> 
> The mask cares for the reserved bits 2, 5 and 6.

OK, I'll be adding a comment in your code then.

Thanks.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Attached you'll find the updated patch.
> 
> Great. Looks really good. I've taken out the sensors-detect part, since
> I had already added support for the Hermes some weeks ago (amazing how
> we came to the very same function, almost to the byte). I've also fixed
> a few typos in the docs, reformatted things a bit to conform to the
> project standards (if there is such a thing). I obviously should have
> told you that part of the docs is generated from the code (insmod
> parameters, /proc files list).

Please tell me.

> I couldn't test your driver (don't have the hardware) but I read it
> quickly and I believe it's ready for integration. I've committed your
> work to our CVS repository, and updated our webpage to reflect that.
> Good job!

Thanx.

> Still there are two things that I would like to hear you about:
> 
> 1* In the docs, you say that fans have a programmable divider of 1, 2 or
> 4. I can't see how one could change that divider. Am I missing
> something?

RPM value can be devided by 1, 2 or 4 if the ripple pre scaler is set to 2, 4 
or 8 (see LIMITATIONS).

> 2* In the driver itself, the hexadecimal mask 0x9b in fscher_in() would
> probably need some comment. The choice of that value isn't obvious.
> Others masks are simple enough to be guessed or at least trusted at
> first sight, but this one isn't IMHO.

The mask cares for the reserved bits 2, 5 and 6.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Mark Studebaker]

Khali you forgot to check in fscher.c.

Reinhard Nissl wrote:
> 
> Hi,
> 
> Jean Delvare wrote:
> 
> >>Attached you'll find the updated patch.
> >
> > Great. Looks really good. I've taken out the sensors-detect part, since
> > I had already added support for the Hermes some weeks ago (amazing how
> > we came to the very same function, almost to the byte). I've also fixed
> > a few typos in the docs, reformatted things a bit to conform to the
> > project standards (if there is such a thing). I obviously should have
> > told you that part of the docs is generated from the code (insmod
> > parameters, /proc files list).
> 
> Please tell me.
> 
> > I couldn't test your driver (don't have the hardware) but I read it
> > quickly and I believe it's ready for integration. I've committed your
> > work to our CVS repository, and updated our webpage to reflect that.
> > Good job!
> 
> Thanx.
> 
> > Still there are two things that I would like to hear you about:
> >
> > 1* In the docs, you say that fans have a programmable divider of 1, 2 or
> > 4. I can't see how one could change that divider. Am I missing
> > something?
> 
> RPM value can be devided by 1, 2 or 4 if the ripple pre scaler is set to 2, 4
> or 8 (see LIMITATIONS).
> 
> > 2* In the driver itself, the hexadecimal mask 0x9b in fscher_in() would
> > probably need some comment. The choice of that value isn't obvious.
> > Others masks are simple enough to be guessed or at least trusted at
> > first sight, but this one isn't IMHO.
> 
> The mask cares for the reserved bits 2, 5 and 6.
> 
> Bye.
> --
> Dipl.-Inform. (FH) Reinhard Nissl
> mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Khali you forgot to check in fscher.c.

Hu-oh, really? :*)

Sorry, that was a new file and I forgot the cvs add before committing
everything. This is done now.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> 
> > Khali you forgot to check in fscher.c.
> 
> Hu-oh, really? :*)
> 
> Sorry, that was a new file and I forgot the cvs add before committing
> everything. This is done now.

And I had also forgotten the documentation file, shame on me. This is
done too now.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

I didn't find fscher on the supported devices web page. I'm you've been just 
to busy to add it ;-)

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> I didn't find fscher on the supported devices web page. I'm you've
> been just to busy to add it ;-)

That's because it is considered a new driver, so you'll find it under
"New Drivers". There's always some delay between the moment support is
added for a chipset and the moment we declare it "officially supported".
(Which might be discussed, but that's the way it is for now.)

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>I didn't find fscher on the supported devices web page. I'm you've
>>been just to busy to add it ;-)
> 
> That's because it is considered a new driver, so you'll find it under
> "New Drivers". There's always some delay between the moment support is
> added for a chipset and the moment we declare it "officially supported".
> (Which might be discussed, but that's the way it is for now.)

That's ok. A few seconds after sending the email, I found it myself in that 
section :-)

Now as kernel 2.6.0 is released, I feel the need to port fscher to 2.6.0. I've 
already started today, but didn't have success so far.

The biggest issue in porting is the change from procfs to sysfs. Looking at 
other drivers, I realized, that they define a lot of macros to deduce all the 
necessary callback functions.

Is there a coding convention for these macros?

Attached you'll find what I've done so far. Please send me your comments and 
hints.

Thanks in advance.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fscher.c.gz
Type: application/x-gzip
Size: 6630 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040106/89cb6ad5/fscher.c.gz

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Now as kernel 2.6.0 is released, I feel the need to port fscher to
> 2.6.0. I've already started today, but didn't have success so far.

That would be great to have a ported driver for sure :)

> The biggest issue in porting is the change from procfs to sysfs.
> Looking at other drivers, I realized, that they define a lot of macros
> to deduce all the necessary callback functions.

You don't have to define them, but you'll probably find that handy after
you realize you've been defining sets of functions that only differ in
the register they are accessing.

> Is there a coding convention for these macros?

Not that I know. But cut'n'paste rulez ;)

> Attached you'll find what I've done so far. Please send me your
> comments and hints.

I'd suggest you first read Documentation/i2c/porting-clients as found in
Linux 2.6.1-rc1 and later. That's a document I wrote about porting chip
drivers from linux 2.4 to linux 2.6. I believe that almost all questions
you'd ask are answered there. If you don't mind, I'll take a look at
your code after you read that document and applied its contents to
your driver.

If you have questions that are not answered there, ask. If you want
things to be added to the document, tell me.

Please note that limits initialization has been removed from all
drivers since it sometimes caused much trouble, and the same can be
achived with "sensors -s". That's not a 2.6 porting issue, but that's
something new since you wrote your original driver.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Now as kernel 2.6.0 is released, I feel the need to port fscher to
>>2.6.0. I've already started today, but didn't have success so far.
> 
> That would be great to have a ported driver for sure :)

[snip]

>>Attached you'll find what I've done so far. Please send me your
>>comments and hints.
> 
> I'd suggest you first read Documentation/i2c/porting-clients as found in
> Linux 2.6.1-rc1 and later. That's a document I wrote about porting chip
> drivers from linux 2.4 to linux 2.6. I believe that almost all questions
> you'd ask are answered there. If you don't mind, I'll take a look at
> your code after you read that document and applied its contents to
> your driver.

I had a look at it.

> If you have questions that are not answered there, ask. If you want
> things to be added to the document, tell me.

How would you name the file, where the chip revision will be made available 
(rev, revision, ...)? Should it be made availble?

HERMES has a 'Global event status register'. It's something like alarms. E. g. 
bit 0 tells you, that there is a 'Global fan fault event'. Which fan is faulty 
must be determined by looking at the fan status registers, where bit 2 tells 
one, that the fan is faulty.

Is it ok to create fan_status1...3 files, containing the fan's status register 
value?

Concerning alarms: what do you mean by comparator mode?

HERMES e. g. signals 'Global fan fault event' as long as any of the fan status 
registers signals 'Fan fault'. Clearing all fan status registers (by writing 
this bit2 to 1) will finally have HERMES clear the 'Global fan fault event' 
(Globel event status register bit 0). Is this the expected behaviour?

One of the event bits is called 'Global software event'. This bit can be set 
and cleared by writing bit 0 of the global control register to 1 respectively 
0. Is it ok to supply a file named 'control' for this purpose?

Concerning beep*: I assume, that the hardware must support some way of making 
noise, when an alarm occurs. This is not the case for HERMES.

Concerning in_input*: HERMES only tells me unscaled values for +5.0V, +12.0V 
and +3.0V for the backup battery. In the current release, I used dmidecode to 
get the scaling factors and offsets from my mainboard and used them to finally 
hardcode scaling of the supplied values.

What shall I do for kernel 2.6.x? Which numbers shall I append to in_input? 
Where shell I put the battery voltage? Is there a way to read the dmi values 
from withing the fscher module?

Concerning temp*: HERMES supplies a 'Sensor status register' for each 
temperature sensor, where it signals over temperature respectively sensor 
hadrware faults (similar to fan_status). Is it ok to supply temp_status files?

Finally the watchdog:
I'd supply a 'watchdog_control' file, which is used e. g. to retrigger the 
watchdog. Then I would supply a 'watchdog_status' file, which tells you that a 
  system reset due to a watchdog event has occured. And last but not least a 
file 'watchdog_preset' where to set the watchdog time.

Is this all ok?

> Please note that limits initialization has been removed from all
> drivers since it sometimes caused much trouble, and the same can be
> achived with "sensors -s". That's not a 2.6 porting issue, but that's
> something new since you wrote your original driver.

Can you tell me more, if it is still of interest?

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Mark M. Hoffman]

* Jean Delvare <khali@linux-fr.org> [2004-01-11 21:14:40 +0100]:
> > Well, as stated above, you have to write 4 to the fan status register,
> > which will in turn clear bit 2 of the fan status register. If no more
> > fan status registers indicate a fan failure, the 'Global fan fault
> > event' will clear too (simple OR-logic, as you mentioned above).
> 
> Writing a 1 to the bit so that it is reset is a strange idea, isn't it?
> ;)

Just in case you were not entirely joking...

This is a common idiom for h/w status registers:  write '1' to reset
and write '0' is ignored.  This allows you to clear a single status
bit without needing a read-modify-write, i.e. you'll not accidentally
clear a status bit that was set just before your write.

For sticky-status bitfields in sysfs, this idiom is still appropriate.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Well, as stated above, you have to write 4 to the fan status register,
>>which will in turn clear bit 2 of the fan status register. If no more
>>fan status registers indicate a fan failure, the 'Global fan fault
>>event' will clear too (simple OR-logic, as you mentioned above).
> 
> Writing a 1 to the bit so that it is reset is a strange idea, isn't it?
> ;)

Well, how would you indicate the bit to clear otherwise?

[ snip: global software event ]

>>Which suffixes shall I choose for the unscaled 5 V and 12 V voltages,
>>and especially, which one for the 3 V backup battery?
> 
> I don't see any difficulty here. They are in0, in1 and in2. We never
> give names to the channels, only number. Labels are set with
> sensors.conf.

Ok, so everything is like before, besides the values beeing unscaled.

[ snip: voltage scaling ]

>>>Go for it. Strange how they split their alarms in so many different
>>>registers. Usually we combine all of them into a single, 32-bit max
>>>value (the "alarms" file) but it looks like the Hermes has just too
>>>many of them do to so. Unless only a few bits are used in each
>>>registers? If
>>
>>True. Each fan status has just one bit while each temperature sensor
>>has 2.
>>
>>>the total number of alarm bits is less than 32, you should consider
>>>combining them all into a single value.
>>
>>Well, all status flags would be no more than 32. Shall I therefore
>>drop fan_status*, temp_status* and watchdog_status and put them all
>>together into alarms?
> 
> That's what I was suggesting, but...
> 
>>Clearing a certain fan status bit would then require to calculate a
>>proper value. This value must then be written into alarms to get the
>>bit cleared. Writing -1 to alarms would therefore clear all clearable
>>status bits. The others would then get updated by HERMES' internal
>>logic.
> 
> ...the fact that you could write to this file would be something
> unusual. And it would for sure make your code more complex, if not
> unreadable (unless you document it a lot). That might not be worth it,
> especially since the benefit would be thin. It is very interesting to
> respect file namings when the name is enough for a third party software
> to use the value. This isn't the case with alarms anyway, since the
> meaning of each bit is driver-dependant. So in the end, if having
> different files is more convenient, go for it.

I think so, too.

> I'd like to hear Greg (and possibly others) about this though.

BTW: I'm not on the ML.

>>The drawback is, that one has to fiddle around with large powers of
>>two. This makes it more complicated for a human beeing to see at a
>>glance, where the problem is.
> 
> This is the way it is done in all other drivers. Users are not supposed
> to read raw values from /sys or /proc. They run sensors or any other
> piece software which decode the alarms file for them. So that's not a
> valid reason IMHO.

Ok, but then I should complete the output of sensors for fscher and also 
include alarms and watchdog.

>>So, with a single 'sensors -s' instruction one could for example set
>>all the pwm* values for the fans?
>>
>>Currently, I use
>>
>>	echo 4 2 > /proc/sys/dev/sensors/fscher*/fan1
>>	echo 4 1 > /proc/sys/dev/sensors/fscher*/fan2
>>	echo 4 1 > /proc/sys/dev/sensors/fscher*/fan3
> 
> You like reading and writing to /proc files directly, don't you? ;)

Maybe, I was just to lazy to look for a different solution. From coding the 
module, I knew that it would work that easy ;-).

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Well, as stated above, you have to write 4 to the fan status register,
> which will in turn clear bit 2 of the fan status register. If no more
> fan status registers indicate a fan failure, the 'Global fan fault
> event' will clear too (simple OR-logic, as you mentioned above).

Writing a 1 to the bit so that it is reset is a strange idea, isn't it?
;)

> When an alarm condition is active, the mainboard drives a LED at a
> frequency of 5 Hz (or 2 Hz, depending on the kind of event, if the
> documentation is correct).
> 
> By setting bit 0 in the 'Global control register', bit 4 ('Global
> software event') of the 'Global event status register' turns on, and
> therefore, the alert LED starts flashing.
> 
> I use the computer with this mainboard as video disk recorder. One
> practical use of the 'Global software event' might be to indicate the
> 'almost out of disk space' condition, as the machine then needs
> additional care soon.

Hehe, that's nice :) OK I now see the idea of it all.

> Which suffixes shall I choose for the unscaled 5 V and 12 V voltages,
> and especially, which one for the 3 V backup battery?

I don't see any difficulty here. They are in0, in1 and in2. We never
give names to the channels, only number. Labels are set with
sensors.conf.

> You've got it. Currently, they are hardcoded. The next time, they will
> go to /etc/sensors.conf.

That's it.

> > And no, I don't think you can get dmi values from a kernel module.
> 
> That would have been the user friendliest way ;-)

Yeah, until the dmi table is wrong and the user can't fix the values, so
the sensors.conf solution would still be needed. What's possible OTOH is
to add a hint about dmidecode in the fscher-* section of sensors.conf so
that users can check if the default value is correct for them.

> > Go for it. Strange how they split their alarms in so many different
> > registers. Usually we combine all of them into a single, 32-bit max
> > value (the "alarms" file) but it looks like the Hermes has just too
> > many of them do to so. Unless only a few bits are used in each
> > registers? If
> 
> True. Each fan status has just one bit while each temperature sensor
> has 2.
> 
> > the total number of alarm bits is less than 32, you should consider
> > combining them all into a single value.
> 
> Well, all status flags would be no more than 32. Shall I therefore
> drop fan_status*, temp_status* and watchdog_status and put them all
> together into alarms?

That's what I was suggesting, but...

> Clearing a certain fan status bit would then require to calculate a
> proper value. This value must then be written into alarms to get the
> bit cleared. Writing -1 to alarms would therefore clear all clearable
> status bits. The others would then get updated by HERMES' internal
> logic.

..the fact that you could write to this file would be something
unusual. And it would for sure make your code more complex, if not
unreadable (unless you document it a lot). That might not be worth it,
especially since the benefit would be thin. It is very interesting to
respect file namings when the name is enough for a third party software
to use the value. This isn't the case with alarms anyway, since the
meaning of each bit is driver-dependant. So in the end, if having
different files is more convenient, go for it.

I'd like to hear Greg (and possibly others) about this though.

> The drawback is, that one has to fiddle around with large powers of
> two. This makes it more complicated for a human beeing to see at a
> glance, where the problem is.

This is the way it is done in all other drivers. Users are not supposed
to read raw values from /sys or /proc. They run sensors or any other
piece software which decode the alarms file for them. So that's not a
valid reason IMHO.

> So, with a single 'sensors -s' instruction one could for example set
> all the pwm* values for the fans?
> 
> Currently, I use
> 
> 	echo 4 2 > /proc/sys/dev/sensors/fscher*/fan1
> 	echo 4 1 > /proc/sys/dev/sensors/fscher*/fan2
> 	echo 4 1 > /proc/sys/dev/sensors/fscher*/fan3

You like reading and writing to /proc files directly, don't you? ;)

> This would then be obsolete.

Yes, this would be.

> > For you, all what this means is that if your old fscher driver used
> > to set limits as it is loaded, you are invited not to port that part
> > of the code into your 2.6 driver.
> 
> No, there was no such code in the old fscher driver.

Then you just can ignore that part of the problem :)

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > If you have questions that are not answered there, ask. If you want
> > things to be added to the document, tell me.
> 
> How would you name the file, where the chip revision will be made
> available (rev, revision, ...)? Should it be made availble?

I don't think that any other drivers do, but this is no reason for yours
not to. "revision" sounds fine.

> HERMES has a 'Global event status register'. It's something like
> alarms. E. g. bit 0 tells you, that there is a 'Global fan fault
> event'. Which fan is faulty must be determined by looking at the fan
> status registers, where bit 2 tells one, that the fan is faulty.
> 
> Is it ok to create fan_status1...3 files, containing the fan's status
> register value?

This makes sense.

> Concerning alarms: what do you mean by comparator mode?

Some chipsets can be configured in different modes. Interrupt mode means
that alarms don't wear off automatically as the cause of the alarm
disappears. Comparator means that it does. I'm not too comfortable with
changing modes, since hardware may be wired so as to work in one mode
and not in the other. So I would leave things as they are.

> HERMES e. g. signals 'Global fan fault event' as long as any of the
> fan status registers signals 'Fan fault'. Clearing all fan status
> registers (by writing this bit2 to 1) will finally have HERMES clear
> the 'Global fan fault event' (Globel event status register bit 0). Is
> this the expected behaviour?

I'm surprise that you are allowed to write to a status registers. Would
you rather mean "as the bit2 clears itself due to the fan working
correctly again"?

Anyway, the "Global fan fault event" is only a summary (technically a
logical OR) of the other registers' bit2, so it doesn't really matter.
What does (as far as interrupt vs comparator mode is concerned) is how
fan registers bit2 is cleared.

> One of the event bits is called 'Global software event'. This bit can
> be set and cleared by writing bit 0 of the global control register to
> 1 respectively 0. Is it ok to supply a file named 'control' for this
> purpose?

I don't see when you will want to set it. Can you provide a concrete
example?

> Concerning beep*: I assume, that the hardware must support some way of
> making noise, when an alarm occurs. This is not the case for HERMES.

Then you just don't have beep* files. That said, chipsets that do don't
make noise themselves. Instead, they have some way to have the PC
speaker make noise for them (don't know all the details).

> Concerning in_input*: HERMES only tells me unscaled values for +5.0V,
> +12.0V and +3.0V for the backup battery. In the current release, I
> used dmidecode to get the scaling factors and offsets from my
> mainboard and used them to finally hardcode scaling of the supplied
> values.
> 
> What shall I do for kernel 2.6.x? Which numbers shall I append to
> in_input? Where shell I put the battery voltage? Is there a way to
> read the dmi values from withing the fscher module?

I don't really see the problem here, but I admit I haven't read the
Hermes data sheet. If scaling factors are motherboard-dependant, they
don't belong to your driver but to /etc/sensors.conf instead. That is,
your driver will return the raw value, and then the conversions formulae
are taken from the configuration files. People with a different
motherboard only have to edit the configuration file, as opposed to edit
the kernel driver code and recompile. Your 2.4 driver should do the same
BTW, unless I misunderstood something.

And no, I don't think you can get dmi values from a kernel module.

> Concerning temp*: HERMES supplies a 'Sensor status register' for each 
> temperature sensor, where it signals over temperature respectively
> sensor hadrware faults (similar to fan_status). Is it ok to supply
> temp_status files?

Go for it. Strange how they split their alarms in so many different
registers. Usually we combine all of them into a single, 32-bit max
value (the "alarms" file) but it looks like the Hermes has just too many
of them do to so. Unless only a few bits are used in each registers? If
the total number of alarm bits is less than 32, you should consider
combining them all into a single value.

> Finally the watchdog:
> I'd supply a 'watchdog_control' file, which is used e. g. to retrigger
> the watchdog. Then I would supply a 'watchdog_status' file, which
> tells you that a system reset due to a watchdog event has occured. And
> last but not least a file 'watchdog_preset' where to set the watchdog
> time.
> 
> Is this all ok?

No objection.

> > Please note that limits initialization has been removed from all
> > drivers since it sometimes caused much trouble, and the same can be
> > achived with "sensors -s". That's not a 2.6 porting issue, but
> > that's something new since you wrote your original driver.
> 
> Can you tell me more, if it is still of interest?

Basically we considered that limits initializations belong to
user-space, and is done with "sensors -s" which reads them from
/etc/sensors.conf, so leaving them into the driver themselves
additionally wasn't needed. This makes drivers smaller.

We are also considering removing some initializations (not limits this
time) that are known to cause problems for certain chips, but that's
something different.

For you, all what this means is that if your old fscher driver used to
set limits as it is loaded, you are invited not to port that part of the
code into your 2.6 driver.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>>If you have questions that are not answered there, ask. If you want
>>>things to be added to the document, tell me.

[ snip: revision file ]

>>HERMES has a 'Global event status register'. It's something like
>>alarms. E. g. bit 0 tells you, that there is a 'Global fan fault
>>event'. Which fan is faulty must be determined by looking at the fan
>>status registers, where bit 2 tells one, that the fan is faulty.
>>
>>Is it ok to create fan_status1...3 files, containing the fan's status
>>register value?
> 
> This makes sense.
> 
>>Concerning alarms: what do you mean by comparator mode?
> 
> Some chipsets can be configured in different modes. Interrupt mode means
> that alarms don't wear off automatically as the cause of the alarm
> disappears. Comparator means that it does. I'm not too comfortable with
> changing modes, since hardware may be wired so as to work in one mode
> and not in the other. So I would leave things as they are.

HERMES doesn't allow changing this behaviour (at least it is not documented). 
So, from the above, I'd say that HERMES is using interrupt mode.

>>HERMES e. g. signals 'Global fan fault event' as long as any of the
>>fan status registers signals 'Fan fault'. Clearing all fan status
>>registers (by writing this bit2 to 1) will finally have HERMES clear
>>the 'Global fan fault event' (Globel event status register bit 0). Is
>>this the expected behaviour?
> 
> I'm surprise that you are allowed to write to a status registers. Would
> you rather mean "as the bit2 clears itself due to the fan working
> correctly again"?

No, it's not cleared automatically. The BIOS also uses this bit to show the 
user an appropriate 'FAN failed' message at boot time.

> Anyway, the "Global fan fault event" is only a summary (technically a
> logical OR) of the other registers' bit2, so it doesn't really matter.
> What does (as far as interrupt vs comparator mode is concerned) is how
> fan registers bit2 is cleared.

Well, as stated above, you have to write 4 to the fan status register, which 
will in turn clear bit 2 of the fan status register. If no more fan status 
registers indicate a fan failure, the 'Global fan fault event' will clear too 
(simple OR-logic, as you mentioned above).

>>One of the event bits is called 'Global software event'. This bit can
>>be set and cleared by writing bit 0 of the global control register to
>>1 respectively 0. Is it ok to supply a file named 'control' for this
>>purpose?
 >
> I don't see when you will want to set it. Can you provide a concrete
> example?

When an alarm condition is active, the mainboard drives a LED at a frequency 
of 5 Hz (or 2 Hz, depending on the kind of event, if the documentation is 
correct).

By setting bit 0 in the 'Global control register', bit 4 ('Global software 
event') of the 'Global event status register' turns on, and therefore, the 
alert LED starts flashing.

I use the computer with this mainboard as video disk recorder. One practical 
use of the 'Global software event' might be to indicate the 'almost out of 
disk space' condition, as the machine then needs additional care soon.

[ snip: beep files ]

>>Concerning in_input*: HERMES only tells me unscaled values for +5.0V,
>>+12.0V and +3.0V for the backup battery. In the current release, I
>>used dmidecode to get the scaling factors and offsets from my
>>mainboard and used them to finally hardcode scaling of the supplied
>>values.
>>
>>What shall I do for kernel 2.6.x? Which numbers shall I append to

Which suffixes shall I choose for the unscaled 5 V and 12 V voltages, and 
especially, which one for the 3 V backup battery?

>>in_input? Where shell I put the battery voltage? Is there a way to
>>read the dmi values from withing the fscher module?
> 
> I don't really see the problem here, but I admit I haven't read the
> Hermes data sheet. If scaling factors are motherboard-dependant, they

 From the documentation I would say, that they are. They could easily change 
with the next board revision.

> don't belong to your driver but to /etc/sensors.conf instead. That is,
> your driver will return the raw value, and then the conversions formulae
> are taken from the configuration files. People with a different
> motherboard only have to edit the configuration file, as opposed to edit
> the kernel driver code and recompile. Your 2.4 driver should do the same
> BTW, unless I misunderstood something.

You've got it. Currently, they are hardcoded. The next time, they will go to 
/etc/sensors.conf.

> And no, I don't think you can get dmi values from a kernel module.

That would have been the user friendliest way ;-)

>>Concerning temp*: HERMES supplies a 'Sensor status register' for each 
>>temperature sensor, where it signals over temperature respectively
>>sensor hadrware faults (similar to fan_status). Is it ok to supply
>>temp_status files?
> 
> Go for it. Strange how they split their alarms in so many different
> registers. Usually we combine all of them into a single, 32-bit max
> value (the "alarms" file) but it looks like the Hermes has just too many
> of them do to so. Unless only a few bits are used in each registers? If

True. Each fan status has just one bit while each temperature sensor has 2.

> the total number of alarm bits is less than 32, you should consider
> combining them all into a single value.

Well, all status flags would be no more than 32. Shall I therefore drop 
fan_status*, temp_status* and watchdog_status and put them all together into 
alarms?

Clearing a certain fan status bit would then require to calculate a proper 
value. This value must then be written into alarms to get the bit cleared. 
Writing -1 to alarms would therefore clear all clearable status bits. The 
others would then get updated by HERMES' internal logic.

The drawback is, that one has to fiddle around with large powers of two. This 
  makes it more complicated for a human beeing to see at a glance, where the 
problem is.

What do you think?

[ snip: watchdog ]

>>>Please note that limits initialization has been removed from all
>>>drivers since it sometimes caused much trouble, and the same can be
>>>achived with "sensors -s". That's not a 2.6 porting issue, but
>>>that's something new since you wrote your original driver.
>>
>>Can you tell me more, if it is still of interest?
> 
> Basically we considered that limits initializations belong to
> user-space, and is done with "sensors -s" which reads them from
> /etc/sensors.conf, so leaving them into the driver themselves
> additionally wasn't needed. This makes drivers smaller.

So, with a single 'sensors -s' instruction one could for example set all the 
pwm* values for the fans?

Currently, I use

	echo 4 2 > /proc/sys/dev/sensors/fscher*/fan1
	echo 4 1 > /proc/sys/dev/sensors/fscher*/fan2
	echo 4 1 > /proc/sys/dev/sensors/fscher*/fan3

This would then be obsolete.

> We are also considering removing some initializations (not limits this
> time) that are known to cause problems for certain chips, but that's
> something different.
> 
> For you, all what this means is that if your old fscher driver used to
> set limits as it is loaded, you are invited not to port that part of the
> code into your 2.6 driver.

No, there was no such code in the old fscher driver.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > Writing a 1 to the bit so that it is reset is a strange idea, isn't
> > it?;)
> 
> Well, how would you indicate the bit to clear otherwise?

Sure, I agree. Just found it strange at first, but that's the correct
way.

> > I don't see any difficulty here. They are in0, in1 and in2. We never
> > give names to the channels, only number. Labels are set with
> > sensors.conf.
> 
> Ok, so everything is like before, besides the values beeing unscaled.

Correct. Please consider updating the 2.4 driver in a similar way once
you're over with the 2.6 driver, so that the same sensors.conf works
with both versions.

> BTW: I'm not on the ML.

Want to be?

> Ok, but then I should complete the output of sensors for fscher and
> also include alarms and watchdog.

You wanted to do so anyway, DIDN'T YOU? ;)

> > You like reading and writing to /proc files directly, don't you? ;)
> 
> Maybe, I was just to lazy to look for a different solution. From
> coding the module, I knew that it would work that easy ;-).

Adding a few lines to sensors.conf isn't that difficult, really. I'd
even find it easier than setting up a startup script that echoes values
to /proc files. But that's a matter of taste I guess.

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

Fujitsu Siemens sensor HERMES

[Mark Studebaker]

Reinhard Nissl wrote:

> Jean Delvare wrote:
> 

>>
>>> Concerning alarms: what do you mean by comparator mode?
>>
>>
>> Some chipsets can be configured in different modes. Interrupt mode means
>> that alarms don't wear off automatically as the cause of the alarm
>> disappears. Comparator means that it does. I'm not too comfortable with
>> changing modes, since hardware may be wired so as to work in one mode
>> and not in the other. So I would leave things as they are.
> 
> 
> HERMES doesn't allow changing this behaviour (at least it is not 
> documented). So, from the above, I'd say that HERMES is using interrupt 
> mode.
> 


I think Jean has the descriptions backwards, at least as Winbond has documented it,
and we copied that terminology into the documents doc/developers/proc in our package,
and i2c/sysfs-interface in the 2.6 kernel.
The lm_sensors standard is that alarms are persistent ("don't wear off automatically")
which Winbond calls "comparator".

Drivers should configure the hardware for that mode (whatever it's called).

mds

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>>I don't see any difficulty here. They are in0, in1 and in2. We never
>>>give names to the channels, only number. Labels are set with
>>>sensors.conf.
>>
>>Ok, so everything is like before, besides the values beeing unscaled.
> 
> Correct. Please consider updating the 2.4 driver in a similar way once
> you're over with the 2.6 driver, so that the same sensors.conf works
> with both versions.

Ok.

>>BTW: I'm not on the ML.
> 
> Want to be?

Not yet. Maybe soon, if I get further documentation from Fujitsu-Siemens.

>>Ok, but then I should complete the output of sensors for fscher and
>>also include alarms and watchdog.
> 
> You wanted to do so anyway, DIDN'T YOU? ;)

As I'm now more familar with sensors, shure!

>>>You like reading and writing to /proc files directly, don't you? ;)
>>
>>Maybe, I was just to lazy to look for a different solution. From
>>coding the module, I knew that it would work that easy ;-).
> 
> Adding a few lines to sensors.conf isn't that difficult, really. I'd
> even find it easier than setting up a startup script that echoes values
> to /proc files. But that's a matter of taste I guess.

It's time to change to sensors, because it solves dual-boot issues (2.4.x: 
procfs, 2.6.x: sysfs).

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> The problem is, that the voltage values still need to get devided by
> 10.
> 
> Shall I scale the values by 1000 for kernel 2.6.1 and by 100 for
> kernel 2.4.22 and remove the factor 1000 from sensors.conf?

Yes, that's the idea.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Well, looking at the conversion formula again, this idea is not 100 %
>>perfect, because it doesn't care to scale the offset value. Currently,
>>the offset is 0 but there is no guarantee, that it will always be 0.
> 
> This isn't a problem. The formulae are applied *after* libsensors (or
> any other program such as gkrellm) reads the value from sysfs or procfs.
> For example, if you have a raw reading of 1.8V, in 2.4 it'll be stored
> internally as 180, and read through procfs as 1.80. In 2.6 it'll be
> stored internally as 1800 and exported through sysfs as 1800 too. But
> libsensors knows about magnitudes in both cases and will read the value
> as 1.80. And any formula in sensors.conf will be applied to *this*
> value.

This is the kind of information that I was missing, when I wrote the last email.

> So, as you can see, it is safe to have a single formula for both
> versions.

You are right. I've attached the fixed drivers and adjusted sample 
configuration. Documentation will follow later and explains, how to get the 
necessary (M)ultiplier, (R)efVoltage and (O)ffset values from dmidecode.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fscher.c-2.4.22.gz
Type: application/x-gzip
Size: 5767 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040127/28ff505d/fscher.c-2.4.22.gz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fscher.c-2.6.1.gz
Type: application/x-gzip
Size: 4994 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040127/28ff505d/fscher.c-2.6.1.gz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sensors.conf.eg.diff.gz
Type: application/x-gzip
Size: 517 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040127/28ff505d/sensors.conf.eg.diff.gz

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> 1* Fix libsensors so that it handles fscher correctly.

Oh, BTW: Could you please send me the list of files as they appear in
sysfs? It'll be less error prone than reading the macros in your code (I
have nothing against using them, but this makes thing a bit hard to
understand at first sight).

Thanks.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

Hi Reinhard,

> Attached you'll find the 2.6.x module and configuration.

Great :)

> Is there a guide for writing chip documentation, as I don't know what
> to do with "Chip Features" in doc/chips/fscher?

No chip-specific documentation has been commited to Linux so far,
although I planned to do so too. Basically I think that "Chip Features"
should become "Sysfs Interface" and be a simple list of all files with
permissions (R-, -W or RW) and comments if they are needed.

> Is there a guide, how to port the sensors program?
> 
> Without changes, sensors just gives me the following:
> 
> fscher-i2c-0-73
> Adapter: SMBus I801 adapter at 2000
> +12V:           +11.86 V
> +5V:             +5.05 V
> Battery:         +3.05 V

This is libsensors that needs to be ported, not sensors. I was a bit
surprised that sensors doesn't seem to complain about missing values,
but contrary to most other chips, FSC chips support in sensors silently
ignores errors. Maybe this should be fixed for the sake of consistency.

Here are the steps that need to be taken now:
1* Fix libsensors so that it handles fscher correctly.
2* Update the 2.4 version of the driver so that it handles voltages the
   same way the 2.6 driver does.
3* Apply your patch to sensors.conf.eg.
4* Review your 2.6 driver and send it to Greg KH for inclusion into
   Linux 2.6.3.
5* Fix sensors so that it shows errors for FSC chips.

You must understand that step 3 doesn't make sense unless step 2 is also
achieved. The 2.4 and 2.6 drivers have to be as compatible as possible
so that users can switch from one to the other and back.

I will work on 1* (and possibly 5*) right now. Please work on 2*
meanwhile. Once done I'll do 3*, and you'll test that both the old and
the new driver work OK.

Then, and only then, I'll review your 2.6 driver (with changes if you
have done any) and submit it to Greg.

Thanks.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Please have a look at set_pwm(). What shall I do with an unsupported
> value? Is it ok to return -1?

Which values do you consider unsupported? (Pardon my ignorance, I've
never dealt with chipsets with PWM so far).

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> I think I would handle it that way:
> 1* v = (v >> 1) & 0x07.
> 2* v & 0x04 ? -> 3
>    v & 0x02 ? -> 2
>    else -> 1
> 
> Basically this means the following conversion table, if I'm not
> mistaking:
> 0..2 -> 1 (div=2)
> 3..4 -> 2 (div=4)
> 5+   -> 3 (div=8)
> 
> Should be fast and efficient enough, and keeps the code clear (IMHO
> at least).

In fact it isn't, since large values (>8) will lead to random divs.
Maybe the following is better:

if (v >= 8) v = 3;
else if (v >= 4) v = 2;
else v = 1;

Fast enough and much more readable IMHO. Sorry for my initial bogus
idea.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Please have a look at set_pwm(). What shall I do with an unsupported
>>value? Is it ok to return -1?
> 
> Which values do you consider unsupported? (Pardon my ignorance, I've
> never dealt with chipsets with PWM so far).

I was wrong. Correct is set_fan_div().

static ssize_t set_fan_div (struct i2c_client *client, struct fscher_data 
*data, const char *buf, size_t count, int nr, int reg)
{
   /* supported values: 2, 4, 8 */
   int v = simple_strtoul(buf, NULL, 10);

   switch (v)
   {
   case 2:    v = 1;    break;
   case 4:    v = 2;    break;
   case 8:    v = 3;    break;
   default:   return -1;
   }

   /* bits 2..7 reserved => mask with 0x03 */
   data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] &= ~0x03;
   data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] |= v;

   fscher_write_value(client, reg, data->fan_ripple[FAN_INDEX_FROM_NUM(nr)]);
   return count;
}

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> >>1* Fix libsensors so that it handles fscher correctly.

OK, I've hopefully added the necessary stuff into libsensors. Please
checkout from our CVS repository, make user && make user_install, and
tell me how it is going.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> BTW: changing *nrels_mag below from 2 to 3 fixed it for procfs, but
> still not for sensors.
> 
> void fscher_volt(struct i2c_client *client, int operation, int
> ctl_name,
>                   int *nrels_mag, long *results)
> {
>    struct fscher_data *data = client->data;
>    if (operation = SENSORS_PROC_REAL_INFO)
>      *nrels_mag = 3;
>    else if (operation = SENSORS_PROC_REAL_READ) {

You would need to change the library too. The values are internaly
stored as integers. The driver change the way it is displayed in /proc,
but libsensors reads the integer value directly so it also has the
magnitude hardcoded. The least I can say is that it's not very clean.
Fortunately, the new sysfs interface should let us get rid of that mess
in the long term.

Anyway, the correct way to solve your problem is to export values as
pseudo-millivolts in 2.6 and as pseudo-hundreds-of-volts in 2.4. This is
the way libsensors is configured to work, although that might sound
strange at first. The idea is that the formulas in sensors.conf should
not depend on the way values are represented in procfs/sysfs. The
formulas must be relevant of the physical reality (i.e. match the
resistors as set up on the motherboard, no more, no less). For now,
libsensors handles the differences between both kernels, so that sensors
and sensors.conf don't have to care. After all, that's what libraries
are for.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > [Don't forget to CC: the mailing list on replies please]
> 
> Well, all mails so far, have had a reply-to address. Just the last
> one had not. I'll have to be more careful then ;-)

Because I have been sending it through IMP instead of my usual mail
client (Sylpheed for that matter). It doesn't let me set a Reply-To
header, I'm sorry about that... Same goes for this one, BTW.

> video:/soft/src/lm_sensors2/kernel/chips # cat \
> 	/sys/bus/i2c/devices/0-0073/fan_div1
> 2
> video:/soft/src/lm_sensors2/kernel/chips # echo 1 > \
> 	/sys/bus/i2c/devices/0-0073/fan_div1
> video:/soft/src/lm_sensors2/kernel/chips # cat \
> 	/sys/bus/i2c/devices/0-0073/fan_div1
> 2
> video:/soft/src/lm_sensors2/kernel/chips #
> 
> Nothing special. The invalid value is simply ignored. Although
> set_fan_div()  returns -1 in that case, echo doesn't show any error.

Could you please check the value returned by echo? (i.e. "echo 1 >
fan_div1 ; echo $?"). I don't really think it can show the error, but
who knows... But why would we be allowed to return errors in these
functions if we cannot catch them later?

> How about adding a printk() for that case?

Sound like a good idea (although I think you have to use dev_info() or
something similar, not printk(), in Linux 2.6).

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> I was wrong. Correct is set_fan_div().
> 
> static ssize_t set_fan_div (struct i2c_client *client, struct
> fscher_data *data, const char *buf, size_t count, int nr, int reg)
> {
>    /* supported values: 2, 4, 8 */
>    int v = simple_strtoul(buf, NULL, 10);
> 
>    switch (v)
>    {
>    case 2:    v = 1;    break;
>    case 4:    v = 2;    break;
>    case 8:    v = 3;    break;
>    default:   return -1;
>    }
> 
>    /* bits 2..7 reserved => mask with 0x03 */
>    data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] &= ~0x03;
>    data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] |= v;
> 
>    fscher_write_value(client, reg,
>    data->fan_ripple[FAN_INDEX_FROM_NUM(nr)]); return count;
> }

I think that our policy is best effort, i.e. chose a correct value that
is not too far from what the user asked for. If you're really kind you
do some rounding stuff, at the expense of some CPU cycles. My opinion is
that this case is unlikely to happen, and if it happens it is most
likely an error, so we aren't supposed to be smart.

I think I would handle it that way:
1* v = (v >> 1) & 0x07.
2* v & 0x04 ? -> 3
   v & 0x02 ? -> 2
   else -> 1

Basically this means the following conversion table, if I'm not
mistaking:
0..2 -> 1 (div=2)
3..4 -> 2 (div=4)
5+   -> 3 (div=8)

Should be fast and efficient enough, and keeps the code clear (IMHO at
least).

Is it OK for you?

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:
> [Don't forget to CC: the mailing list on replies please]

Well, all mails so far, have had a reply-to address. Just the last one had 
not. I'll have to be more careful then ;-)

>>>>Basically this means the following conversion table, if I'm not
>>>>mistaking:
>>>>0..2 -> 1 (div=2)
>>>>3..4 -> 2 (div=4)
>>>>5+   -> 3 (div=8)
>>
>>I don't think, that this a good idea. If you recall an email from me
>>about VERAX fans, which supply 9 times as many pulses per rotation as
>>normal fans do, those fans would require a div of 18. This value would
>>then silently be converted to div = 8 and results in less then one
>>halve of the actual fan rpm.
>>
>>In my opinion, the driver should complain about any div values it
>>doesn't support, and simply do nothing as it wouldn't be what the user
>>expected, anyway. Therefore, once again the question: How should the
>>driver inform about/react on unsupported div values?
> 
> Well, this makes sense, you're right. You get the point.
> 
> In my mind, there was no possibility to return an error code, so you had
> to accomodate the best you could with the input, and keep quiet. By the
> way, what does conretely happen if you try to write a wrong value to the
> sysfs file?

video:/soft/src/lm_sensors2/kernel/chips # cat \
	/sys/bus/i2c/devices/0-0073/fan_div1
2
video:/soft/src/lm_sensors2/kernel/chips # echo 1 > \
	/sys/bus/i2c/devices/0-0073/fan_div1
video:/soft/src/lm_sensors2/kernel/chips # cat \
	/sys/bus/i2c/devices/0-0073/fan_div1
2
video:/soft/src/lm_sensors2/kernel/chips #

Nothing special. The invalid value is simply ignored. Although set_fan_div() 
returns -1 in that case, echo doesn't show any error.

How about adding a printk() for that case?

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>1* Fix libsensors so that it handles fscher correctly.
> 
> Oh, BTW: Could you please send me the list of files as they appear in
> sysfs? It'll be less error prone than reading the macros in your code (I
> have nothing against using them, but this makes thing a bit hard to
> understand at first sight).

video:~ # dir /sys/bus/i2c/devices/0-0073/
total 0
drwxr-xr-x    3 root     root            0 Jan 26 20:09 .
drwxr-xr-x    6 root     root            0 Jan 26 20:09 ..
-r--r--r--    1 root     root         4096 Jan 26 20:09 alarms
-rw-r--r--    1 root     root         4096 Jan 26 20:09 control
-rw-r--r--    1 root     root         4096 Jan 26 20:09 detach_state
-rw-r--r--    1 root     root         4096 Jan 26 20:09 fan_div1
-rw-r--r--    1 root     root         4096 Jan 26 20:09 fan_div2
-rw-r--r--    1 root     root         4096 Jan 26 20:09 fan_div3
-r--r--r--    1 root     root         4096 Jan 26 20:09 fan_input1
-r--r--r--    1 root     root         4096 Jan 26 20:09 fan_input2
-r--r--r--    1 root     root         4096 Jan 26 20:09 fan_input3
-rw-r--r--    1 root     root         4096 Jan 26 20:09 fan_status1
-rw-r--r--    1 root     root         4096 Jan 26 20:09 fan_status2
-rw-r--r--    1 root     root         4096 Jan 26 20:09 fan_status3
-r--r--r--    1 root     root         4096 Jan 26 20:09 in_input0
-r--r--r--    1 root     root         4096 Jan 26 20:09 in_input1
-r--r--r--    1 root     root         4096 Jan 26 20:09 in_input2
-r--r--r--    1 root     root         4096 Jan 26 20:09 name
drwxr-xr-x    2 root     root            0 Jan 26 20:09 power
-rw-r--r--    1 root     root            0 Jan 26 20:09 pwm1
-rw-r--r--    1 root     root            0 Jan 26 20:09 pwm2
-rw-r--r--    1 root     root            0 Jan 26 20:09 pwm3
-r--r--r--    1 root     root         4096 Jan 26 20:09 revision
-r--r--r--    1 root     root         4096 Jan 26 20:09 temp_input1
-r--r--r--    1 root     root         4096 Jan 26 20:09 temp_input2
-r--r--r--    1 root     root         4096 Jan 26 20:09 temp_input3
-rw-r--r--    1 root     root         4096 Jan 26 20:09 temp_status1
-rw-r--r--    1 root     root         4096 Jan 26 20:09 temp_status2
-rw-r--r--    1 root     root         4096 Jan 26 20:09 temp_status3
-rw-r--r--    1 root     root         4096 Jan 26 20:09 watchdog_control
-rw-r--r--    1 root     root         4096 Jan 26 20:09 watchdog_preset
-rw-r--r--    1 root     root         4096 Jan 26 20:09 watchdog_status

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>video:/soft/src/lm_sensors2/kernel/chips # cat \
>>	/sys/bus/i2c/devices/0-0073/fan_div1
>>2
>>video:/soft/src/lm_sensors2/kernel/chips # echo 1 > \
>>	/sys/bus/i2c/devices/0-0073/fan_div1
>>video:/soft/src/lm_sensors2/kernel/chips # cat \
>>	/sys/bus/i2c/devices/0-0073/fan_div1
>>2
>>video:/soft/src/lm_sensors2/kernel/chips #
>>
>>Nothing special. The invalid value is simply ignored. Although
>>set_fan_div()  returns -1 in that case, echo doesn't show any error.
> 
> Could you please check the value returned by echo? (i.e. "echo 1 >
> fan_div1 ; echo $?"). I don't really think it can show the error, but
> who knows... But why would we be allowed to return errors in these
> functions if we cannot catch them later?

Surprize! The driver's return value has an influence on echo's return value  :-))

In the case, where my driver returns -1, echo returns 1, otherwise 0.

>>How about adding a printk() for that case?
> 
> Sound like a good idea (although I think you have to use dev_info() or
> something similar, not printk(), in Linux 2.6).

I see, dev_info() will always print a message, while dev_dbg() print them only 
only when debug messages are requested.

So, although an application, that is writing the sysfs fan_div file, has a 
chance to detect, whether it worked or not, it might still be a good idea in 
this special case to show a message with acceptable values.

The attached version now outputs a message like below:

i2c_adapter i2c-0: fan_div value 3 not supported. Choose one of 2, 4 or 8!

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fscher.c.gz
Type: application/x-gzip
Size: 5037 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040128/fef3c67e/fscher.c.gz

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Is there a guide, how to port the sensors program?
>>
>>Without changes, sensors just gives me the following:
>>
>>fscher-i2c-0-73
>>Adapter: SMBus I801 adapter at 2000
>>+12V:           +11.86 V
>>+5V:             +5.05 V
>>Battery:         +3.05 V

The above is for kernel 2.6.1 and below for kernel 2.4.22:

fscher-i2c-0-73
Adapter: SMBus I801 adapter at 2000
Temp1/CPU:      +50.00 C
Temp2/MB:       +41.00 C
Temp3/AUX:      +32.00 C
Fan1/CPU:          960 RPM
Fan2/AUX:          960 RPM
Fan3/PS:          1680 RPM
+12V:           +118.58 V
+5V:            +50.47 V
Battery:        +30.54 V

The problem is, that the voltage values still need to get devided by 10.

Shall I scale the values by 1000 for kernel 2.6.1 and by 100 for kernel 2.4.22 
and remove the factor 1000 from sensors.conf?

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

[Don't forget to CC: the mailing list on replies please]

> >>Basically this means the following conversion table, if I'm not
> >>mistaking:
> >>0..2 -> 1 (div=2)
> >>3..4 -> 2 (div=4)
> >>5+   -> 3 (div=8)
> 
> I don't think, that this a good idea. If you recall an email from me
> about VERAX fans, which supply 9 times as many pulses per rotation as
> normal fans do, those fans would require a div of 18. This value would
> then silently be converted to div = 8 and results in less then one
> halve of the actual fan rpm.
> 
> In my opinion, the driver should complain about any div values it
> doesn't support, and simply do nothing as it wouldn't be what the user
> expected, anyway. Therefore, once again the question: How should the
> driver inform about/react on unsupported div values?

Well, this makes sense, you're right. You get the point.

In my mind, there was no possibility to return an error code, so you had
to accomodate the best you could with the input, and keep quiet. By the
way, what does conretely happen if you try to write a wrong value to the
sysfs file?

> >>Should be fast and efficient enough, and keeps the code clear (IMHO
> >>at least).
> 
> I don't see there the need for more efficient coding at that location,
> as it typically gets executed once per system start. Furthermore, I
> think that the switch statement is more readable in my simple case.

Agreed, because you treat only three (valid) values. Since my idea was
to accept them all, I couldn't handle them with a switch, it just
wouldn't have made sense. But a switch is fine for your method.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Well, looking at the conversion formula again, this idea is not 100 %
> perfect, because it doesn't care to scale the offset value. Currently,
> the offset is 0 but there is no guarantee, that it will always be 0.

This isn't a problem. The formulae are applied *after* libsensors (or
any other program such as gkrellm) reads the value from sysfs or procfs.
For example, if you have a raw reading of 1.8V, in 2.4 it'll be stored
internally as 180, and read through procfs as 1.80. In 2.6 it'll be
stored internally as 1800 and exported through sysfs as 1800 too. But
libsensors knows about magnitudes in both cases and will read the value
as 1.80. And any formula in sensors.conf will be applied to *this*
value.

So, as you can see, it is safe to have a single formula for both
versions.

> What do you think about some further RW files
> 
> 	in_refvoltage	= 0x21
> 	in_offset0	= 0x00
> 	in_offset1	= 0x00
> 	in_offset2	= 0x00
> 	in_multiplier0	= 0x31
> 	in_multiplier1	= 0x14
> 	in_multiplier2	= 0x0a
> 
> that initally have the values of the first driver release, which were
> taken from dmidecode. The driver would then use those values and
> supply scaled values for in0, in1 and in2.

I find it more confusing than helpful. All drivers have their formula in
sensors.conf and I believe we should stick to that model. Making
something different for one driver will make third-party apps unable to
handle it correctly unless they know the trick and add chip-specific
code. Remember that our aim with 2.6 and sysfs is to be able to live
without any specific code. Readings from sysfs and sensors.conf should
be sufficent to read temperatures, voltages and fans values and
configure limits and such, with no additional details given. It's still
not perfect since a few things are not well standardized (think to
alarms for example) but that's the idea.

So I suggest you go with your formulas in sensors.conf.eg, they look
just fine to me.

> BTW: was the multiplier for temperatures changed from 100 to 1000
> around 2.6.0-test5, because gkrellm-2.1.25 uses 100 instead of 1000?

I can't remember exactly when the change was made. I remember it was
discussed and we opted for something standard enough so that it would be
OK for everyone. More likely, gkrellm changed to accomodate our
decision.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>The problem is, that the voltage values still need to get devided by
>>10.
>>
>>Shall I scale the values by 1000 for kernel 2.6.1 and by 100 for
>>kernel 2.4.22 and remove the factor 1000 from sensors.conf?
> 
> Yes, that's the idea.

Well, looking at the conversion formula again, this idea is not 100 % perfect, 
because it doesn't care to scale the offset value. Currently, the offset is 0 
but there is no guarantee, that it will always be 0.

What do you think about some further RW files

	in_refvoltage	= 0x21
	in_offset0	= 0x00
	in_offset1	= 0x00
	in_offset2	= 0x00
	in_multiplier0	= 0x31
	in_multiplier1	= 0x14
	in_multiplier2	= 0x0a

that initally have the values of the first driver release, which were taken 
from dmidecode. The driver would then use those values and supply scaled 
values for in0, in1 and in2.

BTW: was the multiplier for temperatures changed from 100 to 1000 around 
2.6.0-test5, because gkrellm-2.1.25 uses 100 instead of 1000?

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Is there a guide for writing chip documentation, as I don't know what
>>to do with "Chip Features" in doc/chips/fscher?
> 
> No chip-specific documentation has been commited to Linux so far,
> although I planned to do so too. Basically I think that "Chip Features"
> should become "Sysfs Interface" and be a simple list of all files with
> permissions (R-, -W or RW) and comments if they are needed.

So I'll make the necessary changes soon.

> Here are the steps that need to be taken now:
> 1* Fix libsensors so that it handles fscher correctly.
> 2* Update the 2.4 version of the driver so that it handles voltages the
>    same way the 2.6 driver does.
> 3* Apply your patch to sensors.conf.eg.
> 4* Review your 2.6 driver and send it to Greg KH for inclusion into
>    Linux 2.6.3.
> 5* Fix sensors so that it shows errors for FSC chips.
> 
> You must understand that step 3 doesn't make sense unless step 2 is also
> achieved. The 2.4 and 2.6 drivers have to be as compatible as possible
> so that users can switch from one to the other and back.

I think, I can achieve this already later today.

> I will work on 1* (and possibly 5*) right now. Please work on 2*
> meanwhile. Once done I'll do 3*, and you'll test that both the old and
> the new driver work OK.
> 
> Then, and only then, I'll review your 2.6 driver (with changes if you
> have done any) and submit it to Greg.

Please have a look at set_pwm(). What shall I do with an unsupported value? Is 
it ok to return -1?

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Is there a guide, how to port the sensors program?
>>
>>Without changes, sensors just gives me the following:
>>
>>fscher-i2c-0-73
>>Adapter: SMBus I801 adapter at 2000
>>+12V:           +11.86 V
>>+5V:             +5.05 V
>>Battery:         +3.05 V

The above is for kernel 2.6.1 and below for kernel 2.4.22:

fscher-i2c-0-73
Adapter: SMBus I801 adapter at 2000
Temp1/CPU:      +50.00 C
Temp2/MB:       +41.00 C
Temp3/AUX:      +32.00 C
Fan1/CPU:          960 RPM
Fan2/AUX:          960 RPM
Fan3/PS:          1680 RPM
+12V:           +118.58 V
+5V:            +50.47 V
Battery:        +30.54 V

The problem is, that the voltage values still need to get devided by 10.

Shall I scale the values by 1000 for kernel 2.6.1 and by 100 for kernel 2.4.22
and remove the factor 1000 from sensors.conf?

BTW: changing *nrels_mag below from 2 to 3 fixed it for procfs, but still not 
for sensors.

void fscher_volt(struct i2c_client *client, int operation, int ctl_name,
                  int *nrels_mag, long *results)
{
   struct fscher_data *data = client->data;
   if (operation = SENSORS_PROC_REAL_INFO)
     *nrels_mag = 3;
   else if (operation = SENSORS_PROC_REAL_READ) {

Anyway, I'll work on it tomorrow :-))

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Now as kernel 2.6.0 is released, I feel the need to port fscher to
>>2.6.0. I've already started today, but didn't have success so far.

[snip]

> I'd suggest you first read Documentation/i2c/porting-clients as found in
> Linux 2.6.1-rc1 and later. That's a document I wrote about porting chip
> drivers from linux 2.4 to linux 2.6. I believe that almost all questions
> you'd ask are answered there. If you don't mind, I'll take a look at
> your code after you read that document and applied its contents to
> your driver.

Attached you'll find the 2.6.x module and configuration.

Is there a guide for writing chip documentation, as I don't know what to do 
with "Chip Features" in doc/chips/fscher?

Is there a guide, how to port the sensors program?

Without changes, sensors just gives me the following:

fscher-i2c-0-73
Adapter: SMBus I801 adapter at 2000
+12V:           +11.86 V
+5V:             +5.05 V
Battery:         +3.05 V

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fscher.c.gz
Type: application/x-gzip
Size: 4890 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040125/8f15d791/fscher.c.gz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sensors.conf.eg.patch.gz
Type: application/x-gzip
Size: 493 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040125/8f15d791/sensors.conf.eg.patch.gz

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> I'm sorry that you had to shape my code twice. Did you use some
> utility?

I did not shape it twice. Instead, I recoded the diffs in the new code.
Wasn't really complicated.

> Well, none of the two file types (pwm/fan_min) is perfekt for that
> case. It would be necessary to have something like 'pwm_min'!

If we have to choose, this is more like pwm, at least because of the
unit. What made me think wrong was the fact that it was named fan_min in
the 2.4 driver, and that code in sensors/chips.c:

      && !sensors_get_feature(*name,SENSORS_FSCHER_FAN1,&fan)
      && !sensors_get_feature(*name,SENSORS_FSCHER_FAN1_MIN,&min_rpm)
(...)
      else if (fan < min_rpm)
        printf("\t%6.0f RPM (not present or faulty)\n",fan);
      else
        printf("\t%6.0f RPM \n",fan);

I just can't make any sense out of this comparison. Units don't even
match. Am I missing something or is it plain wrong?

I read in the Hermes data sheet that FSC doesn't recommend to set the
pwm value lower than what it was set to by the BIOS. Since I don't think
anyone would want to set it higher (what would be the point? it is
supposed to go higher by itself on high load) I wonder if it is even
worth handling it in the driver. If we do, one possible safety would be
to store the registers value as the driver is loaded and not to allow
subsequent sets to set it lower than that.

Here is the plan I propose:

1* Clean up sensors code so that it doesn't reference fan_min. For one
thing, it'll let us get rid of the errors you had when running
"sensors". Or at least use the value the way it should be (doesn't seem
to be the case as of now).

2* Either rename fan_min in libsensors to pwm, or even remove it
completely. Depends if you want to keep it or not. If it is removed,
update sensors.conf accordingly.

3* If you want to keep the pwm stuff, rename it in the 2.4 driver as
well.

4* If you don't want to keep the pwm stuff, remove it from the 2.6
driver.

Usually, fan speeds are controled by scripts, not sensors. In this case
it is a bit different since the system can make fans go higher by
itself, so no closed-loop script is needed.

Basically everything is possible, keeping it in the driver only,
driver+library, driver+library+sensors.conf or
driver+library+sensors.conf+sensors. Depends on how useful the feature
is to you.

I can handle points 1 and 2, while I would let you handle points 3 and
4. Just tell me what you think should be done so that I can fix things
in the correct way.

Thanks.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>>Temp1/CPU:      failed
>>>Temp2/MB:       failed
>>>Temp3/AUX:      failed
> 
> There was a wrong magnitude in libsensors (statuses are not real
> temperatures so a magnitude of 3 wasn't a very subtle choice, my bad).
> Fixed in CVS, if you care to try.

Looks better now, but there are still problems with the fans:

video:~ # sensors
fscher-i2c-0-73
Adapter: SMBus I801 adapter at 2000
Temp1/CPU:      +52.00 C
Temp2/MB:       +44.00 C
Temp3/AUX:      +47.00 C
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
+12V:           +11.79 V
+5V:             +5.05 V
Battery:         +3.05 V

BTW: is it intended, that fscher is found at i2c-1-73 after reloading 
i2c_i801, and at i2c-2-73, ... after reloading it again, ...?

BTW2: your latest diff didn't contain FSCHER's DRIVER_ID.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> It seems, that you've used the latest driver file, I've sent, where
> all occurances of simple_stroul() put their result into a variable of
> type 'unsigned long' (instead of int) and where set_fan_div() outputs
> this message
> 
> 	dev_info(&client->adapter->dev, "fan_div value %ld not
> 	supported. Choose one of 2, 4 or 8!\n", v);
> 
> when it is called with an unsupported div value.

You mean that I have *not* being using it. Shame on me. I'll reintegrate
these changes into my version, sorry. Nice catching by the way. I'm glad
to see you're keeping a serious eye on me.

> > I committed a change to libsensors so that it finds fan_rippleX
> > files from the 2.6 driver. There are still a few matchings missing
> > (rev, control and wdog_*) but I don't think sensors uses them, does
> > it?
> 
> We've already discussed about that and I'd like to add them, but I
> still do not know, how to access those values.

Basically you can refer to them by their item names (see lib/chips.c) in
sensors.conf and access them with their ID in sensors. The thing is, I
don't think you want to do that since these are not things you want to
display in sensors' output nor write to with "sensors -s".

This is the reason I didn't bother supporting them in libsensors for the
2.6 version. But if you think it's better, I can do.

How are you using these files at the moment? How would you want to use
these values if you can't do that now? My impression is that rev is
mainly decorative, and that wdog would more likely be accessed directly
using shell scripts. I don't know about control - what is it for?

> Both drivers work as expected. The conversion formula returns correct
> values. But sensors gives the following output on 2.6.1:
> 
> fscher-i2c-1-73
> Adapter: SMBus I801 adapter at 2000
> Temp1/CPU:      failed
> Temp2/MB:       failed
> Temp3/AUX:      failed
> ERROR: Can't get FAN1 data!
> ERROR: Can't get FAN2 data!
> ERROR: Can't get FAN3 data!
> +12V:           +11.79 V
> +5V:             +5.05 V
> Battery:         +3.04 V

Is is libsensors from CVS? If if is, then I have not hacked in
correctly. Will take a look tomorrow.

> sensors-detect works for both kernels, both times considers fscher as
> 'todo'.

My bad, I forgot to update that by the time you commited the original
driver. Fixed in CVS.

> How can I set pwm* with 'sensors -s', as it gives me
> 
> Error: Line 1589: Unknown feature name
> Error: Line 1590: Unknown feature name
> Error: Line 1591: Unknown feature name
> 
> when I enable those lines in 'sensors.conf'.

This is also a libsensors hacking issue. I must really have missed
something.

I hope to fix everything by tomorrow.

Thanks.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

Hi Reinhard,

> You are right. I've attached the fixed drivers and adjusted sample 
> configuration. Documentation will follow later and explains, how to
> get the necessary (M)ultiplier, (R)efVoltage and (O)ffset values from
> dmidecode.

Sorry for the delay, I've been busy with the port of another chip driver
(gl518sm) these days. I am only reviewing what you sent to me now. I
have to apologize for that, it's not very fair from me.

I have updated the 2.4 driver and sensors.conf. I've slightly changed
the way input voltages are exported. I exported them as
pseudo-hundredths of Volts (range 0V - 2.55V) instead of an integer
value. I know that the value doesn't make much sense per se, but it
keeps the value within usual ranges and also saves a few conversions
later. I've updated the 2.6 driver accordingly so the same sensors.conf
will still work with both.

As far as the 2.6 driver is concerned, I've included it to a 2.6.2-rc3
source tree and made the other required changes (Makefile, Kconfig,
i2c-id.h). Full patch attached.

I've made a few changes to your original work. Especially I reindented
everything according to what kernel folks want. I don't like their
standard more than you seem to do, but we have to comply to it or your
driver won't be accepted.

I've also made several unimportant changes (comments, debug stuff, code
"design"). I left the functional stuff unchanged, since I know you know
what you're doing and I couldn't possibly test my changes anyway.

I committed a change to libsensors so that it finds fan_rippleX files
from the 2.6 driver. There are still a few matchings missing (rev,
control and wdog_*) but I don't think sensors uses them, does it?

I'd like you to get lm_sensors CVS, ensure that the new 2.4 driver works
with the updated sensors.conf, lib and sensors. Then try the first
candidate 2.6 driver, and report how it is going.

Once again, please forgive me for the delay, and thanks a lot for your
good work.

Greg, I know how precious your comments can be, so if you would like to
review and comment this proposed new driver, please do.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
-------------- next part --------------
diff -ruN linux-2.6.2-rc3/drivers/i2c/chips/Kconfig linux-2.6.2-rc3-k2/drivers/i2c/chips/Kconfig
--- linux-2.6.2-rc3/drivers/i2c/chips/Kconfig	Sat Jan 31 19:16:29 2004
+++ linux-2.6.2-rc3-k2/drivers/i2c/chips/Kconfig	Sun Feb  1 14:23:38 2004
@@ -45,6 +45,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called eeprom.
 
+config SENSORS_FSCHER
+	tristate "FSC Hermes"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Fujitsu Siemens
+	  Computers Hermes sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called fscher.
+
 config SENSORS_IT87
 	tristate "ITE IT87xx and compatibles"
 	depends on I2C && EXPERIMENTAL
diff -ruN linux-2.6.2-rc3/drivers/i2c/chips/Makefile linux-2.6.2-rc3-k2/drivers/i2c/chips/Makefile
--- linux-2.6.2-rc3/drivers/i2c/chips/Makefile	Sat Jan 31 19:16:29 2004
+++ linux-2.6.2-rc3-k2/drivers/i2c/chips/Makefile	Sun Feb  1 14:20:58 2004
@@ -8,6 +8,7 @@
 
 obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
+obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
 obj-$(CONFIG_SENSORS_LM78)	+= lm78.o
diff -ruN linux-2.6.2-rc3/drivers/i2c/chips/fscher.c linux-2.6.2-rc3-k2/drivers/i2c/chips/fscher.c
--- linux-2.6.2-rc3/drivers/i2c/chips/fscher.c	Thu Jan  1 01:00:00 1970
+++ linux-2.6.2-rc3-k2/drivers/i2c/chips/fscher.c	Sun Feb  1 15:36:26 2004
@@ -0,0 +1,685 @@
+/*
+ * fscher.c - Part of lm_sensors, Linux kernel modules for hardware
+ * monitoring
+ * Copyright (C) 2003, 2004 Reinhard Nissl <rnissl@gmx.de>
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* 
+ *  fujitsu siemens hermes chip, 
+ *  module based on fscpos.c 
+ *  Copyright (C) 2000 Hermann Jung <hej@odn.de>
+ *  Copyright (C) 1998, 1999 Frodo Looijaard <frodol@dds.nl>
+ *  and Philip Edelbrock <phil@netroedge.com>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+
+/*
+ * Addresses to scan
+ */
+
+static unsigned short normal_i2c[] = { 0x73, I2C_CLIENT_END };
+static unsigned short normal_i2c_range[] = { I2C_CLIENT_END };
+static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
+static unsigned int normal_isa_range[] = { I2C_CLIENT_ISA_END };
+
+/*
+ * Insmod parameters
+ */
+
+SENSORS_INSMOD_1(fscher);
+
+/*
+ * The FSCHER registers
+ */
+
+/* chip identification */
+#define FSCHER_REG_IDENT_0		0x00
+#define FSCHER_REG_IDENT_1		0x01
+#define FSCHER_REG_IDENT_2		0x02
+#define FSCHER_REG_REVISION		0x03
+
+/* global control and status */
+#define FSCHER_REG_EVENT_STATE		0x04
+#define FSCHER_REG_CONTROL		0x05
+
+/* watchdog */
+#define FSCHER_REG_WDOG_PRESET		0x28
+#define FSCHER_REG_WDOG_STATE		0x23
+#define FSCHER_REG_WDOG_CONTROL		0x21
+
+/* fan 0 */
+#define FSCHER_REG_FAN0_MIN		0x55
+#define FSCHER_REG_FAN0_ACT		0x0e
+#define FSCHER_REG_FAN0_STATE		0x0d
+#define FSCHER_REG_FAN0_RIPPLE		0x0f
+
+/* fan 1 */
+#define FSCHER_REG_FAN1_MIN		0x65
+#define FSCHER_REG_FAN1_ACT		0x6b
+#define FSCHER_REG_FAN1_STATE		0x62
+#define FSCHER_REG_FAN1_RIPPLE		0x6f
+
+/* fan 2 */
+#define FSCHER_REG_FAN2_MIN		0xb5
+#define FSCHER_REG_FAN2_ACT		0xbb
+#define FSCHER_REG_FAN2_STATE		0xb2
+#define FSCHER_REG_FAN2_RIPPLE		0xbf
+
+/* voltage supervision */
+#define FSCHER_REG_VOLT_12		0x45
+#define FSCHER_REG_VOLT_5		0x42
+#define FSCHER_REG_VOLT_BATT		0x48
+
+/* temperature 0 */
+#define FSCHER_REG_TEMP0_ACT		0x64
+#define FSCHER_REG_TEMP0_STATE		0x71
+
+/* temperature 1 */
+#define FSCHER_REG_TEMP1_ACT		0x32
+#define FSCHER_REG_TEMP1_STATE		0x81
+
+/* temperature 2 */
+#define FSCHER_REG_TEMP2_ACT		0x35
+#define FSCHER_REG_TEMP2_STATE		0x91
+
+/*
+ * Functions declaration
+ */
+
+static int fscher_attach_adapter(struct i2c_adapter *adapter);
+static int fscher_detect(struct i2c_adapter *adapter, int address, int kind);
+static int fscher_detach_client(struct i2c_client *client);
+static void fscher_update_client(struct i2c_client *client);
+
+
+static int fscher_read_value(struct i2c_client *client, u8 register);
+static int fscher_write_value(struct i2c_client *client, u8 register,
+			      u8 value);
+static void fscher_init_client(struct i2c_client *client);
+
+/*
+ * Driver data (common to all clients)
+ */
+ 
+static struct i2c_driver fscher_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "fscher",
+	.id		= I2C_DRIVERID_FSCHER,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= fscher_attach_adapter,
+	.detach_client	= fscher_detach_client,
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+
+struct fscher_data {
+	struct semaphore update_lock;
+	char valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	u8 revision;		/* revision of chip */
+	u8 global_event;	/* global event status */
+	u8 global_control;	/* global control register */
+	u8 watchdog[3];		/* watchdog */
+	u8 volt[3];		/* 12, 5, battery voltage */ 
+	u8 temp_act[3];		/* temperature */
+	u8 temp_status[3];	/* status of sensor */
+	u8 fan_act[3];		/* fans revolutions per second */
+	u8 fan_status[3];	/* fan status */
+	u8 fan_min[3];		/* fan min value for rps */
+	u8 fan_ripple[3];	/* divider for rps */
+};
+
+/*
+ * Internal variables
+ */
+
+static int fscher_id = 0;
+
+/*
+ * Sysfs stuff
+ */
+
+#define sysfs_r(kind, offset, reg) \
+static ssize_t show_##kind (struct fscher_data *, char *, int); \
+static ssize_t show_##kind##offset (struct device *, char *); \
+static ssize_t show_##kind##offset (struct device *dev, char *buf) \
+{ \
+	struct i2c_client *client = to_i2c_client(dev); \
+	struct fscher_data *data = i2c_get_clientdata(client); \
+	fscher_update_client(client); \
+	return show_##kind(data, buf, (offset)); \
+}
+
+#define sysfs_w(kind, offset, reg) \
+static ssize_t set_##kind (struct i2c_client *, struct fscher_data *, const char *, size_t, int, int); \
+static ssize_t set_##kind##offset (struct device *, const char *, size_t); \
+static ssize_t set_##kind##offset (struct device *dev, const char *buf, size_t count) \
+{ \
+	struct i2c_client *client = to_i2c_client(dev); \
+	struct fscher_data *data = i2c_get_clientdata(client); \
+	return set_##kind(client, data, buf, count, (offset), reg); \
+}
+
+#define sysfs_rw_n(kind, offset, reg) \
+sysfs_r(kind, offset, reg) \
+sysfs_w(kind, offset, reg) \
+static DEVICE_ATTR(kind##offset, S_IRUGO | S_IWUSR, show_##kind##offset, set_##kind##offset);
+
+#define sysfs_rw(kind, reg) \
+sysfs_r(kind, 0, reg) \
+sysfs_w(kind, 0, reg) \
+static DEVICE_ATTR(kind, S_IRUGO | S_IWUSR, show_##kind##0, set_##kind##0);
+
+#define sysfs_ro_n(kind, offset, reg) \
+sysfs_r(kind, offset, reg) \
+static DEVICE_ATTR(kind##offset, S_IRUGO, show_##kind##offset, NULL);
+
+#define sysfs_ro(kind, reg) \
+sysfs_r(kind, 0, reg) \
+static DEVICE_ATTR(kind, S_IRUGO, show_##kind##0, NULL);
+
+#define sysfs_fan(offset, reg_status, reg_min, reg_ripple, reg_act) \
+sysfs_rw_n(pwm       , offset, reg_min) \
+sysfs_rw_n(fan_status, offset, reg_status) \
+sysfs_rw_n(fan_div   , offset, reg_ripple) \
+sysfs_ro_n(fan_input , offset, reg_act)
+
+#define sysfs_temp(offset, reg_status, reg_act) \
+sysfs_rw_n(temp_status, offset, reg_status) \
+sysfs_ro_n(temp_input , offset, reg_act)
+    
+#define sysfs_in(offset, reg_act) \
+sysfs_ro_n(in_input, offset, reg_act)
+
+#define sysfs_revision(reg_revision) \
+sysfs_ro(revision, reg_revision)
+
+#define sysfs_alarms(reg_events) \
+sysfs_ro(alarms, reg_events)
+
+#define sysfs_control(reg_control) \
+sysfs_rw(control, reg_control)
+
+#define sysfs_watchdog(reg_control, reg_status, reg_preset) \
+sysfs_rw(watchdog_control, reg_control) \
+sysfs_rw(watchdog_status , reg_status) \
+sysfs_rw(watchdog_preset , reg_preset)
+
+sysfs_fan(1, FSCHER_REG_FAN0_STATE, FSCHER_REG_FAN0_MIN,
+	     FSCHER_REG_FAN0_RIPPLE, FSCHER_REG_FAN0_ACT)
+sysfs_fan(2, FSCHER_REG_FAN1_STATE, FSCHER_REG_FAN1_MIN,
+	     FSCHER_REG_FAN1_RIPPLE, FSCHER_REG_FAN1_ACT)
+sysfs_fan(3, FSCHER_REG_FAN2_STATE, FSCHER_REG_FAN2_MIN,
+	     FSCHER_REG_FAN2_RIPPLE, FSCHER_REG_FAN2_ACT)
+
+sysfs_temp(1, FSCHER_REG_TEMP0_STATE, FSCHER_REG_TEMP0_ACT)
+sysfs_temp(2, FSCHER_REG_TEMP1_STATE, FSCHER_REG_TEMP1_ACT)
+sysfs_temp(3, FSCHER_REG_TEMP2_STATE, FSCHER_REG_TEMP2_ACT)
+
+sysfs_in(0, FSCHER_REG_VOLT_12)
+sysfs_in(1, FSCHER_REG_VOLT_5)
+sysfs_in(2, FSCHER_REG_VOLT_BATT)
+
+sysfs_revision(FSCHER_REG_REVISION)
+sysfs_alarms(FSCHER_REG_EVENTS)
+sysfs_control(FSCHER_REG_CONTROL)
+sysfs_watchdog(FSCHER_REG_WDOG_CONTROL, FSCHER_REG_WDOG_STATE, FSCHER_REG_WDOG_PRESET)
+  
+#define device_create_file_fan(client, offset) \
+do { \
+	device_create_file(&client->dev, &dev_attr_fan_status##offset); \
+	device_create_file(&client->dev, &dev_attr_pwm##offset); \
+	device_create_file(&client->dev, &dev_attr_fan_div##offset); \
+	device_create_file(&client->dev, &dev_attr_fan_input##offset); \
+} while (0)
+
+#define device_create_file_temp(client, offset) \
+do { \
+	device_create_file(&client->dev, &dev_attr_temp_status##offset); \
+	device_create_file(&client->dev, &dev_attr_temp_input##offset); \
+} while (0)
+
+#define device_create_file_in(client, offset) \
+do { \
+	device_create_file(&client->dev, &dev_attr_in_input##offset); \
+} while (0)
+
+#define device_create_file_revision(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_revision); \
+} while (0)
+
+#define device_create_file_alarms(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_alarms); \
+} while (0)
+
+#define device_create_file_control(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_control); \
+} while (0)
+
+#define device_create_file_watchdog(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_watchdog_status); \
+	device_create_file(&client->dev, &dev_attr_watchdog_control); \
+	device_create_file(&client->dev, &dev_attr_watchdog_preset); \
+} while (0)
+  
+/*
+ * Real code
+ */
+
+static int fscher_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_detect(adapter, &addr_data, fscher_detect);
+}
+
+static int fscher_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *new_client;
+	struct fscher_data *data;
+	int err = 0;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		goto exit;
+
+	/* OK. For now, we presume we have a valid client. We now create the
+	 * client structure, even though we cannot fill it completely yet.
+	 * But it allows us to access i2c_smbus_read_byte_data. */
+	if (!(new_client = kmalloc(sizeof(struct i2c_client) +
+	    sizeof(struct fscher_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+  	}
+	memset(new_client, 0x00, sizeof(struct i2c_client) +
+	       sizeof(struct fscher_data));
+
+	/* The Hermes-specific data is placed right after the common I2C
+	 * client data. */
+	data = (struct fscher_data *) (new_client + 1);
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &fscher_driver;
+	new_client->flags = 0;
+
+	/* Do the remaining detection unless force or force_fscher parameter */
+	if (kind < 0) {
+		if (i2c_smbus_read_byte_data(new_client,
+		    FSCHER_REG_IDENT_0) != 0x48		/* 'H' */
+		 || i2c_smbus_read_byte_data(new_client,
+		    FSCHER_REG_IDENT_1) != 0x45		/* 'E' */
+		 || i2c_smbus_read_byte_data(new_client,
+		    FSCHER_REG_IDENT_2) != 0x52)	/* 'R' */
+			goto exit_free;
+	}
+
+	/* Fill in the remaining client fields and put it into the
+	 * global list */
+	strlcpy(new_client->name, "fscher", I2C_NAME_SIZE);
+	new_client->id = fscher_id++;
+	data->valid = 0;
+	init_MUTEX(&data->update_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+
+	fscher_init_client(new_client);
+
+	/* Register sysfs hooks */
+	device_create_file_revision(new_client);
+	device_create_file_alarms(new_client);
+	device_create_file_control(new_client);
+	device_create_file_watchdog(new_client);
+
+	device_create_file_in(new_client, 0);
+	device_create_file_in(new_client, 1);
+	device_create_file_in(new_client, 2);
+
+	device_create_file_fan(new_client, 1);
+	device_create_file_fan(new_client, 2);
+	device_create_file_fan(new_client, 3);
+
+	device_create_file_temp(new_client, 1);
+	device_create_file_temp(new_client, 2);
+	device_create_file_temp(new_client, 3);
+
+	return 0;
+
+exit_free:
+	kfree(new_client);
+exit:
+	return err;
+}
+
+static int fscher_detach_client(struct i2c_client *client)
+{
+	int err;
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->adapter->dev,
+			"Client deregistration failed, client not detached.\n");
+		return err;
+	}
+
+	kfree(client);
+	return 0;
+}
+
+static int fscher_read_value(struct i2c_client *client, u8 reg)
+{
+	dev_dbg(&client->dev, "read reg 0x%02x\n", reg);
+
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fscher_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	dev_dbg(&client->dev, "write reg 0x%02x, val 0x%02x\n",
+		reg, value);
+
+	return i2c_smbus_write_byte_data(client, reg, value);
+}
+
+/* Called when we have found a new FSC Hermes. */
+static void fscher_init_client(struct i2c_client *client)
+{
+	struct fscher_data *data = i2c_get_clientdata(client);
+
+	/* Read revision from chip */
+	data->revision =  fscher_read_value(client, FSCHER_REG_REVISION);
+}
+
+static void fscher_update_client(struct i2c_client *client)
+{
+	struct fscher_data *data = i2c_get_clientdata(client);
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > 2 * HZ) ||
+	    (jiffies < data->last_updated) || !data->valid) {
+
+		dev_dbg(&client->dev, "Starting fscher update\n");
+
+		data->temp_act[0] = fscher_read_value(client, FSCHER_REG_TEMP0_ACT);
+		data->temp_act[1] = fscher_read_value(client, FSCHER_REG_TEMP1_ACT);
+		data->temp_act[2] = fscher_read_value(client, FSCHER_REG_TEMP2_ACT);
+		data->temp_status[0] = fscher_read_value(client, FSCHER_REG_TEMP0_STATE);
+		data->temp_status[1] = fscher_read_value(client, FSCHER_REG_TEMP1_STATE);
+		data->temp_status[2] = fscher_read_value(client, FSCHER_REG_TEMP2_STATE);
+
+		data->volt[0] = fscher_read_value(client, FSCHER_REG_VOLT_12);
+		data->volt[1] = fscher_read_value(client, FSCHER_REG_VOLT_5);
+		data->volt[2] = fscher_read_value(client, FSCHER_REG_VOLT_BATT);
+
+		data->fan_act[0] = fscher_read_value(client, FSCHER_REG_FAN0_ACT);
+		data->fan_act[1] = fscher_read_value(client, FSCHER_REG_FAN1_ACT);
+		data->fan_act[2] = fscher_read_value(client, FSCHER_REG_FAN2_ACT);
+		data->fan_status[0] = fscher_read_value(client, FSCHER_REG_FAN0_STATE);
+		data->fan_status[1] = fscher_read_value(client, FSCHER_REG_FAN1_STATE);
+		data->fan_status[2] = fscher_read_value(client, FSCHER_REG_FAN2_STATE);
+		data->fan_min[0] = fscher_read_value(client, FSCHER_REG_FAN0_MIN);
+		data->fan_min[1] = fscher_read_value(client, FSCHER_REG_FAN1_MIN);
+		data->fan_min[2] = fscher_read_value(client, FSCHER_REG_FAN2_MIN);
+		data->fan_ripple[0] = fscher_read_value(client, FSCHER_REG_FAN0_RIPPLE);
+		data->fan_ripple[1] = fscher_read_value(client, FSCHER_REG_FAN1_RIPPLE);
+		data->fan_ripple[2] = fscher_read_value(client, FSCHER_REG_FAN2_RIPPLE);
+
+		data->watchdog[0] = fscher_read_value(client, FSCHER_REG_WDOG_PRESET);
+		data->watchdog[1] = fscher_read_value(client, FSCHER_REG_WDOG_STATE);
+		data->watchdog[2] = fscher_read_value(client, FSCHER_REG_WDOG_CONTROL);
+
+		data->global_event = fscher_read_value(client, FSCHER_REG_EVENT_STATE);
+
+		data->last_updated = jiffies;
+		data->valid = 1;                 
+	}
+
+	up(&data->update_lock);
+}
+
+
+
+#define FAN_INDEX_FROM_NUM(nr)	((nr) - 1)
+
+static ssize_t set_fan_status(struct i2c_client *client, struct fscher_data *data,
+			      const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 0..1, 3..7 reserved => mask with 0x04 */  
+	int v = simple_strtoul(buf, NULL, 10) & 0x04;
+	data->fan_status[FAN_INDEX_FROM_NUM(nr)] &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_fan_status(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 0..1, 3..7 reserved => mask with 0x04 */  
+	return sprintf(buf, "%u\n", data->fan_status[FAN_INDEX_FROM_NUM(nr)] & 0x04);
+}
+
+static ssize_t set_pwm(struct i2c_client *client, struct fscher_data *data,
+		       const char *buf, size_t count, int nr, int reg)
+{
+	data->fan_min[FAN_INDEX_FROM_NUM(nr)] = simple_strtoul(buf, NULL, 10) & 0xff;
+
+	fscher_write_value(client, reg, data->fan_min[FAN_INDEX_FROM_NUM(nr)]);
+	return count;
+}
+
+static ssize_t show_pwm (struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", data->fan_min[FAN_INDEX_FROM_NUM(nr)]);
+}
+
+static ssize_t set_fan_div(struct i2c_client *client, struct fscher_data *data,
+			   const char *buf, size_t count, int nr, int reg)
+{
+	/* supported values: 2, 4, 8 */
+	int v = simple_strtoul(buf, NULL, 10);
+
+	switch (v)
+	{
+	case 2: v = 1; break;
+	case 4: v = 2; break;
+	case 8: v = 3; break;
+	default: return -1;
+	}
+
+	/* bits 2..7 reserved => mask with 0x03 */
+	data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] &= ~0x03;
+	data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] |= v;
+
+	fscher_write_value(client, reg, data->fan_ripple[FAN_INDEX_FROM_NUM(nr)]);
+	return count;
+}
+
+static ssize_t show_fan_div(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 2..7 reserved => mask with 0x03 */  
+	return sprintf(buf, "%u\n", 1 << (data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] & 0x03));
+}
+
+#define RPM_FROM_REG(val)	(val*60)
+
+static ssize_t show_fan_input (struct fscher_data *data, char *buf, int nr)
+{
+  return sprintf(buf, "%u\n", RPM_FROM_REG(data->fan_act[FAN_INDEX_FROM_NUM(nr)]));
+}
+
+
+
+#define TEMP_INDEX_FROM_NUM(nr)		((nr) - 1)
+
+static ssize_t set_temp_status(struct i2c_client *client, struct fscher_data *data,
+			       const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 2..7 reserved, 0 read only => mask with 0x02 */  
+	int v = simple_strtoul(buf, NULL, 10) & 0x02;
+	data->temp_status[TEMP_INDEX_FROM_NUM(nr)] &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_temp_status(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 2..7 reserved => mask with 0x03 */
+	return sprintf(buf, "%u\n", data->temp_status[TEMP_INDEX_FROM_NUM(nr)] & 0x03);
+}
+
+#define TEMP_FROM_REG(val)	(((val) - 128) * 1000)
+
+static ssize_t show_temp_input(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_act[TEMP_INDEX_FROM_NUM(nr)]));
+}
+
+
+
+/*
+ * The final conversion is specified in sensors.conf, as it depends on
+ * mainboard specific values. We export the registers contents as
+ * pseudo-hundredths-of-Volts (range 0V - 2.55V). Not that it makes much
+ * sense per se, but it minimizes the conversions count and keeps the
+ * values within a usual range.
+ */
+#define VOLT_FROM_REG(val)	((val) * 10)
+
+static ssize_t show_in_input(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[nr]));
+}
+
+
+
+static ssize_t show_revision(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", data->revision);
+}
+
+
+
+static ssize_t show_alarms(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 2, 5..6 reserved => mask with 0x9b */
+	return sprintf(buf, "%u\n", data->global_event & 0x9b);
+}
+
+
+
+static ssize_t set_control(struct i2c_client *client, struct fscher_data *data,
+			   const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 1..7 reserved => mask with 0x01 */  
+	int v = simple_strtoul(buf, NULL, 10) & 0x01;
+	data->global_control &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_control(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 1..7 reserved => mask with 0x01 */
+	return sprintf(buf, "%u\n", data->global_control & 0x01);
+}
+
+
+
+static ssize_t set_watchdog_control(struct i2c_client *client, struct
+				    fscher_data *data, const char *buf, size_t count,
+				    int nr, int reg)
+{
+	/* bits 0..3 reserved => mask with 0xf0 */  
+	int v = simple_strtoul(buf, NULL, 10) & 0xf0;
+	data->watchdog[2] &= ~0xf0;
+	data->watchdog[2] |= v;
+
+	fscher_write_value(client, reg, data->watchdog[2]);
+	return count;
+}
+
+static ssize_t show_watchdog_control(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 0..3 reserved, bit 5 write only => mask with 0xd0 */
+	return sprintf(buf, "%u\n", data->watchdog[2] & 0xd0);
+}
+
+static ssize_t set_watchdog_status(struct i2c_client *client, struct fscher_data *data,
+				   const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 0, 2..7 reserved => mask with 0x02 */  
+	int v = simple_strtoul(buf, NULL, 10) & 0x02;
+	data->watchdog[1] &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_watchdog_status(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 0, 2..7 reserved => mask with 0x02 */
+	return sprintf(buf, "%u\n", data->watchdog[1] & 0x02);
+}
+
+static ssize_t set_watchdog_preset(struct i2c_client *client, struct fscher_data *data,
+				   const char *buf, size_t count, int nr, int reg)
+{
+	data->watchdog[0] = simple_strtoul(buf, NULL, 10) & 0xff;
+
+	fscher_write_value(client, reg, data->watchdog[0]);
+	return count;
+}
+
+static ssize_t show_watchdog_preset(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", data->watchdog[0]);
+}
+
+
+
+static int __init sensors_fscher_init(void)
+{
+	return i2c_add_driver(&fscher_driver);
+}
+
+static void __exit sensors_fscher_exit(void)
+{
+	i2c_del_driver(&fscher_driver);
+}
+
+MODULE_AUTHOR("Reinhard Nissl <rnissl@gmx.de> based on work from Hermann"
+	      " Jung <hej@odn.de>, Frodo Looijaard <frodol@dds.nl> and"
+	      " Philip Edelbrock <phil@netroedge.com>");
+MODULE_DESCRIPTION("FSC Hermes driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_fscher_init);
+module_exit(sensors_fscher_exit);
--- linux-2.6.2-rc3/include/linux/i2c-id.h	Sat Jan 31 19:16:37 2004
+++ linux-2.6.2-rc3-k2/include/linux/i2c-id.h	Sun Feb  1 15:25:25 2004
@@ -156,6 +156,7 @@
 #define I2C_DRIVERID_LM83 1040
 #define I2C_DRIVERID_LM90 1042
 #define I2C_DRIVERID_ASB100 1043
+#define I2C_DRIVERID_FSCHER 1046
 #define I2C_DRIVERID_W83L785TS 1047
 
 /*

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>>It seems, that you've used the latest driver file, I've sent, where
>>>all occurances of simple_stroul() put their result into a variable
>>>of type 'unsigned long' (instead of int) and where set_fan_div()
>>>outputs this message
>>>
>>>	dev_info(&client->adapter->dev, "fan_div value %ld not
>>>	supported. Choose one of 2, 4 or 8!\n", v);
>>>
>>>when it is called with an unsupported div value.
>>
>>You mean that I have *not* being using it. Shame on me. I'll
>>reintegrate these changes into my version, sorry. Nice catching by the
>>way. I'm glad to see you're keeping a serious eye on me.
> 
> Done, with a bit of shaping to keep it in the Good Coding Style (TM).
> New version attached.

I'm sorry that you had to shape my code twice. Did you use some utility?

>>>Temp1/CPU:      failed
>>>Temp2/MB:       failed
>>>Temp3/AUX:      failed
> 
> There was a wrong magnitude in libsensors (statuses are not real
> temperatures so a magnitude of 3 wasn't a very subtle choice, my bad).
> Fixed in CVS, if you care to try.

I'll give it a try later today.

>>>ERROR: Can't get FAN1 data!
>>>ERROR: Can't get FAN2 data!
>>>ERROR: Can't get FAN3 data!
>>>(...)
>>>How can I set pwm* with 'sensors -s', as it gives me
>>>Error: Line 1589: Unknown feature name
>>>Error: Line 1590: Unknown feature name
>>>Error: Line 1591: Unknown feature name
>>>when I enable those lines in 'sensors.conf'.
> 
> Your driver is in fact mixing fan mins and pwm, while these are
> completely different things. I just realize now that the Hermes doesn't
> have PWM capabilities (at least according to your 2.4 driver
> documentation and the data sheet). PWM designates the possibility to
> regulate the fan speed. fan mins are limits under which fan alarms
> trigger. The Hermes has fan mins, bot no PWM. So, whatever you have been
> trying to do, it is obviously not correct.
> 
> Please replace your set_pwm and get_pwm functions with set_fan_min and
> show_fan_min. show_fan_min should look much like show_fan_input. I leave
> set_fan_min to you. Rename sysfs files accoringly. Remember that these
> sysfs files deal with RPM values, not raw register values as you were
> doing with pwm.

Well, none of the two file types (pwm/fan_min) is perfekt for that case. It 
would be necessary to have something like 'pwm_min'!

You are right, that this register has a little bit the functionality of 
fan_min. If the register is set to 0, the fan is allowed to stop. In any other 
case, the fan must be spinning, but the minimum speed is up to the internals 
of HERMES and is not depending on the register's value. If HERMES thinks, that 
the fan speed is too low, it sets the fan's state register to indicate a fan 
failure.

But I think, this register behaves more like 'pwm': as long as the system is 
cool enough, HERMES will honor the registers value and generate a 3.3 V PWM 
signal (T = 30.8 us) with the following timing:

	reg		tp [us]		U [V]		meaning
	0		0		0		OFF
	1		8.4		6.09		MIN
	...		...		...		...
	255		29.6		11.75		MAX

If the system is put under high load and gets hot, HERMES no longer honors the 
user supplied pwm value and overrides it with its own, that will protect the 
system from overheating. One can think of the following equation:

	pwm_to_use = max(pwm_system_safety, pwm_user_supplied)

pwm_system_safety is determined by a sensor-fan-matrix. It is used to have e. 
g. the AUX fan turn faster if the CPU heats up. But for any reason, the BIOS 
doesn't connect any sensor to the AUX fan, which means that the fan will stay 
off forever, when I set it's register to 0.

I've already contacted FSC for additonal information, but they cannot tell, 
how to access the sensor-fan-matrix, due to the fact, that it would allow to 
drop the connection e. g. between CPU sensor and CPU fan. If no reasonable 
values are programmed to the matrix, one will finally detroy its system due to 
overheating. FSC fears, that a huge number of people might take them into 
warrenty for messed up systems, where FSC would then have to prove, that e. g. 
the board was damaged by improper vales programmed into HERMES. But I was also 
told, that future BIOS versions will give more control over HERMES, but there 
is still no release date.

Therefore, I've already thought about a little utility, that reads the sensors 
and writes reasonable values to this register. One more reason to call it pwm.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>You are right. I've attached the fixed drivers and adjusted sample 
>>configuration. Documentation will follow later and explains, how to
>>get the necessary (M)ultiplier, (R)efVoltage and (O)ffset values from
>>dmidecode.
> 
> I have updated the 2.4 driver and sensors.conf. I've slightly changed
> the way input voltages are exported. I exported them as
> pseudo-hundredths of Volts (range 0V - 2.55V) instead of an integer
> value. I know that the value doesn't make much sense per se, but it
> keeps the value within usual ranges and also saves a few conversions
> later. I've updated the 2.6 driver accordingly so the same sensors.conf
> will still work with both.
> 
> As far as the 2.6 driver is concerned, I've included it to a 2.6.2-rc3
> source tree and made the other required changes (Makefile, Kconfig,
> i2c-id.h). Full patch attached.
> 
> I've made a few changes to your original work. Especially I reindented
> everything according to what kernel folks want. I don't like their
> standard more than you seem to do, but we have to comply to it or your
> driver won't be accepted.

Thank you very much for doing that.

> I've also made several unimportant changes (comments, debug stuff, code
> "design"). I left the functional stuff unchanged, since I know you know
> what you're doing and I couldn't possibly test my changes anyway.

It seems, that you've used the latest driver file, I've sent, where all 
occurances of simple_stroul() put their result into a variable of type 
'unsigned long' (instead of int) and where set_fan_div() outputs this message

	dev_info(&client->adapter->dev, "fan_div value %ld not supported. Choose one 
of 2, 4 or 8!\n", v);

when it is called with an unsupported div value.

> I committed a change to libsensors so that it finds fan_rippleX files
> from the 2.6 driver. There are still a few matchings missing (rev,
> control and wdog_*) but I don't think sensors uses them, does it?

We've already discussed about that and I'd like to add them, but I still do 
not know, how to access those values.

> I'd like you to get lm_sensors CVS, ensure that the new 2.4 driver works
> with the updated sensors.conf, lib and sensors. Then try the first
> candidate 2.6 driver, and report how it is going.

Both drivers work as expected. The conversion formula returns correct values. 
But sensors gives the following output on 2.6.1:

fscher-i2c-1-73
Adapter: SMBus I801 adapter at 2000
Temp1/CPU:      failed
Temp2/MB:       failed
Temp3/AUX:      failed
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
+12V:           +11.79 V
+5V:             +5.05 V
Battery:         +3.04 V

sensors-detect works for both kernels, both times considers fscher as 'todo'.

How can I set pwm* with 'sensors -s', as it gives me

Error: Line 1589: Unknown feature name
Error: Line 1590: Unknown feature name
Error: Line 1591: Unknown feature name

when I enable those lines in 'sensors.conf'.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > It seems, that you've used the latest driver file, I've sent, where
> > all occurances of simple_stroul() put their result into a variable
> > of type 'unsigned long' (instead of int) and where set_fan_div()
> > outputs this message
> > 
> > 	dev_info(&client->adapter->dev, "fan_div value %ld not
> > 	supported. Choose one of 2, 4 or 8!\n", v);
> > 
> > when it is called with an unsupported div value.
> 
> You mean that I have *not* being using it. Shame on me. I'll
> reintegrate these changes into my version, sorry. Nice catching by the
> way. I'm glad to see you're keeping a serious eye on me.

Done, with a bit of shaping to keep it in the Good Coding Style (TM).
New version attached.

> > Temp1/CPU:      failed
> > Temp2/MB:       failed
> > Temp3/AUX:      failed

There was a wrong magnitude in libsensors (statuses are not real
temperatures so a magnitude of 3 wasn't a very subtle choice, my bad).
Fixed in CVS, if you care to try.

> > ERROR: Can't get FAN1 data!
> > ERROR: Can't get FAN2 data!
> > ERROR: Can't get FAN3 data!
> > (...)
> > How can I set pwm* with 'sensors -s', as it gives me
> > Error: Line 1589: Unknown feature name
> > Error: Line 1590: Unknown feature name
> > Error: Line 1591: Unknown feature name
> > when I enable those lines in 'sensors.conf'.

Your driver is in fact mixing fan mins and pwm, while these are
completely different things. I just realize now that the Hermes doesn't
have PWM capabilities (at least according to your 2.4 driver
documentation and the data sheet). PWM designates the possibility to
regulate the fan speed. fan mins are limits under which fan alarms
trigger. The Hermes has fan mins, bot no PWM. So, whatever you have been
trying to do, it is obviously not correct.

Please replace your set_pwm and get_pwm functions with set_fan_min and
show_fan_min. show_fan_min should look much like show_fan_input. I leave
set_fan_min to you. Rename sysfs files accoringly. Remember that these
sysfs files deal with RPM values, not raw register values as you were
doing with pwm.

You'll also need to provide a diff against etc/sensors.conf.eg.

Feel free to ask questions if needed. I admit I'm quite surprized that
you had difficulties on this since it's not supposed to be complex.
Sorry for not noticing and telling you before.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
-------------- next part --------------
diff -ruN linux-2.6.2-rc3/drivers/i2c/chips/Kconfig linux-2.6.2-rc3-k2/drivers/i2c/chips/Kconfig
--- linux-2.6.2-rc3/drivers/i2c/chips/Kconfig	Sat Jan 31 19:16:29 2004
+++ linux-2.6.2-rc3-k2/drivers/i2c/chips/Kconfig	Sun Feb  1 14:23:38 2004
@@ -45,6 +45,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called eeprom.
 
+config SENSORS_FSCHER
+	tristate "FSC Hermes"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Fujitsu Siemens
+	  Computers Hermes sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called fscher.
+
 config SENSORS_IT87
 	tristate "ITE IT87xx and compatibles"
 	depends on I2C && EXPERIMENTAL
diff -ruN linux-2.6.2-rc3/drivers/i2c/chips/Makefile linux-2.6.2-rc3-k2/drivers/i2c/chips/Makefile
--- linux-2.6.2-rc3/drivers/i2c/chips/Makefile	Sat Jan 31 19:16:29 2004
+++ linux-2.6.2-rc3-k2/drivers/i2c/chips/Makefile	Sun Feb  1 14:20:58 2004
@@ -8,6 +8,7 @@
 
 obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
+obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
 obj-$(CONFIG_SENSORS_LM78)	+= lm78.o
diff -ruN linux-2.6.2-rc3/drivers/i2c/chips/fscher.c linux-2.6.2-rc3-k2/drivers/i2c/chips/fscher.c
--- linux-2.6.2-rc3/drivers/i2c/chips/fscher.c	Thu Jan  1 01:00:00 1970
+++ linux-2.6.2-rc3-k2/drivers/i2c/chips/fscher.c	Tue Feb  3 18:52:21 2004
@@ -0,0 +1,687 @@
+/*
+ * fscher.c - Part of lm_sensors, Linux kernel modules for hardware
+ * monitoring
+ * Copyright (C) 2003, 2004 Reinhard Nissl <rnissl@gmx.de>
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* 
+ *  fujitsu siemens hermes chip, 
+ *  module based on fscpos.c 
+ *  Copyright (C) 2000 Hermann Jung <hej@odn.de>
+ *  Copyright (C) 1998, 1999 Frodo Looijaard <frodol@dds.nl>
+ *  and Philip Edelbrock <phil@netroedge.com>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+
+/*
+ * Addresses to scan
+ */
+
+static unsigned short normal_i2c[] = { 0x73, I2C_CLIENT_END };
+static unsigned short normal_i2c_range[] = { I2C_CLIENT_END };
+static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
+static unsigned int normal_isa_range[] = { I2C_CLIENT_ISA_END };
+
+/*
+ * Insmod parameters
+ */
+
+SENSORS_INSMOD_1(fscher);
+
+/*
+ * The FSCHER registers
+ */
+
+/* chip identification */
+#define FSCHER_REG_IDENT_0		0x00
+#define FSCHER_REG_IDENT_1		0x01
+#define FSCHER_REG_IDENT_2		0x02
+#define FSCHER_REG_REVISION		0x03
+
+/* global control and status */
+#define FSCHER_REG_EVENT_STATE		0x04
+#define FSCHER_REG_CONTROL		0x05
+
+/* watchdog */
+#define FSCHER_REG_WDOG_PRESET		0x28
+#define FSCHER_REG_WDOG_STATE		0x23
+#define FSCHER_REG_WDOG_CONTROL		0x21
+
+/* fan 0 */
+#define FSCHER_REG_FAN0_MIN		0x55
+#define FSCHER_REG_FAN0_ACT		0x0e
+#define FSCHER_REG_FAN0_STATE		0x0d
+#define FSCHER_REG_FAN0_RIPPLE		0x0f
+
+/* fan 1 */
+#define FSCHER_REG_FAN1_MIN		0x65
+#define FSCHER_REG_FAN1_ACT		0x6b
+#define FSCHER_REG_FAN1_STATE		0x62
+#define FSCHER_REG_FAN1_RIPPLE		0x6f
+
+/* fan 2 */
+#define FSCHER_REG_FAN2_MIN		0xb5
+#define FSCHER_REG_FAN2_ACT		0xbb
+#define FSCHER_REG_FAN2_STATE		0xb2
+#define FSCHER_REG_FAN2_RIPPLE		0xbf
+
+/* voltage supervision */
+#define FSCHER_REG_VOLT_12		0x45
+#define FSCHER_REG_VOLT_5		0x42
+#define FSCHER_REG_VOLT_BATT		0x48
+
+/* temperature 0 */
+#define FSCHER_REG_TEMP0_ACT		0x64
+#define FSCHER_REG_TEMP0_STATE		0x71
+
+/* temperature 1 */
+#define FSCHER_REG_TEMP1_ACT		0x32
+#define FSCHER_REG_TEMP1_STATE		0x81
+
+/* temperature 2 */
+#define FSCHER_REG_TEMP2_ACT		0x35
+#define FSCHER_REG_TEMP2_STATE		0x91
+
+/*
+ * Functions declaration
+ */
+
+static int fscher_attach_adapter(struct i2c_adapter *adapter);
+static int fscher_detect(struct i2c_adapter *adapter, int address, int kind);
+static int fscher_detach_client(struct i2c_client *client);
+static void fscher_update_client(struct i2c_client *client);
+
+
+static int fscher_read_value(struct i2c_client *client, u8 register);
+static int fscher_write_value(struct i2c_client *client, u8 register,
+			      u8 value);
+static void fscher_init_client(struct i2c_client *client);
+
+/*
+ * Driver data (common to all clients)
+ */
+ 
+static struct i2c_driver fscher_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "fscher",
+	.id		= I2C_DRIVERID_FSCHER,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= fscher_attach_adapter,
+	.detach_client	= fscher_detach_client,
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+
+struct fscher_data {
+	struct semaphore update_lock;
+	char valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	u8 revision;		/* revision of chip */
+	u8 global_event;	/* global event status */
+	u8 global_control;	/* global control register */
+	u8 watchdog[3];		/* watchdog */
+	u8 volt[3];		/* 12, 5, battery voltage */ 
+	u8 temp_act[3];		/* temperature */
+	u8 temp_status[3];	/* status of sensor */
+	u8 fan_act[3];		/* fans revolutions per second */
+	u8 fan_status[3];	/* fan status */
+	u8 fan_min[3];		/* fan min value for rps */
+	u8 fan_ripple[3];	/* divider for rps */
+};
+
+/*
+ * Internal variables
+ */
+
+static int fscher_id = 0;
+
+/*
+ * Sysfs stuff
+ */
+
+#define sysfs_r(kind, offset, reg) \
+static ssize_t show_##kind (struct fscher_data *, char *, int); \
+static ssize_t show_##kind##offset (struct device *, char *); \
+static ssize_t show_##kind##offset (struct device *dev, char *buf) \
+{ \
+	struct i2c_client *client = to_i2c_client(dev); \
+	struct fscher_data *data = i2c_get_clientdata(client); \
+	fscher_update_client(client); \
+	return show_##kind(data, buf, (offset)); \
+}
+
+#define sysfs_w(kind, offset, reg) \
+static ssize_t set_##kind (struct i2c_client *, struct fscher_data *, const char *, size_t, int, int); \
+static ssize_t set_##kind##offset (struct device *, const char *, size_t); \
+static ssize_t set_##kind##offset (struct device *dev, const char *buf, size_t count) \
+{ \
+	struct i2c_client *client = to_i2c_client(dev); \
+	struct fscher_data *data = i2c_get_clientdata(client); \
+	return set_##kind(client, data, buf, count, (offset), reg); \
+}
+
+#define sysfs_rw_n(kind, offset, reg) \
+sysfs_r(kind, offset, reg) \
+sysfs_w(kind, offset, reg) \
+static DEVICE_ATTR(kind##offset, S_IRUGO | S_IWUSR, show_##kind##offset, set_##kind##offset);
+
+#define sysfs_rw(kind, reg) \
+sysfs_r(kind, 0, reg) \
+sysfs_w(kind, 0, reg) \
+static DEVICE_ATTR(kind, S_IRUGO | S_IWUSR, show_##kind##0, set_##kind##0);
+
+#define sysfs_ro_n(kind, offset, reg) \
+sysfs_r(kind, offset, reg) \
+static DEVICE_ATTR(kind##offset, S_IRUGO, show_##kind##offset, NULL);
+
+#define sysfs_ro(kind, reg) \
+sysfs_r(kind, 0, reg) \
+static DEVICE_ATTR(kind, S_IRUGO, show_##kind##0, NULL);
+
+#define sysfs_fan(offset, reg_status, reg_min, reg_ripple, reg_act) \
+sysfs_rw_n(pwm       , offset, reg_min) \
+sysfs_rw_n(fan_status, offset, reg_status) \
+sysfs_rw_n(fan_div   , offset, reg_ripple) \
+sysfs_ro_n(fan_input , offset, reg_act)
+
+#define sysfs_temp(offset, reg_status, reg_act) \
+sysfs_rw_n(temp_status, offset, reg_status) \
+sysfs_ro_n(temp_input , offset, reg_act)
+    
+#define sysfs_in(offset, reg_act) \
+sysfs_ro_n(in_input, offset, reg_act)
+
+#define sysfs_revision(reg_revision) \
+sysfs_ro(revision, reg_revision)
+
+#define sysfs_alarms(reg_events) \
+sysfs_ro(alarms, reg_events)
+
+#define sysfs_control(reg_control) \
+sysfs_rw(control, reg_control)
+
+#define sysfs_watchdog(reg_control, reg_status, reg_preset) \
+sysfs_rw(watchdog_control, reg_control) \
+sysfs_rw(watchdog_status , reg_status) \
+sysfs_rw(watchdog_preset , reg_preset)
+
+sysfs_fan(1, FSCHER_REG_FAN0_STATE, FSCHER_REG_FAN0_MIN,
+	     FSCHER_REG_FAN0_RIPPLE, FSCHER_REG_FAN0_ACT)
+sysfs_fan(2, FSCHER_REG_FAN1_STATE, FSCHER_REG_FAN1_MIN,
+	     FSCHER_REG_FAN1_RIPPLE, FSCHER_REG_FAN1_ACT)
+sysfs_fan(3, FSCHER_REG_FAN2_STATE, FSCHER_REG_FAN2_MIN,
+	     FSCHER_REG_FAN2_RIPPLE, FSCHER_REG_FAN2_ACT)
+
+sysfs_temp(1, FSCHER_REG_TEMP0_STATE, FSCHER_REG_TEMP0_ACT)
+sysfs_temp(2, FSCHER_REG_TEMP1_STATE, FSCHER_REG_TEMP1_ACT)
+sysfs_temp(3, FSCHER_REG_TEMP2_STATE, FSCHER_REG_TEMP2_ACT)
+
+sysfs_in(0, FSCHER_REG_VOLT_12)
+sysfs_in(1, FSCHER_REG_VOLT_5)
+sysfs_in(2, FSCHER_REG_VOLT_BATT)
+
+sysfs_revision(FSCHER_REG_REVISION)
+sysfs_alarms(FSCHER_REG_EVENTS)
+sysfs_control(FSCHER_REG_CONTROL)
+sysfs_watchdog(FSCHER_REG_WDOG_CONTROL, FSCHER_REG_WDOG_STATE, FSCHER_REG_WDOG_PRESET)
+  
+#define device_create_file_fan(client, offset) \
+do { \
+	device_create_file(&client->dev, &dev_attr_fan_status##offset); \
+	device_create_file(&client->dev, &dev_attr_pwm##offset); \
+	device_create_file(&client->dev, &dev_attr_fan_div##offset); \
+	device_create_file(&client->dev, &dev_attr_fan_input##offset); \
+} while (0)
+
+#define device_create_file_temp(client, offset) \
+do { \
+	device_create_file(&client->dev, &dev_attr_temp_status##offset); \
+	device_create_file(&client->dev, &dev_attr_temp_input##offset); \
+} while (0)
+
+#define device_create_file_in(client, offset) \
+do { \
+	device_create_file(&client->dev, &dev_attr_in_input##offset); \
+} while (0)
+
+#define device_create_file_revision(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_revision); \
+} while (0)
+
+#define device_create_file_alarms(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_alarms); \
+} while (0)
+
+#define device_create_file_control(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_control); \
+} while (0)
+
+#define device_create_file_watchdog(client) \
+do { \
+	device_create_file(&client->dev, &dev_attr_watchdog_status); \
+	device_create_file(&client->dev, &dev_attr_watchdog_control); \
+	device_create_file(&client->dev, &dev_attr_watchdog_preset); \
+} while (0)
+  
+/*
+ * Real code
+ */
+
+static int fscher_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_detect(adapter, &addr_data, fscher_detect);
+}
+
+static int fscher_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *new_client;
+	struct fscher_data *data;
+	int err = 0;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		goto exit;
+
+	/* OK. For now, we presume we have a valid client. We now create the
+	 * client structure, even though we cannot fill it completely yet.
+	 * But it allows us to access i2c_smbus_read_byte_data. */
+	if (!(new_client = kmalloc(sizeof(struct i2c_client) +
+	    sizeof(struct fscher_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+  	}
+	memset(new_client, 0x00, sizeof(struct i2c_client) +
+	       sizeof(struct fscher_data));
+
+	/* The Hermes-specific data is placed right after the common I2C
+	 * client data. */
+	data = (struct fscher_data *) (new_client + 1);
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &fscher_driver;
+	new_client->flags = 0;
+
+	/* Do the remaining detection unless force or force_fscher parameter */
+	if (kind < 0) {
+		if (i2c_smbus_read_byte_data(new_client,
+		    FSCHER_REG_IDENT_0) != 0x48		/* 'H' */
+		 || i2c_smbus_read_byte_data(new_client,
+		    FSCHER_REG_IDENT_1) != 0x45		/* 'E' */
+		 || i2c_smbus_read_byte_data(new_client,
+		    FSCHER_REG_IDENT_2) != 0x52)	/* 'R' */
+			goto exit_free;
+	}
+
+	/* Fill in the remaining client fields and put it into the
+	 * global list */
+	strlcpy(new_client->name, "fscher", I2C_NAME_SIZE);
+	new_client->id = fscher_id++;
+	data->valid = 0;
+	init_MUTEX(&data->update_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+
+	fscher_init_client(new_client);
+
+	/* Register sysfs hooks */
+	device_create_file_revision(new_client);
+	device_create_file_alarms(new_client);
+	device_create_file_control(new_client);
+	device_create_file_watchdog(new_client);
+
+	device_create_file_in(new_client, 0);
+	device_create_file_in(new_client, 1);
+	device_create_file_in(new_client, 2);
+
+	device_create_file_fan(new_client, 1);
+	device_create_file_fan(new_client, 2);
+	device_create_file_fan(new_client, 3);
+
+	device_create_file_temp(new_client, 1);
+	device_create_file_temp(new_client, 2);
+	device_create_file_temp(new_client, 3);
+
+	return 0;
+
+exit_free:
+	kfree(new_client);
+exit:
+	return err;
+}
+
+static int fscher_detach_client(struct i2c_client *client)
+{
+	int err;
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->adapter->dev,
+			"Client deregistration failed, client not detached.\n");
+		return err;
+	}
+
+	kfree(client);
+	return 0;
+}
+
+static int fscher_read_value(struct i2c_client *client, u8 reg)
+{
+	dev_dbg(&client->dev, "read reg 0x%02x\n", reg);
+
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fscher_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	dev_dbg(&client->dev, "write reg 0x%02x, val 0x%02x\n",
+		reg, value);
+
+	return i2c_smbus_write_byte_data(client, reg, value);
+}
+
+/* Called when we have found a new FSC Hermes. */
+static void fscher_init_client(struct i2c_client *client)
+{
+	struct fscher_data *data = i2c_get_clientdata(client);
+
+	/* Read revision from chip */
+	data->revision =  fscher_read_value(client, FSCHER_REG_REVISION);
+}
+
+static void fscher_update_client(struct i2c_client *client)
+{
+	struct fscher_data *data = i2c_get_clientdata(client);
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > 2 * HZ) ||
+	    (jiffies < data->last_updated) || !data->valid) {
+
+		dev_dbg(&client->dev, "Starting fscher update\n");
+
+		data->temp_act[0] = fscher_read_value(client, FSCHER_REG_TEMP0_ACT);
+		data->temp_act[1] = fscher_read_value(client, FSCHER_REG_TEMP1_ACT);
+		data->temp_act[2] = fscher_read_value(client, FSCHER_REG_TEMP2_ACT);
+		data->temp_status[0] = fscher_read_value(client, FSCHER_REG_TEMP0_STATE);
+		data->temp_status[1] = fscher_read_value(client, FSCHER_REG_TEMP1_STATE);
+		data->temp_status[2] = fscher_read_value(client, FSCHER_REG_TEMP2_STATE);
+
+		data->volt[0] = fscher_read_value(client, FSCHER_REG_VOLT_12);
+		data->volt[1] = fscher_read_value(client, FSCHER_REG_VOLT_5);
+		data->volt[2] = fscher_read_value(client, FSCHER_REG_VOLT_BATT);
+
+		data->fan_act[0] = fscher_read_value(client, FSCHER_REG_FAN0_ACT);
+		data->fan_act[1] = fscher_read_value(client, FSCHER_REG_FAN1_ACT);
+		data->fan_act[2] = fscher_read_value(client, FSCHER_REG_FAN2_ACT);
+		data->fan_status[0] = fscher_read_value(client, FSCHER_REG_FAN0_STATE);
+		data->fan_status[1] = fscher_read_value(client, FSCHER_REG_FAN1_STATE);
+		data->fan_status[2] = fscher_read_value(client, FSCHER_REG_FAN2_STATE);
+		data->fan_min[0] = fscher_read_value(client, FSCHER_REG_FAN0_MIN);
+		data->fan_min[1] = fscher_read_value(client, FSCHER_REG_FAN1_MIN);
+		data->fan_min[2] = fscher_read_value(client, FSCHER_REG_FAN2_MIN);
+		data->fan_ripple[0] = fscher_read_value(client, FSCHER_REG_FAN0_RIPPLE);
+		data->fan_ripple[1] = fscher_read_value(client, FSCHER_REG_FAN1_RIPPLE);
+		data->fan_ripple[2] = fscher_read_value(client, FSCHER_REG_FAN2_RIPPLE);
+
+		data->watchdog[0] = fscher_read_value(client, FSCHER_REG_WDOG_PRESET);
+		data->watchdog[1] = fscher_read_value(client, FSCHER_REG_WDOG_STATE);
+		data->watchdog[2] = fscher_read_value(client, FSCHER_REG_WDOG_CONTROL);
+
+		data->global_event = fscher_read_value(client, FSCHER_REG_EVENT_STATE);
+
+		data->last_updated = jiffies;
+		data->valid = 1;                 
+	}
+
+	up(&data->update_lock);
+}
+
+
+
+#define FAN_INDEX_FROM_NUM(nr)	((nr) - 1)
+
+static ssize_t set_fan_status(struct i2c_client *client, struct fscher_data *data,
+			      const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 0..1, 3..7 reserved => mask with 0x04 */  
+	unsigned long v = simple_strtoul(buf, NULL, 10) & 0x04;
+	data->fan_status[FAN_INDEX_FROM_NUM(nr)] &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_fan_status(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 0..1, 3..7 reserved => mask with 0x04 */  
+	return sprintf(buf, "%u\n", data->fan_status[FAN_INDEX_FROM_NUM(nr)] & 0x04);
+}
+
+static ssize_t set_pwm(struct i2c_client *client, struct fscher_data *data,
+		       const char *buf, size_t count, int nr, int reg)
+{
+	data->fan_min[FAN_INDEX_FROM_NUM(nr)] = simple_strtoul(buf, NULL, 10) & 0xff;
+
+	fscher_write_value(client, reg, data->fan_min[FAN_INDEX_FROM_NUM(nr)]);
+	return count;
+}
+
+static ssize_t show_pwm (struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", data->fan_min[FAN_INDEX_FROM_NUM(nr)]);
+}
+
+static ssize_t set_fan_div(struct i2c_client *client, struct fscher_data *data,
+			   const char *buf, size_t count, int nr, int reg)
+{
+	/* supported values: 2, 4, 8 */
+	unsigned long v = simple_strtoul(buf, NULL, 10);
+
+	switch (v) {
+	case 2: v = 1; break;
+	case 4: v = 2; break;
+	case 8: v = 3; break;
+	default:
+		dev_info(&client->dev, "fan_div value %ld not "
+			 "supported. Choose one of 2, 4 or 8!\n", v);
+		return -1;
+	}
+
+	/* bits 2..7 reserved => mask with 0x03 */
+	data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] &= ~0x03;
+	data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] |= v;
+
+	fscher_write_value(client, reg, data->fan_ripple[FAN_INDEX_FROM_NUM(nr)]);
+	return count;
+}
+
+static ssize_t show_fan_div(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 2..7 reserved => mask with 0x03 */  
+	return sprintf(buf, "%u\n", 1 << (data->fan_ripple[FAN_INDEX_FROM_NUM(nr)] & 0x03));
+}
+
+#define RPM_FROM_REG(val)	(val*60)
+
+static ssize_t show_fan_input (struct fscher_data *data, char *buf, int nr)
+{
+  return sprintf(buf, "%u\n", RPM_FROM_REG(data->fan_act[FAN_INDEX_FROM_NUM(nr)]));
+}
+
+
+
+#define TEMP_INDEX_FROM_NUM(nr)		((nr) - 1)
+
+static ssize_t set_temp_status(struct i2c_client *client, struct fscher_data *data,
+			       const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 2..7 reserved, 0 read only => mask with 0x02 */  
+	unsigned long v = simple_strtoul(buf, NULL, 10) & 0x02;
+	data->temp_status[TEMP_INDEX_FROM_NUM(nr)] &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_temp_status(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 2..7 reserved => mask with 0x03 */
+	return sprintf(buf, "%u\n", data->temp_status[TEMP_INDEX_FROM_NUM(nr)] & 0x03);
+}
+
+#define TEMP_FROM_REG(val)	(((val) - 128) * 1000)
+
+static ssize_t show_temp_input(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_act[TEMP_INDEX_FROM_NUM(nr)]));
+}
+
+
+
+/*
+ * The final conversion is specified in sensors.conf, as it depends on
+ * mainboard specific values. We export the registers contents as
+ * pseudo-hundredths-of-Volts (range 0V - 2.55V). Not that it makes much
+ * sense per se, but it minimizes the conversions count and keeps the
+ * values within a usual range.
+ */
+#define VOLT_FROM_REG(val)	((val) * 10)
+
+static ssize_t show_in_input(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[nr]));
+}
+
+
+
+static ssize_t show_revision(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", data->revision);
+}
+
+
+
+static ssize_t show_alarms(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 2, 5..6 reserved => mask with 0x9b */
+	return sprintf(buf, "%u\n", data->global_event & 0x9b);
+}
+
+
+
+static ssize_t set_control(struct i2c_client *client, struct fscher_data *data,
+			   const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 1..7 reserved => mask with 0x01 */  
+	unsigned long v = simple_strtoul(buf, NULL, 10) & 0x01;
+	data->global_control &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_control(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 1..7 reserved => mask with 0x01 */
+	return sprintf(buf, "%u\n", data->global_control & 0x01);
+}
+
+
+
+static ssize_t set_watchdog_control(struct i2c_client *client, struct
+				    fscher_data *data, const char *buf, size_t count,
+				    int nr, int reg)
+{
+	/* bits 0..3 reserved => mask with 0xf0 */  
+	unsigned long v = simple_strtoul(buf, NULL, 10) & 0xf0;
+	data->watchdog[2] &= ~0xf0;
+	data->watchdog[2] |= v;
+
+	fscher_write_value(client, reg, data->watchdog[2]);
+	return count;
+}
+
+static ssize_t show_watchdog_control(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 0..3 reserved, bit 5 write only => mask with 0xd0 */
+	return sprintf(buf, "%u\n", data->watchdog[2] & 0xd0);
+}
+
+static ssize_t set_watchdog_status(struct i2c_client *client, struct fscher_data *data,
+				   const char *buf, size_t count, int nr, int reg)
+{
+	/* bits 0, 2..7 reserved => mask with 0x02 */  
+	unsigned long v = simple_strtoul(buf, NULL, 10) & 0x02;
+	data->watchdog[1] &= ~v;
+
+	fscher_write_value(client, reg, v);
+	return count;
+}
+
+static ssize_t show_watchdog_status(struct fscher_data *data, char *buf, int nr)
+{
+	/* bits 0, 2..7 reserved => mask with 0x02 */
+	return sprintf(buf, "%u\n", data->watchdog[1] & 0x02);
+}
+
+static ssize_t set_watchdog_preset(struct i2c_client *client, struct fscher_data *data,
+				   const char *buf, size_t count, int nr, int reg)
+{
+	data->watchdog[0] = simple_strtoul(buf, NULL, 10) & 0xff;
+
+	fscher_write_value(client, reg, data->watchdog[0]);
+	return count;
+}
+
+static ssize_t show_watchdog_preset(struct fscher_data *data, char *buf, int nr)
+{
+	return sprintf(buf, "%u\n", data->watchdog[0]);
+}
+
+
+
+static int __init sensors_fscher_init(void)
+{
+	return i2c_add_driver(&fscher_driver);
+}
+
+static void __exit sensors_fscher_exit(void)
+{
+	i2c_del_driver(&fscher_driver);
+}
+
+MODULE_AUTHOR("Reinhard Nissl <rnissl@gmx.de> based on work from Hermann"
+	      " Jung <hej@odn.de>, Frodo Looijaard <frodol@dds.nl> and"
+	      " Philip Edelbrock <phil@netroedge.com>");
+MODULE_DESCRIPTION("FSC Hermes driver");
+MODULE_LICENSE("GPL");
+
+module_init(sensors_fscher_init);
+module_exit(sensors_fscher_exit);

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Looks better now, but there are still problems with the fans:
> 
> video:~ # sensors
> fscher-i2c-0-73
> Adapter: SMBus I801 adapter at 2000
> Temp1/CPU:      +52.00 C
> Temp2/MB:       +44.00 C
> Temp3/AUX:      +47.00 C
> ERROR: Can't get FAN1 data!
> ERROR: Can't get FAN2 data!
> ERROR: Can't get FAN3 data!
> +12V:           +11.79 V
> +5V:             +5.05 V
> Battery:         +3.05 V

This is due to the fan_min vs pwm issue. The way it should be fixed
depends on what the driver will do. See my other reply for more.

> BTW: is it intended, that fscher is found at i2c-1-73 after reloading 
> i2c_i801, and at i2c-2-73, ... after reloading it again, ...?

Not really intended, but known. I don't like it much but I think I read
it couldn't be easily fixed. We'll have to cope with that for some times
at least.

> BTW2: your latest diff didn't contain FSCHER's DRIVER_ID.

Correct. It's not in the same directory so I forgot it. Anyway, my goal
was to send the new version of the driver itself as a base for future
changes. It wasn't meant as a definitive version. That said, thanks for
reminding me that the ID needs to be added, so that I will hopefully not
forget about it next time.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Attached you'll find the changes for kernel 2.4.x. Updated
> documentation will follow later.

I commited your changes, with almost no changes, except rewording in
your sensors.conf comment, and mappings in the library. In your original
patch, the mappings for pwm symbols were broken (pointing to themselves,
which isn't correct).

Please check that the commited version works OK, and provide a
documentation update that reflects these changes.

I will send the 2.6 kernel driver to Greg KH in a minute.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>I can't speak for Scylla, as I don't own it's specs.
> 
> Oh, OK. Ignore my question in the other mail then, please.

Well, I could ask my contact person at FSC, but he is on vaccation till mid of 
February. Would you like a copy of the Poseidon spec too?

>>>I don't quite agree with you. They documented the limitation, this
>>>sounds like a piece of advice to driver programers.
>>
>>I don't think so. SystemGuard doesn't even give a warning, when one
>>allows the fans to turn off.
> 
> OK, as you feel then. After all, you're the first user of your driver,
> so I can assume you know what you're doing.

The BIOS sets reasonable values and the default sensors.conf doesn't override 
them. Maybe a "WARNING: read documentation or you may damage your system!" 
might be useful to add to the configuration file, heading the supplied sample 
settings.

>>>>This means renaming the procfs files to pwm, doesn't it?
>>
>>Nonsense! procfs has all information in one file, so there is nothing
>>to rename.
> 
> No. All information *was* in one file. pwm should now be moved to a
> separate file, with a single value.

Now, I've got the point :-))

>>>>I don't think, that it would be good idea to rename every fan_min to
>>>>pwm, as there would then no longer be a relationship to the
>>>>documenation.
>>>
>>>Well, in this case, rewrite the documentation as well. Doesn't sound
>>
>>I ment the specification from FSC. But you are right: I still have to
>>update the documentation for sysfs.
> 
> Ideally, you should first update the doc file as found in our package,
> then submit a modified version (with sysfs information instead of
> procfs) for inclusion into Linux 2.6. I think these have to be two
> different files (although with a common part) since they document two
> different versions of the driver that live in different places.

Ok.

>>If I got anything wrong, please point me to the locations, where I
>>need to make changes.
> 
> All you have to do is split pwm into a different file in the 2.4 driver,
> and update the rest of the driver accordingly. This will require changes
> to be made to libsensors, and possibly sensors too, and I'll do them.
> 
> This may sound like much work for nothing, and I agree we could do
> without it. But since we're at cleaning the driver, let's do it
> completely.

I don't mind to work on this. It was just not clear to me, what I should 
change in 2.4.x. But now it is :-)

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Attached, you'll find some minor changes, because it's bit 2 that
> signals a fan fault, and the fan labels where assigned in the wrong
> order.

Correct. I've commited these fixes.

What about Scylla's fan fault mask? The driver masks it with 0x0f, this
means four candidate bits. On the one hand I don't see why this wouldn't
be the same bit as for Poseidon and Hermes. On the other hand, without a
data sheet I can hardly conclude. If you have information, please share.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Silly me. I've just copied it from FSCPOS, but didn't check it. I had
> a look into Poseidon's data sheet and cannot find a difference to
> Hermes'. You are right, that this code doesn't make sense and is
> wrong.

Indeed, Poseidon and Scylla do the same, meaningless comparison. I
wonder how this could survive unseen until today. I will clean it up
right now.

> Well, the BIOS sets reasonable values at boot time, but I wouldn't
> have the driver block writing lower values to the registers. If FSC
> had concerns about the values written to the registers, they simply
> wouldn't have documented them(see my last email about the
> sensor-fan-matrix).

I don't quite agree with you. They documented the limitation, this
sounds like a piece of advice to driver programers.

> > 1* Clean up sensors code so that it doesn't reference fan_min. For
> > one thing, it'll let us get rid of the errors you had when running
> > "sensors". (...)

I'll do this, as mentioned above.

> > 2* Either rename fan_min in libsensors to pwm, or even remove it
> > completely. Depends if you want to keep it or not. If it is removed,
> > update sensors.conf accordingly.
> 
> I would like to keep it as pwm.

OK, let's go this way. I'll do that as soon as you have submitted a
change to the 2.4 driver.

> > 3* If you want to keep the pwm stuff, rename it in the 2.4 driver as
> > well.
> 
> This means renaming the procfs files to pwm, doesn't it?
> 
> I don't think, that it would be good idea to rename every fan_min to
> pwm, as there would then no longer be a relationship to the
> documenation.

Well, in this case, rewrite the documentation as well. Doesn't sound
very difficult. My reason for suggesting that change is that tweaking
libsensors is much easier if the 2.4 and 2.6 driver are similar. And
anyway, I don't much like calling fan_min something that is not in RPM
unit. See the confusion it has caused to me already ;') and in sensors
as well.

> > 4* If you don't want to keep the pwm stuff, remove it from the 2.6
> > driver.
> 
> So, this one is already up to date.

Yes, shouldn't need to be changed.

> > Basically everything is possible, keeping it in the driver only,
> > driver+library, driver+library+sensors.conf or
> > driver+library+sensors.conf+sensors. Depends on how useful the
> > feature is to you.
> 
> It is imported to me. I've choosen this board, because of its
> possiblity to control all fans very precisely. I use it in my video
> disk recorder, to have a silent PC for watching TV. But it can also be
> powerful (and loud), e. g. for converting recordings. When watching
> TV, the system still stays cool enough to have all fans run at minimum
> speed, where the used VERAX fans are almost not noticeable.

Nice :) So let's keep that functionality in the 2.6 driver, and fix it
everywhere else it is needed.

> Thank you very much for your assistance. If Hermes is ported properly,
> it can be the base for porting Poseidon's driver. I already have
> contact to a person, that is willing to port it.

Interesting how users of old rare chips are surfacing these days, first
with the gl518sm, now with fscpos... Indeed Poseidon seems to be very
similar to Hermes (while Scylla seems more complex, or at least bigger,
to me).

Thanks for helping in the porting process. This is much work and we
couldn't do it without the help of people who actually have the chip and
can test the new driver.


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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> > > 1* Clean up sensors code so that it doesn't reference fan_min. For
> > > one thing, it'll let us get rid of the errors you had when running
> > > "sensors". (...)
> 
> I'll do this, as mentioned above.

Done and commited to CVS. Please checkout and test. Output with 2.4
driver should be unchanged, and output with 2.6 driver should now be
fine now.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

> All you have to do is split pwm into a different file in the 2.4 driver,
> and update the rest of the driver accordingly. This will require changes
> to be made to libsensors, and possibly sensors too, and I'll do them.

Attached you'll find the changes for kernel 2.4.x. Updated documentation will 
follow later.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm_sensors2.diff.gz
Type: application/x-gzip
Size: 2558 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040206/f44e835d/lm_sensors2.diff.gz

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>>>1* Clean up sensors code so that it doesn't reference fan_min. For
>>>>one thing, it'll let us get rid of the errors you had when running
>>>>"sensors". (...)
>>
>>I'll do this, as mentioned above.
> 
> Done and commited to CVS. Please checkout and test. Output with 2.4
> driver should be unchanged, and output with 2.6 driver should now be
> fine now.

Yes, thank you very much for fixing it.

Attached, you'll find some minor changes, because it's bit 2 that signals a 
fan fault, and the fan labels where assigned in the wrong order.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de
-------------- next part --------------
--- ./etc/sensors.conf.eg.orig	2004-02-05 17:53:05.000000000 +0100
+++ ./etc/sensors.conf.eg	2004-02-05 18:57:04.640782410 +0100
@@ -1586,9 +1586,9 @@
     label temp3       "Temp3/AUX"
 
 # Fans
-    label fan1        "Fan1/CPU"
-    label fan2        "Fan2/AUX"
-    label fan3        "Fan3/PS"
+    label fan1        "Fan1/PS"
+    label fan2        "Fan2/CPU"
+    label fan3        "Fan3/AUX"
 
 # Voltage
     label in0         "+12V"
-------------- next part --------------
--- ./prog/sensors/chips.c.orig	2004-02-05 17:54:04.000000000 +0100
+++ ./prog/sensors/chips.c	2004-02-05 18:44:18.197884435 +0100
@@ -3243,7 +3243,7 @@
       !sensors_get_feature(*name,SENSORS_FSCPOS_FAN1_STATE,&state)) { 
     if (valid) {
       print_label(label,10);
-	if((int) state & 0x02)
+	if((int) state & 0x04)
 	  printf("\tfaulty\n");
 	else
 	  printf("\t%6.0f RPM \n",fan);
@@ -3257,7 +3257,7 @@
       !sensors_get_feature(*name,SENSORS_FSCPOS_FAN2_STATE,&state)) { 
     if (valid) {
       print_label(label,10);
-	if((int) state & 0x02)
+	if((int) state & 0x04)
 	  printf("\tfaulty\n");
 	else
 	  printf("\t%6.0f RPM \n",fan);
@@ -3271,7 +3271,7 @@
       !sensors_get_feature(*name,SENSORS_FSCPOS_FAN3_STATE,&state)) { 
     if (valid) {
       print_label(label,10);
-	if((int) state & 0x02)
+	if((int) state & 0x04)
 	  printf("\tfaulty\n");
 	else
 	  printf("\t%6.0f RPM \n",fan);
@@ -3557,7 +3557,7 @@
       && !sensors_get_feature(*name,SENSORS_FSCHER_FAN1_STATE,&state)) { 
     if (valid) {
       print_label(label,10);
-      if((int) state & 0x02)
+      if((int) state & 0x04)
         printf("\tfaulty\n");
       else
         printf("\t%6.0f RPM \n",fan);
@@ -3571,7 +3571,7 @@
       && !sensors_get_feature(*name,SENSORS_FSCHER_FAN2_STATE,&state)) { 
     if (valid) {
       print_label(label,10);
-      if((int) state & 0x02)
+      if((int) state & 0x04)
         printf("\tfaulty\n");
       else
         printf("\t%6.0f RPM \n",fan);
@@ -3585,7 +3585,7 @@
       && !sensors_get_feature(*name,SENSORS_FSCHER_FAN3_STATE,&state)) { 
     if (valid) {
       print_label(label,10);
-      if((int) state & 0x02)
+      if((int) state & 0x04)
         printf("\tfaulty\n");
       else
         printf("\t%6.0f RPM \n",fan);

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Well, none of the two file types (pwm/fan_min) is perfekt for that
>>case. It would be necessary to have something like 'pwm_min'!
> 
> If we have to choose, this is more like pwm, at least because of the
> unit. What made me think wrong was the fact that it was named fan_min in
> the 2.4 driver, and that code in sensors/chips.c:
> 
>       && !sensors_get_feature(*name,SENSORS_FSCHER_FAN1,&fan)
>       && !sensors_get_feature(*name,SENSORS_FSCHER_FAN1_MIN,&min_rpm)
> (...)
>       else if (fan < min_rpm)
>         printf("\t%6.0f RPM (not present or faulty)\n",fan);
>       else
>         printf("\t%6.0f RPM \n",fan);
> 
> I just can't make any sense out of this comparison. Units don't even
> match. Am I missing something or is it plain wrong?

Silly me. I've just copied it from FSCPOS, but didn't check it. I had a look
into Poseidon's data sheet and cannot find a difference to Hermes'. You are
right, that this code doesn't make sense and is wrong.

> I read in the Hermes data sheet that FSC doesn't recommend to set the
> pwm value lower than what it was set to by the BIOS. Since I don't think
> anyone would want to set it higher (what would be the point? it is
> supposed to go higher by itself on high load) I wonder if it is even
> worth handling it in the driver. If we do, one possible safety would be
> to store the registers value as the driver is loaded and not to allow
> subsequent sets to set it lower than that.

Well, the BIOS sets reasonable values at boot time, but I wouldn't have the
driver block writing lower values to the registers. If FSC had concerns about
the values written to the registers, they simply wouldn't have documented them
(see my last email about the sensor-fan-matrix).

> Here is the plan I propose:
> 
> 1* Clean up sensors code so that it doesn't reference fan_min. For one
> thing, it'll let us get rid of the errors you had when running
> "sensors". Or at least use the value the way it should be (doesn't seem
> to be the case as of now).
>
> 2* Either rename fan_min in libsensors to pwm, or even remove it
> completely. Depends if you want to keep it or not. If it is removed,
> update sensors.conf accordingly.

I would like to keep it as pwm.

> 3* If you want to keep the pwm stuff, rename it in the 2.4 driver as
> well.

This means renaming the procfs files to pwm, doesn't it?

I don't think, that it would be good idea to rename every fan_min to pwm, as 
there would then no longer be a relationship to the documenation.

> 4* If you don't want to keep the pwm stuff, remove it from the 2.6
> driver.

So, this one is already up to date.

> Usually, fan speeds are controled by scripts, not sensors. In this case
> it is a bit different since the system can make fans go higher by
> itself, so no closed-loop script is needed.

That's fine with me. Usually, a script should be enough to set reasonable pwm
values depending to the sensor values.

> Basically everything is possible, keeping it in the driver only,
> driver+library, driver+library+sensors.conf or
> driver+library+sensors.conf+sensors. Depends on how useful the feature
> is to you.

It is imported to me. I've choosen this board, because of its possiblity to
control all fans very precisely. I use it in my video disk recorder, to have a
silent PC for watching TV. But it can also be powerful (and loud), e. g. for
converting recordings. When watching TV, the system still stays cool enough to 
have all fans run at minimum speed, where the used VERAX fans are almost not 
noticeable.

> I can handle points 1 and 2, while I would let you handle points 3 and
> 4. Just tell me what you think should be done so that I can fix things
> in the correct way.

Thank you very much for your assistance. If Hermes is ported properly, it can
be the base for porting Poseidon's driver. I already have contact to a person,
that is willing to port it.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> I can't speak for Scylla, as I don't own it's specs.

Oh, OK. Ignore my question in the other mail then, please.

> > I don't quite agree with you. They documented the limitation, this
> > sounds like a piece of advice to driver programers.
> 
> I don't think so. SystemGuard doesn't even give a warning, when one
> allows the fans to turn off.

OK, as you feel then. After all, you're the first user of your driver,
so I can assume you know what you're doing.

> >>This means renaming the procfs files to pwm, doesn't it?
> 
> Nonsense! procfs has all information in one file, so there is nothing
> to rename.

No. All information *was* in one file. pwm should now be moved to a
separate file, with a single value.

> >>I don't think, that it would be good idea to rename every fan_min to
> >>pwm, as there would then no longer be a relationship to the
> >>documenation.
> > 
> > Well, in this case, rewrite the documentation as well. Doesn't sound
> 
> I ment the specification from FSC. But you are right: I still have to
> update the documentation for sysfs.

Ideally, you should first update the doc file as found in our package,
then submit a modified version (with sysfs information instead of
procfs) for inclusion into Linux 2.6. I think these have to be two
different files (although with a common part) since they document two
different versions of the driver that live in different places.

BTW, we would need to do the same for other drivers as well. Much work
this is...

> If I got anything wrong, please point me to the locations, where I
> need to make changes.

All you have to do is split pwm into a different file in the 2.4 driver,
and update the rest of the driver accordingly. This will require changes
to be made to libsensors, and possibly sensors too, and I'll do them.

This may sound like much work for nothing, and I agree we could do
without it. But since we're at cleaning the driver, let's do it
completely.

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

Fujitsu Siemens sensor HERMES

[Reinhard Nissl]

Hi,

Jean Delvare wrote:

>>Silly me. I've just copied it from FSCPOS, but didn't check it. I had
>>a look into Poseidon's data sheet and cannot find a difference to
>>Hermes'. You are right, that this code doesn't make sense and is
>>wrong.
> 
> Indeed, Poseidon and Scylla do the same, meaningless comparison. I
> wonder how this could survive unseen until today. I will clean it up
> right now.

I can't speak for Scylla, as I don't own it's specs.

>>Well, the BIOS sets reasonable values at boot time, but I wouldn't
>>have the driver block writing lower values to the registers. If FSC
>>had concerns about the values written to the registers, they simply
>>wouldn't have documented them(see my last email about the
>>sensor-fan-matrix).
> 
> I don't quite agree with you. They documented the limitation, this
> sounds like a piece of advice to driver programers.

I don't think so. SystemGuard doesn't even give a warning, when one allows the 
fans to turn off.

>>>1* Clean up sensors code so that it doesn't reference fan_min. For
>>>one thing, it'll let us get rid of the errors you had when running
>>>"sensors". (...)
> 
> I'll do this, as mentioned above.
> 
>>>2* Either rename fan_min in libsensors to pwm, or even remove it
>>>completely. Depends if you want to keep it or not. If it is removed,
>>>update sensors.conf accordingly.
>>
>>I would like to keep it as pwm.
> 
> OK, let's go this way. I'll do that as soon as you have submitted a
> change to the 2.4 driver.
> 
>>>3* If you want to keep the pwm stuff, rename it in the 2.4 driver as
>>>well.
>>
>>This means renaming the procfs files to pwm, doesn't it?

Nonsense! procfs has all information in one file, so there is nothing to rename.

>>I don't think, that it would be good idea to rename every fan_min to
>>pwm, as there would then no longer be a relationship to the
>>documenation.
> 
> Well, in this case, rewrite the documentation as well. Doesn't sound

I ment the specification from FSC. But you are right: I still have to update 
the documentation for sysfs.

> very difficult. My reason for suggesting that change is that tweaking
> libsensors is much easier if the 2.4 and 2.6 driver are similar. And
> anyway, I don't much like calling fan_min something that is not in RPM
> unit. See the confusion it has caused to me already ;') and in sensors
> as well.

If I got anything wrong, please point me to the locations, where I need to 
make changes.

Bye.
-- 
Dipl.-Inform. (FH) Reinhard Nissl
mailto:rnissl@gmx.de

Fujitsu Siemens sensor HERMES

[Jean Delvare]

> Attached you'll find the changes for kernel 2.4.x. Updated
> documentation will follow later.

Great. I'll take a look. It's too late for lm_sensors 2.8.4, but I'll
commit it to CVS right after the release.

> Well, I could ask my contact person at FSC, but he is on vaccation
> till mid of February. Would you like a copy of the Poseidon spec
> too?

The more specs, the better, especially if we are to port these drivers
to Linux 2.6. See how many bugs we found already, there's no reason
there are not a few more hiding.

> The BIOS sets reasonable values and the default sensors.conf doesn't
> override them. Maybe a "WARNING: read documentation or you may damage
> your system!" might be useful to add to the configuration file, heading
> the supplied sample settings.

Yes, I is a good idea (already part of your patch as I can see). And you
even provided a patch for libsensors. Lovely :)

Right after 2.8.4 is released (which should happen really soon now),
I'll check your patch and commit it, then I'll review the 2.6 driver one
last time and send it to Greg KH for inclusion into Linux 2.6.3. One
more chip driver, yeehoo :)

Thanks a lot for the good work.

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

Fujitsu Siemens sensor HERMES

[Jean Delvare]

Hello Reinhard,

Congratulations, your driver is now part of Linux since version 2.6.3-rc2 :)

Good, good work, really.

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