da9062: DA9062 driver submission

drivers/mfd/da9062-core.c new file +14 -0

new file mode 100644
index 0000000..e6a9878
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+
+#include <linux/irq.h>
+#include <linux/mfd/core.h>
+#include <linux/i2c.h>
+#include <linux/err.h>

drivers/mfd/da9062-core.c new file +13 -0

+	{
+		.name		= "da9062-watchdog",
+		.num_resources	= ARRAY_SIZE(da9062_wdt_resources),
+		.resources	= da9062_wdt_resources,
+		.of_compatible  = "dlg,da9062-wdt",
+	},
+	{
+		.name		= "da9062-onkey",
+		.num_resources	= ARRAY_SIZE(da9062_onkey_resources),
+		.resources	= da9062_onkey_resources,
+		.of_compatible  = "dlg,da9062-onkey",
+	},
+	{

Quoted 19 lines

drivers/mfd/da9062-core.c new file +19 -0

+	{
+		.name		= "da9062-watchdog",
+		.num_resources	= ARRAY_SIZE(da9062_wdt_resources),
+		.resources	= da9062_wdt_resources,
+		.of_compatible  = "dlg,da9062-wdt",
+	},
+	{
+		.name		= "da9062-onkey",
+		.num_resources	= ARRAY_SIZE(da9062_onkey_resources),
+		.resources	= da9062_onkey_resources,
+		.of_compatible  = "dlg,da9062-onkey",
+	},
+	{
+		.name		= "da9062-thermal",
+		.num_resources	= ARRAY_SIZE(da9062_thermal_resources),
+		.resources	= da9062_thermal_resources,
+		.of_compatible  = "dlg,da9062-thermal",
+	},
+	{

drivers/mfd/da9062-core.c line 242

Quoted 4 lines

drivers/mfd/da9062-core.c new file +4 -0

+		.name		= "da9062-rtc",
+		.num_resources	= ARRAY_SIZE(da9062_rtc_resources),
+		.resources	= da9062_rtc_resources,
+		.of_compatible  = "dlg,da9062-rtc",

Did you try to use "da9063-rtc"? The register set seems to be exactly the same. Unfortunately, the datasheet are not available on the diasemi website...

Also, the .of_compatibles are not necessary because you don't add any of bindings to the underlying drivers. The match happens on .name.

Review

Quoted 1 line

(quoted diff) +0 -0

b/include/linux/mfd/da9062/registers.h

include/linux/mfd/da9062/registers.h

Quoted 4 lines

new file mode 100644
index 0000000..d07c2bc

include/linux/mfd/da9062/registers.h new file +0 -0

Comparing that file with da9063/registers.h, It really seems that DA062AA, DA9063AD and DA9063BB are register compatible, apart from a few
differences in the regulator and the gpio count.

Also, at least the watchdog and rtc driver are duplicating their da9063 counterpart. I'm not trying to annoy you, I just want you to understand that the less code is duplicated, the easiest it will be to maintain later.

From: S Twiss <[email protected]>

Add watchdog driver support for DA9062

Signed-off-by Steve Twiss <[email protected]>

---

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

drivers/watchdog/Kconfig +9 -0

index e5e7c55..dfdb6c6 100644
@@ -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

drivers/watchdog/Makefile +1 -0

index 5c19294..57ba815 100644
@@ -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

drivers/watchdog/da9062_wdt.c new file +54 -0

new file mode 100644
index 0000000..9e6c93b
@@ -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;

drivers/watchdog/da9062_wdt.c new file +30 -0

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

drivers/watchdog/da9062_wdt.c new file +26 -0

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

drivers/watchdog/da9062_wdt.c new file +70 -0

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

drivers/watchdog/da9062_wdt.c new file +6 -0

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

drivers/watchdog/da9062_wdt.c new file +24 -0

+		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,

Subject: Re: [PATCH V2 3/4] watchdog: da9062: DA9062 watchdog driver

Quoted 1 line

+static void da9062_apply_window_protection(struct da9062_watchdog

*wdt)

Review

Quoted 2 lines

(quoted diff) +2 -0

+{
+	unsigned long delay =

msecs_to_jiffies(DA9062_RESET_PROTECTION_MS);

drivers/watchdog/da9062_wdt.c line 64

Quoted 13 lines

drivers/watchdog/da9062_wdt.c new file +13 -0

+	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.

Quoted 28 lines

drivers/watchdog/da9062_wdt.c new file +28 -0

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

drivers/watchdog/da9062_wdt.c line 104

Quoted 10 lines

drivers/watchdog/da9062_wdt.c new file +10 -0

+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 ?

drivers/watchdog/da9062_wdt.c line 111

Quoted 7 lines

drivers/watchdog/da9062_wdt.c new file +7 -0

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

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

drivers/watchdog/da9062_wdt.c

Quoted 8 lines

drivers/watchdog/da9062_wdt.c new file +8 -0

+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",

drivers/watchdog/da9062_wdt.c

Quoted 10 lines

drivers/watchdog/da9062_wdt.c new file +10 -0

+			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",

Review

Quoted 1 line

(quoted diff) +1 -0

+			  ret);

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

Quoted 5 lines

drivers/watchdog/da9062_wdt.c new file +5 -0

+
+	return ret;
+}
+
+static int da9062_wdt_ping(struct watchdog_device *wdd)

drivers/watchdog/da9062_wdt.c line 158

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +6 -0

+{
+	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 ?

drivers/watchdog/da9062_wdt.c

Quoted 3 lines

drivers/watchdog/da9062_wdt.c new file +3 -0

+	ret = da9062_reset_watchdog_timer(wdt);
+	if (ret)
+		dev_err(wdt->hw->dev, "Failed to ping the watchdog (err =

%d)\n",

Quoted 5 lines

drivers/watchdog/da9062_wdt.c new file +5 -0

+			ret);
+
+	return ret;
+}
+

