[PATCH v2 0/3] thermal: Add support of multiple sensors

[PATCH v2 0/3] thermal: Add support of multiple sensors

[Alexandre Bailon]

Following this comment [1], this updates thermal_of to support multiple
sensors.

This has some limitations:
- A sensor must have its own termal zone, even if it is also registered
  inside a thermal zone supporting multiple sensors.
- Only support weighted average

Changes in v2:
- Rebased on 6.7
- Seperated generic multi sensor and dt specfic code
- Simplified the code
- Drop min / max and only do weighted average (seems more adequate for IPA)

[1]: https://patchwork.kernel.org/comment/24723927/

Alexandre Bailon (3):
  dt-bindings: thermal: Restore the thermal-sensors property
  thermal: Add support of multi sensors to thermal_core
  thermal: Add support of multi sensors to thermal_of

 .../bindings/thermal/thermal-zones.yaml       |   5 +-
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/thermal_core.h                |   7 +
 drivers/thermal/thermal_multi.c               | 178 ++++++++++++++++++
 drivers/thermal/thermal_of.c                  | 139 ++++++++++++++
 5 files changed, 327 insertions(+), 3 deletions(-)
 create mode 100644 drivers/thermal/thermal_multi.c

-- 
2.41.0

[PATCH v2 1/3] dt-bindings: thermal: Restore the thermal-sensors property

[Alexandre Bailon]

thermal-sensors was defined in thermal.txt but when the yaml binding
has been defined, its definition has changed, dropping support of multi
sensors.
Since we are adding support of multi sensors, use the original definition
for thermal-sensors property.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 Documentation/devicetree/bindings/thermal/thermal-zones.yaml | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
index 4a8dabc48170..b2988758d6ff 100644
--- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
+++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
@@ -77,10 +77,9 @@ patternProperties:
 
       thermal-sensors:
         $ref: /schemas/types.yaml#/definitions/phandle-array
-        maxItems: 1
         description:
-          The thermal sensor phandle and sensor specifier used to monitor this
-          thermal zone.
+          A list of thermal sensor phandles and sensor specifier
+          used while monitoring the thermal zone.
 
       coefficients:
         $ref: /schemas/types.yaml#/definitions/uint32-array
-- 
2.41.0

[PATCH v2 2/3] thermal: Add support of multi sensors to thermal_core

[Alexandre Bailon]

This adds support of multi sensors to thermal.
Currently, this only support the get_temp operation.
This returns an average temperature of all the sensors.
If defined, a coefficient is applied to the value read from the sensor
before computing the average.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/thermal/Makefile        |   1 +
 drivers/thermal/thermal_core.h  |   7 ++
 drivers/thermal/thermal_multi.c | 178 ++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)
 create mode 100644 drivers/thermal/thermal_multi.c

diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index c934cab309ae..757289a406f7 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -6,6 +6,7 @@ CFLAGS_thermal_core.o		:= -I$(src)
 obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 thermal_sys-y			+= thermal_core.o thermal_sysfs.o
 thermal_sys-y			+= thermal_trip.o thermal_helpers.o
+thermal_sys-y			+= thermal_multi.o
 
 # netlink interface to manage the thermal framework
 thermal_sys-$(CONFIG_THERMAL_NETLINK)		+= thermal_netlink.o
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 0a3b3ec5120b..26e83a5c8298 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -138,6 +138,13 @@ ssize_t weight_show(struct device *, struct device_attribute *, char *);
 ssize_t weight_store(struct device *, struct device_attribute *, const char *,
 		     size_t);
 
+/* Multi sensors */
+int thermal_multi_sensor_validate_coeff(int *coeff, int count, int offset);
+int thermal_multi_sensor_register(const char *name,
+	struct thermal_zone_device *sensor_tz, int coeff);
+void thermal_multi_sensor_unregister(struct thermal_zone_device *sensor_tz);
+
+
 #ifdef CONFIG_THERMAL_STATISTICS
 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
 					 unsigned long new_state);
