Re: [v3,4/4] watchdog: add Gateworks System Controller support

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Tim Harvey <tharvey@gateworks.com>,
	Lee Jones <lee.jones@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Mark Brown <broonie@kernel.org>, Wim Van Sebroeck <wim@iguana.be>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-hwmon@vger.kernel.org, linux-input@vger.kernel.org,
	linux-watchdog@vger.kernel.org
Subject: Re: [v3,4/4] watchdog: add Gateworks System Controller support
Date: Fri, 30 Mar 2018 10:48:38 -0700	[thread overview]
Message-ID: <20180330174838.GA141984@dtor-ws> (raw)
In-Reply-To: <20180330010757.GA12896@roeck-us.net>

On Thu, Mar 29, 2018 at 06:07:57PM -0700, Guenter Roeck wrote:
> On Wed, Mar 28, 2018 at 08:14:03AM -0700, Tim Harvey wrote:
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  drivers/watchdog/Kconfig   |  10 ++++
> >  drivers/watchdog/Makefile  |   1 +
> >  drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 157 insertions(+)
> >  create mode 100644 drivers/watchdog/gsc_wdt.c
> > 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index ca200d1..c9d4b2e 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL
> >  	  arch_initcall.
> >  	  If in doubt, say N.
> >  
> > +config GSC_WATCHDOG
> > +	tristate "Gateworks System Controller (GSC) Watchdog support"
> > +	depends on MFD_GATEWORKS_GSC
> > +	select WATCHDOG_CORE
> > +	help
> > +	  Say Y here to include support for the GSC Watchdog.
> > +
> > +	  This driver can also be built as a module. If so the module
> > +	  will be called gsc_wdt.
> > +
> >  config MENF21BMC_WATCHDOG
> >  	tristate "MEN 14F021P00 BMC Watchdog"
> >  	depends on MFD_MENF21BMC || COMPILE_TEST
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 715a210..499327e 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
> >  obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
> >  obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o
> >  obj-$(CONFIG_GPIO_WATCHDOG)	+= gpio_wdt.o
> > +obj-$(CONFIG_GSC_WATCHDOG)	+= gsc_wdt.o
> >  obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o
> >  obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o
> >  obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
> > diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c
> > new file mode 100644
> > index 0000000..b43d083
> > --- /dev/null
> > +++ b/drivers/watchdog/gsc_wdt.c
> > @@ -0,0 +1,146 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * Copyright (C) 2018 Gateworks Corporation
> > + *
> > + * This driver registers a Linux Watchdog for the GSC
> > + */
> > +#include <linux/mfd/gsc.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDT_DEFAULT_TIMEOUT	60
> > +
> > +struct gsc_wdt {
> > +	struct watchdog_device wdt_dev;
> > +	struct gsc_dev *gsc;
> > +};
> > +
> > +static int gsc_wdt_start(struct watchdog_device *wdd)
> > +{
> > +	struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> > +	unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
> 
> Please use BIT().
> 
> > +	int ret;
> > +
> > +	dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout);
> 
> I don't think those debug messages add any value.
> 
> > +
> > +	/* clear first as regmap_update_bits will not write if no change */
> > +	ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
> > +	if (ret)
> > +		return ret;
> > +	return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg);
> > +}
> > +
> > +static int gsc_wdt_stop(struct watchdog_device *wdd)
> > +{
> > +	struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> > +	unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
> > +
> 
> BIT(). You might as well drop the variable and just use
> BIT(GSC_CTRL_1_WDT_ENABLE) below.
> 
> > +	dev_dbg(wdd->parent, "%s\n", __func__);
> > +
> > +	return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
> > +}
> > +
> > +static int gsc_wdt_set_timeout(struct watchdog_device *wdd,
> > +			       unsigned int timeout)
> > +{
> > +	struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
> > +	unsigned int long_sel = 0;
> > +
> > +	dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout);
> > +
> > +	switch (timeout) {
> > +	case 60:
> > +		long_sel = (1 << GSC_CTRL_1_WDT_TIME);
> > +	case 30:
> > +		regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1,
> > +				   (1 << GSC_CTRL_1_WDT_TIME),
> 
> BIT()
> 
> > +				   (long_sel << GSC_CTRL_1_WDT_TIME));

This looks wrong. Are you sure that in case of 60 timeout you want to
actually write

	((1 << GSC_CTRL_1_WDT_TIME) << GSC_CTRL_1_WDT_TIME)

to the register?

Or you meant to write:

	long_sel = timeout > 30;
	regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1,
			   (long_sel << GSC_CTRL_1_WDT_TIME),

Thanks.

-- 
Dmitry