diff mbox

[V2,3/4] watchdog: da9062: DA9062 watchdog driver

Message ID 6a51f163b99edfad9165ad29609abb072dbaa2b7.1431621833.git.stwiss.opensource@diasemi.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve Twiss May 14, 2015, 4:43 p.m. UTC
From: S Twiss <stwiss.opensource@diasemi.com>

Add watchdog driver support for DA9062


Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com>

---

Changes in V2:
 - Removed informational dev_info() 'installed watchdog' message
 - Copyright headers GPL v2 (and later) match correct 'GPL' in MODULE_LICENSE
 - Removed the explicit 300 msecs delay from the reset_watchdog_timer()
   function and replaced it with a variable delay (depending on the
   difference since the last ping). A debug message is used to catch the
   multiple pings trying to break the 300 msecs protection barrier.
 - Fix error paths for the functions da9062_wdt_update_timeout_register()
   and da9062_wdt_stop() 
 - Add error paths in the probe() and correctly clean-up the registered
   device if there is a problem after registration.

This patch applies against linux-next and v4.1-rc3 



 drivers/watchdog/Kconfig      |   9 ++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/da9062_wdt.c | 288 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 298 insertions(+)
 create mode 100644 drivers/watchdog/da9062_wdt.c

Comments

Guenter Roeck May 15, 2015, 2:13 a.m. UTC | #1
On 05/14/2015 09:43 AM, S Twiss wrote:
> From: S Twiss <stwiss.opensource@diasemi.com>
>
> Add watchdog driver support for DA9062
>
>
> Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com>
>
> ---
>
> Changes in V2:
>   - Removed informational dev_info() 'installed watchdog' message
>   - Copyright headers GPL v2 (and later) match correct 'GPL' in MODULE_LICENSE
>   - Removed the explicit 300 msecs delay from the reset_watchdog_timer()
>     function and replaced it with a variable delay (depending on the
>     difference since the last ping). A debug message is used to catch the
>     multiple pings trying to break the 300 msecs protection barrier.
>   - Fix error paths for the functions da9062_wdt_update_timeout_register()
>     and da9062_wdt_stop()
>   - Add error paths in the probe() and correctly clean-up the registered
>     device if there is a problem after registration.
>
> This patch applies against linux-next and v4.1-rc3
>
>
>
>   drivers/watchdog/Kconfig      |   9 ++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/da9062_wdt.c | 288 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 298 insertions(+)
>   create mode 100644 drivers/watchdog/da9062_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e5e7c55..dfdb6c6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -96,6 +96,15 @@ config DA9063_WATCHDOG
>
>   	  This driver can be built as a module. The module name is da9063_wdt.
>
> +config DA9062_WATCHDOG
> +	tristate "Dialog DA9062 Watchdog"
> +	depends on MFD_DA9062
> +	select WATCHDOG_CORE
> +	help
> +	  Support for the watchdog in the DA9062 PMIC.
> +
> +	  This driver can be built as a module. The module name is da9062_wdt.
> +
>   config GPIO_WATCHDOG
>   	tristate "Watchdog device controlled through GPIO-line"
>   	depends on OF_GPIO
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 5c19294..57ba815 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -179,6 +179,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o
>   # Architecture Independent
>   obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
>   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_WM831X_WATCHDOG) += wm831x_wdt.o
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> new file mode 100644
> index 0000000..9e6c93b
> --- /dev/null
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -0,0 +1,288 @@
> +/*
> + * da9062_wdt.c - WDT device driver for DA9062
> + * Copyright (C) 2015  Dialog Semiconductor Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/da9062/registers.h>
> +#include <linux/mfd/da9062/core.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
> +
> +static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 };
> +#define DA9062_TWDSCALE_DISABLE		0
> +#define DA9062_TWDSCALE_MIN		1
> +#define DA9062_TWDSCALE_MAX		(ARRAY_SIZE(wdt_timeout) - 1)
> +#define DA9062_WDT_MIN_TIMEOUT		wdt_timeout[DA9062_TWDSCALE_MIN]
> +#define DA9062_WDT_MAX_TIMEOUT		wdt_timeout[DA9062_TWDSCALE_MAX]
> +#define DA9062_WDG_DEFAULT_TIMEOUT	wdt_timeout[DA9062_TWDSCALE_MAX-1]
> +#define DA9062_RESET_PROTECTION_MS	300
> +
> +struct da9062_watchdog {
> +	struct da9062 *hw;
> +	struct watchdog_device wdtdev;
> +	unsigned long j_time_stamp;
> +};
> +
> +static void da9062_set_window_start(struct da9062_watchdog *wdt)
> +{
> +	wdt->j_time_stamp = jiffies;
> +}
> +
> +static void da9062_apply_window_protection(struct da9062_watchdog *wdt)
> +{
> +	unsigned long delay = msecs_to_jiffies(DA9062_RESET_PROTECTION_MS);
> +	unsigned long timeout = wdt->j_time_stamp + delay;
> +	unsigned long now = jiffies;
> +	unsigned int diff_ms;
> +
> +	/* if time-limit has not elapsed then wait for remainder */
> +	if (time_before(now, timeout)) {
> +		diff_ms = jiffies_to_msecs(timeout-now);
> +		dev_dbg(wdt->hw->dev,
> +			"Kicked too quickly. Delaying %u msecs\n", diff_ms);
> +		msleep(diff_ms);
> +	}
> +
> +	return;

