[PATCH 0/2] Add a generic virtual thermal sensor

[PATCH 0/2] Add a generic virtual thermal sensor

[Alexandre Bailon]

This series add a virtual thermal sensor.
It could be used to get a temperature using some thermal sensors.
Currently, the supported operations are max, min and avg.
The virtual sensor could be easily extended to support others operations.

Note:
Currently, thermal drivers must explicitly register their sensors to make them
available to the virtual sensor.
This doesn't seem a good solution to me and I think it would be preferable to
update the framework to register the list of each available sensors.

Alexandre Bailon (2):
  dt-bindings: Add bindings for the virtual thermal sensor
  thermal: add a virtual sensor to aggregate temperatures

 .../thermal/virtual,thermal-sensor.yaml       |  67 +++
 drivers/thermal/Kconfig                       |   8 +
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/virtual-sensor.h              |  51 +++
 drivers/thermal/virtual_sensor.c              | 400 ++++++++++++++++++
 include/dt-bindings/thermal/virtual-sensor.h  |  15 +
 6 files changed, 542 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
 create mode 100644 drivers/thermal/virtual-sensor.h
 create mode 100644 drivers/thermal/virtual_sensor.c
 create mode 100644 include/dt-bindings/thermal/virtual-sensor.h

-- 
2.31.1

[PATCH 1/2] dt-bindings: Add bindings for the virtual thermal sensor

[Alexandre Bailon]

This adds the device tree bidings for the virtual thermal sensor.
The virtual sensor could be used to a temperature computed from
many thermal sensors.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 .../thermal/virtual,thermal-sensor.yaml       | 67 +++++++++++++++++++
 include/dt-bindings/thermal/virtual-sensor.h  | 15 +++++
 2 files changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
 create mode 100644 include/dt-bindings/thermal/virtual-sensor.h

