it87 Part2(reset option)
PATCH 1/1 it87 Part2(reset option)
Hi,
Thanks Jean for your works and kind advices.
I'll go on the next step.
My second proposal for it87 is adding 'reset' option and do not reset the registers unless users really want to do. It's the same behavior as it87 of kernel2.4.
If 'smart guardian' was enabled by BIOS, resetting registers makes all fans go to full power, very noisy.
Resetting registers is, I think, not required for normal use.
Here's the small patch.
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.sensor_type +6 -1
@@ -58,4 +58,7 @@static int update_vbat =0 ; +/* Reset the registers on init */ +static int reset =0 ; +/* Many IT87 constants specified below */
@@ -828,5 +831,7 @@/* Reset all except Watchdog values and last conversion values This sets fan-divs to 2, among others */ -it87_write_value (client,IT87_REG_CONFIG ,0x80 ); +if ( reset ) { +it87_write_value (client,IT87_REG_CONFIG ,0x80 ); + }it87_write_value (client,IT87_REG_VIN_MIN (0 ),IN_TO_REG (IT87_INIT_IN_MIN_0 ));
@@ -989,4 +994,6 @@MODULE_PARM (update_vbat,"i" ); MODULE_PARM_DESC(update_vbat, "Update vbat if set else return powerup
value");
+MODULE_PARM(reset, "i");
+MODULE_PARM_DESC(reset, "Reset the chip's registers, default no");
MODULE_LICENSE("GPL");
Thanks in advance.
-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/
PATCH 1/1 it87 Part2(reset option)
Hi Takeru,
Quoted 5 lines
My second proposal for it87 is adding 'reset' option and do not reset the registers unless users really want to do. It's the same behavior as it87 of kernel 2.4.
If 'smart guardian' was enabled by BIOS, resetting registers makes all fans go to full power, very noisy.
We have been having reports (not to say complaints) about that.
Quoted 1 line
Resetting registers is, I think, not required for normal use.
I fully agree, not only for the it87 but for all other chips too.
drivers/i2c/chips/it87.c line 61
Quoted 2 lines
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.sensor_type +2
-0+/* Reset the registers on init */ +static int reset =0 ;
Do not initialize it explicitely to 0. It's done for you (if I'm not mistaking).
drivers/i2c/chips/it87.c line 833
Quoted 1 line
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.sensor_type +1
-0+if ( reset ) {
No spaces inside the parentheses please (I know that the 2.4 driver do have spaces, I'll fix that).
drivers/i2c/chips/it87.c line 837
Quoted 2 lines
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.sensor_type +0
-0it87_write_value (client,IT87_REG_VIN_MIN (0 ),IN_TO_REG (IT87_INIT_IN_MIN_0 ));
While we're at it, I'd suggest that you remove all limit initializations. They are already gone in the 2.4 driver, and almost all 2.6 drivers. The prefered way of setting these is from user-space (sensors -s). Don't forget to remove the associated comment (I'll do it in 2.4).
Also, now that the reset is optional, the rest of the init (enable all voltages, all temperatures and all fans) should depend on it. If you do not reset the registers, the values are supposed to be already correct. In the 2.4 driver, these initialisations are even completely gone. We should take a look at the datasheets. Maybe the value we are writing are the default (post-reset) values anyway, so writing them again is useless?
The only thing that should be kept (and has been in 2.4) is the monitoring start, of course (with or without reset). That said, I think that the 0xb7 mask is wrong and should be 0x36 (that too should be verified with the datasheets).
Please provide a new version of your patch doing all this. It will of course be a bit longer than your initial attempt.
Aligato.
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
PATCH 1/1 it87 Part2(reset option)
In reply to Takeru KOMORIYAQuoting Takeru KOMORIYA <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Do not initialize it explicitely to 0. It's done for you (if I'm not mistaking).
OK, I think
update_vbatshould be the same then.
Correct.
Quoted 3 lines
According to the datasheet, voltage scan, temperature channels and fan tachometers are disabled at reset. We can enable temperature channels from user-space, but not for voltages and thachometers.
This would mean that the reset option of the 2.4 driver is unusable at the moment (since it would also disable all readings)?
Quoted 4 lines
So I think we should keep initializations for voltages and thachometers, even if reset option is not given, because it may not be guaranteed that these are always set by BIOS(especially if hardware monitor features are disabled by BIOS settings).
This could be done on purpose by the BIOS. For example, the BIOS could know that a given voltage input isn't wired on the motherboard, and disable that particular input. I regret that we would lose this information if we were to force all inputs on by default.
Maybe we could first check whether all voltage inputs and tachometers are disabled. If they are (which is the case after a reset), enable them all. If not, don't touch anything. That way, we would respect the choices of the BIOS in the case they are correct.
This is only a suggestion, you might not agree with it.
Quoted 7 lines
Quoted 4 lines
The only thing that should be kept (and has been in 2.4) is the monitoring start, of course (with or without reset). That said, I think that the
0xb7mask is wrong and should be0x36(that too should be verified with the datasheets).I looked into the datasheet and I think
0x36is the right value(though0xb7may not cause problems).
Correct. It simply sounds better to me, expecially because we have no way to write a 1 to bit 7 anymore. Since we retrieve the configuration register value right after a reset, I was thinking that under some conditions, the bit 7 could be still high at that moment. This would make us reset the chip twice, and possibly lose the values we were trying to write to the other bits. I agree that this isn't supposed to be possible, but being safe doesn't cost us anything here.
Quoted 3 lines
Quoted 1 line
Aligato.
Wow, you speak Japanese! Aligato!
Not really. I think I know three words: "ushi", "dicho" and "aligato". Only the last one is obviously usable on a regular basis ;) My grilfriend, on the other hand, knows quite a bit of Jananese, and we like watching original Japanese animation videos together.
Review line 829
Quoted 3 lines
(quoted diff) +0
-0@@ -825,77 +828,24 @@ /* Called when we have found a new IT87. It should set limits, etc. */
The second part of this comment doesn't make sense anymore.
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
PATCH 1/1 it87 Part2(reset option)
In reply to Takeru KOMORIYAQuoted 4 lines
I commented out the code for initializing voltages and thachometers, and tested with reset=1, and I say yes though I have tested for 2.6 but not for 2.4.
Sensor values stopped updating without these initializations.
OK, so the datasheets are correct, and I'll have to fix the 2.4 driver.
Quoted 4 lines
I agree with you. I don't like to lose such information. Voltages and thachometers could be sensed without initialization even if I set "ignore this sensor" in BIOS settings. My assumption was not correct.
The "ignore" function of your BIOS setup screen must be similar to our "ignore" statements in sensors.conf. It simply hides the value, nothing more.
Quoted 2 lines
I feel this is good idea. I added the code to check if all voltages (or tachometers) are disabled.
This make me think of something. You do not handle the temperatures at all, because the user can configure them from user-space. Doesn't it sound a bit strange though that upon reset, all inputs but temperatures are forcibly enabled?
We could save the settings before reset (for temperatures only) and restore them right after.
Or we could forcibly enable temperatures with what used to be the default value in older versions of the driver (0x2a if my memory is correct).
Or we could mix both (restore values for enabled channels, and use the default values for disabled temperature channels).
What do you think? This is a true question, I don't have strong opinion (although solution 2 is the more similar to what is done for voltages and fan tachometers). Even doing nothing (the way it is) is OK for me if you like it that way.
[...]
Quoted 1 line
I know such videos are called Japanimation:)
In France we often refer to them simply as "anime" and everyone understands we speak about Japanese ones.
Quoted 1 line
I wonder why you know the word "dicho" (intestinum crassum?)...
Oh well, I must have been spelling it incorrectly. I only ever heard it,
never seen it written. Maybe it's "disho" or "dijo"? The word I know is supposed to mean "dictionary".
[...]
Quoted 1 line
Here's new version with your suggestions.
Perfect, except:
Review
Quoted 5 lines
(quoted diff) +5
-0+ /* Check if tachometers are reset manually or by some reason */ + if ((it87_read_value(client, IT87_REG_FAN_CTRL) & 0x70) = 0) { + /* Enable all fan tachometers */ + it87_write_value(client, IT87_REG_FAN_CTRL, + (it87_read_value(client, IT87_REG_FAN_CTRL) & 0x8f)
You can spare the second read by saving the value returned by the previous read of the same register in a local variable. Remember that register reads are relatively slow (especially on I2C busses) so we want to spare them each time we can.
Aligato :)
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
PATCH 1/1 it87 Part2(reset option)
In reply to Takeru KOMORIYAHi,
Quoted 5 lines · On Wed, 25 Feb 2004 14:12:39 +0100 Jean Delvare <[email protected]> wrote:
Review
Quoted 2 lines
(quoted diff) +2
-0+/* Reset the registers on init */ +static int reset = 0;Do not initialize it explicitely to 0. It's done for you (if I'm not mistaking).
OK, I think update_vbat should be the same then.
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ if ( reset ) {No spaces inside the parentheses please (I know that the 2.4 driver do have spaces, I'll fix that).
Sorry, I fixed it.
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +0
-0it87_write_value(client, IT87_REG_VIN_MIN(0), IN_TO_REG(IT87_INIT_IN_MIN_0));While we're at it, I'd suggest that you remove all limit initializations. They are already gone in the 2.4 driver, and almost all 2.6 drivers. The prefered way of setting these is from user-space(sensors -s). Don't forget to remove the associated comment (I'll do it in 2.4).
I agree with you. I've deleted these initializations.
Quoted 7 lines
Also, now that the reset is optional, the rest of the init (enable all voltages, all temperatures and all fans) should depend on it. If you do not reset the registers, the values are supposed to be already correct. In the 2.4 driver, these initialisations are even completely gone. We should take a look at the datasheets. Maybe the value we are writing are the default (post-reset) values anyway, so writing them again is useless?
According to the datasheet, voltage scan, temperature channels and fan tachometers are disabled at reset. We can enable temperature channels from user-space, but not for voltages and thachometers.
So I think we should keep initializations for voltages and thachometers, even if reset option is not given, because it may not be guaranteed that these are always set by BIOS(especially if hardware monitor features are disabled by BIOS settings).
Quoted 4 lines
The only thing that should be kept (and has been in 2.4) is the monitoring start, of course (with or without reset). That said, I think that the
0xb7mask is wrong and should be0x36(that too should be verified with the datasheets).
I looked into the datasheet and I think 0x36 is the right value(though 0xb7 may not cause problems).
Quoted 2 lines
Please provide a new version of your patch doing all this. It will of course be a bit longer than your initial attempt.
I've attached my next trial patch below.
Quoted 1 line
Aligato.
Wow, you speak Japanese! Aligato!
it87.c renamed from it87.c.sensor_type +14 -62
@@ -55,7 +55,10 @@/* Update battery voltage after every reading if true */ -static int update_vbat =0 ; +static int update_vbat; + +/* Reset the registers on init if true */ +static int reset;/* Many IT87 constants specified below */
@@ -825,77 +828,24 @@/* Called when we have found a new IT87. It should set limits, etc. */ static void it87_init_client (struct i2c_client *client,struct it87_data *data) { -/* Reset all except Watchdog values and last conversion values -This sets fan-divs to 2, among others */ -it87_write_value (client,IT87_REG_CONFIG ,0x80 ); -it87_write_value (client,IT87_REG_VIN_MIN (0 ), -IN_TO_REG (IT87_INIT_IN_MIN_0 )); -it87_write_value (client,IT87_REG_VIN_MAX (0 ), -IN_TO_REG (IT87_INIT_IN_MAX_0 )); -it87_write_value (client,IT87_REG_VIN_MIN (1 ), -IN_TO_REG (IT87_INIT_IN_MIN_1 )); -it87_write_value (client,IT87_REG_VIN_MAX (1 ), -IN_TO_REG (IT87_INIT_IN_MAX_1 )); -it87_write_value (client,IT87_REG_VIN_MIN (2 ), -IN_TO_REG (IT87_INIT_IN_MIN_2 )); -it87_write_value (client,IT87_REG_VIN_MAX (2 ), -IN_TO_REG (IT87_INIT_IN_MAX_2 )); -it87_write_value (client,IT87_REG_VIN_MIN (3 ), -IN_TO_REG (IT87_INIT_IN_MIN_3 )); -it87_write_value (client,IT87_REG_VIN_MAX (3 ), -IN_TO_REG (IT87_INIT_IN_MAX_3 )); -it87_write_value (client,IT87_REG_VIN_MIN (4 ), -IN_TO_REG (IT87_INIT_IN_MIN_4 )); -it87_write_value (client,IT87_REG_VIN_MAX (4 ), -IN_TO_REG (IT87_INIT_IN_MAX_4 )); -it87_write_value (client,IT87_REG_VIN_MIN (5 ), -IN_TO_REG (IT87_INIT_IN_MIN_5 )); -it87_write_value (client,IT87_REG_VIN_MAX (5 ), -IN_TO_REG (IT87_INIT_IN_MAX_5 )); -it87_write_value (client,IT87_REG_VIN_MIN (6 ), -IN_TO_REG (IT87_INIT_IN_MIN_6 )); -it87_write_value (client,IT87_REG_VIN_MAX (6 ), -IN_TO_REG (IT87_INIT_IN_MAX_6 )); -it87_write_value (client,IT87_REG_VIN_MIN (7 ), -IN_TO_REG (IT87_INIT_IN_MIN_7 )); -it87_write_value (client,IT87_REG_VIN_MAX (7 ), -IN_TO_REG (IT87_INIT_IN_MAX_7 )); -/* Note: Battery voltage does not have limit registers */ -it87_write_value (client,IT87_REG_FAN_MIN (0 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_1 ,2 )); -it87_write_value (client,IT87_REG_FAN_MIN (1 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_2 ,2 )); -it87_write_value (client,IT87_REG_FAN_MIN (2 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_3 ,2 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (0 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_1 )); -it87_write_value (client,IT87_REG_TEMP_LOW (0 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_1 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (1 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_2 )); -it87_write_value (client,IT87_REG_TEMP_LOW (1 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_2 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (2 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_3 )); -it87_write_value (client,IT87_REG_TEMP_LOW (2 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_3 )); +if (reset) { +/* Reset all except Watchdog values and last conversion values +This sets fan-divs to 2, among others */ +it87_write_value (client,IT87_REG_CONFIG ,0x80 ); + + }/* Enable voltage monitors */ it87_write_value (client,IT87_REG_VIN_ENABLE ,0xff ); -/* Enable Temp1-Temp3 */ - data->sensor = (it87_read_value (client,IT87_REG_TEMP_ENABLE ) &0xc0 ); - data->sensor |=0x2a ;/* Temp1,Temp3=thermistor; Temp2=thermal diode */ -it87_write_value (client,IT87_REG_TEMP_ENABLE , data->sensor); - -/* Enable fans */ +/* Enable fan tachometers */ it87_write_value (client,IT87_REG_FAN_CTRL , (it87_read_value (client,IT87_REG_FAN_CTRL ) &0x8f ) |0x70 );/* Start monitoring */ it87_write_value (client,IT87_REG_CONFIG , - (it87_read_value (client,IT87_REG_CONFIG ) &0xb7 ) + (it87_read_value (client,IT87_REG_CONFIG ) &0x36 ) | (update_vbat ?0x41 :0x01 )); }
@@ -988,6 +938,8 @@MODULE_DESCRIPTION ("IT8705F, IT8712F, Sis950 driver" );MODULE_PARM (update_vbat,"i" );MODULE_PARM_DESC (update_vbat,"Update vbat if set else return powerup value" ); +MODULE_PARM (reset,"i" ); +MODULE_PARM_DESC (reset,"Reset the chip's registers, default no" );MODULE_LICENSE ("GPL" );module_init (sm_it87_init);
Thanks in advance,
-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/
PATCH 1/1 it87 Part2(reset option)
In reply to Takeru KOMORIYAHi,
Quoted 6 lines · On Thu, 26 Feb 2004 16:34:26 +0100 Jean Delvare <[email protected]> wrote:
Quoted 3 lines
According to the datasheet, voltage scan, temperature channels and fan tachometers are disabled at reset. We can enable temperature channels from user-space, but not for voltages and thachometers.
This would mean that the reset option of the 2.4 driver is unusable at the moment (since it would also disable all readings)?
I commented out the code for initializing voltages and thachometers, and tested with reset=1, and I say yes though I have tested for 2.6 but not for 2.4.
Sensor values stopped updating without these initializations.
Quoted 10 lines
Quoted 4 lines
So I think we should keep initializations for voltages and thachometers, even if reset option is not given, because it may not be guaranteed that these are always set by BIOS(especially if hardware monitor features are disabled by BIOS settings).
This could be done on purpose by the BIOS. For example, the BIOS could know that a given voltage input isn't wired on the motherboard, and disable that particular input. I regret that we would lose this information if we were to force all inputs on by default.
I agree with you. I don't like to lose such information. Voltages and thachometers could be sensed without initialization even if I set "ignore this sensor" in BIOS settings. My assumption was not correct.
Quoted 6 lines
Maybe we could first check whether all voltage inputs and tachometers are disabled. If they are (which is the case after a reset), enable them all. If not, don't touch anything. That way, we would respect the choices of the BIOS in the case they are correct.
This is only a suggestion, you might not agree with it.
I feel this is good idea. I added the code to check if all voltages(or tachometers) are disabled.
Quoted 6 lines
Quoted 1 line
Wow, you speak Japanese! Aligato!
Not really. I think I know three words: "ushi", "dicho" and "aligato". Only the last one is obviously usable on a regular basis ;) My grilfriend, on the other hand, knows quite a bit of Jananese, and we like watching original Japanese animation videos together.
I know such videos are called Japanimation:)
I wonder why you know the word "dicho" (intestinum crassum?)...
Quoted 6 lines
Review line 830
Quoted 4 lines
(quoted diff) +0
-0@@ -825,77 +828,24 @@ /* Called when we have found a new IT87. It should set limits, etc. */The second part of this comment doesn't make sense anymore.
Sure. I fixed it.
Here's new version with your suggestions.
it87.c renamed from it87.c.sensor_type +27 -69
@@ -55,7 +55,10 @@/* Update battery voltage after every reading if true */ -static int update_vbat =0 ; +static int update_vbat; + +/* Reset the registers on init if true */ +static int reset;/* Many IT87 constants specified below */
@@ -822,80 +825,33 @@return i2c_smbus_write_byte_data (client, reg, value); } -/* Called when we have found a new IT87. It should set limits, etc. */ +/* Called when we have found a new IT87. */ static void it87_init_client (struct i2c_client *client,struct it87_data *data) { -/* Reset all except Watchdog values and last conversion values -This sets fan-divs to 2, among others */ -it87_write_value (client,IT87_REG_CONFIG ,0x80 ); -it87_write_value (client,IT87_REG_VIN_MIN (0 ), -IN_TO_REG (IT87_INIT_IN_MIN_0 )); -it87_write_value (client,IT87_REG_VIN_MAX (0 ), -IN_TO_REG (IT87_INIT_IN_MAX_0 )); -it87_write_value (client,IT87_REG_VIN_MIN (1 ), -IN_TO_REG (IT87_INIT_IN_MIN_1 )); -it87_write_value (client,IT87_REG_VIN_MAX (1 ), -IN_TO_REG (IT87_INIT_IN_MAX_1 )); -it87_write_value (client,IT87_REG_VIN_MIN (2 ), -IN_TO_REG (IT87_INIT_IN_MIN_2 )); -it87_write_value (client,IT87_REG_VIN_MAX (2 ), -IN_TO_REG (IT87_INIT_IN_MAX_2 )); -it87_write_value (client,IT87_REG_VIN_MIN (3 ), -IN_TO_REG (IT87_INIT_IN_MIN_3 )); -it87_write_value (client,IT87_REG_VIN_MAX (3 ), -IN_TO_REG (IT87_INIT_IN_MAX_3 )); -it87_write_value (client,IT87_REG_VIN_MIN (4 ), -IN_TO_REG (IT87_INIT_IN_MIN_4 )); -it87_write_value (client,IT87_REG_VIN_MAX (4 ), -IN_TO_REG (IT87_INIT_IN_MAX_4 )); -it87_write_value (client,IT87_REG_VIN_MIN (5 ), -IN_TO_REG (IT87_INIT_IN_MIN_5 )); -it87_write_value (client,IT87_REG_VIN_MAX (5 ), -IN_TO_REG (IT87_INIT_IN_MAX_5 )); -it87_write_value (client,IT87_REG_VIN_MIN (6 ), -IN_TO_REG (IT87_INIT_IN_MIN_6 )); -it87_write_value (client,IT87_REG_VIN_MAX (6 ), -IN_TO_REG (IT87_INIT_IN_MAX_6 )); -it87_write_value (client,IT87_REG_VIN_MIN (7 ), -IN_TO_REG (IT87_INIT_IN_MIN_7 )); -it87_write_value (client,IT87_REG_VIN_MAX (7 ), -IN_TO_REG (IT87_INIT_IN_MAX_7 )); -/* Note: Battery voltage does not have limit registers */ -it87_write_value (client,IT87_REG_FAN_MIN (0 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_1 ,2 )); -it87_write_value (client,IT87_REG_FAN_MIN (1 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_2 ,2 )); -it87_write_value (client,IT87_REG_FAN_MIN (2 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_3 ,2 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (0 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_1 )); -it87_write_value (client,IT87_REG_TEMP_LOW (0 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_1 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (1 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_2 )); -it87_write_value (client,IT87_REG_TEMP_LOW (1 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_2 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (2 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_3 )); -it87_write_value (client,IT87_REG_TEMP_LOW (2 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_3 )); - -/* Enable voltage monitors */ -it87_write_value (client,IT87_REG_VIN_ENABLE ,0xff ); - -/* Enable Temp1-Temp3 */ - data->sensor = (it87_read_value (client,IT87_REG_TEMP_ENABLE ) &0xc0 ); - data->sensor |=0x2a ;/* Temp1,Temp3=thermistor; Temp2=thermal diode */ -it87_write_value (client,IT87_REG_TEMP_ENABLE , data->sensor); +if (reset) { +/* Reset all except Watchdog values and last conversion values +This sets fan-divs to 2, among others */ +it87_write_value (client,IT87_REG_CONFIG ,0x80 ); + + } -/* Enable fans */ -it87_write_value (client,IT87_REG_FAN_CTRL , - (it87_read_value (client,IT87_REG_FAN_CTRL ) &0x8f ) - |0x70 ); +/* Check if voltage monitors are reset manually or by some reason */ +if (it87_read_value (client,IT87_REG_VIN_ENABLE ) =0 ) { +/* Enable all voltage monitors */ +it87_write_value (client,IT87_REG_VIN_ENABLE ,0xff ); + } + +/* Check if tachometers are reset manually or by some reason */ +if ((it87_read_value (client,IT87_REG_FAN_CTRL ) &0x70 ) =0 ) { +/* Enable all fan tachometers */ +it87_write_value (client,IT87_REG_FAN_CTRL , + (it87_read_value (client,IT87_REG_FAN_CTRL ) &0x8f ) + |0x70 ); + }/* Start monitoring */ it87_write_value (client,IT87_REG_CONFIG , - (it87_read_value (client,IT87_REG_CONFIG ) &0xb7 ) + (it87_read_value (client,IT87_REG_CONFIG ) &0x36 ) | (update_vbat ?0x41 :0x01 )); }
@@ -988,6 +944,8 @@MODULE_DESCRIPTION ("IT8705F, IT8712F, Sis950 driver" );MODULE_PARM (update_vbat,"i" );MODULE_PARM_DESC (update_vbat,"Update vbat if set else return powerup value" ); +MODULE_PARM (reset,"i" ); +MODULE_PARM_DESC (reset,"Reset the chip's registers, default no" );MODULE_LICENSE ("GPL" );module_init (sm_it87_init);
Thanks,
-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/
PATCH 1/1 it87 Part2(reset option)
In reply to Takeru KOMORIYAQuoted 1 line
Here's new version.
Great :) It's OK for me.
Now I could send it to Greg, but you could also to it yourself, so as to
get the credit you deserve for your work. It's easy. Send your patch
again, to Greg KH, CC the lm_sensors mailing list. The patch must be
inline (as you always do) and work when one uses "patch -p1" from the
first level of the Linux kernel tree. This means that you have to
generate it using
"diff -u linux-2.6.3/drivers/i2c/chips/it87.c.sensor_type linux-2.6.3/drivers/i2c/chips/it87.c" or similar. Your email should explain what the patch does and why it is
good. You are also invited to mention the fact that I already reviewed
your work and liked it (so as to ease Greg's decision).
In the meantime, I'll be backporting your code to our 2.4 driver.
Thanks a lot for your good work so far :)
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
PATCH 1/1 it87 Part2(reset option)
In reply to Takeru KOMORIYAQuoted 7 lines
Quoted 2 lines
I feel this is good idea. I added the code to check if all voltages (or tachometers) are disabled.
This make me think of something. You do not handle the temperatures at all, because the user can configure them from user-space. Doesn't it sound a bit strange though that upon reset, all inputs but temperatures are forcibly enabled?
I feel so when one comes to think of it.
Quoted 7 lines
We could save the settings before reset (for temperatures only) and restore them right after.
Or we could forcibly enable temperatures with what used to be the default value in older versions of the driver (
0x2aif my memory is correct).
Quoted 7 lines
Or we could mix both (restore values for enabled channels, and use the default values for disabled temperature channels).
What do you think? This is a true question, I don't have strong opinion(although solution 2 is the more similar to what is done for voltages and fan tachometers). Even doing nothing (the way it is) is OK for me if you like it that way.
I like the second one though I don't have strong opinion too. If we use reset option, I think this means that register settings by BIOS may be broken or can not be reliable.
Quoted 4 lines
Quoted 1 line
I know such videos are called Japanimation:)
In France we often refer to them simply as "anime" and everyone understands we speak about Japanese ones.
Well, I guess it's because of special abbreviation way of Japanese. We abbreviate many foreign words in similar way. telebi(television), paso-com(personal computer), nego-ru(make negotiation), and so on.
Quoted 5 lines
Quoted 1 line
I wonder why you know the word "dicho" (intestinum crassum?)...
Oh well, I must have been spelling it incorrectly. I only ever heard it, never seen it written. Maybe it's "disho" or "dijo"? The word I know is supposed to mean "dictionary".
I see:) "disho" is correct for dictionary, though "jisho" is formal spelling for Japanse.
Quoted 10 lines
Review
Quoted 5 lines
(quoted diff) +1
-0+ /* Check if tachometers are reset manually or by some reason*/+ if ((
it87_read_value(client, IT87_REG_FAN_CTRL)&0x70) = 0)
{+ /* Enable all fan tachometers */(quoted diff) +2
-0+ it87_write_value(client, IT87_REG_FAN_CTRL, + (it87_read_value(client, IT87_REG_FAN_CTRL) & 0x8f)You can spare the second read by saving the value returned by the previous read of the same register in a local variable. Remember that register reads are relatively slow (especially on I2C busses) so we want to spare them each time we can.
Thanks. I haven't thought of the access speed to registers.
Here's new version.
Merci:)
it87.c renamed from it87.c.sensor_type +39 -69
@@ -55,7 +55,10 @@/* Update battery voltage after every reading if true */ -static int update_vbat =0 ; +static int update_vbat; + +/* Reset the registers on init if true */ +static int reset;/* Many IT87 constants specified below */
@@ -822,80 +825,45 @@return i2c_smbus_write_byte_data (client, reg, value); } -/* Called when we have found a new IT87. It should set limits, etc. */ +/* Called when we have found a new IT87. */ static void it87_init_client (struct i2c_client *client,struct it87_data *data) { -/* Reset all except Watchdog values and last conversion values -This sets fan-divs to 2, among others */ -it87_write_value (client,IT87_REG_CONFIG ,0x80 ); -it87_write_value (client,IT87_REG_VIN_MIN (0 ), -IN_TO_REG (IT87_INIT_IN_MIN_0 )); -it87_write_value (client,IT87_REG_VIN_MAX (0 ), -IN_TO_REG (IT87_INIT_IN_MAX_0 )); -it87_write_value (client,IT87_REG_VIN_MIN (1 ), -IN_TO_REG (IT87_INIT_IN_MIN_1 )); -it87_write_value (client,IT87_REG_VIN_MAX (1 ), -IN_TO_REG (IT87_INIT_IN_MAX_1 )); -it87_write_value (client,IT87_REG_VIN_MIN (2 ), -IN_TO_REG (IT87_INIT_IN_MIN_2 )); -it87_write_value (client,IT87_REG_VIN_MAX (2 ), -IN_TO_REG (IT87_INIT_IN_MAX_2 )); -it87_write_value (client,IT87_REG_VIN_MIN (3 ), -IN_TO_REG (IT87_INIT_IN_MIN_3 )); -it87_write_value (client,IT87_REG_VIN_MAX (3 ), -IN_TO_REG (IT87_INIT_IN_MAX_3 )); -it87_write_value (client,IT87_REG_VIN_MIN (4 ), -IN_TO_REG (IT87_INIT_IN_MIN_4 )); -it87_write_value (client,IT87_REG_VIN_MAX (4 ), -IN_TO_REG (IT87_INIT_IN_MAX_4 )); -it87_write_value (client,IT87_REG_VIN_MIN (5 ), -IN_TO_REG (IT87_INIT_IN_MIN_5 )); -it87_write_value (client,IT87_REG_VIN_MAX (5 ), -IN_TO_REG (IT87_INIT_IN_MAX_5 )); -it87_write_value (client,IT87_REG_VIN_MIN (6 ), -IN_TO_REG (IT87_INIT_IN_MIN_6 )); -it87_write_value (client,IT87_REG_VIN_MAX (6 ), -IN_TO_REG (IT87_INIT_IN_MAX_6 )); -it87_write_value (client,IT87_REG_VIN_MIN (7 ), -IN_TO_REG (IT87_INIT_IN_MIN_7 )); -it87_write_value (client,IT87_REG_VIN_MAX (7 ), -IN_TO_REG (IT87_INIT_IN_MAX_7 )); -/* Note: Battery voltage does not have limit registers */ -it87_write_value (client,IT87_REG_FAN_MIN (0 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_1 ,2 )); -it87_write_value (client,IT87_REG_FAN_MIN (1 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_2 ,2 )); -it87_write_value (client,IT87_REG_FAN_MIN (2 ), -FAN_TO_REG (IT87_INIT_FAN_MIN_3 ,2 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (0 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_1 )); -it87_write_value (client,IT87_REG_TEMP_LOW (0 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_1 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (1 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_2 )); -it87_write_value (client,IT87_REG_TEMP_LOW (1 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_2 )); -it87_write_value (client,IT87_REG_TEMP_HIGH (2 ), -TEMP_TO_REG (IT87_INIT_TEMP_HIGH_3 )); -it87_write_value (client,IT87_REG_TEMP_LOW (2 ), -TEMP_TO_REG (IT87_INIT_TEMP_LOW_3 )); - -/* Enable voltage monitors */ -it87_write_value (client,IT87_REG_VIN_ENABLE ,0xff ); - -/* Enable Temp1-Temp3 */ - data->sensor = (it87_read_value (client,IT87_REG_TEMP_ENABLE ) &0xc0 ); - data->sensor |=0x2a ;/* Temp1,Temp3=thermistor; Temp2=thermal diode */ -it87_write_value (client,IT87_REG_TEMP_ENABLE , data->sensor); +int tmp; + +if (reset) { +/* Reset all except Watchdog values and last conversion values +This sets fan-divs to 2, among others */ +it87_write_value (client,IT87_REG_CONFIG ,0x80 ); + + } -/* Enable fans */ -it87_write_value (client,IT87_REG_FAN_CTRL , - (it87_read_value (client,IT87_REG_FAN_CTRL ) &0x8f ) - |0x70 ); +/* Check if temperature channnels are reset manually or by some reason */ + tmp =it87_read_value (client,IT87_REG_TEMP_ENABLE ); +if ((tmp &0x3f ) =0 ) { +/* Temp1,Temp3=thermistor; Temp2=thermal diode */ + tmp = (tmp &0xc0 ) |0x2a ; +it87_write_value (client,IT87_REG_TEMP_ENABLE , tmp); + } + data->sensor = tmp; + +/* Check if voltage monitors are reset manually or by some reason */ + tmp =it87_read_value (client,IT87_REG_VIN_ENABLE ); +if ((tmp &0xff ) =0 ) { +/* Enable all voltage monitors */ +it87_write_value (client,IT87_REG_VIN_ENABLE ,0xff ); + } + +/* Check if tachometers are reset manually or by some reason */ + tmp =it87_read_value (client,IT87_REG_FAN_CTRL ); +if ((tmp &0x70 ) =0 ) { +/* Enable all fan tachometers */ + tmp = (tmp &0x8f ) |0x70 ; +it87_write_value (client,IT87_REG_FAN_CTRL , tmp); + }/* Start monitoring */ it87_write_value (client,IT87_REG_CONFIG , - (it87_read_value (client,IT87_REG_CONFIG ) &0xb7 ) + (it87_read_value (client,IT87_REG_CONFIG ) &0x36 ) | (update_vbat ?0x41 :0x01 )); }
@@ -988,6 +956,8 @@MODULE_DESCRIPTION ("IT8705F, IT8712F, Sis950 driver" );MODULE_PARM (update_vbat,"i" );MODULE_PARM_DESC (update_vbat,"Update vbat if set else return powerup value" ); +MODULE_PARM (reset,"i" ); +MODULE_PARM_DESC (reset,"Reset the chip's registers, default no" );MODULE_LICENSE ("GPL" );module_init (sm_it87_init);
-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/