[PATCH v4 0/2] scpi-sensors: Fix SCP sensor readings scale

[PATCH v4 0/2] scpi-sensors: Fix SCP sensor readings scale

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

Sorry for the delay. This is the follow-up on the patches about scpi-sensors
with the fix suggested in [0].

The previous version is at [1].

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 compatible
'amlogic,meson-gxbb-scpi-sensors' 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

v4:
 - Removed new binding and added a new compatible instead

[0] https://patchwork.kernel.org/patch/9612959/
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/492553.html

Carlo Caione (2):
  Documentation: bindings: add amlogic,meson-gxbb-scpi-sensors
    compatible
  hwmon: (scpi) Fix the scale of SCP sensor readings

 Documentation/devicetree/bindings/arm/arm,scpi.txt |  2 +-
 drivers/hwmon/scpi-hwmon.c                         | 54 +++++++++++++++++++---
 2 files changed, 49 insertions(+), 7 deletions(-)

-- 
2.9.3

[PATCH v4 1/2] Documentation: bindings: add amlogic,meson-gxbb-scpi-sensors compatible

[Carlo Caione]

From: Carlo Caione <carlo@endlessm.com>

Introduce a new Amlogic specific compatible to handle how Amlogic reports
the sensor readings from SCPI using a different scale.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
index 4018319..40f50c1 100644
--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
@@ -76,7 +76,7 @@ Sensor bindings for the sensors based on SCPI Message Protocol
 SCPI provides an API to access the various sensors on the SoC.
 
 Required properties:
-- compatible : should be "arm,scpi-sensors".
+- compatible : should be "arm,scpi-sensors" or "amlogic,meson-gxbb-scpi-sensors"
 - #thermal-sensor-cells: should be set to 1. This property follows the
 			 thermal device tree bindings[2].
 
-- 
2.9.3

Re: [PATCH v4 1/2] Documentation: bindings: add amlogic,meson-gxbb-scpi-sensors compatible

[Guenter Roeck]

On 05/30/2017 02:05 AM, Carlo Caione wrote:
> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> Introduce a new Amlogic specific compatible to handle how Amlogic reports
> the sensor readings from SCPI using a different scale.
> 
> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

Formally waiting for DT approval. However, I don't see any problems with it,
so

Acked-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

Not sure what branch this is supposed to go through. For now I assume it
will be arm/arm64.

Thanks,
Guenter

> ---
>   Documentation/devicetree/bindings/arm/arm,scpi.txt | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> index 4018319..40f50c1 100644
> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> @@ -76,7 +76,7 @@ Sensor bindings for the sensors based on SCPI Message Protocol
>   SCPI provides an API to access the various sensors on the SoC.
>   
>   Required properties:
> -- compatible : should be "arm,scpi-sensors".
> +- compatible : should be "arm,scpi-sensors" or "amlogic,meson-gxbb-scpi-sensors"
>   - #thermal-sensor-cells: should be set to 1. This property follows the
>   			 thermal device tree bindings[2].
>   
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] Documentation: bindings: add amlogic,meson-gxbb-scpi-sensors compatible

[Sudeep Holla]

On 02/06/17 14:14, Guenter Roeck wrote:
> On 05/30/2017 02:05 AM, Carlo Caione wrote:
>> From: Carlo Caione <carlo@endlessm.com>
>>
>> Introduce a new Amlogic specific compatible to handle how Amlogic reports
>> the sensor readings from SCPI using a different scale.
>>
>> Signed-off-by: Carlo Caione <carlo@endlessm.com>
> 
> Formally waiting for DT approval. However, I don't see any problems with
> it,
> so
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> 
> Not sure what branch this is supposed to go through. For now I assume it
> will be arm/arm64.
> 

Initially I submitted it via arm-soc. But I think you can take it along
with the driver change in your tree as I don't see any other patch
touching arm,scpi.txt in time around.

-- 
Regards,
Sudeep

Re: [PATCH v4 1/2] Documentation: bindings: add amlogic,meson-gxbb-scpi-sensors compatible

[Rob Herring]

On Tue, May 30, 2017 at 11:05:06AM +0200, Carlo Caione wrote:
> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> Introduce a new Amlogic specific compatible to handle how Amlogic reports
> the sensor readings from SCPI using a different scale.
> 
> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/2] hwmon: (scpi) Fix the scale of SCP sensor readings

[Carlo Caione]

From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

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.

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

Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
---
 drivers/hwmon/scpi-hwmon.c | 54 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
index 094f948..a586480 100644
--- a/drivers/hwmon/scpi-hwmon.c
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -16,6 +16,7 @@
 
 #include <linux/hwmon.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/scpi_protocol.h>
 #include <linux/slab.h>
@@ -23,6 +24,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 +46,30 @@ struct scpi_sensors {
 	const struct attribute_group *groups[2];
 };
 
+static const u32 gxbb_scpi_scale[] = {
+	[TEMPERATURE]	= 1,		/* (celsius)		*/
+	[VOLTAGE]	= 1000,		/* (millivolts)		*/
+	[CURRENT]	= 1000,		/* (milliamperes)	*/
+	[POWER]		= 1000000,	/* (microwatts)		*/
+	[ENERGY]	= 1000000,	/* (microjoules)	*/
+};
+
+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 +83,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 +105,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);
 }
 
@@ -94,14 +124,23 @@ static struct thermal_zone_of_device_ops scpi_sensor_ops = {
 	.get_temp = scpi_read_temp,
 };
 
