fan speed for it87?? chips added

fan speed for it87?? chips added

[Michael Hufer]

Hi,
I recently bought a new computer system based on Shuttles XPC FN42G2 barebone 
its motherboard uses the it8705 chip. Unfortunatly, I had to find out that 
the current it87 driver resets the chip on init which switches the CPU/case 
fan to a very noisy full speed. So, after some grumpling :-) I decided to 
take things in my own hands and add some new features to the driver.

First, I added a new mod parameter "reset_it87" which controls whether or not 
the chip is reset on module load (modprobe it87 reset_it=1, default) or 
not(modprobe it87 reset_it87=0).
Since this worked out quiet nicely I decided to go one step further and also 
add the chips manual and automatic temperature controlled fan speed features 
(PWM etc.).

Finally I also updated the doc to document the new features.

Attached is a diff against the current cvs version.

So long,
	Micha.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: it87_pwm-cvs.diff
Type: text/x-diff
Size: 22967 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20030923/5293d2b5/it87_pwm-cvs.bin

fan speed for it87?? chips added

[Greg KH]

On Tue, Sep 23, 2003 at 08:35:29PM +0200, Michael Hufer wrote:
> Hi,
> I recently bought a new computer system based on Shuttles XPC FN42G2 barebone 
> its motherboard uses the it8705 chip. Unfortunatly, I had to find out that 
> the current it87 driver resets the chip on init which switches the CPU/case 
> fan to a very noisy full speed. So, after some grumpling :-) I decided to 
> take things in my own hands and add some new features to the driver.
> 
> First, I added a new mod parameter "reset_it87" which controls whether or not 
> the chip is reset on module load (modprobe it87 reset_it=1, default) or 
> not(modprobe it87 reset_it87=0).
> Since this worked out quiet nicely I decided to go one step further and also 
> add the chips manual and automatic temperature controlled fan speed features 
> (PWM etc.).
> 
> Finally I also updated the doc to document the new features.
> 
> Attached is a diff against the current cvs version.

Care to make a patch against the latest 2.6 kernel too?

thanks,

greg k-h

fan speed for it87?? chips added

[Jean Delvare]

> First, I added a new mod parameter "reset_it87" which controls whether
> or not the chip is reset on module load (modprobe it87 reset_it=1,
> default) or not(modprobe it87 reset_it87=0).
> Since this worked out quiet nicely I decided to go one step further
> and also add the chips manual and automatic temperature controlled fan
> speed features (PWM etc.).
> 
> Finally I also updated the doc to document the new features.
> 
> Attached is a diff against the current cvs version.

Not tested yet (when I say tested, I should say review, since I don't
have any it87 chip here) but I already like it. The "fan to a very noisy
full speed" was already reported once (Hi Joost!) and I was analysing
the phenomenon no later than one hour ago - just to come to the same
conclusion as yours: the reset thing is silly.

(quickly reading the patch)

Mmm, looks good, except one part in init_client_it87. Not only the
reset_it parameter prevents the reset, but it also disables all default
settings - so it acts like the "init=0" parameter some of our drivers
have. I think that only the reset command (writing 0x80 in register
0x00) is evil. The rest of the init isn't dangerous (also it has also
been discussed wether it should be done or not). Also, did you checked
your fan3_div code? Looks buggy.

Your patch looks overall great and I'm willing to apply it after the
points above are fixed and the questions below are discussed.

1* Should the reset command be kept at all? I really believe it should
go away. The chip is obviously initialized with correct values by the
BIOS, so the reset is unnecessary at least, and can break thinks
(experience showed it did). If we decide to keep it in the code, I'd at
least expect it not to be the default.

2* Should we provide a init=0 parameter? I think yes. Should it be the
default? Not too sure. (But actually, some things *need* to be
initialized, while some others do not need to. Especially, temperature
and voltage limits belong to userspace, as it has already been
discussed). Greg, what's your policy in 2.6 (if you have one)?

So, what do you mean?

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

fan speed for it87?? chips added

[Philip Pokorny]

I would recommend that you rename your 'reset_it87' option to 'init' to 
be consistent with the other drives.

Other drivers use 'init=0' to disable initialization of the chip limits.

:v)

