[hwmon PATCH v3 0/6] hwmon: (adt7470): Multiple fixes

[hwmon PATCH v3 0/6] hwmon: (adt7470): Multiple fixes

[Luiz Angelo Daros de Luca]

This series fixes several issues in the adt7470 driver:

- Restore PWM configuration after I2C failures.
- Avoid cache corruption after failed writes.
- Prevent busy looping when auto_update_interval is zero.
- Fix incorrect temperature alarm reporting.
- Cache the PWM frequency and fix error propagation.
- Correct unused PWM auto-mode mask definitions.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
Luiz Angelo Daros de Luca (6):
      hwmon: (adt7470) Fix fans stuck in manual mode on I2C errors
      hwmon: (adt7470) Fix cache updated before hardware write on I2C error
      hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread
      hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks
      hwmon: (adt7470) Fix temperature alarm logic in hwmon_temp_read()
      hwmon: (adt7470) Use cached PWM frequency value

 drivers/hwmon/adt7470.c | 80 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 22 deletions(-)
---
base-commit: 7e12ea5aae19ee07d466e9613ccba753349c9c1d
change-id: 20260716-adt7470_thermalzone-59a102278d72

Best regards,
--  
Luiz Angelo Daros de Luca <luizluca@gmail.com>

[hwmon PATCH v3 1/6] hwmon: (adt7470) Fix fans stuck in manual mode on I2C errors

[Luiz Angelo Daros de Luca]

During adt7470_read_temperatures(), the driver temporarily switches
the PWM channels to manual mode, performs the temperature collection,
and then restores the original configuration registers.

However, if an I2C transaction fails at any point after entering manual
mode, the function aborts and returns immediately. This leaves the
configuration registers un-restored, permanently trapping the fans in
manual mode.

