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@xxxxxxxxxxxxx>
> > ---
> > 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