Unnecessary return statement.

> +}
> +
> +static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs)
> +{
> +	unsigned int i;
> +
> +	for (i = DA9062_TWDSCALE_MIN; i <= DA9062_TWDSCALE_MAX; i++) {
> +		if (wdt_timeout[i] >= secs)
> +			return i;
> +	}
> +
> +	return DA9062_TWDSCALE_MAX;
> +}
> +
> +static int da9062_reset_watchdog_timer(struct da9062_watchdog *wdt)
> +{
> +	int ret;
> +
> +	da9062_apply_window_protection(wdt);
> +
> +	ret = regmap_update_bits(wdt->hw->regmap,
> +			   DA9062AA_CONTROL_F,
> +			   DA9062AA_WATCHDOG_MASK,
> +			   DA9062AA_WATCHDOG_MASK);
> +
> +	da9062_set_window_start(wdt);
> +
> +	return ret;
> +}
> +
> +static int da9062_wdt_update_timeout_register(struct da9062_watchdog *wdt,
> +					      unsigned int regval)
> +{
> +	struct da9062 *chip = wdt->hw;
> +	int ret;
> +
> +	ret = da9062_reset_watchdog_timer(wdt);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to ping the watchdog (err = %d)\n",
> +			ret);

I am kind of torn about all this noisiness on error. Personally I would tend to ask
people to let user space handle it, and not be that noisy in the kernel.

Wim, any guidance ?

> +		return ret;
> +	}
> +
> +	return regmap_update_bits(chip->regmap,
> +				  DA9062AA_CONTROL_D,
> +				  DA9062AA_TWDSCALE_MASK,
> +				  regval);

... and it is inconsistent - no error message here.

> +}
> +
> +static int da9062_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	unsigned int selector;
> +	int ret;
> +
> +	selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout);
> +	ret = da9062_wdt_update_timeout_register(wdt, selector);
> +	if (ret)
> +		dev_err(wdt->hw->dev, "Watchdog failed to start (err = %d)\n",
> +			ret);
> +
> +	return ret;
> +}
> +
> +static int da9062_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	int ret;
> +
> +	ret = da9062_reset_watchdog_timer(wdt);
> +	if (ret) {
> +		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(wdt->hw->regmap,
> +				 DA9062AA_CONTROL_D,
> +				 DA9062AA_TWDSCALE_MASK,
> +				 DA9062_TWDSCALE_DISABLE);
> +	if (ret)
> +		dev_alert(wdt->hw->dev, "Watchdog failed to stop (err = %d)\n",
> +			  ret);

.. and now we have an alert. Hmm..

> +
> +	return ret;
> +}
> +
> +static int da9062_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	int ret;
> +
> +	dev_dbg(wdt->hw->dev, "watchdog ping\n");
> +

Is this really valuable enough to keep in the code ?

> +	ret = da9062_reset_watchdog_timer(wdt);
> +	if (ret)
> +		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
> +			ret);
> +
> +	return ret;
> +}
> +
> +static int da9062_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	unsigned int selector;
> +	int ret;
> +
> +	selector = da9062_wdt_timeout_to_sel(timeout);
> +	ret = da9062_wdt_update_timeout_register(wdt, selector);
> +	if (ret)
> +		dev_err(wdt->hw->dev, "Failed to set watchdog timeout (err = %d)\n",
> +			ret);
> +	else
> +		wdd->timeout = wdt_timeout[selector];
> +
> +	return ret;
> +}
> +
> +/* E_WDG_WARN interrupt handler */
> +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data)
> +{
> +	struct da9062_watchdog *wdt = data;
> +
> +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct watchdog_info da9062_watchdog_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.identity = "DA9062 WDT",
> +};
> +
> +static const struct watchdog_ops da9062_watchdog_ops = {
> +	.owner = THIS_MODULE,
> +	.start = da9062_wdt_start,
> +	.stop = da9062_wdt_stop,
> +	.ping = da9062_wdt_ping,
> +	.set_timeout = da9062_wdt_set_timeout,
> +};
> +
> +static int da9062_wdt_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct da9062 *chip;
> +	struct da9062_watchdog *wdt;
> +	int irq;
> +
> +	chip = dev_get_drvdata(pdev->dev.parent);
> +	if (!chip)
> +		return -EINVAL;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdt->hw = chip;
> +
> +	wdt->wdtdev.info = &da9062_watchdog_info;
> +	wdt->wdtdev.ops = &da9062_watchdog_ops;
> +	wdt->wdtdev.min_timeout = DA9062_WDT_MIN_TIMEOUT;
> +	wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT;
> +	wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT;
> +	wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> +	dev_set_drvdata(&pdev->dev, wdt);
> +
> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> +	if (irq < 0) {
> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> +		ret = irq;
> +		goto error;

Just return; the label does not serve a useful purpose. Same for the other goto
statements below.

Also, is the interrupt mandatory ? All it does is to display a message.
Looks very optional to me.

> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +			   da9062_wdt_wdg_warn_irq_handler,
> +			    IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
> +			   "WDG_WARN", wdt);
> +	if (ret) {
> +		dev_err(wdt->hw->dev,
> +			"Failed to request watchdog device IRQ.\n");
> +		goto error;
> +	}
> +
> +	ret = watchdog_register_device(&wdt->wdtdev);
> +	if (ret < 0) {
> +		dev_err(wdt->hw->dev,
> +			"watchdog registration incomplete (%d)\n", ret);

incomplete ? Should that be "failed" ?

> +		goto error;
> +	}
> +
> +	da9062_set_window_start(wdt);
> +
> +	ret = da9062_wdt_ping(&wdt->wdtdev);
> +	if (ret < 0)
> +		watchdog_unregister_device(&wdt->wdtdev);
> +
> +error:
> +	return ret;
> +}
> +
> +static int da9062_wdt_remove(struct platform_device *pdev)
> +{
> +	struct da9062_watchdog *wdt = dev_get_drvdata(&pdev->dev);
> +
> +	watchdog_unregister_device(&wdt->wdtdev);
> +	return 0;
> +}
> +
> +static struct platform_driver da9062_wdt_driver = {
> +	.probe = da9062_wdt_probe,
> +	.remove = da9062_wdt_remove,
> +	.driver = {
> +		.name = "da9062-watchdog",
> +	},
> +};
> +module_platform_driver(da9062_wdt_driver);
> +
> +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
> +MODULE_DESCRIPTION("WDT device driver for Dialog DA9062");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform: da9062-watchdog");
>
Normally I don't see a space between "platform" and the driver name.
Does this work ?

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Twiss May 15, 2015, 8:13 a.m. UTC | #2
Hi Guenter,