Michael Hufer wrote:
> Hi,
> I recently bought a new computer system based on Shuttles XPC FN42G2 barebone 
> its motherboard uses the it8705 chip. Unfortunatly, I had to find out that 
> the current it87 driver resets the chip on init which switches the CPU/case 
> fan to a very noisy full speed. So, after some grumpling :-) I decided to 
> take things in my own hands and add some new features to the driver.
> 
> First, I added a new mod parameter "reset_it87" which controls whether or not 
> the chip is reset on module load (modprobe it87 reset_it=1, default) or 
> not(modprobe it87 reset_it87=0).
> Since this worked out quiet nicely I decided to go one step further and also 
> add the chips manual and automatic temperature controlled fan speed features 
> (PWM etc.).
> 
> Finally I also updated the doc to document the new features.
> 
> Attached is a diff against the current cvs version.
> 
> So long,
> 	Micha.


-- 
Philip Pokorny, Director of Engineering
Tel: 415-358-2635   Fax: 415-358-2646   Toll Free: 888-PENGUIN
PENGUIN COMPUTING, INC.
www.penguincomputing.com

fan speed for it87?? chips added

[Michael Hufer]

On Tuesday 23 September 2003 23:22, Jean Delvare wrote:
> > First, I added a new mod parameter "reset_it87" which controls whether
> Mmm, looks good, except one part in init_client_it87. Not only the
> reset_it parameter prevents the reset, but it also disables all default
> settings - so it acts like the "init=0" parameter some of our drivers
> have. I think that only the reset command (writing 0x80 in register
> 0x00) is evil. The rest of the init isn't dangerous (also it has also
> been discussed wether it should be done or not).
Actually, the 'reset_it87' parameter did only disable the chip reset in an 
early version. But it turned out that some of the init settings also affected 
something of the BIOS's pwm setup. I then extended the scope of the if(reset) 
to all/most of the initialization code. I never investigated exactly which 
register(-value) was responsible, thought. But of course, you are right the 
way it is currently used it should be renamed to 'init', 

> [...] Also, did you checked
> your fan3_div code? Looks buggy.

It might look like that, but its doing exactly what I wanted it to do. ;-)
The reason: 
While the tech docu of the it87xx chips says that the fan divisor 3 is fixed 
to 2, it seems that the CPU fan connected as fan3 needs a 8 as divider to 
report the correct speed as reported by the BIOS Setup and MBM on my Windows 
dual install. The BIOS obviously should know how to calculate the fan speed 
from the register value, I don't know how MBM gets the same value, thought. 
When I use the default divisor of 2 on the register value I get the 
unreasonable high fan speed of ~8000rpm when it is running in low speed mode 
and >~140000rpm on full speed!
Thus, I made this divisor changeable, too. There might be a better, cleaner 
way to do this, thought. Suggestions are welcome.

> Your patch looks overall great and I'm willing to apply it after the
> points above are fixed and the questions below are discussed.
>
> 1* Should the reset command be kept at all? I really believe it should
> go away. The chip is obviously initialized with correct values by the
> BIOS, so the reset is unnecessary at least, and can break thinks
> (experience showed it did). If we decide to keep it in the code, I'd at
> least expect it not to be the default.

It could be made default not to reset the chip but as there might be a 
BIOS/motherboard out there that does not correctly initialize the chip, there 
should be a way to reset the chip on module load.

Cheers,
	Micha.

fan speed for it87?? chips added

[Greg KH]

On Tue, Sep 23, 2003 at 11:22:36PM +0200, Jean Delvare wrote:
> 
> 2* Should we provide a init=0 parameter? I think yes. Should it be the
> default? Not too sure. (But actually, some things *need* to be
> initialized, while some others do not need to. Especially, temperature
> and voltage limits belong to userspace, as it has already been
> discussed). Greg, what's your policy in 2.6 (if you have one)?

I don't have one, yet.  Haven't thought that much about it.  Any good
proposals for one?

thanks,

greg k-h

fan speed for it87?? chips added

[Michael Hufer]

Hi there,
here is a new diff agains current cvs.
I added separate 'init' and 'reset_it87' module parameter. 'reset_it87' 
(default=0, disable chip reset) only enable/disable the chip reset (writing 
0x80 into register 0x00). 'init' (default=1, enable init) enable/disable 
(re-)initialization of the register values.