Introduce a recovery path to ensure that the original PWM configuration
registers are always restored, even when intermediate I2C operations
fail.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/r/20260716213252.EACA71F000E9@smtp.kernel.org
Fixes: ef67959c4253 ("hwmon: (adt7470) Convert to use regmap")
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/hwmon/adt7470.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 664349756dc2..481d51617f4b 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -205,11 +205,12 @@ static inline int adt7470_write_word_data(struct adt7470_data *data, unsigned in
 /* Probe for temperature sensors.  Assumes lock is held */
 static int adt7470_read_temperatures(struct adt7470_data *data)
 {
-	unsigned long res;
+	struct device *dev = regmap_get_device(data->regmap);
+	u8 pwm[ADT7470_FAN_COUNT];
 	unsigned int pwm_cfg[2];
-	int err;
+	unsigned long res;
+	int err, err2;
 	int i;
-	u8 pwm[ADT7470_FAN_COUNT];
 
 	/* save pwm[1-4] config register */
 	err = regmap_read(data->regmap, ADT7470_REG_PWM_CFG(0), &pwm_cfg[0]);
@@ -233,19 +234,19 @@ static int adt7470_read_temperatures(struct adt7470_data *data)
 	err = regmap_update_bits(data->regmap, ADT7470_REG_PWM_CFG(2),
 				 ADT7470_PWM_AUTO_MASK, 0);
 	if (err < 0)
-		return err;
+		goto out_restore;
 
 	/* write pwm control to whatever it was */
 	err = regmap_bulk_write(data->regmap, ADT7470_REG_PWM(0), &pwm[0],
 				ADT7470_PWM_COUNT);
 	if (err < 0)
-		return err;
+		goto out_restore;
 
 	/* start reading temperature sensors */
 	err = regmap_update_bits(data->regmap, ADT7470_REG_CFG,
 				 ADT7470_T05_STB_MASK, ADT7470_T05_STB_MASK);
 	if (err < 0)
-		return err;
+		goto out_restore;
 
 	/* Delay is 200ms * number of temp sensors. */
 	res = msleep_interruptible((data->num_temp_sensors >= 0 ?
@@ -256,13 +257,30 @@ static int adt7470_read_temperatures(struct adt7470_data *data)
 	err = regmap_update_bits(data->regmap, ADT7470_REG_CFG,
 				 ADT7470_T05_STB_MASK, 0);
 	if (err < 0)
-		return err;
+		goto out_restore;
 
+out_restore:
 	/* restore pwm[1-4] config registers */
-	err = regmap_write(data->regmap, ADT7470_REG_PWM_CFG(0), pwm_cfg[0]);
-	if (err < 0)
-		return err;
-	err = regmap_write(data->regmap, ADT7470_REG_PWM_CFG(2), pwm_cfg[1]);
+	err2 = regmap_write(data->regmap, ADT7470_REG_PWM_CFG(0), pwm_cfg[0]);
+	if (err2 < 0) {
+		dev_warn_ratelimited(dev,
+				     "failed to restore PWM{1,2} config (%d)\n",
+				     err2);
+
+		if (!err)
+			err = err2;
+	}
+
+	err2 = regmap_write(data->regmap, ADT7470_REG_PWM_CFG(2), pwm_cfg[1]);
+	if (err2 < 0) {
+		dev_warn_ratelimited(dev,
+				     "failed to restore PWM{3,4} config (%d)\n",
+				     err2);
+
+		if (!err)
+			err = err2;
+	}
+
 	if (err < 0)
 		return err;
 

-- 
2.55.0

Re: [hwmon PATCH v3 1/6] hwmon: (adt7470) Fix fans stuck in manual mode on I2C errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless access to sensor data combined with macro multiple expansion causes TOCTOU divide-by-zero crashes.
--

--- Patch [1]: [hwmon PATCH v3 1/6] hwmon: (adt7470) Fix fans stuck in manual mode on I2C errors ---
commit f43c48cc32db2262b55dd289a4c96b005040eb1e
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Fix fans stuck in manual mode on I2C errors
    
During adt7470_read_temperatures(), the driver temporarily switches
the PWM channels to manual mode, performs the temperature collection,
and then restores the original configuration registers.

However, if an I2C transaction fails at any point after entering manual
mode, the function aborts and returns immediately. This leaves the
configuration registers un-restored, permanently trapping the fans in
manual mode.

Introduce a recovery path to ensure that the original PWM configuration
registers are always restored, even when intermediate I2C operations
fail.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/r/20260716213252.EACA71F000E9@smtp.kernel.org
Fixes: ef67959c4253 ("hwmon: (adt7470) Convert to use regmap")

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 664349756dc2b..481d51617f4be 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this lockless access combined
with macro expansion risk a divide-by-zero?

drivers/hwmon/adt7470.c:adt7470_fan_read() {
        ...
        case hwmon_fan_input:
                if (FAN_DATA_VALID(data->fan[channel]))
                        *val = FAN_PERIOD_TO_RPM(data->fan[channel]);
        ...
}

Because data->fan[channel] is accessed multiple times without a lock or
READ_ONCE(), the compiler can generate multiple memory loads.

Concurrently, if an unprivileged user reads a custom attribute like
num_temp_sensors, it bypasses hwmon_lock and triggers adt7470_update_device()
which acquires data->lock, reads new I2C data, and writes to
data->fan[channel].

If the I2C read returns 0 (e.g., due to a stalled fan or bus glitch) exactly
between the validation check and the division, could this crash the kernel?

The same pattern appears to apply to fan_min and fan_max as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=1

[hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error

[Luiz Angelo Daros de Luca]

adt7470_temp_write() and adt7470_pwm_write() update the driver's
cached values (temp_min, temp_max, pwm_input, pwm_enable) before issuing
the corresponding regmap_write(), and never check whether the write
succeeded before committing that update. If the I2C transaction fails,
the function correctly propagates the error to the caller, but the cache
silently keeps the new value, which was never actually applied to the
hardware. Subsequent reads then report a value that does not match the
device state.

Reorder both write paths to update the cache only after a successful
regmap_write(), so the cache always reflects what was actually
written to the hardware.

Fixes: ef67959c4253 ("hwmon: (adt7470) Convert to use regmap")
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/hwmon/adt7470.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 481d51617f4b..62ec68ea0a40 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -589,14 +589,16 @@ static int adt7470_temp_write(struct device *dev, u32 attr, int channel, long va
 	switch (attr) {
 	case hwmon_temp_min:
 		mutex_lock(&data->lock);
-		data->temp_min[channel] = val;
 		err = regmap_write(data->regmap, ADT7470_TEMP_MIN_REG(channel), val);
+		if (!err)
+			data->temp_min[channel] = val;
 		mutex_unlock(&data->lock);
 		break;
 	case hwmon_temp_max:
 		mutex_lock(&data->lock);
-		data->temp_max[channel] = val;
 		err = regmap_write(data->regmap, ADT7470_TEMP_MAX_REG(channel), val);
+		if (!err)
+			data->temp_max[channel] = val;
 		mutex_unlock(&data->lock);
 		break;
 	default:
@@ -831,9 +833,10 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
 	case hwmon_pwm_input:
 		val = clamp_val(val, 0, 255);
 		mutex_lock(&data->lock);
-		data->pwm[channel] = val;
 		err = regmap_write(data->regmap, ADT7470_REG_PWM(channel),
-				   data->pwm[channel]);
+				   val);
+		if (!err)
+			data->pwm[channel] = val;
 		mutex_unlock(&data->lock);
 		break;
 	case hwmon_pwm_enable:
@@ -847,10 +850,11 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
 		val--;
 
 		mutex_lock(&data->lock);
-		data->pwm_automatic[channel] = val;
 		err = regmap_update_bits(data->regmap, ADT7470_REG_PWM_CFG(channel),
 					 pwm_auto_reg_mask,
 					 val ? pwm_auto_reg_mask : 0);
+		if (!err)
+			data->pwm_automatic[channel] = val;
 		mutex_unlock(&data->lock);
 		break;
 	case hwmon_pwm_freq:

-- 
2.55.0

Re: [hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The PWM enable mask logic swaps the bitmasks for channels 2 and 3 (PWM3 and PWM4).
--

--- Patch [2]: [hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error ---
commit 6776a671fbb0d95bfa3d1f5ca2bae943732d5533
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Fix cache updated before hardware write on I2C error

This commit ensures the driver's cached values are only updated after a
successful regmap_write. By checking the return value, the cache accurately
reflects the actual hardware state even if an I2C transaction fails.

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 481d51617f4be..62ec68ea0a406 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c

[ ... ]

> @@ -847,10 +850,11 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
>  		val--;
>  
>  		mutex_lock(&data->lock);
> -		data->pwm_automatic[channel] = val;
>  		err = regmap_update_bits(data->regmap, ADT7470_REG_PWM_CFG(channel),
>  					 pwm_auto_reg_mask,

[Severity: High]
This is a pre-existing issue, but does this modulo logic for the PWM enable 
mask swap the bitmasks for channels 2 and 3 (PWM3 and PWM4)?

Earlier in adt7470_pwm_write(), the mask is assigned using modulo arithmetic:

    case hwmon_pwm_enable:
        if (channel % 2)
            pwm_auto_reg_mask = ADT7470_PWM2_AUTO_MASK;
        else
            pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;

This assigns the mask for PWM4 (0x80) to channel 2 (PWM3) and the mask for 
PWM3 (0x40) to channel 3 (PWM4) during sysfs write. As a result, operations 
intended for PWM3 modify the hardware state of PWM4, and vice versa.

The same pattern is used during the periodic sensor state update in 
adt7470_update_sensors():

    if (i % 2)
        mask = ADT7470_PWM2_AUTO_MASK;
    else
        mask = ADT7470_PWM1_AUTO_MASK;

Could this cause fans to fail to enter the commanded automatic or manual mode, 
potentially leading to improper cooling?

>  					 val ? pwm_auto_reg_mask : 0);
> +		if (!err)
> +			data->pwm_automatic[channel] = val;
>  		mutex_unlock(&data->lock);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=2

Re: [hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error

[Luiz Angelo Daros de Luca]

Em seg., 27 de jul. de 2026 às 00:50, <sashiko-bot@kernel.org> escreveu:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The PWM enable mask logic swaps the bitmasks for channels 2 and 3 (PWM3 and PWM4).
> --
>
> --- Patch [2]: [hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error ---
> commit 6776a671fbb0d95bfa3d1f5ca2bae943732d5533
> Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>
>
> hwmon: (adt7470) Fix cache updated before hardware write on I2C error
>
> This commit ensures the driver's cached values are only updated after a
> successful regmap_write. By checking the return value, the cache accurately
> reflects the actual hardware state even if an I2C transaction fails.
>
> > diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> > index 481d51617f4be..62ec68ea0a406 100644
> > --- a/drivers/hwmon/adt7470.c
> > +++ b/drivers/hwmon/adt7470.c
>
> [ ... ]
>
> > @@ -847,10 +850,11 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
> >               val--;
> >
> >               mutex_lock(&data->lock);
> > -             data->pwm_automatic[channel] = val;
> >               err = regmap_update_bits(data->regmap, ADT7470_REG_PWM_CFG(channel),
> >                                        pwm_auto_reg_mask,
>
> [Severity: High]
> This is a pre-existing issue, but does this modulo logic for the PWM enable
> mask swap the bitmasks for channels 2 and 3 (PWM3 and PWM4)?
>
> Earlier in adt7470_pwm_write(), the mask is assigned using modulo arithmetic:
>
>     case hwmon_pwm_enable:
>         if (channel % 2)
>             pwm_auto_reg_mask = ADT7470_PWM2_AUTO_MASK;
>         else
>             pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;
>
> This assigns the mask for PWM4 (0x80) to channel 2 (PWM3) and the mask for
> PWM3 (0x40) to channel 3 (PWM4) during sysfs write. As a result, operations
> intended for PWM3 modify the hardware state of PWM4, and vice versa.
>
> The same pattern is used during the periodic sensor state update in
> adt7470_update_sensors():
>
>     if (i % 2)
>         mask = ADT7470_PWM2_AUTO_MASK;
>     else
>         mask = ADT7470_PWM1_AUTO_MASK;
>
> Could this cause fans to fail to enter the commanded automatic or manual mode,
> potentially leading to improper cooling?
>
> >                                        val ? pwm_auto_reg_mask : 0);
> > +             if (!err)
> > +                     data->pwm_automatic[channel] = val;
> >               mutex_unlock(&data->lock);
> >               break;

The modulo logic isn't the issue here; rather, ADT7470_PWM3_AUTO_MASK
and ADT7470_PWM4_AUTO_MASK are incorrectly defined.

This is already addressed later in this series in patch 4/6. While
fixing the definitions is mostly cosmetic, it will prevent Sashiko-bot
from repeatedly flagging this false positive in the future.

Regards,

Luiz

[hwmon PATCH v3 3/6] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

[Luiz Angelo Daros de Luca]

When userspace configures 'auto_update_interval' to 0 via sysfs, the
background kthread executes schedule_timeout_interruptible(0), which
returns immediately.

If 'num_temp_sensors' is concurrently or previously set to 0, the
msleep_interruptible() delay inside adt7470_read_temperatures() also
becomes 0. This combination forces the background thread into a tight,
unbounded busy-loop, hogging the CPU and flooding the I2C bus with a
continuous stream of transactions.

Fix this vulnerability by raising the lower limit of the clamp_val in
auto_update_interval_store() from 0 to 500 milliseconds. This guarantees
a reasonable minimum sleep window between sensor updates, protecting the
system from intentional or accidental I2C bus denial of service.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/r/20260716213252.EACA71F000E9@smtp.kernel.org
Fixes: 89fac11cb3e7 ("adt7470: make automatic fan control really work")
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/hwmon/adt7470.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 62ec68ea0a40..0b19b0925d1c 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -509,7 +509,7 @@ static ssize_t auto_update_interval_store(struct device *dev,
 	if (kstrtol(buf, 10, &temp))
 		return -EINVAL;
 
-	temp = clamp_val(temp, 0, 60000);
+	temp = clamp_val(temp, 500, 60000);
 
 	mutex_lock(&data->lock);
 	data->auto_update_interval = temp;

-- 
2.55.0

Re: [hwmon PATCH v3 3/6] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The temperature alarm reporting logic uses the raw integer channel index instead of a bitmask, resulting in incorrect alarm reporting.
- [High] In `pwm_auto_temp_store`, user input overflows a hardware bit-field due to missing bounds checks, and is written to the wrong state array, corrupting the PWM automatic enable state.
--

--- Patch [3]: [hwmon PATCH v3 3/6] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread ---
commit c10c388e55482dcd15df4bd6225052bbff835213
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

Setting 'auto_update_interval' and 'num_temp_sensors' to 0 forces the
background thread into a tight, unbounded busy-loop, hogging the CPU and
flooding the I2C bus. This patch fixes the vulnerability by raising the
lower limit in auto_update_interval_store() to 500 milliseconds. This
guarantees a reasonable minimum sleep window between sensor updates.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/r/20260716213252.EACA71F000E9@smtp.kernel.org
Fixes: 89fac11cb3e7 ("adt7470: make automatic fan control really work")
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 62ec68ea0a406..0b19b0925d1c7 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -509,7 +509,7 @@ static ssize_t auto_update_interval_store(struct device *dev,
>  	if (kstrtol(buf, 10, &temp))
>  		return -EINVAL;
>  
> -	temp = clamp_val(temp, 0, 60000);
> +	temp = clamp_val(temp, 500, 60000);
>  
>  	mutex_lock(&data->lock);
>  	data->auto_update_interval = temp;

While reviewing the code, I found a few unrelated issues in this driver.

drivers/hwmon/adt7470.c:adt7470_temp_read() {
...
	case hwmon_temp_min:
		*val = 1000 * data->temp_min[channel];
		break;
	case hwmon_temp_max:
		*val = 1000 * data->temp_max[channel];
		break;
	case hwmon_temp_alarm:
		*val = !!(data->alarm & channel);
		break;
...
}

[Severity: High]
This isn't a bug introduced by this patch, but is the temperature alarm
reporting logic using the raw integer channel index instead of a bitmask?

If channel is 0 (temp1_alarm), data->alarm & 0 is always 0. For other
channels, it checks bit 0, 1, 2, or 3 incorrectly, which might lead to
false positives or missed alarms.


drivers/hwmon/adt7470.c:pwm_auto_temp_store() {
...
	temp = cvt_auto_temp(temp);
	if (temp < 0)
		return temp;

	mutex_lock(&data->lock);
	data->pwm_automatic[attr->index] = temp;

	if (!(attr->index % 2)) {
		mask = 0xF0;
		val = (temp << 4) & 0xF0;
	} else {
		mask = 0x0F;
		val = temp & 0x0F;
	}
...
}

[Severity: High]
This is a pre-existing issue, but does this write the parsed value into the
wrong state array, and can the unchecked input overflow the hardware
bit-field?

It appears to overwrite the pwm_automatic enable state instead of updating
data->pwm_auto_temp, corrupting the PWM automatic enable state.

Additionally, since cvt_auto_temp() can return a value up to 32, it can
overflow the 4-bit field mask when constructing the register value.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=3

[hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

[Luiz Angelo Daros de Luca]

The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
currently defined with swapped bit values.

According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
Configuration for register 0x69 follows the exact same bit position
layout as register 0x68:
- 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
- 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
- 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
- 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40

Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.

This typo did not cause any functional bugs because these specific
macros are never referenced in the driver code. Instead, the driver
correctly applies the configuration by relying on the modulo parity of
the channel index (e.g., `channel % 2`) to selectively apply either
ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
Since the bit layout is identical between the two configuration
registers, the hardware is currently configured correctly.

Fix the macro definitions to reflect the datasheet accurately and
prevent future bugs or confusion during code review and refactoring.
As this is a purely cosmetic fix with no functional impact, a backport
to stable kernels is not necessary.

Fixes: 6f9703d0be16 ("hwmon: add support for adt7470")
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/hwmon/adt7470.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 0b19b0925d1c..a34635526aaa 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -70,8 +70,8 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };
 #define		ADT7470_PWM1_AUTO_MASK		0x80
 #define		ADT7470_PWM_AUTO_MASK		0xC0
 #define ADT7470_REG_PWM34_CFG			0x69
-#define		ADT7470_PWM3_AUTO_MASK		0x40
-#define		ADT7470_PWM4_AUTO_MASK		0x80
+#define		ADT7470_PWM4_AUTO_MASK		0x40
+#define		ADT7470_PWM3_AUTO_MASK		0x80
 #define	ADT7470_REG_PWM_MIN_BASE_ADDR		0x6A
 #define ADT7470_REG_PWM_MIN_MAX_ADDR		0x6D
 #define ADT7470_REG_PWM_TEMP_MIN_BASE_ADDR	0x6E

-- 
2.55.0

Re: [hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

[sashiko-bot]

> The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
> currently defined with swapped bit values.
> 
> According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
> Configuration for register 0x69 follows the exact same bit position
> layout as register 0x68:
> - 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
> - 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
> - 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
> - 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40
> 
> Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.
> 
> This typo did not cause any functional bugs because these specific
> macros are never referenced in the driver code. Instead, the driver
> correctly applies the configuration by relying on the modulo parity of
> the channel index (e.g., `channel % 2`) to selectively apply either
> ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
> Since the bit layout is identical between the two configuration
> registers, the hardware is currently configured correctly.
> 
> Fix the macro definitions to reflect the datasheet accurately and
> prevent future bugs or confusion during code review and refactoring.
> As this is a purely cosmetic fix with no functional impact, a backport
> to stable kernels is not necessary.
> 
> Fixes: 6f9703d0be16 ("hwmon: add support for adt7470")
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=4

Re: [hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

[Guenter Roeck]

On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
> The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
> currently defined with swapped bit values.
> 
> According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
> Configuration for register 0x69 follows the exact same bit position
> layout as register 0x68:
> - 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
> - 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
> - 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
> - 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40
> 
> Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.
> 
> This typo did not cause any functional bugs because these specific
> macros are never referenced in the driver code. Instead, the driver
> correctly applies the configuration by relying on the modulo parity of
> the channel index (e.g., `channel % 2`) to selectively apply either
> ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
> Since the bit layout is identical between the two configuration
> registers, the hardware is currently configured correctly.
> 
> Fix the macro definitions to reflect the datasheet accurately and
> prevent future bugs or confusion during code review and refactoring.
> As this is a purely cosmetic fix with no functional impact, a backport
> to stable kernels is not necessary.
> 

If the masks are unused, how about just dropping the definitions instead
(including the unused ADT7470_REG_PWM12_CFG and ADT7470_REG_PWM34_CFG) ?
That could be done as first patch of the series.

Thanks,
Guenter

Re: [hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

[Luiz Angelo Daros de Luca]

> On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
> > The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
> > currently defined with swapped bit values.
> >
> > According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
> > Configuration for register 0x69 follows the exact same bit position
> > layout as register 0x68:
> > - 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
> > - 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
> > - 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
> > - 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40
> >
> > Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.
> >
> > This typo did not cause any functional bugs because these specific
> > macros are never referenced in the driver code. Instead, the driver
> > correctly applies the configuration by relying on the modulo parity of
> > the channel index (e.g., `channel % 2`) to selectively apply either
> > ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
> > Since the bit layout is identical between the two configuration
> > registers, the hardware is currently configured correctly.
> >
> > Fix the macro definitions to reflect the datasheet accurately and
> > prevent future bugs or confusion during code review and refactoring.
> > As this is a purely cosmetic fix with no functional impact, a backport
> > to stable kernels is not necessary.

If you build with W=2, you indeed get tons of unused macros...

Initially, I intended to just drop them. However, just like the
incorrect macro led Sashiko (and other bots) to assume the masks were
swapped, keeping them correctly defined serves as useful context
directly in the code. If we drop them entirely, anyone (or any AI
agent/tool analyzing the driver) would need to fetch and parse the
datasheet to verify the register layout. While the datasheet is the
ultimate source of truth, reading PDF tables costs more than reading
well-defined driver constants, to the point that agents might simply
skip it.

We could certainly drop the unused macros and add a comment explaining
the layout, or even rename them to better reflect their shared usage
across channels (e.g., ADT7470_PWM_ODD_AUTO_MASK and
ADT7470_PWM_EVEN_AUTO_MASK). That said, simply fixing the unused
definition feels like the cleanest and least verbose option. What do
you think?

> If the masks are unused, how about just dropping the definitions instead
> (including the unused ADT7470_REG_PWM12_CFG and ADT7470_REG_PWM34_CFG) ?
> That could be done as first patch of the series.

I'm almost afraid of touching anything else here! Fixing bugs in this
driver feels like fighting a hydra: every time you address one, the
Sashiko bot sprouts two more. :-) I already have seven.

Regards,

Luiz

Re: [hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

[Guenter Roeck]

On 7/27/26 11:09, Luiz Angelo Daros de Luca wrote:
>> On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
>>> The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
>>> currently defined with swapped bit values.
>>>
>>> According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
>>> Configuration for register 0x69 follows the exact same bit position
>>> layout as register 0x68:
>>> - 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
>>> - 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
>>> - 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
>>> - 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40
>>>
>>> Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.
>>>
>>> This typo did not cause any functional bugs because these specific
>>> macros are never referenced in the driver code. Instead, the driver
>>> correctly applies the configuration by relying on the modulo parity of
>>> the channel index (e.g., `channel % 2`) to selectively apply either
>>> ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
>>> Since the bit layout is identical between the two configuration
>>> registers, the hardware is currently configured correctly.
>>>
>>> Fix the macro definitions to reflect the datasheet accurately and
>>> prevent future bugs or confusion during code review and refactoring.
>>> As this is a purely cosmetic fix with no functional impact, a backport
>>> to stable kernels is not necessary.
> 
> If you build with W=2, you indeed get tons of unused macros...
> 
> Initially, I intended to just drop them. However, just like the
> incorrect macro led Sashiko (and other bots) to assume the masks were
> swapped, keeping them correctly defined serves as useful context
> directly in the code. If we drop them entirely, anyone (or any AI
> agent/tool analyzing the driver) would need to fetch and parse the
> datasheet to verify the register layout. While the datasheet is the
> ultimate source of truth, reading PDF tables costs more than reading
> well-defined driver constants, to the point that agents might simply
> skip it.
> 
> We could certainly drop the unused macros and add a comment explaining
> the layout, or even rename them to better reflect their shared usage
> across channels (e.g., ADT7470_PWM_ODD_AUTO_MASK and
> ADT7470_PWM_EVEN_AUTO_MASK). That said, simply fixing the unused
> definition feels like the cleanest and least verbose option. What do
> you think?
> 
>> If the masks are unused, how about just dropping the definitions instead
>> (including the unused ADT7470_REG_PWM12_CFG and ADT7470_REG_PWM34_CFG) ?
>> That could be done as first patch of the series.
> 
> I'm almost afraid of touching anything else here! Fixing bugs in this
> driver feels like fighting a hydra: every time you address one, the
> Sashiko bot sprouts two more. :-) I already have seven.
> 
Good point. Normally a driver should only provide definitions which are
actually used, but for an old driver like this one ends up with a lot of
churn.

Note that you don't_ have_ to fix all the reported pre-existing issues.
It is very much appreciated if you do, and I tried to do it myself, but
nowadays I only fix (or try to fix) the issues marked as critical and/or
as security issues. All I really ask for is that patch authors fix all new
issues (and that they don't tell me that I should accept new issues because
the driver or some other driver is already buggy).

Thanks,
Guenter

Re: [hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

[Guenter Roeck]

On 7/27/26 12:00, Guenter Roeck wrote:
> On 7/27/26 11:09, Luiz Angelo Daros de Luca wrote:
>>> On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
>>>> The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
>>>> currently defined with swapped bit values.
>>>>
>>>> According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
>>>> Configuration for register 0x69 follows the exact same bit position
>>>> layout as register 0x68:
>>>> - 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
>>>> - 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
>>>> - 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
>>>> - 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40
>>>>
>>>> Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.
>>>>
>>>> This typo did not cause any functional bugs because these specific
>>>> macros are never referenced in the driver code. Instead, the driver
>>>> correctly applies the configuration by relying on the modulo parity of
>>>> the channel index (e.g., `channel % 2`) to selectively apply either
>>>> ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
>>>> Since the bit layout is identical between the two configuration
>>>> registers, the hardware is currently configured correctly.
>>>>
>>>> Fix the macro definitions to reflect the datasheet accurately and
>>>> prevent future bugs or confusion during code review and refactoring.
>>>> As this is a purely cosmetic fix with no functional impact, a backport
>>>> to stable kernels is not necessary.
>>
>> If you build with W=2, you indeed get tons of unused macros...
>>
>> Initially, I intended to just drop them. However, just like the
>> incorrect macro led Sashiko (and other bots) to assume the masks were
>> swapped, keeping them correctly defined serves as useful context
>> directly in the code. If we drop them entirely, anyone (or any AI
>> agent/tool analyzing the driver) would need to fetch and parse the
>> datasheet to verify the register layout. While the datasheet is the
>> ultimate source of truth, reading PDF tables costs more than reading
>> well-defined driver constants, to the point that agents might simply
>> skip it.
>>
>> We could certainly drop the unused macros and add a comment explaining
>> the layout, or even rename them to better reflect their shared usage
>> across channels (e.g., ADT7470_PWM_ODD_AUTO_MASK and
>> ADT7470_PWM_EVEN_AUTO_MASK). That said, simply fixing the unused
>> definition feels like the cleanest and least verbose option. What do
>> you think?
>>
>>> If the masks are unused, how about just dropping the definitions instead
>>> (including the unused ADT7470_REG_PWM12_CFG and ADT7470_REG_PWM34_CFG) ?
>>> That could be done as first patch of the series.
>>
>> I'm almost afraid of touching anything else here! Fixing bugs in this
>> driver feels like fighting a hydra: every time you address one, the
>> Sashiko bot sprouts two more. :-) I already have seven.
>>
> Good point. Normally a driver should only provide definitions which are
> actually used, but for an old driver like this one ends up with a lot of
> churn.
> 
> Note that you don't_ have_ to fix all the reported pre-existing issues.
> It is very much appreciated if you do, and I tried to do it myself, but
> nowadays I only fix (or try to fix) the issues marked as critical and/or
> as security issues. All I really ask for is that patch authors fix all new
> issues (and that they don't tell me that I should accept new issues because
> the driver or some other driver is already buggy).
> 
Having said that, and unless I am missing something, it looks like the
only two patches with new issues are the last two patches of the series.
I'll be happy to apply the series after you fixed those issues.

Thanks,
Guenter

[hwmon PATCH v3 5/6] hwmon: (adt7470) Fix temperature alarm logic in hwmon_temp_read()

[Luiz Angelo Daros de Luca]

During the conversion the alarm callback started interpreting the
channel index as an alarm bitmask, resulting in incorrect alarm
reporting. Compute the proper alarm bit instead.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/r/20260717211224.B9E291F000E9@smtp.kernel.org
Fixes: fc958a61ff6d ("hwmon: (adt7470) Convert to devm_hwmon_device_register_with_info API")'
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/hwmon/adt7470.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index a34635526aaa..f1150f0fe217 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -569,7 +569,7 @@ static int adt7470_temp_read(struct device *dev, u32 attr, int channel, long *va
 		*val = 1000 * data->temp_max[channel];
 		break;
 	case hwmon_temp_alarm:
-		*val = !!(data->alarm & channel);
+		*val = !!(data->alarm & BIT(channel));
 		break;
 	default:
 		return -EOPNOTSUPP;

-- 
2.55.0

Re: [hwmon PATCH v3 5/6] hwmon: (adt7470) Fix temperature alarm logic in hwmon_temp_read()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The bitmask calculation `BIT(channel)` for temperature alarms incorrectly maps channels 7, 8, and 9 due to a discontinuity at bit 7.
--

--- Patch [5]: [hwmon PATCH v3 5/6] hwmon: (adt7470) Fix temperature alarm logic in hwmon_temp_read() ---
commit ab0ed15b014e653c238da0d91110d0699630d8f7
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Fix temperature alarm logic in hwmon_temp_read()

This commit changes the temperature alarm read logic to use a bitmask based
on the channel index, attempting to fix an issue where the channel index
was previously interpreted directly as an alarm bitmask.

Link: https://lore.kernel.org/r/20260717211224.B9E291F000E9@smtp.kernel.org

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index a34635526aaa2..f1150f0fe2172 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -569,7 +569,7 @@ static int adt7470_temp_read(struct device *dev, u32 attr, int channel, long *va
>  		*val = 1000 * data->temp_max[channel];
>  		break;
>  	case hwmon_temp_alarm:
> -		*val = !!(data->alarm & channel);
> +		*val = !!(data->alarm & BIT(channel));

[Severity: High]
Does this bitwise calculation map correctly for higher channel indexes?

If bit 7 of the alarm state is the Out-of-Limit (OOL) global flag, it
appears channel 7 will erroneously report the OOL flag instead of its actual
alarm state.

Could this also cause an off-by-one mapping for the remaining channels,
where channel 8 reports channel 7's alarm and channel 9 reports channel 8's
alarm?

>  		break;
>  	default:
>  		return -EOPNOTSUPP;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=5

Re: [hwmon PATCH v3 5/6] hwmon: (adt7470) Fix temperature alarm logic in hwmon_temp_read()

[Luiz Angelo Daros de Luca]

> > +             *val = !!(data->alarm & BIT(channel));

> [Severity: High]
> Does this bitwise calculation map correctly for higher channel indexes?
>
> If bit 7 of the alarm state is the Out-of-Limit (OOL) global flag, it
> appears channel 7 will erroneously report the OOL flag instead of its actual
> alarm state.
>
> Could this also cause an off-by-one mapping for the remaining channels,
> where channel 8 reports channel 7's alarm and channel 9 reports channel 8's
> alarm?

Sashiko is right here:

       err = regmap_read(data->regmap, ADT7470_REG_ALARM1, &val);
       if (err < 0)
               return err;
       data->alarm = val;
       if (data->alarm & ADT7470_OOL_ALARM) {
               err = regmap_read(data->regmap, ADT7470_REG_ALARM2, &val);
               if (err < 0)
                       return err;
               data->alarm |= ALARM2(val);
       }

The BIT(7) is the hint to read ADT7470_REG_ALARM2. So the bitmap is
not a direct BIT(channel) for channels 8, 9, 10.
The data->alarm is also used for fan alarm:

        case hwmon_fan_alarm:
               *val = !!(data->alarm & (1 << (12 + channel)));
               break;

It is too confusing to use the same variable for two different uses
and it is exposing the HW bitmap instead of translating it.
I'll introduce temp_alarm and a fan_alarm and keep the bitmap simpler.

> Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=5

Regards,

Luiz

[hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value

[Luiz Angelo Daros de Luca]

adt7470_pwm_read() currently ignores failures returned by
pwm1_freq_get(). If the register read fails, the negative error code is
returned through *val while the function itself reports success,
potentially exposing a negative PWM frequency through sysfs.

Fix this by using the cached PWM frequency maintained by the driver,
eliminating the register access from the read path.

Apart from the corrected error propagation and using the cached value,
no functional change is intended.

Fixes: ef67959c4253 ("hwmon: (adt7470) Convert to use regmap")
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/hwmon/adt7470.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index f1150f0fe217..405eda900650 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -167,6 +167,7 @@ struct adt7470_data {
 	u8			pwm_min[ADT7470_PWM_COUNT];
 	s8			pwm_tmin[ADT7470_PWM_COUNT];
 	u8			pwm_auto_temp[ADT7470_PWM_COUNT];
+	u32			pwm_freq;
 
 	struct task_struct	*auto_update;
 	unsigned int		auto_update_interval;
@@ -741,7 +742,7 @@ static ssize_t force_pwm_max_store(struct device *dev,
 }
 
 /* These are the valid PWM frequencies to the nearest Hz */
-static const int adt7470_freq_map[] = {
+static const u32 adt7470_freq_map[] = {
 	11, 15, 22, 29, 35, 44, 59, 88, 1400, 22500
 };
 
@@ -781,7 +782,7 @@ static int adt7470_pwm_read(struct device *dev, u32 attr, int channel, long *val
 		*val = 1 + data->pwm_automatic[channel];
 		break;
 	case hwmon_pwm_freq:
-		*val = pwm1_freq_get(dev);
+		*val = data->pwm_freq;
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -817,6 +818,10 @@ static int pwm1_freq_set(struct device *dev, long freq)
 	err = regmap_update_bits(data->regmap, ADT7470_REG_CFG_2,
 				 ADT7470_FREQ_MASK,
 				 index << ADT7470_FREQ_SHIFT);
+	if (err < 0)
+		goto out;
+
+	data->pwm_freq = adt7470_freq_map[index];
 out:
 	mutex_unlock(&data->lock);
 
@@ -1270,6 +1275,7 @@ static int adt7470_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct adt7470_data *data;
 	struct device *hwmon_dev;
+	int freq_val;
 	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct adt7470_data), GFP_KERNEL);
@@ -1294,6 +1300,14 @@ static int adt7470_probe(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
+	freq_val = pwm1_freq_get(dev);
+	if (freq_val <= 0) {
+		err = freq_val < 0 ? freq_val : -EINVAL;
+		return err;
+	}
+
+	data->pwm_freq = (u32)freq_val;
+
 	/* Register sysfs hooks */
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
 							 &adt7470_chip_info,

-- 
2.55.0

Re: [hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incorrect PWM frequency is cached in `pwm1_freq_set()` for high frequencies.
--

--- Patch [6]: [hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value ---
commit 1d5eabde22a3c2244581f951eb2060da34244bbe
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Use cached PWM frequency value

This commit updates the driver to use a cached PWM frequency in the read
path instead of querying the hardware. It aims to prevent exposing negative
error codes through sysfs if the register read fails.

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index f1150f0fe2172..405eda900650f 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c

[ ... ]

> @@ -817,6 +818,10 @@ static int pwm1_freq_set(struct device *dev, long freq)
>  	err = regmap_update_bits(data->regmap, ADT7470_REG_CFG_2,
>  				 ADT7470_FREQ_MASK,
>  				 index << ADT7470_FREQ_SHIFT);
> +	if (err < 0)
> +		goto out;
> +
> +	data->pwm_freq = adt7470_freq_map[index];

[Severity: Medium]
Is the correct index used for caching the frequency here?

Looking at pwm1_freq_set(), when setting a high frequency like 1400Hz or 
22500Hz, the initial index is resolved to 8 or 9. The code then modifies 
this by subtracting 8 (index -= 8) before updating the hardware registers.

By using this modified index to update data->pwm_freq, does the driver end
up caching 11Hz or 15Hz (indexes 0 and 1) instead of the requested high
frequency?

>  out:
>  	mutex_unlock(&data->lock);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-adt7470_thermalzone-v3-0-21424d550c3d@gmail.com?part=6

Re: [hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value

[Luiz Angelo Daros de Luca]

> By using this modified index to update data->pwm_freq, does the driver end
> up caching 11Hz or 15Hz (indexes 0 and 1) instead of the requested high
> frequency?

Yes. I'll fix it.

Regards,

Luiz

Re: [hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value

[Guenter Roeck]

On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
> adt7470_pwm_read() currently ignores failures returned by
> pwm1_freq_get(). If the register read fails, the negative error code is
> returned through *val while the function itself reports success,
> potentially exposing a negative PWM frequency through sysfs.
> 
> Fix this by using the cached PWM frequency maintained by the driver,
> eliminating the register access from the read path.
> 
> Apart from the corrected error propagation and using the cached value,
> no functional change is intended.
> 

An alternative might be to use regmap for caching and drop all driver-internal
caching. I have done that for other drivers; typically it simplifies the code,
sometimes significantly, since the update_device() function is no longer needed.
Something to consider for later, maybe.

Thanks,
Guenter

Re: [hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value

[Luiz Angelo Daros de Luca]

Em seg., 27 de jul. de 2026 às 11:33, Guenter Roeck
<linux@roeck-us.net> escreveu:
>
> On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
> > adt7470_pwm_read() currently ignores failures returned by
> > pwm1_freq_get(). If the register read fails, the negative error code is
> > returned through *val while the function itself reports success,
> > potentially exposing a negative PWM frequency through sysfs.
> >
> > Fix this by using the cached PWM frequency maintained by the driver,
> > eliminating the register access from the read path.
> >
> > Apart from the corrected error propagation and using the cached value,
> > no functional change is intended.
> >
>
> An alternative might be to use regmap for caching and drop all driver-internal
> caching. I have done that for other drivers; typically it simplifies the code,
> sometimes significantly, since the update_device() function is no longer needed.
> Something to consider for later, maybe.

That definitely sounds like a nice upgrade to consider!

I haven't used regmap's caching feature before, so I'm not entirely
sure how cache expiration is typically handled. In adt7470, caching is
mainly used for volatile readings because hardware access over
SMBus/I2C is quite slow. On my device (which might be an extreme
case), reading all registers can take over a second.

What would be the recommended strategy here? Would it be caching all
registers and periodically calling regcache_drop_region() (or mark
them volatile) to force a refresh?

Regards,

Luiz

Re: [hwmon PATCH v3 6/6] hwmon: (adt7470) Use cached PWM frequency value

[Guenter Roeck]

On 7/27/26 11:44, Luiz Angelo Daros de Luca wrote:
> Em seg., 27 de jul. de 2026 às 11:33, Guenter Roeck
> <linux@roeck-us.net> escreveu:
>>
>> On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
>>> adt7470_pwm_read() currently ignores failures returned by
>>> pwm1_freq_get(). If the register read fails, the negative error code is
>>> returned through *val while the function itself reports success,
>>> potentially exposing a negative PWM frequency through sysfs.
>>>
>>> Fix this by using the cached PWM frequency maintained by the driver,
>>> eliminating the register access from the read path.
>>>
>>> Apart from the corrected error propagation and using the cached value,
>>> no functional change is intended.
>>>
>>
>> An alternative might be to use regmap for caching and drop all driver-internal
>> caching. I have done that for other drivers; typically it simplifies the code,
>> sometimes significantly, since the update_device() function is no longer needed.
>> Something to consider for later, maybe.
> 
> That definitely sounds like a nice upgrade to consider!
> 
> I haven't used regmap's caching feature before, so I'm not entirely
> sure how cache expiration is typically handled. In adt7470, caching is
> mainly used for volatile readings because hardware access over
> SMBus/I2C is quite slow. On my device (which might be an extreme
> case), reading all registers can take over a second.
> 

regmap caching does not time out. Registers are either cached or not.
Usually that works just fine because there are not that many volatile
registers in a chip. Most are limits which can be cached forever.

> What would be the recommended strategy here? Would it be caching all
> registers and periodically calling regcache_drop_region() (or mark
> them volatile) to force a refresh?
> 

What I usually do is to just mark the volatile registers in regmap,
and drop all driver-internal caching. I would not see the point of
re-reading limit registers.

There is still a case-by-case decision to be made for cacheable
values which would need excessive calculations (more than just mapping)
if read from a cached register value.

I'd say never mind though - you are right, the driver has a number
of issues, and Sashiko is likely to keep finding more.

Guenter

Re: [hwmon PATCH v3 0/6] hwmon: (adt7470): Multiple fixes

[Luiz Angelo Daros de Luca]

Hello,

The b4 tool mixes the versions with the previous submission from which
these patches originated:
https://patch.msgid.link/20260717-adt7470_thermalzone-v2-0-a55147958fad@gmail.com

Therefore, this is actually v1. Sorry for the noise.

Regards,

Luiz

Re: [hwmon PATCH v3 0/6] hwmon: (adt7470): Multiple fixes

[Guenter Roeck]

On 7/26/26 20:44, Luiz Angelo Daros de Luca wrote:
> Hello,
> 
> The b4 tool mixes the versions with the previous submission from which
> these patches originated:
> https://patch.msgid.link/20260717-adt7470_thermalzone-v2-0-a55147958fad@gmail.com
> 
> Therefore, this is actually v1. Sorry for the noise.
> 
No worries. Thanks for letting me know.

Guenter