diff --git a/drivers/thermal/thermal_multi.c b/drivers/thermal/thermal_multi.c
new file mode 100644
index 000000000000..a5a4f1f2d594
--- /dev/null
+++ b/drivers/thermal/thermal_multi.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+#include <linux/types.h>
+#include <linux/string.h>
+
+#include "thermal_core.h"
+
+struct sensor_interface {
+	struct thermal_zone_device *tz;
+	int coeff;
+
+	struct list_head node;
+};
+
+struct multi_sensor_thermal_zone {
+	struct thermal_zone_device *tz;
+	struct mutex sensors_lock;
+	struct list_head sensors;
+
+	struct list_head node;
+};
+
+static DEFINE_MUTEX(multi_tz_mutex);
+static LIST_HEAD(multi_tz_list);
+
+#define TJ_MAX 120000
+
+static int multi_sensor_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+	struct multi_sensor_thermal_zone *multi_tz = tz->devdata;
+	struct sensor_interface *sensor;
+	int accumulated_temp = 0;
+	u32 accumulated_coeff;
+	int ret;
+
+	mutex_lock(&multi_tz->sensors_lock);
+
+	if (list_empty(&multi_tz->sensors)) {
+		mutex_unlock(&multi_tz->sensors_lock);
+		return -ENODEV;
+	}
+
+	list_for_each_entry(sensor, &multi_tz->sensors, node) {
+		ret = thermal_zone_get_temp(sensor->tz, temp);
+		if (ret) {
+			mutex_unlock(&multi_tz->sensors_lock);
+			return ret;
+		}
+
+		accumulated_temp += *temp * sensor->coeff;
+		accumulated_coeff += sensor->coeff;
+	}
+
+	mutex_unlock(&multi_tz->sensors_lock);
+
+	*temp = accumulated_temp / accumulated_coeff;
+	return ret;
+}
+
+struct thermal_zone_device_ops multi_sensor_ops = {
+	.get_temp = multi_sensor_get_temp,
+};
+
+int thermal_multi_sensor_validate_coeff(int *coeff, int count, int offset)
+{
+	int max_accumulated_temp = 0;
+	int i;
+
+	for (i = 0; i < count; i++) {
+		max_accumulated_temp += TJ_MAX * coeff[i];
+		if (max_accumulated_temp < 0)
+			return -EOVERFLOW;
+	}
+
+	max_accumulated_temp += offset;
+	return max_accumulated_temp < 0 ? -EOVERFLOW : 0;
+}
+
+static struct thermal_zone_device *multi_sensor_tz_alloc(const char *name)
+{
+	struct thermal_zone_device *tz;
+	struct thermal_zone_params tzp = {};
+	struct multi_sensor_thermal_zone *multi_tz;
+
+	tz = thermal_zone_get_zone_by_name(name);
+	if (!IS_ERR(tz)) {
+		mutex_unlock(&multi_tz_mutex);
+		return tz;
+	}
+
+	multi_tz = kzalloc(sizeof(*multi_tz), GFP_KERNEL);
+	if (!multi_tz)
+		return ERR_PTR(-ENOMEM);
+	mutex_init(&multi_tz->sensors_lock);
+	INIT_LIST_HEAD(&multi_tz->sensors);
+
+	tzp.no_hwmon = true;
+	tzp.slope = 1;
+	tzp.offset = 0;
+
+	tz = thermal_tripless_zone_device_register(name, multi_tz,
+						   &multi_sensor_ops, &tzp);
+	if (IS_ERR(tz)) {
+		kfree(multi_tz);
+	} else {
+		multi_tz->tz = tz;
+		list_add(&multi_tz->node, &multi_tz_list);
+	}
+
+	return tz;
+}
+
+int thermal_multi_sensor_register(const char *name,
+	struct thermal_zone_device *sensor_tz, int coeff)
+{
+	struct thermal_zone_device *tz;
+	struct multi_sensor_thermal_zone *multi_tz;
+	struct sensor_interface *sensor;
+
+	mutex_lock(&multi_tz_mutex);
+
+	tz = multi_sensor_tz_alloc(name);
+	if (IS_ERR(tz)) {
+		mutex_unlock(&multi_tz_mutex);
+		return PTR_ERR(tz);
+	}
+	multi_tz =  tz->devdata;
+
+	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	if (!sensor) {
+		mutex_unlock(&multi_tz_mutex);
+		return -ENOMEM;
+	}
+
+	sensor->tz = sensor_tz;
+	sensor->coeff = coeff;
+	mutex_lock(&multi_tz->sensors_lock);
+	list_add(&sensor->node, &multi_tz->sensors);
+	mutex_unlock(&multi_tz->sensors_lock);
+
+	thermal_zone_device_enable(tz);
+
+	mutex_unlock(&multi_tz_mutex);
+
+	return 0;
+}
+
+void thermal_multi_sensor_unregister(struct thermal_zone_device *sensor_tz)
+{
+	struct multi_sensor_thermal_zone *multi_tz;
+	struct sensor_interface *sensor, *tmp;
+
+	mutex_lock(&multi_tz_mutex);
+	list_for_each_entry(multi_tz, &multi_tz_list, node) {
+		mutex_lock(&multi_tz->sensors_lock);
+		list_for_each_entry_safe(sensor, tmp, &multi_tz->sensors, node) {
+			if (sensor->tz == sensor_tz) {
+				list_del(&sensor->node);
+				kfree(sensor);
+				break;
+			}
+		}
+
+		if (list_empty(&multi_tz->sensors)) {
+			thermal_zone_device_unregister(multi_tz->tz);
+			mutex_unlock(&multi_tz->sensors_lock);
+			kfree(multi_tz);
+		} else {
+			mutex_unlock(&multi_tz->sensors_lock);
+		}
+	}
+	mutex_unlock(&multi_tz_mutex);
+}
-- 
2.41.0

[PATCH v2 3/3] thermal: Add support of multi sensors to thermal_of

[Alexandre Bailon]

This updates thermal_of to support more than one sensor.
If during the registration we find another thermal zone referencing
this sensors and some other, then we create the multi sensor thermal
zone (if it doesn't exist) and register the sensor to it.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/thermal/thermal_of.c | 139 +++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 1e0655b63259..3f36d8a3d8e8 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -441,12 +441,146 @@ static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
 	struct thermal_trip *trips = tz->trips;
 	struct thermal_zone_device_ops *ops = tz->ops;
 
+	thermal_multi_sensor_unregister(tz);
 	thermal_zone_device_disable(tz);
 	thermal_zone_device_unregister(tz);
 	kfree(trips);
 	kfree(ops);
 }
 
