[PATCH v2] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[PATCH v2] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Kyle Roeschley]

Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
time controllers.

Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
---
 Documentation/watchdog/watchdog-parameters.txt |   5 +
 drivers/watchdog/Kconfig                       |  11 +
 drivers/watchdog/Makefile                      |   1 +
 drivers/watchdog/ni9x3x_wdt.c                  | 274 +++++++++++++++++++++++++
 4 files changed, 291 insertions(+)
 create mode 100644 drivers/watchdog/ni9x3x_wdt.c

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 9f9ec9f..5273346 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -200,6 +200,11 @@ mv64x60_wdt:
 nowayout: Watchdog cannot be stopped once started
 	(default=kernel config parameter)
 -------------------------------------------------
+ni9x3x_wdt:
+timeout: Intial watchdog timeout in seconds (0<timeout<516, default=60)
+nowayout: Watchdog cannot be stopped once started
+	(default=kernel config parameter)
+-------------------------------------------------
 nuc900_wdt:
 heartbeat: Watchdog heartbeats in seconds.
 	(default = 15)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0f6d851..18bd13a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1214,6 +1214,17 @@ config SBC_EPX_C3_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called sbc_epx_c3.
 
+config NI9X3X_WDT
+	tristate "NI 903x/913x Watchdog"
+	depends on X86 && ACPI
+	select WATCHDOG_CORE
+	---help---
+	  This is the driver for the watchdog timer on the National Instruments
+	  903x/913x real-time controllers.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ni9x3x_wdt.
+
 # M32R Architecture
 
 # M68K Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f566753..527978b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
 obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
 obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
 obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
+obj-$(CONFIG_NI9X3X_WDT) += ni9x3x_wdt.o
 
 # M32R Architecture
 
diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
new file mode 100644
index 0000000..efc8dc6
--- /dev/null
+++ b/drivers/watchdog/ni9x3x_wdt.c
@@ -0,0 +1,274 @@
+/*
+ * Copyright (C) 2013 National Instruments Corp.
+ *
+ * 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/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#define NIWD_CONTROL	0x01
+#define NIWD_COUNTER2	0x02
+#define NIWD_COUNTER1	0x03
+#define NIWD_COUNTER0	0x04
+#define NIWD_SEED2	0x05
+#define NIWD_SEED1	0x06
+#define NIWD_SEED0	0x07
+
+#define NIWD_IO_SIZE	0x08
+
+#define NIWD_CONTROL_MODE		0x80
+#define NIWD_CONTROL_PROC_RESET		0x20
+#define NIWD_CONTROL_PET		0x10
+#define NIWD_CONTROL_RUNNING		0x08
+#define NIWD_CONTROL_CAPTURECOUNTER	0x04
+#define NIWD_CONTROL_RESET		0x02
+#define NIWD_CONTROL_ALARM		0x01
+
+#define NIWD_PERIOD_NS		30720
+#define NIWD_MIN_TIMEOUT	1
+#define NIWD_MAX_TIMEOUT	515
+#define NIWD_DEFAULT_TIMEOUT	60
+
+#define NIWD_NAME		"ni9x3x_wdt"
+
+struct ni9x3x_wdt {
+	struct device *dev;
+	u16 io_base;
+	struct watchdog_device wdd;
+};
+
+static unsigned int timeout;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout,
+		 "Watchdog timeout in seconds. (default="
+		 __MODULE_STRING(NIWD_DEFAULT_TIMEOUT) ")");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, S_IRUGO);
+MODULE_PARM_DESC(nowayout,
+		 "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static void ni9x3x_start(struct ni9x3x_wdt *wdt)
+{
+	u8 control = inb(wdt->io_base + NIWD_CONTROL);
+
+	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
+}
+
+static int ni9x3x_wdd_set_timeout(struct watchdog_device *wdd,
+				  unsigned int timeout)
+{
+	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
+	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
+
+	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
+	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
+	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
+
+	wdd->timeout = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
+
+	return 0;
+}
+
+static unsigned int ni9x3x_wdd_get_timeleft(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
+	u8 control, counter0, counter1, counter2;
+	u32 counter;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+	control |= NIWD_CONTROL_CAPTURECOUNTER;
+	outb(control, wdt->io_base + NIWD_CONTROL);
+
+	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
+	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
+	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
+
+	counter = (counter2 << 16) | (counter1 << 8) | counter0;
+	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
+
+	return counter;
+}
+
+static int ni9x3x_wdd_ping(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
+	u8 control;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
+
+	return 0;
+}
+
+static int ni9x3x_wdd_start(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_PROC_RESET,
+	     wdt->io_base + NIWD_CONTROL);
+
+	ni9x3x_wdd_set_timeout(wdd, wdd->timeout);
+	ni9x3x_start(wdt);
+
+	return 0;
+}
+
+static int ni9x3x_wdd_stop(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	outb(NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
+
+	return 0;
+}
+
+static acpi_status ni9x3x_resources(struct acpi_resource *res, void *data)
+{
+	struct ni9x3x_wdt *wdt = data;
+	u16 io_size;
+
+	switch (res->type) {
+	case ACPI_RESOURCE_TYPE_IO:
+		if (wdt->io_base != 0) {
+			dev_err(wdt->dev, "too many IO resources\n");
+			return AE_ERROR;
+		}
+
+		wdt->io_base = res->data.io.minimum;
+		io_size = res->data.io.address_length;
+
+		if (!devm_request_region(wdt->dev, wdt->io_base, io_size,
+					 NIWD_NAME)) {
+			dev_err(wdt->dev, "failed to get memory region\n");
+			return -EBUSY;
+		}
+
+		return AE_OK;
+
+	case ACPI_RESOURCE_TYPE_IRQ:
+		/* Discard IRQ resource since we don't use it */
+	case ACPI_RESOURCE_TYPE_END_TAG:
+		return AE_OK;
+
+	default:
+		dev_err(wdt->dev, "unsupported resource type %d\n", res->type);
+		return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+static const struct watchdog_info ni9x3x_wdd_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "NI Watchdog",
+};
+
+static const struct watchdog_ops ni9x3x_wdd_ops = {
+	.owner = THIS_MODULE,
+	.start = ni9x3x_wdd_start,
+	.stop = ni9x3x_wdd_stop,
+	.ping = ni9x3x_wdd_ping,
+	.set_timeout = ni9x3x_wdd_set_timeout,
+	.get_timeleft = ni9x3x_wdd_get_timeleft,
+};
+
+static int ni9x3x_acpi_add(struct acpi_device *device)
+{
+	struct device *dev = &device->dev;
+	struct watchdog_device *wdd;
+	struct ni9x3x_wdt *wdt;
+	acpi_status status;
+	int ret;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	device->driver_data = wdt;
+	wdt->dev = dev;
+
+	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+				     ni9x3x_resources, wdt);
+	if (ACPI_FAILURE(status) || wdt->io_base == 0) {
+		dev_err(dev, "failed to get resources\n");
+		return -ENODEV;
+	}
+
+	wdd = &wdt->wdd;
+	wdd->info = &ni9x3x_wdd_info;
+	wdd->ops = &ni9x3x_wdd_ops;
+	wdd->min_timeout = NIWD_MIN_TIMEOUT;
+	wdd->max_timeout = NIWD_MAX_TIMEOUT;
+	wdd->timeout = NIWD_DEFAULT_TIMEOUT;
+	wdd->parent = dev;
+	watchdog_set_drvdata(wdd, wdt);
+	watchdog_set_nowayout(wdd, nowayout);
+	ret = watchdog_init_timeout(wdd, timeout, dev);
+	if (ret) {
+		dev_err(dev, "unable to set timeout value\n");
+		return ret;
+	}
+
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(dev, "failed to register watchdog\n");
+		return ret;
+	}
+
+	/* Switch from boot mode to user mode */
+	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
+	     wdt->io_base + NIWD_CONTROL);
+
+	dev_dbg(dev, "io_base=0x%04X, timeout=%u, nowayout=%s\n",
+		wdt->io_base, timeout, nowayout ? "true" : "false");
+
+	return 0;
+}
+
+static int ni9x3x_acpi_remove(struct acpi_device *device)
+{
+	struct ni9x3x_wdt *wdt = acpi_driver_data(device);
+
+	ni9x3x_wdd_stop(&wdt->wdd);
+	watchdog_unregister_device(&wdt->wdd);
+
+	return 0;
+}
+
+static const struct acpi_device_id ni9x3x_device_ids[] = {
+	{"NIC775C", 0},
+	{"", 0},
+};
+MODULE_DEVICE_TABLE(acpi, ni9x3x_device_ids);
+
+static struct acpi_driver ni9x3x_acpi_driver = {
+	.name = NIWD_NAME,
+	.ids = ni9x3x_device_ids,
+	.ops = {
+		.add = ni9x3x_acpi_add,
+		.remove = ni9x3x_acpi_remove,
+		},
+};
+
+module_acpi_driver(ni9x3x_acpi_driver);
+
+MODULE_DESCRIPTION("NI Watchdog");
+MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
+MODULE_AUTHOR("Kyle Roeschley <kyle.roeschley@ni.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.0

Re: [PATCH v2] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Guenter Roeck]

