Re: [PATCH] hwmon: (nct6775) Add support for NCT5525

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwmon: (nct6775) nct6775_reg_is_word_sized() corrupts NCT5525 target regs
- [Low] hwmon: (nct6775) no-op fan_debounce handling in nct6775_platform_probe_init()
--

commit b2c50b829c2c187ba7bd478ebf5b2095ec0b4f6d
Author: Anton Lundin <glance@ac2.se>

hwmon: (nct6775) Add support for NCT5525

This commit adds hardware monitoring support for the Nuvoton NCT5525D
chip to the nct6775 core, platform, and i2c drivers, as well as the
device tree bindings and documentation.

> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
> index cf27b9c7a89ed..fb99bdf16764a 100644
> --- a/drivers/hwmon/nct6775-core.c
> +++ b/drivers/hwmon/nct6775-core.c

[ ... ]

> @@ -1191,6 +1224,10 @@ bool nct6775_reg_is_word_sized(struct nct6775_data *data, u16 reg)
>  		  (reg >= 0x59 && reg < 0x69 && (reg & 1)) ||
>  		  reg == 0xe0 || reg == 0xe2 || reg == 0xe4 ||
>  		  reg == 0x111 || reg == 0x121 || reg == 0x131;
> +	case nct5525:
> +		return reg == 0x20 || reg == 0x22 || reg == 0x28 ||
> +		  reg == 0x59 || reg == 0x5b || reg == 0xe0 || reg == 0xe2 ||
> +		  reg == 0xe8 || reg == 0x111 || reg == 0x121;

[Severity: Medium]
Should registers 0x111 and 0x121 be marked as word-sized in
nct6775_reg_is_word_sized() for nct5525?

In drivers/hwmon/nct6775-core.c, nct6775_probe() configures nct5525 with:

	data->target_temp_mask = 0xff;
	data->REG_TARGET = NCT6106_REG_TARGET;
	data->REG_TOLERANCE_H = NCT6106_REG_TOLERANCE_H;

where NCT6106_REG_TARGET is { 0x111, 0x121, 0x131 } and
NCT6106_REG_TOLERANCE_H is { 0x112, 0x122, 0x132 }.

When 0x111 and 0x121 are marked as word-sized, nct6775_read_value() performs
a 16-bit big-endian read across reg (high byte) and reg + 1 (low byte), and
nct6775_write_value() writes value >> 8 to reg and value & 0xff to reg + 1.

However, nct6775_update_pwm_limits() and pwm_update_registers() treat
REG_TARGET and REG_TOLERANCE_H as separate 8-bit registers:

drivers/hwmon/nct6775-core.c:nct6775_update_pwm_limits() {
	...
	err = nct6775_read_value(data, data->REG_TARGET[i], &reg_t);
	if (err)
		return err;

	/* Update only in matching mode or if never updated */
	if (!data->target_temp[i] ||
	    data->pwm_enable[i] == thermal_cruise)
		data->target_temp[i] = reg_t & data->target_temp_mask;
	if (!data->target_speed[i] ||
	    data->pwm_enable[i] == speed_cruise) {
		if (data->REG_TOLERANCE_H) {
			err = nct6775_read_value(data, data->REG_TOLERANCE_H[i], &reg);
			if (err)
				return err;
			reg_t |= (reg & 0x0f) << 8;
		}
		data->target_speed[i] = reg_t;
	}
	...
}

And on write in pwm_update_registers():

drivers/hwmon/nct6775-core.c:pwm_update_registers() {
	...
	case speed_cruise:
		err = nct6775_write_value(data, data->REG_TARGET[nr],
					  data->target_speed[nr] & 0xff);
		...
	case thermal_cruise:
		err = nct6775_write_value(data, data->REG_TARGET[nr], data->target_temp[nr]);
	...
}

Because REG_TARGET[i] is read as a 16-bit big-endian value spanning 0x111
and 0x112, masking reg_t with target_temp_mask (0xff) discards byte 0x111
and reads byte 0x112 (REG_TOLERANCE_H) as the target temperature. Similarly,
writing an 8-bit value to REG_TARGET[nr] in pwm_update_registers() writes
0x00 to 0x111/0x121 and overwrites 0x112/0x122 with the low byte.

> diff --git a/drivers/hwmon/nct6775-platform.c b/drivers/hwmon/nct6775-platform.c
> index d359c28f3e0f3..3bc7a27d58a60 100644
> --- a/drivers/hwmon/nct6775-platform.c
> +++ b/drivers/hwmon/nct6775-platform.c

[ ... ]

> @@ -919,6 +926,8 @@ static int nct6775_platform_probe_init(struct nct6775_data *data)
>  		case nct6126:
>  			tmp |= 0xe0;
>  			break;
> +		case nct5525:
> +			break;
>  		case nct6775:
>  			tmp |= 0x1e;
>  			break;

[Severity: Low]
In drivers/hwmon/nct6775-platform.c, when fan_debounce is enabled in
nct6775_platform_probe_init(), the nct5525 case breaks without setting any
bits in tmp, and then falls through to write the unchanged tmp value back to
NCT6775_REG_CR_FAN_DEBOUNCE and log "Enabled fan debounce for chip ...":

drivers/hwmon/nct6775-platform.c:nct6775_platform_probe_init() {
	...
		sio_data->sio_outb(sio_data, NCT6775_REG_CR_FAN_DEBOUNCE,
			     tmp);
		pr_info("Enabled fan debounce for chip %s\n", data->name);
	}
}

Should nct5525 either set the appropriate debounce bits in tmp or skip
writing NCT6775_REG_CR_FAN_DEBOUNCE and logging that fan debounce was
enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103132948.144216-1-glance@ac2.se?part=1