Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation
To:     Guenter Roeck <linux@roeck-us.net>, wim@linux-watchdog.orgw
Cc:     linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
References: <1553225816-24375-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <af4fba18-da99-2b48-7bbe-d59222081ab4@roeck-us.net>
From:   "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Message-ID: <cf37ee27-593c-b809-62d4-8d38265cb710@gmail.com>
Date:   Mon, 25 Mar 2019 14:10:15 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.3
MIME-Version: 1.0
In-Reply-To: <af4fba18-da99-2b48-7bbe-d59222081ab4@roeck-us.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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