+static const struct of_device_id scpi_of_match[] = {
+	{.compatible = "arm,scpi-sensors", .data = &scpi_scale},
+	{.compatible = "amlogic,meson-gxbb-scpi-sensors", .data = &gxbb_scpi_scale},
+	{},
+};
+MODULE_DEVICE_TABLE(of, scpi_of_match);
+
 static int scpi_hwmon_probe(struct platform_device *pdev)
 {
 	u16 nr_sensors, i;
+	const u32 *scale;
 	int num_temp = 0, num_volt = 0, num_current = 0, num_power = 0;
 	int num_energy = 0;
 	struct scpi_ops *scpi_ops;
 	struct device *hwdev, *dev = &pdev->dev;
 	struct scpi_sensors *scpi_sensors;
+	const struct of_device_id *of_id;
 	int idx, ret;
 
 	scpi_ops = get_scpi_ops();
@@ -131,6 +170,13 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 
 	scpi_sensors->scpi_ops = scpi_ops;
 
+	of_id = of_match_device(scpi_of_match, &pdev->dev);
+	if (!of_id) {
+		dev_err(&pdev->dev, "Unable to initialize scpi-hwmon data\n");
+		return -ENODEV;
+	}
+	scale = of_id->data;
+
 	for (i = 0, idx = 0; i < nr_sensors; i++) {
 		struct sensor_data *sensor = &scpi_sensors->data[idx];
 
@@ -178,6 +224,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;
@@ -247,12 +295,6 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id scpi_of_match[] = {
-	{.compatible = "arm,scpi-sensors"},
-	{},
-};
-MODULE_DEVICE_TABLE(of, scpi_of_match);
-
 static struct platform_driver scpi_hwmon_platdrv = {
 	.driver = {
 		.name	= "scpi-hwmon",
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] hwmon: (scpi) Fix the scale of SCP sensor readings

[Sudeep Holla]

On 30/05/17 10:05, Carlo Caione wrote:
> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> 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.
> 
> To take into account this discrepancy and fixup the values reported by
> SCP a new compatible 'amlogic,meson-gxbb-scpi-sensors' is introduced and
> used in this patch by the scpi-hwmon driver to convert the sensor
> readings to the expected scale.
> 

Looks good to me

Acked-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v4,2/2] hwmon: (scpi) Fix the scale of SCP sensor readings

[Guenter Roeck]

On Tue, May 30, 2017 at 11:05:07AM +0200, Carlo Caione wrote:
> From: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> 
> 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.
> 
> To take into account this discrepancy and fixup the values reported by
> SCP a new compatible 'amlogic,meson-gxbb-scpi-sensors' is introduced and
> used in this patch by the scpi-hwmon driver to convert the sensor
> readings to the expected scale.
> 
> Signed-off-by: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> Acked-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>

Applied to hwmon-next. If the dt property needs to be updated, we can do
that later.

Thanks,
Guenter

> ---
>  drivers/hwmon/scpi-hwmon.c | 54 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
> index 094f948..a586480 100644
> --- a/drivers/hwmon/scpi-hwmon.c
> +++ b/drivers/hwmon/scpi-hwmon.c
> @@ -16,6 +16,7 @@
>  
>  #include <linux/hwmon.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/scpi_protocol.h>
>  #include <linux/slab.h>
> @@ -23,6 +24,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 +46,30 @@ struct scpi_sensors {
>  	const struct attribute_group *groups[2];
>  };
>  
> +static const u32 gxbb_scpi_scale[] = {
> +	[TEMPERATURE]	= 1,		/* (celsius)		*/
> +	[VOLTAGE]	= 1000,		/* (millivolts)		*/
> +	[CURRENT]	= 1000,		/* (milliamperes)	*/
> +	[POWER]		= 1000000,	/* (microwatts)		*/
> +	[ENERGY]	= 1000000,	/* (microjoules)	*/
> +};
> +
> +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 +83,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 +105,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);
>  }
>  
> @@ -94,14 +124,23 @@ static struct thermal_zone_of_device_ops scpi_sensor_ops = {
>  	.get_temp = scpi_read_temp,
>  };
>  
> +static const struct of_device_id scpi_of_match[] = {
> +	{.compatible = "arm,scpi-sensors", .data = &scpi_scale},
> +	{.compatible = "amlogic,meson-gxbb-scpi-sensors", .data = &gxbb_scpi_scale},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, scpi_of_match);
> +
>  static int scpi_hwmon_probe(struct platform_device *pdev)
>  {
>  	u16 nr_sensors, i;
> +	const u32 *scale;
>  	int num_temp = 0, num_volt = 0, num_current = 0, num_power = 0;
>  	int num_energy = 0;
>  	struct scpi_ops *scpi_ops;
>  	struct device *hwdev, *dev = &pdev->dev;
>  	struct scpi_sensors *scpi_sensors;
> +	const struct of_device_id *of_id;
>  	int idx, ret;
>  
>  	scpi_ops = get_scpi_ops();
> @@ -131,6 +170,13 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>  
>  	scpi_sensors->scpi_ops = scpi_ops;
>  
> +	of_id = of_match_device(scpi_of_match, &pdev->dev);
> +	if (!of_id) {
> +		dev_err(&pdev->dev, "Unable to initialize scpi-hwmon data\n");
> +		return -ENODEV;
> +	}
> +	scale = of_id->data;
> +
>  	for (i = 0, idx = 0; i < nr_sensors; i++) {
>  		struct sensor_data *sensor = &scpi_sensors->data[idx];
>  
> @@ -178,6 +224,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;
> @@ -247,12 +295,6 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id scpi_of_match[] = {
> -	{.compatible = "arm,scpi-sensors"},
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, scpi_of_match);
> -
>  static struct platform_driver scpi_hwmon_platdrv = {
>  	.driver = {
>  		.name	= "scpi-hwmon",
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html