I also addressed your concerns regarding the fan_div3 code.

More comments?


	Micha.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: it87_pwm-cvs.diff
Type: text/x-diff
Size: 22964 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20030924/f070424e/it87_pwm-cvs.bin

fan speed for it87?? chips added

[Jean Delvare]

> Actually, the 'reset_it87' parameter did only disable the chip reset
> in an early version. But it turned out that some of the init settings
> also affected something of the BIOS's pwm setup. I then extended the
> scope of the if(reset) to all/most of the initialization code. I never
> investigated exactly which register(-value) was responsible, thought.
> But of course, you are right the way it is currently used it should be
> renamed to 'init'.

I'd really like you to investigate and find out which other settings
were affecting the PWM setup. I don't want to remove all initialization
code because we don't know which part of it causes problems.

> > [...] Also, did you checked
> > your fan3_div code? Looks buggy.
> 
> It might look like that, but its doing exactly what I wanted it to do.

C'mon, you can't be serious. Quoting your patch:

+	   This sets fan-divs to 2, among others.

Where does it do that? Can't find it.

+	data->fan_div[2] = 1;

Why does fan_div[2] need special init the other fan_div[] don't require?

-		data->fan_div[2] = 1;
+		data->fan_div[2] = data->fan_div[2];

No comment.

+		if (*nrels_mag >= 3) {
+			data->fan_div[1] = DIV_TO_REG(results[2]);
+		}

This must be fan_div[2] there.

So this just can't work the way you want, unless you wanted it broken
from the start ;) Understand me, I don't want to blame you, I'm
particularly happy that you wrote that patch. But it obviously requires
more reviewing and testing before we can commit it to our CVS
repository.

> ;-) The reason: 
> While the tech docu of the it87xx chips says that the fan divisor 3 is
> fixed to 2, it seems that the CPU fan connected as fan3 needs a 8 as
> divider to report the correct speed as reported by the BIOS Setup and
> MBM on my Windows dual install. The BIOS obviously should know how to
> calculate the fan speed from the register value, I don't know how MBM
> gets the same value, thought. When I use the default divisor of 2 on
> the register value I get the unreasonable high fan speed of ~8000rpm
> when it is running in low speed mode and >~140000rpm on full speed!
> Thus, I made this divisor changeable, too. There might be a better,
> cleaner way to do this, thought. Suggestions are welcome.

The datasheet actually says that fan_div3 can be set to either 2 or 8.
It's controled by bit 6 of register 0x0b. It differs from the other two
divisors in that it is coded on a single bit while the others have
three, which let them chose between 8 different divisors from 1 to 128.
You could as well consider that the bit 6 of register 0x0b controls the
*second* bit of fan_div3, which MSB and LSB are fixed to 0 and 1,
respectively. This make div3 look more like div1 and 2 (to me at least,
and should help in the code too).

BTW,

-#define DIV_TO_REG(val) ((val)=8?3:(val)=4?2:(val)=1?0:1)
+extern inline u8 DIV_TO_REG(long val)
+{
+	if( val = 1 )
+		return 0;
+	u8 i;
+	for( i = 1; i < 7; i++ )
+	{
+		if( val = 1<<i )
+			return i;
+	}
+	return 7;
+}

(BTW, does this really compile? I thought you couldn't declare a
variable after real code in a given block.)

You don't need a special case for 1, unless I'm missing something. And
you definitely want to return 1 as the default value, as it was the case
before, because fan_div = 2 is a reasonable default. I also believe that
we should try to find out the nearest divisor from what the user
entered. Setting the divisor to 128, or either 2 , when the user asked
for 31, is poor. We can do better than that. What about that:

extern inline u8 DIV_TO_REG(long val)
{
	u8 i;
	for( i = 0; i < 7; i++ )
		if( val>>i = 1 )
			return i;
	return 2;
}

This considers the highest weighted bit, whatever the lower bits are set
to.

> It could be made default not to reset the chip but as there might be
> a BIOS/motherboard out there that does not correctly initialize the
> chip, there should be a way to reset the chip on module load.

So far, the BIOS seems to have been more clever than we were ;)

I want the chip reset (0x80 at 0x00) gone, and everything else that
causes trouble with it. The rest will be kept, inside an init=1
conditional. Is it OK?

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

