| Message ID | 20210819123215.591593-3-abailon@baylibre.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Add a generic virtual thermal sensor | expand |
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"); >
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"); >> >
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");
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