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

[PATCH v4 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.

Changes in v2:
- Fix some warnings / errors reported by kernel test robot
- rename some struct and functions with a more accurate name
- update the dt bindings: rename type attribute to aggregation-function
- factorize a little bit the aggregation functions
Changes in v3:
- Aggregate thermal zone instead of thermal sensors
- Use try_get_module / put_module to prevent thermal providers to be removed
- Update the bindings, to be more accurate
Changes in v4:
- Fix two warnings reported by kernel test robot

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

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

 .../thermal/virtual,thermal-sensor.yaml       |  86 +++++++
 drivers/thermal/Kconfig                       |   8 +
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/virtual_thermal_sensor.c      | 218 ++++++++++++++++++
 include/dt-bindings/thermal/virtual-sensor.h  |  15 ++
 5 files changed, 328 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
 create mode 100644 drivers/thermal/virtual_thermal_sensor.c
 create mode 100644 include/dt-bindings/thermal/virtual-sensor.h

-- 
2.25.1

[PATCH v4 1/2] dt-bindings: Add DT bindings for the DT-based virtual sensor driver

[Alexandre Bailon]

This adds the DT bindings for the DT-based virtual sensor driver.
This driver provides a way, using DT, to aggregate the temperature
of multiple thermal zones and get some useful data from it.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 .../thermal/virtual,thermal-sensor.yaml       | 86 +++++++++++++++++++
 include/dt-bindings/thermal/virtual-sensor.h  | 15 ++++
 2 files changed, 101 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 000000000000..b404fd4b2b38
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2021 BayLibre
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/virtual,thermal-sensor.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Virtual thermal sensor binding
+
+maintainers:
+  - Alexandre Bailon <abailon@baylibre.com>
+
+description: |
+  The virtual thermal sensor device provides a way to aggregate the temperature
+  from multiple thermal zones. Basically, this could be used to get the
+  maximum, minimum or average temperature.
+
+allOf:
+  - $ref: thermal-sensor.yaml#
+
+properties:
+  compatible:
+    const: virtual,thermal-sensor
+
+  aggregation-function:
+    $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_THERMAL_SENSOR_MIN_VAL
+      - VIRTUAL_THERMAL_SENSOR_MAX_VAL
+      - VIRTUAL_THERMAL_SENSOR_AVG_VAL
+
+  thermal-sensors:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description:
+      The names of the thermal zone to aggregate.
+
+required:
+  - aggregation-function
+  - 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 = <1>;
+        interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+    cpu_max_sensor: cpu_max_sensor {
+      compatible = "virtual,thermal-sensor";
+      #thermal-sensor-cells = <1>;
+      aggregation-function = <VIRTUAL_THERMAL_SENSOR_MAX_VAL>;
+      thermal-sensors = "cpu1", "cpu2";
+    };
+
+    thermal_zones: thermal-zones {
+      cpu1 {
+        polling-delay = <0>; /* milliseconds */
+        polling-delay-passive = <0>; /* milliseconds */
+        thermal-sensors = <&lvts 0>;
+      };
+      cpu2 {
+        polling-delay = <0>; /* milliseconds */
+        polling-delay-passive = <0>; /* milliseconds */
+        thermal-sensors = <&lvts 1>;
+      };
+      cpu_max {
+        polling-delay = <0>; /* milliseconds */
+        polling-delay-passive = <0>; /* milliseconds */
+        thermal-sensors = <&cpu_max_sensor 0>;
+      };
+    };
+...
diff --git a/include/dt-bindings/thermal/virtual-sensor.h b/include/dt-bindings/thermal/virtual-sensor.h
new file mode 100644
index 000000000000..6254b2aa8f5c
--- /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_THERMAL_SENSOR_MIN_VAL 0
+#define VIRTUAL_THERMAL_SENSOR_MAX_VAL 1
+#define VIRTUAL_THERMAL_SENSOR_AVG_VAL 2
+
+#endif /* _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H */
-- 
2.25.1

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

[Alexandre Bailon]

This adds a virtual thermal sensor driver that reads temperature from
multiple hardware sensors and returns an aggregated temperature.
Currently, this supports three aggregations:
the minimum, maximum and average temperature.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/thermal/Kconfig                  |   8 +
 drivers/thermal/Makefile                 |   1 +
 drivers/thermal/virtual_thermal_sensor.c | 218 +++++++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 drivers/thermal/virtual_thermal_sensor.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index d7f44deab5b1..b326fae5ad1d 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 "DT-based virtual thermal sensor driver"
+	depends on THERMAL_OF || COMPILE_TEST
+	help
+	  This option enables the generic DT-based thermal sensor aggregator.
+	  This driver creates a thermal sensor that reads multiple hardware
+	  sensors and aggregates their output.
+
 config HISI_THERMAL
 	tristate "Hisilicon thermal driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 82fc3e616e54..8bf55973059c 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_thermal_sensor.o
diff --git a/drivers/thermal/virtual_thermal_sensor.c b/drivers/thermal/virtual_thermal_sensor.c
new file mode 100644
index 000000000000..9f38af925fd1
--- /dev/null
+++ b/drivers/thermal/virtual_thermal_sensor.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * virtual_thermal_sensor.c - DT-based virtual thermal sensor driver.
+ *
+ * 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>
+
+struct virtual_thermal_zone_device {
+	struct thermal_zone_device *zone;
+	struct module *owner;
+};
+
+struct virtual_thermal_sensor {
+	int count;
+	struct virtual_thermal_zone_device *zones;
+	struct thermal_zone_device *tzd;
+	int (*aggr_temp)(int temp1, int temp2);
+
+	struct list_head node;
+};
+
+static int max_temp(int temp1, int temp2)
+{
+	return max(temp1, temp2);
+}
+
+static int min_temp(int temp1, int temp2)
+{
+	return min(temp1, temp2);
+}
+
+static int avg_temp(int temp1, int temp2)
+{
+	return (temp1 + temp2) / 2;
+}
+
+static int virtual_thermal_sensor_get_temp(void *data, int *temperature)
+{
+	struct virtual_thermal_sensor *sensor = data;
+	int max_temp = INT_MIN;
+	int temp;
+	int i;
+
+	for (i = 0; i < sensor->count; i++) {
+		struct thermal_zone_device *zone;
+
+		zone = sensor->zones[i].zone;
+		zone->ops->get_temp(zone, &temp);
+		max_temp = sensor->aggr_temp(max_temp, temp);
+	}
+
+	*temperature = max_temp;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops virtual_thermal_sensor_ops = {
+	.get_temp = virtual_thermal_sensor_get_temp,
+};
+
+static int virtual_thermal_sensor_get_module(struct virtual_thermal_zone_device *zone,
+					     const char *name)
+{
+		struct platform_device *sensor_pdev;
+		struct device_node *node;
+
+		node = of_find_node_by_name(NULL, name);
+		if (!node)
+			return -ENODEV;
+
+		node = of_parse_phandle(node, "thermal-sensors", 0);
+		if (!node)
+			return -ENODEV;
+
+		sensor_pdev = of_find_device_by_node(node);
+		if (!sensor_pdev)
+			return -ENODEV;
+
+		if (!sensor_pdev->dev.driver)
+			return -EPROBE_DEFER;
+
+		if (!try_module_get(sensor_pdev->dev.driver->owner))
+			return -ENODEV;
+
+		zone->owner = sensor_pdev->dev.driver->owner;
+
+		return 0;
+}
+
+static void virtual_thermal_sensor_put_modules(struct virtual_thermal_sensor *sensor)
+{
+	int i;
+
+	for (i = 0; i < sensor->count; i++) {
+		if (sensor->zones[i].zone)
+			module_put(sensor->zones[i].owner);
+	}
+}
+
+static int virtual_thermal_sensor_probe(struct platform_device *pdev)
+{
+	struct virtual_thermal_sensor *sensor;
+	struct device *dev = &pdev->dev;
+	struct property *prop;
+	const char *name;
+	u32 type;
+	int ret;
+	int i = 0;
+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+	sensor->count = of_property_count_strings(dev->of_node, "thermal-sensors");
+	if (sensor->count <= 0)
+		return -EINVAL;
+
+	sensor->zones = devm_kmalloc_array(dev, sensor->count,
+					     sizeof(*sensor->zones),
+					     GFP_KERNEL);
+	if (!sensor->zones)
+		return -ENOMEM;
+
+	of_property_for_each_string(dev->of_node, "thermal-sensors", prop, name) {
+		struct virtual_thermal_zone_device *virtual_zone;
+		struct thermal_zone_device *zone;
+
+		virtual_zone = &sensor->zones[i++];
+
+		zone = thermal_zone_get_zone_by_name(name);
+		if (IS_ERR(zone))
+			return PTR_ERR(zone);
+
+		ret = virtual_thermal_sensor_get_module(virtual_zone, name);
+		if (ret)
+			goto err;
+
+		virtual_zone->zone = zone;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "aggregation-function", &type);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case VIRTUAL_THERMAL_SENSOR_MAX_VAL:
+		sensor->aggr_temp = max_temp;
+		break;
+	case VIRTUAL_THERMAL_SENSOR_MIN_VAL:
+		sensor->aggr_temp = min_temp;
+		break;
+	case VIRTUAL_THERMAL_SENSOR_AVG_VAL:
+		sensor->aggr_temp = avg_temp;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor,
+							   &virtual_thermal_sensor_ops);
+	if (IS_ERR(sensor->tzd))
+		return PTR_ERR(sensor->tzd);
+
+	platform_set_drvdata(pdev, sensor);
+
+	return 0;
+
+err:
+	virtual_thermal_sensor_put_modules(sensor);
+
+	return ret;
+}
+
+static int virtual_thermal_sensor_remove(struct platform_device *pdev)
+{
+	struct virtual_thermal_sensor *sensor;
+
+	sensor = platform_get_drvdata(pdev);
+	list_del(&sensor->node);
+
+	virtual_thermal_sensor_put_modules(sensor);
+
+	return 0;
+}
+
+static const struct of_device_id virtual_thermal_sensor_of_match[] = {
+	{
+		.compatible = "virtual,thermal-sensor",
+	},
+	{
+	},
+};
+MODULE_DEVICE_TABLE(of, virtual_thermal_sensor_of_match);
+
+static struct platform_driver virtual_thermal_sensor = {
+	.probe = virtual_thermal_sensor_probe,
+	.remove = virtual_thermal_sensor_remove,
+	.driver = {
+		.name = "virtual-thermal-sensor",
+		.of_match_table = virtual_thermal_sensor_of_match,
+	},
+};
+
+module_platform_driver(virtual_thermal_sensor);
+MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
+MODULE_DESCRIPTION("Virtual thermal sensor");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

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

[Rafael J. Wysocki]

On Fri, Nov 26, 2021 at 1:53 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This adds a virtual thermal sensor driver that reads temperature from
> multiple hardware sensors and returns an aggregated temperature.
> Currently, this supports three aggregations:
> the minimum, maximum and average temperature.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/thermal/Kconfig                  |   8 +
>  drivers/thermal/Makefile                 |   1 +
>  drivers/thermal/virtual_thermal_sensor.c | 218 +++++++++++++++++++++++
>  3 files changed, 227 insertions(+)
>  create mode 100644 drivers/thermal/virtual_thermal_sensor.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index d7f44deab5b1..b326fae5ad1d 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

The name is a bit overly generic IMV and that may be confusing.  It
would be good to use a name reflecting the fact that this is DT-based.
For example, something like thermal_of_virtual_sensor or similar.

> +       tristate "DT-based virtual thermal sensor driver"
> +       depends on THERMAL_OF || COMPILE_TEST
> +       help
> +         This option enables the generic DT-based thermal sensor aggregator.
> +         This driver creates a thermal sensor that reads multiple hardware
> +         sensors and aggregates their output.
> +
>  config HISI_THERMAL
>         tristate "Hisilicon thermal driver"
>         depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 82fc3e616e54..8bf55973059c 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_thermal_sensor.o
> diff --git a/drivers/thermal/virtual_thermal_sensor.c b/drivers/thermal/virtual_thermal_sensor.c
> new file mode 100644
> index 000000000000..9f38af925fd1
> --- /dev/null
> +++ b/drivers/thermal/virtual_thermal_sensor.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * virtual_thermal_sensor.c - DT-based virtual thermal sensor driver.

It would be good to have some more information on what it does here.

> + *
> + * 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>
> +
> +struct virtual_thermal_zone_device {
> +       struct thermal_zone_device *zone;
> +       struct module *owner;
> +};
> +
> +struct virtual_thermal_sensor {
> +       int count;
> +       struct virtual_thermal_zone_device *zones;
> +       struct thermal_zone_device *tzd;
> +       int (*aggr_temp)(int temp1, int temp2);
> +
> +       struct list_head node;
> +};
> +
> +static int max_temp(int temp1, int temp2)
> +{
> +       return max(temp1, temp2);
> +}
> +
> +static int min_temp(int temp1, int temp2)
> +{
> +       return min(temp1, temp2);
> +}
> +
> +static int avg_temp(int temp1, int temp2)
> +{
> +       return (temp1 + temp2) / 2;
> +}
> +
> +static int virtual_thermal_sensor_get_temp(void *data, int *temperature)
> +{
> +       struct virtual_thermal_sensor *sensor = data;
> +       int max_temp = INT_MIN;
> +       int temp;
> +       int i;
> +
> +       for (i = 0; i < sensor->count; i++) {
> +               struct thermal_zone_device *zone;
> +
> +               zone = sensor->zones[i].zone;

What if one of the zones gets unregistered after registering a thermal
sensor depending on it?

I don't think this will still work in that case.

> +               zone->ops->get_temp(zone, &temp);
> +               max_temp = sensor->aggr_temp(max_temp, temp);
> +       }
> +
> +       *temperature = max_temp;
> +
> +       return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops virtual_thermal_sensor_ops = {
> +       .get_temp = virtual_thermal_sensor_get_temp,
> +};
> +
> +static int virtual_thermal_sensor_get_module(struct virtual_thermal_zone_device *zone,
> +                                            const char *name)
> +{
> +               struct platform_device *sensor_pdev;
> +               struct device_node *node;
> +
> +               node = of_find_node_by_name(NULL, name);
> +               if (!node)
> +                       return -ENODEV;
> +
> +               node = of_parse_phandle(node, "thermal-sensors", 0);
> +               if (!node)
> +                       return -ENODEV;
> +
> +               sensor_pdev = of_find_device_by_node(node);
> +               if (!sensor_pdev)
> +                       return -ENODEV;
> +
> +               if (!sensor_pdev->dev.driver)
> +                       return -EPROBE_DEFER;
> +
> +               if (!try_module_get(sensor_pdev->dev.driver->owner))
> +                       return -ENODEV;
> +
> +               zone->owner = sensor_pdev->dev.driver->owner;
> +
> +               return 0;
> +}
> +
> +static void virtual_thermal_sensor_put_modules(struct virtual_thermal_sensor *sensor)
> +{
> +       int i;
> +
> +       for (i = 0; i < sensor->count; i++) {
> +               if (sensor->zones[i].zone)
> +                       module_put(sensor->zones[i].owner);
> +       }
> +}
> +
> +static int virtual_thermal_sensor_probe(struct platform_device *pdev)
> +{
> +       struct virtual_thermal_sensor *sensor;
> +       struct device *dev = &pdev->dev;
> +       struct property *prop;
> +       const char *name;
> +       u32 type;
> +       int ret;
> +       int i = 0;
> +
> +       sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +       if (!sensor)
> +               return -ENOMEM;
> +       sensor->count = of_property_count_strings(dev->of_node, "thermal-sensors");
> +       if (sensor->count <= 0)
> +               return -EINVAL;
> +
> +       sensor->zones = devm_kmalloc_array(dev, sensor->count,
> +                                            sizeof(*sensor->zones),
> +                                            GFP_KERNEL);
> +       if (!sensor->zones)
> +               return -ENOMEM;
> +
> +       of_property_for_each_string(dev->of_node, "thermal-sensors", prop, name) {
> +               struct virtual_thermal_zone_device *virtual_zone;
> +               struct thermal_zone_device *zone;
> +
> +               virtual_zone = &sensor->zones[i++];
> +
> +               zone = thermal_zone_get_zone_by_name(name);
> +               if (IS_ERR(zone))
> +                       return PTR_ERR(zone);
> +
> +               ret = virtual_thermal_sensor_get_module(virtual_zone, name);
> +               if (ret)
> +                       goto err;

The above prevents the module holding the zone driver from being
unloaded while it is used by this virtual sensor, but it still doesn't
prevent its zone from being unregistered after probing the virtual
sensor.

> +
> +               virtual_zone->zone = zone;
> +       }
> +
> +       ret = of_property_read_u32(dev->of_node, "aggregation-function", &type);
> +       if (ret)
> +               return ret;
> +
> +       switch (type) {
> +       case VIRTUAL_THERMAL_SENSOR_MAX_VAL:
> +               sensor->aggr_temp = max_temp;
> +               break;
> +       case VIRTUAL_THERMAL_SENSOR_MIN_VAL:
> +               sensor->aggr_temp = min_temp;
> +               break;
> +       case VIRTUAL_THERMAL_SENSOR_AVG_VAL:
> +               sensor->aggr_temp = avg_temp;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor,
> +                                                          &virtual_thermal_sensor_ops);
> +       if (IS_ERR(sensor->tzd))
> +               return PTR_ERR(sensor->tzd);
> +
> +       platform_set_drvdata(pdev, sensor);
> +
> +       return 0;
> +
> +err:
> +       virtual_thermal_sensor_put_modules(sensor);
> +
> +       return ret;
> +}
> +
> +static int virtual_thermal_sensor_remove(struct platform_device *pdev)
> +{
> +       struct virtual_thermal_sensor *sensor;
> +
> +       sensor = platform_get_drvdata(pdev);
> +       list_del(&sensor->node);
> +
> +       virtual_thermal_sensor_put_modules(sensor);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id virtual_thermal_sensor_of_match[] = {
> +       {
> +               .compatible = "virtual,thermal-sensor",
> +       },
> +       {
> +       },
> +};
> +MODULE_DEVICE_TABLE(of, virtual_thermal_sensor_of_match);
> +
> +static struct platform_driver virtual_thermal_sensor = {
> +       .probe = virtual_thermal_sensor_probe,
> +       .remove = virtual_thermal_sensor_remove,
> +       .driver = {
> +               .name = "virtual-thermal-sensor",
> +               .of_match_table = virtual_thermal_sensor_of_match,
> +       },
> +};
> +
> +module_platform_driver(virtual_thermal_sensor);
> +MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
> +MODULE_DESCRIPTION("Virtual thermal sensor");
> +MODULE_LICENSE("GPL v2");
> --

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

[Rafael J. Wysocki]

On Fri, Jan 28, 2022 at 8:31 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Nov 26, 2021 at 1:53 PM Alexandre Bailon <abailon@baylibre.com> wrote:
> >
> > This adds a virtual thermal sensor driver that reads temperature from
> > multiple hardware sensors and returns an aggregated temperature.
> > Currently, this supports three aggregations:
> > the minimum, maximum and average temperature.
> >
> > Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> > ---
> >  drivers/thermal/Kconfig                  |   8 +
> >  drivers/thermal/Makefile                 |   1 +
> >  drivers/thermal/virtual_thermal_sensor.c | 218 +++++++++++++++++++++++
> >  3 files changed, 227 insertions(+)
> >  create mode 100644 drivers/thermal/virtual_thermal_sensor.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index d7f44deab5b1..b326fae5ad1d 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
>
> The name is a bit overly generic IMV and that may be confusing.  It
> would be good to use a name reflecting the fact that this is DT-based.
> For example, something like thermal_of_virtual_sensor or similar.
>
> > +       tristate "DT-based virtual thermal sensor driver"
> > +       depends on THERMAL_OF || COMPILE_TEST
> > +       help
> > +         This option enables the generic DT-based thermal sensor aggregator.
> > +         This driver creates a thermal sensor that reads multiple hardware
> > +         sensors and aggregates their output.
> > +
> >  config HISI_THERMAL
> >         tristate "Hisilicon thermal driver"
> >         depends on ARCH_HISI || COMPILE_TEST
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 82fc3e616e54..8bf55973059c 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_thermal_sensor.o
> > diff --git a/drivers/thermal/virtual_thermal_sensor.c b/drivers/thermal/virtual_thermal_sensor.c
> > new file mode 100644
> > index 000000000000..9f38af925fd1
> > --- /dev/null
> > +++ b/drivers/thermal/virtual_thermal_sensor.c
> > @@ -0,0 +1,218 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtual_thermal_sensor.c - DT-based virtual thermal sensor driver.
>
> It would be good to have some more information on what it does here.
>
> > + *
> > + * 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>
> > +
> > +struct virtual_thermal_zone_device {
> > +       struct thermal_zone_device *zone;
> > +       struct module *owner;
> > +};
> > +
> > +struct virtual_thermal_sensor {
> > +       int count;
> > +       struct virtual_thermal_zone_device *zones;
> > +       struct thermal_zone_device *tzd;
> > +       int (*aggr_temp)(int temp1, int temp2);
> > +
> > +       struct list_head node;
> > +};
> > +
> > +static int max_temp(int temp1, int temp2)
> > +{
> > +       return max(temp1, temp2);
> > +}
> > +
> > +static int min_temp(int temp1, int temp2)
> > +{
> > +       return min(temp1, temp2);
> > +}
> > +
> > +static int avg_temp(int temp1, int temp2)
> > +{
> > +       return (temp1 + temp2) / 2;
> > +}
> > +
> > +static int virtual_thermal_sensor_get_temp(void *data, int *temperature)
> > +{
> > +       struct virtual_thermal_sensor *sensor = data;
> > +       int max_temp = INT_MIN;
> > +       int temp;
> > +       int i;
> > +
> > +       for (i = 0; i < sensor->count; i++) {
> > +               struct thermal_zone_device *zone;
> > +
> > +               zone = sensor->zones[i].zone;
>
> What if one of the zones gets unregistered after registering a thermal
> sensor depending on it?
>
> I don't think this will still work in that case.
>
> > +               zone->ops->get_temp(zone, &temp);
> > +               max_temp = sensor->aggr_temp(max_temp, temp);

One more thing: AFAICS, this is only going to work for the "average"
type of aggregation if there are two sensors involved.  For three (or
more), you won't really get the average.  For instance, for three
sensors the current patch leads to t = t_0 / 4 + t_1 / 4 + t_2 / 2
whereas the average is the sum of t_i divided by the number of them
(3).

It looks like you need to check the aggregation type here directly
instead of using the callback approach, which would also make it
easier to follow the code.

> > +       }
> > +
> > +       *temperature = max_temp;
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops virtual_thermal_sensor_ops = {
> > +       .get_temp = virtual_thermal_sensor_get_temp,
> > +};
> > +
> > +static int virtual_thermal_sensor_get_module(struct virtual_thermal_zone_device *zone,
> > +                                            const char *name)
> > +{
> > +               struct platform_device *sensor_pdev;
> > +               struct device_node *node;
> > +
> > +               node = of_find_node_by_name(NULL, name);
> > +               if (!node)
> > +                       return -ENODEV;
> > +
> > +               node = of_parse_phandle(node, "thermal-sensors", 0);
> > +               if (!node)
> > +                       return -ENODEV;
> > +
> > +               sensor_pdev = of_find_device_by_node(node);
> > +               if (!sensor_pdev)
> > +                       return -ENODEV;
> > +
> > +               if (!sensor_pdev->dev.driver)
> > +                       return -EPROBE_DEFER;
> > +
> > +               if (!try_module_get(sensor_pdev->dev.driver->owner))
> > +                       return -ENODEV;
> > +
> > +               zone->owner = sensor_pdev->dev.driver->owner;
> > +
> > +               return 0;
> > +}
> > +
> > +static void virtual_thermal_sensor_put_modules(struct virtual_thermal_sensor *sensor)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < sensor->count; i++) {
> > +               if (sensor->zones[i].zone)
> > +                       module_put(sensor->zones[i].owner);
> > +       }
> > +}
> > +
> > +static int virtual_thermal_sensor_probe(struct platform_device *pdev)
> > +{
> > +       struct virtual_thermal_sensor *sensor;
> > +       struct device *dev = &pdev->dev;
> > +       struct property *prop;
> > +       const char *name;
> > +       u32 type;
> > +       int ret;
> > +       int i = 0;
> > +
> > +       sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> > +       if (!sensor)
> > +               return -ENOMEM;
> > +       sensor->count = of_property_count_strings(dev->of_node, "thermal-sensors");
> > +       if (sensor->count <= 0)
> > +               return -EINVAL;
> > +
> > +       sensor->zones = devm_kmalloc_array(dev, sensor->count,
> > +                                            sizeof(*sensor->zones),
> > +                                            GFP_KERNEL);
> > +       if (!sensor->zones)
> > +               return -ENOMEM;
> > +
> > +       of_property_for_each_string(dev->of_node, "thermal-sensors", prop, name) {
> > +               struct virtual_thermal_zone_device *virtual_zone;
> > +               struct thermal_zone_device *zone;
> > +
> > +               virtual_zone = &sensor->zones[i++];
> > +
> > +               zone = thermal_zone_get_zone_by_name(name);
> > +               if (IS_ERR(zone))
> > +                       return PTR_ERR(zone);
> > +
> > +               ret = virtual_thermal_sensor_get_module(virtual_zone, name);
> > +               if (ret)
> > +                       goto err;
>
> The above prevents the module holding the zone driver from being
> unloaded while it is used by this virtual sensor, but it still doesn't
> prevent its zone from being unregistered after probing the virtual
> sensor.
>
> > +
> > +               virtual_zone->zone = zone;
> > +       }
> > +
> > +       ret = of_property_read_u32(dev->of_node, "aggregation-function", &type);
> > +       if (ret)
> > +               return ret;
> > +
> > +       switch (type) {
> > +       case VIRTUAL_THERMAL_SENSOR_MAX_VAL:
> > +               sensor->aggr_temp = max_temp;
> > +               break;
> > +       case VIRTUAL_THERMAL_SENSOR_MIN_VAL:
> > +               sensor->aggr_temp = min_temp;
> > +               break;
> > +       case VIRTUAL_THERMAL_SENSOR_AVG_VAL:
> > +               sensor->aggr_temp = avg_temp;
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor,
> > +                                                          &virtual_thermal_sensor_ops);
> > +       if (IS_ERR(sensor->tzd))
> > +               return PTR_ERR(sensor->tzd);
> > +
> > +       platform_set_drvdata(pdev, sensor);
> > +
> > +       return 0;
> > +
> > +err:
> > +       virtual_thermal_sensor_put_modules(sensor);
> > +
> > +       return ret;
> > +}
> > +
> > +static int virtual_thermal_sensor_remove(struct platform_device *pdev)
> > +{
> > +       struct virtual_thermal_sensor *sensor;
> > +
> > +       sensor = platform_get_drvdata(pdev);
> > +       list_del(&sensor->node);
> > +
> > +       virtual_thermal_sensor_put_modules(sensor);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id virtual_thermal_sensor_of_match[] = {
> > +       {
> > +               .compatible = "virtual,thermal-sensor",
> > +       },
> > +       {
> > +       },
> > +};
> > +MODULE_DEVICE_TABLE(of, virtual_thermal_sensor_of_match);
> > +
> > +static struct platform_driver virtual_thermal_sensor = {
> > +       .probe = virtual_thermal_sensor_probe,
> > +       .remove = virtual_thermal_sensor_remove,
> > +       .driver = {
> > +               .name = "virtual-thermal-sensor",
> > +               .of_match_table = virtual_thermal_sensor_of_match,
> > +       },
> > +};
> > +
> > +module_platform_driver(virtual_thermal_sensor);
> > +MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
> > +MODULE_DESCRIPTION("Virtual thermal sensor");
> > +MODULE_LICENSE("GPL v2");
> > --