On Thu, 28 Aug 2014, Guenter Roeck wrote: > On 08/28/2014 12:19 AM, Lee Jones wrote: > >On Wed, 27 Aug 2014, Guenter Roeck wrote: > >>On Wed, Aug 27, 2014 at 12:13:56AM +0200, Beniamino Galvani wrote: > >>>This adds a driver for the watchdog timer available in Ricoh RN5T618 > >>>PMIC. The device supports a programmable expiration time of 1, 8, 32 > >>>or 128 seconds. > >>> > >>>Signed-off-by: Beniamino Galvani <b.galvani@xxxxxxxxx> > >>>--- > >>> drivers/watchdog/Kconfig | 11 +++ > >>> drivers/watchdog/Makefile | 1 + > >>> drivers/watchdog/rn5t618_wdt.c | 196 ++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/mfd/rn5t618.h | 4 + > >>> 4 files changed, 212 insertions(+) > >>> create mode 100644 drivers/watchdog/rn5t618_wdt.c > > > >[...] > > > >>>+++ b/drivers/watchdog/rn5t618_wdt.c > >>>@@ -0,0 +1,196 @@ > > > >[...] > > > >>>+static int rn5t618_wdt_set_timeout(struct watchdog_device *wdt_dev, > >>>+ unsigned int timeout) > >>>+{ > >>>+ struct rn5t618_wdt *wdt = watchdog_get_drvdata(wdt_dev); > >>>+ int ret, i; > >>>+ > >>>+ for (i = 0; i < ARRAY_SIZE(rn5t618_wdt_map); i++) { > >>>+ if (rn5t618_wdt_map[i].time + 1 >= timeout) > >>>+ break; > >>>+ } > >>>+ > >>>+ if (i == ARRAY_SIZE(rn5t618_wdt_map)) > >>>+ ret = -EINVAL; > >> > >>Can you simplify this a bit ? If you use > >> > >> if (i == ARRAY_SIZE(rn5t618_wdt_map)) > >> return -EINVAL; > > > >This changes the semantics. > > > How so ? If ret is set to -EINVAL, the rest of the function won't do anything > but eventually return -EINVAL. I don't see why returning -EINVAL immediately > would change that. Ah, you're right. I read: if (!ret) wdt_dev->timeout = rn5t618_wdt_map[i].time; As: if (ret) wdt_dev->timeout = rn5t618_wdt_map[i].time; My bad - withdrawn. > >>>+ else > >> > >>You can drop this else statement. > >> > >>>+ ret = regmap_update_bits(wdt->rn5t618->regmap, RN5T618_WATCHDOG, > >>>+ RN5T618_WATCHDOG_WDOGTIM_M, > >>>+ rn5t618_wdt_map[i].reg_val); > >>>+ if (!ret) > >>>+ wdt_dev->timeout = rn5t618_wdt_map[i].time; > > > >... Isn't this important? > > > >>>+ return ret; > >>>+} > > > -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- 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