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

[PATCH 0/2] 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.
- Some callbacks (such as of_thermal_set_trips) have been updated to support
  multiple sensors but I don't know if this really make sense.
- of_thermal_get_trend have not been updated to support multiple sensors.
  This would probably make sense to support it but I am not sure how to do it,
  especially for the average. 

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

Alexandre Bailon (2):
  dt-bindings: thermal: Update the bindings to support multiple sensor
  Thermal: Add support of multi sensor

 .../bindings/thermal/thermal-zones.yaml       |  20 +-
 drivers/thermal/thermal_of.c                  | 491 +++++++++++++++---
 2 files changed, 449 insertions(+), 62 deletions(-)

-- 
2.34.1

[PATCH 1/2] dt-bindings: thermal: Update the bindings to support multiple sensors

[Alexandre Bailon]

This adds two optionals properties and update the thermal-sensors
property description to support multiple sensors with a thermal zone.

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

diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
index 2d34f3ccb2572..9f944c2364589 100644
--- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
+++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
@@ -77,10 +77,24 @@ 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.
+          An array of thermal sensor phandle and sensor specifier used to
+          monitor this thermal zone.
+          If the array contains more than one sensor then the returned value
+          is the maximum unless aggregation-min or aggregation-avg properties
+          are set.
+
+      aggregation-min:
+        type: boolean
+        description:
+          Return the minimum temperature when the thermal monitor multiple
+          sensors.
+
+      aggregation-avg:
+        type: boolean
+        description:
+          Return the average temperature when the thermal monitor multiple
+          sensors.
 
       coefficients:
         $ref: /schemas/types.yaml#/definitions/uint32-array
@@ -338,5 +352,15 @@ examples:
                             };
                     };
             };
+
+            /* ... */
+
+            soc-max-thermal {
+                    polling-delay-passive = <250>;
+                    polling-delay = <1000>;
+                    thermal-sensors = <&tsens0 5>, <&tsens0 11>;
+                    trips {};
+                    cooling-maps {};
+            };
     };
 ...
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: thermal: Update the bindings to support multiple sensors

[Daniel Lezcano]

Hi Alexandre,

thanks for your series

On 18/02/2022 09:46, Alexandre Bailon wrote:
> This adds two optionals properties and update the thermal-sensors
> property description to support multiple sensors with a thermal zone.


I think it would make sense to first send a patch fixing the missing 
attribute from txt -> yaml translation with the Fixes tag which was:

"A list of thermal sensor phandles and sensor specifier
   Type: list of		used while monitoring the thermal zone"

And add in the description if there are more than one sensor, then the 
max value will be returned. Then provide the code to get the max value.

The second series of changes would be the other aggregations and the 
implementation.


> ---
>   .../bindings/thermal/thermal-zones.yaml       | 30 +++++++++++++++++--
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> index 2d34f3ccb2572..9f944c2364589 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> @@ -77,10 +77,24 @@ 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.
> +          An array of thermal sensor phandle and sensor specifier used to
> +          monitor this thermal zone.
> +          If the array contains more than one sensor then the returned value
> +          is the maximum unless aggregation-min or aggregation-avg properties
> +          are set.
> +
> +      aggregation-min:
> +        type: boolean
> +        description:
> +          Return the minimum temperature when the thermal monitor multiple
> +          sensors.
> +
> +      aggregation-avg:
> +        type: boolean
> +        description:
> +          Return the average temperature when the thermal monitor multiple
> +          sensors.

Maybe 'aggregation' could be a string?

	aggregation = "max";
	aggregation = "min";
	aggregation = "avg";

It should be in the optional section and probably a conditional if 
thermal-sensors has more than one item

>         coefficients:
>           $ref: /schemas/types.yaml#/definitions/uint32-array
> @@ -338,5 +352,15 @@ examples:
>                               };
>                       };
>               };
> +
> +            /* ... */
> +
> +            soc-max-thermal {
> +                    polling-delay-passive = <250>;
> +                    polling-delay = <1000>;
> +                    thermal-sensors = <&tsens0 5>, <&tsens0 11>;
> +                    trips {};
> +                    cooling-maps {};
> +            };
>       };
>   ...


-- 
<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

[PATCH 2/2] Thermal: Add support of multiple sensors

[Alexandre Bailon]

This updates thermal_of to add support of multiple sensors.
By default, when several sensors are registered to a thermal zone,
the maximum temperature is returned.
This could be changed to minimum or average temparture by
adding aggretation-min or aggregation-avg to the thermal zone
devioce tree node.
In addition of get_temp, this also updates some
thermal_zone_of_device_ops callbacks to support multiple sensors.
Only get_trend callback is not implemented to support multiple sensors.

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

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 9233f7e744544..fce4332b59734 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -19,6 +19,12 @@
 
 #include "thermal_core.h"
 
