On Mon, Nov 23, 2015 at 06:32:15PM -0800, Guenter Roeck wrote: > Hi Damien, > > On 11/23/2015 07:17 AM, Damien Riegel wrote: > >This watchdog is instantiated in a FPGA that is memory mapped. It is > >made of only one register, called the feed register. Writing to this > >register will re-arm the watchdog for a given time (and enable it if it > >was disable). It can be disabled by writing a special value into it. > > > >It is part of a syscon block, and the watchdog register offset in this > >block varies from board to board. This offset is passed in the syscon > >property after the phandle to the syscon node. > > > >Signed-off-by: Damien Riegel <damien.riegel@xxxxxxxxxxxxxxxxxxxx> > >--- > > [ ... ] > > >+ > >+static int ts4800_wdt_set_timeout(struct watchdog_device *wdd, > >+ unsigned int timeout) > >+{ > >+ struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd); > >+ int i; > >+ > >+ for (i = 0; i <= MAX_TIMEOUT_INDEX; i++) { > >+ if (ts4800_wdt_map[i].timeout >= timeout) > >+ break; > >+ } > > If the loop does not break, i will have a value of MAX_TIMEOUT_INDEX + 1, > or 2, pointing after the end of the table. That should never happen, > but still ... That should never happen, but indeed it's not very elegant. I will change that. > > I preferred the earlier version, where you had an extra function. As I have to set both wdd->timeout and wdt->feed_val, I found it easier and shorter to iterate directly in the set_timeout function. > Only my suggestion was to have that function return MAX_TIMEOUT_INDEX > instead of an error. Alternatively, the check above needs to be > "i < MAX_TIMEOUT_INDEX". That would be a strange stop condition. Would this construct be ok for you: for (i = 0; i < ARRAY_SIZE(ts4800_wdt_map; i++) { if (ts4800_wdt_map[i].timeout >= timeout) break; } if (i >= ARRAY_SIZE(ts4800_wdt_map) i = MAX_TIMEOUT_INDEX; Thanks, Damien -- 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