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@xxxxxx>
Signed-off-by: Kyle Roeschley <kyle.roeschley@xxxxxx>
---
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@xxxxxx>");
+MODULE_AUTHOR("Kyle Roeschley <kyle.roeschley@xxxxxx>");
+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.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
