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

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

[Alexandre Bailon]

This series add a virtual thermal sensor that uses the hardware thermal sensors,
aggregate them to return a temperature.

My first aptempt was to do the aggregation in the thermal zone but it was not
that easy to do, and, there were some case that would have been conflictual
such as setting differents trip for a regular zone and a multisensor zone.

Instead, I made a virtual thermal sensor that could registered in a thermal
zone, and have its own properties.
It could be added in the device tree, with the list of sensors to aggregate,
and the type of aggregation to be done.

As example:
  soc_max_sensor: soc_max_sensor {
    compatible = "generic,thermal-aggregator";
    #thermal-sensor-cells = <1>;
    type = "max";
    thermal-sensors = <&lvts 0>, <&lvts 1>, <&lvts 2>, <&lvts 3>,
          <&lvts 4>, <&lvts 5>, <&lvts 6>, <&lvts 7>,
          <&lvts 8>, <&lvts 9>, <&lvts 10>, <&lvts 11>,
          <&lvts 12>, <&lvts 13>, <&lvts 14>, <&lvts 15>,
          <&lvts 16>;
  };

The current series build and work but it would require to be completed
aswell a lot of cleanup.
Before working on it, I would like to get some feedback and I know if that
would an acceptable solution and continue that way.

Follows the following discussion:
https://patchwork.kernel.org/project/linux-mediatek/patch/20210617114707.10618-3-ben.tseng@mediatek.com/

Alexandre Bailon (2):
  thermal: provide a way to get thermal sensor from a device tree node
  thermal: add a virtual sensor to aggregate temperatures

 drivers/thermal/Kconfig              |   8 ++
 drivers/thermal/Makefile             |   1 +
 drivers/thermal/thermal_aggregator.c | 134 +++++++++++++++++++++++++++
 drivers/thermal/thermal_of.c         |  43 +++++++++
 include/linux/thermal.h              |  12 +++
 5 files changed, 198 insertions(+)
 create mode 100644 drivers/thermal/thermal_aggregator.c

-- 
2.31.1

[RFC PATCH 1/2] thermal: provide a way to get thermal sensor from a device tree node

[Alexandre Bailon]

In order to add support of a virtual thermal sensor,
add a way to get a sensor using the device tree node and sensor id.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/thermal/thermal_of.c | 43 ++++++++++++++++++++++++++++++++++++
 include/linux/thermal.h      | 12 ++++++++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 6379f26a335f6..4d3b07ceef41a 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -82,6 +82,46 @@ struct __thermal_zone {
 	const struct thermal_zone_of_device_ops *ops;
 };
 
+
+static LIST_HEAD(thermal_sensors);
+
+struct thermal_sensor *thermal_of_get_sensor(struct device_node *np, int id)
+{
+	struct thermal_sensor *sensor;
+	list_for_each_entry(sensor, &thermal_sensors, node) {
+		if (sensor->dev->of_node == np && sensor->id == id) {
+			return sensor;
+		}
+	}
+
+	return NULL;
+}
+
+static int thermal_of_register_sensor(struct device *dev, int id,
+				      void *sensor_data,
+				      const struct thermal_zone_of_device_ops *ops)
+{
+
+	struct thermal_sensor *sensor;
+
+	sensor = thermal_of_get_sensor(dev->of_node, id);
+	if (sensor)
+		return 0;
+
+	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	sensor->dev = dev;
+	sensor->id = id;
+	sensor->sensor_data = sensor_data;
+	sensor->ops = ops;
+
+	list_add(&sensor->node, &thermal_sensors);
+
+	return 0;
+}
+
+
+
+
 /***   DT thermal zone device callbacks   ***/
 
 static int of_thermal_get_temp(struct thermal_zone_device *tz,
@@ -518,6 +558,9 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
 			if (!IS_ERR(tzd))
 				thermal_zone_device_enable(tzd);
 
+			/* TODO handle errors */
+			thermal_of_register_sensor(dev, id, data, ops);
+
 			of_node_put(child);
 			goto exit;
 		}
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 8da5b61070472..3c46b142ebfef 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -104,6 +104,16 @@ struct thermal_cooling_device {
 	struct list_head node;
 };
 
