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

[Paul Louvel]

On Tue Oct 6, 2026 at 4:24 PM CEST, Tzung-Bi Shih wrote:
> 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 */
>

Maybe I can just drop these comments, they are not really useful.

>> @@ -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.

This is already done in the next patch.

Thanks,

-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com