fan speed for it87?? chips added

[Jean Delvare]

> > 2* Should we provide a init=0 parameter? I think yes. Should it be
> > the default? Not too sure. (But actually, some things *need* to be
> > initialized, while some others do not need to. Especially,
> > temperature and voltage limits belong to userspace, as it has
> > already been discussed). Greg, what's your policy in 2.6 (if you
> > have one)?
> 
> I don't have one, yet.  Haven't thought that much about it.  Any good
> proposals for one?

So far, most of our drivers are reinitializing chipsets and setting
limits. Some drivers have the init=0 module parameter to prevent that.
But it has been discussed recently that init=0 should be the default, or
even that most of the initialization (limits) could go away from the
driver code since it belongs to userspace.

My position is that the driver should change as few things as possible.
Hardware monitoring chips are a sensible realm. Enabling or disabling an
interrupt line or the like can cause the hardware to react (fan to full
speed or even shutdown) and we have had many reports that this happened.
So I think that the chip drivers should, by default, consider that the
chip is well configured for the hardware/bios settings, and initialize
very few things. My drivers work that way (not for the limits yet, but
for the rest).

The problem, as far as Linux 2.6 is concerned, is that libsensors isn't
ready yet, so removing all limit settings may be kind of suicidal
action. Still the point needs to be discussed for later.

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

fan speed for it87?? chips added

[Greg KH]

On Wed, Sep 24, 2003 at 11:10:30PM +0200, Jean Delvare wrote:
> 
> My position is that the driver should change as few things as possible.
> Hardware monitoring chips are a sensible realm. Enabling or disabling an
> interrupt line or the like can cause the hardware to react (fan to full
> speed or even shutdown) and we have had many reports that this happened.
> So I think that the chip drivers should, by default, consider that the
> chip is well configured for the hardware/bios settings, and initialize
> very few things. My drivers work that way (not for the limits yet, but
> for the rest).

I agree, this should be our policy.  Any current 2.6 drivers that
violate this?

thanks,

greg k-h

fan speed for it87?? chips added

[Michael Hufer]

> I'd really like you to investigate and find out which other settings
> were affecting the PWM setup. I don't want to remove all initialization
> code because we don't know which part of it causes problems.

Well, problem is a bit harsh, the init switched off the power supply's fan. I 
switched off the complete init shortly after starting this little project and 
didn't bother to investigate further as it did what I wanted it to do :-). 
I've some idea now what was causing this and I'll see if I can fix it.

> +	   This sets fan-divs to 2, among others.
>
> Where does it do that? Can't find it.

That comment refers to the reset i.e. write 0x80 into register 0x00.

>
> -		data->fan_div[2] = 1;
> +		data->fan_div[2] = data->fan_div[2];
>
> No comment.

Sometimes even I'm wondering about why I did things the way I did. I stumpled 
about this yesterday, too 
.. and removed it. :-) 

> +		if (*nrels_mag >= 3) {
> +			data->fan_div[1] = DIV_TO_REG(results[2]);
> +		}
>
> This must be fan_div[2] there.

Ouch, I did correct this in the kernel source tree - where I compiled the 
module - but forgot to merge the change back into the lm_sensors-cvs 
checkout.

> So this just can't work the way you want, unless you wanted it broken
> from the start ;) Understand me, I don't want to blame you, I'm
> particularly happy that you wrote that patch. But it obviously requires
> more reviewing and testing before we can commit it to our CVS
> repository.
> [...]
> The datasheet actually says that fan_div3 can be set to either 2 or 8.
> It's controled by bit 6 of register 0x0b. It differs from the other two
> divisors in that it is coded on a single bit while the others have
> three, which let them chose between 8 different divisors from 1 to 128.
> You could as well consider that the bit 6 of register 0x0b controls the
> *second* bit of fan_div3, which MSB and LSB are fixed to 0 and 1,
> respectively. This make div3 look more like div1 and 2 (to me at least,
> and should help in the code too).

Where did you get this info from? In the "IT8705F Preliminary Environment 
Controller (EC) Programming Guide V0.3" (it8705f_PG_ec_v03.pdf) I can't find 
it. There the bits 7 and 6 of register 0x0b are only descripted as reserved. 
I downloaded above pdf-file directly from ITE Inc's web side!

