Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

From: Guenter Roeck
Date: Mon Mar 25 2019 - 06:29:48 EST


On 3/24/19 11:10 PM, Ji-Ze Hong (Peter Hong) wrote:
Guenter Roeck æ 2019/3/22 äå 09:06 åé:
On 3/21/19 8:36 PM, Ji-Ze Hong (Peter Hong) wrote:
Fix error bit operation in watchdog_start()


Hmm ... does that mean it never worked ? Did you test it this time ?

Sorry for lacking test procedure. I had only test the functional (reset)
, not to test the register value.

The F81866 PIN70 (WDTRST#/GPIO15) is default set to WDTRST# function and
the old code only change register 27h bit(4) - PORT_4E_EN.

If the mainboard entry port is 4Eh, the old code is equal to nothing
done, but when the mainboard entry port is 2Eh, this code will make the
change from entry port 2Eh to 4Eh.

https://html.alldatasheet.com/html-pdf/459086/FINTEK/F81866AD/26531/119/F81866AD.html

A secondary concern is that the watchdog doesn't _have_ to trigger WDTRST,
but may also trigger PWOK. But that may be a separate issue.

Out watchdog is only support WDTRST#.

Please add:

Fixes: 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")

OK, I'll add to v2

diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
index 9a1c761258ce..9129485732c7 100644
--- a/drivers/watchdog/f71808e_wdt.c
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -387,18 +387,17 @@ static int watchdog_start(void)
ÂÂÂÂÂ case f81866:
ÂÂÂÂÂÂÂÂÂ /* Set pin 70 to WDTRST# */
-ÂÂÂÂÂÂÂ superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(3) | BIT(0));
-ÂÂÂÂÂÂÂ superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(2));
+ÂÂÂÂÂÂÂ superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 3);
+ÂÂÂÂÂÂÂ superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 0);
+ÂÂÂÂÂÂÂ superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 2);

Better use superio_inb()/superio_outb(). The above is (much) more expensive,
and we have no real idea what the impact of changing one bit at a time may be.

Could I add a superio_mask_write(reg, mask, data) with v2 patch like
following fintek driver ?
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/8250/8250_fintek.c#L113


If you wish. but not necessary. For the fix it would be better to have only the fix.
You could introduce the function in a second patch and use it wherever read/modify/write
of a mask is used in the driver (I think it was used at least in one other place).

Guenter