Re: [PATCH v5 5/5] watchdog: w83627hf_wdt: Add minute mode counting

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

On Tue, Oct 20, 2026 at 10:11:58AM +0200, Paul Louvel wrote:
> @@ -258,21 +263,10 @@ static int w83627hf_init(struct watchdog_device *wdog, enum chips chip)
>  		break;
>  	}
>  
> -	t = superio_inb(data->sio->addr, data->reg->timeout);
> -	if (t != 0) {
> -		if (early_disable) {
> -			pr_warn("Stopping previously enabled watchdog until userland kicks in\n");
> -			superio_outb(data->sio->addr, data->reg->timeout, 0);
> -		} else {
> -			pr_info("Watchdog already running. Resetting timeout to %d sec\n",
> -				wdog->timeout);
> -			superio_outb(data->sio->addr, data->reg->timeout, wdog->timeout);
> -			set_bit(WDOG_HW_RUNNING, &wdog->status);
> -		}
> -	}
> +	data->early_timer_val = superio_inb(data->sio->addr, data->reg->timeout);

I failed to figure out why this block needs to move to probe().  It looks
equivalent to me while introducing a redundant `early_timer_val`.

>  	t = superio_inb(data->sio->addr, data->reg->control) &
> -	    ~(WDT_CTRL_MINUTE_MODE | WDT_CTRL_RISING_EDGE_KBD_RESET);
> +	    ~WDT_CTRL_RISING_EDGE_KBD_RESET;

Why this change?  Wouldn't wdt_set_time() overwrite WDT_CTRL_MINUTE_MODE
anyway?

> @@ -297,6 +292,15 @@ static int wdt_set_time(struct watchdog_device *wdog, unsigned int timeout)
>  		return ret;
>  
>  	superio_select(data->sio->addr, W83627HF_LD_WDT);
> +
> +	ctrl = superio_inb(data->sio->addr, data->reg->control);
> +
> +	if (data->minute_mode)
> +		ctrl |= WDT_CTRL_MINUTE_MODE;
> +	else
> +		ctrl &= ~WDT_CTRL_MINUTE_MODE;
> +
> +	superio_outb(data->sio->addr, data->reg->control, ctrl);

It doesn't need to set `data->reg->control` if `!timeout`.

> @@ -305,7 +309,9 @@ static int wdt_set_time(struct watchdog_device *wdog, unsigned int timeout)
>  
>  static int wdt_start(struct watchdog_device *wdog)
>  {
> -	return wdt_set_time(wdog, wdog->timeout);
> +	struct w83627hf_data *data = watchdog_get_drvdata(wdog);
> +
> +	return wdt_set_time(wdog, data->timer_val);
>  }
>  
>  static int wdt_stop(struct watchdog_device *wdog)
> @@ -315,6 +321,17 @@ static int wdt_stop(struct watchdog_device *wdog)
>  
>  static int wdt_set_timeout(struct watchdog_device *wdog, unsigned int timeout)
>  {
> +	struct w83627hf_data *data = watchdog_get_drvdata(wdog);
> +
> +	if (timeout > 255) {
> +		data->minute_mode = true;
> +		data->timer_val = DIV_ROUND_UP(timeout, 60);
> +		timeout = data->timer_val * 60;
> +	} else {
> +		data->minute_mode = false;
> +		data->timer_val = timeout;
> +	}
> +
>  	wdog->timeout = timeout;

Can't it just use `wdog->timeout` instead of introducing a new
`data->timer_val`?

> @@ -332,6 +349,8 @@ static unsigned int wdt_get_time(struct watchdog_device *wdog)
>  
>  	superio_select(data->sio->addr, W83627HF_LD_WDT);
>  	timeleft = superio_inb(data->sio->addr, data->reg->timeout);
> +	if (data->minute_mode)
> +		timeleft *= 60;
>  	superio_exit(data->sio->addr, data->sio->cfg_leave);

The scaling can move out of the superio_* block.