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

On 7/6/26 03:29, Paul Louvel wrote:
> Add support for the hardware watchdog integrated in the Nuvoton NCT6126
> Super I/O chip. This device is used on a number of x86 single-board
> computers and is compatible with the w83627hf_wdt driver.
> 
> Unlike most supported chips, the NCT6126 shares the same high-byte chip
> ID as the NCT6116. Read the low byte of the chip ID as well to
> distinguish between the two devices and identify the NCT6126 correctly.
> 

Doesn't that (and the code below) mean that the driver already supports
the chip, only it identifies it as NCT6116 instead of NCT6126 ?

This means that the commit message is misleading: It should say that
the chip is already supported but misidentified, and that the added
code helps to correctly identify the chip.

Guenter

> Signed-off-by: Paul Louvel <[email protected]>
> ---
>   drivers/watchdog/w83627hf_wdt.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index bc33b63c5a5d..a6dfa9d01702 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 };
>   
>   static int timeout;			/* in seconds */
>   module_param(timeout, int, 0);
> @@ -94,7 +94,9 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
>   #define NCT6775_ID		0xb4
>   #define NCT6776_ID		0xc3
>   #define NCT6102_ID		0xc4
> -#define NCT6116_ID		0xd2
> +#define NCT6116_ID		0xd2	/* also NCT6126D */
> +#define NCT6126_VER_A_LOW_ID	0x83	/* ... version A */
> +#define NCT6126_VER_B_LOW_ID	0x84	/* ... version B */
>   #define NCT6779_ID		0xc5
>   #define NCT6791_ID		0xc8
>   #define NCT6792_ID		0xc9
> @@ -217,6 +219,7 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>   	case nct6796:
>   	case nct6102:
>   	case nct6116:
> +	case nct6126:
>   		/*
>   		 * These chips have a fixed WDTO# output pin (W83627UHG),
>   		 * or support more than one WDTO# output pin.
> @@ -428,7 +431,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;
> +		else
> +			ret = nct6116;
> +
>   		cr_wdt_timeout = NCT6102D_WDT_TIMEOUT;
>   		cr_wdt_control = NCT6102D_WDT_CONTROL;
>   		cr_wdt_csr = NCT6102D_WDT_CSR;
> @@ -499,6 +507,7 @@ static int __init wdt_init(void)
>   		"NCT6796",
>   		"NCT6102",
>   		"NCT6116",
> +		"NCT6126"
>   	};
>   
>   	/* Apply system-specific quirks */
>