Thank you for your comments again,
Here are my responses.

Regards,
Steve

On 15 May 2015 03:13, Guenter Roeck 
> Subject: Re: [PATCH V2 3/4] watchdog: da9062: DA9062 watchdog driver
> 

[...]

> > +static void da9062_apply_window_protection(struct da9062_watchdog
> *wdt)
> > +{
> > +	unsigned long delay =
> msecs_to_jiffies(DA9062_RESET_PROTECTION_MS);
> > +	unsigned long timeout = wdt->j_time_stamp + delay;
> > +	unsigned long now = jiffies;
> > +	unsigned int diff_ms;
> > +
> > +	/* if time-limit has not elapsed then wait for remainder */
> > +	if (time_before(now, timeout)) {
> > +		diff_ms = jiffies_to_msecs(timeout-now);
> > +		dev_dbg(wdt->hw->dev,
> > +			"Kicked too quickly. Delaying %u msecs\n", diff_ms);
> > +		msleep(diff_ms);
> > +	}
> > +
> > +	return;
> 
> Unnecessary return statement.
> 

Deleted.

> > +static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = DA9062_TWDSCALE_MIN; i <= DA9062_TWDSCALE_MAX; i++) {
> > +		if (wdt_timeout[i] >= secs)
> > +			return i;
> > +	}
> > +
> > +	return DA9062_TWDSCALE_MAX;
> > +}
> > +
> > +static int da9062_reset_watchdog_timer(struct da9062_watchdog *wdt)
> > +{
> > +	int ret;
> > +
> > +	da9062_apply_window_protection(wdt);
> > +
> > +	ret = regmap_update_bits(wdt->hw->regmap,
> > +			   DA9062AA_CONTROL_F,
> > +			   DA9062AA_WATCHDOG_MASK,
> > +			   DA9062AA_WATCHDOG_MASK);
> > +
> > +	da9062_set_window_start(wdt);
> > +
> > +	return ret;
> > +}
> > +
> > +static int da9062_wdt_update_timeout_register(struct da9062_watchdog *wdt,
> > +					      unsigned int regval)
> > +{
> > +	struct da9062 *chip = wdt->hw;
> > +	int ret;
> > +
> > +	ret = da9062_reset_watchdog_timer(wdt);
> > +	if (ret) {
> > +		dev_err(chip->dev, "Failed to ping the watchdog (err = %d)\n",
> > +			ret);
> 
> I am kind of torn about all this noisiness on error. Personally I would tend to
> ask people to let user space handle it, and not be that noisy in the kernel.
> 
> Wim, any guidance ?

At the time I thought it would be a really good idea to keep a debug message in. 
But -- this has been questioned several times and so I will remove.

> > +		return ret;
> > +	}
> > +
> > +	return regmap_update_bits(chip->regmap,
> > +				  DA9062AA_CONTROL_D,
> > +				  DA9062AA_TWDSCALE_MASK,
> > +				  regval);
> 
> ... and it is inconsistent - no error message here.
> 

Removed the dev_err() defined previously and therefore this makes this return
without an error message more consistent with the earlier parts of the function.
(no change needed)

 [...]

> > +static int da9062_wdt_stop(struct watchdog_device *wdd)
> > +{
> > +	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> > +	int ret;
> > +
> > +	ret = da9062_reset_watchdog_timer(wdt);
> > +	if (ret) {
> > +		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err =
> %d)\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = regmap_update_bits(wdt->hw->regmap,
> > +				 DA9062AA_CONTROL_D,
> > +				 DA9062AA_TWDSCALE_MASK,
> > +				 DA9062_TWDSCALE_DISABLE);
> > +	if (ret)
> > +		dev_alert(wdt->hw->dev, "Watchdog failed to stop (err =
> %d)\n",
> > +			  ret);
> 
> .. and now we have an alert. Hmm..

.. I've replaced it with a dev_err()

> > +
> > +	return ret;
> > +}
> > +
> > +static int da9062_wdt_ping(struct watchdog_device *wdd)
> > +{
> > +	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> > +	int ret;
> > +
> > +	dev_dbg(wdt->hw->dev, "watchdog ping\n");
> > +
> 
> Is this really valuable enough to keep in the code ?
> 

Removed also.

> > +	ret = da9062_reset_watchdog_timer(wdt);
> > +	if (ret)
> > +		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err =
> %d)\n",
> > +			ret);
> > +
> > +	return ret;
> > +}
> > +

