updated it87 for kernel 2.6 - Part1

PATCH New Takeru KOMORIYA 2 participants 5 messages May 19, 2005

lore

Share & tools

lore

Takeru KOMORIYA

author

2005-05-19 06:24 UTC #0 lore raw

PATCH 1/1 updated it87 for kernel 2.6 - Part1

Quoted 7 lines · On Mon, 16 Feb 2004 13:39:43 +0100 Jean Delvare <[email protected]> wrote:

Quoted 7 lines
I found it87 in kernel 2.6 may be based on old module. It lacks reset_it87 option and smart guardian (sg) features in lm_sensors cvs.
So I tryed to add these features and made the patch for linux-2.6.2-rc2. It works well for me.

Quoted 7 lines

Quoted 1 line
If you think the attached patch is useful, please merge it in.
I like the idea of porting 2.4 changes to 2.6, but your patch is not correct. In 2.6, we decided that each sysfs file would hold a single value. I agree that this is a bit different here since the "smart guardian" feature is specific to the IT87 chips. Still I don't think that Greg KH will accept such a patch.

Sorry, I didn't know that decision. I should have read porting-client guide carefully.

Quoted 4 lines

What do the three values in sgpwm and the five values in sgtl mean? We would have to find that many filenames.
How different is the "smart guardian" pwm from the standard pwm?

"Smart guardian" is the automatic temperature control mode for the fan. sgpwm means pwm values for low, medium and high speed for fan, and sgtl means the temperature limits for fan off, low, medium, hi speed and over temperature.

How about the sysfs names for Smart guardian like:
sgpwm_low[1-3] sgpwm_medium[1-3] sgpwm_high[1-3] sgtemp_off[1-3] sgtemp_low[1-3] sgtemp_medium[1-3] sgtemp_high[1-3] sgtemp_over[1-3]

Please read the documentation of it87 for more details.
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/chips/it87

Quoted 7 lines

I also think it would be easier for us if you could split your big patch in parts. It looks like you do four different, independant things here:
* change the sensor type selection method
* add it87_reset (which btw I would more simply call "reset")
* add pwm support
* add smart guardian support

O.K., I will post patches step by step.

Here's the first patch for changing the sensor type selection method. The patch deletes temp_type option and make the sensor type be set at runtime instead.

Thanks,
-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: linux-2.6.2-rc2_it87_sensor_type.patch
Type: application/octet-stream
Size: 2655 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040216/15558bfd/linux-2.6.2-rc2_it87_sensor_type.obj

Jean Delvare

2005-05-19 06:24 UTC #1 lore raw

PATCH 1/1 updated it87 for kernel 2.6 - Part1

Quoted 4 lines

"Smart guardian" is the automatic temperature control mode for the fan. sgpwm means pwm values for low, medium and high speed for fan, and sgtl means the temperature limits for fan off, low, medium, hi speed and over temperature.

OK, I think that I roughly see how it works. Later for the details.

Quoted 4 lines

How about the sysfs names for Smart guardian like:
sgpwm_low[1-3] sgpwm_medium[1-3] sgpwm_high[1-3] sgtemp_off[1-3] sgtemp_low[1-3] sgtemp_medium[1-3] sgtemp_high[1-3] sgtemp_over[1-3]

Sounds correct to me.

Quoted 2 lines

Please read the documentation of it87 for more details.
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/chips/it87

Hehe, funny when a developer is told to go and read the docs ;) I took a quick look and will read it more carefully when we will be working on it.

Quoted 3 lines

Here's the first patch for changing the sensor type selection method. The patch deletes temp_type option and make the sensor type be set at runtime instead.

(You will note that I will emit criticisms about your code while you just do the exact same thing that was done in the 2.4 driver you used as an example. This simply means that I want changed made to the 2.6 driver to be better than the 2.4 driver is at the moment. I will then take care that all improvements/changes we do to the 2.6 driver will be backported to the 2.4 driver.)

Looks good overall, except two things (one detail, one critical).

You seem to consider that each temperature channel *must* have either the PIIDIODE (0x01) or the THERMISTOR (0x08) bit set. If none is set, you call it INVALID. According to the data sheets, I would say that the only invalid value is when *both* bits are set at the same time. No bits most probably means that the channel is not in use. For this reason, I would rename the INVALID constant to UNUSED or DISABLED. I don't think we need a numerical value for INVALID, since it is not going to happen (if we do code cleanly, see below). (And, BTW, I think I would prefix them with "SENSOR_" or something similar, but you are free to do or not to do it).