diff --git a/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
new file mode 100644
index 0000000000000..848b5912c79f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2021 BayLibre
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/thermal-sensor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Virtual thermal sensor binding
+
+description: |
+  The virtual thermal sensor devices provide temperature sensing capabilities
+  based on hardware thermal sensors. Basically, this could be used to get the
+  maximum, minimum or average temperature of the hardware thermal sensors.
+properties:
+  "#thermal-sensor-cells":
+    description:
+      Used to uniquely identify a thermal sensor instance within an IC. Will be
+      0 on sensor nodes with only a single sensor and at least 1 on nodes
+      containing several internal sensors.
+    enum: [0, 1]
+
+  type:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Used to select the operations to perform on the sensors to get the virtual
+      sensor temperature.
+    enum:
+      - VIRTUAL_SENSOR_MIN
+      - VIRTUAL_SENSOR_MAX
+      - VIRTUAL_SENSOR_AVG
+
+  thermal-sensors:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      The thermal sensor phandle and sensor specifier used to monitor this
+      thermal zone.
+
+required:
+  - "#thermal-sensor-cells"
+  - type
+  - thermal-sensors
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/thermal/thermal.h>
+    #include <dt-bindings/thermal/virtual-sensor.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/mt8192-clk.h>
+
+    lvts: lvts@1100b000 {
+        compatible = "mediatek,mt6873-lvts";
+        reg = <0x1100b000 0x1000>;
+        clocks = <&infracfg CLK_INFRA_THERM>;
+        clock-names = "lvts_clk";
+        #thermal-sensor-cells = <0>;
+        interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+    soc_max_sensor: soc_max_sensor {
+      compatible = "virtual,thermal-sensor";
+      #thermal-sensor-cells = <1>;
+      type = <VIRTUAL_SENSOR_MAX>;
+      thermal-sensors = <&lvts 0>, <&lvts 1>;
+    };
+...
diff --git a/include/dt-bindings/thermal/virtual-sensor.h b/include/dt-bindings/thermal/virtual-sensor.h
new file mode 100644
index 0000000000000..b3e4032f6f62b
--- /dev/null
+++ b/include/dt-bindings/thermal/virtual-sensor.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * This header provides constants for virtual thermal sensor bindings.
+ *
+ * Copyright (C) 2021 BayLibre
+ */
+
+#ifndef _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H
+#define _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H
+
+#define VIRTUAL_SENSOR_MIN 0
+#define VIRTUAL_SENSOR_MAX 1
+#define VIRTUAL_SENSOR_AVG 2
+
+#endif /* _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H */
-- 
2.31.1

[PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures

[Alexandre Bailon]

This adds a virtual thermal sensor that reads temperature from
hardware sensor and return an aggregated temperature.
Currently, this only return the max temperature.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/thermal/Kconfig          |   8 +
 drivers/thermal/Makefile         |   1 +
 drivers/thermal/virtual-sensor.h |  51 ++++
 drivers/thermal/virtual_sensor.c | 400 +++++++++++++++++++++++++++++++
 4 files changed, 460 insertions(+)
 create mode 100644 drivers/thermal/virtual-sensor.h
 create mode 100644 drivers/thermal/virtual_sensor.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 7a4ba50ba97d0..23dc903da2fc5 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -228,6 +228,14 @@ config THERMAL_MMIO
 	  register or shared memory, is a potential candidate to work with this
 	  driver.
 
+config VIRTUAL_THERMAL
+	tristate "Generic virtual thermal sensor driver"
+	depends on THERMAL_OF || COMPILE_TEST
+	help
+	  This option enables the generic thermal sensor aggregator.
+	  This driver creates a thermal sensor that reads the hardware sensors
+	  and aggregate the temperature.
+
 config HISI_THERMAL
 	tristate "Hisilicon thermal driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 9729a2b089919..76dfa1d61bfc5 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
 obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
 obj-$(CONFIG_SPRD_THERMAL)	+= sprd_thermal.o
 obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL)	+= khadas_mcu_fan.o
+obj-$(CONFIG_VIRTUAL_THERMAL) += virtual_sensor.o
diff --git a/drivers/thermal/virtual-sensor.h b/drivers/thermal/virtual-sensor.h
new file mode 100644
index 0000000000000..e024d434856c7
--- /dev/null
+++ b/drivers/thermal/virtual-sensor.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 BayLibre
+ */
+
+#ifndef __THERMAL_VIRTUAL_SENSOR_H__
+#define __THERMAL_VIRTUAL_SENSOR_H__
+
+struct virtual_sensor;
+struct virtual_sensor_data;
+
+#ifdef CONFIG_VIRTUAL_THERMAL
+struct virtual_sensor_data *
+thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
+				const struct thermal_zone_of_device_ops *ops);
+void thermal_virtual_sensor_unregister(struct device *dev,
+				       struct virtual_sensor_data *sensor_data);
+struct virtual_sensor_data *
+devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
+				     const struct thermal_zone_of_device_ops *ops);
+
+void devm_thermal_virtual_sensor_unregister(struct device *dev,
+					    struct virtual_sensor *sensor);
+#else
+static inline struct virtual_sensor_data *
+thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
+				const struct thermal_zone_of_device_ops *ops)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+void thermal_virtual_sensor_unregister(struct device *dev,
+				       struct virtual_sensor_data *sensor_data)
+{
+}
+
+static inline struct virtual_sensor_data *
+devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
+				     const struct thermal_zone_of_device_ops *ops)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline
+void devm_thermal_virtual_sensor_unregister(struct device *dev,
+					    struct virtual_sensor *sensor)
+{
+}
+#endif
+
+#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */
diff --git a/drivers/thermal/virtual_sensor.c b/drivers/thermal/virtual_sensor.c
new file mode 100644
index 0000000000000..e5bb0ef9adb39
--- /dev/null
+++ b/drivers/thermal/virtual_sensor.c
@@ -0,0 +1,400 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 BayLibre
+ */
+
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+#include <linux/types.h>
+#include <linux/string.h>
+
+#include <dt-bindings/thermal/virtual-sensor.h>
+
+#include "virtual-sensor.h"
+
+struct virtual_sensor_data {
+	struct list_head node;
+
+	/* sensor interface */
+	int id;
+	void *sensor_data;
+	const struct thermal_zone_of_device_ops *ops;
+};
+
+struct virtual_sensor {
+	int count;
+	struct virtual_sensor_data *sensors;
+	struct thermal_zone_device *tzd;
+
+	struct list_head node;
+};
+
+static LIST_HEAD(thermal_sensors);
+static LIST_HEAD(virtual_sensors);
+
+static int virtual_sensor_get_temp_max(void *data, int *temperature)
+{
+	struct virtual_sensor *sensor = data;
+	int max_temp = INT_MIN;
+	int temp;
+	int i;
+
+	for (i = 0; i < sensor->count; i++) {
+		struct virtual_sensor_data *hw_sensor;
+
+		hw_sensor = &sensor->sensors[i];
+		if (!hw_sensor->ops)
+			return -ENODEV;
+
+		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
+		max_temp = max(max_temp, temp);
+	}
+
+	*temperature = max_temp;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops virtual_sensor_max_ops = {
+	.get_temp = virtual_sensor_get_temp_max,
+};
+
+static int virtual_sensor_get_temp_min(void *data, int *temperature)
+{
+	struct virtual_sensor *sensor = data;
+	int min_temp = INT_MAX;
+	int temp;
+	int i;
+
+	for (i = 0; i < sensor->count; i++) {
+		struct virtual_sensor_data *hw_sensor;
+
+		hw_sensor = &sensor->sensors[i];
+		if (!hw_sensor->ops)
+			return -ENODEV;
+
+		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
+		min_temp = min(min_temp, temp);
+	}
+
+	*temperature = min_temp;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops virtual_sensor_min_ops = {
+	.get_temp = virtual_sensor_get_temp_min,
+};
+
+static int do_avg(int val1, int val2)
+{
+	return ((val1) / 2) + ((val2) / 2) + (((val1) % 2 + (val2) % 2) / 2);
+}
+
+static int virtual_sensor_get_temp_avg(void *data, int *temperature)
+{
+	struct virtual_sensor *sensor = data;
+	int avg_temp = 0;
+	int temp;
+	int i;
+
+	for (i = 0; i < sensor->count; i++) {
+		struct virtual_sensor_data *hw_sensor;
+
+		hw_sensor = &sensor->sensors[i];
+		if (!hw_sensor->ops)
+			return -ENODEV;
+
+		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
+		avg_temp = do_avg(avg_temp, temp);
+	}
+
+	*temperature = avg_temp;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops virtual_sensor_avg_ops = {
+	.get_temp = virtual_sensor_get_temp_avg,
+};
+
+static int register_virtual_sensor(struct virtual_sensor *sensor,
+				    struct of_phandle_args args,
+				    int index)
+{
+	struct virtual_sensor_data *sensor_data;
+	int id;
+
+	list_for_each_entry(sensor_data, &thermal_sensors, node) {
+		id = args.args_count ? args.args[0] : 0;
+		if (sensor_data->id == id) {
+			memcpy(&sensor->sensors[index], sensor_data,
+				sizeof(*sensor_data));
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int virtual_sensor_probe(struct platform_device *pdev)
+{
+	const struct thermal_zone_of_device_ops *ops;
+	struct virtual_sensor *sensor;
+	struct device *dev = &pdev->dev;
+	struct of_phandle_args args;
+	u32 type;
+	int ret;
+	int i;
+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor->count = of_count_phandle_with_args(dev->of_node,
+						   "thermal-sensors",
+						   "#thermal-sensor-cells");
+	if (sensor->count <= 0)
+		return -EINVAL;
+
+	sensor->sensors = devm_kmalloc_array(dev, sensor->count,
+					     sizeof(*sensor->sensors),
+					     GFP_KERNEL);
+	if (!sensor->sensors)
+		return -ENOMEM;
+
+	for (i = 0; i < sensor->count; i++) {
+		ret = of_parse_phandle_with_args(dev->of_node,
+						 "thermal-sensors",
+						 "#thermal-sensor-cells",
+						 i,
+						 &args);
+		if (ret)
+			return ret;
+
+		ret = register_virtual_sensor(sensor, args, i);
+		if (ret)
+			return ret;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "type", &type);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case VIRTUAL_SENSOR_MAX:
+		ops = &virtual_sensor_max_ops;
+		break;
+	case VIRTUAL_SENSOR_MIN:
+		ops = &virtual_sensor_min_ops;
+		break;
+	case VIRTUAL_SENSOR_AVG:
+		ops = &virtual_sensor_avg_ops;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor, ops);
+	if (IS_ERR(sensor->tzd))
+		return PTR_ERR(sensor->tzd);
+
+	platform_set_drvdata(pdev, sensor);
+	list_add(&sensor->node, &virtual_sensors);
+
+	return 0;
+}
+
+static int virtual_sensor_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct virtual_sensor *sensor;
+
+	sensor = platform_get_drvdata(pdev);
+	list_del(&sensor->node);
+
+	devm_thermal_zone_of_sensor_unregister(dev, sensor->tzd);
+	devm_kfree(dev, sensor->sensors);
+	devm_kfree(dev, sensor);
+
+	return 0;
+}
+
+static const struct of_device_id virtual_sensor_of_match[] = {
+	{
+		.compatible = "virtual,thermal-sensor",
+	},
+	{
+	},
+};
+MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
+
+static struct platform_driver virtual_sensor = {
+	.probe = virtual_sensor_probe,
+	.remove = virtual_sensor_remove,
+	.driver = {
+		.name = "virtual-sensor",
+		.of_match_table = virtual_sensor_of_match,
+	},
+};
+
+/**
+ * thermal_virtual_sensor_register - registers a sensor that could by a virtual
+ * sensor
+ * @dev: a valid struct device pointer of a sensor device. Must contain
+ *       a valid .of_node, for the sensor node.
+ * @sensor_id: a sensor identifier, in case the sensor IP has more
+ *             than one sensors
+ * @data: a private pointer (owned by the caller) that will be passed
+ *        back, when a temperature reading is needed.
+ * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
+ *
+ * This function will register a thermal sensor to make it available for later
+ * usage by a virtual sensor.
+ *
+ * The thermal zone temperature is provided by the @get_temp function
+ * pointer. When called, it will have the private pointer @data back.
+ *
+ * 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.
+ */
+struct virtual_sensor_data *thermal_virtual_sensor_register(
+	struct device *dev, int sensor_id, void *data,
+	const struct thermal_zone_of_device_ops *ops)
+{
+	struct virtual_sensor_data *sensor_data;
+
+	sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL);
+	if (!sensor_data)
+		return ERR_PTR(-ENOMEM);
+
+	sensor_data->id = sensor_id;
+	sensor_data->sensor_data = data;
+	sensor_data->ops = ops;
+
+	list_add(&sensor_data->node, &thermal_sensors);
+
+	return sensor_data;
+}
+EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register);
+
+/**
+ * thermal_virtual_sensor_unregister - unregisters a sensor
+ * @dev: a valid struct device pointer of a sensor device.
+ * @sensor_data: a pointer to struct virtual_sensor_data to unregister.
+ *
+ * This function removes the sensor from the list of available thermal sensors.
+ * If the sensor is in use, then the next call to .get_temp will return -ENODEV.
+ */
+void thermal_virtual_sensor_unregister(struct device *dev,
+				       struct virtual_sensor_data *sensor_data)
+{
+	struct virtual_sensor_data *temp;
+	struct virtual_sensor *sensor;
+	int i;
+
+	list_del(&sensor_data->node);
+
+	list_for_each_entry(sensor, &virtual_sensors, node) {
+		for (i = 0; i < sensor->count; i++) {
+			temp = &sensor->sensors[i];
+			if (temp->id == sensor_data->id &&
+				temp->sensor_data == sensor_data->sensor_data) {
+				temp->ops = NULL;
+			}
+		}
+	}
+	devm_kfree(dev, sensor_data);
+}
+EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister);
+
+static void devm_thermal_virtual_sensor_release(struct device *dev, void *res)
+{
+	thermal_virtual_sensor_unregister(dev,
+					  *(struct virtual_sensor_data **)res);
+}
+
+static int devm_thermal_virtual_sensor_match(struct device *dev, void *res,
+					     void *data)
+{
+	struct virtual_sensor_data **r = res;
+
+	if (WARN_ON(!r || !*r))
+		return 0;
+
+	return *r == data;
+}
+
+
+/**
+ * devm_thermal_virtual_sensor_register - Resource managed version of
+ *				thermal_virtual_sensor_register()
+ * @dev: a valid struct device pointer of a sensor device. Must contain
+ *       a valid .of_node, for the sensor node.
+ * @sensor_id: a sensor identifier, in case the sensor IP has more
+ *	       than one sensors
+ * @data: a private pointer (owned by the caller) that will be passed
+ *	  back, when a temperature reading is needed.
+ * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
+ *
+ * Refer thermal_zone_of_sensor_register() for more details.
+ *
+ * Return: On success returns a valid struct virtual_sensor_data,
+ * otherwise, it returns a corresponding ERR_PTR(). Caller must
+ * check the return value with help of IS_ERR() helper.
+ * Registered virtual_sensor_data device will automatically be
+ * released when device is unbounded.
+ */
+struct virtual_sensor_data *devm_thermal_virtual_sensor_register(
+	struct device *dev, int sensor_id,
+	void *data, const struct thermal_zone_of_device_ops *ops)
+{
+	struct virtual_sensor_data **ptr, *sensor_data;
+
+	ptr = devres_alloc(devm_thermal_virtual_sensor_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	sensor_data = thermal_virtual_sensor_register(dev, sensor_id, data, ops);
+	if (IS_ERR(sensor_data)) {
+		devres_free(ptr);
+		return sensor_data;
+	}
+
+	*ptr = sensor_data;
+	devres_add(dev, ptr);
+
+	return sensor_data;
+}
+EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_register);
+
+/**
+ * devm_thermal_virtual_sensor_unregister - Resource managed version of
+ *				thermal_virtual_sensor_unregister().
+ * @dev: Device for which resource was allocated.
+ * @sensor: a pointer to struct thermal_zone_device where the sensor is registered.
+ *
+ * This function removes the sensor from the list of sensors registered with
+ * devm_thermal_virtual_sensor_register() API.
+ * Normally this function will not need to be called and the resource
+ * management code will ensure that the resource is freed.
+ */
+void devm_thermal_virtual_sensor_unregister(struct device *dev,
+					    struct virtual_sensor *sensor)
+{
+	WARN_ON(devres_release(dev, devm_thermal_virtual_sensor_release,
+			       devm_thermal_virtual_sensor_match, sensor));
+}
+EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_unregister);
+
+module_platform_driver(virtual_sensor);
+MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
+MODULE_DESCRIPTION("Virtual thermal sensor");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1