On 02/18/2016 03:26 PM, Kyle Roeschley wrote:
> Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
> time controllers.
>
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> ---

This would be an optimal place for a change log.

[ if any of my comments are duplicates, sorry, but a change log
   would help avoiding that ]

>   Documentation/watchdog/watchdog-parameters.txt |   5 +
>   drivers/watchdog/Kconfig                       |  11 +
>   drivers/watchdog/Makefile                      |   1 +
>   drivers/watchdog/ni9x3x_wdt.c                  | 274 +++++++++++++++++++++++++

The name and context make me wonder: Is this driver going to support
all watchdog devices for NI9[0-9]3[0-9] ?

If not, it may be better to select one supported device,
whatever that may be, for the name, and describe in Kconfig
and the documentation which devices the driver is known to support.

>   4 files changed, 291 insertions(+)
>   create mode 100644 drivers/watchdog/ni9x3x_wdt.c
>
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index 9f9ec9f..5273346 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -200,6 +200,11 @@ mv64x60_wdt:
>   nowayout: Watchdog cannot be stopped once started
>   	(default=kernel config parameter)
>   -------------------------------------------------
> +ni9x3x_wdt:
> +timeout: Intial watchdog timeout in seconds (0<timeout<516, default=60)
> +nowayout: Watchdog cannot be stopped once started
> +	(default=kernel config parameter)
> +-------------------------------------------------
>   nuc900_wdt:
>   heartbeat: Watchdog heartbeats in seconds.
>   	(default = 15)
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0f6d851..18bd13a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1214,6 +1214,17 @@ config SBC_EPX_C3_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called sbc_epx_c3.
>
> +config NI9X3X_WDT
> +	tristate "NI 903x/913x Watchdog"

So is it NI9[01]3[0-9] ?

What if NI923x requires a different driver ?