+enum aggr_type {
+	VIRTUAL_THERMAL_SENSOR_MIN_VAL,
+	VIRTUAL_THERMAL_SENSOR_MAX_VAL,
+	VIRTUAL_THERMAL_SENSOR_AVG_VAL,
+};
+
 /***   Private data structures to represent thermal device tree data ***/
 
 /**
@@ -49,6 +55,20 @@ struct __thermal_bind_params {
 	unsigned int usage;
 };
 
+/**
+ * sensor interface
+ * @sensor_data: sensor private data used while reading temperature and trend
+ * @ops: set of callbacks to handle the thermal zone based on DT
+ */
+
+struct __sensor_interface {
+	void *data;
+	const struct thermal_zone_of_device_ops *ops;
+	int tzd_id;
+
+	struct list_head node;
+};
+
 /**
  * struct __thermal_zone - internal representation of a thermal zone
  * @passive_delay: polling interval while passive cooling is activated
@@ -61,6 +81,9 @@ struct __thermal_bind_params {
  * @tbps: an array of thermal bind params (0..num_tbps - 1)
  * @sensor_data: sensor private data used while reading temperature and trend
  * @ops: set of callbacks to handle the thermal zone based on DT
+ * @sensors_lock: a mutex to protect the list of sensors
+ * @sensors: a list of sensors used in multiple sensors configuration
+ * @aggr_type: define how to aggregate the temperature from multiple sensors
  */
 
 struct __thermal_zone {
@@ -78,32 +101,119 @@ struct __thermal_zone {
 	struct __thermal_bind_params *tbps;
 
 	/* sensor interface */
-	void *sensor_data;
-	const struct thermal_zone_of_device_ops *ops;
+	struct __sensor_interface *sensor;
+
+	/* multiple sensors */
+	struct mutex sensors_lock;
+	struct list_head sensors;
+	int aggr_type;
+	int count;
 };
 
 /***   DT thermal zone device callbacks   ***/
 
+static int of_thermal_get_aggr_temp(struct thermal_zone_device *tz,
+				    int *temp)
+{
+	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor;
+	int aggr_temp = 0;
+	int remainder = 0;
+	int sum = 0;
+	int ret;
+
+	if (list_empty(&data->sensors))
+		return -EINVAL;
+
+	if (data->aggr_type == VIRTUAL_THERMAL_SENSOR_MIN_VAL)
+		aggr_temp = INT_MAX;
+
+	list_for_each_entry(sensor, &data->sensors, node) {
+		ret = sensor->ops->get_temp(sensor->data, temp);
+		if (ret)
+			return ret;
+
+		switch (data->aggr_type) {
+		case VIRTUAL_THERMAL_SENSOR_MAX_VAL:
+			aggr_temp = max(aggr_temp, *temp);
+			break;
+		case VIRTUAL_THERMAL_SENSOR_MIN_VAL:
+			aggr_temp = min(aggr_temp, *temp);
+			break;
+		case VIRTUAL_THERMAL_SENSOR_AVG_VAL:
+			sum += *temp / data->count;
+			remainder += *temp % data->count;
+			break;
+		}
+	}
+
+	if (data->aggr_type == VIRTUAL_THERMAL_SENSOR_AVG_VAL)
+		aggr_temp = sum + (remainder) / data->count;
+
+	*temp = aggr_temp;
+	return ret;
+}
+
 static int of_thermal_get_temp(struct thermal_zone_device *tz,
 			       int *temp)
 {
 	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor = data->sensor;
+	int ret;
+
+	if (!sensor) {
+		mutex_lock(&data->sensors_lock);
+		ret = of_thermal_get_aggr_temp(tz, temp);
+		mutex_unlock(&data->sensors_lock);
+
+		return ret;
+	}
+
+	if (!sensor->ops || !sensor->ops->get_temp)
+		return -EINVAL;
+
+	return sensor->ops->get_temp(sensor->data, temp);
+}
+
+static int of_thermal_set_multi_trips(struct thermal_zone_device *tz,
+				      int low, int high)
+{
+	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor;
+	int ret;
 
-	if (!data->ops || !data->ops->get_temp)
+	if (list_empty(&data->sensors))
 		return -EINVAL;
 
-	return data->ops->get_temp(data->sensor_data, temp);
+	list_for_each_entry(sensor, &data->sensors, node) {
+		ret = sensor->ops->set_trips(sensor->data, low, high);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int of_thermal_set_trips(struct thermal_zone_device *tz,
 				int low, int high)
 {
 	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor = data->sensor;
+
+	if (!sensor) {
+		int ret;
+
+		mutex_lock(&data->sensors_lock);
+		ret = of_thermal_set_multi_trips(tz, low, high);
+		mutex_unlock(&data->sensors_lock);
+
+		return ret;
+	}
 
-	if (!data->ops || !data->ops->set_trips)
+	if (!sensor->ops || !sensor->ops->set_trips)
 		return -EINVAL;
 
-	return data->ops->set_trips(data->sensor_data, low, high);
+	return sensor->ops->set_trips(sensor->data, low, high);
 }
 
 /**
@@ -181,26 +291,61 @@ EXPORT_SYMBOL_GPL(of_thermal_get_trip_points);
  *
  * Return: zero on success, error code otherwise
  */
+static int of_thermal_set_multi_emul_temp(struct thermal_zone_device *tz,
+					  int temp)
+{
+	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor;
+	int ret;
+
+	if (list_empty(&data->sensors))
+		return -EINVAL;
+
+	list_for_each_entry(sensor, &data->sensors, node) {
+		ret = sensor->ops->set_emul_temp(sensor->data, temp);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int of_thermal_set_emul_temp(struct thermal_zone_device *tz,
 				    int temp)
 {
 	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor = data->sensor;
+
+	if (!sensor) {
+		int ret;
+
+		mutex_lock(&data->sensors_lock);
+		ret = of_thermal_set_multi_emul_temp(tz, temp);
+		mutex_unlock(&data->sensors_lock);
+
+		return ret;
+	}
 
-	if (!data->ops || !data->ops->set_emul_temp)
+	if (!sensor->ops || !sensor->ops->set_emul_temp)
 		return -EINVAL;
 
-	return data->ops->set_emul_temp(data->sensor_data, temp);
+	return sensor->ops->set_emul_temp(sensor->data, temp);
 }
 
 static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 				enum thermal_trend *trend)
 {
 	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor = data->sensor;
 
-	if (!data->ops || !data->ops->get_trend)
+	/* Currently not supported by multiple sensors configuration */
+	if (!sensor)
 		return -EINVAL;
 
-	return data->ops->get_trend(data->sensor_data, trip, trend);
+	if (!sensor->ops || !sensor->ops->get_trend)
+		return -EINVAL;
+
+	return sensor->ops->get_trend(sensor->data, trip, trend);
 }
 
 static int of_thermal_bind(struct thermal_zone_device *thermal,
@@ -296,22 +441,49 @@ static int of_thermal_get_trip_temp(struct thermal_zone_device *tz, int trip,
 	return 0;
 }
 
+static int of_thermal_set_multi_trip_temp(struct thermal_zone_device *tz,
+					  int trip, int temp)
+{
+	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor;
+	int ret;
+
+	if (list_empty(&data->sensors))
+		return -EINVAL;
+
+	list_for_each_entry(sensor, &data->sensors, node) {
+		if (sensor->ops->set_trip_temp) {
+			ret = sensor->ops->set_trip_temp(sensor->data,
+							 trip, temp);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
 				    int temp)
 {
 	struct __thermal_zone *data = tz->devdata;
+	struct __sensor_interface *sensor = data->sensor;
+	int ret;
 
 	if (trip >= data->ntrips || trip < 0)
 		return -EDOM;
 
-	if (data->ops && data->ops->set_trip_temp) {
-		int ret;
-
-		ret = data->ops->set_trip_temp(data->sensor_data, trip, temp);
-		if (ret)
-			return ret;
+	if (!sensor) {
+		mutex_lock(&data->sensors_lock);
+		ret = of_thermal_set_multi_trip_temp(tz, trip, temp);
+		mutex_unlock(&data->sensors_lock);
+	} else if (sensor->ops && sensor->ops->set_trip_temp) {
+		ret = sensor->ops->set_trip_temp(sensor->data, trip, temp);
 	}
 
+	if (ret)
+		return ret;
+
 	/* thermal framework should take care of data->mask & (1 << trip) */
 	data->trips[trip].temperature = temp;
 
@@ -374,13 +546,66 @@ static struct thermal_zone_device_ops of_thermal_ops = {
 
 /***   sensor API   ***/
 
+static struct __sensor_interface *alloc_sensor_interface(
+		struct __thermal_zone *tz, int tzd_id, void *data,
+		const struct thermal_zone_of_device_ops *ops)
+{
+	struct __sensor_interface *sensor_iface;
+
+	sensor_iface = kzalloc(sizeof(*sensor_iface), GFP_KERNEL);
+	if (!sensor_iface)
+		return NULL;
+
+	sensor_iface->ops = ops;
+	sensor_iface->data = data;
+	sensor_iface->tzd_id = tzd_id;
+	INIT_LIST_HEAD(&sensor_iface->node);
+
+	return sensor_iface;
+}
+
+static int check_sensors_coherency(struct thermal_zone_device *tzd)
+{
+	struct __thermal_zone *tz = tzd->devdata;
+	struct __sensor_interface *sensor;
+	int has_set_trip_temp = 0;
+	int i = 0;
+
+	list_for_each_entry(sensor, &tz->sensors, node) {
+		if (!sensor->ops->get_temp) {
+			pr_err(".get_temp function is missing\n");
+			return -EINVAL;
+		}
+
+		if (tzd->ops->set_trips && sensor->ops->set_trips) {
+			pr_err("Some sensors miss .set_trips\n");
+			return -EINVAL;
+		}
+
+		if (tzd->ops->set_emul_temp && sensor->ops->set_emul_temp) {
+			pr_err("Some sensors miss .set_emul_temp\n");
+			return -EINVAL;
+		}
+
+		if (!sensor->ops->set_trip_temp)
+			has_set_trip_temp++;
+		i++;
+	}
+
+	if (has_set_trip_temp != i)
+		pr_warn_once("Some sensors miss .set_trip_temp function\n");
+
+	return 0;
+}
+
 static struct thermal_zone_device *
 thermal_zone_of_add_sensor(struct device_node *zone,
-			   struct device_node *sensor, void *data,
+			   struct device_node *sensor, int tzd_id, void *data,
 			   const struct thermal_zone_of_device_ops *ops)
 {
 	struct thermal_zone_device *tzd;
 	struct __thermal_zone *tz;
+	struct __sensor_interface *sensor_iface;
 
 	tzd = thermal_zone_get_zone_by_name(zone->name);
 	if (IS_ERR(tzd))
@@ -391,43 +616,56 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 	if (!ops)
 		return ERR_PTR(-EINVAL);
 
-	mutex_lock(&tzd->lock);
-	tz->ops = ops;
-	tz->sensor_data = data;
+	sensor_iface = alloc_sensor_interface(tz, tzd_id, data, ops);
+	if (!sensor_iface)
+		return ERR_PTR(-ENOMEM);
 
-	tzd->ops->get_temp = of_thermal_get_temp;
-	tzd->ops->get_trend = of_thermal_get_trend;
+	mutex_lock(&tz->sensors_lock);
+	mutex_lock(&tzd->lock);
 
-	/*
-	 * The thermal zone core will calculate the window if they have set the
-	 * optional set_trips pointer.
-	 */
-	if (ops->set_trips)
-		tzd->ops->set_trips = of_thermal_set_trips;
+	if (list_empty(&tz->sensors) && !tz->sensor) {
+		/* Adding the first sensor */
+		tzd->ops->get_temp = of_thermal_get_temp;
+		tzd->ops->get_trend = of_thermal_get_trend;
+
+		/*
+		 * The thermal zone core will calculate the window
+		 * if they have set the optional set_trips pointer.
+		 */
+		if (ops->set_trips)
+			tzd->ops->set_trips = of_thermal_set_trips;
+
+		if (ops->set_emul_temp)
+			tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
+
+		/* Assume a mono sensor configuration */
+		tz->sensor = sensor_iface;
+	} else if (list_empty(&tz->sensors) && tz->sensor) {
+		/*
+		 * Multiple sensors configuration
+		 * Move the first sensor to the list
+		 */
+		list_add(&tz->sensor->node, &tz->sensors);
+		tz->sensor = NULL;
+		tz->count = 1;
+	}
 
-	if (ops->set_emul_temp)
-		tzd->ops->set_emul_temp = of_thermal_set_emul_temp;
+	if (!tz->sensor) {
+		list_add(&sensor_iface->node, &tz->sensors);
+		tz->count++;
+	}
 
 	mutex_unlock(&tzd->lock);
+	mutex_unlock(&tz->sensors_lock);
 
 	return tzd;
 }
 
-/**
- * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
- * @tz_np: a valid thermal zone device node.
- * @sensor_np: a sensor node of a valid sensor device.
- * @id: the sensor ID returned if success.
- *
- * This function will get sensor ID from a given thermal zone node and
- * the sensor node must match the temperature provider @sensor_np.
- *
- * Return: 0 on success, proper error code otherwise.
- */
-
-int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
-				  struct device_node *sensor_np,
-				  u32 *id)
+static
+int _thermal_zone_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;
@@ -435,7 +673,7 @@ int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
 	ret = of_parse_phandle_with_args(tz_np,
 					 "thermal-sensors",
 					 "#thermal-sensor-cells",
-					 0,
+					 phandle_index,
 					 &sensor_specs);
 	if (ret)
 		return ret;
@@ -455,8 +693,87 @@ int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
 
 	return 0;
 }
+
+
+/**
+ * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
+ * @tz_np: a valid thermal zone device node.
+ * @sensor_np: a sensor node of a valid sensor device.
+ * @id: the sensor ID returned if success.
+ *
+ * This function will get sensor ID from a given thermal zone node and
+ * the sensor node must match the temperature provider @sensor_np.
+ *
+ * Return: 0 on success, proper error code otherwise.
+ */
+
+int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
+				  struct device_node *sensor_np,
+				  u32 *id)
+{
+	return _thermal_zone_of_get_sensor_id(tz_np, sensor_np, 0, id);
+}
 EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
 
+int thermal_zone_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 0;
+
+	for (i = 0; i < count; i++) {
+		int ret;
+		u32 id;
+
+		ret = _thermal_zone_of_get_sensor_id(tz_np, sensor_np, i, &id);
+		if (ret)
+			return 0;
+
+		if (id == sensor_id)
+			return 1;
+	}
+
+	return 0;
+}
+
+static
+int thermal_zone_of_del_sensor(struct thermal_zone_device *tzd, void *data)
+{
+	int *id = data;
+	struct __thermal_zone *tz;
+	struct __sensor_interface *sensor, *tmp;
+
+	if (!tzd->devdata)
+		return -ENODEV;
+
+	tz = tzd->devdata;
+
+	mutex_lock(&tzd->lock);
+	kfree(tz->sensor);
+	tz->sensor = NULL;
+	mutex_unlock(&tzd->lock);
+
+	mutex_lock(&tz->sensors_lock);
+	list_for_each_entry_safe(sensor, tmp, &tz->sensors, node) {
+		if (sensor->tzd_id != *id)
+			continue;
+
+		list_del(&sensor->node);
+		kfree(sensor);
+		tz->count--;
+	}
+	mutex_unlock(&tz->sensors_lock);
+
+	return 0;
+}
+
 /**
  * thermal_zone_of_sensor_register - registers a sensor to a DT thermal zone
  * @dev: a valid struct device pointer of a sensor device. Must contain
@@ -493,7 +810,7 @@ struct thermal_zone_device *
 thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
 				const struct thermal_zone_of_device_ops *ops)
 {
-	struct device_node *np, *child, *sensor_np;
+	struct device_node *np, *child, *tzd_child, *sensor_np;
 	struct thermal_zone_device *tzd = ERR_PTR(-ENODEV);
 
 	np = of_find_node_by_name(NULL, "thermal-zones");
@@ -506,7 +823,6 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
 	}
 
 	sensor_np = of_node_get(dev->of_node);
-
 	for_each_available_child_of_node(np, child) {
 		int ret, id;
 
@@ -517,15 +833,50 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
 
 		if (id == sensor_id) {
 			tzd = thermal_zone_of_add_sensor(child, sensor_np,
-							 data, ops);
-			if (!IS_ERR(tzd))
-				thermal_zone_device_enable(tzd);
+							 -1, data, ops);
+			tzd_child = child;
+			if (IS_ERR(tzd))
+				goto err;
+
+			thermal_zone_device_enable(tzd);
 
-			of_node_put(child);
-			goto exit;
+			break;
+		}
+	}
+
+	if (IS_ERR(tzd))
+		return tzd;
+
+	/* Register the sensor to all other thermal zone referencing it */
+	for_each_available_child_of_node(np, child) {
+		struct thermal_zone_device *tmp_tzd;
+
+		if (!thermal_zone_of_has_sensor_id(child, sensor_np, sensor_id))
+			continue;
+
+		if (tzd_child == child)
+			continue;
+
+		tmp_tzd = thermal_zone_of_add_sensor(child, sensor_np,
+						     tzd->id, data, ops);
+		if (IS_ERR(tmp_tzd)) {
+			thermal_zone_of_del_sensor(tzd, &tzd->id);
+			goto err;
+		}
+
+		if (check_sensors_coherency(tmp_tzd)) {
+			thermal_zone_of_del_sensor(tzd, &tzd->id);
+			thermal_zone_of_del_sensor(tmp_tzd, &tmp_tzd->id);
+			tzd = ERR_PTR(-EINVAL);
+			goto err;
 		}
+
+		thermal_zone_device_enable(tmp_tzd);
+
 	}
-exit:
+
+err:
+	of_node_put(tzd_child);
 	of_node_put(sensor_np);
 	of_node_put(np);
 
@@ -544,9 +895,6 @@ EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_register);
  * API. It will also silent the zone by remove the .get_temp() and .get_trend()
  * thermal zone device callbacks.
  *
- * TODO: When the support to several sensors per zone is added, this
- * function must search the sensor list based on @dev parameter.
- *
  */
 void thermal_zone_of_sensor_unregister(struct device *dev,
 				       struct thermal_zone_device *tzd)
@@ -562,6 +910,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 	if (!tz)
 		return;
 
+
 	/* stop temperature polling */
 	thermal_zone_device_disable(tzd);
 
@@ -570,9 +919,16 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
 	tzd->ops->get_trend = NULL;
 	tzd->ops->set_emul_temp = NULL;
 
-	tz->ops = NULL;
-	tz->sensor_data = NULL;
+	if (tz->sensor) {
+		tz->sensor->ops = NULL;
+		tz->sensor->data = NULL;
+		tz->sensor = NULL;
+	}
+
 	mutex_unlock(&tzd->lock);
+
+	for_each_thermal_zone(thermal_zone_of_del_sensor, &tzd->id);
+
 }
 EXPORT_SYMBOL_GPL(thermal_zone_of_sensor_unregister);
 
@@ -868,6 +1224,14 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
 	if (!tz)
 		return ERR_PTR(-ENOMEM);
+	INIT_LIST_HEAD(&tz->sensors);
+	mutex_init(&tz->sensors_lock);
+
+	tz->aggr_type = VIRTUAL_THERMAL_SENSOR_MAX_VAL;
+	if (of_property_read_bool(np, "aggregation-min"))
+		tz->aggr_type = VIRTUAL_THERMAL_SENSOR_MIN_VAL;
+	if (of_property_read_bool(np, "aggregation-avg"))
+		tz->aggr_type = VIRTUAL_THERMAL_SENSOR_AVG_VAL;
 
 	ret = of_property_read_u32(np, "polling-delay-passive", &prop);
 	if (ret < 0) {
@@ -980,6 +1344,7 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 static __init void of_thermal_free_zone(struct __thermal_zone *tz)
 {
 	struct __thermal_bind_params *tbp;
+	struct __sensor_interface *sensor, *tmp;
 	int i, j;
 
 	for (i = 0; i < tz->num_tbps; i++) {
@@ -995,6 +1360,14 @@ static __init void of_thermal_free_zone(struct __thermal_zone *tz)
 	for (i = 0; i < tz->ntrips; i++)
 		of_node_put(tz->trips[i].np);
 	kfree(tz->trips);
+
+	mutex_lock(&tz->sensors_lock);
+	list_for_each_entry_safe(sensor, tmp, &tz->sensors, node) {
+		list_del(&sensor->node);
+		kfree(sensor);
+	}
+	mutex_unlock(&tz->sensors_lock);
+
 	kfree(tz);
 }
 
-- 
2.34.1

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

[Eduardo Valentin]

Hello Alexandre,

On Fri, Feb 18, 2022 at 09:46:02AM +0100, Alexandre Bailon wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> 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.
> - Some callbacks (such as of_thermal_set_trips) have been updated to support
>   multiple sensors but I don't know if this really make sense.
> - of_thermal_get_trend have not been updated to support multiple sensors.
>   This would probably make sense to support it but I am not sure how to do it,
>   especially for the average.

Great to see this having somewhat a form now!

Overall the idea is sane and aligned to what I had in mind back during the 2019 Linux plumbers: one thermal zone should have multiple sensor inputs.
https://lpc.events/event/4/page/34-accepted-microconferences#PMSummary

In fact, that is aligned to what I originally wrote in the thermal device tree bindings:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#n79

The only major concern with your series is the usage of of-thermal to achieve the multiple sensors per thermal zone.
While that solves the problem, it has the following limitations:
(1) limited to devices described in device tree. everybody else is left out.
(2) it keeps extending the code duplication in of-thermal. 

My suggestion here is have the thermal core aware of the multiple sensors per thermal zone.

That has the advantage of:
(a) cleanup the sensor handling within of-thermal
(b) expand the multi sensor per zone to all types of thermal drivers
(c) standardize the way to handle the multi sensor.

In my original thoughts of achieving this would have include:
(i) move the sensor handling part to a specific c file within the thermal core. That would include helper functions to execute the aggregations, max, min, avg, geo avg, etc etc
(ii) a way to tell what sensors are being aggregated via sysfs, probably a simple syslink to the original device would suffice
(iii) a way to change the aggregation via sysfs. just like you proposed a way to specify the aggregation via device tree, we should have a way to specify the aggregation at runtime.
(iv) once (i)-(iii) is done, you basically cleanup of-thermal to use the new C api written, and of-thermal simply use the API created to register the sensors. I d expect that all the callbacks related sensor ops would disappear from of-thermal.

> 
> [1]: https://patchwork.kernel.org/comment/24723927/
> 
> Alexandre Bailon (2):
>   dt-bindings: thermal: Update the bindings to support multiple sensor
>   Thermal: Add support of multi sensor
> 
>  .../bindings/thermal/thermal-zones.yaml       |  20 +-
>  drivers/thermal/thermal_of.c                  | 491 +++++++++++++++---
>  2 files changed, 449 insertions(+), 62 deletions(-)
> 
> --
> 2.34.1
> 

-- 
All the best,
Eduardo Valentin

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

[Kevin Hilman]

Hi Eduardo, Daniel,

Eduardo Valentin <eduval@amazon.com> writes:

> On Fri, Feb 18, 2022 at 09:46:02AM +0100, Alexandre Bailon wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>> 
>> 
>> 
>> 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.
>> - Some callbacks (such as of_thermal_set_trips) have been updated to support
>>   multiple sensors but I don't know if this really make sense.
>> - of_thermal_get_trend have not been updated to support multiple sensors.
>>   This would probably make sense to support it but I am not sure how to do it,
>>   especially for the average.
>
> Great to see this having somewhat a form now!
>
> Overall the idea is sane and aligned to what I had in mind back during the 2019 Linux plumbers: one thermal zone should have multiple sensor inputs.
> https://lpc.events/event/4/page/34-accepted-microconferences#PMSummary
>
> In fact, that is aligned to what I originally wrote in the thermal device tree bindings:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#n79
>
> The only major concern with your series is the usage of of-thermal to achieve the multiple sensors per thermal zone.
> While that solves the problem, it has the following limitations:
> (1) limited to devices described in device tree. everybody else is left out.
> (2) it keeps extending the code duplication in of-thermal. 
>
> My suggestion here is have the thermal core aware of the multiple sensors per thermal zone.
>
> That has the advantage of:
> (a) cleanup the sensor handling within of-thermal
> (b) expand the multi sensor per zone to all types of thermal drivers
> (c) standardize the way to handle the multi sensor.

This cleanup all sounds like the right direction to be headed, but since
this has been planned since 2019 and nothing has happended, what is the
level of urgency is for this of-thermal -> thermal core cleanup/rework?

In $SUBJECT series, we have a fully functional series that solves an
existing problem and takes a big step in the right long-term direction.
While it indeed has the has limitations you mention, I don't think that
should block the merging of this series.  More importantly, there are
existing drivers[1] as well as forthcoming ones from MTK that depend on
this series. Those are blocked if you require the of-thermal -> core
move first.

As a maintainer also, I fully understand that maintainer bandwith is
limited, and it's always nice to have contributors do core framework
development when possible, but IMO, in this case I don't think it should
be a prerequisite since a follow-up series to do the core work would not
affect any functionality or bindings etc.  I don't see any reasons not
do to this incrementally.

So I would kindly request (read: beg, plead & grovel) that you seriously
consider merging this series as a first phase and the of-thermal -> core
change be done as a second phase.  Yes, I fully understand that punting
this to a second phase means it might not get done soon.  But it's been
waiting for years already, so it seems the urgency is low.  Meanwhile,
there are OF users that are ready to use this feature today.

Thanks for considering,

Kevin

[1] https://lore.kernel.org/linux-mediatek/20210617114707.10618-1-ben.tseng@mediatek.com/

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

[AngeloGioacchino Del Regno]

Il 23/03/22 22:33, Kevin Hilman ha scritto:
> Hi Eduardo, Daniel,
> 
> Eduardo Valentin <eduval@amazon.com> writes:
> 
>> On Fri, Feb 18, 2022 at 09:46:02AM +0100, Alexandre Bailon wrote:
>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>
>>>
>>>
>>> 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.
>>> - Some callbacks (such as of_thermal_set_trips) have been updated to support
>>>    multiple sensors but I don't know if this really make sense.
>>> - of_thermal_get_trend have not been updated to support multiple sensors.
>>>    This would probably make sense to support it but I am not sure how to do it,
>>>    especially for the average.
>>
>> Great to see this having somewhat a form now!
>>
>> Overall the idea is sane and aligned to what I had in mind back during the 2019 Linux plumbers: one thermal zone should have multiple sensor inputs.
>> https://lpc.events/event/4/page/34-accepted-microconferences#PMSummary
>>
>> In fact, that is aligned to what I originally wrote in the thermal device tree bindings:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/thermal/thermal-zones.yaml#n79
>>
>> The only major concern with your series is the usage of of-thermal to achieve the multiple sensors per thermal zone.
>> While that solves the problem, it has the following limitations:
>> (1) limited to devices described in device tree. everybody else is left out.
>> (2) it keeps extending the code duplication in of-thermal.
>>
>> My suggestion here is have the thermal core aware of the multiple sensors per thermal zone.
>>
>> That has the advantage of:
>> (a) cleanup the sensor handling within of-thermal
>> (b) expand the multi sensor per zone to all types of thermal drivers
>> (c) standardize the way to handle the multi sensor.
> 
> This cleanup all sounds like the right direction to be headed, but since
> this has been planned since 2019 and nothing has happended, what is the
> level of urgency is for this of-thermal -> thermal core cleanup/rework?
> 
> In $SUBJECT series, we have a fully functional series that solves an
> existing problem and takes a big step in the right long-term direction.
> While it indeed has the has limitations you mention, I don't think that
> should block the merging of this series.  More importantly, there are
> existing drivers[1] as well as forthcoming ones from MTK that depend on
> this series. Those are blocked if you require the of-thermal -> core
> move first.
> 
> As a maintainer also, I fully understand that maintainer bandwith is
> limited, and it's always nice to have contributors do core framework
> development when possible, but IMO, in this case I don't think it should
> be a prerequisite since a follow-up series to do the core work would not
> affect any functionality or bindings etc.  I don't see any reasons not
> do to this incrementally.
> 
> So I would kindly request (read: beg, plead & grovel) that you seriously
> consider merging this series as a first phase and the of-thermal -> core
> change be done as a second phase.  Yes, I fully understand that punting
> this to a second phase means it might not get done soon.  But it's been
> waiting for years already, so it seems the urgency is low.  Meanwhile,
> there are OF users that are ready to use this feature today.
> 
> Thanks for considering,
> 
> Kevin
> 
> [1] https://lore.kernel.org/linux-mediatek/20210617114707.10618-1-ben.tseng@mediatek.com/
> 
> 

Hello Eduardo, Kevin,

I would like to add that this series is not only benefitting MediaTek platforms,
and not only Chromebooks.
On some Qualcomm SoCs (from SDM845 onwards, if I'm not wrong!), downstream, there
is some "qti virtual sensor" driver, which is addressing this kind of situation:
on these platforms, averaging, min and max (and some interpolation too, but that's
another story, I guess) is happening and that's used as some advanced way to
ensure that both performance stays high and that the device is safe to operate.
On these platforms, this is done by evaluating CPU, GPU, Hexagon DSPs, modem, wifi
and (modem,wifi)PA IPs and deciding on a thermal throttling strategy.

You understand that, while this is not "excessively" important for a Chromebook,
which is a laptop, it may become even a safety concern in devices of other form
factor, like smartphones, where there is a very strict thermal headroom (hence
requiring a fine grained thermal management).

Even though, on MediaTek, I guess that the primary usecase is Chromebooks and this
kind of mechanism is required primarily for the LVTS sensors that are used for SVS
calculations (read: better power efficiency), the Linux community is huge - and,
with this kept in mind, there will probably be someone that will like to upstream
their MTK smartphone for a reason or another (I think! This happened with Qualcomm
so I guess that it's going to happen with "any other thing")... and that adds up
to this problem being a safety concern to fix.

Of course, I agree with you, Eduardo, about the needed cleanup but, for all of
the aforementioned reasons - mine and Kevin's, like him, I would also beg, plead
and grovel that you consider merging this series as a first phase, and accept the
cleanup and use-case expansion as a second phase.

P.S.: I'm adding Marijn and Konrad to the loop, as people interested to the
       Qualcomm side of things, and mainly upstreaming smartphones.

Kind regards,
Angelo

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

[Daniel Lezcano]

Hi Angelo,


On 05/04/2022 14:14, AngeloGioacchino Del Regno wrote:

[ ... ]


> Hello Eduardo, Kevin,
> 
> I would like to add that this series is not only benefitting MediaTek
>  platforms, and not only Chromebooks. On some Qualcomm SoCs (from
> SDM845 onwards, if I'm not wrong!), downstream, there is some "qti
> virtual sensor" driver, which is addressing this kind of situation: 
> on these platforms, averaging, min and max (and some interpolation
> too, but that's another story, I guess) is happening and that's used
> as some advanced way to ensure that both performance stays high and
> that the device is safe to operate. On these platforms, this is done
> by evaluating CPU, GPU, Hexagon DSPs, modem, wifi and (modem,wifi)PA
> IPs and deciding on a thermal throttling strategy.
> 
> You understand that, while this is not "excessively" important for a
>  Chromebook, which is a laptop, it may become even a safety concern
> in devices of other form factor, like smartphones, where there is a
> very strict thermal headroom (hence requiring a fine grained thermal
> management).
> 
> Even though, on MediaTek, I guess that the primary usecase is 
> Chromebooks and this kind of mechanism is required primarily for the
> LVTS sensors that are used for SVS calculations (read: better power
> efficiency), the Linux community is huge - and, with this kept in
> mind, there will probably be someone that will like to upstream their
> MTK smartphone for a reason or another (I think! This happened with
> Qualcomm so I guess that it's going to happen with "any other
> thing")... and that adds up to this problem being a safety concern to
> fix.
> 
> Of course, I agree with you, Eduardo, about the needed cleanup but,
> for all of the aforementioned reasons - mine and Kevin's, like him, I
> would also beg, plead and grovel that you consider merging this
> series as a first phase, and accept the cleanup and use-case
> expansion as a second phase.

I'll take care of the cleanups and then respin Alex's series on top of 
those.

Thanks

  -- Daniel


> P.S.: I'm adding Marijn and Konrad to the loop, as people interested
> to the Qualcomm side of things, and mainly upstreaming smartphones.
> 
> Kind regards, Angelo


-- 
<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