> BTW,
>
> -#define DIV_TO_REG(val) ((val)=8?3:(val)=4?2:(val)=1?0:1)
> +extern inline u8 DIV_TO_REG(long val)
> +{
> +	if( val = 1 )
> +		return 0;
> +	u8 i;
> +	for( i = 1; i < 7; i++ )
> +	{
> +		if( val = 1<<i )
> +			return i;
> +	}
> +	return 7;
> +}
>
> (BTW, does this really compile? I thought you couldn't declare a
> variable after real code in a given block.)

Umm..., I'm a C++ guy :-) and in C++ it is perfectly OK to declare a variable 
anywhere inside a block. It compiles perfectly with gcc 3.3.1 on my SuSE 8.2 
box, anyway.

> You don't need a special case for 1, unless I'm missing something. And
> you definitely want to return 1 as the default value, as it was the case
> before, because fan_div = 2 is a reasonable default. I also believe that
> we should try to find out the nearest divisor from what the user
> entered. Setting the divisor to 128, or either 2 , when the user asked
> for 31, is poor. We can do better than that. What about that:
>
> extern inline u8 DIV_TO_REG(long val)
> {
> 	u8 i;
> 	for( i = 0; i < 7; i++ )
> 		if( val>>i = 1 )
> 			return i;
> 	return 2;
> }
>
> This considers the highest weighted bit, whatever the lower bits are set
> to.

OK, but it your code returns the register value for a divisor of four (4) in 
case the desired value is greater than 32 (=1<<6), it should be a seven for a 
divisor of 128.
[...]
	return 7;
}

> > It could be made default not to reset the chip but as there might be
> > a BIOS/motherboard out there that does not correctly initialize the
> > chip, there should be a way to reset the chip on module load.
>
> So far, the BIOS seems to have been more clever than we were ;)
>
> I want the chip reset (0x80 at 0x00) gone, and everything else that
> causes trouble with it. The rest will be kept, inside an init=1
> conditional. Is it OK?

It's fine with me, thought I felt better keeping the chip reset in, too. 
Inside a if(reset=1) with reset defaulting to 0.

fan speed for it87?? chips added

[Jean Delvare]

> > My position is that the driver should change as few things as
> > possible. Hardware monitoring chips are a sensible realm. Enabling
> > or disabling an interrupt line or the like can cause the hardware to
> > react (fan to full speed or even shutdown) and we have had many
> > reports that this happened. So I think that the chip drivers should,
> > by default, consider that the chip is well configured for the
> > hardware/bios settings, and initialize very few things. My drivers
> > work that way (not for the limits yet, but for the rest).
> 
> I agree, this should be our policy.  Any current 2.6 drivers that
> violate this?

Any does *not*? ;)

I took a look and here I go. When I say "this should/shouldn't be done",
I'm refering to "if we follow the policy described above", this is not
the Absolute Truth (TM). Bits counted from 0 (LSB) to 7.

adm1021:
Bit 7 of the configuration register (ALERT mask) should be preserved.

it87:
Chip should not be reset (configuration register bit 7). Bits 5-1 of
the configuration register should be preserved. Also, I would leave
the voltage channel enable register untouched (same goes for the
fans). I also would add extra checks to make sure that the
temperature channel enable register is valid (but that's another
point).

lm75:
Bits 1-2 of the configuration register should be left untouched.
Ideally, fault queue should be configurable, if not it should also
be left untouched (bits 3-4 of the configuration register).

lm78:
Chip should not be reset (configuration register bit 7). Bits 1-2,5
should be left untouched.

lm85:
This one is OK.

via686a:
Chip should not be reset (configuration register bit 7). Bit 1
(interrupt enable register) should be left untouched.

w83781d:
Chip should not be reset (configuration register bit 7). Maybe some
other writes should be avoided, but this driver is too complicated
for a quick review. Will need to be analyzed in deep.

So, as you can see, most drivers would need to be changed. However, the
policy I propose is only the way I see the things, but some people here
are much more experienced with evil monitoring chips than I am myself.
Although chip initialization has proven to cause problems, I guess it
may be even worse without it in some cases. I may provide a patch
against 2.6.0-test5 that converts initialization routines as described
above, but I just can't promise the conversions are what we all want.