Then, you forgot to define sysfs files for reading and writing the temperature sensor types! Which makes your patch not acceptable as-is. To follow the example of the other drivers, these files should be named sensor[1-3]. Please note that according to my remark above, it is under your responsability to ensure that you will never set both type bits to 1 for a given channel. The data sheets don't give details about what would happen, but I wouldn't try. The prefered way to make sure it doesn't happen is probably to make invalid writes fail (sysfs callback function returns -1 instead of 0).

Please provide a patch fixing at least this second point, and the first one if you share my views. If you don't, well, I'm listening to you.

Please make sure the driver compiles and works as intended after your patch is applied. This is one of the reasons why it is great to cut large patches into pieces, because it lets you test each change individually.

Thanks.

Signature · 2 lines

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

Jean Delvare

2005-05-19 06:24 UTC #2 lore raw

PATCH 1/1 updated it87 for kernel 2.6 - Part1

In reply to Takeru KOMORIYA

According to Takeru KOMORIYA <[email protected]>:

Quoted 3 lines

I agree with you. I was feeling something strange with INVALID. I think it's better to change "invalid" in sensors program(patch is attached).

Correct, except that now, a really invalid value (i.e. different from 0,
2 and 3) will be silently ignored. I agree that, if the driver is written correctly, this should not possibly happen. Still, an extra check doesn't cost much and could help us trap problems later. I'll correct this.

Quoted 8 lines

Also, documentation for it87 should be changed.
-----
To change sensor x to a thermistor, 'echo 2 > sensorx' where x is 1, 2, or 3.
To change sensor x to a thermal diode, 'echo 3 > sensorx'.
- Any other value is invalid.
+ Give 0 for unused sensor.
----

Correct again, except that "Any other value is invalid" should be kept. I'll handle that too.

Quoted 2 lines

Definition of sysfs files already exist in kernel2.6 source and works for me...

Aha, correct :) I swear I checked wether they were there or not, and did
not see them! Incredible.

Quoted 2 lines

Oh, I was in confusion! Sorry for my rush patch. Please just ingore my first patch which contains useless code.

I guess you refer to the additional chunk in it87_update_client? I don't
really get why you say it is useless. I would keep that code. Our general policy is not to do any supposition as to wether some values don't change in chips (such as limits, and in this case sensor types). We have seen cases where these "constant" values *were* changing, probably because the BIOS is accessing the chip in our back. So, reading the sensor types in it87_update_client sounds like the right thing to do (although I agree that in most case it should be safe not to do so).

Quoted 2 lines

My proposal with attached new patch is:
1. delete temp_type option because it can be set at runtime

OK.

Quoted 2 lines

2. change the value for thermal diode to be compatible with
'sensors' program

OK. Strange it wasn't correct in the first place.

Quoted 1 line

3. add some comments for temperature types

OK. Good idea.

Quoted 1 line

4. Change copyright mark of (c) to (C)

OK.

I still would like set_sensor to fail (return -1) if an attempt to write
an invalid value (i.e. different from 0, 2 and 3) is made. This basically means adding the following lines:

	if (val = 3)
	    data->sensor |= 1 << nr;
	else if (val = 2)
	    data->sensor |= 8 << nr;

+ else if (val != 0)
+ return -1;

Unless you have an objection, of course.

Please resubmit a patch including the requested changes, or explain me why you should not. BTW, don't worry about resubmitting patches. This is how things work and this is perfectly normal :) This is the only way we can ensure that the code that goes into the kernel is the best we can collectively think of.

Thanks.

Signature · 2 lines

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

Takeru KOMORIYA

author

2005-05-19 06:24 UTC #3 lore raw

PATCH 1/1 updated it87 for kernel 2.6 - Part1

In reply to Takeru KOMORIYA

Quoted 10 lines

You seem to consider that each temperature channel *must* have either the PIIDIODE (0x01) or the THERMISTOR (0x08) bit set. If none is set, you call it INVALID. According to the data sheets, I would say that the only invalid value is when *both* bits are set at the same time. No bits most probably means that the channel is not in use. For this reason, I would rename the INVALID constant to UNUSED or DISABLED. I don't think we need a numerical value for INVALID, since it is not going to happen(if we do code cleanly, see below). (And, BTW, I think I would prefix them with "SENSOR_" or something similar, but you are free to do or not to do it).