+struct thermal_sensor {
+	struct device *dev;
+	int id;
+
+	void *sensor_data;
+	const struct thermal_zone_of_device_ops *ops;
+
+	struct list_head node;
+};
+
 /**
  * struct thermal_zone_device - structure for a thermal zone
  * @id:		unique id number for each thermal zone
@@ -394,6 +404,8 @@ int thermal_zone_get_offset(struct thermal_zone_device *tz);
 int thermal_zone_device_enable(struct thermal_zone_device *tz);
 int thermal_zone_device_disable(struct thermal_zone_device *tz);
 void thermal_zone_device_critical(struct thermal_zone_device *tz);
+
+struct thermal_sensor *thermal_of_get_sensor(struct device_node *np, int id);
 #else
 static inline struct thermal_zone_device *thermal_zone_device_register(
 	const char *type, int trips, int mask, void *devdata,
-- 
2.31.1

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

[Alexandre Bailon]

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

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

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 7a4ba50ba97d0..f9c152cfb95bc 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 THERMAL_AGGREGATOR
+	tristate "Generic thermal aggregator driver"
+	depends on TERMAL_OF || COMPILE_TEST
+	help
+	  This option enables the generic thermal sensor aggregator.
+	  This driver creates a thermal sensor that reads the hardware sensors
+	  and aggregate the temperature.
+
 config HISI_THERMAL
 	tristate "Hisilicon thermal driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 9729a2b089919..5b20ef15aebbe 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_THERMAL_AGGREGATOR) += thermal_aggregator.o
diff --git a/drivers/thermal/thermal_aggregator.c b/drivers/thermal/thermal_aggregator.c
new file mode 100644
index 0000000000000..76f871dbfee9e
--- /dev/null
+++ b/drivers/thermal/thermal_aggregator.c
@@ -0,0 +1,134 @@
+#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>
+
+const char *aggr_types[] = {
+	"min",
+	"max",
+	"avg",
+};
+
+struct thermal_aggr;
+
+typedef int (*aggregate_fn)(struct thermal_aggr *aggr);
+
+struct thermal_aggr_sensor {
+	struct thermal_sensor *sensor;
+
+	struct list_head node;
+};
+
+struct thermal_aggr {
+	struct list_head sensors;
+	aggregate_fn *aggregate;
+	//struct thermal_zone_device *tz;
+};
+
+static int thermal_aggr_read_temp(void *data, int *temperature)
+{
+	struct thermal_aggr *aggr = data;
+	struct thermal_aggr_sensor *sensor;
+	int max_temp = 0;
+	int temp;
+
+	list_for_each_entry(sensor, &aggr->sensors, node) {
+		if (!sensor->sensor) {
+			return -ENODEV;
+		}
+
+		sensor->sensor->ops->get_temp(sensor->sensor->sensor_data, &temp);
+		max_temp = max(max_temp, temp);
+	}
+
+	*temperature = max_temp;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops thermal_aggr_max_ops = {
+	.get_temp = thermal_aggr_read_temp,
+};
+
+static int thermal_aggr_probe(struct platform_device *pdev)
+{
+	struct thermal_aggr *aggr;
+	struct device *dev = &pdev->dev;
+	struct of_phandle_args args;
+	int count;
+	int ret;
+	int i;
+
+	count = of_count_phandle_with_args(dev->of_node,
+					   "thermal-sensors",
+					   "#thermal-sensor-cells");
+	if (count <= 0)
+		return -EINVAL;
+
+	aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
+	INIT_LIST_HEAD(&aggr->sensors);
+
+	for (i = 0; i < count; i++) {
+		struct thermal_sensor *sensor;
+		struct thermal_aggr_sensor *aggr_sensor;
+		int id;
+
+		ret = of_parse_phandle_with_args(dev->of_node,
+						 "thermal-sensors",
+						 "#thermal-sensor-cells",
+						 i,
+						 &args);
+		if (ret) {
+			return ret;
+		}
+
+		id = args.args_count ? args.args[0] : 0;
+		sensor = thermal_of_get_sensor(args.np, id);
+		if (sensor == NULL) {
+			return -EPROBE_DEFER;
+		}
+
+		aggr_sensor = kzalloc(sizeof(*aggr_sensor), GFP_KERNEL);
+		aggr_sensor->np = args.np;
+		aggr_sensor->id = id;
+		aggr_sensor->sensor = sensor;
+		list_add(&aggr_sensor->node, &aggr->sensors);
+	}
+
+	/*tzdev = */devm_thermal_zone_of_sensor_register(dev, 0, aggr, &thermal_aggr_max_ops);
+
+	return 0;
+}
+
+static int thermal_aggr_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static const struct of_device_id thermal_aggr_of_match[] = {
+	{
+		.compatible = "generic,thermal-aggregator",
+	},
+	{
+	},
+};
+MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
+
+static struct platform_driver thermal_aggr = {
+	.probe = thermal_aggr_probe,
+	.remove = thermal_aggr_remove,
+	.driver = {
+		.name = "thermal-aggregator",
+		.of_match_table = thermal_aggr_of_match,
+	},
+};
+
+module_platform_driver(thermal_aggr);
+MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
+MODULE_DESCRIPTION("Thermal sensor aggregator");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1

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

