[PATCH 2/2] watchdog: Add Toshiba Visconti watchdog driver

[Nobuhiro Iwamatsu]

Hi,

Thanks for your review.

On Thu, Sep 17, 2020 at 10:51:14PM -0700, Guenter Roeck wrote:
> On 9/17/20 3:39 PM, Nobuhiro Iwamatsu wrote:
> > Add the watchdog driver for Toshiba Visconti series.
> > 
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu at toshiba.co.jp>
> > ---
> >  drivers/watchdog/Kconfig        |   8 ++
> >  drivers/watchdog/Makefile       |   1 +
> >  drivers/watchdog/visconti_wdt.c | 192 ++++++++++++++++++++++++++++++++
> >  3 files changed, 201 insertions(+)
> >  create mode 100644 drivers/watchdog/visconti_wdt.c
> > 

<snip>

> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> 
> Alphabetically, please.
> 

OK, I will fix this.

> > +
> > +#define WDT_CNT			0x00
> > +#define WDT_MIN			0x04
> > +#define WDT_MAX			0x08
> > +#define WDT_CTL			0x0c
> > +#define WDT_CMD			0x10
> > +#define WDT_CMD_CLEAR		0x4352
> > +#define WDT_CMD_START_STOP	0x5354
> > +#define WDT_DIV			0x30
> > +

<snip>

> > +
> > +static unsigned int visconti_wdt_get_timeleft(struct watchdog_device *wdev)
> > +{
> > +	struct visconti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u32 timeout = wdev->timeout * VISCONTI_WDT_FREQ;
> > +
> > +	timeout -= readl(priv->base + WDT_CNT);
> > +
> 
> What happens if this is negative for whatever reason ?
>

Oh, thanks. I will add a negative check and handling.

> > +	return timeout / VISCONTI_WDT_FREQ;
> > +}
> > +
> > +static int visconti_wdt_set_timeout(struct watchdog_device *wdev,
> > +				    unsigned int timeout)
> > +{
> > +	u32 val;
> > +	struct visconti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	wdev->timeout = timeout;
> > +	val = wdev->timeout * VISCONTI_WDT_FREQ;
> > +
> > +	/* Clear counter before setting timeout because WDT expires */
> > +	writel(WDT_CMD_CLEAR, priv->base + WDT_CMD);
> > +	writel(val, priv->base + WDT_MAX);
> > +
> > +	return 0;
> > +}

<snip>

> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		dev_err(dev, "Could not get clock\n");
> 
> devm_clk_get() can return -EPROBE_DEFER. In that case we don't
> want to see an error message. Consider using dev_err_probe().

OK, I will rewrite using using dev_err_probe().

> 
> > +		return PTR_ERR(clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(clk);
> > +	if (ret) {
> > +		dev_err(dev, "Could not enable clock\n");
> > +		return ret;
> > +	}

<snip>

Best regards,
  Nobuhiro