hwmon: (scpi) Add slope and offset to SCP sensor readings

From: Carlo Caione <[email protected]>

The temperature provided by the SCP sensors not always is expressed in millicelsius, whereas this is required by the thermal framework. This is for example the case for the Amlogic devices, where the SCP sensor readings are expressed in degree (and not milli degree) Celsius.

To convert the sensor readings, the thermal framework provides the "coefficients" property, used by the thermal sensor device to adjust their reading. This adjustment in currently not considered by the SCPI hwmon driver. Fix this introducing slope and offset for the SCP sensor readings.

Signed-off-by Carlo Caione <[email protected]>

---
 drivers/hwmon/scpi-hwmon.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/hwmon/scpi-hwmon.c +1 -0

index 094f948f99ff..cdc05c60ba67 100644
@@ -33,6 +33,7 @@ struct sensor_data {
 struct scpi_thermal_zone {
 	int sensor_id;
 	struct scpi_sensors *scpi_sensors;
+	struct thermal_zone_device *z;
 };
 
 struct scpi_sensors {

@@ -51,13 +52,17 @@ static int scpi_read_temp(void *dev, int *temp)
 	struct scpi_ops *scpi_ops = scpi_sensors->scpi_ops;
 	struct sensor_data *sensor = &scpi_sensors->data[zone->sensor_id];
 	u64 value;

drivers/hwmon/scpi-hwmon.c +7 -6

+
+	*temp = value * slope + offset;
 	return 0;
 }
 

@@ -216,7 +221,6 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&scpi_sensors->thermal_zones);
 	for (i = 0; i < nr_sensors; i++) {
 		struct sensor_data *sensor = &scpi_sensors->data[i];
-		struct thermal_zone_device *z;
 		struct scpi_thermal_zone *zone;
 
 		if (sensor->info.class != TEMPERATURE)

@@ -228,17 +232,17 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 
 		zone->sensor_id = i;
 		zone->scpi_sensors = scpi_sensors;
-		z = devm_thermal_zone_of_sensor_register(dev,
-							 sensor->info.sensor_id,
-							 zone,
-							 &scpi_sensor_ops);
+		zone->z = devm_thermal_zone_of_sensor_register(dev,
+							       sensor->info.sensor_id,
+							       zone,
+							       &scpi_sensor_ops);
 		/*
 		 * The call to thermal_zone_of_sensor_register returns
 		 * an error for sensors that are not associated with
 		 * any thermal zones or if the thermal subsystem is
 		 * not configured.
 		 */
-		if (IS_ERR(z)) {
+		if (IS_ERR(zone->z)) {
 			devm_kfree(dev, zone);
 			continue;
 		}

Quoted 3 lines · Carlo Caione

From: Carlo Caione <[email protected]>

Quoted 5 lines

The temperature provided by the SCP sensors not always is expressed in millicelsius, whereas this is required by the thermal framework. This is for example the case for the Amlogic devices, where the SCP sensor readings are expressed in degree (and not milli degree) Celsius.

Are you saying that SCPI does not specify or provide the units to be used when reading values, and thus effectively just reports a more or less random number ?

drivers/hwmon/scpi-hwmon.c static int scpi_read_temp(void *dev, int *temp) · line 63

Quoted 3 lines

drivers/hwmon/scpi-hwmon.c +2 -1

-       *temp = value;
+       slope = thermal_zone_get_slope(zone->z);
+       offset = thermal_zone_get_offset(zone->z);

This is conceptually wrong. The functions return -ENODEV if thermal is disabled.
While a negative slope does not make sense, a negative offset does.

The code in the thermal subsystem does not clarify well how slope and offset are supposed to be used. Since coming from the thermal subsystem, one would think
that the thermal subsystem would apply any corrections if they are supposed to be software correction values, but that does not appear to be the case, leaving it up to drivers to use or not use the provided values.

This is kind of odd; the values are thermal subsystem attributes/properties, and one would think that they are to be used there unless they are supposed to be
written into hardware. Given that, I'd rather wait for the API to be clarified
instead of jumping into using it.

There are secondary problems with this approach; other drivers such as lm90 which
support the thermal subsystem have their own means to specify and use the offset
(since it needs to be programmed into the chip registers).

Also, this would only solve the problem for temperatures and does not address
the generic problem (ie voltage and current values).

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Carlo Caione

From: Carlo Caione <[email protected]>

The temperature provided by the SCP sensors not always is expressed in millicelsius, whereas this is required by the thermal framework. This is for example the case for the Amlogic devices, where the SCP sensor readings are expressed in degree (and not milli degree) Celsius.

Quoted 3 lines

Are you saying that SCPI does not specify or provide the units to be used when reading values, and thus effectively just reports a more or less random number ?

AFAICT the standard does not specify the units to be used for the values or a way to get that information. For the Amlogic case the SCP returns the value of the temperature in degree celsius and I'm not sure how common is that.

[cut]

Quoted 5 lines

Quoted 3 lines

drivers/hwmon/scpi-hwmon.c +2 -1

-       *temp = value;
+       slope = thermal_zone_get_slope(zone->z);
+       offset = thermal_zone_get_offset(zone->z);

Quoted 3 lines

This is conceptually wrong. The functions return -ENODEV if thermal is disabled.
While a negative slope does not make sense, a negative offset does.

Yeah, but in that case we would have already failed to register the thermal zone at all in devm_thermal_zone_of_sensor_register().

Quoted 6 lines

The code in the thermal subsystem does not clarify well how slope and offset are supposed to be used. Since coming from the thermal subsystem, one would think
that the thermal subsystem would apply any corrections if they are supposed to be software correction values, but that does not appear to be the case, leaving it up to drivers to use or not use the provided values.

Yes, this is my understanding also considering this comment here:
http://lxr.free-electrons.com/source/drivers/thermal/of-thermal.c#L1006

So apparently the slope and offset values are left to be used by the driver, that's why I was changing the temperature driver to use those values.

Quoted 6 lines

This is kind of odd; the values are thermal subsystem attributes/properties, and one would think that they are to be used there unless they are supposed to be
written into hardware. Given that, I'd rather wait for the API to be clarified
instead of jumping into using it.

Zhang, Eduardo (+CC)
can you clarify a bit this point?

Quoted 5 lines

There are secondary problems with this approach; other drivers such as lm90 which
support the thermal subsystem have their own means to specify and use the offset
(since it needs to be programmed into the chip registers).

I fail to see how this is related to my patch. If no coefficient is defined in the thermal zone node in the DT the framework sets slope=1 and offset=0, so in the hwmon driver nothing changes.

Quoted 3 lines

Also, this would only solve the problem for temperatures and does not address
the generic problem (ie voltage and current values).

This problem is specific for the interaction between the hwmon SCPI driver and the thermal subsystem. Look for example at the board I'm working with one single thermal zone using the temperature data coming from SCP:

scpi_sensors: sensors {
    compatible = "arm,scpi-sensors";
    #thermal-sensor-cells = <1>;
};

thermal-zones {
    soc_thermal {
        thermal-sensors = <&scpi_sensors 0>;
        coefficients = <1000 0>;

        trips {
            control: trip-point at 1 {
                temperature = <80000>;
                hysteresis = <1000>;
                type = "passive";
            };
        };

        cooling-maps {
            cpufreq_cooling_map {
                trip = <&control>;
                cooling-device = <&cpus THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
            };
        };
    };
};

trip points are defined in milli degree Celsius while the temperature returned by the scpi_sensors node is in degree Celsius. Without using the coefficients property and my changes how can I make the cooling map working fine?

Thanks,

Quoted 8 lines · Carlo Caione

Quoted 3 lines

Are you saying that SCPI does not specify or provide the units to be used when reading values, and thus effectively just reports a more or less random number ?

AFAICT the standard does not specify the units to be used for the values or a way to get that information. For the Amlogic case the SCP returns the value of the temperature in degree celsius and I'm not sure how common is that.

The standard does specify the units, but the way it is written seems to suggest that the units are part of the platform implementation rather than part of the standard [0].

[0] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922g/CABBCJGH.html

Quoted 6 lines

[cut]

Quoted 5 lines

Quoted 3 lines

drivers/hwmon/scpi-hwmon.c +2 -1

-       *temp = value;
+       slope = thermal_zone_get_slope(zone->z);
+       offset = thermal_zone_get_offset(zone->z);

Quoted 7 lines

Quoted 3 lines

This is conceptually wrong. The functions return -ENODEV if thermal is disabled.
While a negative slope does not make sense, a negative offset does.

Yeah, but in that case we would have already failed to register the thermal zone at all in devm_thermal_zone_of_sensor_register().

In addition to the thermal sub-subsystem, hwmon sysfs interface also expects temperature in millidegree Celsius. Ideally, any change should fix the reporting there as well. More below.

Another way to fix this would be to add optional properties to the scpi sensors binding and use them instead. This could then be used to fixup values reported to both thermal and hwmon.

Quoted 10 lines · Carlo Caione

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Carlo Caione

From: Carlo Caione <[email protected]>

The temperature provided by the SCP sensors not always is expressed in millicelsius, whereas this is required by the thermal framework. This is for example the case for the Amlogic devices, where the SCP sensor readings are expressed in degree (and not milli degree) Celsius.

Quoted 8 lines

Quoted 3 lines

Are you saying that SCPI does not specify or provide the units to be used when reading values, and thus effectively just reports a more or less random number ?

AFAICT the standard does not specify the units to be used for the values or a way to get that information. For the Amlogic case the SCP returns the value of the temperature in degree celsius and I'm not sure how common is that.

The standard does specify the units, but the way it is written seems to suggest that the units are part of the platform implementation rather than part of the standard [0].

[0] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922g/CABBCJGH.html

Quoted 7 lines

[cut]

Quoted 5 lines

Quoted 3 lines

drivers/hwmon/scpi-hwmon.c +2 -1

-       *temp = value;
+       slope = thermal_zone_get_slope(zone->z);
+       offset = thermal_zone_get_offset(zone->z);

Quoted 7 lines

Quoted 3 lines

This is conceptually wrong. The functions return -ENODEV if thermal is disabled.
While a negative slope does not make sense, a negative offset does.

Yeah, but in that case we would have already failed to register the thermal zone at all in devm_thermal_zone_of_sensor_register().

In addition to the thermal sub-subsystem, hwmon sysfs interface also expects temperature in millidegree Celsius. Ideally, any change should fix the reporting there as well. More below.

Quoted 21 lines

Quoted 6 lines

The code in the thermal subsystem does not clarify well how slope and offset are supposed to be used. Since coming from the thermal subsystem, one would think
that the thermal subsystem would apply any corrections if they are supposed to be software correction values, but that does not appear to be the case, leaving it up to drivers to use or not use the provided values.

Yes, this is my understanding also considering this comment here:
http://lxr.free-electrons.com/source/drivers/thermal/of-thermal.c#L1006

So apparently the slope and offset values are left to be used by the driver, that's why I was changing the temperature driver to use those values.

Quoted 6 lines

This is kind of odd; the values are thermal subsystem attributes/properties, and one would think that they are to be used there unless they are supposed to be
written into hardware. Given that, I'd rather wait for the API to be clarified
instead of jumping into using it.

Quoted 3 lines

Zhang, Eduardo (+CC)
can you clarify a bit this point?

Another way to fix this would be to add optional properties to the scpi sensors binding and use them instead. This could then be used to fixup values reported to both thermal and hwmon.

Quoted 4 lines · Punit Agrawal

Another way to fix this would be to add optional properties to the scpi sensors binding and use them instead. This could then be used to fixup values reported to both thermal and hwmon.

Yes, this would be much more appropriate.

Quoted 7 lines · Guenter Roeck

Quoted 4 lines · Punit Agrawal

Another way to fix this would be to add optional properties to the scpi sensors binding and use them instead. This could then be used to fixup values reported to both thermal and hwmon.

Quoted 1 line

Yes, this would be much more appropriate.

What about something like:

sensor-coefficients = <x1 y1 z1>,

                                  <x2 y2 z2>,
                                  ...;

Quoted 4 lines · Guenter Roeck

Quoted 3 lines · Carlo Caione

From: Carlo Caione <[email protected]>

Quoted 8 lines

Quoted 5 lines

The temperature provided by the SCP sensors not always is expressed in millicelsius, whereas this is required by the thermal framework. This is for example the case for the Amlogic devices, where the SCP sensor readings are expressed in degree (and not milli degree) Celsius.

Are you saying that SCPI does not specify or provide the units to be used when reading values, and thus effectively just reports a more or less random number ?

AFAICT the standard does not specify the units to be used for the values or a way to get that information. For the Amlogic case the SCP returns the value of the temperature in degree celsius and I'm not sure how common is that.

[cut]

Quoted 5 lines

Quoted 3 lines

drivers/hwmon/scpi-hwmon.c +2 -1

-       *temp = value;
+       slope = thermal_zone_get_slope(zone->z);
+       offset = thermal_zone_get_offset(zone->z);

Quoted 3 lines

This is conceptually wrong. The functions return -ENODEV if thermal is disabled.
While a negative slope does not make sense, a negative offset does.

Yeah, but in that case we would have already failed to register the thermal zone at all in devm_thermal_zone_of_sensor_register().

Quoted 6 lines

The code in the thermal subsystem does not clarify well how slope and offset are supposed to be used. Since coming from the thermal subsystem, one would think
that the thermal subsystem would apply any corrections if they are supposed to be software correction values, but that does not appear to be the case, leaving it up to drivers to use or not use the provided values.

Yes, this is my understanding also considering this comment here:
http://lxr.free-electrons.com/source/drivers/thermal/of-thermal.c#L1006

So apparently the slope and offset values are left to be used by the driver, that's why I was changing the temperature driver to use those values.

Quoted 6 lines

This is kind of odd; the values are thermal subsystem attributes/properties, and one would think that they are to be used there unless they are supposed to be
written into hardware. Given that, I'd rather wait for the API to be clarified
instead of jumping into using it.

Zhang, Eduardo (+CC)
can you clarify a bit this point?

Quoted 5 lines

There are secondary problems with this approach; other drivers such as lm90 which
support the thermal subsystem have their own means to specify and use the offset
(since it needs to be programmed into the chip registers).

I fail to see how this is related to my patch. If no coefficient is defined in the thermal zone node in the DT the framework sets slope=1 and offset=0, so in the hwmon driver nothing changes.

Quoted 3 lines

Also, this would only solve the problem for temperatures and does not address
the generic problem (ie voltage and current values).

This problem is specific for the interaction between the hwmon SCPI driver and the thermal subsystem. Look for example at the board I'm working with one single thermal zone using the temperature data coming from SCP:

scpi_sensors: sensors {
    compatible = "arm,scpi-sensors";
    #thermal-sensor-cells = <1>;
};

thermal-zones {
    soc_thermal {
        thermal-sensors = <&scpi_sensors 0>;
        coefficients = <1000 0>;

        trips {
            control: trip-point at 1 {
                temperature = <80000>;
                hysteresis = <1000>;
                type = "passive";
            };
        };

        cooling-maps {
            cpufreq_cooling_map {
                trip = <&control>;
                cooling-device = <&cpus THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
            };
        };
    };
};

trip points are defined in milli degree Celsius while the temperature returned by the scpi_sensors node is in degree Celsius. Without using the coefficients property and my changes how can I make the cooling map working fine?

Thanks,

Signature · 5 lines

Carlo Caione  |  +39.340.80.30.096  |  Endless
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html