+int thermal_of_get_sensor_id(struct device_node *tz_np,
+			    struct device_node *sensor_np,
+			    int phandle_index, u32 *id)
+{
+	struct of_phandle_args sensor_specs;
+	int ret;
+
+	ret = of_parse_phandle_with_args(tz_np,
+					 "thermal-sensors",
+					 "#thermal-sensor-cells",
+					 phandle_index,
+					 &sensor_specs);
+	if (ret)
+		return ret;
+
+	if (sensor_specs.np != sensor_np) {
+		of_node_put(sensor_specs.np);
+		return -ENODEV;
+	}
+
+	if (sensor_specs.args_count > 1)
+		pr_warn("%pOFn: too many cells in sensor specifier %d\n",
+			sensor_specs.np, sensor_specs.args_count);
+
+	*id = sensor_specs.args_count ? sensor_specs.args[0] : 0;
+	of_node_put(sensor_specs.np);
+
+	return 0;
+}
+
+static int thermal_of_has_sensor_id(struct device_node *tz_np,
+				    struct device_node *sensor_np,
+				    u32 sensor_id)
+{
+	int count;
+	int i;
+
+	count = of_count_phandle_with_args(tz_np,
+					   "thermal-sensors",
+					   "#thermal-sensor-cells");
+	if (count <= 0)
+		return -ENODEV;
+
+	for (i = 0; i < count; i++) {
+		int ret;
+		u32 id;
+
+		ret = thermal_of_get_sensor_id(tz_np, sensor_np, i, &id);
+		if (ret)
+			return ret;
+
+		if (id == sensor_id)
+			return i;
+
+	}
+
+	return -ENODEV;
+}
+
+static int thermal_of_register_mutli_sensor(struct device_node *sensor, int id,
+					    struct thermal_zone_device *tz,
+					    struct device_node *tz_np)
+{
+	struct device_node *child;
+	u32 *coeff;
+	int ret;
+	int i;
+
+	/*
+	 * Go through all the thermal zone and check if the sensor is
+	 * referenced. If so, find or create a multi sensor thermal zone
+	 * and register the sensor to it.
+	 */
+	for_each_available_child_of_node(of_get_parent(tz_np), child) {
+		int count;
+		int index;
+		int offset;
+
+		/* Skip the tz that is currently registering */
+		if (child == tz_np)
+			continue;
+
+		/* Test if the sensor is referenced by a tz*/
+		index = thermal_of_has_sensor_id(child, sensor, id);
+		if (index < 0)
+			continue;
+
+		/*
+		 * Get the coefficients and offset and assign them to the
+		 * multi sensor thermal zone.
+		 */
+		count = of_count_phandle_with_args(child,
+						   "thermal-sensors",
+						   "#thermal-sensor-cells");
+		coeff = kmalloc_array(count, sizeof(*coeff), GFP_KERNEL);
+		if (!coeff)
+			goto err;
+
+		for (i = 0; i < count; i++) {
+			ret = of_property_read_u32_index(child,
+							 "coefficients",
+							 i, coeff + i);
+			if (ret)
+				coeff[i] = 1;
+		}
+
+		ret = of_property_read_u32_index(child, "coefficients",
+						 count, &offset);
+		if (ret)
+			offset = 0;
+
+		/* Make sure the coeff and offset won't cause an overflow */
+		ret = thermal_multi_sensor_validate_coeff(coeff, count, offset);
+		if (ret)
+			goto err_free_coeff;
+
+		ret = thermal_multi_sensor_register(child->name, tz,
+							 coeff[index]);
+		if (ret)
+			goto err_free_coeff;
+		kfree(coeff);
+	}
+
+	return 0;
+
+err_free_coeff:
+	kfree(coeff);
+err:
+	thermal_multi_sensor_unregister(tz);
+
+	return ret;
+}
+
 /**
  * thermal_of_zone_register - Register a thermal zone with device node
  * sensor
@@ -528,6 +662,11 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
 		return ERR_PTR(ret);
 	}
 
+	/* Register the sensor to all other thermal zone referencing it */
+	ret = thermal_of_register_mutli_sensor(sensor, id, tz, np);
+	if (ret)
+		pr_warn("Failed to register a sensor to a multi sensor tz\n");
+
 	return tz;
 
 out_kfree_trips:
-- 
2.41.0

Re: [PATCH v2 1/3] dt-bindings: thermal: Restore the thermal-sensors property

[Rob Herring]

On Fri, 19 Jan 2024 12:08:40 +0100, Alexandre Bailon wrote:
> thermal-sensors was defined in thermal.txt but when the yaml binding
> has been defined, its definition has changed, dropping support of multi
> sensors.
> Since we are adding support of multi sensors, use the original definition
> for thermal-sensors property.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  Documentation/devicetree/bindings/thermal/thermal-zones.yaml | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/3] thermal: Add support of multi sensors to thermal_core

[Pin-yen Lin]

Hi Alexandre,

