[PATCH 0/9] Platform support for thermal management on Junoe

[PATCH 0/9] Platform support for thermal management on Junoe

[Punit Agrawal]

Hi,

The series adds support for thermal management on ARM Juno development
platform. As part of this development, common infrastructure is added
to support registering cpu cooling devices that work with the power
allocator thermal governor.

Patch 1 extends the CPU nodes binding to provide an optional dynamic
power coefficient which can be used to create a dynamic power model
for the CPUs. This model is used to constrain device power consumption
(using power_allocator governor) when the system is thermally
constrained.

Patches 2-3 extends the cpufreq-dt and arm_big_little driver to
register cpu cooling devices with the dynamic coefficient when
provided.

Patches 4-7 create a hwmon sensor driver for sensors provided by SCPI
firmware. Patch 7 adds support for the temperature sensors to register
with the thermal framework. This allows setting up platform thermals
using OF thermal bindings.

The last two patches add support for the sensors and the thermal zones
in the Juno device tree.

The Juno specific patches depend on SCPI and cpufreq patches[0] from Sudeep.

Thanks,
Punit

[0] http://thread.gmane.org/gmane.linux.power-management.general/61073

Punit Agrawal (9):
  devicetree: bindings: Add optional dynamic-power-coefficient property
  cpufreq-dt: Supply power coefficient when registering cooling devices
  cpufreq: arm_big_little: Add support to register a cpufreq cooling
    device
  Documentation: add DT bindings for ARM SCPI sensors
  firmware: arm_scpi: Extend to support sensors
  hwmon: Support sensors exported via ARM SCP interface
  hwmon: Support registration of thermal zones for SCP temperature
    sensors
  arm64: dts: Add sensor node to Juno dt
  arm64: dts: Create SoC thermal zone for Juno

 Documentation/devicetree/bindings/arm/arm,scpi.txt |  15 ++
 Documentation/devicetree/bindings/arm/cpus.txt     |  15 ++
 arch/arm64/boot/dts/arm/juno-base.dtsi             |   5 +
 arch/arm64/boot/dts/arm/juno.dts                   |  50 ++++
 drivers/cpufreq/arm_big_little.c                   |  52 +++-
 drivers/cpufreq/cpufreq-dt.c                       |   9 +-
 drivers/firmware/arm_scpi.c                        |  60 +++++
 drivers/hwmon/Kconfig                              |   8 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/scpi-hwmon.c                         | 265 +++++++++++++++++++++
 include/linux/scpi_protocol.h                      |  17 ++
 11 files changed, 493 insertions(+), 4 deletions(-)
 create mode 100644 drivers/hwmon/scpi-hwmon.c

-- 
2.1.4

[PATCH 1/9] devicetree: bindings: Add optional dynamic-power-coefficient property

[Punit Agrawal]

The dynamic power consumption of a device is proportional to the
square of voltage (V) and the clock frequency (f). It can be expressed as

Pdyn = dynamic-power-coefficient * V^2 * f.

The dynamic-power-coefficient property represents an indicative
running time dynamic power coefficient in fundamental units of
mw/MHz/uVolt^2 and can be used in the above formula to calculate the
dynamic power in mW.

Signed-off-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index d6b794c..c32bfac 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -239,6 +239,21 @@ nodes to be present and contain the properties described below.
 		Definition: Specifies the syscon node controlling the cpu core
 			    power domains.
 
+	- dynamic-power-coefficient
+		Usage: optional
+		Value type: <prop-encoded-array>
+		Definition: A u32 value that represents an indicative
+			    running time dynamic power coefficient in
+			    fundamental units of mW/MHz/uVolt^2.
+
+			    The dynamic energy consumption of the CPU
+			    is proportional to the square of the
+			    Voltage (V) and the clock frequency (f).
+
+			    Pdyn = dynamic-power-coefficient * V^2 * f
+
+			    where voltage is in uV, frequency is in MHz.
+
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
 	cpus {
-- 
2.1.4

--
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 2/9] cpufreq-dt: Supply power coefficient when registering cooling devices

[Punit Agrawal]

