Re: [PATCH v4 3/5] watchdog: ts4800: add driver for TS-4800 watchdog

[Damien Riegel <damien.riegel@savoirfairelinux.com>]

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@savoirfairelinux.com>
> >---
> 
> [ ... ]
> 
> >+
> >+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