On Thu, Apr 11, 2024 at 4:34 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This adds support of multi sensors to thermal.
> Currently, this only support the get_temp operation.
> This returns an average temperature of all the sensors.
> If defined, a coefficient is applied to the value read from the sensor
> before computing the average.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/thermal/Makefile        |   1 +
>  drivers/thermal/thermal_core.h  |   7 ++
>  drivers/thermal/thermal_multi.c | 178 ++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
>  create mode 100644 drivers/thermal/thermal_multi.c
>
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c934cab309ae..757289a406f7 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -6,6 +6,7 @@ CFLAGS_thermal_core.o           := -I$(src)
>  obj-$(CONFIG_THERMAL)          += thermal_sys.o
>  thermal_sys-y                  += thermal_core.o thermal_sysfs.o
>  thermal_sys-y                  += thermal_trip.o thermal_helpers.o
> +thermal_sys-y                  += thermal_multi.o
>
>  # netlink interface to manage the thermal framework
>  thermal_sys-$(CONFIG_THERMAL_NETLINK)          += thermal_netlink.o
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 0a3b3ec5120b..26e83a5c8298 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -138,6 +138,13 @@ ssize_t weight_show(struct device *, struct device_attribute *, char *);
>  ssize_t weight_store(struct device *, struct device_attribute *, const char *,
>                      size_t);
>
> +/* Multi sensors */
> +int thermal_multi_sensor_validate_coeff(int *coeff, int count, int offset);
> +int thermal_multi_sensor_register(const char *name,
> +       struct thermal_zone_device *sensor_tz, int coeff);
> +void thermal_multi_sensor_unregister(struct thermal_zone_device *sensor_tz);
> +
> +
>  #ifdef CONFIG_THERMAL_STATISTICS
>  void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>                                          unsigned long new_state);
> diff --git a/drivers/thermal/thermal_multi.c b/drivers/thermal/thermal_multi.c
> new file mode 100644
> index 000000000000..a5a4f1f2d594
> --- /dev/null
> +++ b/drivers/thermal/thermal_multi.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +
> +#include "thermal_core.h"
> +
> +struct sensor_interface {
> +       struct thermal_zone_device *tz;
> +       int coeff;
> +
> +       struct list_head node;
> +};
> +
> +struct multi_sensor_thermal_zone {
> +       struct thermal_zone_device *tz;
> +       struct mutex sensors_lock;
> +       struct list_head sensors;
> +
> +       struct list_head node;
> +};
> +
> +static DEFINE_MUTEX(multi_tz_mutex);
> +static LIST_HEAD(multi_tz_list);
> +
> +#define TJ_MAX 120000
> +
> +static int multi_sensor_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +       struct multi_sensor_thermal_zone *multi_tz = tz->devdata;
> +       struct sensor_interface *sensor;
> +       int accumulated_temp = 0;
> +       u32 accumulated_coeff;

Should we initialize accumulated_coeff to 0 as well?

> +       int ret;
> +
> +       mutex_lock(&multi_tz->sensors_lock);
> +
> +       if (list_empty(&multi_tz->sensors)) {
> +               mutex_unlock(&multi_tz->sensors_lock);
> +               return -ENODEV;
> +       }
> +
> +       list_for_each_entry(sensor, &multi_tz->sensors, node) {
> +               ret = thermal_zone_get_temp(sensor->tz, temp);
> +               if (ret) {
> +                       mutex_unlock(&multi_tz->sensors_lock);
> +                       return ret;
> +               }
> +
> +               accumulated_temp += *temp * sensor->coeff;
> +               accumulated_coeff += sensor->coeff;
> +       }
> +
> +       mutex_unlock(&multi_tz->sensors_lock);
> +
> +       *temp = accumulated_temp / accumulated_coeff;
> +       return ret;
> +}
> +
> +struct thermal_zone_device_ops multi_sensor_ops = {
> +       .get_temp = multi_sensor_get_temp,
> +};
> +
> +int thermal_multi_sensor_validate_coeff(int *coeff, int count, int offset)
> +{
> +       int max_accumulated_temp = 0;
> +       int i;
> +
> +       for (i = 0; i < count; i++) {
> +               max_accumulated_temp += TJ_MAX * coeff[i];
> +               if (max_accumulated_temp < 0)
> +                       return -EOVERFLOW;
> +       }
> +
> +       max_accumulated_temp += offset;
> +       return max_accumulated_temp < 0 ? -EOVERFLOW : 0;
> +}
> +
> +static struct thermal_zone_device *multi_sensor_tz_alloc(const char *name)
> +{
> +       struct thermal_zone_device *tz;
> +       struct thermal_zone_params tzp = {};
> +       struct multi_sensor_thermal_zone *multi_tz;
> +
> +       tz = thermal_zone_get_zone_by_name(name);
> +       if (!IS_ERR(tz)) {
> +               mutex_unlock(&multi_tz_mutex);
> +               return tz;
> +       }
> +
> +       multi_tz = kzalloc(sizeof(*multi_tz), GFP_KERNEL);
> +       if (!multi_tz)
> +               return ERR_PTR(-ENOMEM);
> +       mutex_init(&multi_tz->sensors_lock);
> +       INIT_LIST_HEAD(&multi_tz->sensors);
> +
> +       tzp.no_hwmon = true;
> +       tzp.slope = 1;
> +       tzp.offset = 0;
> +
> +       tz = thermal_tripless_zone_device_register(name, multi_tz,
> +                                                  &multi_sensor_ops, &tzp);
> +       if (IS_ERR(tz)) {
> +               kfree(multi_tz);
> +       } else {
> +               multi_tz->tz = tz;
> +               list_add(&multi_tz->node, &multi_tz_list);
> +       }
> +
> +       return tz;
> +}
> +
> +int thermal_multi_sensor_register(const char *name,
> +       struct thermal_zone_device *sensor_tz, int coeff)
> +{
> +       struct thermal_zone_device *tz;
> +       struct multi_sensor_thermal_zone *multi_tz;
> +       struct sensor_interface *sensor;
> +
> +       mutex_lock(&multi_tz_mutex);
> +
> +       tz = multi_sensor_tz_alloc(name);
> +       if (IS_ERR(tz)) {
> +               mutex_unlock(&multi_tz_mutex);
> +               return PTR_ERR(tz);
> +       }
> +       multi_tz =  tz->devdata;
> +
> +       sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> +       if (!sensor) {
> +               mutex_unlock(&multi_tz_mutex);
> +               return -ENOMEM;
> +       }
> +
> +       sensor->tz = sensor_tz;
> +       sensor->coeff = coeff;
> +       mutex_lock(&multi_tz->sensors_lock);
> +       list_add(&sensor->node, &multi_tz->sensors);
> +       mutex_unlock(&multi_tz->sensors_lock);
> +
> +       thermal_zone_device_enable(tz);
> +
> +       mutex_unlock(&multi_tz_mutex);
> +
> +       return 0;
> +}
> +
> +void thermal_multi_sensor_unregister(struct thermal_zone_device *sensor_tz)
> +{
> +       struct multi_sensor_thermal_zone *multi_tz;
> +       struct sensor_interface *sensor, *tmp;
> +
> +       mutex_lock(&multi_tz_mutex);
> +       list_for_each_entry(multi_tz, &multi_tz_list, node) {
> +               mutex_lock(&multi_tz->sensors_lock);
> +               list_for_each_entry_safe(sensor, tmp, &multi_tz->sensors, node) {
> +                       if (sensor->tz == sensor_tz) {
> +                               list_del(&sensor->node);
> +                               kfree(sensor);
> +                               break;
> +                       }
> +               }
> +
> +               if (list_empty(&multi_tz->sensors)) {
> +                       thermal_zone_device_unregister(multi_tz->tz);
> +                       mutex_unlock(&multi_tz->sensors_lock);
> +                       kfree(multi_tz);
> +               } else {
> +                       mutex_unlock(&multi_tz->sensors_lock);
> +               }
> +       }
> +       mutex_unlock(&multi_tz_mutex);
> +}
> --
> 2.41.0
>

