Re: [PATCH 3/4] watchdog: add driver for Ricoh RN5T618 watchdog

[Guenter Roeck]

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.

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;
> > > +}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/