One possible compromise would be to provide an init parameter for all
module, ranging from 0 to N, where N is 3 (but could depend on the
module after all). 0 wouldn't initialize anything. 1 would initialize
the configuration registers. 2 would be 1 plus initialize limits to
default value. 3 would reset the chip plus 1 and 2. Or we may decide
that init is a bit vector, where 1<<0 resets the chip, 1<<1 sets the
limits, 1<<2 sets the configuration, (order to be defined) and so on.
I'd like to hear opinions about that. My purpose here is to provide a
standard way for the user to force a given init if the default (which
would probably depend on the driver) doesn't for for him/her. This would
make me feel more comfortable with changing the default initialization.

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

fan speed for it87?? chips added

[Jean Delvare]

> > +	   This sets fan-divs to 2, among others.
> >
> > Where does it do that? Can't find it.
> 
> That comment refers to the reset i.e. write 0x80 into register 0x00.

Oh, OK, I missed that at first.

> > The datasheet actually says that fan_div3 can be set to either 2 or
> > 8. It's controled by bit 6 of register 0x0b. It differs from the
> > other two divisors in that it is coded on a single bit while the
> > others have three, which let them chose between 8 different divisors
> > from 1 to 128. You could as well consider that the bit 6 of register
> > 0x0b controls the*second* bit of fan_div3, which MSB and LSB are
> > fixed to 0 and 1, respectively. This make div3 look more like div1
> > and 2 (to me at least, and should help in the code too).
> 
> Where did you get this info from? In the "IT8705F Preliminary
> Environment Controller (EC) Programming Guide V0.3"
> (it8705f_PG_ec_v03.pdf) I can't find it. There the bits 7 and 6 of
> register 0x0b are only descripted as reserved. I downloaded above
> pdf-file directly from ITE Inc's web side!

The version I have says:

7   -   Reserved
6  R/W  FAN_TAC3 Counter Divisor
        0: divided by 2
        1: divided by 8

I don't remember where I got it from, although I'm pretty sure I also
got them from ITE's website. I don't remember the original name either
(I'm renaming files to a standardized format). Maybe the filesize will
help:

1644525 Jul 18  2000 IT8702-0.3.pdf
1281351 Mar 16  2001 IT8705F-p1-0.3.pdf <-- this one
 657518 Mar 16  2001 IT8705F-p2-0.3.pdf
1150423 Mar 25  2002 IT8712F-0.6.pdf
 993541 Jun 11 09:20 IT8712F-0.7.pdf

I may send any of them to you if you want.

> > (BTW, does this really compile? I thought you couldn't declare a
> > variable after real code in a given block.)
> 
> Umm..., I'm a C++ guy :-) and in C++ it is perfectly OK to declare a
> variable anywhere inside a block. It compiles perfectly with gcc 3.3.1
> on my SuSE 8.2 box, anyway.

Still I don't want this in our repository.

> > You don't need a special case for 1, unless I'm missing something.
> > And you definitely want to return 1 as the default value, as it was
> > the case before, because fan_div = 2 is a reasonable default. I also
> > believe that we should try to find out the nearest divisor from what
> > the user entered. Setting the divisor to 128, or either 2 , when the
> > user asked for 31, is poor. We can do better than that. What about
> > that:
> >
> > extern inline u8 DIV_TO_REG(long val)
> > {
> > 	u8 i;
> > 	for( i = 0; i < 7; i++ )
> > 		if( val>>i = 1 )
> > 			return i;
> > 	return 2;
> > }
> >
> > This considers the highest weighted bit, whatever the lower bits are
> > set to.
> 
> OK, but it your code returns the register value for a divisor of four
> (4) in case the desired value is greater than 32 (=1<<6), it should be
> a seven for a divisor of 128.
> [...]
> 	return 7;
> }

We're both wrong here. I am because what I wanted is return 1 (for a fan
divisor of 2, as said above). You are because 1<<6 is 64, not 32 ;) And
I am once again because the condition is "i <= 7", not "i < 7". So this
should read:

extern inline u8 DIV_TO_REG(long val)
{
	u8 i;
	for( i = 0; i <= 7; i++ )
		if( val>>i = 1 )
			return i;
	return 1;
}

