Messages in this thread | | | Subject | Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation | From | Guenter Roeck <> | Date | Mon, 25 Mar 2019 03:29:43 -0700 |
| |
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
| |