Hi Thierry, >> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd) >> +{ >> + struct tegra186_wdt *wdt = to_tegra186_wdt(wdd); >> + u32 timeleft; >> + u32 expiration; >> + >> + if (!watchdog_active(&wdt->base)) { >> + /* return zero if the watchdog timer is not activated. */ >> + return 0; >> + } >> + >> + /* >> + * System power-on reset occurs on the fifth expiration of the watchdog timer and so > >Is "system power-on reset" really what this is called? Power-on reset >sounds like something that only happens after you power the device on, >not something that can be triggered by the watchdog. I will change it from "system power-on reset" to "System POR(Power On Reset)" in next patch. AFAIK Power On Reset is used for decribing resetting circuits and initialing whatever it needs when received a POR signal after powered up. This term should also be applicable for hardware watchdog reset since the system is already powered up at that moment and >> + * when the watchdog timer is configured, the actual value programmed into the counter >> + * is 1/5 of the timeout value. Once the counter reaches 0, expiration count will be >> + * increased by 1 and the down counter restarts. >> + * Hence to get the time left before system reset we must combine 2 parts: >> + * 1. value of the current down counter >> + * 2. (number of counter expirations remaining) * (timeout/5) >> + */ > >Can you wrap this comment so that it fits within 80 columns? It's fine >to occasionally go beyond that limit if there's a good reason for it, >but this comment doesn't seem to fall into that category. Sorry, I missed to check the line length before submit, will wrap comments in next patch. >> + >> + /* Get the current number of counter expirations. Should be a value between 0 and 4. */ >> + expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, readl_relaxed(wdt->regs + WDTSR)); >> + >> + /* Convert the current counter value to seconds, rounding up to the nearest second. */ >> + timeleft = FIELD_GET(TMRSR_PCV, readl_relaxed(wdt->tmr->regs + TMRSR)); >> + timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC; > >Same for these. Maybe make an extra variable to store the register value >in to get rid of some of that extra horizontal space. Thanks for the suggestion, will do. >> + >> + /* >> + * Calculate the time remaining by adding the time for the counter value >> + * to the time of the counter expirations that remain. >> + */ >> + timeleft += wdt->base.timeout * (4 - expiration) / 5; > >This doesn't quite match what the comment above says. Shouldn't this be: > > timeleft += (wdt->base.timeout / 5) * (4 - expiration); > >instead? Here I made a transposition on purpose just for keeping the precision due to the integer division. e.g. If 'divided by 5' goes first, the equation goes to 0 when wdt->base.timeout is less than 5 no matter which timer expiration it is. But the number will be still inaccurate because I made a mistake that the number should be calculated in microseconds all the time and be converted to a nearest second only in the last step. A new patch has been made and I will submit it after verifications. Thank you for your time on reviewing this! Best, -- Pohsun