Mailing List Archive

[PATCHv3 0/3] watchdog: add f71862fg support
Hi everyone,

this patch adds support for the watchdog included in Fintek f71862fg
Super-I/O chip to the f71808e_wdt driver and adds WDIOC_GETTIMELEFT
ioctl which i found very helpful.

This patch comes in three parts:
1/3: add f71862fg support
2/3: add WDIOC_GETTIMELEFT ioctl and helper function
3/3: clean up/replace some magic numbers/constants

changelog
---------

PATCHv2:
* module parameter for WDTRST# pin configuration added
* checking/printk of WDTRST# pin config
* removing int typecast

NOTE: due to an unprobable chain of events (sort of ineffable BOB stuff)
there wasn't posted more than PATCHv2 0/3 - sorry for the confusion.


PATCHv3:
* pin parameter check and pin configuration moved to helper function
* cleaned up a little more

Furthermore i was thinking about modifying the WDIOC_GETTIMELEFT
helper function since it simply returns the content of the WDT timer
register without regarding whether the driver runs in minute-mode or not
(thanks to Giel for pointing that out). This means if the driver is loaded
with a timeout of >255 seconds (and switches to minute-mode) WDIOC_GETTIMELEFT
ioctl returns minutes instead of seconds and thus doesn't comply with
watchdog-api (e.g. ioctl returns 6(min) not 360(sec)). It might be
confusing if one loads the driver with a timeout of let's say 360 seconds
and gets 6 minutes returned by ioctl. But imho it's not very accurate to
return a value of seconds if the wdt doesn't count in seconds. That might
be even more confusing. This inconsistency on the part of the hardware
maybe can't get satisfyingly compensated by the driver. So i'd prefer the
most simple solution.

---------

drivers/watchdog/f71808e_wdt.c | 78 ++++++++++++++++++++++++++++++++++++---
1 files changed, 72 insertions(+), 6 deletions(-)


Thanks for help and comments,

Lutz Ballaschke

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 0/3] watchdog: add f71862fg support [ In reply to ]
On Sun, Sep 26, 2010 at 04:06:29PM +0200, Lutz Ballaschke wrote:
> Furthermore i was thinking about modifying the WDIOC_GETTIMELEFT
> helper function since it simply returns the content of the WDT timer
> register without regarding whether the driver runs in minute-mode or not
> (thanks to Giel for pointing that out). This means if the driver is loaded
> with a timeout of >255 seconds (and switches to minute-mode) WDIOC_GETTIMELEFT
> ioctl returns minutes instead of seconds and thus doesn't comply with
> watchdog-api (e.g. ioctl returns 6(min) not 360(sec)). It might be
> confusing if one loads the driver with a timeout of let's say 360 seconds
> and gets 6 minutes returned by ioctl. But imho it's not very accurate to
> return a value of seconds if the wdt doesn't count in seconds. That might
> be even more confusing. This inconsistency on the part of the hardware
> maybe can't get satisfyingly compensated by the driver. So i'd prefer the
> most simple solution.

IMO the worst option would be to just return minutes. Returning
'minutes * 60 = seconds' would be significantly less bad. So if these
are your options I'd suggest going with the lesser evil.

One other possible option could be returning '(minutes - 1) * 60 + 1'.
That way bad timing decisions based on the (wrong) assumption, that
there are still 60 seconds left until reset, can be averted.

I would however like Wim's opinion on this, as this affects the
Watchdog API's usage.

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Good code is its own best documentation. As you're about to add a
comment, ask yourself, 'How can I improve the code so that this comment
isn't needed?' Improve the code and then document it to make it even
clearer."
-- Steve McConnell
Re: [PATCHv3 0/3] watchdog: add f71862fg support [ In reply to ]
Hi all,

> On Sun, Sep 26, 2010 at 04:06:29PM +0200, Lutz Ballaschke wrote:
> > Furthermore i was thinking about modifying the WDIOC_GETTIMELEFT
> > helper function since it simply returns the content of the WDT timer
> > register without regarding whether the driver runs in minute-mode or not
> > (thanks to Giel for pointing that out). This means if the driver is loaded
> > with a timeout of >255 seconds (and switches to minute-mode) WDIOC_GETTIMELEFT
> > ioctl returns minutes instead of seconds and thus doesn't comply with
> > watchdog-api (e.g. ioctl returns 6(min) not 360(sec)). It might be
> > confusing if one loads the driver with a timeout of let's say 360 seconds
> > and gets 6 minutes returned by ioctl. But imho it's not very accurate to
> > return a value of seconds if the wdt doesn't count in seconds. That might
> > be even more confusing. This inconsistency on the part of the hardware
> > maybe can't get satisfyingly compensated by the driver. So i'd prefer the
> > most simple solution.
>
> IMO the worst option would be to just return minutes. Returning
> 'minutes * 60 = seconds' would be significantly less bad. So if these
> are your options I'd suggest going with the lesser evil.
>
> One other possible option could be returning '(minutes - 1) * 60 + 1'.
> That way bad timing decisions based on the (wrong) assumption, that
> there are still 60 seconds left until reset, can be averted.
>
> I would however like Wim's opinion on this, as this affects the
> Watchdog API's usage.

The return value should be the seconds before the watchdog will reboot.
So value should be seconds, not minutes and is best the safest value
(so (minutes - 1) * 60 + 1 in this case ).
Secondly: the WDIOC_GETTIMELEFT should only be a DEBUGging ioctl.
The reason I created it, is to check if certain drivers are working
or not. We should not use it in normal operations imho.

Kind regards,
Wim.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 0/3] watchdog: add f71862fg support [ In reply to ]
On Sat, Oct 02, 2010 at 14:33 +0200, Wim Van Sebroeck wrote:

> On Sun, Sep 26, 2010 at 04:06:29PM +0200, Lutz Ballaschke wrote:
> > > Furthermore i was thinking about modifying the WDIOC_GETTIMELEFT
> ...
> > and gets 6 minutes returned by ioctl. But imho it's not very accurate to
> > > return a value of seconds if the wdt doesn't count in seconds. That might
> > > be even more confusing. This inconsistency on the part of the hardware
> > > maybe can't get satisfyingly compensated by the driver. So i'd prefer the
> > > most simple solution.
> ...
> The return value should be the seconds before the watchdog will reboot.
> So value should be seconds, not minutes and is best the safest value
> (so (minutes - 1) * 60 + 1 in this case ).
> Secondly: the WDIOC_GETTIMELEFT should only be a DEBUGging ioctl.
> The reason I created it, is to check if certain drivers are working
> or not. We should not use it in normal operations imho.
>
I noticed the patches for f71862fg support in f71808e driver are still
missing in the recent pull request. This might be due to the ioctl
implementation (PATCHv3 2/3) mentioned above which wasn't clarified.
Since i missed to reply at your remarks this was my fault. Now i hope
the PATCHv3 1/3 and 3/3 to get through as they already were signed-off
by the author of that driver. Since the patch in question isn't
essential for driver operation i suggest to drop that issue.
Sorry for the inconvenience.

With kind regards,

Lutz Ballaschke

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/