Re: [PATCH v3 1/6] watchdog: w83627hf_wdt: Replace magic numbers with descriptive macros

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Sun, Oct 04, 2026 at 02:12:49PM +0200, Paul Louvel wrote:
> @@ -71,6 +72,12 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
>   *	Kernel methods.
>   */
>  
> +#define SIO_REG_LDSEL		0x07	/* Logical device select */
> +#define SIO_REG_DEVID		0x20	/* Device ID (1 or 2 bytes) */
> +#define SIO_REG_ENABLE		0x30	/* Logical device enable */
> +#define SIO_REG_CONF_ADDR0	0x2E
> +#define SIO_REG_CONF_ADDR1	0x4E

They are actually I/O ports rather than SIO registers.  SIO_PORT_2E and
SIO_PORT_4E (or SIO_CONF_PORT_*) would be better names to distinguish them
from SIO_REG_*.

> @@ -248,16 +258,17 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>  		}
>  	}
>  
> -	/* set second mode & disable keyboard turning off watchdog */
> -	t = superio_inb(cr_wdt_control) & ~0x0C;
> +	/* set second mode & disable keyboard reset turning off watchdog */

This is confusing.  How about:

    /* set second mode & disable watchdog reload on keyboard reset */

> +	t = superio_inb(cr_wdt_control) &
> +	    ~(WDT_CTRL_MINUTE_MODE | WDT_CTRL_RISING_EDGE_KBD_RESET);
>  	superio_outb(cr_wdt_control, t);
>  
>  	t = superio_inb(cr_wdt_csr);
>  	if (t & WDT_CSR_STATUS)
>  		wdog->bootstatus |= WDIOF_CARDRESET;
>  
> -	/* reset status, disable keyboard & mouse turning off watchdog */
> -	t &= ~(WDT_CSR_STATUS | WDT_CSR_KBD | WDT_CSR_MOUSE);
> +	/* reset status, disable keyboard & mouse interrupt turning off watchdog */

Same here.  How about:

    /* reset status & disable watchdog reload on keyboard/mouse interrupts */

> @@ -513,11 +524,11 @@ static int __init wdt_init(void)
>  	/* Apply system-specific quirks */
>  	dmi_check_system(wdt_dmi_table);
>  
> -	wdt_io = 0x2e;
> -	chip = wdt_find(0x2e);
> +	wdt_io = SIO_REG_CONF_ADDR0;
> +	chip = wdt_find(SIO_REG_CONF_ADDR0);
>  	if (chip < 0) {
> -		wdt_io = 0x4e;
> -		chip = wdt_find(0x4e);
> +		wdt_io = SIO_REG_CONF_ADDR1;
> +		chip = wdt_find(SIO_REG_CONF_ADDR1);

I noticed the 'addr' parameter in wdt_find() is unused.  We could consider
removing it in a later cleanup patch.