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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added `nct6126` chip is missing from the `switch (chip)` statement in `w83627hf_init()`, causing critical hardware watchdog initialization (WDTO# output routing) to be skipped.
- [Low] The macro `NCT6116_ID` is redundantly defined twice in a row with the identical value.
--

commit 0f99926a9f6fc9f6f9e5246a88211f0c8ad46b5b
Author: Paul Louvel <[email protected]>

watchdog: w83627hf_wdt: Add support for Nuvoton NCT6126

Add support for the hardware watchdog integrated in the Nuvoton NCT6126
Super I/O chip. This distinguishes the NCT6126 from the NCT6116 by reading
the low byte of the chip ID.

> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index bc33b63c5a5df..9533baa453841 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
> @@ -49,7 +49,7 @@ static int wdt_cfg_leave = 0xAA;/* key to lock configuration space */
>  enum chips { w83627hf, w83627s, w83697hf, w83697ug, w83637hf, w83627thf,
>  	     w83687thf, w83627ehf, w83627dhg, w83627uhg, w83667hg, w83627dhg_p,
>  	     w83667hg_b, nct6775, nct6776, nct6779, nct6791, nct6792, nct6793,
> -	     nct6795, nct6796, nct6102, nct6116 };
> +	     nct6795, nct6796, nct6102, nct6116, nct6126 };
>  

[Severity: High]
Does this code miss handling the new nct6126 chip in w83627hf_init()?

When w83627hf_init() initializes the watchdog device, it uses a switch
statement on the chip type. Because nct6126 is omitted from the switch cases
in w83627hf_init(), it falls through to the default case:

drivers/watchdog/w83627hf_wdt.c:w83627hf_init() {
        ...
        switch (chip) {
        ...
        case nct6116:
                ...
                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;
        }
}

Without nct6126 being added alongside nct6116, the WDTO# output initialization
is skipped. Will this cause the watchdog timer to count down and expire
without successfully triggering a physical system reset?

[ ... ]

> @@ -95,6 +95,9 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
>  #define NCT6776_ID		0xc3
>  #define NCT6102_ID		0xc4
>  #define NCT6116_ID		0xd2
> +#define NCT6116_ID		0xd2	/* also NCT6126D */

[Severity: Low]
This isn't a bug, but NCT6116_ID is now defined twice with the exact same
value. Was the intention to just add a comment to the existing definition
instead of redefining it entirely?

> +#define NCT6126_VER_A_LOW_ID	0x83	/* ... version A */
> +#define NCT6126_VER_B_LOW_ID	0x84	/* ... version B */
>  #define NCT6779_ID		0xc5

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