* [PATCH 0/2] Add a generic virtual thermal sensor @ 2021-09-06 19:04 Alexandre Bailon 2021-09-06 19:04 ` [PATCH 1/2] dt-bindings: Add bindings for the " Alexandre Bailon 2021-09-06 19:04 ` [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures Alexandre Bailon 0 siblings, 2 replies; 7+ messages in thread From: Alexandre Bailon @ 2021-09-06 19:04 UTC (permalink / raw) To: rui.zhang, daniel.lezcano, amitk Cc: linux-pm, linux-kernel, ben.tseng, khilman, gpain, Alexandre Bailon This series add a virtual thermal sensor. It could be used to get a temperature using some thermal sensors. Currently, the supported operations are max, min and avg. The virtual sensor could be easily extended to support others operations. Note: Currently, thermal drivers must explicitly register their sensors to make them available to the virtual sensor. This doesn't seem a good solution to me and I think it would be preferable to update the framework to register the list of each available sensors. Alexandre Bailon (2): dt-bindings: Add bindings for the virtual thermal sensor thermal: add a virtual sensor to aggregate temperatures .../thermal/virtual,thermal-sensor.yaml | 67 +++ drivers/thermal/Kconfig | 8 + drivers/thermal/Makefile | 1 + drivers/thermal/virtual-sensor.h | 51 +++ drivers/thermal/virtual_sensor.c | 400 ++++++++++++++++++ include/dt-bindings/thermal/virtual-sensor.h | 15 + 6 files changed, 542 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml create mode 100644 drivers/thermal/virtual-sensor.h create mode 100644 drivers/thermal/virtual_sensor.c create mode 100644 include/dt-bindings/thermal/virtual-sensor.h -- 2.31.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] dt-bindings: Add bindings for the virtual thermal sensor 2021-09-06 19:04 [PATCH 0/2] Add a generic virtual thermal sensor Alexandre Bailon @ 2021-09-06 19:04 ` Alexandre Bailon 2021-10-07 15:56 ` Rafael J. Wysocki 2021-09-06 19:04 ` [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures Alexandre Bailon 1 sibling, 1 reply; 7+ messages in thread From: Alexandre Bailon @ 2021-09-06 19:04 UTC (permalink / raw) To: rui.zhang, daniel.lezcano, amitk Cc: linux-pm, linux-kernel, ben.tseng, khilman, gpain, Alexandre Bailon This adds the device tree bidings for the virtual thermal sensor. The virtual sensor could be used to a temperature computed from many thermal sensors. Signed-off-by: Alexandre Bailon <abailon@baylibre.com> --- .../thermal/virtual,thermal-sensor.yaml | 67 +++++++++++++++++++ include/dt-bindings/thermal/virtual-sensor.h | 15 +++++ 2 files changed, 82 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml create mode 100644 include/dt-bindings/thermal/virtual-sensor.h diff --git a/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml new file mode 100644 index 0000000000000..848b5912c79f1 --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright 2021 BayLibre +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/thermal/thermal-sensor.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Virtual thermal sensor binding + +description: | + The virtual thermal sensor devices provide temperature sensing capabilities + based on hardware thermal sensors. Basically, this could be used to get the + maximum, minimum or average temperature of the hardware thermal sensors. +properties: + "#thermal-sensor-cells": + description: + Used to uniquely identify a thermal sensor instance within an IC. Will be + 0 on sensor nodes with only a single sensor and at least 1 on nodes + containing several internal sensors. + enum: [0, 1] + + type: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Used to select the operations to perform on the sensors to get the virtual + sensor temperature. + enum: + - VIRTUAL_SENSOR_MIN + - VIRTUAL_SENSOR_MAX + - VIRTUAL_SENSOR_AVG + + thermal-sensors: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: + The thermal sensor phandle and sensor specifier used to monitor this + thermal zone. + +required: + - "#thermal-sensor-cells" + - type + - thermal-sensors + +additionalProperties: true + +examples: + - | + #include <dt-bindings/thermal/thermal.h> + #include <dt-bindings/thermal/virtual-sensor.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/mt8192-clk.h> + + lvts: lvts@1100b000 { + compatible = "mediatek,mt6873-lvts"; + reg = <0x1100b000 0x1000>; + clocks = <&infracfg CLK_INFRA_THERM>; + clock-names = "lvts_clk"; + #thermal-sensor-cells = <0>; + interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>; + }; + + soc_max_sensor: soc_max_sensor { + compatible = "virtual,thermal-sensor"; + #thermal-sensor-cells = <1>; + type = <VIRTUAL_SENSOR_MAX>; + thermal-sensors = <&lvts 0>, <&lvts 1>; + }; +... diff --git a/include/dt-bindings/thermal/virtual-sensor.h b/include/dt-bindings/thermal/virtual-sensor.h new file mode 100644 index 0000000000000..b3e4032f6f62b --- /dev/null +++ b/include/dt-bindings/thermal/virtual-sensor.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * This header provides constants for virtual thermal sensor bindings. + * + * Copyright (C) 2021 BayLibre + */ + +#ifndef _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H +#define _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H + +#define VIRTUAL_SENSOR_MIN 0 +#define VIRTUAL_SENSOR_MAX 1 +#define VIRTUAL_SENSOR_AVG 2 + +#endif /* _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: Add bindings for the virtual thermal sensor 2021-09-06 19:04 ` [PATCH 1/2] dt-bindings: Add bindings for the " Alexandre Bailon @ 2021-10-07 15:56 ` Rafael J. Wysocki 0 siblings, 0 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2021-10-07 15:56 UTC (permalink / raw) To: Alexandre Bailon Cc: Zhang, Rui, Daniel Lezcano, Amit Kucheria, Linux PM, Linux Kernel Mailing List, ben.tseng, Kevin Hilman, gpain On Mon, Sep 6, 2021 at 9:05 PM Alexandre Bailon <abailon@baylibre.com> wrote: > > This adds the device tree bidings for the virtual thermal sensor. I'm not sure what "the virtual thermal sensor" is. I'm guessing that you mean "DT bindings for the DT-based virtual sensor driver introduced by a subsequent patch" or something like this. I also guess that the purpose is to allow the platform designer to tell the kernel that some sensors need to be aggregated in order to get useful information from them and how to aggregate them. Otherwise it would be hard to say why the aggregation needed to take place in the kernel. Moreover, the aggregation functions supported by this series are somewhat simple and I'm not sure if they are really sufficient in practice. > The virtual sensor could be used to a temperature computed from > many thermal sensors. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> Admittedly, I'm not a DT bindings expert, so if I say something blatantly silly below, sorry about that. > --- > .../thermal/virtual,thermal-sensor.yaml | 67 +++++++++++++++++++ > include/dt-bindings/thermal/virtual-sensor.h | 15 +++++ > 2 files changed, 82 insertions(+) > create mode 100644 Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml > create mode 100644 include/dt-bindings/thermal/virtual-sensor.h > > diff --git a/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml > new file mode 100644 > index 0000000000000..848b5912c79f1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/thermal/virtual,thermal-sensor.yaml > @@ -0,0 +1,67 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2021 BayLibre > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/thermal/thermal-sensor.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Virtual thermal sensor binding > + > +description: | > + The virtual thermal sensor devices provide temperature sensing capabilities > + based on hardware thermal sensors. Basically, this could be used to get the > + maximum, minimum or average temperature of the hardware thermal sensors. > +properties: > + "#thermal-sensor-cells": It isn't clear to me why this is needed. If the "thermal-sensors" property is required anyway, I'm not sure why it's still necessary to have another one to find out whether there is just one sensor or more of them. > + description: > + Used to uniquely identify a thermal sensor instance within an IC. Will be > + 0 on sensor nodes with only a single sensor and at least 1 on nodes > + containing several internal sensors. > + enum: [0, 1] > + > + type: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Used to select the operations to perform on the sensors to get the virtual > + sensor temperature. > + enum: > + - VIRTUAL_SENSOR_MIN > + - VIRTUAL_SENSOR_MAX > + - VIRTUAL_SENSOR_AVG > + > + thermal-sensors: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + description: > + The thermal sensor phandle and sensor specifier used to monitor this > + thermal zone. > + > +required: > + - "#thermal-sensor-cells" > + - type > + - thermal-sensors > + > +additionalProperties: true > + > +examples: > + - | > + #include <dt-bindings/thermal/thermal.h> > + #include <dt-bindings/thermal/virtual-sensor.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/mt8192-clk.h> > + > + lvts: lvts@1100b000 { > + compatible = "mediatek,mt6873-lvts"; > + reg = <0x1100b000 0x1000>; > + clocks = <&infracfg CLK_INFRA_THERM>; > + clock-names = "lvts_clk"; > + #thermal-sensor-cells = <0>; > + interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>; > + }; > + > + soc_max_sensor: soc_max_sensor { > + compatible = "virtual,thermal-sensor"; Where/how is the above defined? > + #thermal-sensor-cells = <1>; > + type = <VIRTUAL_SENSOR_MAX>; > + thermal-sensors = <&lvts 0>, <&lvts 1>; > + }; > +... > diff --git a/include/dt-bindings/thermal/virtual-sensor.h b/include/dt-bindings/thermal/virtual-sensor.h > new file mode 100644 > index 0000000000000..b3e4032f6f62b > --- /dev/null > +++ b/include/dt-bindings/thermal/virtual-sensor.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * This header provides constants for virtual thermal sensor bindings. > + * > + * Copyright (C) 2021 BayLibre > + */ > + > +#ifndef _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H > +#define _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H > + It would be better to use an enum type here. > +#define VIRTUAL_SENSOR_MIN 0 > +#define VIRTUAL_SENSOR_MAX 1 > +#define VIRTUAL_SENSOR_AVG 2 Also note that the _MIN and _MAX symbols may be confused as limits, so it may be better to call them _MIN_VAL and _MAX_VAL, respectively. > + > +#endif /* _DT_BINDINGS_THERMAL_VIRTUAL_SENSOR_H */ > -- ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures 2021-09-06 19:04 [PATCH 0/2] Add a generic virtual thermal sensor Alexandre Bailon 2021-09-06 19:04 ` [PATCH 1/2] dt-bindings: Add bindings for the " Alexandre Bailon @ 2021-09-06 19:04 ` Alexandre Bailon 2021-09-07 5:14 ` kernel test robot 2021-09-09 22:29 ` Matthias Kaehlcke 1 sibling, 2 replies; 7+ messages in thread From: Alexandre Bailon @ 2021-09-06 19:04 UTC (permalink / raw) To: rui.zhang, daniel.lezcano, amitk Cc: linux-pm, linux-kernel, ben.tseng, khilman, gpain, 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/virtual-sensor.h | 51 ++++ drivers/thermal/virtual_sensor.c | 400 +++++++++++++++++++++++++++++++ 4 files changed, 460 insertions(+) create mode 100644 drivers/thermal/virtual-sensor.h create mode 100644 drivers/thermal/virtual_sensor.c diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 7a4ba50ba97d0..23dc903da2fc5 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 VIRTUAL_THERMAL + tristate "Generic virtual thermal sensor driver" + depends on THERMAL_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..76dfa1d61bfc5 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_VIRTUAL_THERMAL) += virtual_sensor.o diff --git a/drivers/thermal/virtual-sensor.h b/drivers/thermal/virtual-sensor.h new file mode 100644 index 0000000000000..e024d434856c7 --- /dev/null +++ b/drivers/thermal/virtual-sensor.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2021 BayLibre + */ + +#ifndef __THERMAL_VIRTUAL_SENSOR_H__ +#define __THERMAL_VIRTUAL_SENSOR_H__ + +struct virtual_sensor; +struct virtual_sensor_data; + +#ifdef CONFIG_VIRTUAL_THERMAL +struct virtual_sensor_data * +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, + const struct thermal_zone_of_device_ops *ops); +void thermal_virtual_sensor_unregister(struct device *dev, + struct virtual_sensor_data *sensor_data); +struct virtual_sensor_data * +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, + const struct thermal_zone_of_device_ops *ops); + +void devm_thermal_virtual_sensor_unregister(struct device *dev, + struct virtual_sensor *sensor); +#else +static inline struct virtual_sensor_data * +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, + const struct thermal_zone_of_device_ops *ops) +{ + return ERR_PTR(-ENODEV); +} + +void thermal_virtual_sensor_unregister(struct device *dev, + struct virtual_sensor_data *sensor_data) +{ +} + +static inline struct virtual_sensor_data * +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, + const struct thermal_zone_of_device_ops *ops) +{ + return ERR_PTR(-ENODEV); +} + +static inline +void devm_thermal_virtual_sensor_unregister(struct device *dev, + struct virtual_sensor *sensor) +{ +} +#endif + +#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */ diff --git a/drivers/thermal/virtual_sensor.c b/drivers/thermal/virtual_sensor.c new file mode 100644 index 0000000000000..e5bb0ef9adb39 --- /dev/null +++ b/drivers/thermal/virtual_sensor.c @@ -0,0 +1,400 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2021 BayLibre + */ + +#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> + +#include <dt-bindings/thermal/virtual-sensor.h> + +#include "virtual-sensor.h" + +struct virtual_sensor_data { + struct list_head node; + + /* sensor interface */ + int id; + void *sensor_data; + const struct thermal_zone_of_device_ops *ops; +}; + +struct virtual_sensor { + int count; + struct virtual_sensor_data *sensors; + struct thermal_zone_device *tzd; + + struct list_head node; +}; + +static LIST_HEAD(thermal_sensors); +static LIST_HEAD(virtual_sensors); + +static int virtual_sensor_get_temp_max(void *data, int *temperature) +{ + struct virtual_sensor *sensor = data; + int max_temp = INT_MIN; + int temp; + int i; + + for (i = 0; i < sensor->count; i++) { + struct virtual_sensor_data *hw_sensor; + + hw_sensor = &sensor->sensors[i]; + if (!hw_sensor->ops) + return -ENODEV; + + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); + max_temp = max(max_temp, temp); + } + + *temperature = max_temp; + + return 0; +} + +static const struct thermal_zone_of_device_ops virtual_sensor_max_ops = { + .get_temp = virtual_sensor_get_temp_max, +}; + +static int virtual_sensor_get_temp_min(void *data, int *temperature) +{ + struct virtual_sensor *sensor = data; + int min_temp = INT_MAX; + int temp; + int i; + + for (i = 0; i < sensor->count; i++) { + struct virtual_sensor_data *hw_sensor; + + hw_sensor = &sensor->sensors[i]; + if (!hw_sensor->ops) + return -ENODEV; + + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); + min_temp = min(min_temp, temp); + } + + *temperature = min_temp; + + return 0; +} + +static const struct thermal_zone_of_device_ops virtual_sensor_min_ops = { + .get_temp = virtual_sensor_get_temp_min, +}; + +static int do_avg(int val1, int val2) +{ + return ((val1) / 2) + ((val2) / 2) + (((val1) % 2 + (val2) % 2) / 2); +} + +static int virtual_sensor_get_temp_avg(void *data, int *temperature) +{ + struct virtual_sensor *sensor = data; + int avg_temp = 0; + int temp; + int i; + + for (i = 0; i < sensor->count; i++) { + struct virtual_sensor_data *hw_sensor; + + hw_sensor = &sensor->sensors[i]; + if (!hw_sensor->ops) + return -ENODEV; + + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); + avg_temp = do_avg(avg_temp, temp); + } + + *temperature = avg_temp; + + return 0; +} + +static const struct thermal_zone_of_device_ops virtual_sensor_avg_ops = { + .get_temp = virtual_sensor_get_temp_avg, +}; + +static int register_virtual_sensor(struct virtual_sensor *sensor, + struct of_phandle_args args, + int index) +{ + struct virtual_sensor_data *sensor_data; + int id; + + list_for_each_entry(sensor_data, &thermal_sensors, node) { + id = args.args_count ? args.args[0] : 0; + if (sensor_data->id == id) { + memcpy(&sensor->sensors[index], sensor_data, + sizeof(*sensor_data)); + return 0; + } + } + + return -ENODEV; +} + +static int virtual_sensor_probe(struct platform_device *pdev) +{ + const struct thermal_zone_of_device_ops *ops; + struct virtual_sensor *sensor; + struct device *dev = &pdev->dev; + struct of_phandle_args args; + u32 type; + int ret; + int i; + + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); + if (!sensor) + return -ENOMEM; + + sensor->count = of_count_phandle_with_args(dev->of_node, + "thermal-sensors", + "#thermal-sensor-cells"); + if (sensor->count <= 0) + return -EINVAL; + + sensor->sensors = devm_kmalloc_array(dev, sensor->count, + sizeof(*sensor->sensors), + GFP_KERNEL); + if (!sensor->sensors) + return -ENOMEM; + + for (i = 0; i < sensor->count; i++) { + ret = of_parse_phandle_with_args(dev->of_node, + "thermal-sensors", + "#thermal-sensor-cells", + i, + &args); + if (ret) + return ret; + + ret = register_virtual_sensor(sensor, args, i); + if (ret) + return ret; + } + + ret = of_property_read_u32(dev->of_node, "type", &type); + if (ret) + return ret; + + switch (type) { + case VIRTUAL_SENSOR_MAX: + ops = &virtual_sensor_max_ops; + break; + case VIRTUAL_SENSOR_MIN: + ops = &virtual_sensor_min_ops; + break; + case VIRTUAL_SENSOR_AVG: + ops = &virtual_sensor_avg_ops; + break; + default: + return -EINVAL; + } + + sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor, ops); + if (IS_ERR(sensor->tzd)) + return PTR_ERR(sensor->tzd); + + platform_set_drvdata(pdev, sensor); + list_add(&sensor->node, &virtual_sensors); + + return 0; +} + +static int virtual_sensor_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct virtual_sensor *sensor; + + sensor = platform_get_drvdata(pdev); + list_del(&sensor->node); + + devm_thermal_zone_of_sensor_unregister(dev, sensor->tzd); + devm_kfree(dev, sensor->sensors); + devm_kfree(dev, sensor); + + return 0; +} + +static const struct of_device_id virtual_sensor_of_match[] = { + { + .compatible = "virtual,thermal-sensor", + }, + { + }, +}; +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match); + +static struct platform_driver virtual_sensor = { + .probe = virtual_sensor_probe, + .remove = virtual_sensor_remove, + .driver = { + .name = "virtual-sensor", + .of_match_table = virtual_sensor_of_match, + }, +}; + +/** + * thermal_virtual_sensor_register - registers a sensor that could by a virtual + * sensor + * @dev: a valid struct device pointer of a sensor device. Must contain + * a valid .of_node, for the sensor node. + * @sensor_id: a sensor identifier, in case the sensor IP has more + * than one sensors + * @data: a private pointer (owned by the caller) that will be passed + * back, when a temperature reading is needed. + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. + * + * This function will register a thermal sensor to make it available for later + * usage by a virtual sensor. + * + * The thermal zone temperature is provided by the @get_temp function + * pointer. When called, it will have the private pointer @data back. + * + * Return: On success returns a valid struct thermal_zone_device, + * otherwise, it returns a corresponding ERR_PTR(). Caller must + * check the return value with help of IS_ERR() helper. + */ +struct virtual_sensor_data *thermal_virtual_sensor_register( + struct device *dev, int sensor_id, void *data, + const struct thermal_zone_of_device_ops *ops) +{ + struct virtual_sensor_data *sensor_data; + + sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL); + if (!sensor_data) + return ERR_PTR(-ENOMEM); + + sensor_data->id = sensor_id; + sensor_data->sensor_data = data; + sensor_data->ops = ops; + + list_add(&sensor_data->node, &thermal_sensors); + + return sensor_data; +} +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register); + +/** + * thermal_virtual_sensor_unregister - unregisters a sensor + * @dev: a valid struct device pointer of a sensor device. + * @sensor_data: a pointer to struct virtual_sensor_data to unregister. + * + * This function removes the sensor from the list of available thermal sensors. + * If the sensor is in use, then the next call to .get_temp will return -ENODEV. + */ +void thermal_virtual_sensor_unregister(struct device *dev, + struct virtual_sensor_data *sensor_data) +{ + struct virtual_sensor_data *temp; + struct virtual_sensor *sensor; + int i; + + list_del(&sensor_data->node); + + list_for_each_entry(sensor, &virtual_sensors, node) { + for (i = 0; i < sensor->count; i++) { + temp = &sensor->sensors[i]; + if (temp->id == sensor_data->id && + temp->sensor_data == sensor_data->sensor_data) { + temp->ops = NULL; + } + } + } + devm_kfree(dev, sensor_data); +} +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister); + +static void devm_thermal_virtual_sensor_release(struct device *dev, void *res) +{ + thermal_virtual_sensor_unregister(dev, + *(struct virtual_sensor_data **)res); +} + +static int devm_thermal_virtual_sensor_match(struct device *dev, void *res, + void *data) +{ + struct virtual_sensor_data **r = res; + + if (WARN_ON(!r || !*r)) + return 0; + + return *r == data; +} + + +/** + * devm_thermal_virtual_sensor_register - Resource managed version of + * thermal_virtual_sensor_register() + * @dev: a valid struct device pointer of a sensor device. Must contain + * a valid .of_node, for the sensor node. + * @sensor_id: a sensor identifier, in case the sensor IP has more + * than one sensors + * @data: a private pointer (owned by the caller) that will be passed + * back, when a temperature reading is needed. + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. + * + * Refer thermal_zone_of_sensor_register() for more details. + * + * Return: On success returns a valid struct virtual_sensor_data, + * otherwise, it returns a corresponding ERR_PTR(). Caller must + * check the return value with help of IS_ERR() helper. + * Registered virtual_sensor_data device will automatically be + * released when device is unbounded. + */ +struct virtual_sensor_data *devm_thermal_virtual_sensor_register( + struct device *dev, int sensor_id, + void *data, const struct thermal_zone_of_device_ops *ops) +{ + struct virtual_sensor_data **ptr, *sensor_data; + + ptr = devres_alloc(devm_thermal_virtual_sensor_release, sizeof(*ptr), + GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + sensor_data = thermal_virtual_sensor_register(dev, sensor_id, data, ops); + if (IS_ERR(sensor_data)) { + devres_free(ptr); + return sensor_data; + } + + *ptr = sensor_data; + devres_add(dev, ptr); + + return sensor_data; +} +EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_register); + +/** + * devm_thermal_virtual_sensor_unregister - Resource managed version of + * thermal_virtual_sensor_unregister(). + * @dev: Device for which resource was allocated. + * @sensor: a pointer to struct thermal_zone_device where the sensor is registered. + * + * This function removes the sensor from the list of sensors registered with + * devm_thermal_virtual_sensor_register() API. + * Normally this function will not need to be called and the resource + * management code will ensure that the resource is freed. + */ +void devm_thermal_virtual_sensor_unregister(struct device *dev, + struct virtual_sensor *sensor) +{ + WARN_ON(devres_release(dev, devm_thermal_virtual_sensor_release, + devm_thermal_virtual_sensor_match, sensor)); +} +EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_unregister); + +module_platform_driver(virtual_sensor); +MODULE_AUTHOR("Alexandre Bailon <abailon@baylibre.com>"); +MODULE_DESCRIPTION("Virtual thermal sensor"); +MODULE_LICENSE("GPL v2"); -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures 2021-09-06 19:04 ` [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures Alexandre Bailon @ 2021-09-07 5:14 ` kernel test robot 2021-09-09 22:29 ` Matthias Kaehlcke 1 sibling, 0 replies; 7+ messages in thread From: kernel test robot @ 2021-09-07 5:14 UTC (permalink / raw) To: Alexandre Bailon, rui.zhang, daniel.lezcano, amitk Cc: kbuild-all, linux-pm, linux-kernel, ben.tseng, khilman, gpain, Alexandre Bailon [-- Attachment #1: Type: text/plain, Size: 12231 bytes --] Hi Alexandre, Thank you for the patch! Yet something to improve: [auto build test ERROR on robh/for-next] [also build test ERROR on linus/master v5.14 next-20210906] [cannot apply to thermal/next soc-thermal/next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Alexandre-Bailon/Add-a-generic-virtual-thermal-sensor/20210907-030547 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next config: riscv-allmodconfig (attached as .config) compiler: riscv64-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/2e3e80b0ee6c69039ada990aaf0380e8c6c024c0 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Alexandre-Bailon/Add-a-generic-virtual-thermal-sensor/20210907-030547 git checkout 2e3e80b0ee6c69039ada990aaf0380e8c6c024c0 # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash drivers/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from drivers/thermal/virtual_sensor.c:18: drivers/thermal/virtual-sensor.h:32:6: warning: no previous prototype for 'thermal_virtual_sensor_unregister' [-Wmissing-prototypes] 32 | void thermal_virtual_sensor_unregister(struct device *dev, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/thermal/virtual_sensor.c:8: >> drivers/thermal/virtual_sensor.c:235:25: error: 'thermal_aggr_of_match' undeclared here (not in a function) 235 | MODULE_DEVICE_TABLE(of, thermal_aggr_of_match); | ^~~~~~~~~~~~~~~~~~~~~ include/linux/module.h:244:15: note: in definition of macro 'MODULE_DEVICE_TABLE' 244 | extern typeof(name) __mod_##type##__##name##_device_table \ | ^~~~ >> drivers/thermal/virtual_sensor.c:267:29: error: redefinition of 'thermal_virtual_sensor_register' 267 | struct virtual_sensor_data *thermal_virtual_sensor_register( | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/thermal/virtual_sensor.c:18: drivers/thermal/virtual-sensor.h:26:1: note: previous definition of 'thermal_virtual_sensor_register' with type 'struct virtual_sensor_data *(struct device *, int, void *, const struct thermal_zone_of_device_ops *)' 26 | thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/thermal/virtual_sensor.c:295:6: error: redefinition of 'thermal_virtual_sensor_unregister' 295 | void thermal_virtual_sensor_unregister(struct device *dev, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/thermal/virtual_sensor.c:18: drivers/thermal/virtual-sensor.h:32:6: note: previous definition of 'thermal_virtual_sensor_unregister' with type 'void(struct device *, struct virtual_sensor_data *)' 32 | void thermal_virtual_sensor_unregister(struct device *dev, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/thermal/virtual_sensor.c:354:29: error: redefinition of 'devm_thermal_virtual_sensor_register' 354 | struct virtual_sensor_data *devm_thermal_virtual_sensor_register( | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/thermal/virtual_sensor.c:18: drivers/thermal/virtual-sensor.h:38:1: note: previous definition of 'devm_thermal_virtual_sensor_register' with type 'struct virtual_sensor_data *(struct device *, int, void *, const struct thermal_zone_of_device_ops *)' 38 | devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/thermal/virtual_sensor.c:389:6: error: redefinition of 'devm_thermal_virtual_sensor_unregister' 389 | void devm_thermal_virtual_sensor_unregister(struct device *dev, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/thermal/virtual_sensor.c:18: drivers/thermal/virtual-sensor.h:45:6: note: previous definition of 'devm_thermal_virtual_sensor_unregister' with type 'void(struct device *, struct virtual_sensor *)' 45 | void devm_thermal_virtual_sensor_unregister(struct device *dev, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from drivers/thermal/virtual_sensor.c:8: >> include/linux/module.h:244:21: error: '__mod_of__thermal_aggr_of_match_device_table' aliased to undefined symbol 'thermal_aggr_of_match' 244 | extern typeof(name) __mod_##type##__##name##_device_table \ | ^~~~~~ drivers/thermal/virtual_sensor.c:235:1: note: in expansion of macro 'MODULE_DEVICE_TABLE' 235 | MODULE_DEVICE_TABLE(of, thermal_aggr_of_match); | ^~~~~~~~~~~~~~~~~~~ vim +/thermal_aggr_of_match +235 drivers/thermal/virtual_sensor.c 227 228 static const struct of_device_id virtual_sensor_of_match[] = { 229 { 230 .compatible = "virtual,thermal-sensor", 231 }, 232 { 233 }, 234 }; > 235 MODULE_DEVICE_TABLE(of, thermal_aggr_of_match); 236 237 static struct platform_driver virtual_sensor = { 238 .probe = virtual_sensor_probe, 239 .remove = virtual_sensor_remove, 240 .driver = { 241 .name = "virtual-sensor", 242 .of_match_table = virtual_sensor_of_match, 243 }, 244 }; 245 246 /** 247 * thermal_virtual_sensor_register - registers a sensor that could by a virtual 248 * sensor 249 * @dev: a valid struct device pointer of a sensor device. Must contain 250 * a valid .of_node, for the sensor node. 251 * @sensor_id: a sensor identifier, in case the sensor IP has more 252 * than one sensors 253 * @data: a private pointer (owned by the caller) that will be passed 254 * back, when a temperature reading is needed. 255 * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. 256 * 257 * This function will register a thermal sensor to make it available for later 258 * usage by a virtual sensor. 259 * 260 * The thermal zone temperature is provided by the @get_temp function 261 * pointer. When called, it will have the private pointer @data back. 262 * 263 * Return: On success returns a valid struct thermal_zone_device, 264 * otherwise, it returns a corresponding ERR_PTR(). Caller must 265 * check the return value with help of IS_ERR() helper. 266 */ > 267 struct virtual_sensor_data *thermal_virtual_sensor_register( 268 struct device *dev, int sensor_id, void *data, 269 const struct thermal_zone_of_device_ops *ops) 270 { 271 struct virtual_sensor_data *sensor_data; 272 273 sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL); 274 if (!sensor_data) 275 return ERR_PTR(-ENOMEM); 276 277 sensor_data->id = sensor_id; 278 sensor_data->sensor_data = data; 279 sensor_data->ops = ops; 280 281 list_add(&sensor_data->node, &thermal_sensors); 282 283 return sensor_data; 284 } 285 EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register); 286 287 /** 288 * thermal_virtual_sensor_unregister - unregisters a sensor 289 * @dev: a valid struct device pointer of a sensor device. 290 * @sensor_data: a pointer to struct virtual_sensor_data to unregister. 291 * 292 * This function removes the sensor from the list of available thermal sensors. 293 * If the sensor is in use, then the next call to .get_temp will return -ENODEV. 294 */ > 295 void thermal_virtual_sensor_unregister(struct device *dev, 296 struct virtual_sensor_data *sensor_data) 297 { 298 struct virtual_sensor_data *temp; 299 struct virtual_sensor *sensor; 300 int i; 301 302 list_del(&sensor_data->node); 303 304 list_for_each_entry(sensor, &virtual_sensors, node) { 305 for (i = 0; i < sensor->count; i++) { 306 temp = &sensor->sensors[i]; 307 if (temp->id == sensor_data->id && 308 temp->sensor_data == sensor_data->sensor_data) { 309 temp->ops = NULL; 310 } 311 } 312 } 313 devm_kfree(dev, sensor_data); 314 } 315 EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister); 316 317 static void devm_thermal_virtual_sensor_release(struct device *dev, void *res) 318 { 319 thermal_virtual_sensor_unregister(dev, 320 *(struct virtual_sensor_data **)res); 321 } 322 323 static int devm_thermal_virtual_sensor_match(struct device *dev, void *res, 324 void *data) 325 { 326 struct virtual_sensor_data **r = res; 327 328 if (WARN_ON(!r || !*r)) 329 return 0; 330 331 return *r == data; 332 } 333 334 335 /** 336 * devm_thermal_virtual_sensor_register - Resource managed version of 337 * thermal_virtual_sensor_register() 338 * @dev: a valid struct device pointer of a sensor device. Must contain 339 * a valid .of_node, for the sensor node. 340 * @sensor_id: a sensor identifier, in case the sensor IP has more 341 * than one sensors 342 * @data: a private pointer (owned by the caller) that will be passed 343 * back, when a temperature reading is needed. 344 * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. 345 * 346 * Refer thermal_zone_of_sensor_register() for more details. 347 * 348 * Return: On success returns a valid struct virtual_sensor_data, 349 * otherwise, it returns a corresponding ERR_PTR(). Caller must 350 * check the return value with help of IS_ERR() helper. 351 * Registered virtual_sensor_data device will automatically be 352 * released when device is unbounded. 353 */ > 354 struct virtual_sensor_data *devm_thermal_virtual_sensor_register( 355 struct device *dev, int sensor_id, 356 void *data, const struct thermal_zone_of_device_ops *ops) 357 { 358 struct virtual_sensor_data **ptr, *sensor_data; 359 360 ptr = devres_alloc(devm_thermal_virtual_sensor_release, sizeof(*ptr), 361 GFP_KERNEL); 362 if (!ptr) 363 return ERR_PTR(-ENOMEM); 364 365 sensor_data = thermal_virtual_sensor_register(dev, sensor_id, data, ops); 366 if (IS_ERR(sensor_data)) { 367 devres_free(ptr); 368 return sensor_data; 369 } 370 371 *ptr = sensor_data; 372 devres_add(dev, ptr); 373 374 return sensor_data; 375 } 376 EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_register); 377 378 /** 379 * devm_thermal_virtual_sensor_unregister - Resource managed version of 380 * thermal_virtual_sensor_unregister(). 381 * @dev: Device for which resource was allocated. 382 * @sensor: a pointer to struct thermal_zone_device where the sensor is registered. 383 * 384 * This function removes the sensor from the list of sensors registered with 385 * devm_thermal_virtual_sensor_register() API. 386 * Normally this function will not need to be called and the resource 387 * management code will ensure that the resource is freed. 388 */ > 389 void devm_thermal_virtual_sensor_unregister(struct device *dev, 390 struct virtual_sensor *sensor) 391 { 392 WARN_ON(devres_release(dev, devm_thermal_virtual_sensor_release, 393 devm_thermal_virtual_sensor_match, sensor)); 394 } 395 EXPORT_SYMBOL_GPL(devm_thermal_virtual_sensor_unregister); 396 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 70094 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures 2021-09-06 19:04 ` [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures Alexandre Bailon 2021-09-07 5:14 ` kernel test robot @ 2021-09-09 22:29 ` Matthias Kaehlcke 2021-09-15 13:32 ` Alexandre Bailon 1 sibling, 1 reply; 7+ messages in thread From: Matthias Kaehlcke @ 2021-09-09 22:29 UTC (permalink / raw) To: Alexandre Bailon Cc: rui.zhang, daniel.lezcano, amitk, linux-pm, linux-kernel, ben.tseng, khilman, gpain On Mon, Sep 06, 2021 at 09:04:54PM +0200, Alexandre Bailon wrote: > This adds a virtual thermal sensor that reads temperature from > hardware sensor and return an aggregated temperature. s/hardware sensor/hardware sensors/ > Currently, this only return the max temperature. it seems this is outdated. > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > drivers/thermal/Kconfig | 8 + > drivers/thermal/Makefile | 1 + > drivers/thermal/virtual-sensor.h | 51 ++++ > drivers/thermal/virtual_sensor.c | 400 +++++++++++++++++++++++++++++++ > 4 files changed, 460 insertions(+) > create mode 100644 drivers/thermal/virtual-sensor.h > create mode 100644 drivers/thermal/virtual_sensor.c > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index 7a4ba50ba97d0..23dc903da2fc5 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 VIRTUAL_THERMAL > + tristate "Generic virtual thermal sensor driver" Not sure if 'Generic' adds much value here. > + depends on THERMAL_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..76dfa1d61bfc5 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_VIRTUAL_THERMAL) += virtual_sensor.o > diff --git a/drivers/thermal/virtual-sensor.h b/drivers/thermal/virtual-sensor.h > new file mode 100644 > index 0000000000000..e024d434856c7 > --- /dev/null > +++ b/drivers/thermal/virtual-sensor.h > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2021 BayLibre > + */ > + > +#ifndef __THERMAL_VIRTUAL_SENSOR_H__ > +#define __THERMAL_VIRTUAL_SENSOR_H__ > + > +struct virtual_sensor; > +struct virtual_sensor_data; Other types of virtual sensors could be added in the future, you might want to name these virtual_thermal_sensor(_data). Then again, these structs are of internal use only, so it's probably not super important. > + > +#ifdef CONFIG_VIRTUAL_THERMAL > +struct virtual_sensor_data * > +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, > + const struct thermal_zone_of_device_ops *ops); > +void thermal_virtual_sensor_unregister(struct device *dev, > + struct virtual_sensor_data *sensor_data); > +struct virtual_sensor_data * > +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, > + const struct thermal_zone_of_device_ops *ops); > + > +void devm_thermal_virtual_sensor_unregister(struct device *dev, > + struct virtual_sensor *sensor); > +#else > +static inline struct virtual_sensor_data * > +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, > + const struct thermal_zone_of_device_ops *ops) > +{ > + return ERR_PTR(-ENODEV); > +} > + > +void thermal_virtual_sensor_unregister(struct device *dev, > + struct virtual_sensor_data *sensor_data) > +{ > +} > + > +static inline struct virtual_sensor_data * > +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, > + const struct thermal_zone_of_device_ops *ops) > +{ > + return ERR_PTR(-ENODEV); > +} > + > +static inline > +void devm_thermal_virtual_sensor_unregister(struct device *dev, > + struct virtual_sensor *sensor) > +{ > +} > +#endif > + > +#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */ > diff --git a/drivers/thermal/virtual_sensor.c b/drivers/thermal/virtual_sensor.c > new file mode 100644 > index 0000000000000..e5bb0ef9adb39 > --- /dev/null > +++ b/drivers/thermal/virtual_sensor.c > @@ -0,0 +1,400 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2021 BayLibre > + */ > + > +#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> > + > +#include <dt-bindings/thermal/virtual-sensor.h> > + > +#include "virtual-sensor.h" > + > +struct virtual_sensor_data { This struct (typically) corresponds to an actual sensor, maybe name it 'thermal_sensor_data' instead? > + struct list_head node; > + > + /* sensor interface */ > + int id; > + void *sensor_data; > + const struct thermal_zone_of_device_ops *ops; > +}; > + > +struct virtual_sensor { > + int count; > + struct virtual_sensor_data *sensors; > + struct thermal_zone_device *tzd; > + > + struct list_head node; > +}; > + > +static LIST_HEAD(thermal_sensors); > +static LIST_HEAD(virtual_sensors); > + > +static int virtual_sensor_get_temp_max(void *data, int *temperature) > +{ > + struct virtual_sensor *sensor = data; > + int max_temp = INT_MIN; > + int temp; > + int i; > + > + for (i = 0; i < sensor->count; i++) { > + struct virtual_sensor_data *hw_sensor; > + > + hw_sensor = &sensor->sensors[i]; > + if (!hw_sensor->ops) > + return -ENODEV; > + > + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); > + max_temp = max(max_temp, temp); > + } > + > + *temperature = max_temp; > + > + return 0; > +} > + > +static const struct thermal_zone_of_device_ops virtual_sensor_max_ops = { > + .get_temp = virtual_sensor_get_temp_max, > +}; > + > +static int virtual_sensor_get_temp_min(void *data, int *temperature) > +{ > + struct virtual_sensor *sensor = data; > + int min_temp = INT_MAX; > + int temp; > + int i; > + > + for (i = 0; i < sensor->count; i++) { > + struct virtual_sensor_data *hw_sensor; > + > + hw_sensor = &sensor->sensors[i]; > + if (!hw_sensor->ops) > + return -ENODEV; > + > + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); > + min_temp = min(min_temp, temp); > + } > + > + *temperature = min_temp; > + > + return 0; > +} > + > +static const struct thermal_zone_of_device_ops virtual_sensor_min_ops = { > + .get_temp = virtual_sensor_get_temp_min, > +}; > + > +static int do_avg(int val1, int val2) > +{ > + return ((val1) / 2) + ((val2) / 2) + (((val1) % 2 + (val2) % 2) / 2); > +} > + > +static int virtual_sensor_get_temp_avg(void *data, int *temperature) > +{ > + struct virtual_sensor *sensor = data; > + int avg_temp = 0; > + int temp; > + int i; > + > + for (i = 0; i < sensor->count; i++) { > + struct virtual_sensor_data *hw_sensor; > + > + hw_sensor = &sensor->sensors[i]; > + if (!hw_sensor->ops) > + return -ENODEV; > + > + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); > + avg_temp = do_avg(avg_temp, temp); > + } > + > + *temperature = avg_temp; > + > + return 0; > +} _get_temp_min(), _get_temp_max() and _get_temp_avg() have the same structure, the only differences is the aggregator function and the initialization value. If you wanted to save a few lines of code you could have a meta-function that receives the initialization value and a pointer of the aggregator function. The functions are relatively short though, so I wouldn't claim that it would be a huge improvement, one could also argue that the code is easier to follow as is. > + > +static const struct thermal_zone_of_device_ops virtual_sensor_avg_ops = { > + .get_temp = virtual_sensor_get_temp_avg, > +}; > + > +static int register_virtual_sensor(struct virtual_sensor *sensor, > + struct of_phandle_args args, > + int index) Does this really register a virtual sensor? IIUC the registered sensor is (typically) an actual sensor, which is used by a virtual sensor. Shouldn't it be something like 'register_thermal_sensor' or 'virtual_sensor_add_sensor'? > +{ > + struct virtual_sensor_data *sensor_data; > + int id; > + > + list_for_each_entry(sensor_data, &thermal_sensors, node) { > + id = args.args_count ? args.args[0] : 0; > + if (sensor_data->id == id) { > + memcpy(&sensor->sensors[index], sensor_data, > + sizeof(*sensor_data)); > + return 0; > + } > + } > + > + return -ENODEV; > +} > + > +static int virtual_sensor_probe(struct platform_device *pdev) > +{ > + const struct thermal_zone_of_device_ops *ops; > + struct virtual_sensor *sensor; > + struct device *dev = &pdev->dev; > + struct of_phandle_args args; > + u32 type; > + int ret; > + int i; > + > + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > + if (!sensor) > + return -ENOMEM; > + > + sensor->count = of_count_phandle_with_args(dev->of_node, > + "thermal-sensors", > + "#thermal-sensor-cells"); > + if (sensor->count <= 0) > + return -EINVAL; > + > + sensor->sensors = devm_kmalloc_array(dev, sensor->count, > + sizeof(*sensor->sensors), > + GFP_KERNEL); > + if (!sensor->sensors) > + return -ENOMEM; > + > + for (i = 0; i < sensor->count; i++) { > + ret = of_parse_phandle_with_args(dev->of_node, > + "thermal-sensors", > + "#thermal-sensor-cells", > + i, > + &args); > + if (ret) > + return ret; > + > + ret = register_virtual_sensor(sensor, args, i); > + if (ret) > + return ret; > + } > + > + ret = of_property_read_u32(dev->of_node, "type", &type); More a question for the binding, butthis should probably be something more specific than 'type', like 'aggregation-function'. > + if (ret) > + return ret; > + > + switch (type) { > + case VIRTUAL_SENSOR_MAX: > + ops = &virtual_sensor_max_ops; > + break; > + case VIRTUAL_SENSOR_MIN: > + ops = &virtual_sensor_min_ops; > + break; > + case VIRTUAL_SENSOR_AVG: > + ops = &virtual_sensor_avg_ops; > + break; > + default: > + return -EINVAL; > + } > + > + sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor, ops); > + if (IS_ERR(sensor->tzd)) > + return PTR_ERR(sensor->tzd); > + > + platform_set_drvdata(pdev, sensor); > + list_add(&sensor->node, &virtual_sensors); If you also added the sensor to 'thermal_sensors' you could support virtual sensors using virtual sensors, though it's not clear how useful that would be in practice and it could raise issues with the initialization order. > + > + return 0; > +} > + > +static int virtual_sensor_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct virtual_sensor *sensor; > + > + sensor = platform_get_drvdata(pdev); > + list_del(&sensor->node); > + > + devm_thermal_zone_of_sensor_unregister(dev, sensor->tzd); > + devm_kfree(dev, sensor->sensors); > + devm_kfree(dev, sensor); Are the above 3 statements really needed, shouldn't devm_* handle that automagically? > + > + return 0; > +} > + > +static const struct of_device_id virtual_sensor_of_match[] = { > + { > + .compatible = "virtual,thermal-sensor", > + }, > + { > + }, > +}; > +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match); > + > +static struct platform_driver virtual_sensor = { > + .probe = virtual_sensor_probe, > + .remove = virtual_sensor_remove, > + .driver = { > + .name = "virtual-sensor", I suggest to change it to 'virtual-thermal-sensor' since there might be other types of virtual sensors. > + .of_match_table = virtual_sensor_of_match, > + }, > +}; > + > +/** > + * thermal_virtual_sensor_register - registers a sensor that could by a virtual s/by/be/ > + * sensor > + * @dev: a valid struct device pointer of a sensor device. Must contain > + * a valid .of_node, for the sensor node. > + * @sensor_id: a sensor identifier, in case the sensor IP has more > + * than one sensors s/sensors/sensor/ > + * @data: a private pointer (owned by the caller) that will be passed > + * back, when a temperature reading is needed. > + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. > + * > + * This function will register a thermal sensor to make it available for later > + * usage by a virtual sensor. > + * > + * The thermal zone temperature is provided by the @get_temp function > + * pointer. When called, it will have the private pointer @data back. > + * > + * Return: On success returns a valid struct thermal_zone_device, > + * otherwise, it returns a corresponding ERR_PTR(). Caller must > + * check the return value with help of IS_ERR() helper. > + */ > +struct virtual_sensor_data *thermal_virtual_sensor_register( > + struct device *dev, int sensor_id, void *data, > + const struct thermal_zone_of_device_ops *ops) > +{ > + struct virtual_sensor_data *sensor_data; > + > + sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL); > + if (!sensor_data) > + return ERR_PTR(-ENOMEM); > + > + sensor_data->id = sensor_id; > + sensor_data->sensor_data = data; > + sensor_data->ops = ops; > + > + list_add(&sensor_data->node, &thermal_sensors); > + > + return sensor_data; > +} > +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register); > + > +/** > + * thermal_virtual_sensor_unregister - unregisters a sensor > + * @dev: a valid struct device pointer of a sensor device. > + * @sensor_data: a pointer to struct virtual_sensor_data to unregister. > + * > + * This function removes the sensor from the list of available thermal sensors. > + * If the sensor is in use, then the next call to .get_temp will return -ENODEV. > + */ > +void thermal_virtual_sensor_unregister(struct device *dev, > + struct virtual_sensor_data *sensor_data) > +{ > + struct virtual_sensor_data *temp; 'temp' might not be the best name in this context, since it's associated with temperature. Maybe name it 'sd'? > + struct virtual_sensor *sensor; > + int i; > + > + list_del(&sensor_data->node); > + > + list_for_each_entry(sensor, &virtual_sensors, node) { > + for (i = 0; i < sensor->count; i++) { > + temp = &sensor->sensors[i]; > + if (temp->id == sensor_data->id && > + temp->sensor_data == sensor_data->sensor_data) { > + temp->ops = NULL; > + } > + } > + } > + devm_kfree(dev, sensor_data); Does it actually make sense to allocate the memory with devm_kzalloc() if it is explicitly freed here? > +} > +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister); > + > +static void devm_thermal_virtual_sensor_release(struct device *dev, void *res) > +{ > + thermal_virtual_sensor_unregister(dev, > + *(struct virtual_sensor_data **)res); > +} > + > +static int devm_thermal_virtual_sensor_match(struct device *dev, void *res, > + void *data) > +{ > + struct virtual_sensor_data **r = res; > + > + if (WARN_ON(!r || !*r)) > + return 0; > + > + return *r == data; > +} > + > + delete one of the empty lines > +/** > + * devm_thermal_virtual_sensor_register - Resource managed version of > + * thermal_virtual_sensor_register() > + * @dev: a valid struct device pointer of a sensor device. Must contain > + * a valid .of_node, for the sensor node. > + * @sensor_id: a sensor identifier, in case the sensor IP has more > + * than one sensors s/sensors/sensor/ > + * @data: a private pointer (owned by the caller) that will be passed > + * back, when a temperature reading is needed. > + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. > + * > + * Refer thermal_zone_of_sensor_register() for more details. > + * > + * Return: On success returns a valid struct virtual_sensor_data, > + * otherwise, it returns a corresponding ERR_PTR(). Caller must > + * check the return value with help of IS_ERR() helper. > + * Registered virtual_sensor_data device will automatically be > + * released when device is unbounded. s/unbounded/unbound/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures 2021-09-09 22:29 ` Matthias Kaehlcke @ 2021-09-15 13:32 ` Alexandre Bailon 0 siblings, 0 replies; 7+ messages in thread From: Alexandre Bailon @ 2021-09-15 13:32 UTC (permalink / raw) To: Matthias Kaehlcke Cc: rui.zhang, daniel.lezcano, amitk, linux-pm, linux-kernel, ben.tseng, khilman, gpain Hi Matthias, On 9/10/21 12:29 AM, Matthias Kaehlcke wrote: > On Mon, Sep 06, 2021 at 09:04:54PM +0200, Alexandre Bailon wrote: >> This adds a virtual thermal sensor that reads temperature from >> hardware sensor and return an aggregated temperature. > s/hardware sensor/hardware sensors/ > >> Currently, this only return the max temperature. > it seems this is outdated. > >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> drivers/thermal/Kconfig | 8 + >> drivers/thermal/Makefile | 1 + >> drivers/thermal/virtual-sensor.h | 51 ++++ >> drivers/thermal/virtual_sensor.c | 400 +++++++++++++++++++++++++++++++ >> 4 files changed, 460 insertions(+) >> create mode 100644 drivers/thermal/virtual-sensor.h >> create mode 100644 drivers/thermal/virtual_sensor.c >> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig >> index 7a4ba50ba97d0..23dc903da2fc5 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 VIRTUAL_THERMAL >> + tristate "Generic virtual thermal sensor driver" > Not sure if 'Generic' adds much value here. > >> + depends on THERMAL_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..76dfa1d61bfc5 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_VIRTUAL_THERMAL) += virtual_sensor.o >> diff --git a/drivers/thermal/virtual-sensor.h b/drivers/thermal/virtual-sensor.h >> new file mode 100644 >> index 0000000000000..e024d434856c7 >> --- /dev/null >> +++ b/drivers/thermal/virtual-sensor.h >> @@ -0,0 +1,51 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (c) 2021 BayLibre >> + */ >> + >> +#ifndef __THERMAL_VIRTUAL_SENSOR_H__ >> +#define __THERMAL_VIRTUAL_SENSOR_H__ >> + >> +struct virtual_sensor; >> +struct virtual_sensor_data; > Other types of virtual sensors could be added in the future, you might > want to name these virtual_thermal_sensor(_data). Then again, these > structs are of internal use only, so it's probably not super important. You are right. This might become confusing at some point. I will rename them. > >> + >> +#ifdef CONFIG_VIRTUAL_THERMAL >> +struct virtual_sensor_data * >> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, >> + const struct thermal_zone_of_device_ops *ops); >> +void thermal_virtual_sensor_unregister(struct device *dev, >> + struct virtual_sensor_data *sensor_data); >> +struct virtual_sensor_data * >> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, >> + const struct thermal_zone_of_device_ops *ops); >> + >> +void devm_thermal_virtual_sensor_unregister(struct device *dev, >> + struct virtual_sensor *sensor); >> +#else >> +static inline struct virtual_sensor_data * >> +thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, >> + const struct thermal_zone_of_device_ops *ops) >> +{ >> + return ERR_PTR(-ENODEV); >> +} >> + >> +void thermal_virtual_sensor_unregister(struct device *dev, >> + struct virtual_sensor_data *sensor_data) >> +{ >> +} >> + >> +static inline struct virtual_sensor_data * >> +devm_thermal_virtual_sensor_register(struct device *dev, int sensor_id, void *data, >> + const struct thermal_zone_of_device_ops *ops) >> +{ >> + return ERR_PTR(-ENODEV); >> +} >> + >> +static inline >> +void devm_thermal_virtual_sensor_unregister(struct device *dev, >> + struct virtual_sensor *sensor) >> +{ >> +} >> +#endif >> + >> +#endif /* __THERMAL_VIRTUAL_SENSOR_H__ */ >> diff --git a/drivers/thermal/virtual_sensor.c b/drivers/thermal/virtual_sensor.c >> new file mode 100644 >> index 0000000000000..e5bb0ef9adb39 >> --- /dev/null >> +++ b/drivers/thermal/virtual_sensor.c >> @@ -0,0 +1,400 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2021 BayLibre >> + */ >> + >> +#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> >> + >> +#include <dt-bindings/thermal/virtual-sensor.h> >> + >> +#include "virtual-sensor.h" >> + >> +struct virtual_sensor_data { > This struct (typically) corresponds to an actual sensor, maybe name it > 'thermal_sensor_data' instead? > >> + struct list_head node; >> + >> + /* sensor interface */ >> + int id; >> + void *sensor_data; >> + const struct thermal_zone_of_device_ops *ops; >> +}; >> + >> +struct virtual_sensor { >> + int count; >> + struct virtual_sensor_data *sensors; >> + struct thermal_zone_device *tzd; >> + >> + struct list_head node; >> +}; >> + >> +static LIST_HEAD(thermal_sensors); >> +static LIST_HEAD(virtual_sensors); >> + >> +static int virtual_sensor_get_temp_max(void *data, int *temperature) >> +{ >> + struct virtual_sensor *sensor = data; >> + int max_temp = INT_MIN; >> + int temp; >> + int i; >> + >> + for (i = 0; i < sensor->count; i++) { >> + struct virtual_sensor_data *hw_sensor; >> + >> + hw_sensor = &sensor->sensors[i]; >> + if (!hw_sensor->ops) >> + return -ENODEV; >> + >> + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); >> + max_temp = max(max_temp, temp); >> + } >> + >> + *temperature = max_temp; >> + >> + return 0; >> +} >> + >> +static const struct thermal_zone_of_device_ops virtual_sensor_max_ops = { >> + .get_temp = virtual_sensor_get_temp_max, >> +}; >> + >> +static int virtual_sensor_get_temp_min(void *data, int *temperature) >> +{ >> + struct virtual_sensor *sensor = data; >> + int min_temp = INT_MAX; >> + int temp; >> + int i; >> + >> + for (i = 0; i < sensor->count; i++) { >> + struct virtual_sensor_data *hw_sensor; >> + >> + hw_sensor = &sensor->sensors[i]; >> + if (!hw_sensor->ops) >> + return -ENODEV; >> + >> + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); >> + min_temp = min(min_temp, temp); >> + } >> + >> + *temperature = min_temp; >> + >> + return 0; >> +} >> + >> +static const struct thermal_zone_of_device_ops virtual_sensor_min_ops = { >> + .get_temp = virtual_sensor_get_temp_min, >> +}; >> + >> +static int do_avg(int val1, int val2) >> +{ >> + return ((val1) / 2) + ((val2) / 2) + (((val1) % 2 + (val2) % 2) / 2); >> +} >> + >> +static int virtual_sensor_get_temp_avg(void *data, int *temperature) >> +{ >> + struct virtual_sensor *sensor = data; >> + int avg_temp = 0; >> + int temp; >> + int i; >> + >> + for (i = 0; i < sensor->count; i++) { >> + struct virtual_sensor_data *hw_sensor; >> + >> + hw_sensor = &sensor->sensors[i]; >> + if (!hw_sensor->ops) >> + return -ENODEV; >> + >> + hw_sensor->ops->get_temp(hw_sensor->sensor_data, &temp); >> + avg_temp = do_avg(avg_temp, temp); >> + } >> + >> + *temperature = avg_temp; >> + >> + return 0; >> +} > _get_temp_min(), _get_temp_max() and _get_temp_avg() have the same > structure, the only differences is the aggregator function and the > initialization value. If you wanted to save a few lines of code you > could have a meta-function that receives the initialization value and > a pointer of the aggregator function. > > The functions are relatively short though, so I wouldn't claim that > it would be a huge improvement, one could also argue that the code is > easier to follow as is. My intent was to make possible adding more complex functions. I will factorize the code, and if later there are some use cases, we will made the update to support more complex functions. > >> + >> +static const struct thermal_zone_of_device_ops virtual_sensor_avg_ops = { >> + .get_temp = virtual_sensor_get_temp_avg, >> +}; >> + >> +static int register_virtual_sensor(struct virtual_sensor *sensor, >> + struct of_phandle_args args, >> + int index) > Does this really register a virtual sensor? IIUC the registered sensor is > (typically) an actual sensor, which is used by a virtual sensor. > > Shouldn't it be something like 'register_thermal_sensor' or > 'virtual_sensor_add_sensor'? You are right, virtual_sensor_add_sensor sounds a lot better. > >> +{ >> + struct virtual_sensor_data *sensor_data; >> + int id; >> + >> + list_for_each_entry(sensor_data, &thermal_sensors, node) { >> + id = args.args_count ? args.args[0] : 0; >> + if (sensor_data->id == id) { >> + memcpy(&sensor->sensors[index], sensor_data, >> + sizeof(*sensor_data)); >> + return 0; >> + } >> + } >> + >> + return -ENODEV; >> +} >> + >> +static int virtual_sensor_probe(struct platform_device *pdev) >> +{ >> + const struct thermal_zone_of_device_ops *ops; >> + struct virtual_sensor *sensor; >> + struct device *dev = &pdev->dev; >> + struct of_phandle_args args; >> + u32 type; >> + int ret; >> + int i; >> + >> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >> + if (!sensor) >> + return -ENOMEM; >> + >> + sensor->count = of_count_phandle_with_args(dev->of_node, >> + "thermal-sensors", >> + "#thermal-sensor-cells"); >> + if (sensor->count <= 0) >> + return -EINVAL; >> + >> + sensor->sensors = devm_kmalloc_array(dev, sensor->count, >> + sizeof(*sensor->sensors), >> + GFP_KERNEL); >> + if (!sensor->sensors) >> + return -ENOMEM; >> + >> + for (i = 0; i < sensor->count; i++) { >> + ret = of_parse_phandle_with_args(dev->of_node, >> + "thermal-sensors", >> + "#thermal-sensor-cells", >> + i, >> + &args); >> + if (ret) >> + return ret; >> + >> + ret = register_virtual_sensor(sensor, args, i); >> + if (ret) >> + return ret; >> + } >> + >> + ret = of_property_read_u32(dev->of_node, "type", &type); > More a question for the binding, butthis should probably be something > more specific than 'type', like 'aggregation-function'. > >> + if (ret) >> + return ret; >> + >> + switch (type) { >> + case VIRTUAL_SENSOR_MAX: >> + ops = &virtual_sensor_max_ops; >> + break; >> + case VIRTUAL_SENSOR_MIN: >> + ops = &virtual_sensor_min_ops; >> + break; >> + case VIRTUAL_SENSOR_AVG: >> + ops = &virtual_sensor_avg_ops; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + sensor->tzd = devm_thermal_zone_of_sensor_register(dev, 0, sensor, ops); >> + if (IS_ERR(sensor->tzd)) >> + return PTR_ERR(sensor->tzd); >> + >> + platform_set_drvdata(pdev, sensor); >> + list_add(&sensor->node, &virtual_sensors); > If you also added the sensor to 'thermal_sensors' you could support virtual > sensors using virtual sensors, though it's not clear how useful that would be > in practice and it could raise issues with the initialization order. Unless if we found some use cases, I think we should not do that. As you mentioned, this would add some complexity to the initialization to handle it. > >> + >> + return 0; >> +} >> + >> +static int virtual_sensor_remove(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct virtual_sensor *sensor; >> + >> + sensor = platform_get_drvdata(pdev); >> + list_del(&sensor->node); >> + >> + devm_thermal_zone_of_sensor_unregister(dev, sensor->tzd); >> + devm_kfree(dev, sensor->sensors); >> + devm_kfree(dev, sensor); > Are the above 3 statements really needed, shouldn't devm_* handle that > automagically? No, I will remove them. > >> + >> + return 0; >> +} >> + >> +static const struct of_device_id virtual_sensor_of_match[] = { >> + { >> + .compatible = "virtual,thermal-sensor", >> + }, >> + { >> + }, >> +}; >> +MODULE_DEVICE_TABLE(of, thermal_aggr_of_match); >> + >> +static struct platform_driver virtual_sensor = { >> + .probe = virtual_sensor_probe, >> + .remove = virtual_sensor_remove, >> + .driver = { >> + .name = "virtual-sensor", > I suggest to change it to 'virtual-thermal-sensor' since there might be > other types of virtual sensors. > >> + .of_match_table = virtual_sensor_of_match, >> + }, >> +}; >> + >> +/** >> + * thermal_virtual_sensor_register - registers a sensor that could by a virtual > s/by/be/ > >> + * sensor >> + * @dev: a valid struct device pointer of a sensor device. Must contain >> + * a valid .of_node, for the sensor node. >> + * @sensor_id: a sensor identifier, in case the sensor IP has more >> + * than one sensors > s/sensors/sensor/ > >> + * @data: a private pointer (owned by the caller) that will be passed >> + * back, when a temperature reading is needed. >> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. >> + * >> + * This function will register a thermal sensor to make it available for later >> + * usage by a virtual sensor. >> + * >> + * The thermal zone temperature is provided by the @get_temp function >> + * pointer. When called, it will have the private pointer @data back. >> + * >> + * Return: On success returns a valid struct thermal_zone_device, >> + * otherwise, it returns a corresponding ERR_PTR(). Caller must >> + * check the return value with help of IS_ERR() helper. >> + */ >> +struct virtual_sensor_data *thermal_virtual_sensor_register( >> + struct device *dev, int sensor_id, void *data, >> + const struct thermal_zone_of_device_ops *ops) >> +{ >> + struct virtual_sensor_data *sensor_data; >> + >> + sensor_data = devm_kzalloc(dev, sizeof(*sensor_data), GFP_KERNEL); >> + if (!sensor_data) >> + return ERR_PTR(-ENOMEM); >> + >> + sensor_data->id = sensor_id; >> + sensor_data->sensor_data = data; >> + sensor_data->ops = ops; >> + >> + list_add(&sensor_data->node, &thermal_sensors); >> + >> + return sensor_data; >> +} >> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_register); >> + >> +/** >> + * thermal_virtual_sensor_unregister - unregisters a sensor >> + * @dev: a valid struct device pointer of a sensor device. >> + * @sensor_data: a pointer to struct virtual_sensor_data to unregister. >> + * >> + * This function removes the sensor from the list of available thermal sensors. >> + * If the sensor is in use, then the next call to .get_temp will return -ENODEV. >> + */ >> +void thermal_virtual_sensor_unregister(struct device *dev, >> + struct virtual_sensor_data *sensor_data) >> +{ >> + struct virtual_sensor_data *temp; > 'temp' might not be the best name in this context, since it's associated with > temperature. Maybe name it 'sd'? > >> + struct virtual_sensor *sensor; >> + int i; >> + >> + list_del(&sensor_data->node); >> + >> + list_for_each_entry(sensor, &virtual_sensors, node) { >> + for (i = 0; i < sensor->count; i++) { >> + temp = &sensor->sensors[i]; >> + if (temp->id == sensor_data->id && >> + temp->sensor_data == sensor_data->sensor_data) { >> + temp->ops = NULL; >> + } >> + } >> + } >> + devm_kfree(dev, sensor_data); > Does it actually make sense to allocate the memory with devm_kzalloc() if > it is explicitly freed here? No, I should not call devm_kfree here. I will remove it. Thanks, Alexandre > >> +} >> +EXPORT_SYMBOL_GPL(thermal_virtual_sensor_unregister); >> + >> +static void devm_thermal_virtual_sensor_release(struct device *dev, void *res) >> +{ >> + thermal_virtual_sensor_unregister(dev, >> + *(struct virtual_sensor_data **)res); >> +} >> + >> +static int devm_thermal_virtual_sensor_match(struct device *dev, void *res, >> + void *data) >> +{ >> + struct virtual_sensor_data **r = res; >> + >> + if (WARN_ON(!r || !*r)) >> + return 0; >> + >> + return *r == data; >> +} >> + >> + > delete one of the empty lines > >> +/** >> + * devm_thermal_virtual_sensor_register - Resource managed version of >> + * thermal_virtual_sensor_register() >> + * @dev: a valid struct device pointer of a sensor device. Must contain >> + * a valid .of_node, for the sensor node. >> + * @sensor_id: a sensor identifier, in case the sensor IP has more >> + * than one sensors > s/sensors/sensor/ > >> + * @data: a private pointer (owned by the caller) that will be passed >> + * back, when a temperature reading is needed. >> + * @ops: struct thermal_zone_of_device_ops *. Must contain at least .get_temp. >> + * >> + * Refer thermal_zone_of_sensor_register() for more details. >> + * >> + * Return: On success returns a valid struct virtual_sensor_data, >> + * otherwise, it returns a corresponding ERR_PTR(). Caller must >> + * check the return value with help of IS_ERR() helper. >> + * Registered virtual_sensor_data device will automatically be >> + * released when device is unbounded. > s/unbounded/unbound/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-07 15:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-06 19:04 [PATCH 0/2] Add a generic virtual thermal sensor Alexandre Bailon 2021-09-06 19:04 ` [PATCH 1/2] dt-bindings: Add bindings for the " Alexandre Bailon 2021-10-07 15:56 ` Rafael J. Wysocki 2021-09-06 19:04 ` [PATCH 2/2] thermal: add a virtual sensor to aggregate temperatures Alexandre Bailon 2021-09-07 5:14 ` kernel test robot 2021-09-09 22:29 ` Matthias Kaehlcke 2021-09-15 13:32 ` Alexandre Bailon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).