scpi-sensors: Fix SCP sensor readings scale

PATCH Superseded Carlo Caione 5 participants 8 messages Mar 9, 2017 – Mar 15, 2017

Subsystems (3)

Device tree bindings Arm/Amlogic Meson SoC support Hardware monitoring

lore

Share & tools

lore

Carlo Caione

author

2017-03-09 10:17 UTC #0 lore raw

v3 cover scpi-sensors: Fix SCP sensor readings scale

From: Carlo Caione <[email protected]>

The implementation details for SCPI seems to suggest that the sensor readings must be reported by SCP using a well defined scale (millidegree Celsius for temperature, millivolts for voltage, milliamperes for current, microwatts for power and microjoules for energy).

This is also important for the interaction with other subsystems: for example both the thermal sub-system and the hwmon sysfs interface expect the temperature expressed in millidegree Celsius.

Unfortunately since this behaviour is dependent on the firmware implementation there are cases where the sensor readings are reported using a different scale. For example in the Amlogic SoCs the temperature is reported in degree and not millidegree Celsius.

In this patchset we introduce a new DT property scpi,sensors-scale that is used by the scpi-hwmon driver to convert the sensor readings to the expected scale.

v2:
- Added data validation (scale != 0)
- s/unsigned int/u32/ for the array
- Switched to array initialiser notation
- Introduced scpi_scale_reading()

v3:
- scpi_scale_reading marked as static

Carlo Caione (2):
Documentation: bindings: Introduce scpi,sensors-scale
hwmon: (scpi) Fix the scale of SCP sensor readings

 Documentation/devicetree/bindings/arm/arm,scpi.txt | 18 ++++++++++++
 drivers/hwmon/scpi-hwmon.c                         | 34 ++++++++++++++++++++++
 2 files changed, 52 insertions(+)

Signature · 1 line

2.9.3

Carlo Caione

author

2017-03-09 10:17 UTC #1 lore raw

PATCH v3 1/2 Documentation: bindings: Introduce scpi,sensors-scale

From: Carlo Caione <[email protected]>

Document the new property scpi,sensors-scale used by the hwmon-scpi driver to convert the sensor readings to the correct / expected scale.

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

---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Documentation/devicetree/bindings/arm/arm,scpi.txt +18 -0

index 4018319..35c621c 100644
@@ -84,6 +84,24 @@ Required properties:
 			 as used by the firmware. Refer to  platform details
 			 for your implementation for the IDs to use.
 
+Optional properties:
+- scpi,sensors-scale: array of 5 elements defining the unit scale used by the
+                      SCP for the 5 different sensor classes (temperature,
+                      voltage, current, power, energy) such that 1 == unit,
+                      1000 == milli, 1000000 == micro, ...
+
+                      Example:
+                         scpi,sensors-scale = <1 1000 1000 1000000 1000000>
+                      In this case the values reported by SCP are:
+                      -> temperature in degree Celsius
+                      -> voltage in millivolts
+                      -> current in milliamperes
+                      -> power in microwatts
+                      -> energy in microjoules
+
+                      The default value is:
+                         scpi,sensors-scale = <1000 1000 1000 1000000 1000000>
+
 Power domain bindings for the power domains based on SCPI Message Protocol
 ------------------------------------------------------------

Signature · 1 line

2.9.3

Sudeep Holla

2017-03-15 18:20 UTC #2 lore raw

PATCH v3 1/2 Documentation: bindings: Introduce scpi,sensors-scale

Quoted 5 lines · Carlo Caione

From: Carlo Caione <[email protected]>
Document the new property scpi,sensors-scale used by the hwmon-scpi driver to convert the sensor readings to the correct / expected scale.

I am fine with DT bindings if DT maintainers agree with having one but I would like to add couple of points to get their feedback:

1. ARM recognized the drawbacks of SCPI which started informally with ARM Ltd platforms and got adopted by few vendors. It's now being worked on and a much better and scalable replacement protocol is being discussed with vendors now. So future enhancement to this SCPI protocol is unlikely.

2. AmLogic has diverted from base protocol in many ways and most of them are handled with just compatible so far without any need for additional bindings. Can we avoid it ?

So based on these, I prefer not to add more bindings to handle such deviations but make it just AmLogic specific.

Signature · 2 lines

Regards,
Sudeep

Guenter Roeck

maintainer

2017-03-15 19:30 UTC #3 lore raw

