Messages in this thread | | | Date | Thu, 28 Aug 2014 05:11:04 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH 3/4] watchdog: add driver for Ricoh RN5T618 watchdog |
| |
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@gmail.com> >>> --- >>> 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.
Guenter
>>> + 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; >>> +} >
| |