Quoted 10 lines

drivers/watchdog/da9062_wdt.c new file +10 -0

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

Quoted 27 lines

drivers/watchdog/da9062_wdt.c new file +27 -0

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

drivers/watchdog/da9062_wdt.c line 238

Quoted 5 lines

drivers/watchdog/da9062_wdt.c new file +5 -0

+	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.

drivers/watchdog/da9062_wdt.c

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +5 -0

+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+			   da9062_wdt_wdg_warn_irq_handler,
+			    IRQF_TRIGGER_LOW | IRQF_ONESHOT |

IRQF_SHARED,

drivers/watchdog/da9062_wdt.c line 254

Quoted 11 lines

drivers/watchdog/da9062_wdt.c new file +11 -0

+			   "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" ?

Quoted 8 lines

drivers/watchdog/da9062_wdt.c new file +8 -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);

drivers/watchdog/da9062_wdt.c line 288

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +5 -0

+
+MODULE_AUTHOR("S Twiss <[email protected]>");
+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 ?

Hi Guenter,

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

Regards,
Steve

On 15 May 2015 03:13, Guenter Roeck

Quoted 2 lines

Subject: Re: [PATCH V2 3/4] watchdog: da9062: DA9062 watchdog driver

[...]

Quoted 9 lines

drivers/watchdog/da9062_wdt.c line 238

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +6 -0

+
+	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.

Quoted 2 lines

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 ..

drivers/watchdog/da9062_wdt.c line 193

Quoted 11 lines

drivers/watchdog/da9062_wdt.c new file +10 -0

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

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +6 -0

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

Quoted 11 lines

Quoted 3 lines

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.

Quoted 11 lines

drivers/watchdog/da9062_wdt.c line 193

Quoted 11 lines

drivers/watchdog/da9062_wdt.c new file +10 -0

+
+/* 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:

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +6 -0

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

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

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 ..

Quoted 6 lines

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

Quoted 45 lines

Quoted 10 lines

drivers/watchdog/da9062_wdt.c line 193

Quoted 10 lines

drivers/watchdog/da9062_wdt.c new file +10 -0

+
+/* 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:

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/watchdog/da9062_wdt.c new file +6 -0

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

[...]

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

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 ..

Quoted 6 lines

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,

Quoted 7 lines

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.

On 15 May 2015 21:20 Guenter Roeck,

Quoted 13 lines

Quoted 13 lines

Quoted 10 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+/* 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:

Quoted 7 lines

Quoted 7 lines

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Quoted 17 lines · Opensource [Steve Twiss]

On 15 May 2015 21:20 Guenter Roeck,

Quoted 15 lines

Quoted 15 lines

Quoted 10 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+/* 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:

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+	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 ?

Quoted 2 lines

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.

Quoted 3 lines

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.

From: S Twiss <[email protected]>

This patch set adds support for the Dialog DA9062 Power Management IC.

In this patch set the following is provided:
- [PATCH V2 1/4]: MFD core support
- [PATCH V2 2/4]: BUCK and LDO regulator driver
- [PATCH V2 3/4]: Watchdog driver
- [PATCH V2 4/4]: Add bindings for all DA9062 components

Quoted 4 lines

From: S Twiss <[email protected]>

This patch set adds support for the Dialog DA9062 Power Management IC.

Quoted 5 lines

In this patch set the following is provided:
- [PATCH V2 1/4]: MFD core support
- [PATCH V2 2/4]: BUCK and LDO regulator driver
- [PATCH V2 3/4]: Watchdog driver
- [PATCH V2 4/4]: Add bindings for all DA9062 components

This patch should actually be the first one to go in, else checkpatch will complain about compatibles not being found.