I agree with you. I was feeling something strange with INVALID. I think it's better to change "invalid" in sensors program(patch is attached).

Also, documentation for it87 should be changed.
-----
To change sensor x to a thermistor, 'echo 2 > sensorx' where x is 1, 2, or 3.
To change sensor x to a thermal diode, 'echo 3 > sensorx'.
- Any other value is invalid.
+ Give 0 for unused sensor.
----

Quoted 10 lines

Then, you forgot to define sysfs files for reading and writing the temperature sensor types! Which makes your patch not acceptable as-is. To follow the example of the other drivers, these files should be named sensor[1-3]. Please note that according to my remark above, it is under your responsability to ensure that you will never set both type bits to 1 for a given channel. The data sheets don't give details about what would happen, but I wouldn't try. The prefered way to make sure it doesn't happen is probably to make invalid writes fail (sysfs callback function returns -1 instead of 0).

Definition of sysfs files already exist in kernel2.6 source and works for me... Oh, I was in confusion! Sorry for my rush patch. Please just ingore my first patch which contains useless code.

My proposal with attached new patch is:
1. delete temp_type option because it can be set at runtime
2. change the value for thermal diode to be compatible with 'sensors' program
3. add some comments for temperature types
4. Change copyright mark of (c) to (C)

Quoted 4 lines

Please make sure the driver compiles and works as intended after your patch is applied. This is one of the reasons why it is great to cut large patches into pieces, because it lets you test each change individually.

I'll be more careful from this time.

Thanks in advance.
-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm_sensors-cvs-fix-it87-sensor.patch
Type: application/octet-stream
Size: 1338 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040217/91d04206/lm_sensors-cvs-fix-it87-sensor.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: linux-2.6.2-rc2_it87_sensor_type.patch
Type: application/octet-stream
Size: 2311 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040217/91d04206/linux-2.6.2-rc2_it87_sensor_type.obj

Takeru KOMORIYA

author

2005-05-19 06:24 UTC #4 lore raw

PATCH 1/1 updated it87 for kernel 2.6 - Part1

In reply to Takeru KOMORIYA

Quoted 10 lines · On Tue, 17 Feb 2004 10:01:54 +0100 Jean Delvare <[email protected]> wrote:

Quoted 3 lines
I agree with you. I was feeling something strange with INVALID. I think it's better to change "invalid" in sensors program(patch is attached).
Correct, except that now, a really invalid value (i.e. different from 0,
2 and 3) will be silently ignored. I agree that, if the driver is written correctly, this should not possibly happen. Still, an extra check doesn't cost much and could help us trap problems later. I'll correct this.

Thanks.

Quoted 11 lines

Quoted 8 lines
Also, documentation for it87 should be changed.
-----
To change sensor x to a thermistor, 'echo 2 > sensorx' where x is 1, 2, or 3.
To change sensor x to a thermal diode, 'echo 3 > sensorx'.
- Any other value is invalid.
+ Give 0 for unused sensor.
----
Correct again, except that "Any other value is invalid" should be kept. I'll handle that too.

Thanks again.

Quoted 13 lines

Quoted 2 lines
Oh, I was in confusion! Sorry for my rush patch. Please just ingore my first patch which contains useless code.
I guess you refer to the additional chunk in it87_update_client? I don't
really get why you say it is useless. I would keep that code. Our general policy is not to do any supposition as to wether some values don't change in chips (such as limits, and in this case sensor types). We have seen cases where these "constant" values *were* changing, probably because the BIOS is accessing the chip in our back. So, reading the sensor types in it87_update_client sounds like the right thing to do (although I agree that in most case it should be safe not to do so).

I think I understand what you say. We should handle all register values as "volatile". I feel it's reasonable.
I added the code for updating sensor types into it87_update_client.

Quoted 11 lines

I still would like set_sensor to fail (return -1) if an attempt to write an invalid value (i.e. different from 0, 2 and 3) is made. This basically means adding the following lines:
	if (val = 3)
	    data->sensor |= 1 << nr;
	else if (val = 2)
	    data->sensor |= 8 << nr;
(quoted diff) +2 -0
+	else if (val != 0)
+	    return -1;

Quoted 1 line

Unless you have an objection, of course.

OK, I agree with definition that the values other than 0,2,3 are invalid value.

Here's new patch with this mail. Please check it.