PATCH v3 1/2 Documentation: bindings: Introduce scpi,sensors-scale

Quoted 18 lines · Sudeep Holla

Quoted 5 lines · Carlo Caione
From: Carlo Caione <[email protected]>
Document the new property scpi,sensors-scale used by the hwmon-scpi driver to convert the sensor readings to the correct / expected scale.
I am fine with DT bindings if DT maintainers agree with having one but I would like to add couple of points to get their feedback:
1. ARM recognized the drawbacks of SCPI which started informally with ARM Ltd platforms and got adopted by few vendors. It's now being worked on and a much better and scalable replacement protocol is being discussed with vendors now. So future enhancement to this SCPI protocol is unlikely.

Quoted 7 lines

2. AmLogic has diverted from base protocol in many ways and most of them are handled with just compatible so far without any need for additional bindings. Can we avoid it ?
So based on these, I prefer not to add more bindings to handle such deviations but make it just AmLogic specific.

I like the idea.

Guenter

Rob Herring

maintainer

2017-03-15 20:36 UTC #4 lore raw

PATCH v3 1/2 Documentation: bindings: Introduce scpi,sensors-scale

In reply to Sudeep Holla

Quoted 18 lines · Sudeep Holla

Quoted 5 lines · Carlo Caione
From: Carlo Caione <[email protected]>
Document the new property scpi,sensors-scale used by the hwmon-scpi driver to convert the sensor readings to the correct / expected scale.
I am fine with DT bindings if DT maintainers agree with having one but I would like to add couple of points to get their feedback:
1. ARM recognized the drawbacks of SCPI which started informally with ARM Ltd platforms and got adopted by few vendors. It's now being worked on and a much better and scalable replacement protocol is being discussed with vendors now. So future enhancement to this SCPI protocol is unlikely.

Quoted 6 lines

2. AmLogic has diverted from base protocol in many ways and most of them are handled with just compatible so far without any need for additional bindings. Can we avoid it ?
So based on these, I prefer not to add more bindings to handle such deviations but make it just AmLogic specific.

Agreed.

Rob

Martin Blumenstingl

reviewer

2017-03-15 20:56 UTC #5 lore raw

PATCH v3 1/2 Documentation: bindings: Introduce scpi, sensors-scale

In reply to Sudeep Holla

Quoted 18 lines · Sudeep Holla

Quoted 5 lines · Carlo Caione
From: Carlo Caione <[email protected]>
Document the new property scpi,sensors-scale used by the hwmon-scpi driver to convert the sensor readings to the correct / expected scale.
I am fine with DT bindings if DT maintainers agree with having one but I would like to add couple of points to get their feedback:
1. ARM recognized the drawbacks of SCPI which started informally with ARM Ltd platforms and got adopted by few vendors. It's now being worked on and a much better and scalable replacement protocol is being discussed with vendors now. So future enhancement to this SCPI protocol is unlikely.

Quoted 6 lines

2. AmLogic has diverted from base protocol in many ways and most of them are handled with just compatible so far without any need for additional bindings. Can we avoid it ?
So based on these, I prefer not to add more bindings to handle such deviations but make it just AmLogic specific.

could you please share your idea where this "Amlogic specific implementation" fits best?
- in firmware/arm_scpi to keep a contract (for example: millicelsius) to everyone who reads a temperature sensor for example
- or in hwmon/scpi-hwmon

Regards,
Martin

Guenter Roeck

maintainer

2017-03-15 22:40 UTC #6 lore raw

PATCH v3 1/2 Documentation: bindings: Introduce scpi,sensors-scale

Quoted 23 lines · Martin Blumenstingl

Quoted 22 lines · Sudeep Holla
Quoted 5 lines · Carlo Caione
From: Carlo Caione <[email protected]>
Document the new property scpi,sensors-scale used by the hwmon-scpi driver to convert the sensor readings to the correct / expected scale.
I am fine with DT bindings if DT maintainers agree with having one but I would like to add couple of points to get their feedback:
1. ARM recognized the drawbacks of SCPI which started informally with ARM Ltd platforms and got adopted by few vendors. It's now being worked on and a much better and scalable replacement protocol is being discussed with vendors now. So future enhancement to this SCPI protocol is unlikely.
2. AmLogic has diverted from base protocol in many ways and most of them are handled with just compatible so far without any need for additional bindings. Can we avoid it ?

Quoted 8 lines