> +	depends on X86 && ACPI
> +	select WATCHDOG_CORE
> +	---help---
> +	  This is the driver for the watchdog timer on the National Instruments
> +	  903x/913x real-time controllers.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ni9x3x_wdt.
> +
>   # M32R Architecture
>
>   # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f566753..527978b 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -126,6 +126,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
>   obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
>   obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
>   obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> +obj-$(CONFIG_NI9X3X_WDT) += ni9x3x_wdt.o
>
>   # M32R Architecture
>
> diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
> new file mode 100644
> index 0000000..efc8dc6
> --- /dev/null
> +++ b/drivers/watchdog/ni9x3x_wdt.c
> @@ -0,0 +1,274 @@
> +/*
> + * Copyright (C) 2013 National Instruments Corp.
> + *
2013 ? Sure you don't want to add 2016 ?

> + * 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/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +
> +#define NIWD_CONTROL	0x01
> +#define NIWD_COUNTER2	0x02
> +#define NIWD_COUNTER1	0x03
> +#define NIWD_COUNTER0	0x04
> +#define NIWD_SEED2	0x05
> +#define NIWD_SEED1	0x06
> +#define NIWD_SEED0	0x07
> +
> +#define NIWD_IO_SIZE	0x08
> +
> +#define NIWD_CONTROL_MODE		0x80
> +#define NIWD_CONTROL_PROC_RESET		0x20
> +#define NIWD_CONTROL_PET		0x10
> +#define NIWD_CONTROL_RUNNING		0x08
> +#define NIWD_CONTROL_CAPTURECOUNTER	0x04
> +#define NIWD_CONTROL_RESET		0x02
> +#define NIWD_CONTROL_ALARM		0x01
> +
> +#define NIWD_PERIOD_NS		30720
> +#define NIWD_MIN_TIMEOUT	1
> +#define NIWD_MAX_TIMEOUT	515
> +#define NIWD_DEFAULT_TIMEOUT	60
> +
> +#define NIWD_NAME		"ni9x3x_wdt"
> +
> +struct ni9x3x_wdt {
> +	struct device *dev;
> +	u16 io_base;
> +	struct watchdog_device wdd;
> +};
> +
> +static unsigned int timeout;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +		 "Watchdog timeout in seconds. (default="
> +		 __MODULE_STRING(NIWD_DEFAULT_TIMEOUT) ")");
> +
> +static int nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, int, S_IRUGO);
> +MODULE_PARM_DESC(nowayout,
> +		 "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static void ni9x3x_start(struct ni9x3x_wdt *wdt)
> +{
> +	u8 control = inb(wdt->io_base + NIWD_CONTROL);
> +
> +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);

Using inb/outb asks for including linux/io.h.

> +}
> +
> +static int ni9x3x_wdd_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> +
> +	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
> +	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
> +	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
> +
> +	wdd->timeout = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;

Unless I am missing something, this should result in a link time failure
when building for 32 bit. Either use div_u64(), or something like

	wdd->timeout = counter / (1000000000 / NIWD_PERIOD_NS)

Though wouldn't that be the same as 'timeout' ? Are you trying to work around
some rounding error here ?

> +
> +	return 0;
> +}
> +
> +static unsigned int ni9x3x_wdd_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u8 control, counter0, counter1, counter2;
> +	u32 counter;
> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +	control |= NIWD_CONTROL_CAPTURECOUNTER;
> +	outb(control, wdt->io_base + NIWD_CONTROL);
> +
> +	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
> +	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
> +	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
> +
> +	counter = (counter2 << 16) | (counter1 << 8) | counter0;
> +	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
> +

Same here. You have to be careful when using 64 bit arithmetic.

> +	return counter;
> +}
> +
> +static int ni9x3x_wdd_ping(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
> +	u8 control;
> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
> +
> +	return 0;
> +}
> +
> +static int ni9x3x_wdd_start(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_PROC_RESET,
> +	     wdt->io_base + NIWD_CONTROL);
> +
> +	ni9x3x_wdd_set_timeout(wdd, wdd->timeout);
> +	ni9x3x_start(wdt);
> +
> +	return 0;
> +}
> +
> +static int ni9x3x_wdd_stop(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	outb(NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +
> +	return 0;
> +}
> +
> +static acpi_status ni9x3x_resources(struct acpi_resource *res, void *data)
> +{
> +	struct ni9x3x_wdt *wdt = data;
> +	u16 io_size;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_IO:
> +		if (wdt->io_base != 0) {
> +			dev_err(wdt->dev, "too many IO resources\n");

dev__err and similar functions are declared in linux/device.h,
which should be included.

> +			return AE_ERROR;
> +		}
> +
> +		wdt->io_base = res->data.io.minimum;
> +		io_size = res->data.io.address_length;
> +

What if io_size is less than NIWD_IO_SIZE ?

> +		if (!devm_request_region(wdt->dev, wdt->io_base, io_size,

linux/ioport.h

> +					 NIWD_NAME)) {
> +			dev_err(wdt->dev, "failed to get memory region\n");
> +			return -EBUSY;

Shouldn't this return AE_ERROR ?

> +		}
> +
> +		return AE_OK;
> +
> +	case ACPI_RESOURCE_TYPE_IRQ:
> +		/* Discard IRQ resource since we don't use it */
> +	case ACPI_RESOURCE_TYPE_END_TAG:
> +		return AE_OK;
> +
> +	default:
> +		dev_err(wdt->dev, "unsupported resource type %d\n", res->type);

I am wondering ... who cares ? Is there a reason for reporting an error
in this case, instead of just ignoring the unknown field ?

> +		return AE_ERROR;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static const struct watchdog_info ni9x3x_wdd_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,

WDIOF_MAGICCLOSE ?

> +	.identity = "NI Watchdog",
> +};
> +
> +static const struct watchdog_ops ni9x3x_wdd_ops = {
> +	.owner = THIS_MODULE,
> +	.start = ni9x3x_wdd_start,
> +	.stop = ni9x3x_wdd_stop,
> +	.ping = ni9x3x_wdd_ping,
> +	.set_timeout = ni9x3x_wdd_set_timeout,
> +	.get_timeleft = ni9x3x_wdd_get_timeleft,
> +};
> +
> +static int ni9x3x_acpi_add(struct acpi_device *device)
> +{
> +	struct device *dev = &device->dev;
> +	struct watchdog_device *wdd;
> +	struct ni9x3x_wdt *wdt;
> +	acpi_status status;
> +	int ret;
> +
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);

