Re: [PATCH v2] watchdog: f71808e_wdt: Add F81865 support

[Knud Poulsen <knpo@ieee.org>]

On 2016-04-22 15:59, Guenter Roeck wrote:
> On 04/22/2016 06:51 AM, Guenter Roeck wrote:
>> On 04/22/2016 04:48 AM, Knud Poulsen wrote:
>>> Adds watchdog support for Fintek F81865 Super-IO chip to
>>> Fintek wdt driver (f71808e_wdt)
>>>
>>> Tested and verified on Lanner LEC-3030 Industrial PC
>>>
>>> Datasheet references:
>>> http://www.hardwaresecrets.com/datasheets/F81865_V028P.pdf
>>> http://www.alldatasheet.com/datasheet-pdf/pdf/406317/FINTEK/F81865.html
>>>
>>> Signed-off-by: Knud Poulsen <knpo@ieee.org>
>>> ---
>>>   drivers/watchdog/f71808e_wdt.c | 31 +++++++++++++++++++++++++++----
>>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
>>> index 016bd93..4133cd3 100644
>>> --- a/drivers/watchdog/f71808e_wdt.c
>>> +++ b/drivers/watchdog/f71808e_wdt.c
>>> @@ -59,6 +59,7 @@
>>>   #define SIO_F71869A_ID        0x1007    /* Chipset ID */
>>>   #define SIO_F71882_ID        0x0541    /* Chipset ID */
>>>   #define SIO_F71889_ID        0x0723    /* Chipset ID */
>>> +#define SIO_F81865_ID        0x0704    /* Chipset ID */
>>>
>>>   #define F71808FG_REG_WDO_CONF        0xf0
>>>   #define F71808FG_REG_WDT_CONF        0xf5
>>> @@ -71,6 +72,10 @@
>>>   #define F71808FG_FLAG_WD_PULSE        4
>>>   #define F71808FG_FLAG_WD_UNIT        3
>>>
>>> +#define F81865_REG_WDO_CONF        0xfa
>>> +#define F81865_FLAG_WDOUT_EN        0
>>> +#define F81865_FLAG_WDTMOUT_STS        6
>>> +
> [ ... ]
> 
>>>
>>>       wdt_conf = superio_inb(sioaddr, F71808FG_REG_WDT_CONF);
>>> -    watchdog.caused_reboot = wdt_conf & F71808FG_FLAG_WDTMOUT_STS;
>>> +
>>> +    if (watchdog.type == f81865)
>>> +        watchdog.caused_reboot = wdt_conf & F81865_FLAG_WDTMOUT_STS;
>>> +    else
>>> +        watchdog.caused_reboot = wdt_conf & F71808FG_FLAG_WDTMOUT_STS;
>>>
>> Are you sure this one is correct ? The bit configuration is exactly the same
>> as with other chips supported by this driver. Actually, I don't see an indication
>> in any of the datasheets I have which would suggest that reading bit 5 means
>> that the last reboot occurred due to a watchdog timeout. As far as I can see,
>> it just means that the watchdog is enabled.
>>
>> Am I missing something here ?
>>
> After another look, I am even more confused. F81865_FLAG_WDTMOUT_STS and
> F71808FG_FLAG_WDTMOUT_STS are bit maps, not bits.
> 
> 5 = 0b0101, 6 = 0b0110. Bit 0/1 select the pulse width, bit 2 selects
> the output polarity of RSTOUT. What does this have to do with the reboot cause ?

You're right, great catch!!, I didn't grok the bit-map, not bit-index part here
and got thrown by the 6 vs 5 in the datasheet vs the existing code, *Doh*

ps3 en route.

BR,
Knud

> 
> Thanks,
> Guenter
>