Quoted 2 lines
So based on these, I prefer not to add more bindings to handle such deviations but make it just AmLogic specific.
could you please share your idea where this "Amlogic specific implementation" fits best?
- in firmware/arm_scpi to keep a contract (for example: millicelsius) to everyone who reads a temperature sensor for example
- or in hwmon/scpi-hwmon

So far the assumption was that it would be in hwmon. I don't mind for it to be elsewhere, but I would think that the presence or non-presence of a devicetree property should not affect or change the implementation.

Thanks,
Guenter

Carlo Caione

author

2017-03-09 10:17 UTC #7 lore raw

PATCH v3 2/2 hwmon: (scpi) Fix the scale of SCP sensor readings

In reply to Carlo Caione

From: Carlo Caione <[email protected]>

This is also important for the interaction with other subsystems: for example both the thermal sub-system and the hwmon sysfs interface expect the temperature expressed in millidegree Celsius.

To take into account this discrepancy and fixup the values reported by SCP a new DT property `scpi,sensors-scale' is introduced and used in this patch by the scpi-hwmon driver to convert the sensor readings to the expected scale.

Acked-by Punit Agrawal <[email protected]>

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

---
 drivers/hwmon/scpi-hwmon.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

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

index 094f948..4a4b8a4 100644
@@ -23,6 +23,7 @@
 #include <linux/thermal.h>
 
 struct sensor_data {
+	unsigned int scale;
 	struct scpi_sensor_info info;
 	struct device_attribute dev_attr_input;
 	struct device_attribute dev_attr_label;

@@ -44,6 +45,22 @@ struct scpi_sensors {
 	const struct attribute_group *groups[2];
 };
 
+static const u32 scpi_scale[] = {
+	[TEMPERATURE]	= 1000,		/* (millicelsius)	*/
+	[VOLTAGE]	= 1000,		/* (millivolts)		*/
+	[CURRENT]	= 1000,		/* (milliamperes)	*/
+	[POWER]		= 1000000,	/* (microwatts)		*/
+	[ENERGY]	= 1000000,	/* (microjoules)	*/
+};
+
+static void scpi_scale_reading(u64 *value, struct sensor_data *sensor)
+{
+	if (scpi_scale[sensor->info.class] != sensor->scale) {
+		*value *= scpi_scale[sensor->info.class];
+		do_div(*value, sensor->scale);
+	}
+}
+
 static int scpi_read_temp(void *dev, int *temp)
 {
 	struct scpi_thermal_zone *zone = dev;

@@ -57,6 +74,8 @@ static int scpi_read_temp(void *dev, int *temp)
 	if (ret)
 		return ret;
 
+	scpi_scale_reading(&value, sensor);
+
 	*temp = value;
 	return 0;
 }

@@ -77,6 +96,8 @@ scpi_show_sensor(struct device *dev, struct device_attribute *attr, char *buf)
 	if (ret)
 		return ret;
 
+	scpi_scale_reading(&value, sensor);
+
 	return sprintf(buf, "%llu\n", value);
 }

@@ -97,6 +118,7 @@ static struct thermal_zone_of_device_ops scpi_sensor_ops = {
 static int scpi_hwmon_probe(struct platform_device *pdev)
 {
 	u16 nr_sensors, i;
+	u32 scale[] = { 1000, 1000, 1000, 1000000, 1000000 };
 	int num_temp = 0, num_volt = 0, num_current = 0, num_power = 0;
 	int num_energy = 0;
 	struct scpi_ops *scpi_ops;

@@ -131,6 +153,16 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 
 	scpi_sensors->scpi_ops = scpi_ops;
 
+	of_property_read_u32_array(dev->of_node, "scpi,sensors-scale",
+				   scale, ARRAY_SIZE(scale));
+
+	for (i = 0; i < ARRAY_SIZE(scale); i++) {
+		if (!scale[i]) {
+			dev_err(dev, "%s: scale cannot be zero (%d)\n", __func__, i);
+			return -EINVAL;
+		}
+	}
+
 	for (i = 0, idx = 0; i < nr_sensors; i++) {
 		struct sensor_data *sensor = &scpi_sensors->data[idx];

@@ -178,6 +210,8 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		sensor->scale = scale[sensor->info.class];
+
 		sensor->dev_attr_input.attr.mode = S_IRUGO;
 		sensor->dev_attr_input.show = scpi_show_sensor;
 		sensor->dev_attr_input.attr.name = sensor->input;

Signature · 1 line

2.9.3