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/[email protected]/
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
This adds a virtual thermal sensor that reads temperature from
hardware sensor and return an aggregated temperature.
Currently, this only return the max temperature.
Signed-off-by: Alexandre Bailon <[email protected]>
---
drivers/thermal/Kconfig | 8 ++
drivers/thermal/Makefile | 1 +
drivers/thermal/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 <[email protected]>");
+MODULE_DESCRIPTION("Thermal sensor aggregator");
+MODULE_LICENSE("GPL v2");
--
2.31.1
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/[email protected]/
>
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>");
> +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
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/[email protected]/
>>
>> 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
>>
>
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 <[email protected]>
>> ---
>> 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 <[email protected]>");
>> +MODULE_DESCRIPTION("Thermal sensor aggregator");
>> +MODULE_LICENSE("GPL v2");
>>
>
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