...
> > +
> > +/* TIMEOUT_MAX = ffff0/90kHz =11.65,so longer than 11 seconds will time out */
> > +static int sp_wdt_ping(struct watchdog_device *wdev)
> > +{
> > + struct sp_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > + void __iomem *base = priv->base;
> > + u32 count;
> > + u32 actual;
> > +
> > + actual = min(wdev->timeout, SP_WDT_MAX_TIMEOUT);
> > +
> > + if (actual > SP_WDT_MAX_TIMEOUT) {
>
> This is never true. because actual is set to the minimum of wdev->timeout
> and SP_WDT_MAX_TIMEOUT. It is also not clear to me what this code path
> is supposed to be doing, compared to the code below. Please fix and add
> a comment explaining the code (ie what does writing WDT_CONMAX into
> the control register do ?)
>
I will fix "if (actual > SP_WDT_MAX_TIMEOUT)" to "if (wdev->timeout >
SP_WDT_MAX_TIMEOUT)". I will add comment "WDT_CONMAX sets the
count to the maximum (down-counting)."
When timeout > SP_WDT_MAX_TIMEOUT, I try to set a smaller value or
do nothing. Watchdog driver don't work properly.
> > + writel(WDT_CONMAX, base + WDT_CTRL);
> > + } else {
> > + writel(WDT_UNLOCK, base + WDT_CTRL);
> > + /* tiemrw_cnt[3:0]can't be write,only [19:4] can be write. */
>
> timerw_cnd ?, space before can't, and s/write/written/g.
>
> > + count = (actual * STC_CLK) >> 4;
>
> so this is (actual * 90000) >> 4 and does set bit 0..3. Is that intentional
> or is it supposed to be << 4 ? The comment above does not really explain
> the calculation; if anything, it just creates confusion.
>
I will add comment "Watchdog timer is a 20-bit down-counting based
on STC_CLK. This register bits[16]~[0] is from bit[19]~bit[4] of the
watchdog timer counter. "
Thanks,
Best Regards