[Daniel Lezcano]

Hi Alexandre,

thanks for the proposal.

On 19/08/2021 14:32, Alexandre Bailon wrote:
> This series add a virtual thermal sensor that uses the hardware thermal sensors,
> aggregate them to return a temperature.
> 
> My first aptempt was to do the aggregation in the thermal zone but it was not
> that easy to do, and, there were some case that would have been conflictual
> such as setting differents trip for a regular zone and a multisensor zone.
> 
> Instead, I made a virtual thermal sensor that could registered in a thermal
> zone, and have its own properties.
> It could be added in the device tree, with the list of sensors to aggregate,
> and the type of aggregation to be done.
> 
> As example:
>   soc_max_sensor: soc_max_sensor {
>     compatible = "generic,thermal-aggregator";
>     #thermal-sensor-cells = <1>;
>     type = "max";
>     thermal-sensors = <&lvts 0>, <&lvts 1>, <&lvts 2>, <&lvts 3>,
>           <&lvts 4>, <&lvts 5>, <&lvts 6>, <&lvts 7>,
>           <&lvts 8>, <&lvts 9>, <&lvts 10>, <&lvts 11>,
>           <&lvts 12>, <&lvts 13>, <&lvts 14>, <&lvts 15>,
>           <&lvts 16>;
>   };
> 
> The current series build and work but it would require to be completed
> aswell a lot of cleanup.
> Before working on it, I would like to get some feedback and I know if that
> would an acceptable solution and continue that way.

Yes, I think it is going to the right direction.

IMO, we can get rid of the thermal_of changes. From a design PoV, the
patch itself should be the virtual thermal driver without any changes in
the core code, including thermal_of.

I have some comments on patch 2/2