linux/device.h, already mentioned.

> +	if (!wdt) {
> +		dev_err(dev, "failed to allocate memory\n");

Unnecessary (duplicate) error message; devm_kzalloc() already creates one.

> +		return -ENOMEM;
> +	}
> +
> +	device->driver_data = wdt;
> +	wdt->dev = dev;
> +
> +	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +				     ni9x3x_resources, wdt);
> +	if (ACPI_FAILURE(status) || wdt->io_base == 0) {
> +		dev_err(dev, "failed to get resources\n");
> +		return -ENODEV;
> +	}
> +
> +	wdd = &wdt->wdd;
> +	wdd->info = &ni9x3x_wdd_info;
> +	wdd->ops = &ni9x3x_wdd_ops;
> +	wdd->min_timeout = NIWD_MIN_TIMEOUT;
> +	wdd->max_timeout = NIWD_MAX_TIMEOUT;
> +	wdd->timeout = NIWD_DEFAULT_TIMEOUT;
> +	wdd->parent = dev;
> +	watchdog_set_drvdata(wdd, wdt);
> +	watchdog_set_nowayout(wdd, nowayout);
> +	ret = watchdog_init_timeout(wdd, timeout, dev);
> +	if (ret) {
> +		dev_err(dev, "unable to set timeout value\n");

Sure you want to abort here, instead of using the default ?

> +		return ret;
> +	}
> +
> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(dev, "failed to register watchdog\n");
> +		return ret;
> +	}
> +
> +	/* Switch from boot mode to user mode */
> +	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
> +	     wdt->io_base + NIWD_CONTROL);
> +
> +	dev_dbg(dev, "io_base=0x%04X, timeout=%u, nowayout=%s\n",
> +		wdt->io_base, timeout, nowayout ? "true" : "false");
> +
This would be the first driver to display true/false instead of 0/1.
Let's stick with 0/1 for simplicity and consistency.