By the way, may I know why min/max aggregation is dropped in this
version? I thought that checking max temperature is the most direct
approach to protect the hardware and the users from high temperature.

Best regards,
Pin-yen

Re: [PATCH v2 3/3] thermal: Add support of multi sensors to thermal_of

[Pin-yen Lin]

Hi Alexandre,

On Fri, Apr 12, 2024 at 4:23 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This updates thermal_of to support more than one sensor.
> If during the registration we find another thermal zone referencing
> this sensors and some other, then we create the multi sensor thermal
> zone (if it doesn't exist) and register the sensor to it.
>
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/thermal/thermal_of.c | 139 +++++++++++++++++++++++++++++++++++
>  1 file changed, 139 insertions(+)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 1e0655b63259..3f36d8a3d8e8 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -441,12 +441,146 @@ static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>         struct thermal_trip *trips = tz->trips;
>         struct thermal_zone_device_ops *ops = tz->ops;
>
> +       thermal_multi_sensor_unregister(tz);
>         thermal_zone_device_disable(tz);
>         thermal_zone_device_unregister(tz);
>         kfree(trips);
>         kfree(ops);
>  }
>
> +int thermal_of_get_sensor_id(struct device_node *tz_np,
> +                           struct device_node *sensor_np,
> +                           int phandle_index, u32 *id)
> +{
> +       struct of_phandle_args sensor_specs;
> +       int ret;
> +
> +       ret = of_parse_phandle_with_args(tz_np,
> +                                        "thermal-sensors",
> +                                        "#thermal-sensor-cells",
> +                                        phandle_index,
> +                                        &sensor_specs);
> +       if (ret)
> +               return ret;
> +
> +       if (sensor_specs.np != sensor_np) {
> +               of_node_put(sensor_specs.np);
> +               return -ENODEV;
> +       }
> +
> +       if (sensor_specs.args_count > 1)
> +               pr_warn("%pOFn: too many cells in sensor specifier %d\n",
> +                       sensor_specs.np, sensor_specs.args_count);
> +
> +       *id = sensor_specs.args_count ? sensor_specs.args[0] : 0;
> +       of_node_put(sensor_specs.np);
> +
> +       return 0;
> +}
> +
> +static int thermal_of_has_sensor_id(struct device_node *tz_np,
> +                                   struct device_node *sensor_np,
> +                                   u32 sensor_id)
> +{
> +       int count;
> +       int i;
> +
> +       count = of_count_phandle_with_args(tz_np,
> +                                          "thermal-sensors",
> +                                          "#thermal-sensor-cells");
> +       if (count <= 0)
> +               return -ENODEV;
> +
> +       for (i = 0; i < count; i++) {
> +               int ret;
> +               u32 id;
> +
> +               ret = thermal_of_get_sensor_id(tz_np, sensor_np, i, &id);
> +               if (ret)
> +                       return ret;

We should just `continue` here or handle -ENODEV differently. The
current implementation doesn't support a thermal zone references
sensors from different nodes.

> +
> +               if (id == sensor_id)
> +                       return i;
> +
> +       }
> +
> +       return -ENODEV;
> +}
> +
> +static int thermal_of_register_mutli_sensor(struct device_node *sensor, int id,
> +                                           struct thermal_zone_device *tz,
> +                                           struct device_node *tz_np)
> +{
> +       struct device_node *child;
> +       u32 *coeff;
> +       int ret;
> +       int i;
> +
> +       /*
> +        * Go through all the thermal zone and check if the sensor is
> +        * referenced. If so, find or create a multi sensor thermal zone
> +        * and register the sensor to it.
> +        */
> +       for_each_available_child_of_node(of_get_parent(tz_np), child) {
> +               int count;
> +               int index;
> +               int offset;
> +
> +               /* Skip the tz that is currently registering */
> +               if (child == tz_np)
> +                       continue;

of_thermal_zone_find() simply returns the first thermal zone
containing the sensor, so there's no guarantee that tz here is the
currently registering thermal zone. Maybe we should make
of_thermal_zone_find() only return thermal zones with only one sensor
attached?

> +
> +               /* Test if the sensor is referenced by a tz*/
> +               index = thermal_of_has_sensor_id(child, sensor, id);
> +               if (index < 0)
> +                       continue;
> +
> +               /*
> +                * Get the coefficients and offset and assign them to the
> +                * multi sensor thermal zone.
> +                */
> +               count = of_count_phandle_with_args(child,
> +                                                  "thermal-sensors",
> +                                                  "#thermal-sensor-cells");
> +               coeff = kmalloc_array(count, sizeof(*coeff), GFP_KERNEL);
> +               if (!coeff)
> +                       goto err;
> +
> +               for (i = 0; i < count; i++) {
> +                       ret = of_property_read_u32_index(child,
> +                                                        "coefficients",
> +                                                        i, coeff + i);
> +                       if (ret)
> +                               coeff[i] = 1;
> +               }
> +
> +               ret = of_property_read_u32_index(child, "coefficients",
> +                                                count, &offset);
> +               if (ret)
> +                       offset = 0;
> +
> +               /* Make sure the coeff and offset won't cause an overflow */
> +               ret = thermal_multi_sensor_validate_coeff(coeff, count, offset);
> +               if (ret)
> +                       goto err_free_coeff;
> +
> +               ret = thermal_multi_sensor_register(child->name, tz,
> +                                                        coeff[index]);

The indentation of this line is wrong.

> +               if (ret)
> +                       goto err_free_coeff;
> +               kfree(coeff);
> +       }
> +
> +       return 0;
> +
> +err_free_coeff:
> +       kfree(coeff);
> +err:
> +       thermal_multi_sensor_unregister(tz);
> +
> +       return ret;
> +}
> +
>  /**
>   * thermal_of_zone_register - Register a thermal zone with device node
>   * sensor
> @@ -528,6 +662,11 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>                 return ERR_PTR(ret);
>         }
>
> +       /* Register the sensor to all other thermal zone referencing it */
> +       ret = thermal_of_register_mutli_sensor(sensor, id, tz, np);
> +       if (ret)
> +               pr_warn("Failed to register a sensor to a multi sensor tz\n");
> +
>         return tz;
>
>  out_kfree_trips:
> --
> 2.41.0
>

