Re: [PATCH v2 1/2] watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing hardware reset initialization for the NCT6126 watchdog chip.
--

commit e0ddc629265e45d1982854f304eefd4e2a86e9dc
Author: Paul Louvel <[email protected]>

watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126

This commit adds support for the Nuvoton NCT6126 hardware watchdog to the
w83627hf_wdt driver. It differentiates the NCT6126 from the NCT6116 by
checking the low byte of the chip ID.

> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index bc33b63c5a5df..1f649fda8a813 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c

[ ... ]

> @@ -428,7 +430,12 @@ static int wdt_find(int addr)
>  		cr_wdt_csr = NCT6102D_WDT_CSR;
>  		break;
>  	case NCT6116_ID:
> -		ret = nct6116;
> +		val = superio_inb(0x21);
> +		if (val == NCT6126_VER_A_LOW_ID || val == NCT6126_VER_B_LOW_ID)
> +			ret = nct6126;

[Severity: High]
Does this code miss updating the switch statement in w83627hf_init() to
include nct6126?

Looking at w83627hf_init(), the other NCT chips are explicitly handled to
enable the WDTO# output pulse:

drivers/watchdog/w83627hf_wdt.c:w83627hf_init() {
	...
	case nct6102:
	case nct6116:
		/*
		 * These chips have a fixed WDTO# output pin (W83627UHG),
		 * or support more than one WDTO# output pin.
		 * Don't touch its configuration, and hope the BIOS
		 * does the right thing.
		 */
		t = superio_inb(cr_wdt_control);
		t |= 0x02;	/* enable the WDTO# output low pulse
				 * to the KBRST# pin */
		superio_outb(cr_wdt_control, t);
		break;
	default:
		break;
	...
}

If nct6126 isn't added to this switch statement, it will fall through to the
default case.  Will this cause the chip to skip enabling the reset logic,
resulting in the watchdog timing out without physically resetting the system?

> +		else
> +			ret = nct6116;
> +
>  		cr_wdt_timeout = NCT6102D_WDT_TIMEOUT;
>  		cr_wdt_control = NCT6102D_WDT_CONTROL;
>  		cr_wdt_csr = NCT6102D_WDT_CSR;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1