Is it OK now?

> > I want the chip reset (0x80 at 0x00) gone, and everything else that
> > causes trouble with it. The rest will be kept, inside an init=1
> > conditional. Is it OK?
> 
> It's fine with me, thought I felt better keeping the chip reset in,
> too. Inside a if(reset=1) with reset defaulting to 0.

OK for me.

Thanks.

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

fan speed for it87?? chips added

[Michael Hufer]

I found and fixed the little problem I had with the original chip 
initialization. I can now load the module with reset_it87=0 and init=1  
without messing up the settings of the power supply fan from the BIOS. It was 
quite simple after all.

> > Where did you get this info from? In the "IT8705F Preliminary
> > Environment Controller (EC) Programming Guide V0.3"
> > (it8705f_PG_ec_v03.pdf) I can't find it. There the bits 7 and 6 of
> > register 0x0b are only descripted as reserved. I downloaded above
> > pdf-file directly from ITE Inc's web side!
>
> The version I have says:
>
> 7   -   Reserved
> 6  R/W  FAN_TAC3 Counter Divisor
>         0: divided by 2
>         1: divided by 8
>
> [...]
> 1281351 Mar 16  2001 IT8705F-p1-0.3.pdf <-- this one
>  [...]
> I may send any of them to you if you want.

OK, this is the complete datasheet for the chip, I have it, too. But I only 
precursory read and compared it to the hardware monitoring specific 
programming guide which I then used for the implementation. I missed the 
small differences in the register descriptions. 
I changed the code for fan_div3 to read and write this bit. 
Lets see, I'll let the driver set this bit if the desired divisor value for 
fan3 is greater or equal eight (>= 8) and clear it for a divisor < 8. Or do 
you want a different behaviour.

> > > (BTW, does this really compile? I thought you couldn't declare a
> > > variable after real code in a given block.)
> >
> > Umm..., I'm a C++ guy :-) and in C++ it is perfectly OK to declare a
> > variable anywhere inside a block. It compiles perfectly with gcc 3.3.1
> > on my SuSE 8.2 box, anyway.
>
> Still I don't want this in our repository.

Sure, I just pointed out that it is valid C++ code and also compiles in gcc's 
C-mode.

> extern inline u8 DIV_TO_REG(long val)
> {
> 	u8 i;
> 	for( i = 0; i <= 7; i++ )
> 		if( val>>i = 1 )
> 			return i;
> 	return 1;
> }
>
> Is it OK now?

OK, I'm currently using and testing it with the latest changes for the 
fan_div3 read and write to chip and above DIV_TO_REG(). Seem to work OK.

Do you want me to send the latest diff?

	Micha.

fan speed for it87?? chips added

[Jean Delvare]

> I found and fixed the little problem I had with the original chip 
> initialization. I can now load the module with reset_it87=0 and
> init=1 without messing up the settings of the power supply fan
> from the BIOS. It was quite simple after all.

I'm interested in details, can you tell us what it was?

> Do you want me to send the latest diff?

Sure I want.

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

fan speed for it87?? chips added

[Michael Hufer]

On Friday 26 September 2003 22:40, Jean Delvare wrote:
> > I found and fixed the little problem I had with the original chip
> > initialization. I can now load the module with reset_it87=0 and
> > init=1 without messing up the settings of the power supply fan
> > from the BIOS. It was quite simple after all.
>
> I'm interested in details, can you tell us what it was?