[...]

> > +
> > +/* E_WDG_WARN interrupt handler */
> > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data)
> > +{
> > +	struct da9062_watchdog *wdt = data;
> > +
> > +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> > +	return IRQ_HANDLED;
> > +}
> > +

[...]

> > +static int da9062_wdt_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +	struct da9062 *chip;
> > +	struct da9062_watchdog *wdt;
> > +	int irq;
> > +
> > +	chip = dev_get_drvdata(pdev->dev.parent);
> > +	if (!chip)
> > +		return -EINVAL;
> > +
> > +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > +	if (!wdt)
> > +		return -ENOMEM;
> > +
> > +	wdt->hw = chip;
> > +
> > +	wdt->wdtdev.info = &da9062_watchdog_info;
> > +	wdt->wdtdev.ops = &da9062_watchdog_ops;
> > +	wdt->wdtdev.min_timeout = DA9062_WDT_MIN_TIMEOUT;
> > +	wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT;
> > +	wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT;
> > +	wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> > +
> > +	watchdog_set_drvdata(&wdt->wdtdev, wdt);
> > +	dev_set_drvdata(&pdev->dev, wdt);
> > +
> > +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> > +	if (irq < 0) {
> > +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> > +		ret = irq;
> > +		goto error;
> 
> Just return; the label does not serve a useful purpose. Same for the other
> goto statements below.

Agreed. This is changed now.

> Also, is the interrupt mandatory ? All it does is to display a message.
> Looks very optional to me.

It is a place holder for something more application specific. 
I could remove it, but I figured it would just get re-added when somebody takes the
driver and modifies it for their needs.

If this is a problem however, it can go.
Please advise ..

> 
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +			   da9062_wdt_wdg_warn_irq_handler,
> > +			    IRQF_TRIGGER_LOW | IRQF_ONESHOT |
> IRQF_SHARED,
> > +			   "WDG_WARN", wdt);
> > +	if (ret) {
> > +		dev_err(wdt->hw->dev,
> > +			"Failed to request watchdog device IRQ.\n");
> > +		goto error;
> > +	}
> > +
> > +	ret = watchdog_register_device(&wdt->wdtdev);
> > +	if (ret < 0) {
> > +		dev_err(wdt->hw->dev,
> > +			"watchdog registration incomplete (%d)\n", ret);
> 
> incomplete ? Should that be "failed" ?

Sure. Changed the dev_err()

[...]

> > +static struct platform_driver da9062_wdt_driver = {
> > +	.probe = da9062_wdt_probe,
> > +	.remove = da9062_wdt_remove,
> > +	.driver = {
> > +		.name = "da9062-watchdog",
> > +	},
> > +};
> > +module_platform_driver(da9062_wdt_driver);
> > +
> > +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
> > +MODULE_DESCRIPTION("WDT device driver for Dialog DA9062");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform: da9062-watchdog");
> >
> Normally I don't see a space between "platform" and the driver name.
> Does this work ?

Removed the space

Regards,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck May 15, 2015, 12:57 p.m. UTC | #3
Hi Steve,

On 05/15/2015 01:13 AM, Opensource [Steve Twiss] wrote:
> Hi Guenter,
>
> Thank you for your comments again,
> Here are my responses.
>
> Regards,
> Steve
>
> On 15 May 2015 03:13, Guenter Roeck
>> Subject: Re: [PATCH V2 3/4] watchdog: da9062: DA9062 watchdog driver
>>
>
> [...]
>

[ ... ]

>>> +
>>> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
>>> +	if (irq < 0) {
>>> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
>>> +		ret = irq;
>>> +		goto error;
>>
>> Just return; the label does not serve a useful purpose. Same for the other
>> goto statements below.
>
> Agreed. This is changed now.
>
>> Also, is the interrupt mandatory ? All it does is to display a message.
>> Looks very optional to me.
>
> It is a place holder for something more application specific.
> I could remove it, but I figured it would just get re-added when somebody takes the
> driver and modifies it for their needs.
>
> If this is a problem however, it can go.
> Please advise ..
>

Then this someone should add the code. For the time being, it just increases
kernel size and may cause the driver to fail for no good reason. Plus, given
the driver apparently works without interrupt, even then it should be optional,
and the driver does not have to fail loading if it is not supported on a
given platform.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Twiss May 15, 2015, 3:35 p.m. UTC | #4
> > +
> > +/* E_WDG_WARN interrupt handler */
> > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data)
> > +{
> > +	struct da9062_watchdog *wdt = data;
> > +
> > +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> > +	return IRQ_HANDLED;
> > +}
> > +

On 15 May 2015 13:58 Guenter Roeck wrote:

 [...]
 
> >>> +
> >>> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> >>> +	if (irq < 0) {
> >>> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> >>> +		ret = irq;
> >>> +		goto error;

[...]

> >
> >> Also, is the interrupt mandatory ? All it does is to display a message.
> >> Looks very optional to me.
> >
> > It is a place holder for something more application specific.
> > I could remove it, but I figured it would just get re-added when somebody takes the
> > driver and modifies it for their needs.
> >
> > If this is a problem however, it can go.
> > Please advise ..
> >
> 
> Then this someone should add the code. For the time being, it just increases
> kernel size and may cause the driver to fail for no good reason. Plus, given
> the driver apparently works without interrupt, even then it should be
> optional, and the driver does not have to fail loading if it is not supported on a
> given platform.
> 

Hi Guenter,

I'm not sure if I got my previous point across there ...

Leaving this in wouldn't really do any real harm I think. If this feature is not supported
in somebody's platform then there wouldn't be a problem, the IRQ would fire (as a
warning that the watchdog was about to time-out), the handler function would be
executed, it would handle the IRQ -- and that would be it. Nothing would happen apart
from a debug print.

There are already examples of this in the kernel, I've not looked very hard ...
http://lxr.free-electrons.com/source/drivers/mfd/qcom_rpm.c#L412
http://lxr.free-electrons.com/source/drivers/mfd/arizona-irq.c#L72

The problem with removing it is, I am depreciating the functionality of the chip.
Unless there is a really good reason -- I would like to leave this part in please.
If I was to take this part out then there could be an argument to remove WDG_WARN
from the resource of the MFD -- but that would then hide this watchdog warning
functionality completely.

The function da9062_wdt_wdg_warn_irq_handler() is blank -- and it just does nothing
apart from handle the IRQ. But it is an important feature of the chip .. say for a developer
to add in their product code to send a uevent into userspace to trigger a watchdog kick
(for instance).. but that part is very specific and usually only part of a final system
integration. I've just left the function as a stub for that reason.

There is the possibility that the function platform_get_irq_byname() could fail
but that would mean an different type of critical failure.

Regards,
Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck May 15, 2015, 8:20 p.m. UTC | #5
On Fri, May 15, 2015 at 03:35:50PM +0000, Opensource [Steve Twiss] wrote:
> 
> > > +
> > > +/* E_WDG_WARN interrupt handler */
> > > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data)
> > > +{
> > > +	struct da9062_watchdog *wdt = data;
> > > +
> > > +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> 
> On 15 May 2015 13:58 Guenter Roeck wrote:
> 
>  [...]
>  
> > >>> +
> > >>> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> > >>> +	if (irq < 0) {
> > >>> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> > >>> +		ret = irq;
> > >>> +		goto error;
> 
> [...]
> 
> > >
> > >> Also, is the interrupt mandatory ? All it does is to display a message.
> > >> Looks very optional to me.
> > >
> > > It is a place holder for something more application specific.
> > > I could remove it, but I figured it would just get re-added when somebody takes the
> > > driver and modifies it for their needs.
> > >
> > > If this is a problem however, it can go.
> > > Please advise ..
> > >
> > 
> > Then this someone should add the code. For the time being, it just increases
> > kernel size and may cause the driver to fail for no good reason. Plus, given
> > the driver apparently works without interrupt, even then it should be
> > optional, and the driver does not have to fail loading if it is not supported on a
> > given platform.
> > 
> 
> Hi Guenter,
> 
> I'm not sure if I got my previous point across there ...
> 
> Leaving this in wouldn't really do any real harm I think. If this feature is not supported
> in somebody's platform then there wouldn't be a problem, the IRQ would fire (as a
> warning that the watchdog was about to time-out), the handler function would be
> executed, it would handle the IRQ -- and that would be it. Nothing would happen apart
> from a debug print.

I didn't get my point across either. Problem is that your driver fails to load
if the interrupt is not there. With the interrupt really being optional, I don't
see the point in making it mandatory just to display a message if it fires.

> 
> There are already examples of this in the kernel, I've not looked very hard ...
> http://lxr.free-electrons.com/source/drivers/mfd/qcom_rpm.c#L412
> http://lxr.free-electrons.com/source/drivers/mfd/arizona-irq.c#L72
> 

Never a good argument to make with me. 100 people doing something wrong
doesn't mean you should continue to do so.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Twiss May 18, 2015, 2:15 p.m. UTC | #6
On 15 May 2015 21:20 Guenter Roeck,

> > > > +
> > > > +/* E_WDG_WARN interrupt handler */
> > > > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void*data)
> > > > +{
> > > > +	struct da9062_watchdog *wdt = data;
> > > > +
> > > > +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> >
> > On 15 May 2015 13:58 Guenter Roeck wrote:
> >
> >  [...]
> >
> > > >>> +
> > > >>> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> > > >>> +	if (irq < 0) {
> > > >>> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> > > >>> +		ret = irq;
> > > >>> +		goto error;
> >
> > [...]
> >
> > > >
> > > >> Also, is the interrupt mandatory ? All it does is to display a message.
> > > >> Looks very optional to me.
> > > >
> > > > It is a place holder for something more application specific.
> > > > I could remove it, but I figured it would just get re-added when somebody takes the
> > > > driver and modifies it for their needs.
> > > >
> > > > If this is a problem however, it can go.
> > > > Please advise ..
> > > >
> > >
> > > Then this someone should add the code. For the time being, it just increases
> > > kernel size and may cause the driver to fail for no good reason. Plus, given
> > > the driver apparently works without interrupt, even then it should be
> > > optional, and the driver does not have to fail loading if it is not supported on a
> > > given platform.
> > >
> >
> > Hi Guenter,
> >
> > I'm not sure if I got my previous point across there ...
> >
> > Leaving this in wouldn't really do any real harm I think. If this feature is not supported
> > in somebody's platform then there wouldn't be a problem, the IRQ would fire (as a
> > warning that the watchdog was about to time-out), the handler function would be
> > executed, it would handle the IRQ -- and that would be it. Nothing would happen apart
> > from a debug print.
> 
> I didn't get my point across either. Problem is that your driver fails to load
> if the interrupt is not there. With the interrupt really being optional, I don't
> see the point in making it mandatory just to display a message if it fires.
> 

Hi Guenter,

Ok. I see now. 

It's not a mandatory interrupt and so it should not fail the whole driver upon an error from
the devm_request_threaded_irq() request ... I will let it pass through if there is a problem and
just display a debug message. Something like this:

@@ -234,11 +234,9 @@ static int da9062_wdt_probe(struct platform_device *pdev)
                           da9062_wdt_wdg_warn_irq_handler,
                            IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
                           "WDG_WARN", wdt);
-       if (ret) {
-               dev_err(wdt->hw->dev,
+       if (ret)
+               dev_dbg(wdt->hw->dev,
                        "Failed to request watchdog device IRQ.\n");
-               return ret;
-       }
 
        ret = watchdog_register_device(&wdt->wdtdev);
        if (ret < 0) {


I think I've understood now. I guess this is it..
I'll send a PATCH V3 with this change.

Regards,
Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck May 18, 2015, 3:27 p.m. UTC | #7
On Mon, May 18, 2015 at 02:15:01PM +0000, Opensource [Steve Twiss] wrote:
> On 15 May 2015 21:20 Guenter Roeck,
> 
> > > > > +
> > > > > +/* E_WDG_WARN interrupt handler */
> > > > > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void*data)
> > > > > +{
> > > > > +	struct da9062_watchdog *wdt = data;
> > > > > +
> > > > > +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > >
> > > On 15 May 2015 13:58 Guenter Roeck wrote:
> > >
> > >  [...]
> > >
> > > > >>> +
> > > > >>> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> > > > >>> +	if (irq < 0) {
> > > > >>> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> > > > >>> +		ret = irq;

Hi Steve,

Since the interrupt is optional, the driver should also not fail to load
if no interrupt is assigned to it in the first place.

On a separate note, there was a comment stating that the da9062 watchdog is
identical to the da9063 watchdog. If so, why can't you just use the da9063
watchdog driver ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Twiss May 18, 2015, 4:03 p.m. UTC | #8
On 18 May 2015 16:28 Guenter Roeck wrote:

> On Mon, May 18, 2015 at 02:15:01PM +0000, Opensource [Steve Twiss]
> wrote:
> > On 15 May 2015 21:20 Guenter Roeck,
> >
> > > > > > +
> > > > > > +/* E_WDG_WARN interrupt handler */
> > > > > > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void*data)
> > > > > > +{
> > > > > > +	struct da9062_watchdog *wdt = data;
> > > > > > +
> > > > > > +	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
> > > > > > +	return IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > >
> > > > On 15 May 2015 13:58 Guenter Roeck wrote:
> > > >
> > > >  [...]
> > > >
> > > > > >>> +
> > > > > >>> +	irq = platform_get_irq_byname(pdev, "WDG_WARN");
> > > > > >>> +	if (irq < 0) {
> > > > > >>> +		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
> > > > > >>> +		ret = irq;
> 
> Hi Steve,

Hi Guenter,

> Since the interrupt is optional, the driver should also not fail to load
> if no interrupt is assigned to it in the first place.

Yeah. I've been thinking about it and I agree now. I'll erase the handler. 

> On a separate note, there was a comment stating that the da9062 watchdog
> is identical to the da9063 watchdog. If so, why can't you just use the da9063
> watchdog driver ?

Well, the short answer to this is, it's not the same. I was just in the process of
replying to that other thread. The OnKey and RTC are functionally similar, so I
am going to look at integrating the two drivers in some future patch sets, but
the watchdog is definitely not based upon DA9063.

I did mention this in a previous thread: 
https://lkml.org/lkml/2015/5/6/505

Regards,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Belloni May 18, 2015, 5:39 p.m. UTC | #9
On 18/05/2015 at 16:03:06 +0000, Opensource [Steve Twiss] wrote :
> > Since the interrupt is optional, the driver should also not fail to load
> > if no interrupt is assigned to it in the first place.
> 
> Yeah. I've been thinking about it and I agree now. I'll erase the handler. 
> 
> > On a separate note, there was a comment stating that the da9062 watchdog
> > is identical to the da9063 watchdog. If so, why can't you just use the da9063
> > watchdog driver ?
> 
> Well, the short answer to this is, it's not the same. I was just in the process of
> replying to that other thread. The OnKey and RTC are functionally similar, so I
> am going to look at integrating the two drivers in some future patch sets, but
> the watchdog is definitely not based upon DA9063.
> 
> I did mention this in a previous thread: 
> https://lkml.org/lkml/2015/5/6/505
> 

Sure, what I understand is that the base functionality is the same and
even the registers are compatible. Are you sure the new features can't
be added to the da9063 and called conditionally? Plenty of drivers are
doing that.
Steve Twiss May 19, 2015, 10:16 a.m. UTC | #10
On 18 May 2015 18:39 Alexandre Belloni wrote:

> Sure, what I understand is that the base functionality is the same and
> even the registers are compatible. Are you sure the new features can't
> be added to the da9063 and called conditionally? Plenty of drivers are
> doing that.

Hi Alexandre/Guenter,

I didn't say it couldn't be added to DA9063. Of course it's all possible, but I think
it would become unmaintainable in future without separate drivers. There is some
extra freeze functionality and a watchdog warn interrupt: both do not exist in the
DA9063.

What I did say **one month ago** was the DA9062 watchdog driver does have some
similarities with the DA9063 watchdog (https://lkml.org/lkml/2015/5/6/505).

But I would prefer two drivers for watchdog in this case. When compared to the RTC and
OnKey the watchdog is very different on the hardware side. Despite the appearance of
some similarity, the watchdog is not a functional clone like the RTC and OnKey components.

I would not like a combined watchdog driver because I think that the extra functions
that the DA9062 contains would bloat out the DA9063 watchdog, and would make it difficult
to test and to maintain for the future..

Regards,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e5e7c55..dfdb6c6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -96,6 +96,15 @@  config DA9063_WATCHDOG
 
 	  This driver can be built as a module. The module name is da9063_wdt.
 
+config DA9062_WATCHDOG
+	tristate "Dialog DA9062 Watchdog"
+	depends on MFD_DA9062
+	select WATCHDOG_CORE
+	help
+	  Support for the watchdog in the DA9062 PMIC.
+
+	  This driver can be built as a module. The module name is da9062_wdt.
+
 config GPIO_WATCHDOG
 	tristate "Watchdog device controlled through GPIO-line"
 	depends on OF_GPIO
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 5c19294..57ba815 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -179,6 +179,7 @@  obj-$(CONFIG_XEN_WDT) += xen_wdt.o
 # Architecture Independent
 obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o
 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_WM831X_WATCHDOG) += wm831x_wdt.o
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
new file mode 100644
index 0000000..9e6c93b
--- /dev/null
+++ b/drivers/watchdog/da9062_wdt.c
@@ -0,0 +1,288 @@ 
+/*
+ * da9062_wdt.c - WDT device driver for DA9062
+ * Copyright (C) 2015  Dialog Semiconductor Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/mfd/da9062/registers.h>
+#include <linux/mfd/da9062/core.h>
+#include <linux/regmap.h>
+#include <linux/of.h>
+
+static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 };
+#define DA9062_TWDSCALE_DISABLE		0
+#define DA9062_TWDSCALE_MIN		1
+#define DA9062_TWDSCALE_MAX		(ARRAY_SIZE(wdt_timeout) - 1)
+#define DA9062_WDT_MIN_TIMEOUT		wdt_timeout[DA9062_TWDSCALE_MIN]
+#define DA9062_WDT_MAX_TIMEOUT		wdt_timeout[DA9062_TWDSCALE_MAX]
+#define DA9062_WDG_DEFAULT_TIMEOUT	wdt_timeout[DA9062_TWDSCALE_MAX-1]
+#define DA9062_RESET_PROTECTION_MS	300
+
+struct da9062_watchdog {
+	struct da9062 *hw;
+	struct watchdog_device wdtdev;
+	unsigned long j_time_stamp;
+};
+
+static void da9062_set_window_start(struct da9062_watchdog *wdt)
+{
+	wdt->j_time_stamp = jiffies;
+}
+
+static void da9062_apply_window_protection(struct da9062_watchdog *wdt)
+{
+	unsigned long delay = msecs_to_jiffies(DA9062_RESET_PROTECTION_MS);
+	unsigned long timeout = wdt->j_time_stamp + delay;
+	unsigned long now = jiffies;
+	unsigned int diff_ms;
+
+	/* if time-limit has not elapsed then wait for remainder */
+	if (time_before(now, timeout)) {
+		diff_ms = jiffies_to_msecs(timeout-now);
+		dev_dbg(wdt->hw->dev,
+			"Kicked too quickly. Delaying %u msecs\n", diff_ms);
+		msleep(diff_ms);
+	}
+
+	return;
+}
+
+static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs)
+{
+	unsigned int i;
+
+	for (i = DA9062_TWDSCALE_MIN; i <= DA9062_TWDSCALE_MAX; i++) {
+		if (wdt_timeout[i] >= secs)
+			return i;
+	}
+
+	return DA9062_TWDSCALE_MAX;
+}
+
+static int da9062_reset_watchdog_timer(struct da9062_watchdog *wdt)
+{
+	int ret;
+
+	da9062_apply_window_protection(wdt);
+
+	ret = regmap_update_bits(wdt->hw->regmap,
+			   DA9062AA_CONTROL_F,
+			   DA9062AA_WATCHDOG_MASK,
+			   DA9062AA_WATCHDOG_MASK);
+
+	da9062_set_window_start(wdt);
+
+	return ret;
+}
+
+static int da9062_wdt_update_timeout_register(struct da9062_watchdog *wdt,
+					      unsigned int regval)
+{
+	struct da9062 *chip = wdt->hw;
+	int ret;
+
+	ret = da9062_reset_watchdog_timer(wdt);
+	if (ret) {
+		dev_err(chip->dev, "Failed to ping the watchdog (err = %d)\n",
+			ret);
+		return ret;
+	}
+
+	return regmap_update_bits(chip->regmap,
+				  DA9062AA_CONTROL_D,
+				  DA9062AA_TWDSCALE_MASK,
+				  regval);
+}
+
+static int da9062_wdt_start(struct watchdog_device *wdd)
+{
+	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	unsigned int selector;
+	int ret;
+
+	selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout);
+	ret = da9062_wdt_update_timeout_register(wdt, selector);
+	if (ret)
+		dev_err(wdt->hw->dev, "Watchdog failed to start (err = %d)\n",
+			ret);
+
+	return ret;
+}
+
+static int da9062_wdt_stop(struct watchdog_device *wdd)
+{
+	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	ret = da9062_reset_watchdog_timer(wdt);
+	if (ret) {
+		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
+			ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(wdt->hw->regmap,
+				 DA9062AA_CONTROL_D,
+				 DA9062AA_TWDSCALE_MASK,
+				 DA9062_TWDSCALE_DISABLE);
+	if (ret)
+		dev_alert(wdt->hw->dev, "Watchdog failed to stop (err = %d)\n",
+			  ret);
+
+	return ret;
+}
+
+static int da9062_wdt_ping(struct watchdog_device *wdd)
+{
+	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	dev_dbg(wdt->hw->dev, "watchdog ping\n");
+
+	ret = da9062_reset_watchdog_timer(wdt);
+	if (ret)
+		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n",
+			ret);
+
+	return ret;
+}
+
+static int da9062_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int timeout)
+{
+	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	unsigned int selector;
+	int ret;
+
+	selector = da9062_wdt_timeout_to_sel(timeout);
+	ret = da9062_wdt_update_timeout_register(wdt, selector);
+	if (ret)
+		dev_err(wdt->hw->dev, "Failed to set watchdog timeout (err = %d)\n",
+			ret);
+	else
+		wdd->timeout = wdt_timeout[selector];
+
+	return ret;
+}
+
+/* E_WDG_WARN interrupt handler */
+static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data)
+{
+	struct da9062_watchdog *wdt = data;
+
+	dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n");
+	return IRQ_HANDLED;
+}
+
+static const struct watchdog_info da9062_watchdog_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "DA9062 WDT",
+};
+
+static const struct watchdog_ops da9062_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = da9062_wdt_start,
+	.stop = da9062_wdt_stop,
+	.ping = da9062_wdt_ping,
+	.set_timeout = da9062_wdt_set_timeout,
+};
+
+static int da9062_wdt_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct da9062 *chip;
+	struct da9062_watchdog *wdt;
+	int irq;
+
+	chip = dev_get_drvdata(pdev->dev.parent);
+	if (!chip)
+		return -EINVAL;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->hw = chip;
+
+	wdt->wdtdev.info = &da9062_watchdog_info;
+	wdt->wdtdev.ops = &da9062_watchdog_ops;
+	wdt->wdtdev.min_timeout = DA9062_WDT_MIN_TIMEOUT;
+	wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT;
+	wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT;
+	wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	watchdog_set_drvdata(&wdt->wdtdev, wdt);
+	dev_set_drvdata(&pdev->dev, wdt);
+
+	irq = platform_get_irq_byname(pdev, "WDG_WARN");
+	if (irq < 0) {
+		dev_err(wdt->hw->dev, "Failed to get IRQ.\n");
+		ret = irq;
+		goto error;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+			   da9062_wdt_wdg_warn_irq_handler,
+			    IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
+			   "WDG_WARN", wdt);
+	if (ret) {
+		dev_err(wdt->hw->dev,
+			"Failed to request watchdog device IRQ.\n");
+		goto error;
+	}
+
+	ret = watchdog_register_device(&wdt->wdtdev);
+	if (ret < 0) {
+		dev_err(wdt->hw->dev,
+			"watchdog registration incomplete (%d)\n", ret);
+		goto error;
+	}
+
+	da9062_set_window_start(wdt);
+
+	ret = da9062_wdt_ping(&wdt->wdtdev);
+	if (ret < 0)
+		watchdog_unregister_device(&wdt->wdtdev);
+
+error:
+	return ret;
+}
+
+static int da9062_wdt_remove(struct platform_device *pdev)
+{
+	struct da9062_watchdog *wdt = dev_get_drvdata(&pdev->dev);
+
+	watchdog_unregister_device(&wdt->wdtdev);
+	return 0;
+}
+
+static struct platform_driver da9062_wdt_driver = {
+	.probe = da9062_wdt_probe,
+	.remove = da9062_wdt_remove,
+	.driver = {
+		.name = "da9062-watchdog",
+	},
+};
+module_platform_driver(da9062_wdt_driver);
+
+MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
+MODULE_DESCRIPTION("WDT device driver for Dialog DA9062");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform: da9062-watchdog");