> +	return 0;
> +}
> +
> +static int ni9x3x_acpi_remove(struct acpi_device *device)
> +{
> +	struct ni9x3x_wdt *wdt = acpi_driver_data(device);
> +
> +	ni9x3x_wdd_stop(&wdt->wdd);
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id ni9x3x_device_ids[] = {
> +	{"NIC775C", 0},
> +	{"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, ni9x3x_device_ids);
> +
> +static struct acpi_driver ni9x3x_acpi_driver = {
> +	.name = NIWD_NAME,
> +	.ids = ni9x3x_device_ids,
> +	.ops = {
> +		.add = ni9x3x_acpi_add,
> +		.remove = ni9x3x_acpi_remove,
> +		},

Indentation - closing bracket should align with .ops

> +};
> +
> +module_acpi_driver(ni9x3x_acpi_driver);
> +
> +MODULE_DESCRIPTION("NI Watchdog");
> +MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
> +MODULE_AUTHOR("Kyle Roeschley <kyle.roeschley@ni.com>");
> +MODULE_LICENSE("GPL v2");
>
Text above is more generic. Please either change the text to
limit the license to GPL v2, or change the MODULE_LICENSE to GPL.

Re: [PATCH v2] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Kyle Roeschley]

On Thu, Feb 18, 2016 at 09:27:05PM -0800, Guenter Roeck wrote:
> On 02/18/2016 03:26 PM, Kyle Roeschley wrote:
> >Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
> >time controllers.
> >
> >Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> >Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> >---
> 
> This would be an optimal place for a change log.
> 
> [ if any of my comments are duplicates, sorry, but a change log
>   would help avoiding that ]
> 

Sorry about that, I'll make sure to do so in the future.

> >  Documentation/watchdog/watchdog-parameters.txt |   5 +
> >  drivers/watchdog/Kconfig                       |  11 +
> >  drivers/watchdog/Makefile                      |   1 +
> >  drivers/watchdog/ni9x3x_wdt.c                  | 274 +++++++++++++++++++++++++
> 
> The name and context make me wonder: Is this driver going to support
> all watchdog devices for NI9[0-9]3[0-9] ?
> 
> If not, it may be better to select one supported device,
> whatever that may be, for the name, and describe in Kconfig
> and the documentation which devices the driver is known to support.
> 

All of the 9[2-9]3[0-9] devices but the 963x do not and cannot run Linux,
so they won't be a problem. There is already a driver for the 963x watchdog in
our tree which is currently third in line for upstreaming, so that should be
fine as well. That would lead me to think 9x3x is fine, but if not then 903x is
fine.

> >  4 files changed, 291 insertions(+)
> >  create mode 100644 drivers/watchdog/ni9x3x_wdt.c
[...]
> >+
> >+static int ni9x3x_wdd_set_timeout(struct watchdog_device *wdd,
> >+				  unsigned int timeout)
> >+{
> >+	struct ni9x3x_wdt *wdt = watchdog_get_drvdata(wdd);
> >+	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> >+
> >+	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
> >+	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
> >+	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
> >+
> >+	wdd->timeout = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
> 
> Unless I am missing something, this should result in a link time failure
> when building for 32 bit. Either use div_u64(), or something like
> 
> 	wdd->timeout = counter / (1000000000 / NIWD_PERIOD_NS)
> 
> Though wouldn't that be the same as 'timeout' ? Are you trying to work around
> some rounding error here ?
> 

I was, but for no reason apparently. Just tested and I think our resolution is
high enough that you never end up with a shorter timeout than you requested.

> >+
> >+	return 0;
> >+}
> >+
[...]
> >+
> >+		wdt->io_base = res->data.io.minimum;
> >+		io_size = res->data.io.address_length;
> >+
> 
> What if io_size is less than NIWD_IO_SIZE ?
> 

Meant to yoink this check from its previous home in acpi_add when the io_size
element was removed from ni9x3x_wdt, but somehow I lost the snippet entirely.
I'll re-add.

> >+		if (!devm_request_region(wdt->dev, wdt->io_base, io_size,
[...]
> >+	wdd = &wdt->wdd;
> >+	wdd->info = &ni9x3x_wdd_info;
> >+	wdd->ops = &ni9x3x_wdd_ops;
> >+	wdd->min_timeout = NIWD_MIN_TIMEOUT;
> >+	wdd->max_timeout = NIWD_MAX_TIMEOUT;
> >+	wdd->timeout = NIWD_DEFAULT_TIMEOUT;
> >+	wdd->parent = dev;
> >+	watchdog_set_drvdata(wdd, wdt);
> >+	watchdog_set_nowayout(wdd, nowayout);
> >+	ret = watchdog_init_timeout(wdd, timeout, dev);
> >+	if (ret) {
> >+		dev_err(dev, "unable to set timeout value\n");
> 
> Sure you want to abort here, instead of using the default ?
> 

Not at all, that makes more sense.

> >+		return ret;
> >+	}
> >+

Regards,

Kyle

Re: [PATCH v2] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Guenter Roeck]

On 02/19/2016 09:12 AM, Kyle Roeschley wrote:
> On Thu, Feb 18, 2016 at 09:27:05PM -0800, Guenter Roeck wrote:
>> On 02/18/2016 03:26 PM, Kyle Roeschley wrote:
>>> Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
>>> time controllers.
>>>
>>> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
>>> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
>>> ---
>>
>> This would be an optimal place for a change log.
>>
>> [ if any of my comments are duplicates, sorry, but a change log
>>    would help avoiding that ]
>>
>
> Sorry about that, I'll make sure to do so in the future.
>
>>>   Documentation/watchdog/watchdog-parameters.txt |   5 +
>>>   drivers/watchdog/Kconfig                       |  11 +
>>>   drivers/watchdog/Makefile                      |   1 +
>>>   drivers/watchdog/ni9x3x_wdt.c                  | 274 +++++++++++++++++++++++++
>>
>> The name and context make me wonder: Is this driver going to support
>> all watchdog devices for NI9[0-9]3[0-9] ?
>>
>> If not, it may be better to select one supported device,
>> whatever that may be, for the name, and describe in Kconfig
>> and the documentation which devices the driver is known to support.
>>
>
> All of the 9[2-9]3[0-9] devices but the 963x do not and cannot run Linux,
> so they won't be a problem. There is already a driver for the 963x watchdog in
> our tree which is currently third in line for upstreaming, so that should be
> fine as well. That would lead me to think 9x3x is fine, but if not then 903x is
> fine.
>

Please use 903x, if you don't mind. After all, 9x3x _does_ include 963x
and is thus misleading.

Thanks,
Guenter