Re: [lm-sensors] add AMDSI PECI sensortype support to

Re: [lm-sensors] add AMDSI PECI sensortype support to

[Jean Delvare]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

On Thu, 5 Jul 2007 13:23:04 +0200 (CEST), lm-sensors-notify@lm-sensors.org wrote:
>       Author: jwrdegoede
>         Date: Thu Jul  5 13:22:57 2007
> New Revision: 4552
>    Changeset: http://lm-sensors.org/changeset/4552
> 
> Modified:
>    lm-sensors/branches/lm-sensors-3.0.0/prog/sensors/chips_generic.c
> 
> Log:
> add AMDSI PECI sensortype support to generic_temp_print()

Good catch, but this does not yet cover all the values listed in
Documentation/hwmon/sysfs-interface:

temp[1-*]_type	Sensor type selection.
		Integers 1 to 6 or thermistor Beta value (typically 3435)
		RW
		1: PII/Celeron Diode
		2: 3904 transistor
		3: thermal diode
		4: thermistor (default/unknown Beta)
		5: AMD AMDSI
		6: Intel PECI
		Not all types are supported by all chips

Notice the "or thermistor Beta value". The w83627hf and w83781d drivers
actually export 3435 and not just 4 for thermistors, so the new
"sensors", in its current state, won't work for them. We could make it
default to "thermistor" for unknown or arbitrarily large values. But
OTOH, this feature (exporting the beta value) doesn't seem terribly
useful. In the two drivers which export the beta value... this value is
hard-coded in the driver, and not used anywhere, so what's the point?

Thus I think that this is also the right time to get rid of this old
convention, and start using always value 4 for thermistors. The upgrade
plan would be:

* Update Documentation/hwmon/sysfs-interface to no longer mention
beta values, and change the w83781d and w83627hf drivers to export
a thermal sensor value of 4 instead of 3435 for thermistors. We may
want to still accept when 3435 is written to the tempN_type files for
backwards compatibility for now, and drop support for it only later.
Proposed patch attached.
* Update sensors (v3) to still expect a thermal type value of 3435, and
treat it as value 4. We want it to work OK with the drivers from
previous 2.6 kernels.
* Update sensors.conf.eg to mention this change.

The good news is that the old "sensors" already defaults to
"thermistor" for unknown values for w83781d and w83627hf, so it won't
be affected by the change. Hopefully, other applications using
libsensors either do the same, or use generic code to display the
thermal types, of don't report thermal types at all.

Objection anyone?

-- 
Jean Delvare

[-- Attachment #2: hwmon-dont-export-beta.patch --]
[-- Type: text/x-patch, Size: 4298 bytes --]

Deprecate the use of thermistor beta values as thermal sensor types.
No driver supports changing the beta value anyway.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 Documentation/hwmon/sysfs-interface |    4 ++--
 drivers/hwmon/w83627hf.c            |   19 ++++++++++---------
 drivers/hwmon/w83781d.c             |   16 +++++++++-------
 3 files changed, 21 insertions(+), 18 deletions(-)

--- linux-2.6.22-rc7.orig/Documentation/hwmon/sysfs-interface	2007-07-07 10:58:43.000000000 +0200
+++ linux-2.6.22-rc7/Documentation/hwmon/sysfs-interface	2007-07-07 16:48:41.000000000 +0200
@@ -219,12 +219,12 @@ temp[1-*]_auto_point[1-*]_temp_hyst
 ****************
 
 temp[1-*]_type	Sensor type selection.
-		Integers 1 to 6 or thermistor Beta value (typically 3435)
+		Integers 1 to 6
 		RW
 		1: PII/Celeron Diode
 		2: 3904 transistor
 		3: thermal diode
-		4: thermistor (default/unknown Beta)
+		4: thermistor
 		5: AMD AMDSI
 		6: Intel PECI
 		Not all types are supported by all chips
--- linux-2.6.22-rc7.orig/drivers/hwmon/w83627hf.c	2007-07-07 10:58:39.000000000 +0200
+++ linux-2.6.22-rc7/drivers/hwmon/w83627hf.c	2007-07-07 16:59:10.000000000 +0200
@@ -372,11 +372,8 @@ struct w83627hf_data {
 	u8 beep_enable;		/* Boolean */
 	u8 pwm[3];		/* Register value */
 	u8 pwm_freq[3];		/* Register value */
-	u16 sens[3];		/* 782D/783S only.
-				   1 = pentium diode; 2 = 3904 diode;
-				   3000-5000 = thermistor beta.
-				   Default = 3435.
-				   Other Betas unimplemented */
+	u16 sens[3];		/* 1 = pentium diode; 2 = 3904 diode;
+				   4 = thermistor */
 	u8 vrm;
 	u8 vrm_ovt;		/* Register value, 627THF/637HF/687THF only */
 };
@@ -1001,7 +998,11 @@ store_sensor_reg(struct device *dev, con
 				    tmp & ~BIT_SCFG2[nr - 1]);
 		data->sens[nr - 1] = val;
 		break;
-	case W83781D_DEFAULT_BETA:	/* thermistor */
+	case W83781D_DEFAULT_BETA:
+		dev_warn(dev, "Sensor type %d is deprecated, please use 4 "
+			 "instead\n", W83781D_DEFAULT_BETA);
+		/* fall through */
+	case 4:		/* thermistor */
 		tmp = w83627hf_read_value(data, W83781D_REG_SCFG1);
 		w83627hf_write_value(data, W83781D_REG_SCFG1,
 				    tmp & ~BIT_SCFG1[nr - 1]);
@@ -1009,8 +1010,8 @@ store_sensor_reg(struct device *dev, con
 		break;
 	default:
 		dev_err(dev,
-		       "Invalid sensor type %ld; must be 1, 2, or %d\n",
-		       (long) val, W83781D_DEFAULT_BETA);
+		       "Invalid sensor type %ld; must be 1, 2, or 4\n",
+		       (long) val);
 		break;
 	}
 
