Hi Guenther,
On Fri, May 31, 2013 at 04:40:37AM -0700, Guenter Roeck wrote:
> > +#define GPIO_WD_ENAB 169
> > +#define GPIO_WD_FAST 170
> > +#define GPIO_WD_TRIG 171
> > +
> > +#define GPIO_RST_CAUSE_BASE 166
> > +
>
> I think I asked that before ... as you are supporting devicetree, gpio pins
> should really be provided through devicetree properties and not be hardcoded.
>
Yes you did and I didn't come up with a solution to this problem yet. I understand
and agree to your concerns but I'm lacking example code/documentation for it, maybe
you can point me to an example on that and then I'll update my code accordingly.
> > +struct a21_wdt_drv {
> > + struct watchdog_device wdt;
> > + struct mutex lock;
> > + }
> > +
> > + if (timeout == 30 && wdt->timeout == 1) {
> > + dev_err(wdt->dev,
> > + "Transition from fast to slow mode not allowed\n");
> > + return -EINVAL;
> > + }
> > +
> I dislike that [2 .. 29] timeout values are not supported, and would rather have
> you accept that range and set the timeout to 30. Also, it is somewhat undesirable
> that the timeout can not be changed back to 30 after being set to 1.
The not changing back issue was a hardware design criterion and I have no
influence on that one. I've already checked with the IC developer and he said,
this is intended. I don't quite understand why I should accept the timeout and
set it to 30? In my opinion this only introduces some magic the user won't
understand. Instead failing and giving an error message is something a user can
take as a hint for investigating.
> As Wim recommended, a softdog on top of the hardware watchdog would be the best
> solution. I'll leave it up to him to decide if he wants to accept the code
> as-is.
Yes let's see what he's saying.
>
> > + mutex_lock(&drv->lock);
> > +
> > + ret = watchdog_register_device(&a21_wdt);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Cannot register watchdog device\n");
> > + goto err_register_wd;
> > + }
> > +
> > + ret = a21_wdt_get_bootstatus(&reset);
> > + if (ret)
> > + dev_warn(&pdev->dev, "Reset Cause contains invalid data\n");
> > + else {
> > + if (reset == 2)
> > + a21_wdt.bootstatus |= WDIOF_EXTERN1;
> > + else if (reset == 4)
> > + a21_wdt.bootstatus |= WDIOF_CARDRESET;
> > + else if (reset == 5)
> > + a21_wdt.bootstatus |= WDIOF_POWERUNDER;
> > + else if (reset == 7)
> > + a21_wdt.bootstatus |= WDIOF_EXTERN2;
>
> What about other causes ? No useful match ?
>
None I could find. Actually WDIOF_EXTERN[12] already are a pretty creative
mapping in my opinion. EXTERN1 is a "Push Button" event, EXTERN2 is a reset
caused by a JTAG/BDM adapter...
> I think bootstatus should be set prior to registering the watchdog device
> to avoid race conditions where an application reads it prior to being set.
>
Agreed, didn't see that one *doh*, must be fixed before inclusion.
> > + }
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