Sure, it actually was something I introduced myself :-(. I initialized the 
fan_ctl register with  (read_value & 0x8f | 0x77) to enable the smart 
guardian feature for all 3 fans. But the power supply fan does not support 
pwm. It can only be operated in on/off mode, i.e. bit 0 must be clear. I 
switched it back to the original  (rv & 0x8f | 0x70 ) and everything is back 
to working.

> > Do you want me to send the latest diff?
>
> Sure I want.

 Here it comes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: it87_pwm-cvs.diff
Type: text/x-diff
Size: 23000 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20030926/aa2e4f94/it87_pwm-cvs.bin

fan speed for it87?? chips added

[Greg KH]

On Thu, Sep 25, 2003 at 07:33:39PM +0200, Michael Hufer wrote:
> > BTW,
> >
> > -#define DIV_TO_REG(val) ((val)=8?3:(val)=4?2:(val)=1?0:1)
> > +extern inline u8 DIV_TO_REG(long val)
> > +{
> > +	if( val = 1 )
> > +		return 0;
> > +	u8 i;
> > +	for( i = 1; i < 7; i++ )
> > +	{
> > +		if( val = 1<<i )
> > +			return i;
> > +	}
> > +	return 7;
> > +}
> >
> > (BTW, does this really compile? I thought you couldn't declare a
> > variable after real code in a given block.)
> 
> Umm..., I'm a C++ guy :-) and in C++ it is perfectly OK to declare a variable 
> anywhere inside a block. It compiles perfectly with gcc 3.3.1 on my SuSE 8.2 
> box, anyway.

It will not compile properly with older versions of gcc which are still
allowed to build the kernel with (like gcc 2.96 or so.)

thanks,

greg k-h

fan speed for it87?? chips added

[Greg KH]

On Thu, Sep 25, 2003 at 11:32:38PM +0200, Jean Delvare wrote:

<snip>

Thanks for the good summary.

> So, as you can see, most drivers would need to be changed. However, the
> policy I propose is only the way I see the things, but some people here
> are much more experienced with evil monitoring chips than I am myself.
> Although chip initialization has proven to cause problems, I guess it
> may be even worse without it in some cases. I may provide a patch
> against 2.6.0-test5 that converts initialization routines as described
> above, but I just can't promise the conversions are what we all want.
> 
> One possible compromise would be to provide an init parameter for all
> module, ranging from 0 to N, where N is 3 (but could depend on the
> module after all). 0 wouldn't initialize anything. 1 would initialize
> the configuration registers. 2 would be 1 plus initialize limits to
> default value. 3 would reset the chip plus 1 and 2. Or we may decide
> that init is a bit vector, where 1<<0 resets the chip, 1<<1 sets the
> limits, 1<<2 sets the configuration, (order to be defined) and so on.
> I'd like to hear opinions about that. My purpose here is to provide a
> standard way for the user to force a given init if the default (which
> would probably depend on the driver) doesn't for for him/her. This would
> make me feel more comfortable with changing the default initialization.

0, 1, 2, and 3 sound good to me.  Anyone else have an opinion?

thanks,

greg k-h

fan speed for it87?? chips added

[Mark Studebaker]

agreed

Greg KH wrote:
> 
> On Wed, Sep 24, 2003 at 11:10:30PM +0200, Jean Delvare wrote:
> >
> > My position is that the driver should change as few things as possible.
> > Hardware monitoring chips are a sensible realm. Enabling or disabling an
> > interrupt line or the like can cause the hardware to react (fan to full
> > speed or even shutdown) and we have had many reports that this happened.
> > So I think that the chip drivers should, by default, consider that the
> > chip is well configured for the hardware/bios settings, and initialize
> > very few things. My drivers work that way (not for the limits yet, but
> > for the rest).
> 
> I agree, this should be our policy.  Any current 2.6 drivers that
> violate this?
> 
> thanks,
> 
> greg k-h

fan speed for it87?? chips added

[Mark Studebaker]

checked in.
thanks.
mds

Michael Hufer wrote:
> 
> On Friday 26 September 2003 22:40, Jean Delvare wrote:
> > > I found and fixed the little problem I had with the original chip
> > > initialization. I can now load the module with reset_it87=0 and
> > > init=1 without messing up the settings of the power supply fan
> > > from the BIOS. It was quite simple after all.
> >
> > I'm interested in details, can you tell us what it was?
> 
> Sure, it actually was something I introduced myself :-(. I initialized the
> fan_ctl register with  (read_value & 0x8f | 0x77) to enable the smart
> guardian feature for all 3 fans. But the power supply fan does not support
> pwm. It can only be operated in on/off mode, i.e. bit 0 must be clear. I
> switched it back to the original  (rv & 0x8f | 0x70 ) and everything is back
> to working.
> 
> > > Do you want me to send the latest diff?
> >
> > Sure I want.
> 
>  Here it comes.
> 
> 
>                         Name: it87_pwm-cvs.diff
>    it87_pwm-cvs.diff    Type: text/x-diff
>                     Encoding: 7bit