Best regards,
Pin-yen

Re: [PATCH v2 0/3] thermal: Add support of multiple sensors

[Daniel Lezcano]

Hi Alexandre,

On 19/01/2024 12:08, Alexandre Bailon wrote:
> Following this comment [1], this updates thermal_of to support multiple
> sensors.
> 
> This has some limitations:
> - A sensor must have its own termal zone, even if it is also registered
>    inside a thermal zone supporting multiple sensors.
> - Only support weighted average

Is it possible to elaborate why this feature is needed ?

The thermal framework is able to aggregate the cooling device requests, 
so having multiple sensors aggregated or the cooling device requests is 
from my point of view the same.

I can imagine one reason to do that is to group the sensors in order to 
use the IPA because it won't work the setup mentioned above.

Is that the goal you want to achieve ?

> Changes in v2:
> - Rebased on 6.7
> - Seperated generic multi sensor and dt specfic code
> - Simplified the code
> - Drop min / max and only do weighted average (seems more adequate for IPA)
> 
> [1]: https://patchwork.kernel.org/comment/24723927/
> 
> Alexandre Bailon (3):
>    dt-bindings: thermal: Restore the thermal-sensors property
>    thermal: Add support of multi sensors to thermal_core
>    thermal: Add support of multi sensors to thermal_of
> 
>   .../bindings/thermal/thermal-zones.yaml       |   5 +-
>   drivers/thermal/Makefile                      |   1 +
>   drivers/thermal/thermal_core.h                |   7 +
>   drivers/thermal/thermal_multi.c               | 178 ++++++++++++++++++
>   drivers/thermal/thermal_of.c                  | 139 ++++++++++++++
>   5 files changed, 327 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/thermal/thermal_multi.c
> 

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 0/3] thermal: Add support of multiple sensors

[Alexandre Bailon]

Hi Daniel,

On 4/30/24 16:37, Daniel Lezcano wrote:
> 
> Hi Alexandre,
> 
> On 19/01/2024 12:08, Alexandre Bailon wrote:
>> Following this comment [1], this updates thermal_of to support multiple
>> sensors.
>>
>> This has some limitations:
>> - A sensor must have its own termal zone, even if it is also registered
>>    inside a thermal zone supporting multiple sensors.
>> - Only support weighted average
> 
> Is it possible to elaborate why this feature is needed ?
> 
> The thermal framework is able to aggregate the cooling device requests, 
> so having multiple sensors aggregated or the cooling device requests is 
> from my point of view the same.
> 
> I can imagine one reason to do that is to group the sensors in order to 
> use the IPA because it won't work the setup mentioned above.
> 
> Is that the goal you want to achieve ?
Yes, the goal is to use IPA.
I will give more details in the v3.

