RE: [PATCH v6 2/2] watchdog: Add watchdog driver for Sunplus SP7021

["xt.hu[胡先韬]" <xt.hu@cqplus1.com>]

...
> > +
> > +/* 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