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

[Guenter Roeck <linux@roeck-us.net>]

On 7/25/26 03:29, Paul Louvel wrote:
> Raw hex values for Super I/O register addresses are difficult to understand
> without constantly consulting the datasheet. Use named macros instead.
> 

This is not the only change. The patch also introduces the use of BIT().

> Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
> ---
>   drivers/watchdog/w83627hf_wdt.c | 43 ++++++++++++++++++++++++++---------------
>   1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index db77599e43a0..1529a4e16820 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
> @@ -27,6 +27,7 @@
>   
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   
> +#include <linux/bits.h>
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
>   #include <linux/types.h>
> @@ -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
> +
>   #define WDT_EFER (wdt_io+0)   /* Extended Function Enable Registers */
>   #define WDT_EFIR (wdt_io+0)   /* Extended Function Index Register
>   							(same as EFER) */
> @@ -115,9 +122,12 @@ MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=0)");
>   #define W836X7HF_WDT_CSR	0xf7
>   #define NCT6102D_WDT_CSR	0xf2
>   
> -#define WDT_CSR_STATUS		0x10
> -#define WDT_CSR_KBD		0x40
> -#define WDT_CSR_MOUSE		0x80
> +#define WDT_CSR_STATUS		BIT(4)
> +#define WDT_CSR_KBD_INT_RESET	BIT(6)
> +#define WDT_CSR_MOUSE_INT_RESET	BIT(7)
> +
> +#define WDT_CTRL_RISING_EDGE_KBD_RESET	BIT(2)
> +#define WDT_CTRL_MINUTE_MODE		BIT(3)
>   
>   static void superio_outb(int reg, int val)
>   {
> @@ -144,7 +154,7 @@ static int superio_enter(void)
>   
>   static void superio_select(int ld)
>   {
> -	superio_outb(0x07, ld);
> +	superio_outb(SIO_REG_LDSEL, ld);
>   }
>   
>   static void superio_exit(void)
> @@ -165,9 +175,9 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>   	superio_select(W83627HF_LD_WDT);
>   
>   	/* set CR30 bit 0 to activate GPIO2 */
> -	t = superio_inb(0x30);
> +	t = superio_inb(SIO_REG_ENABLE);
>   	if (!(t & 0x01))
> -		superio_outb(0x30, t | 0x01);
> +		superio_outb(SIO_REG_ENABLE, t | 0x01);

Address 0x30 is always the enable register for a logical device (here: the watchdog
logical device plus for some of the chips some GPIO information). Bit 0 enables the
watchdog logical device.

Guenter