Thanks,
Alexandre
> 
>> Changes in v2:
>> - Rebased on 6.7
>> - Seperated generic multi sensor and dt specfic code
>> - Simplified the code
>> - Drop min / max and only do weighted average (seems more adequate for 
>> IPA)
>>
>> [1]: https://patchwork.kernel.org/comment/24723927/
>>
>> Alexandre Bailon (3):
>>    dt-bindings: thermal: Restore the thermal-sensors property
>>    thermal: Add support of multi sensors to thermal_core
>>    thermal: Add support of multi sensors to thermal_of
>>
>>   .../bindings/thermal/thermal-zones.yaml       |   5 +-
>>   drivers/thermal/Makefile                      |   1 +
>>   drivers/thermal/thermal_core.h                |   7 +
>>   drivers/thermal/thermal_multi.c               | 178 ++++++++++++++++++
>>   drivers/thermal/thermal_of.c                  | 139 ++++++++++++++
>>   5 files changed, 327 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/thermal/thermal_multi.c
>>
> 

Re: [PATCH v2 2/3] thermal: Add support of multi sensors to thermal_core

[Alexandre Bailon]

Hi Pin-yen,

> Hi Alexandre,
> 
> On Thu, Apr 11, 2024 at 4:34 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>>
>> This adds support of multi sensors to thermal.
>> Currently, this only support the get_temp operation.
>> This returns an average temperature of all the sensors.
>> If defined, a coefficient is applied to the value read from the sensor
>> before computing the average.
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>>   drivers/thermal/Makefile        |   1 +
>>   drivers/thermal/thermal_core.h  |   7 ++
>>   drivers/thermal/thermal_multi.c | 178 ++++++++++++++++++++++++++++++++
>>   3 files changed, 186 insertions(+)
>>   create mode 100644 drivers/thermal/thermal_multi.c
>>
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index c934cab309ae..757289a406f7 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -6,6 +6,7 @@ CFLAGS_thermal_core.o           := -I$(src)
>>   obj-$(CONFIG_THERMAL)          += thermal_sys.o
>>   thermal_sys-y                  += thermal_core.o thermal_sysfs.o
>>   thermal_sys-y                  += thermal_trip.o thermal_helpers.o
>> +thermal_sys-y                  += thermal_multi.o
>>
>>   # netlink interface to manage the thermal framework
>>   thermal_sys-$(CONFIG_THERMAL_NETLINK)          += thermal_netlink.o
>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>> index 0a3b3ec5120b..26e83a5c8298 100644
>> --- a/drivers/thermal/thermal_core.h
>> +++ b/drivers/thermal/thermal_core.h
>> @@ -138,6 +138,13 @@ ssize_t weight_show(struct device *, struct device_attribute *, char *);
>>   ssize_t weight_store(struct device *, struct device_attribute *, const char *,
>>                       size_t);
>>
>> +/* Multi sensors */
>> +int thermal_multi_sensor_validate_coeff(int *coeff, int count, int offset);
>> +int thermal_multi_sensor_register(const char *name,
>> +       struct thermal_zone_device *sensor_tz, int coeff);
>> +void thermal_multi_sensor_unregister(struct thermal_zone_device *sensor_tz);
>> +
>> +
>>   #ifdef CONFIG_THERMAL_STATISTICS
>>   void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>>                                           unsigned long new_state);
>> diff --git a/drivers/thermal/thermal_multi.c b/drivers/thermal/thermal_multi.c
>> new file mode 100644
>> index 000000000000..a5a4f1f2d594
>> --- /dev/null
>> +++ b/drivers/thermal/thermal_multi.c
>> @@ -0,0 +1,178 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/thermal.h>
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +
>> +#include "thermal_core.h"
>> +
>> +struct sensor_interface {
>> +       struct thermal_zone_device *tz;
>> +       int coeff;
>> +
>> +       struct list_head node;
>> +};
>> +
>> +struct multi_sensor_thermal_zone {
>> +       struct thermal_zone_device *tz;
>> +       struct mutex sensors_lock;
>> +       struct list_head sensors;
>> +
>> +       struct list_head node;
>> +};
>> +
>> +static DEFINE_MUTEX(multi_tz_mutex);
>> +static LIST_HEAD(multi_tz_list);
>> +
>> +#define TJ_MAX 120000
>> +
>> +static int multi_sensor_get_temp(struct thermal_zone_device *tz, int *temp)
>> +{
>> +       struct multi_sensor_thermal_zone *multi_tz = tz->devdata;
>> +       struct sensor_interface *sensor;
>> +       int accumulated_temp = 0;
>> +       u32 accumulated_coeff;
> 
> Should we initialize accumulated_coeff to 0 as well?
Yes, indeed, I will fix it.
> 
>> +       int ret;
>> +
>> +       mutex_lock(&multi_tz->sensors_lock);
>> +
>> +       if (list_empty(&multi_tz->sensors)) {
>> +               mutex_unlock(&multi_tz->sensors_lock);
>> +               return -ENODEV;
>> +       }
>> +
>> +       list_for_each_entry(sensor, &multi_tz->sensors, node) {
>> +               ret = thermal_zone_get_temp(sensor->tz, temp);
>> +               if (ret) {
>> +                       mutex_unlock(&multi_tz->sensors_lock);
>> +                       return ret;
>> +               }
>> +
>> +               accumulated_temp += *temp * sensor->coeff;
>> +               accumulated_coeff += sensor->coeff;
>> +       }
>> +
>> +       mutex_unlock(&multi_tz->sensors_lock);
>> +
>> +       *temp = accumulated_temp / accumulated_coeff;
>> +       return ret;
>> +}
>> +
>> +struct thermal_zone_device_ops multi_sensor_ops = {
>> +       .get_temp = multi_sensor_get_temp,
>> +};
>> +
>> +int thermal_multi_sensor_validate_coeff(int *coeff, int count, int offset)
>> +{
>> +       int max_accumulated_temp = 0;
>> +       int i;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               max_accumulated_temp += TJ_MAX * coeff[i];
>> +               if (max_accumulated_temp < 0)
>> +                       return -EOVERFLOW;
>> +       }
>> +
>> +       max_accumulated_temp += offset;
>> +       return max_accumulated_temp < 0 ? -EOVERFLOW : 0;
>> +}
>> +
>> +static struct thermal_zone_device *multi_sensor_tz_alloc(const char *name)
>> +{
>> +       struct thermal_zone_device *tz;
>> +       struct thermal_zone_params tzp = {};
>> +       struct multi_sensor_thermal_zone *multi_tz;
>> +
>> +       tz = thermal_zone_get_zone_by_name(name);
>> +       if (!IS_ERR(tz)) {
>> +               mutex_unlock(&multi_tz_mutex);
>> +               return tz;
>> +       }
>> +
>> +       multi_tz = kzalloc(sizeof(*multi_tz), GFP_KERNEL);
>> +       if (!multi_tz)
>> +               return ERR_PTR(-ENOMEM);
>> +       mutex_init(&multi_tz->sensors_lock);
>> +       INIT_LIST_HEAD(&multi_tz->sensors);
>> +
>> +       tzp.no_hwmon = true;
>> +       tzp.slope = 1;
>> +       tzp.offset = 0;
>> +
>> +       tz = thermal_tripless_zone_device_register(name, multi_tz,
>> +                                                  &multi_sensor_ops, &tzp);
>> +       if (IS_ERR(tz)) {
>> +               kfree(multi_tz);
>> +       } else {
>> +               multi_tz->tz = tz;
>> +               list_add(&multi_tz->node, &multi_tz_list);
>> +       }
>> +
>> +       return tz;
>> +}
>> +
>> +int thermal_multi_sensor_register(const char *name,
>> +       struct thermal_zone_device *sensor_tz, int coeff)
>> +{
>> +       struct thermal_zone_device *tz;
>> +       struct multi_sensor_thermal_zone *multi_tz;
>> +       struct sensor_interface *sensor;
>> +
>> +       mutex_lock(&multi_tz_mutex);
>> +
>> +       tz = multi_sensor_tz_alloc(name);
>> +       if (IS_ERR(tz)) {
>> +               mutex_unlock(&multi_tz_mutex);
>> +               return PTR_ERR(tz);
>> +       }
>> +       multi_tz =  tz->devdata;
>> +
>> +       sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
>> +       if (!sensor) {
>> +               mutex_unlock(&multi_tz_mutex);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       sensor->tz = sensor_tz;
>> +       sensor->coeff = coeff;
>> +       mutex_lock(&multi_tz->sensors_lock);
>> +       list_add(&sensor->node, &multi_tz->sensors);
>> +       mutex_unlock(&multi_tz->sensors_lock);
>> +
>> +       thermal_zone_device_enable(tz);
>> +
>> +       mutex_unlock(&multi_tz_mutex);
>> +
>> +       return 0;
>> +}
>> +
>> +void thermal_multi_sensor_unregister(struct thermal_zone_device *sensor_tz)
>> +{
>> +       struct multi_sensor_thermal_zone *multi_tz;
>> +       struct sensor_interface *sensor, *tmp;
>> +
>> +       mutex_lock(&multi_tz_mutex);
>> +       list_for_each_entry(multi_tz, &multi_tz_list, node) {
>> +               mutex_lock(&multi_tz->sensors_lock);
>> +               list_for_each_entry_safe(sensor, tmp, &multi_tz->sensors, node) {
>> +                       if (sensor->tz == sensor_tz) {
>> +                               list_del(&sensor->node);
>> +                               kfree(sensor);
>> +                               break;
>> +                       }
>> +               }
>> +
>> +               if (list_empty(&multi_tz->sensors)) {
>> +                       thermal_zone_device_unregister(multi_tz->tz);
>> +                       mutex_unlock(&multi_tz->sensors_lock);
>> +                       kfree(multi_tz);
>> +               } else {
>> +                       mutex_unlock(&multi_tz->sensors_lock);
>> +               }
>> +       }
>> +       mutex_unlock(&multi_tz_mutex);
>> +}
>> --
>> 2.41.0
>>
> 
> By the way, may I know why min/max aggregation is dropped in this
> version? I thought that checking max temperature is the most direct
> approach to protect the hardware and the users from high temperature.
You are right. The main reason was device tree bindings.
I was expecting getting this accepted first before adding max and min.

Thanks,
Alexandre
> 
> Best regards,
> Pin-yen