Support registering cooling devices with dynamic power coefficient
where provided by the device tree. This allows OF registered cooling
devices driver to be used with the power_allocator thermal governor.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/cpufreq-dt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 528a82bf..1d1f61a 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -344,8 +344,13 @@ static void cpufreq_ready(struct cpufreq_policy *policy)
 	 * thermal DT code takes care of matching them.
 	 */
 	if (of_find_property(np, "#cooling-cells", NULL)) {
-		priv->cdev = of_cpufreq_cooling_register(np,
-							 policy->related_cpus);
+		u32 power_coefficient = 0;
+
+		of_property_read_u32(np, "dynamic-power-coefficient",
+				     &power_coefficient);
+
+		priv->cdev = of_cpufreq_power_cooling_register(np,
+				policy->related_cpus, power_coefficient, NULL);
 		if (IS_ERR(priv->cdev)) {
 			dev_err(priv->cpu_dev,
 				"running cpufreq without cooling device: %ld\n",
-- 
2.1.4

[PATCH 3/9] cpufreq: arm_big_little: Add support to register a cpufreq cooling device

[Punit Agrawal]

Register passive cooling devices when initialising cpufreq on
big.LITTLE systems. If the device tree provides a dynamic power
coefficient for the CPUs then the bound cooling device will support
the extensions that allow it to be used with all the existing thermal
governors including the power allocator governor.

A cooling device will be created per individual frequency domain and
can be bound to thermal zones via the thermal DT bindings.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/cpufreq/arm_big_little.c | 52 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
index f1e42f8..72a2777 100644
--- a/drivers/cpufreq/arm_big_little.c
+++ b/drivers/cpufreq/arm_big_little.c
@@ -23,6 +23,7 @@
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_cooling.h>
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -55,6 +56,10 @@ static bool bL_switching_enabled;
 #define ACTUAL_FREQ(cluster, freq)  ((cluster == A7_CLUSTER) ? freq << 1 : freq)
 #define VIRT_FREQ(cluster, freq)    ((cluster == A7_CLUSTER) ? freq >> 1 : freq)
 
+struct private_data {
+	struct thermal_cooling_device *cdev;
+};
+
 static struct cpufreq_arm_bL_ops *arm_bL_ops;
 static struct clk *clk[MAX_CLUSTERS];
 static struct cpufreq_frequency_table *freq_table[MAX_CLUSTERS + 1];
@@ -440,6 +445,7 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 {
 	u32 cur_cluster = cpu_to_cluster(policy->cpu);
 	struct device *cpu_dev;
+	struct private_data *priv;
 	int ret;
 
 	cpu_dev = get_cpu_device(policy->cpu);
@@ -457,9 +463,15 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 	if (ret) {
 		dev_err(cpu_dev, "CPU %d, cluster: %d invalid freq table\n",
 				policy->cpu, cur_cluster);
-		put_cluster_clk_and_freq_table(cpu_dev);
-		return ret;
+		goto out_free_cpufreq_table;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto out_free_cpufreq_table;
 	}
+	policy->driver_data = priv;
 
 	if (cur_cluster < MAX_CLUSTERS) {
 		int cpu;
@@ -484,12 +496,19 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy)
 
 	dev_info(cpu_dev, "%s: CPU %d initialized\n", __func__, policy->cpu);
 	return 0;
+
+out_free_cpufreq_table:
+	put_cluster_clk_and_freq_table(cpu_dev);
+
+	return ret;
 }
 
 static int bL_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct device *cpu_dev;
+	struct private_data *priv = policy->driver_data;
 
+	cpufreq_cooling_unregister(priv->cdev);
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
 		pr_err("%s: failed to get cpu%d device\n", __func__,
@@ -498,11 +517,39 @@ static int bL_cpufreq_exit(struct cpufreq_policy *policy)
 	}
 
 	put_cluster_clk_and_freq_table(cpu_dev);
+	kfree(priv);
 	dev_dbg(cpu_dev, "%s: Exited, cpu: %d\n", __func__, policy->cpu);
 
 	return 0;
 }
 
+static void bL_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct device *cpu_dev = get_cpu_device(policy->cpu);
+	struct device_node *np = of_node_get(cpu_dev->of_node);
+	struct private_data *priv = policy->driver_data;
+
+	if (WARN_ON(!np))
+		return;
+
+	if (of_find_property(np, "#cooling-cells", NULL)) {
+		u32 power_coefficient = 0;
+
+		of_property_read_u32(np, "dynamic-power-coefficient",
+				     &power_coefficient);
+
+		priv->cdev = of_cpufreq_power_cooling_register(np,
+				policy->related_cpus, power_coefficient, NULL);
+		if (IS_ERR(priv->cdev)) {
+			dev_err(cpu_dev,
+				"running cpufreq without cooling device: %ld\n",
+				PTR_ERR(priv->cdev));
+			priv->cdev = NULL;
+		}
+	}
+	of_node_put(np);
+}
+
 static struct cpufreq_driver bL_cpufreq_driver = {
 	.name			= "arm-big-little",
 	.flags			= CPUFREQ_STICKY |
@@ -513,6 +560,7 @@ static struct cpufreq_driver bL_cpufreq_driver = {
 	.get			= bL_cpufreq_get_rate,
 	.init			= bL_cpufreq_init,
 	.exit			= bL_cpufreq_exit,
+	.ready			= bL_cpufreq_ready,
 	.attr			= cpufreq_generic_attr,
 };
 
-- 
2.1.4

[PATCH 4/9] Documentation: add DT bindings for ARM SCPI sensors

[Punit Agrawal]

The System Control Processor (SCP) provides access to SoC sensors via
the System Control and Power Interface (SCPI) Message Protocol. Add
bindings to allow probing of these sensors. Also support referencing
of the sensors for setting up thermal zones via the thermal DT
bindings.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
index e3757bc..5cb2b18 100644
--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
@@ -74,8 +74,18 @@ Required sub-node properties:
 - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
 	       shared memory on Juno platforms
 
+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".
+- #thermal-sensor-cells: should be set to 1. This property follows the
+			 thermal device tree bindings[2].
+
 [0] http://community.arm.com/servlet/JiveServlet/download/8401-45-18326/DUI0922B_scp_message_interface.pdf
 [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+[2] Documentation/devicetree/bindings/thermal/thermal.txt
 
 Example:
 
@@ -124,6 +134,11 @@ scpi_protocol: scpi@2e000000 {
 			clock-output-names = "pxlclk0", "pxlclk1";
 		};
 	};
+
+	scpi_sensors0: sensors {
+		compatible = "arm,scpi-sensors";
+		#thermal-sensor-cells = <1>;
+	};
 };
 
 cpu@0 {
-- 
2.1.4

[PATCH 5/9] firmware: arm_scpi: Extend to support sensors

[Punit Agrawal]

ARM System Control Processor (SCP) provides an API to query and use
the sensors available in the system. Extend the SCPI driver to support
 sensor messages.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scpi.c   | 60 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/scpi_protocol.h | 17 ++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index a70b7ba..478c5c5 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -219,6 +219,21 @@ struct dvfs_set {
 	u8 index;
 } __packed;
 
+struct sensor_capabilities {
+	__le16 sensors;
+} __packed;
+
+struct _scpi_sensor_info {
+	__le16 sensor_id;
+	u8 class;
+	u8 trigger_type;
+	char name[20];
+};
+
+struct sensor_value {
+	__le32 val;
+} __packed;
+
 static struct scpi_drvinfo *scpi_info;
 
 static int scpi_linux_errmap[SCPI_ERR_MAX] = {
@@ -481,6 +496,48 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 	return info;
 }
 
+static int scpi_sensor_get_capability(u16 *sensors)
+{
+	struct sensor_capabilities cap_buf;
+	int ret;
+
+	ret = scpi_send_message(SCPI_CMD_SENSOR_CAPABILITIES, NULL, 0, &cap_buf,
+				sizeof(cap_buf));
+	if (!ret)
+		*sensors = le16_to_cpu(cap_buf.sensors);
+
+	return ret;
+}
+
+static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
+{
+	__le16 id = cpu_to_le16(sensor_id);
+	struct _scpi_sensor_info _info;
+	int ret;
+
+	ret = scpi_send_message(SCPI_CMD_SENSOR_INFO, &id,
+				 sizeof(id), &_info, sizeof(_info));
+	if (!ret) {
+		memcpy(info, &_info, sizeof(*info));
+		info->sensor_id = le16_to_cpu(_info.sensor_id);
+	}
+
+	return ret;
+}
+
+int scpi_sensor_get_value(u16 sensor, u32 *val)
+{
+	struct sensor_value buf;
+	int ret;
+
+	ret = scpi_send_message(SCPI_CMD_SENSOR_VALUE, &sensor, sizeof(sensor),
+				&buf, sizeof(buf));
+	if (!ret)
+		*val = le32_to_cpu(buf.val);
+
+	return ret;
+}
+
 static struct scpi_ops scpi_ops = {
 	.get_version = scpi_get_version,
 	.clk_get_range = scpi_clk_get_range,
@@ -489,6 +546,9 @@ static struct scpi_ops scpi_ops = {
 	.dvfs_get_idx = scpi_dvfs_get_idx,
 	.dvfs_set_idx = scpi_dvfs_set_idx,
 	.dvfs_get_info = scpi_dvfs_get_info,
+	.sensor_get_capability = scpi_sensor_get_capability,
+	.sensor_get_info = scpi_sensor_get_info,
+	.sensor_get_value = scpi_sensor_get_value,
 };
 
 struct scpi_ops *get_scpi_ops(void)
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index e7169cd..80af3cd 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -28,6 +28,20 @@ struct scpi_dvfs_info {
 	struct scpi_opp *opps;
 };
 
+enum scpi_sensor_class {
+	TEMPERATURE,
+	VOLTAGE,
+	CURRENT,
+	POWER,
+};
+
+struct scpi_sensor_info {
+	u16 sensor_id;
+	u8 class;
+	u8 trigger_type;
+	char name[20];
+} __packed;
+
 /**
  * struct scpi_ops - represents the various operations provided
  *	by SCP through SCPI message protocol
@@ -52,6 +66,9 @@ struct scpi_ops {
 	int (*dvfs_get_idx)(u8);
 	int (*dvfs_set_idx)(u8, u8);
 	struct scpi_dvfs_info *(*dvfs_get_info)(u8);
+	int (*sensor_get_capability)(u16 *sensors);
+	int (*sensor_get_info)(u16 sensor_id, struct scpi_sensor_info *);
+	int (*sensor_get_value)(u16, u32 *);
 };
 
 #if IS_ENABLED(CONFIG_ARM_SCPI_PROTOCOL)
-- 
2.1.4

[PATCH 6/9] hwmon: Support sensors exported via ARM SCP interface

[Punit Agrawal]

Create a driver to add support for SoC sensors exported by the System
Control Processor (SCP) via the System Control and Power Interface
(SCPI). The supported sensor types is one of voltage, temperature,
current, and power.

The sensor labels and values provided by the SCP are exported via the
hwmon sysfs interface.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/hwmon/Kconfig      |   8 ++
 drivers/hwmon/Makefile     |   1 +
 drivers/hwmon/scpi-hwmon.c | 212 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+)
 create mode 100644 drivers/hwmon/scpi-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4943c3c..f5e0862 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1551,6 +1551,14 @@ config SENSORS_VEXPRESS
 	  the ARM Ltd's Versatile Express platform. It can provide wide
 	  range of information like temperature, power, energy.
 
+config SENSORS_ARM_SCPI
+	tristate "ARM SCPI Sensors"
+	depends on ARM_SCPI_PROTOCOL
+	help
+	  This driver provides support for hardware sensors available on
+	  the ARM Ltd's SCP based platforms. It can provide temperature
+	  and voltage for range of devices on the SoC.
+
 config SENSORS_VIA_CPUTEMP
 	tristate "VIA CPU temperature sensor"
 	depends on X86
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 8aba87f..4961710 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
 obj-$(CONFIG_SENSORS_V2M_JUNO)	+= v2m-juno.o
 obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress.o
+obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
 obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
new file mode 100644
index 0000000..dd0a6f1
--- /dev/null
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -0,0 +1,212 @@
+/*
+ * System Control and Power Interface(SCPI) based hwmon sensor driver
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Punit Agrawal <punit.agrawal@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/scpi_protocol.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+static struct scpi_ops *scpi_ops;
+
+struct sensor_dev {
+	struct scpi_sensor_info info;
+	struct device_attribute dev_attr_input;
+	struct device_attribute dev_attr_label;
+	char input[20];
+	char label[20];
+};
+
+struct scpi_sensors {
+	int num_volt;
+	int num_temp;
+	int num_current;
+	int num_power;
+	struct sensor_dev *device;
+	struct device *hwdev;
+};
+struct scpi_sensors scpi_sensors;
+
+static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
+{
+	int ret;
+
+	ret = scpi_ops->sensor_get_value(sensor->info.sensor_id, value);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/* hwmon callback functions */
+static ssize_t
+scpi_hwmon_show_sensor(struct device *dev,
+		       struct device_attribute *attr, char *buf)
+{
+	struct sensor_dev *sensor;
+	u32 value;
+	int ret;
+
+	sensor = container_of(attr, struct sensor_dev, dev_attr_input);
+
+	ret = scpi_read_sensor(sensor, &value);
+	if (ret) {
+		dev_warn(dev, "error (ret=%d) reading sensor (id=%u) value\n",
+			 ret, sensor->info.sensor_id);
+		return 0;
+	}
+
+	return sprintf(buf, "%u\n", value);
+}
+
+static ssize_t
+scpi_hwmon_show_label(struct device *dev,
+		      struct device_attribute *attr, char *buf)
+{
+	struct sensor_dev *sensor;
+
+	sensor = container_of(attr, struct sensor_dev, dev_attr_label);
+
+	return sprintf(buf, "%s\n", sensor->info.name);
+}
+
+struct attribute *scpi_attrs[24] = { 0 };
+struct attribute_group scpi_group;
+const struct attribute_group *scpi_groups[2] = { 0 };
+
+static int scpi_hwmon_probe(struct platform_device *pdev)
+{
+	u16 sensors, i;
+	int ret;
+
+	scpi_ops = get_scpi_ops();
+	if (!scpi_ops)
+		return -EPROBE_DEFER;
+
+	ret = scpi_ops->sensor_get_capability(&sensors);
+	if (ret)
+		return ret;
+
+	scpi_sensors.device = devm_kcalloc(&pdev->dev, sensors,
+				   sizeof(*scpi_sensors.device), GFP_KERNEL);
+	if (!scpi_sensors.device)
+		return -ENOMEM;
+
+	dev_info(&pdev->dev, "Found %d sensors\n", sensors);
+	for (i = 0; i < sensors; i++) {
+		struct sensor_dev *dev = &scpi_sensors.device[i];
+
+		ret = scpi_ops->sensor_get_info(i, &dev->info);
+		if (ret) {
+			dev_info(&pdev->dev,
+				 "Error ret=%d when querying for sensor %d\n",
+				 ret, i);
+			return ret;
+		}
+
+		dev_info(&pdev->dev, "Probed \'%s\' sensor.\n",
+			 dev->info.name);
+
+		switch (dev->info.class) {
+		case TEMPERATURE:
+			snprintf(dev->input, 20,
+				 "temp%d_input", scpi_sensors.num_temp);
+			snprintf(dev->label, 20,
+				 "temp%d_label", scpi_sensors.num_temp);
+			scpi_sensors.num_temp++;
+			break;
+		case VOLTAGE:
+			snprintf(dev->input, 20,
+				 "in%d_input", scpi_sensors.num_volt);
+			snprintf(dev->label, 20,
+				 "in%d_label", scpi_sensors.num_volt);
+			scpi_sensors.num_volt++;
+			break;
+		case CURRENT:
+			snprintf(dev->input, 20,
+				 "curr%d_input", scpi_sensors.num_current);
+			snprintf(dev->label, 20,
+				 "curr%d_label", scpi_sensors.num_current);
+			scpi_sensors.num_current++;
+			break;
+		case POWER:
+			snprintf(dev->input, 20,
+				 "power%d_input", scpi_sensors.num_power);
+			snprintf(dev->label, 20,
+				 "power%d_label", scpi_sensors.num_power);
+			scpi_sensors.num_power++;
+			break;
+		default:
+			break;
+		}
+
+		dev->dev_attr_input.attr.mode = S_IRUGO;
+		dev->dev_attr_input.store = NULL;
+		dev->dev_attr_input.show = scpi_hwmon_show_sensor;
+		dev->dev_attr_input.attr.name = dev->input;
+
+		dev->dev_attr_label.attr.mode = S_IRUGO;
+		dev->dev_attr_label.store = NULL;
+		dev->dev_attr_label.show = scpi_hwmon_show_label;
+		dev->dev_attr_label.attr.name = dev->label;
+
+		scpi_attrs[i << 1] = &dev->dev_attr_input.attr;
+		scpi_attrs[(i << 1) + 1] = &dev->dev_attr_label.attr;
+	}
+
+	scpi_group.attrs = scpi_attrs;
+	scpi_groups[0] = &scpi_group;
+
+	scpi_sensors.hwdev = devm_hwmon_device_register_with_groups(&pdev->dev,
+				    "scpi_sensors", &scpi_sensors, scpi_groups);
+
+	if (IS_ERR(scpi_sensors.hwdev)) {
+		dev_err(&pdev->dev, "Error registering scpi_sensors hwmon device\n");
+		return PTR_ERR(scpi_sensors.hwdev);
+	}
+
+	dev_info(&pdev->dev, "SCPI hwmon driver initialised.\n");
+	return 0;
+}
+
+static int scpi_hwmon_remove(struct platform_device *pdev)
+{
+	scpi_ops = NULL;
+	return 0;
+}
+
+static const struct of_device_id scpi_of_match[] = {
+	{.compatible = "arm,scpi-sensors"},
+	{},
+};
+
+static struct platform_driver scpi_hwmon_platdrv = {
+	.driver = {
+		.name	= "scpi-hwmon",
+		.owner	= THIS_MODULE,
+		.of_match_table = scpi_of_match,
+	},
+	.probe		= scpi_hwmon_probe,
+	.remove	= scpi_hwmon_remove,
+};
+module_platform_driver(scpi_hwmon_platdrv);
+
+MODULE_AUTHOR("Punit Agrawal <punit.agrawal@arm.com>");
+MODULE_DESCRIPTION("ARM SCPI HWMON interface driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH 6/9] hwmon: Support sensors exported via ARM SCP interface

[Guenter Roeck]

On 07/22/2015 07:02 AM, Punit Agrawal wrote:
> Create a driver to add support for SoC sensors exported by the System
> Control Processor (SCP) via the System Control and Power Interface
> (SCPI). The supported sensor types is one of voltage, temperature,
> current, and power.
>
> The sensor labels and values provided by the SCP are exported via the
> hwmon sysfs interface.
>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/hwmon/Kconfig      |   8 ++
>   drivers/hwmon/Makefile     |   1 +
>   drivers/hwmon/scpi-hwmon.c | 212 +++++++++++++++++++++++++++++++++++++++++++++

Please also provide Documentation/hwmon/scpi-hwmon.

>   3 files changed, 221 insertions(+)
>   create mode 100644 drivers/hwmon/scpi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4943c3c..f5e0862 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1551,6 +1551,14 @@ config SENSORS_VEXPRESS
>   	  the ARM Ltd's Versatile Express platform. It can provide wide
>   	  range of information like temperature, power, energy.
>
> +config SENSORS_ARM_SCPI
> +	tristate "ARM SCPI Sensors"
> +	depends on ARM_SCPI_PROTOCOL
> +	help
> +	  This driver provides support for hardware sensors available on
> +	  the ARM Ltd's SCP based platforms. It can provide temperature

can provide or provides ?
> +	  and voltage for range of devices on the SoC.

current, power ?

> +
>   config SENSORS_VIA_CPUTEMP
>   	tristate "VIA CPU temperature sensor"
>   	depends on X86
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 8aba87f..4961710 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -150,6 +150,7 @@ obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>   obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
>   obj-$(CONFIG_SENSORS_V2M_JUNO)	+= v2m-juno.o
>   obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress.o
> +obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>   obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>   obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>   obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
> new file mode 100644
> index 0000000..dd0a6f1
> --- /dev/null
> +++ b/drivers/hwmon/scpi-hwmon.c
> @@ -0,0 +1,212 @@
> +/*
> + * System Control and Power Interface(SCPI) based hwmon sensor driver
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + * Punit Agrawal <punit.agrawal@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Not needed since there are no pr_xxx() messages in this driver.

> +
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/scpi_protocol.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +static struct scpi_ops *scpi_ops;
> +
This variable should be kept in a local data structure (scpi_sensors).

> +struct sensor_dev {
> +	struct scpi_sensor_info info;
> +	struct device_attribute dev_attr_input;
> +	struct device_attribute dev_attr_label;
> +	char input[20];
> +	char label[20];
> +};
> +
> +struct scpi_sensors {
> +	int num_volt;
> +	int num_temp;
> +	int num_current;
> +	int num_power;

num_volt, num_temp, num_current, and num_power are not used
outside the probe function and are thus not needed in this structure.

> +	struct sensor_dev *device;

'device' is a bit misleading here. It might be better to use
something like sensor_data (and maybe struct sensor_data),
or just 'data'.

> +	struct device *hwdev;

hwdev is not used and thus not needed in this structure.

> +};
> +struct scpi_sensors scpi_sensors;

This variable should be allocated in the probe function (and either case not be global).

> +
> +static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
> +{
> +	int ret;
> +
> +	ret = scpi_ops->sensor_get_value(sensor->info.sensor_id, value);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

This can be reduced to
	return scpi_ops->sensor_get_value(sensor->info.sensor_id, value);

which makes me wonder if the function is that useful to start with,
but I leave that up to you.

> +}
> +
> +/* hwmon callback functions */
> +static ssize_t
> +scpi_hwmon_show_sensor(struct device *dev,
> +		       struct device_attribute *attr, char *buf)

_hwmon in the function names do not really add value.

> +{
> +	struct sensor_dev *sensor;
> +	u32 value;
> +	int ret;
> +
> +	sensor = container_of(attr, struct sensor_dev, dev_attr_input);
> +
> +	ret = scpi_read_sensor(sensor, &value);
> +	if (ret) {
> +		dev_warn(dev, "error (ret=%d) reading sensor (id=%u) value\n",
> +			 ret, sensor->info.sensor_id);
> +		return 0;

Please no error message (it can get noisy), and return the error.

> +	}
> +
> +	return sprintf(buf, "%u\n", value);
> +}
> +
> +static ssize_t
> +scpi_hwmon_show_label(struct device *dev,
> +		      struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_dev *sensor;
> +
> +	sensor = container_of(attr, struct sensor_dev, dev_attr_label);
> +
> +	return sprintf(buf, "%s\n", sensor->info.name);
> +}
> +
> +struct attribute *scpi_attrs[24] = { 0 };

This creates an unnecessary (hard) limit on the number of supported
sensors (12). I would suggest to allocate the array based on the number
of reported sensors.

> +struct attribute_group scpi_group;
> +const struct attribute_group *scpi_groups[2] = { 0 };

Please move those variables into struct scpi_sensors.

Note that you don't have to initialize globale variables with 0,
and that globale variables should be static. It does not matter here
since you should not have those variables to start with (they should
be in struct scpi_sensors and be allocated), just mentioning it.

> +
> +static int scpi_hwmon_probe(struct platform_device *pdev)
> +{
> +	u16 sensors, i;
> +	int ret;
> +
> +	scpi_ops = get_scpi_ops();
> +	if (!scpi_ops)
> +		return -EPROBE_DEFER;
> +
Is this appropriate ? I am a bit concerned that we might get a lot
of "requests probe deferral" if the underlying infrastructure is
not there at all.

> +	ret = scpi_ops->sensor_get_capability(&sensors);
> +	if (ret)
> +		return ret;
> +
> +	scpi_sensors.device = devm_kcalloc(&pdev->dev, sensors,
> +				   sizeof(*scpi_sensors.device), GFP_KERNEL);

What if sensors == 0 ?

It might be useful to introduce a local variable
	struct device *dev = &pdev->dev;
and use it in this function.

> +	if (!scpi_sensors.device)
> +		return -ENOMEM;
> +
> +	dev_info(&pdev->dev, "Found %d sensors\n", sensors);

Please drop this message and the other info messages below (too noisy).
If you think you need those messages for debugging, make it dev_dbg().

> +	for (i = 0; i < sensors; i++) {
> +		struct sensor_dev *dev = &scpi_sensors.device[i];
> +
> +		ret = scpi_ops->sensor_get_info(i, &dev->info);
> +		if (ret) {
> +			dev_info(&pdev->dev,
> +				 "Error ret=%d when querying for sensor %d\n",
> +				 ret, i);

If this is an error, it should be dev_err().

> +			return ret;
> +		}
> +
> +		dev_info(&pdev->dev, "Probed \'%s\' sensor.\n",
> +			 dev->info.name);
> +
> +		switch (dev->info.class) {
> +		case TEMPERATURE:
> +			snprintf(dev->input, 20,

Can you use sizeof() to determine the string length ?

> +				 "temp%d_input", scpi_sensors.num_temp);
> +			snprintf(dev->label, 20,
> +				 "temp%d_label", scpi_sensors.num_temp);
> +			scpi_sensors.num_temp++;

temp sensor numbering starts with 1.

> +			break;
> +		case VOLTAGE:
> +			snprintf(dev->input, 20,
> +				 "in%d_input", scpi_sensors.num_volt);
> +			snprintf(dev->label, 20,
> +				 "in%d_label", scpi_sensors.num_volt);
> +			scpi_sensors.num_volt++;
> +			break;
> +		case CURRENT:
> +			snprintf(dev->input, 20,
> +				 "curr%d_input", scpi_sensors.num_current);
> +			snprintf(dev->label, 20,
> +				 "curr%d_label", scpi_sensors.num_current);
> +			scpi_sensors.num_current++;

current sensor numbering starts with 1.

> +			break;
> +		case POWER:
> +			snprintf(dev->input, 20,
> +				 "power%d_input", scpi_sensors.num_power);
> +			snprintf(dev->label, 20,
> +				 "power%d_label", scpi_sensors.num_power);
> +			scpi_sensors.num_power++;

power sensor numbering starts with 1.

> +			break;
> +		default:
> +			break;
> +		}
> +
> +		dev->dev_attr_input.attr.mode = S_IRUGO;
> +		dev->dev_attr_input.store = NULL;

kcalloc() initializes the allocated memory, so this assignment is unnecessary.

> +		dev->dev_attr_input.show = scpi_hwmon_show_sensor;
> +		dev->dev_attr_input.attr.name = dev->input;
> +
> +		dev->dev_attr_label.attr.mode = S_IRUGO;
> +		dev->dev_attr_label.store = NULL;

Same as above.

> +		dev->dev_attr_label.show = scpi_hwmon_show_label;
> +		dev->dev_attr_label.attr.name = dev->label;
> +
> +		scpi_attrs[i << 1] = &dev->dev_attr_input.attr;
> +		scpi_attrs[(i << 1) + 1] = &dev->dev_attr_label.attr;

You need to call sysfs_attr_init() for each of the attributes.

> +	}
> +
> +	scpi_group.attrs = scpi_attrs;
> +	scpi_groups[0] = &scpi_group;
> +
> +	scpi_sensors.hwdev = devm_hwmon_device_register_with_groups(&pdev->dev,
> +				    "scpi_sensors", &scpi_sensors, scpi_groups);

Please allocate scpi_sensors and pass a pointer to it.
All local variables should be kept in scpi_sensors (and can then be accessed
with dev_get_drvdata).

> +
> +	if (IS_ERR(scpi_sensors.hwdev)) {
> +		dev_err(&pdev->dev, "Error registering scpi_sensors hwmon device\n");
> +		return PTR_ERR(scpi_sensors.hwdev);
> +	}
> +
> +	dev_info(&pdev->dev, "SCPI hwmon driver initialised.\n");

Please consider just returning
	return PTR_ERR_OR_ZERO(hwdev);
without the noise.

> +	return 0;
> +}
> +
> +static int scpi_hwmon_remove(struct platform_device *pdev)
> +{
> +	scpi_ops = NULL;
> +	return 0;
> +}

Not needed.

> +
> +static const struct of_device_id scpi_of_match[] = {
> +	{.compatible = "arm,scpi-sensors"},
> +	{},
> +};
> +
> +static struct platform_driver scpi_hwmon_platdrv = {
> +	.driver = {
> +		.name	= "scpi-hwmon",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = scpi_of_match,
> +	},
> +	.probe		= scpi_hwmon_probe,
> +	.remove	= scpi_hwmon_remove,
> +};
> +module_platform_driver(scpi_hwmon_platdrv);
> +
> +MODULE_AUTHOR("Punit Agrawal <punit.agrawal@arm.com>");
> +MODULE_DESCRIPTION("ARM SCPI HWMON interface driver");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH 6/9] hwmon: Support sensors exported via ARM SCP interface

[Punit Agrawal]

Hi Guenter,

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

> On 07/22/2015 07:02 AM, Punit Agrawal wrote:
>> Create a driver to add support for SoC sensors exported by the System
>> Control Processor (SCP) via the System Control and Power Interface
>> (SCPI). The supported sensor types is one of voltage, temperature,
>> current, and power.
>>
>> The sensor labels and values provided by the SCP are exported via the
>> hwmon sysfs interface.
>>
>> Signed-off-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
>> Cc: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
>> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>> Cc: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>> ---
>>   drivers/hwmon/Kconfig      |   8 ++
>>   drivers/hwmon/Makefile     |   1 +
>>   drivers/hwmon/scpi-hwmon.c | 212 +++++++++++++++++++++++++++++++++++++++++++++
>
> Please also provide Documentation/hwmon/scpi-hwmon.
>

Added documentation in the above location.

>>   3 files changed, 221 insertions(+)
>>   create mode 100644 drivers/hwmon/scpi-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 4943c3c..f5e0862 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1551,6 +1551,14 @@ config SENSORS_VEXPRESS
>>   	  the ARM Ltd's Versatile Express platform. It can provide wide
>>   	  range of information like temperature, power, energy.
>>
>> +config SENSORS_ARM_SCPI
>> +	tristate "ARM SCPI Sensors"
>> +	depends on ARM_SCPI_PROTOCOL
>> +	help
>> +	  This driver provides support for hardware sensors available on
>> +	  the ARM Ltd's SCP based platforms. It can provide temperature
>
> can provide or provides ?

The actual sensors exported are dependent on the platform and the
firmware. But the above wording can be improved. I've updated it to

  This driver provides support for temperature, voltage, current
  and power sensors available on ARM Ltd's SCP based platforms. The
  actual number and type of sensors exported depend the platform.


>> +	  and voltage for range of devices on the SoC.
>
> current, power ?
>
>> +
>>   config SENSORS_VIA_CPUTEMP
>>   	tristate "VIA CPU temperature sensor"
>>   	depends on X86
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 8aba87f..4961710 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -150,6 +150,7 @@ obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>>   obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
>>   obj-$(CONFIG_SENSORS_V2M_JUNO)	+= v2m-juno.o
>>   obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress.o
>> +obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>>   obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>   obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>>   obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
>> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
>> new file mode 100644
>> index 0000000..dd0a6f1
>> --- /dev/null
>> +++ b/drivers/hwmon/scpi-hwmon.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + * System Control and Power Interface(SCPI) based hwmon sensor driver
>> + *
>> + * Copyright (C) 2015 ARM Ltd.
>> + * Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> Not needed since there are no pr_xxx() messages in this driver.
>

Dropped it.

>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/scpi_protocol.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +
>> +static struct scpi_ops *scpi_ops;
>> +
> This variable should be kept in a local data structure (scpi_sensors).
>

Moved now.

>> +struct sensor_dev {
>> +	struct scpi_sensor_info info;
>> +	struct device_attribute dev_attr_input;
>> +	struct device_attribute dev_attr_label;
>> +	char input[20];
>> +	char label[20];
>> +};
>> +
>> +struct scpi_sensors {
>> +	int num_volt;
>> +	int num_temp;
>> +	int num_current;
>> +	int num_power;
>
> num_volt, num_temp, num_current, and num_power are not used
> outside the probe function and are thus not needed in this structure.
>

Moved to the probe function.

>> +	struct sensor_dev *device;
>
> 'device' is a bit misleading here. It might be better to use
> something like sensor_data (and maybe struct sensor_data),
> or just 'data'.
>

Now renamed as -

	struct sensor_data *data

That makes it a lot more readable elsewhere as well. Thanks.

>> +	struct device *hwdev;
>
> hwdev is not used and thus not needed in this structure.
>

Moved to the probe function.

>> +};
>> +struct scpi_sensors scpi_sensors;
>
> This variable should be allocated in the probe function (and either case not be global).
>
>> +
>> +static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
>> +{
>> +	int ret;
>> +
>> +	ret = scpi_ops->sensor_get_value(sensor->info.sensor_id, value);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>
> This can be reduced to
> 	return scpi_ops->sensor_get_value(sensor->info.sensor_id, value);
>
> which makes me wonder if the function is that useful to start with,
> but I leave that up to you.
>

I added the indirection to use common code for reading temperatures for
sensors connected to the thermal zones in the next patch. But you're
right - the function can be dropped without any loss.

>> +}
>> +
>> +/* hwmon callback functions */
>> +static ssize_t
>> +scpi_hwmon_show_sensor(struct device *dev,
>> +		       struct device_attribute *attr, char *buf)
>
> _hwmon in the function names do not really add value.
>

Dropped.

>> +{
>> +	struct sensor_dev *sensor;
>> +	u32 value;
>> +	int ret;
>> +
>> +	sensor = container_of(attr, struct sensor_dev, dev_attr_input);
>> +
>> +	ret = scpi_read_sensor(sensor, &value);
>> +	if (ret) {
>> +		dev_warn(dev, "error (ret=%d) reading sensor (id=%u) value\n",
>> +			 ret, sensor->info.sensor_id);
>> +		return 0;
>
> Please no error message (it can get noisy), and return the error.
>

Done.

>> +	}
>> +
>> +	return sprintf(buf, "%u\n", value);
>> +}
>> +
>> +static ssize_t
>> +scpi_hwmon_show_label(struct device *dev,
>> +		      struct device_attribute *attr, char *buf)
>> +{
>> +	struct sensor_dev *sensor;
>> +
>> +	sensor = container_of(attr, struct sensor_dev, dev_attr_label);
>> +
>> +	return sprintf(buf, "%s\n", sensor->info.name);
>> +}
>> +
>> +struct attribute *scpi_attrs[24] = { 0 };
>
> This creates an unnecessary (hard) limit on the number of supported
> sensors (12). I would suggest to allocate the array based on the number
> of reported sensors.
>
>> +struct attribute_group scpi_group;
>> +const struct attribute_group *scpi_groups[2] = { 0 };
>
> Please move those variables into struct scpi_sensors.
>

The attributes are now dynamically allocated and the above structures
have been moved to struct scpi_sensors.

> Note that you don't have to initialize globale variables with 0,
> and that globale variables should be static. It does not matter here
> since you should not have those variables to start with (they should
> be in struct scpi_sensors and be allocated), just mentioning it.
>

I'll keep this in mind for future code.

>> +
>> +static int scpi_hwmon_probe(struct platform_device *pdev)
>> +{
>> +	u16 sensors, i;
>> +	int ret;
>> +
>> +	scpi_ops = get_scpi_ops();
>> +	if (!scpi_ops)
>> +		return -EPROBE_DEFER;
>> +
> Is this appropriate ? I am a bit concerned that we might get a lot
> of "requests probe deferral" if the underlying infrastructure is
> not there at all.
>

This driver depends on SCPI protocol driver which in turn depends on the
mailbox driver (arm_mhu) each of which can be built as a
module. Requesting probe deferral avoids having to explicitly impose
ordering for loading these drivers.

IIUC, a driver requesting probe deferral is re-tried only if some other
driver probes successfully which might cause this driver to be
re-probeable (for lack of a better word).

>> +	ret = scpi_ops->sensor_get_capability(&sensors);
>> +	if (ret)
>> +		return ret;
>> +
>> +	scpi_sensors.device = devm_kcalloc(&pdev->dev, sensors,
>> +				   sizeof(*scpi_sensors.device), GFP_KERNEL);
>
> What if sensors == 0 ?
>

I've now handled the case of 0 sensors.

> It might be useful to introduce a local variable
> 	struct device *dev = &pdev->dev;
> and use it in this function.
>

Done.

>> +	if (!scpi_sensors.device)
>> +		return -ENOMEM;
>> +
>> +	dev_info(&pdev->dev, "Found %d sensors\n", sensors);
>
> Please drop this message and the other info messages below (too noisy).
> If you think you need those messages for debugging, make it dev_dbg().
>
>> +	for (i = 0; i < sensors; i++) {
>> +		struct sensor_dev *dev = &scpi_sensors.device[i];
>> +
>> +		ret = scpi_ops->sensor_get_info(i, &dev->info);
>> +		if (ret) {
>> +			dev_info(&pdev->dev,
>> +				 "Error ret=%d when querying for sensor %d\n",
>> +				 ret, i);
>
> If this is an error, it should be dev_err().
>

I've dropped the message and just return the error now.

>> +			return ret;
>> +		}
>> +
>> +		dev_info(&pdev->dev, "Probed \'%s\' sensor.\n",
>> +			 dev->info.name);
>> +
>> +		switch (dev->info.class) {
>> +		case TEMPERATURE:
>> +			snprintf(dev->input, 20,
>
> Can you use sizeof() to determine the string length ?
>

Done.

>> +				 "temp%d_input", scpi_sensors.num_temp);
>> +			snprintf(dev->label, 20,
>> +				 "temp%d_label", scpi_sensors.num_temp);
>> +			scpi_sensors.num_temp++;
>
> temp sensor numbering starts with 1.
>
>> +			break;
>> +		case VOLTAGE:
>> +			snprintf(dev->input, 20,
>> +				 "in%d_input", scpi_sensors.num_volt);
>> +			snprintf(dev->label, 20,
>> +				 "in%d_label", scpi_sensors.num_volt);
>> +			scpi_sensors.num_volt++;
>> +			break;
>> +		case CURRENT:
>> +			snprintf(dev->input, 20,
>> +				 "curr%d_input", scpi_sensors.num_current);
>> +			snprintf(dev->label, 20,
>> +				 "curr%d_label", scpi_sensors.num_current);
>> +			scpi_sensors.num_current++;
>
> current sensor numbering starts with 1.
>
>> +			break;
>> +		case POWER:
>> +			snprintf(dev->input, 20,
>> +				 "power%d_input", scpi_sensors.num_power);
>> +			snprintf(dev->label, 20,
>> +				 "power%d_label", scpi_sensors.num_power);
>> +			scpi_sensors.num_power++;
>
> power sensor numbering starts with 1.
>

Corrected the numbering for all of the above.

>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +
>> +		dev->dev_attr_input.attr.mode = S_IRUGO;
>> +		dev->dev_attr_input.store = NULL;
>
> kcalloc() initializes the allocated memory, so this assignment is unnecessary.
>
>> +		dev->dev_attr_input.show = scpi_hwmon_show_sensor;
>> +		dev->dev_attr_input.attr.name = dev->input;
>> +
>> +		dev->dev_attr_label.attr.mode = S_IRUGO;
>> +		dev->dev_attr_label.store = NULL;
>
> Same as above.
>

Fixed.

>> +		dev->dev_attr_label.show = scpi_hwmon_show_label;
>> +		dev->dev_attr_label.attr.name = dev->label;
>> +
>> +		scpi_attrs[i << 1] = &dev->dev_attr_input.attr;
>> +		scpi_attrs[(i << 1) + 1] = &dev->dev_attr_label.attr;
>
> You need to call sysfs_attr_init() for each of the attributes.
>

Now done.

>> +	}
>> +
>> +	scpi_group.attrs = scpi_attrs;
>> +	scpi_groups[0] = &scpi_group;
>> +
>> +	scpi_sensors.hwdev = devm_hwmon_device_register_with_groups(&pdev->dev,
>> +				    "scpi_sensors", &scpi_sensors, scpi_groups);
>
> Please allocate scpi_sensors and pass a pointer to it.
> All local variables should be kept in scpi_sensors (and can then be accessed
> with dev_get_drvdata).
>

Done.

>> +
>> +	if (IS_ERR(scpi_sensors.hwdev)) {
>> +		dev_err(&pdev->dev, "Error registering scpi_sensors hwmon device\n");
>> +		return PTR_ERR(scpi_sensors.hwdev);
>> +	}
>> +
>> +	dev_info(&pdev->dev, "SCPI hwmon driver initialised.\n");
>
> Please consider just returning
> 	return PTR_ERR_OR_ZERO(hwdev);
> without the noise.
>

Done.

>> +	return 0;
>> +}
>> +
>> +static int scpi_hwmon_remove(struct platform_device *pdev)
>> +{
>> +	scpi_ops = NULL;
>> +	return 0;
>> +}
>
> Not needed.
>

Dropped.


I've incorporated all your commments on this patch locally.

Thanks a lot for the review. Much appreciated.

I'll post an update once some of the other patches have had some review
as well.

Cheers,
Punit

>> +
>> +static const struct of_device_id scpi_of_match[] = {
>> +	{.compatible = "arm,scpi-sensors"},
>> +	{},
>> +};
>> +
>> +static struct platform_driver scpi_hwmon_platdrv = {
>> +	.driver = {
>> +		.name	= "scpi-hwmon",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = scpi_of_match,
>> +	},
>> +	.probe		= scpi_hwmon_probe,
>> +	.remove	= scpi_hwmon_remove,
>> +};
>> +module_platform_driver(scpi_hwmon_platdrv);
>> +
>> +MODULE_AUTHOR("Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>");
>> +MODULE_DESCRIPTION("ARM SCPI HWMON interface driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
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 7/9] hwmon: Support registration of thermal zones for SCP temperature sensors

[Punit Agrawal]

Add support to create thermal zones based on the temperature sensors
provided by the SCP. The thermal zones can be defined using the
thermal DT bindings and should refer to the SCP sensor id to select
the sensor.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/hwmon/scpi-hwmon.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
index dd0a6f1..1e52ced 100644
--- a/drivers/hwmon/scpi-hwmon.c
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -22,6 +22,7 @@
 #include <linux/scpi_protocol.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
+#include <linux/thermal.h>
 
 static struct scpi_ops *scpi_ops;
 
@@ -33,12 +34,18 @@ struct sensor_dev {
 	char label[20];
 };
 
+struct scpi_thermal_zone {
+	struct list_head list;
+	struct thermal_zone_device *tzd;
+};
+
 struct scpi_sensors {
 	int num_volt;
 	int num_temp;
 	int num_current;
 	int num_power;
 	struct sensor_dev *device;
+	struct list_head thermal_zones;
 	struct device *hwdev;
 };
 struct scpi_sensors scpi_sensors;
@@ -54,6 +61,20 @@ static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
 	return 0;
 }
 
+static int scpi_read_temp(void *dev, long *temp)
+{
+	struct sensor_dev *sensor = dev;
+	u32 value;
+	int ret;
+
+	ret = scpi_read_sensor(sensor, &value);
+	if (ret)
+		return ret;
+
+	*temp = value;
+	return 0;
+}
+
 /* hwmon callback functions */
 static ssize_t
 scpi_hwmon_show_sensor(struct device *dev,
@@ -90,6 +111,10 @@ struct attribute *scpi_attrs[24] = { 0 };
 struct attribute_group scpi_group;
 const struct attribute_group *scpi_groups[2] = { 0 };
 
+struct thermal_zone_of_device_ops scpi_sensor_ops = {
+	.get_temp = scpi_read_temp,
+};
+
 static int scpi_hwmon_probe(struct platform_device *pdev)
 {
 	u16 sensors, i;
@@ -108,9 +133,12 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 	if (!scpi_sensors.device)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&scpi_sensors.thermal_zones);
+
 	dev_info(&pdev->dev, "Found %d sensors\n", sensors);
 	for (i = 0; i < sensors; i++) {
 		struct sensor_dev *dev = &scpi_sensors.device[i];
+		struct scpi_thermal_zone *zone;
 
 		ret = scpi_ops->sensor_get_info(i, &dev->info);
 		if (ret) {
@@ -130,6 +158,20 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 			snprintf(dev->label, 20,
 				 "temp%d_label", scpi_sensors.num_temp);
 			scpi_sensors.num_temp++;
+
+			zone = devm_kmalloc(&pdev->dev, sizeof(*zone),
+					    GFP_KERNEL);
+			if (!zone)
+				return -ENOMEM;
+
+			zone->tzd = thermal_zone_of_sensor_register(&pdev->dev,
+						    i, dev, &scpi_sensor_ops);
+			if (!IS_ERR(zone->tzd))
+				list_add(&zone->list,
+					 &scpi_sensors.thermal_zones);
+			else
+				devm_kfree(&pdev->dev, zone);
+
 			break;
 		case VOLTAGE:
 			snprintf(dev->input, 20,
@@ -187,7 +229,18 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 
 static int scpi_hwmon_remove(struct platform_device *pdev)
 {
+	struct list_head *pos;
+
 	scpi_ops = NULL;
+
+	list_for_each(pos, &scpi_sensors.thermal_zones) {
+		struct scpi_thermal_zone *zone;
+
+		zone = list_entry(pos, struct scpi_thermal_zone, list);
+		thermal_zone_of_sensor_unregister(scpi_sensors.hwdev,
+						  zone->tzd);
+	}
+
 	return 0;
 }
 
-- 
2.1.4


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

Re: [PATCH 7/9] hwmon: Support registration of thermal zones for SCP temperature sensors

[Guenter Roeck]

On 07/22/2015 07:02 AM, Punit Agrawal wrote:
> Add support to create thermal zones based on the temperature sensors
> provided by the SCP. The thermal zones can be defined using the
> thermal DT bindings and should refer to the SCP sensor id to select
> the sensor.
>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> ---
>   drivers/hwmon/scpi-hwmon.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
> index dd0a6f1..1e52ced 100644
> --- a/drivers/hwmon/scpi-hwmon.c
> +++ b/drivers/hwmon/scpi-hwmon.c
> @@ -22,6 +22,7 @@
>   #include <linux/scpi_protocol.h>
>   #include <linux/slab.h>
>   #include <linux/sysfs.h>
> +#include <linux/thermal.h>
>
>   static struct scpi_ops *scpi_ops;
>
> @@ -33,12 +34,18 @@ struct sensor_dev {
>   	char label[20];
>   };
>
> +struct scpi_thermal_zone {
> +	struct list_head list;
> +	struct thermal_zone_device *tzd;
> +};
> +
>   struct scpi_sensors {
>   	int num_volt;
>   	int num_temp;
>   	int num_current;
>   	int num_power;
>   	struct sensor_dev *device;
> +	struct list_head thermal_zones;
>   	struct device *hwdev;
>   };
>   struct scpi_sensors scpi_sensors;
> @@ -54,6 +61,20 @@ static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
>   	return 0;
>   }
>
> +static int scpi_read_temp(void *dev, long *temp)
> +{
> +	struct sensor_dev *sensor = dev;
> +	u32 value;
> +	int ret;
> +
> +	ret = scpi_read_sensor(sensor, &value);
> +	if (ret)
> +		return ret;
> +
> +	*temp = value;
> +	return 0;
> +}
> +
>   /* hwmon callback functions */
>   static ssize_t
>   scpi_hwmon_show_sensor(struct device *dev,
> @@ -90,6 +111,10 @@ struct attribute *scpi_attrs[24] = { 0 };
>   struct attribute_group scpi_group;
>   const struct attribute_group *scpi_groups[2] = { 0 };
>
> +struct thermal_zone_of_device_ops scpi_sensor_ops = {

static struct ...

> +	.get_temp = scpi_read_temp,
> +};
> +
>   static int scpi_hwmon_probe(struct platform_device *pdev)
>   {
>   	u16 sensors, i;
> @@ -108,9 +133,12 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>   	if (!scpi_sensors.device)
>   		return -ENOMEM;
>
> +	INIT_LIST_HEAD(&scpi_sensors.thermal_zones);
> +
>   	dev_info(&pdev->dev, "Found %d sensors\n", sensors);
>   	for (i = 0; i < sensors; i++) {
>   		struct sensor_dev *dev = &scpi_sensors.device[i];
> +		struct scpi_thermal_zone *zone;
>
>   		ret = scpi_ops->sensor_get_info(i, &dev->info);
>   		if (ret) {
> @@ -130,6 +158,20 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>   			snprintf(dev->label, 20,
>   				 "temp%d_label", scpi_sensors.num_temp);
>   			scpi_sensors.num_temp++;
> +
> +			zone = devm_kmalloc(&pdev->dev, sizeof(*zone),
> +					    GFP_KERNEL);

Please consider using devm_kzalloc().

> +			if (!zone)
> +				return -ENOMEM;
> +
> +			zone->tzd = thermal_zone_of_sensor_register(&pdev->dev,
> +						    i, dev, &scpi_sensor_ops);
> +			if (!IS_ERR(zone->tzd))
> +				list_add(&zone->list,
> +					 &scpi_sensors.thermal_zones);
> +			else
> +				devm_kfree(&pdev->dev, zone);
> +

I would prefer

			if (IS_ERR_OR_NULL(zone->tzd)) {
				devm_kfree(&pdev->dev, zone);
				break;
			}
			list_add(&zone->list, &scpi_sensors.thermal_zones);

The code has a problem, though: You don't clean up thermal zones if
there is an error later on in the probe function. Either you need to
implement a cleanup function to be called from an error handler (and
from the remove function), or you need to wait with registering
thermal zones to the very end of the probe function.

Note that thermal_zone_of_sensor_register can return NULL if thermal
zones are not configured, so you have to use IS_ERR_OR_NULL
when checking for errors.

>   			break;
>   		case VOLTAGE:
>   			snprintf(dev->input, 20,
> @@ -187,7 +229,18 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>
>   static int scpi_hwmon_remove(struct platform_device *pdev)
>   {
> +	struct list_head *pos;
> +
>   	scpi_ops = NULL;
> +
> +	list_for_each(pos, &scpi_sensors.thermal_zones) {
> +		struct scpi_thermal_zone *zone;
> +
> +		zone = list_entry(pos, struct scpi_thermal_zone, list);
> +		thermal_zone_of_sensor_unregister(scpi_sensors.hwdev,

Not sure how this can work. The registration was with &pdev->dev,
not with hwdev. What happens if you actually unload this driver ?

> +						  zone->tzd);
> +	}
> +
>   	return 0;
>   }
>
>

Re: [PATCH 7/9] hwmon: Support registration of thermal zones for SCP temperature sensors

[Punit Agrawal]

Hi Guenter,

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

> On 07/22/2015 07:02 AM, Punit Agrawal wrote:
>> Add support to create thermal zones based on the temperature sensors
>> provided by the SCP. The thermal zones can be defined using the
>> thermal DT bindings and should refer to the SCP sensor id to select
>> the sensor.
>>
>> Signed-off-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
>> Cc: Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>
>> Cc: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>> Cc: Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>   drivers/hwmon/scpi-hwmon.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
>> index dd0a6f1..1e52ced 100644
>> --- a/drivers/hwmon/scpi-hwmon.c
>> +++ b/drivers/hwmon/scpi-hwmon.c

[...]

>> @@ -54,6 +61,20 @@ static int scpi_read_sensor(struct sensor_dev *sensor, u32 *value)
>>   	return 0;
>>   }
>>
>> +static int scpi_read_temp(void *dev, long *temp)
>> +{
>> +	struct sensor_dev *sensor = dev;
>> +	u32 value;
>> +	int ret;
>> +
>> +	ret = scpi_read_sensor(sensor, &value);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*temp = value;
>> +	return 0;
>> +}
>> +
>>   /* hwmon callback functions */
>>   static ssize_t
>>   scpi_hwmon_show_sensor(struct device *dev,
>> @@ -90,6 +111,10 @@ struct attribute *scpi_attrs[24] = { 0 };
>>   struct attribute_group scpi_group;
>>   const struct attribute_group *scpi_groups[2] = { 0 };
>>
>> +struct thermal_zone_of_device_ops scpi_sensor_ops = {
>
> static struct ...
>

Updated.

>> +	.get_temp = scpi_read_temp,
>> +};
>> +
>>   static int scpi_hwmon_probe(struct platform_device *pdev)
>>   {
>>   	u16 sensors, i;
>> @@ -108,9 +133,12 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>>   	if (!scpi_sensors.device)
>>   		return -ENOMEM;
>>
>> +	INIT_LIST_HEAD(&scpi_sensors.thermal_zones);
>> +
>>   	dev_info(&pdev->dev, "Found %d sensors\n", sensors);
>>   	for (i = 0; i < sensors; i++) {
>>   		struct sensor_dev *dev = &scpi_sensors.device[i];
>> +		struct scpi_thermal_zone *zone;
>>
>>   		ret = scpi_ops->sensor_get_info(i, &dev->info);
>>   		if (ret) {
>> @@ -130,6 +158,20 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>>   			snprintf(dev->label, 20,
>>   				 "temp%d_label", scpi_sensors.num_temp);
>>   			scpi_sensors.num_temp++;
>> +
>> +			zone = devm_kmalloc(&pdev->dev, sizeof(*zone),
>> +					    GFP_KERNEL);
>
> Please consider using devm_kzalloc().
>

Done

>> +			if (!zone)
>> +				return -ENOMEM;
>> +
>> +			zone->tzd = thermal_zone_of_sensor_register(&pdev->dev,
>> +						    i, dev, &scpi_sensor_ops);
>> +			if (!IS_ERR(zone->tzd))
>> +				list_add(&zone->list,
>> +					 &scpi_sensors.thermal_zones);
>> +			else
>> +				devm_kfree(&pdev->dev, zone);
>> +
>
> I would prefer
>
> 			if (IS_ERR_OR_NULL(zone->tzd)) {
> 				devm_kfree(&pdev->dev, zone);
> 				break;
> 			}
> 			list_add(&zone->list, &scpi_sensors.thermal_zones);
>
> The code has a problem, though: You don't clean up thermal zones if
> there is an error later on in the probe function. Either you need to
> implement a cleanup function to be called from an error handler (and
> from the remove function), or you need to wait with registering
> thermal zones to the very end of the probe function.
>

I've re-factored to register the sensors at the end of the probe.

> Note that thermal_zone_of_sensor_register can return NULL if thermal
> zones are not configured, so you have to use IS_ERR_OR_NULL
> when checking for errors.
>

You're right. I missed the NULL return when THERMAL_OF is configured
off. This doesn't match the documentation at the top of the function in
drivers/thermal/of-thermal.c

 * Return: On success returns a valid struct thermal_zone_device,
 * otherwise, it returns a corresponding ERR_PTR(). Caller must
 * check the return value with help of IS_ERR() helper.

Usage of thermal_zone_of_sensor_register in other drivers matches the above
doc. I'll include a patch fixing the NULL return with the next version.

>>   			break;
>>   		case VOLTAGE:
>>   			snprintf(dev->input, 20,
>> @@ -187,7 +229,18 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
>>
>>   static int scpi_hwmon_remove(struct platform_device *pdev)
>>   {
>> +	struct list_head *pos;
>> +
>>   	scpi_ops = NULL;
>> +
>> +	list_for_each(pos, &scpi_sensors.thermal_zones) {
>> +		struct scpi_thermal_zone *zone;
>> +
>> +		zone = list_entry(pos, struct scpi_thermal_zone, list);
>> +		thermal_zone_of_sensor_unregister(scpi_sensors.hwdev,
>
> Not sure how this can work. The registration was with &pdev->dev,
> not with hwdev. What happens if you actually unload this driver ?
>

Thanks for catching this! I'll fix this in the next version.

I had tested unloading the driver and didn't notice any adverse
ill-effects. Although thermal_zone_of_sensor_unregister expects a
pointer to a device, other then checking for non-NULLness, it doesn't do
anything with it.

Perhaps, this argument should be dropped from the function. If the
thermal maintainers agree I can cook up a patch. Eduardo?

Thanks,
Punit

>> +						  zone->tzd);
>> +	}
>> +
>>   	return 0;
>>   }
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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 8/9] arm64: dts: Add sensor node to Juno dt

[Punit Agrawal]

The SCP firmware on Juno provides access to SoC sensors via the
SCPI. Add the sensor nodes to the device tree to enable this support.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/boot/dts/arm/juno-base.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index c624208..dd5158e 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -96,6 +96,11 @@
 				clock-output-names = "pxlclk0", "pxlclk1";
 			};
 		};
+
+		scpi_sensors0: sensors {
+			compatible = "arm,scpi-sensors";
+			#thermal-sensor-cells = <1>;
+		};
 	};
 
 	/include/ "juno-clocks.dtsi"
-- 
2.1.4

[PATCH 9/9] arm64: dts: Create SoC thermal zone for Juno

[Punit Agrawal]

Setup a thermal zone driven by the SoC temperature sensor on Juno
r0. Create passive trip points and bind them to cpufreq cooling
devices that support the power extensions.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/boot/dts/arm/juno.dts | 50 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index d2e67f3..664961c 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -9,6 +9,7 @@
 /dts-v1/;
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	model = "ARM Juno development board (r0)";
@@ -90,6 +91,8 @@
 			next-level-cache = <&A57_L2>;
 			clocks = <&scpi_dvfs 0>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			#cooling-cells = <2>;
+			dynamic-power-coefficient = <530>;
 		};
 
 		A57_1: cpu@1 {
@@ -100,6 +103,8 @@
 			next-level-cache = <&A57_L2>;
 			clocks = <&scpi_dvfs 0>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			#cooling-cells = <2>;
+			dynamic-power-coefficient = <530>;
 		};
 
 		A53_0: cpu@100 {
@@ -110,6 +115,8 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			#cooling-cells = <2>;
+			dynamic-power-coefficient = <140>;
 		};
 
 		A53_1: cpu@101 {
@@ -120,6 +127,8 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			#cooling-cells = <2>;
+			dynamic-power-coefficient = <140>;
 		};
 
 		A53_2: cpu@102 {
@@ -130,6 +139,8 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			#cooling-cells = <2>;
+			dynamic-power-coefficient = <140>;
 		};
 
 		A53_3: cpu@103 {
@@ -140,6 +151,8 @@
 			next-level-cache = <&A53_L2>;
 			clocks = <&scpi_dvfs 1>;
 			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
+			#cooling-cells = <2>;
+			dynamic-power-coefficient = <140>;
 		};
 
 		A57_L2: l2-cache0 {
@@ -167,5 +180,42 @@
 				     <&A53_3>;
 	};
 
+	thermal-zones {
+		soc_thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			sustainable-power = <2500>;
+
+			thermal-sensors = <&scpi_sensors0 3>;
+
+			trips {
+				threshold: trip-point@0 {
+					temperature = <55000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+				target: trip-point@1 {
+					temperature = <65000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&target>;
+					cooling-device = <&A53_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+					contribution = <2048>;
+				};
+				map1 {
+					trip = <&target>;
+					cooling-device = <&A57_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+					contribution = <1024>;
+				};
+
+			};
+		};
+	};
+
 	#include "juno-base.dtsi"
 };
-- 
2.1.4

Re: [PATCH 0/9] Platform support for thermal management on Junoe

[Viresh Kumar]

On 22-07-15, 15:02, Punit Agrawal wrote:
>   cpufreq-dt: Supply power coefficient when registering cooling devices
>   cpufreq: arm_big_little: Add support to register a cpufreq cooling
>     device

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 0/9] Platform support for thermal management on Junoe

[Punit Agrawal]

Hi Viresh,

Viresh Kumar <viresh.kumar@linaro.org> writes:

> On 22-07-15, 15:02, Punit Agrawal wrote:
>>   cpufreq-dt: Supply power coefficient when registering cooling devices
>>   cpufreq: arm_big_little: Add support to register a cpufreq cooling
>>     device
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks for the acks.

Once the device tree property is agreed upon, will you be picking up these
patches?

Punit

Re: [PATCH 0/9] Platform support for thermal management on Junoe

[Viresh Kumar]

On 24-07-15, 15:16, Punit Agrawal wrote:
> Hi Viresh,
> 
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > On 22-07-15, 15:02, Punit Agrawal wrote:
> >>   cpufreq-dt: Supply power coefficient when registering cooling devices
> >>   cpufreq: arm_big_little: Add support to register a cpufreq cooling
> >>     device
> >
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Thanks for the acks.
> 
> Once the device tree property is agreed upon, will you be picking up these
> patches?

Since it is already present in patchwork, Rafael can apply these.

-- 
viresh