hwmon: (adt7470) Add thermal framework support
Subsystems (2)
cover hwmon: (adt7470) Add thermal framework support
The ADT7470 is a temperature monitor and PWM fan controller capable of monitoring up to 10 external temperature sensors and controlling up to 4 PWM outputs.
Currently, the driver exposes these sensors and PWM controls solely via the standard hwmon sysfs interface. This patch series integrates the ADT7470 driver with the kernel's thermal subsystem, allowing its temperature sensors to be referenced by Device Tree thermal zones.
To avoid saturating the I2C bus during frequent polling by the thermal core, the thermal callbacks are designed to read from the driver's internal cache.
Patch 1 adds the YAML device tree binding documentation, defining the necessary #cooling-cells and #thermal-sensor-cells properties. Patch 2 performs a minor refactoring to introduce the ADT7470_PWM_MAX macro, replacing hardcoded values.
Patch 3 registers the 4 PWM channels as optional thermal cooling devices.
Patch 4 registers the external temperature sensors as optional thermal zones.
---
Luiz Angelo Daros de Luca (4):
dt-bindings: hwmon: add binding for adi,adt7470
hwmon: (adt7470) Add ADT7470_PWM_MAX macro
hwmon: (adt7470) Add thermal cooling device support
hwmon: (adt7470) Add thermal zone sensor support
.../devicetree/bindings/hwmon/adi,adt7470.yaml | 53 ++++++ drivers/hwmon/adt7470.c | 180 ++++++++++++++++++++- 2 files changed, 230 insertions(+), 3 deletions(-)
---
base-commit: ca078d004cf58137bcf8cb24a8b271397431ba58
change-id: 20260716-adt7470_thermalzone-59a102278d72
Best regards,
Signature · 1 line
Luiz Angelo Daros de Luca <[email protected]>
PATCH 1/4 dt-bindings: hwmon: add binding for adi,adt7470
The Analog Devices ADT7470 is a temperature monitor and PWM fan controller. It supports up to 10 external temperature sensors and up to 4 PWM fan outputs.
Add the YAML device tree binding documentation for it. This includes support for the thermal framework by defining the "#cooling-cells" and "#thermal-sensor-cells" properties.
--- .../devicetree/bindings/hwmon/adi,adt7470.yaml | 53 ++++++++++++++++++++++ 1 file changed, 53 insertions(+)
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +53 -0
@@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/adi,adt7470.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices ADT7470 hwmon sensor + +maintainers: + - Luiz Angelo Daros de Luca <[email protected]> + +description: | + The ADT7470 is a temperature monitor and multiple PWM outputs. + It supports monitoring up to 10 external temperature sensors and controlling + up to four fans. + +properties: + compatible: + const: adi,adt7470 + + reg: + maxItems: 1 + + "#cooling-cells": + const: 2 + description: | + Cooling device exposing the 4 PWM channels. + + "#thermal-sensor-cells": + const: 1 + description: | + Number of cells required to uniquely identify the temperature sensors. + Valid index values are 0 to 9, corresponding to temp1 through temp10. + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + hwmon@2f { + compatible = "adi,adt7470"; + reg = <0x2f>; + #cooling-cells = <2>; + #thermal-sensor-cells = <1>; + }; + };
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 1/4] dt-bindings: hwmon: add binding for adi,adt7470
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The dt-binding for the multi-channel ADT7470 PWM fan controller omits the mandatory `#pwm-cells`…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The dt-binding for the multi-channel ADT7470 PWM fan controller omits the mandatory #pwm-cells property for a PWM output provider and incorrectly models the channels as a single cooling device. --
--- Patch [1]: [PATCH 1/4] dt-bindings: hwmon: add binding for adi,adt7470 --- commit 5b60ee676010a73eb2ebad187c402e61ddc230ad
Author: Luiz Angelo Daros de Luca <[email protected]>
dt-bindings: hwmon: add binding for adi,adt7470
This commit adds the YAML device tree binding documentation for the Analog Devices ADT7470 temperature monitor and PWM fan controller. It includes support for the thermal framework by defining cooling and thermal sensor properties.
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +0
-0
[ ... ]
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 27
Quoted 11 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +11
-0+properties: + compatible: + const: adi,adt7470 + + reg: + maxItems: 1 + + "#cooling-cells": + const: 2 + description: | + Cooling device exposing the 4 PWM channels.
[Severity: Medium]
Does defining #cooling-cells directly on the parent node force all 4 PWM channels to be ganged together into a single cooling device? A 2-cell cooling specifier lacks a cell for the channel index, which might permanently prevent the thermal framework from addressing the individual fans independently. Would it be more appropriate to model this using explicit fan sub-nodes or PWM consumers?
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 37
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +10
-0+ + "#thermal-sensor-cells": + const: 1 + description: | + Number of cells required to uniquely identify the temperature sensors. + Valid index values are 0 to 9, corresponding to temp1 through temp10. + +required: + - compatible + - reg
[Severity: Medium]
Is the #pwm-cells property missing from this required list? Since the ADT7470 acts as a multi-channel PWM provider, subsystem guidelines suggest that PWM output providers need to include #pwm-cells in both the properties and the required list. Omitting this allows incomplete device trees to pass schema validation.
Quoted 2 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +2
-0+ +additionalProperties: false
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH 1/4] dt-bindings: hwmon: add binding for adi,adt7470
In reply to Luiz Angelo Daros de LucaDocumentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 27
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +4
-0+ "#cooling-cells": + const: 2 + description: | + Cooling device exposing the 4 PWM channels.
Since "thermal/of: Support cooling device ID in cooling-spec", I could be 2 or 3. I don't know if I should use enum [2, 3] or just go directly to the new const: 3.
My test device has only the 0 channel in use and 2 is fine for now.
Sashiko also asked to require #pwm-cells, but it would mean to implement a parser for that too. I don't know if it is worth it but I can do it if needed.
Regards,
Luiz
Rob Herring
maintainerRe: [PATCH 1/4] dt-bindings: hwmon: add binding for adi,adt7470
Quoted 5 lines · Luiz Angelo Daros de Luca
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 27
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +4
-0+ "#cooling-cells": + const: 2 + description: | + Cooling device exposing the 4 PWM channels.
Quoted 8 lines
Since "thermal/of: Support cooling device ID in cooling-spec", I could be 2 or 3. I don't know if I should use enum [2, 3] or just go directly to the new const: 3.
My test device has only the 0 channel in use and 2 is fine for now.Sashiko also asked to require #pwm-cells, but it would mean to implement a parser for that too. I don't know if it is worth it but I can do it if needed.
What does implement a parser mean? You can define the property now, but support it in a driver later.
Rob
Guenter Roeck
maintainerRe: [PATCH 1/4] dt-bindings: hwmon: add binding for adi,adt7470
Quoted 11 lines · Rob Herring
Quoted 10 lines · Luiz Angelo Daros de Luca
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 27
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +4
-0+ "#cooling-cells": + const: 2 + description: | + Cooling device exposing the 4 PWM channels.Since "thermal/of: Support cooling device ID in cooling-spec", I could be 2 or 3. I don't know if I should use enum [2, 3] or just go directly to the new const: 3.
My test device has only the 0 channel in use and 2 is fine for now.
Quoted 7 lines
Quoted 3 lines
Sashiko also asked to require #pwm-cells, but it would mean to implement a parser for that too. I don't know if it is worth it but I can do it if needed.
What does implement a parser mean? You can define the property now, but support it in a driver later.
FWIW, several hardware monitoring drivers already use "pwms" and "#pwm-cells". The code doesn't seem overly complex. It is commonly used to configure pwm frequency and polarity, and sometimes to set parameters such as the default duty cycle. The code has similar complexity as reading other device specific properties.
$ git grep pwm-cells drivers/hwmon/ drivers/hwmon/adt7475.c: ret = fwnode_property_get_reference_args(fwnode, "pwms", "#pwm-cells", 0, 0, &rargs); drivers/hwmon/amc6821.c: if (of_parse_phandle_with_args(fan_np, "pwms", "#pwm-cells", 0, &args)) drivers/hwmon/emc2305.c: ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells", 0, &args); drivers/hwmon/nct7363.c: ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
Guenter
Rob Herring
maintainerRe: [PATCH 1/4] dt-bindings: hwmon: add binding for adi,adt7470
In reply to Luiz Angelo Daros de LucaQuoted 21 lines · Luiz Angelo Daros de Luca
The Analog Devices ADT7470 is a temperature monitor and PWM fan controller. It supports up to 10 external temperature sensors and up to 4 PWM fan outputs.
Add the YAML device tree binding documentation for it. This includes support for the thermal framework by defining the "#cooling-cells" and "#thermal-sensor-cells" properties.
Signed-off-by Luiz Angelo Daros de Luca <[email protected]>--- .../devicetree/bindings/hwmon/adi,adt7470.yaml | 53 ++++++++++++++++++++++ 1 file changed, 53 insertions(+)Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +2
-0@@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 12
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +10
-0+--- +$id: http://devicetree.org/schemas/hwmon/adi,adt7470.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices ADT7470 hwmon sensor + +maintainers: + - Luiz Angelo Daros de Luca <[email protected]> + +description: |
Don't need '|' if no formatting, but if the below is paragraphs, then you need '>' and a blank line. But it reads as 1 paragraph to me. In that case, your word wrap is off.
Quoted 4 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +4
-0+ The ADT7470 is a temperature monitor and multiple PWM outputs. + It supports monitoring up to 10 external temperature sensors and controlling + up to four fans. +
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 26
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +10
-0+properties: + compatible: + const: adi,adt7470 + + reg: + maxItems: 1 + + "#cooling-cells": + const: 2 + description: |
Don't need '|'.
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml line 31
Quoted 5 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +5
-0+ Cooling device exposing the 4 PWM channels. + + "#thermal-sensor-cells": + const: 1 + description: |
Don't need '|'.
Quoted 26 lines
Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml new file +22
-0+ Number of cells required to uniquely identify the temperature sensors. + Valid index values are 0 to 9, corresponding to temp1 through temp10. + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + hwmon@2f { + compatible = "adi,adt7470"; + reg = <0x2f>; + #cooling-cells = <2>; + #thermal-sensor-cells = <1>; + }; + };Signature · 2 lines
-- 2.55.0
PATCH 2/4 hwmon: (adt7470) Add ADT7470_PWM_MAX macro
In reply to Luiz Angelo Daros de LucaInstead of a magic 255, use a macro to refer to it.
--- drivers/hwmon/adt7470.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/hwmon/adt7470.c +4 -3
@@ -100,6 +100,7 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };#define ADT7470_REG_FAN_MAX (x) (ADT7470_REG_FAN_MAX_BASE_ADDR + ((x) * 2))#define ADT7470_PWM_COUNT 4 +#define ADT7470_PWM_MAX 255#define ADT7470_REG_PWM (x) (ADT7470_REG_PWM_BASE_ADDR + (x))#define ADT7470_REG_PWM_MAX (x) (ADT7470_REG_PWM_MAX_BASE_ADDR + (x))#define ADT7470_REG_PWM_MIN (x) (ADT7470_REG_PWM_MIN_BASE_ADDR + (x))
@@ -811,7 +812,7 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long valswitch (attr) {case hwmon_pwm_input: - val =clamp_val (val,0 ,255 ); + val =clamp_val (val,0 ,ADT7470_PWM_MAX );mutex_lock (&data->lock); data->pwm[channel] = val; err = regmap_write(data->regmap, ADT7470_REG_PWM(channel),
@@ -869,7 +870,7 @@ static ssize_t pwm_max_store(struct device *dev,if (kstrtol (buf,10 , &temp))return -EINVAL ; - temp =clamp_val (temp,0 ,255 ); + temp =clamp_val (temp,0 ,ADT7470_PWM_MAX );mutex_lock (&data->lock); data->pwm_max[attr->index] = temp;
@@ -904,7 +905,7 @@ static ssize_t pwm_min_store(struct device *dev,if (kstrtol (buf,10 , &temp))return -EINVAL ; - temp =clamp_val (temp,0 ,255 ); + temp =clamp_val (temp,0 ,ADT7470_PWM_MAX );mutex_lock (&data->lock); data->pwm_min[attr->index] = temp;
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 2/4] hwmon: (adt7470) Add ADT7470_PWM_MAX macro
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] I2C error during adt7470_read_temperatures() leaves fans permanently stuck in manual mode
- [High] Unbounded sysfs values for auto_update_interval and num_temp_sensors cause a tight loop and I2C bus flooding --
--- Patch [2]: [PATCH 2/4] hwmon: (adt7470) Add ADT7470_PWM_MAX macro --- commit 176c7576aebc83ec3b52518b957109a401b37ada
Author: Luiz Angelo Daros de Luca <[email protected]>
hwmon: (adt7470) Add ADT7470_PWM_MAX macro
Instead of a magic 255, use a macro to refer to it.
drivers/hwmon/adt7470.c static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END }; · line 106
Quoted 12 lines
drivers/hwmon/adt7470.c +1
-0@@ -100,6 +100,7 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };#define ADT7470_REG_FAN_MAX (x) (ADT7470_REG_FAN_MAX_BASE_ADDR + ((x) * 2))#define ADT7470_PWM_COUNT 4 +#define ADT7470_PWM_MAX 255#define ADT7470_REG_PWM (x) (ADT7470_REG_PWM_BASE_ADDR + (x))#define ADT7470_REG_PWM_MAX (x) (ADT7470_REG_PWM_MAX_BASE_ADDR + (x))#define ADT7470_REG_PWM_MIN (x) (ADT7470_REG_PWM_MIN_BASE_ADDR + (x))
[ ... ]
[Severity: High]
This is a pre-existing issue, but could an I2C error during adt7470_read_temperatures() leave the fans permanently stuck in manual mode?
If there is a transient I2C bus error (e.g., NACK) during adt7470_read_temperatures() when updating ADT7470_REG_PWM_CFG(2) or resetting ADT7470_T05_STB_MASK, it looks like it returns immediately:
drivers/hwmon/adt7470.c:adt7470_read_temperatures() {
...
err = regmap_update_bits(data->regmap, ADT7470_REG_PWM_CFG(2),
ADT7470_PWM_AUTO_MASK, 0);
if (err < 0)
return err;
...
}If the function returns here, does it fail to restore PWM_CFG(0) back from manual mode?
On the next iteration of the periodic kthread, might the driver read this corrupted manual mode as the original state, permanently locking the fans in manual mode?
[Severity: High]
This is a pre-existing issue, but does an unbounded sysfs value for auto_update_interval and num_temp_sensors cause a tight loop and I2C bus flooding?
When setting the auto_update_interval, the value is allowed to be 0:
drivers/hwmon/adt7470.c:auto_update_interval_store() {
...
temp = clamp_val(temp, 0, 60000);
mutex_lock(&data->lock);
data->auto_update_interval = temp;
...
}If auto_update_interval is set to 0, does the background thread spin without delay?
drivers/hwmon/adt7470.c:adt7470_update_thread() {
...
schedule_timeout_interruptible(msecs_to_jiffies(data->auto_update_interval));
...
}If both auto_update_interval and num_temp_sensors are 0, does this create a busy-loop of I2C bus transactions?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH 3/4 hwmon: (adt7470) Add thermal cooling device support
In reply to Luiz Angelo Daros de LucaThe ADT7470 has four PWM outputs that can be used to control fans. Register each PWM output as an optional thermal cooling device via Device Tree so it can be referenced by thermal zones.
When the thermal subsystem sets a cooling state, the driver switches the corresponding PWM channel to manual mode so that the requested duty cycle takes effect.
Registration is optional: when the "#cooling-cells" property is absent, devm_thermal_of_child_cooling_device_register() returns -ENODEV and the driver silently skips that PWM channel.
The PWM cache is populated with an explicit adt7470_update_sensors() call before the cooling devices are registered, so that an early get_cur_state() call from the thermal core does not observe stale (zeroed) values.
--- drivers/hwmon/adt7470.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+)
drivers/hwmon/adt7470.c +122 -0
@@ -22,6 +22,7 @@#include <linux/sched.h> #include <linux/slab.h> #include <linux/util_macros.h> +#include <linux/thermal.h> /* Addresses to scan */ static const unsigned short normal_i2c[] = {0x2C ,0x2E ,0x2F ,I2C_CLIENT_END };
@@ -142,6 +143,13 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };#define ADT7470_FREQ_MASK 0x70#define ADT7470_FREQ_SHIFT 4 +struct adt7470_data ; + +struct adt7470_cooling_device { +struct adt7470_data *data; +int pwm_index; +}; +struct adt7470_data {struct regmap *regmap;struct mutex lock;
@@ -171,6 +179,8 @@ struct adt7470_data {struct task_struct *auto_update;unsigned int auto_update_interval; + +struct adt7470_cooling_device cooling_devices[ADT7470_PWM_COUNT ]; };/*
@@ -846,6 +856,107 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long valreturn err; } +static int adt7470_get_max_state (struct thermal_cooling_device *cdev, +unsigned long *state) +{ + *state =ADT7470_PWM_MAX ; +return 0 ; +} + +static int adt7470_get_cur_state (struct thermal_cooling_device *cdev, +unsigned long *state) +{ +struct adt7470_cooling_device *cooling = cdev->devdata; +struct adt7470_data *data = cooling->data; + +mutex_lock (&data->lock); + *state = data->pwm[cooling->pwm_index]; +mutex_unlock (&data->lock); + +return 0 ; +} + +static int adt7470_set_cur_state (struct thermal_cooling_device *cdev, +unsigned long state) +{ +struct adt7470_cooling_device *cooling = cdev->devdata; +struct adt7470_data *data = cooling->data; +unsigned int pwm_auto_reg_mask; +int err; + +if (cooling->pwm_index &1 ) + pwm_auto_reg_mask =ADT7470_PWM2_AUTO_MASK ; +else + pwm_auto_reg_mask =ADT7470_PWM1_AUTO_MASK ; + + state =clamp_val (state,0 ,ADT7470_PWM_MAX ); + +mutex_lock (&data->lock); + +if (data->pwm[cooling->pwm_index] == state && + data->pwm_automatic[cooling->pwm_index] ==0 ) { +mutex_unlock (&data->lock); +return 0 ; + } + +/* Put the PWM channel in manual mode before updating it. */ + err =regmap_update_bits (data->regmap, +ADT7470_REG_PWM_CFG (cooling->pwm_index), + pwm_auto_reg_mask,0 ); +if (err <0 ) + goto out; + + data->pwm_automatic[cooling->pwm_index] =0 ; + + err =regmap_write (data->regmap, +ADT7470_REG_PWM (cooling->pwm_index), state); + +if (err <0 ) + goto out; + + data->pwm[cooling->pwm_index] = state; +out: +mutex_unlock (&data->lock); + +return err; +} + +static const struct thermal_cooling_device_ops adt7470_cooling_ops = { + .get_max_state = adt7470_get_max_state, + .get_cur_state = adt7470_get_cur_state, + .set_cur_state = adt7470_set_cur_state, +}; + +static int adt7470_register_cooling_devices (struct device *dev, +struct adt7470_data *data) +{ +int i; + +for (i =0 ; i <ADT7470_PWM_COUNT ; i++) { +struct thermal_cooling_device *cdev; +char cdev_name[THERMAL_NAME_LENGTH ]; + + data->cooling_devices[i].data = data; + data->cooling_devices[i].pwm_index = i; + +snprintf (cdev_name,sizeof (cdev_name),"adt7470-pwm%d" , i); + cdev =devm_thermal_of_child_cooling_device_register (dev, + dev->of_node, cdev_name, + &data->cooling_devices[i], + &adt7470_cooling_ops); + +if (IS_ERR (cdev)) { +if (PTR_ERR (cdev) == -ENODEV ) +continue ; +return dev_warn_probe (dev,PTR_ERR (cdev), +"failed to register cooling device %s\n" , + cdev_name); + } + } + +return 0 ; +} +static ssize_t pwm_max_show (struct device *dev,struct device_attribute *devattr,char *buf) {
@@ -1281,6 +1392,17 @@ static int adt7470_probe(struct i2c_client *client)if (IS_ERR (hwmon_dev))return PTR_ERR (hwmon_dev); +if (IS_ENABLED (CONFIG_THERMAL )) { +/* fill the cache before registering the cooling devices */ + err =adt7470_update_sensors (data); +if (err) +return err; + + err =adt7470_register_cooling_devices (dev, data); +if (err) +return err; + } + data->auto_update =kthread_run (adt7470_update_thread, client,"%s" ,dev_name (hwmon_dev));if (IS_ERR (data->auto_update))
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 3/4] hwmon: (adt7470) Add thermal cooling device support
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] All 4 PWM cooling devices are registered with identical device tree nodes and cdev_id, preventing…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] All 4 PWM cooling devices are registered with identical device tree nodes and cdev_id, preventing independent thermal zone binding.
- [High] adt7470_update_sensors() is called locklessly in adt7470_probe() after the hwmon device is registered, leading to a severe data race.
- [Medium] The driver unconditionally registers all 4 cooling devices, failing to make them optional based on #cooling-cells, contrary to the commit message. --
--- Patch [3]: [PATCH 3/4] hwmon: (adt7470) Add thermal cooling device support --- commit f3b7f3fcc935f749dd7f53e14d3533656b277e5b
Author: Luiz Angelo Daros de Luca <[email protected]>
hwmon: (adt7470) Add thermal cooling device support
The ADT7470 has four PWM outputs that can be used to control fans. Register each PWM output as an optional thermal cooling device via Device Tree so it can be referenced by thermal zones.
When the thermal subsystem sets a cooling state, the driver switches the corresponding PWM channel to manual mode so that the requested duty cycle takes effect.
Quoted 3 lines
Registration is optional: when the "#cooling-cells" property is absent,
devm_thermal_of_child_cooling_device_register()returns-ENODEVand the driver silently skips that PWM channel.
[Severity: Medium]
Is this description accurate? Looking at thermal_of_cooling_device_register(), it does not appear to parse the device tree for #cooling-cells during registration. Since it doesn't return -ENODEV for a missing property, doesn't this result in unconditionally registering all 4 cooling devices even if the device tree does not specify them?
[ ... ]
Quoted 4 lines
drivers/hwmon/adt7470.c +0
-0
[ ... ]
Quoted 1 line
drivers/hwmon/adt7470.c +0
-0@@ -846,6 +856,107 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
[ ... ]
Quoted 7 lines
drivers/hwmon/adt7470.c +7
-0+static int adt7470_register_cooling_devices (struct device *dev, +struct adt7470_data *data) +{ +int i; + +for (i =0 ; i <ADT7470_PWM_COUNT ; i++) { +struct thermal_cooling_device *cdev;
drivers/hwmon/adt7470.c static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val · line 946
Quoted 10 lines
drivers/hwmon/adt7470.c +10
-0+char cdev_name[THERMAL_NAME_LENGTH ]; + + data->cooling_devices[i].data = data; + data->cooling_devices[i].pwm_index = i; + +snprintf (cdev_name,sizeof (cdev_name),"adt7470-pwm%d" , i); + cdev =devm_thermal_of_child_cooling_device_register (dev, + dev->of_node, cdev_name, + &data->cooling_devices[i], + &adt7470_cooling_ops);
[Severity: High]
When registering the cooling devices in a loop, does passing the same parent dev->of_node for every PWM channel prevent independent thermal zone binding?
Since __devm_thermal_of_cooling_device_register() binds them all with the same node, they will all get assigned cdev_id = 0. When the thermal core parses a DT cooling map requiring a specific cdev_id, will it fail to match the individual channels since they are indistinguishable?
drivers/hwmon/adt7470.c static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val · line 950
Quoted 4 lines
drivers/hwmon/adt7470.c +4
-0+ +if (IS_ERR (cdev)) { +if (PTR_ERR (cdev) == -ENODEV ) +continue ;
[Severity: Medium]
Will this check ever be hit? As noted in the commit message review above, the thermal registration API does not return -ENODEV for missing device tree properties, which causes all PWM channels to be registered unconditionally.
Quoted 8 lines
drivers/hwmon/adt7470.c +8
-0+return dev_warn_probe (dev,PTR_ERR (cdev), +"failed to register cooling device %s\n" , + cdev_name); + } + } + +return 0 ; +}
[ ... ]
drivers/hwmon/adt7470.c static int adt7470_probe(struct i2c_client *client) · line 1397
Quoted 7 lines
drivers/hwmon/adt7470.c +3
-0@@ -1281,6 +1392,17 @@ static int adt7470_probe(struct i2c_client *client)if (IS_ERR (hwmon_dev))return PTR_ERR (hwmon_dev); +if (IS_ENABLED (CONFIG_THERMAL )) { +/* fill the cache before registering the cooling devices */ + err =adt7470_update_sensors (data);
[Severity: High]
Can this call to adt7470_update_sensors() cause hardware configuration corruption due to a missing lock?
This is executed in adt7470_probe() immediately after the hwmon device is registered, meaning sysfs attributes are already exposed to userspace.
Since adt7470_update_sensors() calls adt7470_read_temperatures(), which explicitly assumes data->lock is held while performing multi-step register manipulations and a 2-second sleep, what prevents a concurrent userspace sysfs access from racing during this window and corrupting the PWM configuration?
Quoted 10 lines
drivers/hwmon/adt7470.c +8
-0+if (err) +return err; + + err =adt7470_register_cooling_devices (dev, data); +if (err) +return err; + } + data->auto_update =kthread_run (adt7470_update_thread, client,"%s" ,dev_name (hwmon_dev));
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH 4/4 hwmon: (adt7470) Add thermal zone sensor support
In reply to Luiz Angelo Daros de LucaExpose the ADT7470 external temperature sensors to the thermal framework via Device Tree. The thermal callbacks use the driver's cached sensor data, avoiding any I2C bus traffic during frequent polling by the thermal core.
--- drivers/hwmon/adt7470.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)
drivers/hwmon/adt7470.c +51 -0
@@ -150,6 +150,11 @@ struct adt7470_cooling_device {int pwm_index; }; +struct adt7470_thermal_sensor { +struct adt7470_data *data; +int id; +}; +struct adt7470_data {struct regmap *regmap;struct mutex lock;
@@ -181,6 +186,7 @@ struct adt7470_data {unsigned int auto_update_interval;struct adt7470_cooling_device cooling_devices[ADT7470_PWM_COUNT ]; +struct adt7470_thermal_sensor thermal_sensors[ADT7470_TEMP_COUNT ]; };/*
@@ -957,6 +963,47 @@ static int adt7470_register_cooling_devices(struct device *dev,return 0 ; } +static int adt7470_get_temp (struct thermal_zone_device *tz,int *temp) +{ +struct adt7470_thermal_sensor *sensor =thermal_zone_device_priv (tz); +struct adt7470_data *data = sensor->data; + +mutex_lock (&data->lock); + *temp =1000 * data->temp[sensor->id]; +mutex_unlock (&data->lock); + +return 0 ; +} + +static const struct thermal_zone_device_ops adt7470_thermal_ops = { + .get_temp = adt7470_get_temp, +}; + +static int adt7470_register_thermal_sensors (struct device *dev, +struct adt7470_data *data) +{ +int i; + +for (i =0 ; i <ADT7470_TEMP_COUNT ; i++) { +struct thermal_zone_device *tz; + + data->thermal_sensors[i].data = data; + data->thermal_sensors[i].id = i; + + tz =devm_thermal_of_zone_register (dev, i, &data->thermal_sensors[i], + &adt7470_thermal_ops); +if (IS_ERR (tz)) { +if (PTR_ERR (tz) == -ENODEV ) +continue ; +return dev_warn_probe (dev,PTR_ERR (tz), +"failed to register thermal zone %d\n" , + i); + } + } + +return 0 ; +} +static ssize_t pwm_max_show (struct device *dev,struct device_attribute *devattr,char *buf) {
@@ -1401,6 +1448,10 @@ static int adt7470_probe(struct i2c_client *client) err =adt7470_register_cooling_devices (dev, data);if (err)return err; + + err =adt7470_register_thermal_sensors (dev, data); +if (err) +return err; } data->auto_update = kthread_run(adt7470_update_thread, client, "%s",
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 4/4] hwmon: (adt7470) Add thermal zone sensor support
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] The thermal zone callback `adt7470_get_temp()` relies on a cached temperature array (`data->temp`)…
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The thermal zone callback adt7470_get_temp() relies on a cached temperature array (data->temp) that is never updated by the background thread, resulting in permanently stale temperature readings.
- [High] Severe lock contention on data->lock stalling the thermal polling worker for up to 2 seconds.
- [High] Unprotected call to adt7470_update_sensors() during probe races with sysfs reads, allowing concurrent execution and hardware state corruption.
- [High] Cooling device ID collision prevents independent PWM channel control because devm_thermal_of_child_cooling_device_register() hardcodes cdev_id = 0.
- [Medium] The newly added thermal callback violates the hwmon subsystem locking guidelines by using a private mutex instead of hwmon_lock().
--
--- Patch [4]: [PATCH 4/4] hwmon: (adt7470) Add thermal zone sensor support --- commit 76a97c2b3fe41c135377aaeddcae6f2428b2ba1f
Author: Luiz Angelo Daros de Luca <[email protected]>
hwmon: (adt7470) Add thermal zone sensor support
Expose the ADT7470 external temperature sensors to the thermal framework via Device Tree. The thermal callbacks use the driver's cached sensor data, avoiding any I2C bus traffic during frequent polling by the thermal core.
Quoted 4 lines
drivers/hwmon/adt7470.c +0
-0
[ ... ]
drivers/hwmon/adt7470.c static int adt7470_register_cooling_devices(struct device *dev, · line 971
Quoted 10 lines
drivers/hwmon/adt7470.c +6
-0@@ -957,6 +963,47 @@ static int adt7470_register_cooling_devices(struct device *dev,return 0 ; } +static int adt7470_get_temp (struct thermal_zone_device *tz,int *temp) +{ +struct adt7470_thermal_sensor *sensor =thermal_zone_device_priv (tz); +struct adt7470_data *data = sensor->data; + +mutex_lock (&data->lock);
[Severity: Medium]
Does using this private mutex violate the hwmon subsystem locking guidelines?
The documentation specifies that drivers should use hwmon_lock() and hwmon_unlock() for attributes registered outside the hwmon core, rather than their internal lock.
[Severity: High]
Can taking this lock cause the thermal polling worker to stall?
The background thread adt7470_update_thread() acquires data->lock and then sleeps for up to 2 seconds while waiting for hardware measurements:
adt7470_read_temperatures()
res = msleep_interruptible((data->num_temp_sensors >= 0 ?
data->num_temp_sensors * 200 :
TEMP_COLLECTION_TIME));Any concurrent attempt by the thermal core to call adt7470_get_temp() will block on this exact same mutex for the duration of that sleep, potentially starving the system thermal workqueue.
drivers/hwmon/adt7470.c static int adt7470_register_cooling_devices(struct device *dev, · line 972
Quoted 1 line
drivers/hwmon/adt7470.c +1
-0+ *temp =1000 * data->temp[sensor->id];
[Severity: High]
Will this temperature reading remain permanently stale?
The background thread adt7470_update_thread() calls adt7470_read_temperatures(), which initiates conversions but returns early without reading the values:
adt7470_read_temperatures()
if (data->num_temp_sensors >= 0)
return 0;Since it bypasses reading the results into the data->temp cache, and the cache is exclusively updated when userspace reads sysfs attributes via adt7470_update_sensors(), relying entirely on the thermal core for polling will prevent the reported temperature from ever changing.
Quoted 4 lines
drivers/hwmon/adt7470.c +4
-0+mutex_unlock (&data->lock); + +return 0 ; +}
[ ... ]
drivers/hwmon/adt7470.c static int adt7470_probe(struct i2c_client *client) · line 1448
Quoted 2 lines
drivers/hwmon/adt7470.c +0
-0@@ -1401,6 +1448,10 @@ static int adt7470_probe(struct i2c_client *client) err =adt7470_register_cooling_devices (dev, data);
[Severity: High]
Does this cause a cooling device ID collision?
In adt7470_register_cooling_devices(), the driver loops to register 4 separate cooling devices using devm_thermal_of_child_cooling_device_register(), passing the exact same device node:
adt7470_register_cooling_devices()
cdev = devm_thermal_of_child_cooling_device_register(dev,
dev->of_node,
...);Because this child API internally hardcodes cdev_id = 0, all 4 channels end up sharing identical matching criteria. This prevents the thermal framework from distinguishing between the PWM cooling devices, destroying independent channel control.
[Severity: High]
Is there a race condition with sysfs reads during probe?
Earlier in adt7470_probe(), the hwmon device is registered with devm_hwmon_device_register_with_info(), exposing sysfs attributes to userspace. Immediately after, adt7470_update_sensors(data) is called without holding data->lock:
adt7470_probe()
hwmon_dev = devm_hwmon_device_register_with_info(...)
...
err = adt7470_update_sensors(data);A concurrent sysfs read can trigger adt7470_update_device(), which takes the lock and calls adt7470_update_sensors() simultaneously. This interleaved execution inside adt7470_read_temperatures() could corrupt hardware sequences.
Quoted 7 lines
drivers/hwmon/adt7470.c +4
-0if (err)return err; + + err =adt7470_register_thermal_sensors (dev, data); +if (err) +return err; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Guenter Roeck
maintainerRe: [PATCH 4/4] hwmon: (adt7470) Add thermal zone sensor support
In reply to Luiz Angelo Daros de LucaQuoted 4 lines · Luiz Angelo Daros de Luca
Expose the ADT7470 external temperature sensors to the thermal framework via Device Tree. The thermal callbacks use the driver's cached sensor data, avoiding any I2C bus traffic during frequent polling by the thermal core.
Detailed explanation why HWMON_C_REGISTER_TZ does not work or is insufficient goes here and into the driver code as comment.
Thanks,
Guenter
Quoted 89 lines
Signed-off-by Luiz Angelo Daros de Luca <[email protected]>--- drivers/hwmon/adt7470.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)drivers/hwmon/adt7470.c +51
-0@@ -150,6 +150,11 @@ struct adt7470_cooling_device {int pwm_index; }; +struct adt7470_thermal_sensor { +struct adt7470_data *data; +int id; +}; +struct adt7470_data {struct regmap *regmap;struct mutex lock;@@ -181,6 +186,7 @@ struct adt7470_data {unsigned int auto_update_interval;struct adt7470_cooling_device cooling_devices[ADT7470_PWM_COUNT ]; +struct adt7470_thermal_sensor thermal_sensors[ADT7470_TEMP_COUNT ]; };/* @@ -957,6 +963,47 @@ static int adt7470_register_cooling_devices(struct device *dev,return 0 ; } +static int adt7470_get_temp (struct thermal_zone_device *tz,int *temp) +{ +struct adt7470_thermal_sensor *sensor =thermal_zone_device_priv (tz); +struct adt7470_data *data = sensor->data; + +mutex_lock (&data->lock); + *temp =1000 * data->temp[sensor->id]; +mutex_unlock (&data->lock); + +return 0 ; +} + +static const struct thermal_zone_device_ops adt7470_thermal_ops = { + .get_temp = adt7470_get_temp, +}; + +static int adt7470_register_thermal_sensors (struct device *dev, +struct adt7470_data *data) +{ +int i; + +for (i =0 ; i <ADT7470_TEMP_COUNT ; i++) { +struct thermal_zone_device *tz; + + data->thermal_sensors[i].data = data; + data->thermal_sensors[i].id = i; + + tz =devm_thermal_of_zone_register (dev, i, &data->thermal_sensors[i], + &adt7470_thermal_ops); +if (IS_ERR (tz)) { +if (PTR_ERR (tz) == -ENODEV ) +continue ; +return dev_warn_probe (dev,PTR_ERR (tz), +"failed to register thermal zone %d\n" , + i); + } + } + +return 0 ; +} +static ssize_t pwm_max_show (struct device *dev,struct device_attribute *devattr,char *buf) {@@ -1401,6 +1448,10 @@ static int adt7470_probe(struct i2c_client *client) err =adt7470_register_cooling_devices (dev, data);if (err)return err; + + err =adt7470_register_thermal_sensors (dev, data); +if (err) +return err; } data->auto_update = kthread_run(adt7470_update_thread, client, "%s",
Re: [PATCH 4/4] hwmon: (adt7470) Add thermal zone sensor support
Quoted 2 lines
Detailed explanation why
HWMON_C_REGISTER_TZdoes not work or is insufficient goes here and into the driver code as comment.
No need to explain anything. I was just unaware of the existence of HWMON_C_REGISTER_TZ.
You are absolutely right. I'll refactor the driver to use it and make the code much cleaner. Thanks!
Regards,
Luiz
Guenter Roeck
maintainerQuoted 22 lines · Luiz Angelo Daros de Luca
The ADT7470 is a temperature monitor and PWM fan controller capable of monitoring up to 10 external temperature sensors and controlling up to 4 PWM outputs.
Currently, the driver exposes these sensors and PWM controls solely via the standard hwmon sysfs interface. This patch series integrates the ADT7470 driver with the kernel's thermal subsystem, allowing its temperature sensors to be referenced by Device Tree thermal zones.
To avoid saturating the I2C bus during frequent polling by the thermal core, the thermal callbacks are designed to read from the driver's internal cache.
Patch 1 adds the YAML device tree binding documentation, defining the necessary #cooling-cells and #thermal-sensor-cells properties. Patch 2 performs a minor refactoring to introduce the
ADT7470_PWM_MAXmacro, replacing hardcoded values.
Patch 3 registers the 4 PWM channels as optional thermal cooling devices.
Patch 4 registers the external temperature sensors as optional thermal zones.
Quoted 1 line
Signed-off-by Luiz Angelo Daros de Luca <[email protected]>
This chip is 20+ years old, and it is very unlikely that it is used in current designs. Please provide evidence that this code is actually needed and not just a coding exercise.
Thanks,
Guenter
Quoted 3 lines
This chip is 20+ years old, and it is very unlikely that it is used in current designs. Please provide evidence that this code is actually needed and not just a coding exercise.
I understand. It is used in multiple Edgecore switches like ECS2100. I'm preparing a port to OpenWrt.
https://github.com/luizluca/openwrt/commits/ecs2110/
https://forum.openwrt.org/t/add-support-for-edgecore-ecs2100-ecs4100-switches/251818/10
Regards,
Luiz