Re: [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures

[kernel test robot]

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

Hi Alexandre,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v5.14 next-20210906]
[cannot apply to thermal/next soc-thermal/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Bailon/Add-a-generic-virtual-thermal-sensor/20210907-030547
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: riscv-allmodconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/2e3e80b0ee6c69039ada990aaf0380e8c6c024c0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexandre-Bailon/Add-a-generic-virtual-thermal-sensor/20210907-030547
        git checkout 2e3e80b0ee6c69039ada990aaf0380e8c6c024c0
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/thermal/virtual_sensor.c:18:
   drivers/thermal/virtual-sensor.h:32:6: warning: no previous prototype for 'thermal_virtual_sensor_unregister' [-Wmissing-prototypes]
      32 | void thermal_virtual_sensor_unregister(struct device *dev,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/thermal/virtual_sensor.c:8:
>> drivers/thermal/virtual_sensor.c:235:25: error: 'thermal_aggr_of_match' undeclared here (not in a function)
     235 | MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
         |                         ^~~~~~~~~~~~~~~~~~~~~
   include/linux/module.h:244:15: note: in definition of macro 'MODULE_DEVICE_TABLE'
     244 | extern typeof(name) __mod_##type##__##name##_device_table               \
         |               ^~~~
>> drivers/thermal/virtual_sensor.c:267:29: error: redefinition of 'thermal_virtual_sensor_register'
     267 | struct virtual_sensor_data *thermal_virtual_sensor_register(
         |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/thermal/virtual_sensor.c:18:
   drivers/thermal/virtual-sensor.h:26:1: note: previous definition of 'thermal_virtual_sensor_register' with type 'struct virtual_sensor_data *(struct device *, int,  void *, const struct thermal_zone_of_device_ops *)'
      26 | thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/thermal/virtual_sensor.c:295:6: error: redefinition of 'thermal_virtual_sensor_unregister'
     295 | void thermal_virtual_sensor_unregister(struct device *dev,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/thermal/virtual_sensor.c:18:
   drivers/thermal/virtual-sensor.h:32:6: note: previous definition of 'thermal_virtual_sensor_unregister' with type 'void(struct device *, struct virtual_sensor_data *)'
      32 | void thermal_virtual_sensor_unregister(struct device *dev,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/thermal/virtual_sensor.c:354:29: error: redefinition of 'devm_thermal_virtual_sensor_register'
     354 | struct virtual_sensor_data *devm_thermal_virtual_sensor_register(
         |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/thermal/virtual_sensor.c:18:
   drivers/thermal/virtual-sensor.h:38:1: note: previous definition of 'devm_thermal_virtual_sensor_register' with type 'struct virtual_sensor_data *(struct device *, int,  void *, const struct thermal_zone_of_device_ops *)'
      38 | devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/thermal/virtual_sensor.c:389:6: error: redefinition of 'devm_thermal_virtual_sensor_unregister'
     389 | void devm_thermal_virtual_sensor_unregister(struct device *dev,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/thermal/virtual_sensor.c:18:
   drivers/thermal/virtual-sensor.h:45:6: note: previous definition of 'devm_thermal_virtual_sensor_unregister' with type 'void(struct device *, struct virtual_sensor *)'
      45 | void devm_thermal_virtual_sensor_unregister(struct device *dev,
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/thermal/virtual_sensor.c:8:
>> include/linux/module.h:244:21: error: '__mod_of__thermal_aggr_of_match_device_table' aliased to undefined symbol 'thermal_aggr_of_match'
     244 | extern typeof(name) __mod_##type##__##name##_device_table               \
         |                     ^~~~~~
   drivers/thermal/virtual_sensor.c:235:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
     235 | MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
         | ^~~~~~~~~~~~~~~~~~~


vim +/thermal_aggr_of_match +235 drivers/thermal/virtual_sensor.c

   227	
   228	static const struct of_device_id virtual_sensor_of_match[] = {
   229		{
   230			.compatible = "virtual,thermal-sensor",
   231		},
   232		{
   233		},
   234	};
 > 235	MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
   236	
   237	static struct platform_driver virtual_sensor = {
   238		.probe = virtual_sensor_probe,
   239		.remove = virtual_sensor_remove,
   240		.driver = {
   241			.name = "virtual-sensor",
   242			.of_match_table = virtual_sensor_of_match,
   243		},
   244	};
   245	
   246	/**
   247	 * thermal_virtual_sensor_register - registers a sensor that could by a virtual
   248	 * sensor
   249	 * @dev: a valid struct device pointer of a sensor device. Must contain
   250	 *       a valid .of_node, for the sensor node.
   251	 * @sensor_id: a sensor identifier, in case the sensor IP has more
   252	 *             than one sensors
   253	 * @data: a private pointer (owned by the caller) that will be passed
   254	 *        back, when a temperature reading is needed.
   255	 * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
   256	 *
   257	 * This function will register a thermal sensor to make it available for later
   258	 * usage by a virtual sensor.
   259	 *
   260	 * The thermal zone temperature is provided by the @get_temp function
   261	 * pointer. When called, it will have the private pointer @data back.
   262	 *
   263	 * Return: On success returns a valid struct thermal_zone_device,
   264	 * otherwise, it returns a corresponding ERR_PTR(). Caller must
   265	 * check the return value with help of IS_ERR() helper.
   266	 */
 > 267	struct virtual_sensor_data *thermal_virtual_sensor_register(
   268		struct device *dev, int sensor_id, void *data,
   269		const struct thermal_zone_of_device_ops *ops)
   270	{
   271		struct virtual_sensor_data *sensor_data;
   272	
   273		sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL);
   274		if (!sensor_data)
   275			return ERR_PTR(-ENOMEM);
   276	
   277		sensor_data->id = sensor_id;
   278		sensor_data->sensor_data = data;
   279		sensor_data->ops = ops;
   280	
   281		list_add(&sensor_data->node, &thermal_sensors);
   282	
   283		return sensor_data;
   284	}
   285	EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register);
   286	
   287	/**
   288	 * thermal_virtual_sensor_unregister - unregisters a sensor
   289	 * @dev: a valid struct device pointer of a sensor device.
   290	 * @sensor_data: a pointer to struct virtual_sensor_data to unregister.
   291	 *
   292	 * This function removes the sensor from the list of available thermal sensors.
   293	 * If the sensor is in use, then the next call to .get_temp will return -ENODEV.
   294	 */
 > 295	void thermal_virtual_sensor_unregister(struct device *dev,
   296					       struct virtual_sensor_data *sensor_data)
   297	{
   298		struct virtual_sensor_data *temp;
   299		struct virtual_sensor *sensor;
   300		int i;
   301	
   302		list_del(&sensor_data->node);
   303	
   304		list_for_each_entry(sensor, &virtual_sensors, node) {
   305			for (i = 0; i < sensor->count; i++) {
   306				temp = &sensor->sensors[i];
   307				if (temp->id == sensor_data->id &&
   308					temp->sensor_data == sensor_data->sensor_data) {
   309					temp->ops = NULL;
   310				}
   311			}
   312		}
   313		devm_kfree(dev, sensor_data);
   314	}
   315	EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister);
   316	
   317	static void devm_thermal_virtual_sensor_release(struct device *dev, void *res)
   318	{
   319		thermal_virtual_sensor_unregister(dev,
   320						  *(struct virtual_sensor_data **)res);
   321	}
   322	
   323	static int devm_thermal_virtual_sensor_match(struct device *dev, void *res,
   324						     void *data)
   325	{
   326		struct virtual_sensor_data **r = res;
   327	
   328		if (WARN_ON(!r || !*r))
   329			return 0;
   330	
   331		return *r == data;
   332	}
   333	
   334	
   335	/**
   336	 * devm_thermal_virtual_sensor_register - Resource managed version of
   337	 *				thermal_virtual_sensor_register()
   338	 * @dev: a valid struct device pointer of a sensor device. Must contain
   339	 *       a valid .of_node, for the sensor node.
   340	 * @sensor_id: a sensor identifier, in case the sensor IP has more
   341	 *	       than one sensors
   342	 * @data: a private pointer (owned by the caller) that will be passed
   343	 *	  back, when a temperature reading is needed.
   344	 * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
   345	 *
   346	 * Refer thermal_zone_of_sensor_register() for more details.
   347	 *
   348	 * Return: On success returns a valid struct virtual_sensor_data,
   349	 * otherwise, it returns a corresponding ERR_PTR(). Caller must
   350	 * check the return value with help of IS_ERR() helper.
   351	 * Registered virtual_sensor_data device will automatically be
   352	 * released when device is unbounded.
   353	 */
 > 354	struct virtual_sensor_data *devm_thermal_virtual_sensor_register(
   355		struct device *dev, int sensor_id,
   356		void *data, const struct thermal_zone_of_device_ops *ops)
   357	{
   358		struct virtual_sensor_data **ptr, *sensor_data;
   359	
   360		ptr = devres_alloc(devm_thermal_virtual_sensor_release, sizeof(*ptr),
   361				   GFP_KERNEL);
   362		if (!ptr)
   363			return ERR_PTR(-ENOMEM);
   364	
   365		sensor_data = thermal_virtual_sensor_register(dev, sensor_id, data, ops);
   366		if (IS_ERR(sensor_data)) {
   367			devres_free(ptr);
   368			return sensor_data;
   369		}
   370	
   371		*ptr = sensor_data;
   372		devres_add(dev, ptr);
   373	
   374		return sensor_data;
   375	}
   376	EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_register);
   377	
   378	/**
   379	 * devm_thermal_virtual_sensor_unregister - Resource managed version of
   380	 *				thermal_virtual_sensor_unregister().
   381	 * @dev: Device for which resource was allocated.
   382	 * @sensor: a pointer to struct thermal_zone_device where the sensor is registered.
   383	 *
   384	 * This function removes the sensor from the list of sensors registered with
   385	 * devm_thermal_virtual_sensor_register() API.
   386	 * Normally this function will not need to be called and the resource
   387	 * management code will ensure that the resource is freed.
   388	 */
 > 389	void devm_thermal_virtual_sensor_unregister(struct device *dev,
   390						    struct virtual_sensor *sensor)
   391	{
   392		WARN_ON(devres_release(dev, devm_thermal_virtual_sensor_release,
   393				       devm_thermal_virtual_sensor_match, sensor));
   394	}
   395	EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_unregister);
   396	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 70094 bytes --]

Re: [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures

[Matthias Kaehlcke]

On Mon, Sep 06, 2021 at 09:04:54PM +0200, Alexandre Bailon wrote:
> This adds a virtual thermal sensor that reads temperature from
> hardware sensor and return an aggregated temperature.

s/hardware sensor/hardware sensors/

> Currently, this only return the max temperature.

it seems this is outdated.

> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/thermal/Kconfig          |   8 +
>  drivers/thermal/Makefile         |   1 +
>  drivers/thermal/virtual-sensor.h |  51 ++++
>  drivers/thermal/virtual_sensor.c | 400 +++++++++++++++++++++++++++++++
>  4 files changed, 460 insertions(+)
>  create mode 100644 drivers/thermal/virtual-sensor.h
>  create mode 100644 drivers/thermal/virtual_sensor.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 7a4ba50ba97d0..23dc903da2fc5 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -228,6 +228,14 @@ config THERMAL_MMIO
>  	  register or shared memory, is a potential candidate to work with this
>  	  driver.
>  
> +config VIRTUAL_THERMAL
> +	tristate "Generic virtual thermal sensor driver"

Not sure if 'Generic' adds much value here.

> +	depends on THERMAL_OF || COMPILE_TEST
> +	help
> +	  This option enables the generic thermal sensor aggregator.
> +	  This driver creates a thermal sensor that reads the hardware sensors
> +	  and aggregate the temperature.
> +
>  config HISI_THERMAL
>  	tristate "Hisilicon thermal driver"
>  	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 9729a2b089919..76dfa1d61bfc5 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
>  obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
>  obj-$(CONFIG_SPRD_THERMAL)	+= sprd_thermal.o
>  obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL)	+= khadas_mcu_fan.o
> +obj-$(CONFIG_VIRTUAL_THERMAL) += virtual_sensor.o
> diff --git a/drivers/thermal/virtual-sensor.h b/drivers/thermal/virtual-sensor.h
> new file mode 100644
> index 0000000000000..e024d434856c7
> --- /dev/null
> +++ b/drivers/thermal/virtual-sensor.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 BayLibre
> + */
> +
> +#ifndef __THERMAL_VIRTUAL_SENSOR_H__
> +#define __THERMAL_VIRTUAL_SENSOR_H__
> +
> +struct virtual_sensor;
> +struct virtual_sensor_data;

Other types of virtual sensors could be added in the future, you might
want to name these virtual_thermal_sensor(_data). Then again, these
structs are of internal use only, so it's probably not super important.

> +
> +#ifdef CONFIG_VIRTUAL_THERMAL
> +struct virtual_sensor_data *
> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> +				const struct thermal_zone_of_device_ops *ops);
> +void thermal_virtual_sensor_unregister(struct device *dev,
> +				       struct virtual_sensor_data *sensor_data);
> +struct virtual_sensor_data *
> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> +				     const struct thermal_zone_of_device_ops *ops);
> +
> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
> +					    struct virtual_sensor *sensor);
> +#else
> +static inline struct virtual_sensor_data *
> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> +				const struct thermal_zone_of_device_ops *ops)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +void thermal_virtual_sensor_unregister(struct device *dev,
> +				       struct virtual_sensor_data *sensor_data)
> +{
> +}
> +
> +static inline struct virtual_sensor_data *
> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
> +				     const struct thermal_zone_of_device_ops *ops)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline
> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
> +					    struct virtual_sensor *sensor)
> +{
> +}
> +#endif
> +
> +#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */
> diff --git a/drivers/thermal/virtual_sensor.c b/drivers/thermal/virtual_sensor.c
> new file mode 100644
> index 0000000000000..e5bb0ef9adb39
> --- /dev/null
> +++ b/drivers/thermal/virtual_sensor.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 BayLibre
> + */
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +
> +#include <dt-bindings/thermal/virtual-sensor.h>
> +
> +#include "virtual-sensor.h"
> +
> +struct virtual_sensor_data {

This struct (typically) corresponds to an actual sensor, maybe name it
'thermal_sensor_data' instead?

> +	struct list_head node;
> +
> +	/* sensor interface */
> +	int id;
> +	void *sensor_data;
> +	const struct thermal_zone_of_device_ops *ops;
> +};
> +
> +struct virtual_sensor {
> +	int count;
> +	struct virtual_sensor_data *sensors;
> +	struct thermal_zone_device *tzd;
> +
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(thermal_sensors);
> +static LIST_HEAD(virtual_sensors);
> +
> +static int virtual_sensor_get_temp_max(void *data, int *temperature)
> +{
> +	struct virtual_sensor *sensor = data;
> +	int max_temp = INT_MIN;
> +	int temp;
> +	int i;
> +
> +	for (i = 0; i < sensor->count; i++) {
> +		struct virtual_sensor_data *hw_sensor;
> +
> +		hw_sensor = &sensor->sensors[i];
> +		if (!hw_sensor->ops)
> +			return -ENODEV;
> +
> +		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
> +		max_temp = max(max_temp, temp);
> +	}
> +
> +	*temperature = max_temp;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops virtual_sensor_max_ops = {
> +	.get_temp = virtual_sensor_get_temp_max,
> +};
> +
> +static int virtual_sensor_get_temp_min(void *data, int *temperature)
> +{
> +	struct virtual_sensor *sensor = data;
> +	int min_temp = INT_MAX;
> +	int temp;
> +	int i;
> +
> +	for (i = 0; i < sensor->count; i++) {
> +		struct virtual_sensor_data *hw_sensor;
> +
> +		hw_sensor = &sensor->sensors[i];
> +		if (!hw_sensor->ops)
> +			return -ENODEV;
> +
> +		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
> +		min_temp = min(min_temp, temp);
> +	}
> +
> +	*temperature = min_temp;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops virtual_sensor_min_ops = {
> +	.get_temp = virtual_sensor_get_temp_min,
> +};
> +
> +static int do_avg(int val1, int val2)
> +{
> +	return ((val1) / 2) + ((val2) / 2) + (((val1) % 2 + (val2) % 2) / 2);
> +}
> +
> +static int virtual_sensor_get_temp_avg(void *data, int *temperature)
> +{
> +	struct virtual_sensor *sensor = data;
> +	int avg_temp = 0;
> +	int temp;
> +	int i;
> +
> +	for (i = 0; i < sensor->count; i++) {
> +		struct virtual_sensor_data *hw_sensor;
> +
> +		hw_sensor = &sensor->sensors[i];
> +		if (!hw_sensor->ops)
> +			return -ENODEV;
> +
> +		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
> +		avg_temp = do_avg(avg_temp, temp);
> +	}
> +
> +	*temperature = avg_temp;
> +
> +	return 0;
> +}

_get_temp_min(), _get_temp_max() and _get_temp_avg() have the same
structure, the only differences is the aggregator function and the
initialization value. If you wanted to save a few lines of code you
could have a meta-function that receives the initialization value and
a pointer of the aggregator function.

The functions are relatively short though, so I wouldn't claim that
it would be a huge improvement, one could also argue that the code is
easier to follow as is.

> +
> +static const struct thermal_zone_of_device_ops virtual_sensor_avg_ops = {
> +	.get_temp = virtual_sensor_get_temp_avg,
> +};
> +
> +static int register_virtual_sensor(struct virtual_sensor *sensor,
> +				    struct of_phandle_args args,
> +				    int index)

Does this really register a virtual sensor? IIUC the registered sensor is
(typically) an actual sensor, which is used by a virtual sensor.

Shouldn't it be something like 'register_thermal_sensor' or
'virtual_sensor_add_sensor'?

> +{
> +	struct virtual_sensor_data *sensor_data;
> +	int id;
> +
> +	list_for_each_entry(sensor_data, &thermal_sensors, node) {
> +		id = args.args_count ? args.args[0] : 0;
> +		if (sensor_data->id == id) {
> +			memcpy(&sensor->sensors[index], sensor_data,
> +				sizeof(*sensor_data));
> +			return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int virtual_sensor_probe(struct platform_device *pdev)
> +{
> +	const struct thermal_zone_of_device_ops *ops;
> +	struct virtual_sensor *sensor;
> +	struct device *dev = &pdev->dev;
> +	struct of_phandle_args args;
> +	u32 type;
> +	int ret;
> +	int i;
> +
> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	sensor->count = of_count_phandle_with_args(dev->of_node,
> +						   "thermal-sensors",
> +						   "#thermal-sensor-cells");
> +	if (sensor->count <= 0)
> +		return -EINVAL;
> +
> +	sensor->sensors = devm_kmalloc_array(dev, sensor->count,
> +					     sizeof(*sensor->sensors),
> +					     GFP_KERNEL);
> +	if (!sensor->sensors)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sensor->count; i++) {
> +		ret = of_parse_phandle_with_args(dev->of_node,
> +						 "thermal-sensors",
> +						 "#thermal-sensor-cells",
> +						 i,
> +						 &args);
> +		if (ret)
> +			return ret;
> +
> +		ret = register_virtual_sensor(sensor, args, i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "type", &type);

More a question for the binding, butthis should probably be something
more specific than 'type', like 'aggregation-function'.

> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case VIRTUAL_SENSOR_MAX:
> +		ops = &virtual_sensor_max_ops;
> +		break;
> +	case VIRTUAL_SENSOR_MIN:
> +		ops = &virtual_sensor_min_ops;
> +		break;
> +	case VIRTUAL_SENSOR_AVG:
> +		ops = &virtual_sensor_avg_ops;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor, ops);
> +	if (IS_ERR(sensor->tzd))
> +		return PTR_ERR(sensor->tzd);
> +
> +	platform_set_drvdata(pdev, sensor);
> +	list_add(&sensor->node, &virtual_sensors);

If you also added the sensor to 'thermal_sensors' you could support virtual
sensors using virtual sensors, though it's not clear how useful that would be
in practice and it could raise issues with the initialization order.

> +
> +	return 0;
> +}
> +
> +static int virtual_sensor_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct virtual_sensor *sensor;
> +
> +	sensor = platform_get_drvdata(pdev);
> +	list_del(&sensor->node);
> +
> +	devm_thermal_zone_of_sensor_unregister(dev, sensor->tzd);
> +	devm_kfree(dev, sensor->sensors);
> +	devm_kfree(dev, sensor);

Are the above 3 statements really needed, shouldn't devm_* handle that
automagically?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id virtual_sensor_of_match[] = {
> +	{
> +		.compatible = "virtual,thermal-sensor",
> +	},
> +	{
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
> +
> +static struct platform_driver virtual_sensor = {
> +	.probe = virtual_sensor_probe,
> +	.remove = virtual_sensor_remove,
> +	.driver = {
> +		.name = "virtual-sensor",

I suggest to change it to 'virtual-thermal-sensor' since there might be
other types of virtual sensors.

> +		.of_match_table = virtual_sensor_of_match,
> +	},
> +};
> +
> +/**
> + * thermal_virtual_sensor_register - registers a sensor that could by a virtual

s/by/be/

> + * sensor
> + * @dev: a valid struct device pointer of a sensor device. Must contain
> + *       a valid .of_node, for the sensor node.
> + * @sensor_id: a sensor identifier, in case the sensor IP has more
> + *             than one sensors

s/sensors/sensor/

> + * @data: a private pointer (owned by the caller) that will be passed
> + *        back, when a temperature reading is needed.
> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
> + *
> + * This function will register a thermal sensor to make it available for later
> + * usage by a virtual sensor.
> + *
> + * The thermal zone temperature is provided by the @get_temp function
> + * pointer. When called, it will have the private pointer @data back.
> + *
> + * 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.
> + */
> +struct virtual_sensor_data *thermal_virtual_sensor_register(
> +	struct device *dev, int sensor_id, void *data,
> +	const struct thermal_zone_of_device_ops *ops)
> +{
> +	struct virtual_sensor_data *sensor_data;
> +
> +	sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL);
> +	if (!sensor_data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sensor_data->id = sensor_id;
> +	sensor_data->sensor_data = data;
> +	sensor_data->ops = ops;
> +
> +	list_add(&sensor_data->node, &thermal_sensors);
> +
> +	return sensor_data;
> +}
> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register);
> +
> +/**
> + * thermal_virtual_sensor_unregister - unregisters a sensor
> + * @dev: a valid struct device pointer of a sensor device.
> + * @sensor_data: a pointer to struct virtual_sensor_data to unregister.
> + *
> + * This function removes the sensor from the list of available thermal sensors.
> + * If the sensor is in use, then the next call to .get_temp will return -ENODEV.
> + */
> +void thermal_virtual_sensor_unregister(struct device *dev,
> +				       struct virtual_sensor_data *sensor_data)
> +{
> +	struct virtual_sensor_data *temp;

'temp' might not be the best name in this context, since it's associated with
temperature. Maybe name it 'sd'?

> +	struct virtual_sensor *sensor;
> +	int i;
> +
> +	list_del(&sensor_data->node);
> +
> +	list_for_each_entry(sensor, &virtual_sensors, node) {
> +		for (i = 0; i < sensor->count; i++) {
> +			temp = &sensor->sensors[i];
> +			if (temp->id == sensor_data->id &&
> +				temp->sensor_data == sensor_data->sensor_data) {
> +				temp->ops = NULL;
> +			}
> +		}
> +	}
> +	devm_kfree(dev, sensor_data);

Does it actually make sense to allocate the memory with devm_kzalloc() if
it is explicitly freed here?

> +}
> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister);
> +
> +static void devm_thermal_virtual_sensor_release(struct device *dev, void *res)
> +{
> +	thermal_virtual_sensor_unregister(dev,
> +					  *(struct virtual_sensor_data **)res);
> +}
> +
> +static int devm_thermal_virtual_sensor_match(struct device *dev, void *res,
> +					     void *data)
> +{
> +	struct virtual_sensor_data **r = res;
> +
> +	if (WARN_ON(!r || !*r))
> +		return 0;
> +
> +	return *r == data;
> +}
> +
> +

delete one of the empty lines

> +/**
> + * devm_thermal_virtual_sensor_register - Resource managed version of
> + *				thermal_virtual_sensor_register()
> + * @dev: a valid struct device pointer of a sensor device. Must contain
> + *       a valid .of_node, for the sensor node.
> + * @sensor_id: a sensor identifier, in case the sensor IP has more
> + *	       than one sensors

s/sensors/sensor/

> + * @data: a private pointer (owned by the caller) that will be passed
> + *	  back, when a temperature reading is needed.
> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
> + *
> + * Refer thermal_zone_of_sensor_register() for more details.
> + *
> + * Return: On success returns a valid struct virtual_sensor_data,
> + * otherwise, it returns a corresponding ERR_PTR(). Caller must
> + * check the return value with help of IS_ERR() helper.
> + * Registered virtual_sensor_data device will automatically be
> + * released when device is unbounded.

s/unbounded/unbound/

Re: [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures

[Alexandre Bailon]

Hi Matthias,

On 9/10/21 12:29 AM, Matthias Kaehlcke wrote:
> On Mon, Sep 06, 2021 at 09:04:54PM +0200, Alexandre Bailon wrote:
>> This adds a virtual thermal sensor that reads temperature from
>> hardware sensor and return an aggregated temperature.
> s/hardware sensor/hardware sensors/
>
>> Currently, this only return the max temperature.
> it seems this is outdated.
>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>>   drivers/thermal/Kconfig          |   8 +
>>   drivers/thermal/Makefile         |   1 +
>>   drivers/thermal/virtual-sensor.h |  51 ++++
>>   drivers/thermal/virtual_sensor.c | 400 +++++++++++++++++++++++++++++++
>>   4 files changed, 460 insertions(+)
>>   create mode 100644 drivers/thermal/virtual-sensor.h
>>   create mode 100644 drivers/thermal/virtual_sensor.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 7a4ba50ba97d0..23dc903da2fc5 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -228,6 +228,14 @@ config THERMAL_MMIO
>>   	  register or shared memory, is a potential candidate to work with this
>>   	  driver.
>>   
>> +config VIRTUAL_THERMAL
>> +	tristate "Generic virtual thermal sensor driver"
> Not sure if 'Generic' adds much value here.
>
>> +	depends on THERMAL_OF || COMPILE_TEST
>> +	help
>> +	  This option enables the generic thermal sensor aggregator.
>> +	  This driver creates a thermal sensor that reads the hardware sensors
>> +	  and aggregate the temperature.
>> +
>>   config HISI_THERMAL
>>   	tristate "Hisilicon thermal driver"
>>   	depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 9729a2b089919..76dfa1d61bfc5 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -60,3 +60,4 @@ obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
>>   obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
>>   obj-$(CONFIG_SPRD_THERMAL)	+= sprd_thermal.o
>>   obj-$(CONFIG_KHADAS_MCU_FAN_THERMAL)	+= khadas_mcu_fan.o
>> +obj-$(CONFIG_VIRTUAL_THERMAL) += virtual_sensor.o
>> diff --git a/drivers/thermal/virtual-sensor.h b/drivers/thermal/virtual-sensor.h
>> new file mode 100644
>> index 0000000000000..e024d434856c7
>> --- /dev/null
>> +++ b/drivers/thermal/virtual-sensor.h
>> @@ -0,0 +1,51 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2021 BayLibre
>> + */
>> +
>> +#ifndef __THERMAL_VIRTUAL_SENSOR_H__
>> +#define __THERMAL_VIRTUAL_SENSOR_H__
>> +
>> +struct virtual_sensor;
>> +struct virtual_sensor_data;
> Other types of virtual sensors could be added in the future, you might
> want to name these virtual_thermal_sensor(_data). Then again, these
> structs are of internal use only, so it's probably not super important.
You are right. This might become confusing at some point.
I will rename them.
>
>> +
>> +#ifdef CONFIG_VIRTUAL_THERMAL
>> +struct virtual_sensor_data *
>> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
>> +				const struct thermal_zone_of_device_ops *ops);
>> +void thermal_virtual_sensor_unregister(struct device *dev,
>> +				       struct virtual_sensor_data *sensor_data);
>> +struct virtual_sensor_data *
>> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
>> +				     const struct thermal_zone_of_device_ops *ops);
>> +
>> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
>> +					    struct virtual_sensor *sensor);
>> +#else
>> +static inline struct virtual_sensor_data *
>> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
>> +				const struct thermal_zone_of_device_ops *ops)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +void thermal_virtual_sensor_unregister(struct device *dev,
>> +				       struct virtual_sensor_data *sensor_data)
>> +{
>> +}
>> +
>> +static inline struct virtual_sensor_data *
>> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data,
>> +				     const struct thermal_zone_of_device_ops *ops)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline
>> +void devm_thermal_virtual_sensor_unregister(struct device *dev,
>> +					    struct virtual_sensor *sensor)
>> +{
>> +}
>> +#endif
>> +
>> +#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */
>> diff --git a/drivers/thermal/virtual_sensor.c b/drivers/thermal/virtual_sensor.c
>> new file mode 100644
>> index 0000000000000..e5bb0ef9adb39
>> --- /dev/null
>> +++ b/drivers/thermal/virtual_sensor.c
>> @@ -0,0 +1,400 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2021 BayLibre
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/slab.h>
>> +#include <linux/thermal.h>
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +
>> +#include <dt-bindings/thermal/virtual-sensor.h>
>> +
>> +#include "virtual-sensor.h"
>> +
>> +struct virtual_sensor_data {
> This struct (typically) corresponds to an actual sensor, maybe name it
> 'thermal_sensor_data' instead?
>
>> +	struct list_head node;
>> +
>> +	/* sensor interface */
>> +	int id;
>> +	void *sensor_data;
>> +	const struct thermal_zone_of_device_ops *ops;
>> +};
>> +
>> +struct virtual_sensor {
>> +	int count;
>> +	struct virtual_sensor_data *sensors;
>> +	struct thermal_zone_device *tzd;
>> +
>> +	struct list_head node;
>> +};
>> +
>> +static LIST_HEAD(thermal_sensors);
>> +static LIST_HEAD(virtual_sensors);
>> +
>> +static int virtual_sensor_get_temp_max(void *data, int *temperature)
>> +{
>> +	struct virtual_sensor *sensor = data;
>> +	int max_temp = INT_MIN;
>> +	int temp;
>> +	int i;
>> +
>> +	for (i = 0; i < sensor->count; i++) {
>> +		struct virtual_sensor_data *hw_sensor;
>> +
>> +		hw_sensor = &sensor->sensors[i];
>> +		if (!hw_sensor->ops)
>> +			return -ENODEV;
>> +
>> +		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
>> +		max_temp = max(max_temp, temp);
>> +	}
>> +
>> +	*temperature = max_temp;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct thermal_zone_of_device_ops virtual_sensor_max_ops = {
>> +	.get_temp = virtual_sensor_get_temp_max,
>> +};
>> +
>> +static int virtual_sensor_get_temp_min(void *data, int *temperature)
>> +{
>> +	struct virtual_sensor *sensor = data;
>> +	int min_temp = INT_MAX;
>> +	int temp;
>> +	int i;
>> +
>> +	for (i = 0; i < sensor->count; i++) {
>> +		struct virtual_sensor_data *hw_sensor;
>> +
>> +		hw_sensor = &sensor->sensors[i];
>> +		if (!hw_sensor->ops)
>> +			return -ENODEV;
>> +
>> +		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
>> +		min_temp = min(min_temp, temp);
>> +	}
>> +
>> +	*temperature = min_temp;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct thermal_zone_of_device_ops virtual_sensor_min_ops = {
>> +	.get_temp = virtual_sensor_get_temp_min,
>> +};
>> +
>> +static int do_avg(int val1, int val2)
>> +{
>> +	return ((val1) / 2) + ((val2) / 2) + (((val1) % 2 + (val2) % 2) / 2);
>> +}
>> +
>> +static int virtual_sensor_get_temp_avg(void *data, int *temperature)
>> +{
>> +	struct virtual_sensor *sensor = data;
>> +	int avg_temp = 0;
>> +	int temp;
>> +	int i;
>> +
>> +	for (i = 0; i < sensor->count; i++) {
>> +		struct virtual_sensor_data *hw_sensor;
>> +
>> +		hw_sensor = &sensor->sensors[i];
>> +		if (!hw_sensor->ops)
>> +			return -ENODEV;
>> +
>> +		hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp);
>> +		avg_temp = do_avg(avg_temp, temp);
>> +	}
>> +
>> +	*temperature = avg_temp;
>> +
>> +	return 0;
>> +}
> _get_temp_min(), _get_temp_max() and _get_temp_avg() have the same
> structure, the only differences is the aggregator function and the
> initialization value. If you wanted to save a few lines of code you
> could have a meta-function that receives the initialization value and
> a pointer of the aggregator function.
>
> The functions are relatively short though, so I wouldn't claim that
> it would be a huge improvement, one could also argue that the code is
> easier to follow as is.
My intent was to make possible adding more complex functions.

I will factorize the code, and if later there are some use cases,
we will made the update to support more complex functions.

>
>> +
>> +static const struct thermal_zone_of_device_ops virtual_sensor_avg_ops = {
>> +	.get_temp = virtual_sensor_get_temp_avg,
>> +};
>> +
>> +static int register_virtual_sensor(struct virtual_sensor *sensor,
>> +				    struct of_phandle_args args,
>> +				    int index)
> Does this really register a virtual sensor? IIUC the registered sensor is
> (typically) an actual sensor, which is used by a virtual sensor.
>
> Shouldn't it be something like 'register_thermal_sensor' or
> 'virtual_sensor_add_sensor'?
You are right, virtual_sensor_add_sensor sounds a lot better.
>
>> +{
>> +	struct virtual_sensor_data *sensor_data;
>> +	int id;
>> +
>> +	list_for_each_entry(sensor_data, &thermal_sensors, node) {
>> +		id = args.args_count ? args.args[0] : 0;
>> +		if (sensor_data->id == id) {
>> +			memcpy(&sensor->sensors[index], sensor_data,
>> +				sizeof(*sensor_data));
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -ENODEV;
>> +}
>> +
>> +static int virtual_sensor_probe(struct platform_device *pdev)
>> +{
>> +	const struct thermal_zone_of_device_ops *ops;
>> +	struct virtual_sensor *sensor;
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_args args;
>> +	u32 type;
>> +	int ret;
>> +	int i;
>> +
>> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>> +	if (!sensor)
>> +		return -ENOMEM;
>> +
>> +	sensor->count = of_count_phandle_with_args(dev->of_node,
>> +						   "thermal-sensors",
>> +						   "#thermal-sensor-cells");
>> +	if (sensor->count <= 0)
>> +		return -EINVAL;
>> +
>> +	sensor->sensors = devm_kmalloc_array(dev, sensor->count,
>> +					     sizeof(*sensor->sensors),
>> +					     GFP_KERNEL);
>> +	if (!sensor->sensors)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < sensor->count; i++) {
>> +		ret = of_parse_phandle_with_args(dev->of_node,
>> +						 "thermal-sensors",
>> +						 "#thermal-sensor-cells",
>> +						 i,
>> +						 &args);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = register_virtual_sensor(sensor, args, i);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(dev->of_node, "type", &type);
> More a question for the binding, butthis should probably be something
> more specific than 'type', like 'aggregation-function'.
>
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (type) {
>> +	case VIRTUAL_SENSOR_MAX:
>> +		ops = &virtual_sensor_max_ops;
>> +		break;
>> +	case VIRTUAL_SENSOR_MIN:
>> +		ops = &virtual_sensor_min_ops;
>> +		break;
>> +	case VIRTUAL_SENSOR_AVG:
>> +		ops = &virtual_sensor_avg_ops;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor, ops);
>> +	if (IS_ERR(sensor->tzd))
>> +		return PTR_ERR(sensor->tzd);
>> +
>> +	platform_set_drvdata(pdev, sensor);
>> +	list_add(&sensor->node, &virtual_sensors);
> If you also added the sensor to 'thermal_sensors' you could support virtual
> sensors using virtual sensors, though it's not clear how useful that would be
> in practice and it could raise issues with the initialization order.
Unless if  we found some use cases, I think we should not do that.
As you mentioned, this would add some complexity to the initialization 
to handle it.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int virtual_sensor_remove(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct virtual_sensor *sensor;
>> +
>> +	sensor = platform_get_drvdata(pdev);
>> +	list_del(&sensor->node);
>> +
>> +	devm_thermal_zone_of_sensor_unregister(dev, sensor->tzd);
>> +	devm_kfree(dev, sensor->sensors);
>> +	devm_kfree(dev, sensor);
> Are the above 3 statements really needed, shouldn't devm_* handle that
> automagically?
No, I will remove them.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id virtual_sensor_of_match[] = {
>> +	{
>> +		.compatible = "virtual,thermal-sensor",
>> +	},
>> +	{
>> +	},
>> +};
>> +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
>> +
>> +static struct platform_driver virtual_sensor = {
>> +	.probe = virtual_sensor_probe,
>> +	.remove = virtual_sensor_remove,
>> +	.driver = {
>> +		.name = "virtual-sensor",
> I suggest to change it to 'virtual-thermal-sensor' since there might be
> other types of virtual sensors.
>
>> +		.of_match_table = virtual_sensor_of_match,
>> +	},
>> +};
>> +
>> +/**
>> + * thermal_virtual_sensor_register - registers a sensor that could by a virtual
> s/by/be/
>
>> + * sensor
>> + * @dev: a valid struct device pointer of a sensor device. Must contain
>> + *       a valid .of_node, for the sensor node.
>> + * @sensor_id: a sensor identifier, in case the sensor IP has more
>> + *             than one sensors
> s/sensors/sensor/
>
>> + * @data: a private pointer (owned by the caller) that will be passed
>> + *        back, when a temperature reading is needed.
>> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
>> + *
>> + * This function will register a thermal sensor to make it available for later
>> + * usage by a virtual sensor.
>> + *
>> + * The thermal zone temperature is provided by the @get_temp function
>> + * pointer. When called, it will have the private pointer @data back.
>> + *
>> + * 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.
>> + */
>> +struct virtual_sensor_data *thermal_virtual_sensor_register(
>> +	struct device *dev, int sensor_id, void *data,
>> +	const struct thermal_zone_of_device_ops *ops)
>> +{
>> +	struct virtual_sensor_data *sensor_data;
>> +
>> +	sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL);
>> +	if (!sensor_data)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	sensor_data->id = sensor_id;
>> +	sensor_data->sensor_data = data;
>> +	sensor_data->ops = ops;
>> +
>> +	list_add(&sensor_data->node, &thermal_sensors);
>> +
>> +	return sensor_data;
>> +}
>> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register);
>> +
>> +/**
>> + * thermal_virtual_sensor_unregister - unregisters a sensor
>> + * @dev: a valid struct device pointer of a sensor device.
>> + * @sensor_data: a pointer to struct virtual_sensor_data to unregister.
>> + *
>> + * This function removes the sensor from the list of available thermal sensors.
>> + * If the sensor is in use, then the next call to .get_temp will return -ENODEV.
>> + */
>> +void thermal_virtual_sensor_unregister(struct device *dev,
>> +				       struct virtual_sensor_data *sensor_data)
>> +{
>> +	struct virtual_sensor_data *temp;
> 'temp' might not be the best name in this context, since it's associated with
> temperature. Maybe name it 'sd'?
>
>> +	struct virtual_sensor *sensor;
>> +	int i;
>> +
>> +	list_del(&sensor_data->node);
>> +
>> +	list_for_each_entry(sensor, &virtual_sensors, node) {
>> +		for (i = 0; i < sensor->count; i++) {
>> +			temp = &sensor->sensors[i];
>> +			if (temp->id == sensor_data->id &&
>> +				temp->sensor_data == sensor_data->sensor_data) {
>> +				temp->ops = NULL;
>> +			}
>> +		}
>> +	}
>> +	devm_kfree(dev, sensor_data);
> Does it actually make sense to allocate the memory with devm_kzalloc() if
> it is explicitly freed here?
No, I should not call devm_kfree here.
I will remove it.

Thanks,
Alexandre
>
>> +}
>> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister);
>> +
>> +static void devm_thermal_virtual_sensor_release(struct device *dev, void *res)
>> +{
>> +	thermal_virtual_sensor_unregister(dev,
>> +					  *(struct virtual_sensor_data **)res);
>> +}
>> +
>> +static int devm_thermal_virtual_sensor_match(struct device *dev, void *res,
>> +					     void *data)
>> +{
>> +	struct virtual_sensor_data **r = res;
>> +
>> +	if (WARN_ON(!r || !*r))
>> +		return 0;
>> +
>> +	return *r == data;
>> +}
>> +
>> +
> delete one of the empty lines
>
>> +/**
>> + * devm_thermal_virtual_sensor_register - Resource managed version of
>> + *				thermal_virtual_sensor_register()
>> + * @dev: a valid struct device pointer of a sensor device. Must contain
>> + *       a valid .of_node, for the sensor node.
>> + * @sensor_id: a sensor identifier, in case the sensor IP has more
>> + *	       than one sensors
> s/sensors/sensor/
>
>> + * @data: a private pointer (owned by the caller) that will be passed
>> + *	  back, when a temperature reading is needed.
>> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp.
>> + *
>> + * Refer thermal_zone_of_sensor_register() for more details.
>> + *
>> + * Return: On success returns a valid struct virtual_sensor_data,
>> + * otherwise, it returns a corresponding ERR_PTR(). Caller must
>> + * check the return value with help of IS_ERR() helper.
>> + * Registered virtual_sensor_data device will automatically be
>> + * released when device is unbounded.
> s/unbounded/unbound/

Re: [PATCH 1/2] dt-bindings: Add bindings for the virtual thermal sensor

[Rafael J. Wysocki]

On Mon, Sep 6, 2021 at 9:05 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This adds the device tree bidings for the virtual thermal sensor.

I'm not sure what "the virtual thermal sensor" is.

I'm guessing that you mean "DT bindings for the DT-based virtual
sensor driver introduced by a subsequent patch" or something like
this.

I also guess that the purpose is to allow the platform designer to
tell the kernel that some sensors need to be aggregated in order to
get useful information from them and how to aggregate them.  Otherwise
it would be hard to say why the aggregation needed to take place in
the kernel.

Moreover, the aggregation functions supported by this series are
somewhat simple and I'm not sure if they are really sufficient in
practice.

> The virtual sensor could be used to a temperature computed from
> many thermal sensors.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

Admittedly, I'm not a DT bindings expert, so if I say something
blatantly silly below, sorry about that.

> ---
>  .../thermal/virtual,thermal-sensor.yaml       | 67 +++++++++++++++++++
>  include/dt-bindings/thermal/virtual-sensor.h  | 15 +++++
>  2 files changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
>  create mode 100644 include/dt-bindings/thermal/virtual-sensor.h
>
> diff --git a/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
> new file mode 100644
> index 0000000000000..848b5912c79f1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2021 BayLibre
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/thermal-sensor.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Virtual thermal sensor binding
> +
> +description: |
> +  The virtual thermal sensor devices provide temperature sensing capabilities
> +  based on hardware thermal sensors. Basically, this could be used to get the
> +  maximum, minimum or average temperature of the hardware thermal sensors.
> +properties:
> +  "#thermal-sensor-cells":

It isn't clear to me why this is needed.  If the "thermal-sensors"
property is required anyway, I'm not sure why it's still necessary to
have another one to find out whether there is just one sensor or more
of them.

> +    description:
> +      Used to uniquely identify a thermal sensor instance within an IC. Will be
> +      0 on sensor nodes with only a single sensor and at least 1 on nodes
> +      containing several internal sensors.
> +    enum: [0, 1]
> +
> +  type:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Used to select the operations to perform on the sensors to get the virtual
> +      sensor temperature.
> +    enum:
> +      - VIRTUAL_SENSOR_MIN
> +      - VIRTUAL_SENSOR_MAX
> +      - VIRTUAL_SENSOR_AVG
> +
> +  thermal-sensors:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      The thermal sensor phandle and sensor specifier used to monitor this
> +      thermal zone.
> +
> +required:
> +  - "#thermal-sensor-cells"
> +  - type
> +  - thermal-sensors
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    #include <dt-bindings/thermal/thermal.h>
> +    #include <dt-bindings/thermal/virtual-sensor.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/mt8192-clk.h>
> +
> +    lvts: lvts@1100b000 {
> +        compatible = "mediatek,mt6873-lvts";
> +        reg = <0x1100b000 0x1000>;
> +        clocks = <&infracfg CLK_INFRA_THERM>;
> +        clock-names = "lvts_clk";
> +        #thermal-sensor-cells = <0>;
> +        interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +    soc_max_sensor: soc_max_sensor {
> +      compatible = "virtual,thermal-sensor";

Where/how is the above defined?

> +      #thermal-sensor-cells = <1>;
> +      type = <VIRTUAL_SENSOR_MAX>;
> +      thermal-sensors = <&lvts 0>, <&lvts 1>;
> +    };
> +...
> diff --git a/include/dt-bindings/thermal/virtual-sensor.h b/include/dt-bindings/thermal/virtual-sensor.h
> new file mode 100644
> index 0000000000000..b3e4032f6f62b
> --- /dev/null
> +++ b/include/dt-bindings/thermal/virtual-sensor.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * This header provides constants for virtual thermal sensor bindings.
> + *
> + * Copyright (C) 2021 BayLibre
> + */
> +
> +#ifndef _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H
> +#define _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H
> +

It would be better to use an enum type here.

> +#define VIRTUAL_SENSOR_MIN 0
> +#define VIRTUAL_SENSOR_MAX 1
> +#define VIRTUAL_SENSOR_AVG 2

Also note that the _MIN and _MAX symbols may be confused as limits, so
it may be better to call them _MIN_VAL and _MAX_VAL, respectively.

> +
> +#endif /* _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H */
> --