> Follows the following discussion:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20210617114707.10618-3-ben.tseng@mediatek.com/
> 
> Alexandre Bailon (2):
>   thermal: provide a way to get thermal sensor from a device tree node
>   thermal: add a virtual sensor to aggregate temperatures
> 
>  drivers/thermal/Kconfig              |   8 ++
>  drivers/thermal/Makefile             |   1 +
>  drivers/thermal/thermal_aggregator.c | 134 +++++++++++++++++++++++++++
>  drivers/thermal/thermal_of.c         |  43 +++++++++
>  include/linux/thermal.h              |  12 +++
>  5 files changed, 198 insertions(+)
>  create mode 100644 drivers/thermal/thermal_aggregator.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: [RFC PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures

[Daniel Lezcano]

On 19/08/2021 14:32, Alexandre Bailon wrote:
> This adds a virtual thermal sensor that reads temperature from
> hardware sensor and return an aggregated temperature.
> Currently, this only return the max temperature.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/thermal/Kconfig              |   8 ++
>  drivers/thermal/Makefile             |   1 +
>  drivers/thermal/thermal_aggregator.c | 134 +++++++++++++++++++++++++++
>  3 files changed, 143 insertions(+)
>  create mode 100644 drivers/thermal/thermal_aggregator.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 7a4ba50ba97d0..f9c152cfb95bc 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 THERMAL_AGGREGATOR

We discussed the virtual sensor doing aggregation but may be we can give
it another purpose in the future like returning a constant temp or
low/high pass filter.

It may make sense to use a more generic name like virtual sensor for
example.

> +	tristate "Generic thermal aggregator driver"
> +	depends on TERMAL_OF || COMPILE_TEST

s/TERMAL_OF/THERMAL_OF/

> +	help
> +	  This option enables the generic thermal sensor aggregator.
> +	  This driver creates a thermal sensor that reads the hardware sensors
> +	  and aggregate the temperature.
> +
>  config HISI_THERMAL
>  	tristate "Hisilicon thermal driver"
>  	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 9729a2b089919..5b20ef15aebbe 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_THERMAL_AGGREGATOR) += thermal_aggregator.o
> diff --git a/drivers/thermal/thermal_aggregator.c b/drivers/thermal/thermal_aggregator.c
> new file mode 100644
> index 0000000000000..76f871dbfee9e
> --- /dev/null
> +++ b/drivers/thermal/thermal_aggregator.c
> @@ -0,0 +1,134 @@
> +#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>
> +
> +const char *aggr_types[] = {
> +	"min",
> +	"max",
> +	"avg",
> +};
> +
> +struct thermal_aggr;
> +
> +typedef int (*aggregate_fn)(struct thermal_aggr *aggr);
> +
> +struct thermal_aggr_sensor {
> +	struct thermal_sensor *sensor;
> +
> +	struct list_head node;
> +};
> +
> +struct thermal_aggr {
> +	struct list_head sensors;
> +	aggregate_fn *aggregate;
> +	//struct thermal_zone_device *tz;
> +};
> +
> +static int thermal_aggr_read_temp(void *data, int *temperature)
> +{
> +	struct thermal_aggr *aggr = data;
> +	struct thermal_aggr_sensor *sensor;
> +	int max_temp = 0;
> +	int temp;
> +

What happens if a hardware sensor module is unloaded ?

> +	list_for_each_entry(sensor, &aggr->sensors, node) {
> +		if (!sensor->sensor) {
> +			return -ENODEV;
> +		}




> +		sensor->sensor->ops->get_temp(sensor->sensor->sensor_data, &temp);
> +		max_temp = max(max_temp, temp);
> +	}
> +
> +	*temperature = max_temp;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops thermal_aggr_max_ops = {
> +	.get_temp = thermal_aggr_read_temp,

	.get_temp = thermal_virtual_sensor_get_temp ?
> +};
> +
> +static int thermal_aggr_probe(struct platform_device *pdev)
> +{
> +	struct thermal_aggr *aggr;
> +	struct device *dev = &pdev->dev;
> +	struct of_phandle_args args;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	count = of_count_phandle_with_args(dev->of_node,
> +					   "thermal-sensors",
> +					   "#thermal-sensor-cells");
> +	if (count <= 0)
> +		return -EINVAL;
> +
> +	aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);

	devm_kzalloc

> +	INIT_LIST_HEAD(&aggr->sensors);
> +
> +	for (i = 0; i < count; i++) {
> +		struct thermal_sensor *sensor;
> +		struct thermal_aggr_sensor *aggr_sensor;
> +		int id;
> +
> +		ret = of_parse_phandle_with_args(dev->of_node,
> +						 "thermal-sensors",
> +						 "#thermal-sensor-cells",
> +						 i,
> +						 &args);
> +		if (ret) {
> +			return ret;
> +		}
> +
> +		id = args.args_count ? args.args[0] : 0;
> +		sensor = thermal_of_get_sensor(args.np, id);
> +		if (sensor == NULL) {
> +			return -EPROBE_DEFER;
> +		}
> +
> +		aggr_sensor = kzalloc(sizeof(*aggr_sensor), GFP_KERNEL);

		devm_kzalloc

> +		aggr_sensor->np = args.np;

Why the 'np' and id are stored, they won't be needed anymore, no ?

> +		aggr_sensor->id = id;
> +		aggr_sensor->sensor = sensor;
> +		list_add(&aggr_sensor->node, &aggr->sensors);
> +	}
> +
> +	/*tzdev = */devm_thermal_zone_of_sensor_register(dev, 0, aggr, &thermal_aggr_max_ops);
> +
> +	return 0;
> +}
> +
> +static int thermal_aggr_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static const struct of_device_id thermal_aggr_of_match[] = {
> +	{
> +		.compatible = "generic,thermal-aggregator",  "^virtual,.*":

As stated in the documentation
Documentation/devicetree/bindings/vendor-prefixes.yaml

  "^virtual,.*":
    description: Used for virtual device without specific vendor.

I suggest something like:

	.compatible = "virtual,thermal-sensor",


> +	},
> +	{
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
> +
> +static struct platform_driver thermal_aggr = {
> +	.probe = thermal_aggr_probe,
> +	.remove = thermal_aggr_remove,
> +	.driver = {
> +		.name = "thermal-aggregator",
> +		.of_match_table = thermal_aggr_of_match,
> +	},
> +};
> +
> +module_platform_driver(thermal_aggr);
> +MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
> +MODULE_DESCRIPTION("Thermal sensor aggregator");
> +MODULE_LICENSE("GPL v2");
> 


-- 
<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: [RFC PATCH 0/2] Add a generic virtual thermal sensor

[Alexandre Bailon]

Hi Daniel,

On 20/08/2021 13:30, Daniel Lezcano wrote:
> Hi Alexandre,
>
> thanks for the proposal.
>
> On 19/08/2021 14:32, Alexandre Bailon wrote:
>> This series add a virtual thermal sensor that uses the hardware thermal sensors,
>> aggregate them to return a temperature.
>>
>> My first aptempt was to do the aggregation in the thermal zone but it was not
>> that easy to do, and, there were some case that would have been conflictual
>> such as setting differents trip for a regular zone and a multisensor zone.
>>
>> Instead, I made a virtual thermal sensor that could registered in a thermal
>> zone, and have its own properties.
>> It could be added in the device tree, with the list of sensors to aggregate,
>> and the type of aggregation to be done.
>>
>> As example:
>>    soc_max_sensor: soc_max_sensor {
>>      compatible = "generic,thermal-aggregator";
>>      #thermal-sensor-cells = <1>;
>>      type = "max";
>>      thermal-sensors = <&lvts 0>, <&lvts 1>, <&lvts 2>, <&lvts 3>,
>>            <&lvts 4>, <&lvts 5>, <&lvts 6>, <&lvts 7>,
>>            <&lvts 8>, <&lvts 9>, <&lvts 10>, <&lvts 11>,
>>            <&lvts 12>, <&lvts 13>, <&lvts 14>, <&lvts 15>,
>>            <&lvts 16>;
>>    };
>>
>> The current series build and work but it would require to be completed
>> aswell a lot of cleanup.
>> Before working on it, I would like to get some feedback and I know if that
>> would an acceptable solution and continue that way.
> Yes, I think it is going to the right direction.
>
> IMO, we can get rid of the thermal_of changes. From a design PoV, the
> patch itself should be the virtual thermal driver without any changes in
> the core code, including thermal_of.
I made that changes in order to be able to get the hw sensors from the 
virtual sensor.
I am not really satisfied of that patch but that the simplest way I 
found to do it.
How would you proceed to get the hw sensor from its device tree phandle 
and id ?

Thanks,
Alexandre

>
> I have some comments on patch 2/2
>
>
>> Follows the following discussion:
>> https://patchwork.kernel.org/project/linux-mediatek/patch/20210617114707.10618-3-ben.tseng@mediatek.com/
>>
>> Alexandre Bailon (2):
>>    thermal: provide a way to get thermal sensor from a device tree node
>>    thermal: add a virtual sensor to aggregate temperatures
>>
>>   drivers/thermal/Kconfig              |   8 ++
>>   drivers/thermal/Makefile             |   1 +
>>   drivers/thermal/thermal_aggregator.c | 134 +++++++++++++++++++++++++++
>>   drivers/thermal/thermal_of.c         |  43 +++++++++
>>   include/linux/thermal.h              |  12 +++
>>   5 files changed, 198 insertions(+)
>>   create mode 100644 drivers/thermal/thermal_aggregator.c
>>
>

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

[Alexandre Bailon]

Hi Daniel,

On 20/08/2021 14:52, Daniel wrote:
> On 19/08/2021 14:32, Alexandre Bailon wrote:
>> This adds a virtual thermal sensor that reads temperature from
>> hardware sensor and return an aggregated temperature.
>> Currently, this only return the max temperature.
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>>   drivers/thermal/Kconfig              |   8 ++
>>   drivers/thermal/Makefile             |   1 +
>>   drivers/thermal/thermal_aggregator.c | 134 +++++++++++++++++++++++++++
>>   3 files changed, 143 insertions(+)
>>   create mode 100644 drivers/thermal/thermal_aggregator.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 7a4ba50ba97d0..f9c152cfb95bc 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 THERMAL_AGGREGATOR
> We discussed the virtual sensor doing aggregation but may be we can give
> it another purpose in the future like returning a constant temp or
> low/high pass filter.
>
> It may make sense to use a more generic name like virtual sensor for
> example.
Indeed, this would make more sense.
>
>> +	tristate "Generic thermal aggregator driver"
>> +	depends on TERMAL_OF || COMPILE_TEST
> s/TERMAL_OF/THERMAL_OF/
>
>> +	help
>> +	  This option enables the generic thermal sensor aggregator.
>> +	  This driver creates a thermal sensor that reads the hardware sensors
>> +	  and aggregate the temperature.
>> +
>>   config HISI_THERMAL
>>   	tristate "Hisilicon thermal driver"
>>   	depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 9729a2b089919..5b20ef15aebbe 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_THERMAL_AGGREGATOR) += thermal_aggregator.o
>> diff --git a/drivers/thermal/thermal_aggregator.c b/drivers/thermal/thermal_aggregator.c
>> new file mode 100644
>> index 0000000000000..76f871dbfee9e
>> --- /dev/null
>> +++ b/drivers/thermal/thermal_aggregator.c
>> @@ -0,0 +1,134 @@
>> +#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>
>> +
>> +const char *aggr_types[] = {
>> +	"min",
>> +	"max",
>> +	"avg",
>> +};
>> +
>> +struct thermal_aggr;
>> +
>> +typedef int (*aggregate_fn)(struct thermal_aggr *aggr);
>> +
>> +struct thermal_aggr_sensor {
>> +	struct thermal_sensor *sensor;
>> +
>> +	struct list_head node;
>> +};
>> +
>> +struct thermal_aggr {
>> +	struct list_head sensors;
>> +	aggregate_fn *aggregate;
>> +	//struct thermal_zone_device *tz;
>> +};
>> +
>> +static int thermal_aggr_read_temp(void *data, int *temperature)
>> +{
>> +	struct thermal_aggr *aggr = data;
>> +	struct thermal_aggr_sensor *sensor;
>> +	int max_temp = 0;
>> +	int temp;
>> +
> What happens if a hardware sensor module is unloaded ?
Hum, I don't know how to deal with it.
Maybe adding refcounting to sensors to prevent module unloading ?
>
>> +	list_for_each_entry(sensor, &aggr->sensors, node) {
>> +		if (!sensor->sensor) {
>> +			return -ENODEV;
>> +		}
>
>
>
>> +		sensor->sensor->ops->get_temp(sensor->sensor->sensor_data, &temp);
>> +		max_temp = max(max_temp, temp);
>> +	}
>> +
>> +	*temperature = max_temp;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct thermal_zone_of_device_ops thermal_aggr_max_ops = {
>> +	.get_temp = thermal_aggr_read_temp,
> 	.get_temp = thermal_virtual_sensor_get_temp ?

Actually, I though I could create a thermol_zone_of_device_ops for each 
type of operation
(min, max, etc) we would support and would just to register the right 
ops at probe time.

>> +};
>> +
>> +static int thermal_aggr_probe(struct platform_device *pdev)
>> +{
>> +	struct thermal_aggr *aggr;
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_args args;
>> +	int count;
>> +	int ret;
>> +	int i;
>> +
>> +	count = of_count_phandle_with_args(dev->of_node,
>> +					   "thermal-sensors",
>> +					   "#thermal-sensor-cells");
>> +	if (count <= 0)
>> +		return -EINVAL;
>> +
>> +	aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> 	devm_kzalloc
>
>> +	INIT_LIST_HEAD(&aggr->sensors);
>> +
>> +	for (i = 0; i < count; i++) {
>> +		struct thermal_sensor *sensor;
>> +		struct thermal_aggr_sensor *aggr_sensor;
>> +		int id;
>> +
>> +		ret = of_parse_phandle_with_args(dev->of_node,
>> +						 "thermal-sensors",
>> +						 "#thermal-sensor-cells",
>> +						 i,
>> +						 &args);
>> +		if (ret) {
>> +			return ret;
>> +		}
>> +
>> +		id = args.args_count ? args.args[0] : 0;
>> +		sensor = thermal_of_get_sensor(args.np, id);
>> +		if (sensor == NULL) {
>> +			return -EPROBE_DEFER;
>> +		}
>> +
>> +		aggr_sensor = kzalloc(sizeof(*aggr_sensor), GFP_KERNEL);
> 		devm_kzalloc
>
>> +		aggr_sensor->np = args.np;
> Why the 'np' and id are stored, they won't be needed anymore, no ?
Right. At some point, I had issues with the sensors that was not available.
I though it was an probe orderings issue and I tried to get the sensor 
later from the callback.
It was an issue with the sensor module itself, and not with the ordering 
but I forgot to remove
np and id that not useful anymore.
>
>> +		aggr_sensor->id = id;
>> +		aggr_sensor->sensor = sensor;
>> +		list_add(&aggr_sensor->node, &aggr->sensors);
>> +	}
>> +
>> +	/*tzdev = */devm_thermal_zone_of_sensor_register(dev, 0, aggr, &thermal_aggr_max_ops);
>> +
>> +	return 0;
>> +}
>> +
>> +static int thermal_aggr_remove(struct platform_device *pdev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id thermal_aggr_of_match[] = {
>> +	{
>> +		.compatible = "generic,thermal-aggregator",  "^virtual,.*":
> As stated in the documentation
> Documentation/devicetree/bindings/vendor-prefixes.yaml
>
>    "^virtual,.*":
>      description: Used for virtual device without specific vendor.
>
> I suggest something like:
>
> 	.compatible = "virtual,thermal-sensor",
OK, makes sense to me.

Thanks you for the review.
Alexandre

>
>
>> +	},
>> +	{
>> +	},
>> +};
>> +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match);
>> +
>> +static struct platform_driver thermal_aggr = {
>> +	.probe = thermal_aggr_probe,
>> +	.remove = thermal_aggr_remove,
>> +	.driver = {
>> +		.name = "thermal-aggregator",
>> +		.of_match_table = thermal_aggr_of_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(thermal_aggr);
>> +MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>");
>> +MODULE_DESCRIPTION("Thermal sensor aggregator");
>> +MODULE_LICENSE("GPL v2");
>>
>

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

[Daniel Lezcano]

On 23/08/2021 09:35, Alexandre Bailon wrote:

[ ... ]

> I am not really satisfied of that patch but that the simplest way I
> found to do it.
> How would you proceed to get the hw sensor from its device tree phandle
> and id ?

Could the function 'thermal_zone_of_get_sensor_id' help ?


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