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

[Ji-Ze Hong (Peter Hong)]

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

Thanks
--
With Best Regards,
Peter Hong