> 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.