@@ -1513,7 +1514,7 @@ static void __devinit w83627hf_init_devi
 	tmp = w83627hf_read_value(data, W83781D_REG_SCFG1);
 	for (i = 1; i <= 3; i++) {
 		if (!(tmp & BIT_SCFG1[i - 1])) {
-			data->sens[i - 1] = W83781D_DEFAULT_BETA;
+			data->sens[i - 1] = 4;
 		} else {
 			if (w83627hf_read_value
 			    (data,
--- linux-2.6.22-rc7.orig/drivers/hwmon/w83781d.c	2007-05-13 10:01:15.000000000 +0200
+++ linux-2.6.22-rc7/drivers/hwmon/w83781d.c	2007-07-07 17:02:55.000000000 +0200
@@ -251,9 +251,7 @@ struct w83781d_data {
 	u8 pwm2_enable;		/* Boolean */
 	u16 sens[3];		/* 782D/783S only.
 				   1 = pentium diode; 2 = 3904 diode;
-				   3000-5000 = thermistor beta.
-				   Default = 3435. 
-				   Other Betas unimplemented */
+				   4 = thermistor */
 	u8 vrm;
 };
 
@@ -721,15 +719,19 @@ store_sensor(struct device *dev, struct 
 				    tmp & ~BIT_SCFG2[nr]);
 		data->sens[nr] = val;
 		break;
-	case W83781D_DEFAULT_BETA:	/* thermistor */
+	case W83781D_DEFAULT_BETA:
+		dev_warn(dev, "Sensor type %d is deprecated, please use 4 "
+			 "instead\n", W83781D_DEFAULT_BETA);
+		/* fall through */
+	case 4:		/* thermistor */
 		tmp = w83781d_read_value(data, W83781D_REG_SCFG1);
 		w83781d_write_value(data, W83781D_REG_SCFG1,
 				    tmp & ~BIT_SCFG1[nr]);
 		data->sens[nr] = val;
 		break;
 	default:
-		dev_err(dev, "Invalid sensor type %ld; must be 1, 2, or %d\n",
-		       (long) val, W83781D_DEFAULT_BETA);
+		dev_err(dev, "Invalid sensor type %ld; must be 1, 2, or 4\n",
+		       (long) val);
 		break;
 	}
 
@@ -1485,7 +1487,7 @@ w83781d_init_device(struct device *dev)
 		tmp = w83781d_read_value(data, W83781D_REG_SCFG1);
 		for (i = 1; i <= 3; i++) {
 			if (!(tmp & BIT_SCFG1[i - 1])) {
-				data->sens[i - 1] = W83781D_DEFAULT_BETA;
+				data->sens[i - 1] = 4;
 			} else {
 				if (w83781d_read_value
 				    (data,

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] add AMDSI PECI sensortype support to

[Jean Delvare]

Hi Hans,

On Sat, 07 Jul 2007 17:45:36 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > On Thu, 5 Jul 2007 13:23:04 +0200 (CEST), lm-sensors-notify@lm-sensors.org wrote:
> >>       Author: jwrdegoede
> >>         Date: Thu Jul  5 13:22:57 2007
> >> New Revision: 4552
> >>    Changeset: http://lm-sensors.org/changeset/4552
> >>
> >> Modified:
> >>    lm-sensors/branches/lm-sensors-3.0.0/prog/sensors/chips_generic.c
> >>
> >> Log:
> >> add AMDSI PECI sensortype support to generic_temp_print()
> > 
> > Good catch, but this does not yet cover all the values listed in
> > Documentation/hwmon/sysfs-interface:
> > 
> > temp[1-*]_type	Sensor type selection.
> > 		Integers 1 to 6 or thermistor Beta value (typically 3435)
> > 		RW
> > 		1: PII/Celeron Diode
> > 		2: 3904 transistor
> > 		3: thermal diode
> > 		4: thermistor (default/unknown Beta)
> > 		5: AMD AMDSI
> > 		6: Intel PECI
> > 		Not all types are supported by all chips
> > 
> > Notice the "or thermistor Beta value". The w83627hf and w83781d drivers
> > actually export 3435 and not just 4 for thermistors, so the new
> > "sensors", in its current state, won't work for them. We could make it
> > default to "thermistor" for unknown or arbitrarily large values. But
> > OTOH, this feature (exporting the beta value) doesn't seem terribly
> > useful. In the two drivers which export the beta value... this value is
> > hard-coded in the driver, and not used anywhere, so what's the point?
> > 
> > Thus I think that this is also the right time to get rid of this old
> > convention, and start using always value 4 for thermistors. The upgrade
> > plan would be:
> > 
> > * Update Documentation/hwmon/sysfs-interface to no longer mention
> > beta values, and change the w83781d and w83627hf drivers to export
> > a thermal sensor value of 4 instead of 3435 for thermistors. We may
> > want to still accept when 3435 is written to the tempN_type files for
> > backwards compatibility for now, and drop support for it only later.
> > Proposed patch attached.
> > * Update sensors (v3) to still expect a thermal type value of 3435, and
> > treat it as value 4. We want it to work OK with the drivers from
> > previous 2.6 kernels.
> > * Update sensors.conf.eg to mention this change.
> > 
> > The good news is that the old "sensors" already defaults to
> > "thermistor" for unknown values for w83781d and w83627hf, so it won't
> > be affected by the change. Hopefully, other applications using
> > libsensors either do the same, or use generic code to display the
> > thermal types, of don't report thermal types at all.
> > 
> > Objection anyone?
> 
> Your proposal sounds good, and the patches look good. I didn't see a patch for 
> sensors3 to recognise the old 3435 value though, whichbring us to the queston 
> what will be the minimum kernel version we want to support with 3.0.0.

The sensors3 patch should be pretty easy, maybe you could take care of
it? It's needed anyway, whether my kernel patch gets applied or not.

I'm still not sure which minimum version we want to support. I don't
want to support anything older than 2.6.5 for sure, things were just
too badly broken before that. And I don't want to clutter the
libsensors code too much to deal with the non-standard things that some
old kernels had. Unless someone has a better plan, I think we'll make
things work for the systems we developers are using, and release that,
and then if users complain that it doesn't work with some older kernel
they're using, we'll consider adding support on a case-by-case basis,
depending on how intrusive that would be.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] add AMDSI PECI sensortype support to

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Sat, 07 Jul 2007 17:45:36 +0200, Hans de Goede wrote:
>> Your proposal sounds good, and the patches look good. I didn't see a patch for 
>> sensors3 to recognise the old 3435 value though, whichbring us to the queston 
>> what will be the minimum kernel version we want to support with 3.0.0.
> 
> The sensors3 patch should be pretty easy, maybe you could take care of
> it? It's needed anyway, whether my kernel patch gets applied or not.
> 

Ok, here is a proposal:

Index: prog/sensors/chips_generic.c
=================================--- prog/sensors/chips_generic.c        (revision 4566)
+++ prog/sensors/chips_generic.c        (working copy)
@@ -209,6 +209,11 @@
    /* print out temperature sensor info */
    if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_SENS)) {
      int sens = (int)TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_SENS);
+
+    /* older kernels / drivers sometimes report a beta value for thermistors */
+    if (sens > 1000)
+      sens = 4;
+
      printf("sensor = %s", sens = 0 ? "disabled" :
                            sens = 1 ? "diode" :
                            sens = 2 ? "transistor" :

The idea here is that a beta value should (reasonably) always be > 1000, and we 
don't want to assume thermistor for any unknown value as we might add new types 
to the list later. Does this look ok?

> I'm still not sure which minimum version we want to support. I don't
> want to support anything older than 2.6.5 for sure, things were just
> too badly broken before that. And I don't want to clutter the
> libsensors code too much to deal with the non-standard things that some
> old kernels had. Unless someone has a better plan, I think we'll make
> things work for the systems we developers are using, and release that,
> and then if users complain that it doesn't work with some older kernel
> they're using, we'll consider adding support on a case-by-case basis,
> depending on how intrusive that would be.
> 

That sounds like a plan, but not like something one would put in a README, 
which then brings me to the question what do we put in the readme, just that it 
requires a kernel > 2.6.5, or maybe we should require one which has the hwmon 
class?

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] add AMDSI PECI sensortype support to

[Jean Delvare]

Hi Hans,

On Sat, 07 Jul 2007 21:23:40 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > The sensors3 patch should be pretty easy, maybe you could take care of
> > it? It's needed anyway, whether my kernel patch gets applied or not.
> 
> Ok, here is a proposal:
> 
> Index: prog/sensors/chips_generic.c
> =================================> --- prog/sensors/chips_generic.c        (revision 4566)
> +++ prog/sensors/chips_generic.c        (working copy)
> @@ -209,6 +209,11 @@
>     /* print out temperature sensor info */
>     if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_SENS)) {
>       int sens = (int)TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_SENS);
> +
> +    /* older kernels / drivers sometimes report a beta value for thermistors */
> +    if (sens > 1000)
> +      sens = 4;
> +
>       printf("sensor = %s", sens = 0 ? "disabled" :
>                             sens = 1 ? "diode" :
>                             sens = 2 ? "transistor" :
> 
> The idea here is that a beta value should (reasonably) always be > 1000, and we 
> don't want to assume thermistor for any unknown value as we might add new types 
> to the list later. Does this look ok?

Yes, that's fine with me.

> > I'm still not sure which minimum version we want to support. I don't
> > want to support anything older than 2.6.5 for sure, things were just
> > too badly broken before that. And I don't want to clutter the
> > libsensors code too much to deal with the non-standard things that some
> > old kernels had. Unless someone has a better plan, I think we'll make
> > things work for the systems we developers are using, and release that,
> > and then if users complain that it doesn't work with some older kernel
> > they're using, we'll consider adding support on a case-by-case basis,
> > depending on how intrusive that would be.
> 
> That sounds like a plan, but not like something one would put in a README, 
> which then brings me to the question what do we put in the readme, just that it 
> requires a kernel > 2.6.5, or maybe we should require one which has the hwmon 
> class?

Well, requiring the hwmon class would let us cleanup a number of things
for sure, so it may be a good idea. That would mean we "only" support
kernels >= 2.6.14. But even then, the README will have to mention that
some features are not available before much more recent kernels. Most
notably, the alarms for most devices won't be available before 2.6.24.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] add AMDSI PECI sensortype support to

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Sat, 07 Jul 2007 21:23:40 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> The sensors3 patch should be pretty easy, maybe you could take care of
>>> it? It's needed anyway, whether my kernel patch gets applied or not.
>> Ok, here is a proposal:
>>
>> Index: prog/sensors/chips_generic.c
>> =================================>> --- prog/sensors/chips_generic.c        (revision 4566)
>> +++ prog/sensors/chips_generic.c        (working copy)
>> @@ -209,6 +209,11 @@
>>     /* print out temperature sensor info */
>>     if (TEMP_FEATURE(SENSORS_FEATURE_TEMP_SENS)) {
>>       int sens = (int)TEMP_FEATURE_VAL(SENSORS_FEATURE_TEMP_SENS);
>> +
>> +    /* older kernels / drivers sometimes report a beta value for thermistors */
>> +    if (sens > 1000)
>> +      sens = 4;
>> +
>>       printf("sensor = %s", sens = 0 ? "disabled" :
>>                             sens = 1 ? "diode" :
>>                             sens = 2 ? "transistor" :
>>
>> The idea here is that a beta value should (reasonably) always be > 1000, and we 
>> don't want to assume thermistor for any unknown value as we might add new types 
>> to the list later. Does this look ok?
> 
> Yes, that's fine with me.
> 

committed

>>> I'm still not sure which minimum version we want to support. I don't
>>> want to support anything older than 2.6.5 for sure, things were just
>>> too badly broken before that. And I don't want to clutter the
>>> libsensors code too much to deal with the non-standard things that some
>>> old kernels had. Unless someone has a better plan, I think we'll make
>>> things work for the systems we developers are using, and release that,
>>> and then if users complain that it doesn't work with some older kernel
>>> they're using, we'll consider adding support on a case-by-case basis,
>>> depending on how intrusive that would be.
>> That sounds like a plan, but not like something one would put in a README, 
>> which then brings me to the question what do we put in the readme, just that it 
>> requires a kernel > 2.6.5, or maybe we should require one which has the hwmon 
>> class?
> 
> Well, requiring the hwmon class would let us cleanup a number of things
> for sure, so it may be a good idea. That would mean we "only" support
> kernels >= 2.6.14. But even then, the README will have to mention that
> some features are not available before much more recent kernels. Most
> notably, the alarms for most devices won't be available before 2.6.24.
> 

2.6.14 sounds ok as a base, most people shoild be using something newer then 
that by now.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors