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 <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("Virtual thermal sensor");
+MODULE_LICENSE("GPL v2");
--
2.31.1
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 <[email protected]>
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/[email protected]
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 <[email protected]>
> ---
> 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/
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 <[email protected]>
>> ---
>> 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/