Hi,
On Fri, Oct 28, 2016 at 03:51:07PM +0800, Hui Chun Ong wrote:
> Add support for the watchdog timer on PXI Embedded Controller.
>
> Signed-off-by: Hui Chun Ong <hui.chun.ong@xxxxxx>
The problem with your patch are the non-standard attributes it introduces.
+static WDT_ATTR(enable_reset, WDT_CTRL, WDT_CTRL_RESET_EN);
+static WDT_ATTR(enable_soft_ping, WDT_RELOAD_CTRL, WDT_RELOAD_PORT_EN);
+static WDT_ATTR(trigger_polarity, WDT_CTRL, WDT_CTRL_TRIG_POLARITY);
+static WDT_ATTR(keepalive_trigger_polarity, WDT_RELOAD_CTRL,
+ WDT_RELOAD_TRIG_POLARITY);
At first glance, those appear to be platform configuration parameters,
which should either be hard-coded or configured through devicetree /
platform data. Whatever it is, such non-standard attributes consume a
large amount of time, since reviewers will have to understand what the
attributes are used for. On top of that, it is likely that we'll probably
need several rounds of reviews and arguments to determine if the attributes
are really needed or inappropriate. This all takes a lot of time from
reviewers. Because of that, patches like this tend to end up at the end
of my review queue.
Guenter
> ---
> Documentation/watchdog/watchdog-parameters.txt | 5 +
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/ni7018_wdt.c | 506 +++++++++++++++++++++++++
> 4 files changed, 523 insertions(+)
> create mode 100644 drivers/watchdog/ni7018_wdt.c
>
> diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
> index a8d3642..25dbfa2 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -204,6 +204,11 @@ mv64x60_wdt:
> nowayout: Watchdog cannot be stopped once started
> (default=kernel config parameter)
> -------------------------------------------------
> +ni7018_wdt:
> +timeout: Initial watchdog timeout in seconds (0<timeout<464, default=80)
> +nowayout: Watchdog cannot be stopped once started
> + (default=kernel config parameter)
> +-------------------------------------------------
> ni903x_wdt:
> timeout: Initial watchdog timeout in seconds (0<timeout<516, default=60)
> nowayout: Watchdog cannot be stopped once started
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index fdd3228..6d185af 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1314,6 +1314,17 @@ config INTEL_MEI_WDT
> To compile this driver as a module, choose M here:
> the module will be called mei_wdt.
>
> +config NI7018_WDT
> + tristate "NI 7018 Watchdog"
> + depends on X86 && ACPI
> + select WATCHDOG_CORE
> + ---help---
> + This is the device driver for National Instruments NIC7018 watchdog
> + timer.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ni7018_wdt.
> +
> config NI903X_WDT
> tristate "NI 903x/913x Watchdog"
> depends on X86 && ACPI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index caa9f4a..4605cd2 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -139,6 +139,7 @@ obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
> obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> obj-$(CONFIG_INTEL_MEI_WDT) += mei_wdt.o
> obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
> +obj-$(CONFIG_NI7018_WDT) += ni7018_wdt.o
>
> # M32R Architecture
>
> diff --git a/drivers/watchdog/ni7018_wdt.c b/drivers/watchdog/ni7018_wdt.c
> new file mode 100644
> index 0000000..7690314
> --- /dev/null
> +++ b/drivers/watchdog/ni7018_wdt.c
> @@ -0,0 +1,506 @@
> +/*
> + * Copyright (C) 2016 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/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +#include <linux/watchdog.h>
> +
> +#define LOCK 0xA5
> +#define UNLOCK 0x5A
> +
> +#define WDT_CTRL_RESET_EN BIT(7)
> +#define WDT_CTRL_TRIG_POLARITY BIT(4)
> +#define WDT_CTRL_TRIG 0x0F
> +
> +#define WDT_RELOAD_PORT_EN BIT(7)
> +#define WDT_RELOAD_TRIG_POLARITY BIT(6)
> +#define WDT_RELOAD_TRIG 0x0F
> +#define WDT_MIN_TRIG_NUM 0
> +#define WDT_MAX_TRIG_NUM 8
> +
> +#define WDT_CTRL 1
> +#define WDT_RELOAD_CTRL 2
> +#define WDT_PRESET_PRESCALE 4
> +#define WDT_REG_LOCK 5
> +#define WDT_COUNT 6
> +#define WDT_RELOAD_PORT 7
> +
> +#define WDT_IO_SIZE 8
> +
> +#define WDT_MIN_TIMEOUT 1
> +#define WDT_MAX_TIMEOUT 464
> +#define WDT_DEFAULT_TIMEOUT 80
> +
> +#define WDT_MAX_COUNTER 15
> +
> +static unsigned int timeout;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> + "Watchdog timeout in seconds. (default="
> + __MODULE_STRING(WDT_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) ")");
> +
> +struct ni7018_wdt {
> + struct device *dev;
> + u16 io_base;
> + u32 period_ms;
> + struct mutex lock;
> + struct watchdog_device wdd;
> +};
> +
> +struct ni7018_config {
> + u32 period_ms;
> + u8 divider;
> +};
> +
> +static const struct ni7018_config ni7018_configs[] = {
> + { 125, 3 },
> + { 2000, 4 },
> + { 32000, 5 },
> +};
> +
> +static inline u32 ni7018_timeout_ms(u32 period_ms, u8 counter)
> +{
> + return period_ms * counter - period_ms / 2;
> +}
> +
> +static const struct ni7018_config *ni7018_get_config(u32 timeout_ms,
> + u8 *counter)
> +{
> + u32 delta, i, best_delta = U32_MAX;
> + const struct ni7018_config *config, *best_config = NULL;
> + u8 count;
> +
> + for (i = 0; i < ARRAY_SIZE(ni7018_configs); i++) {
> + config = &ni7018_configs[i];
> +
> + count = DIV_ROUND_UP(timeout_ms + config->period_ms / 2,
> + config->period_ms);
> +
> + if (count > WDT_MAX_COUNTER)
> + continue;
> +
> + delta = ni7018_timeout_ms(config->period_ms, count) -
> + timeout_ms;
> +
> + if (delta < best_delta) {
> + best_delta = delta;
> + best_config = config;
> + *counter = count;
> + }
> + }
> + return best_config;
> +}
> +
> +static int ni7018_set_timeout(struct watchdog_device *wdd,
> + unsigned int timeout)
> +{
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + const struct ni7018_config *config;
> + u8 counter;
> +
> + config = ni7018_get_config(timeout * 1000, &counter);
> + if (!config)
> + return -EINVAL;
> +
> + mutex_lock(&wdt->lock);
> +
> + outb(counter << 4 | config->divider,
> + wdt->io_base + WDT_PRESET_PRESCALE);
> +
> + wdd->timeout = ni7018_timeout_ms(config->period_ms, counter) / 1000;
> + wdt->period_ms = config->period_ms;
> +
> + mutex_unlock(&wdt->lock);
> + return 0;
> +}
> +
> +static int ni7018_start(struct watchdog_device *wdd)
> +{
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + u8 control;
> +
> + ni7018_set_timeout(wdd, wdd->timeout);
> +
> + mutex_lock(&wdt->lock);
> +
> + control = inb(wdt->io_base + WDT_RELOAD_CTRL);
> + outb(control | WDT_RELOAD_PORT_EN, wdt->io_base + WDT_RELOAD_CTRL);
> +
> + outb(1, wdt->io_base + WDT_RELOAD_PORT);
> +
> + control = inb(wdt->io_base + WDT_CTRL);
> + outb(control | WDT_CTRL_RESET_EN, wdt->io_base + WDT_CTRL);
> +
> + mutex_unlock(&wdt->lock);
> + return 0;
> +}
> +
> +static int ni7018_stop(struct watchdog_device *wdd)
> +{
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> + mutex_lock(&wdt->lock);
> +
> + outb(0, wdt->io_base + WDT_CTRL);
> + outb(0, wdt->io_base + WDT_RELOAD_CTRL);
> + outb(0xF0, wdt->io_base + WDT_PRESET_PRESCALE);
> +
> + mutex_unlock(&wdt->lock);
> + return 0;
> +}
> +
> +static int ni7018_ping(struct watchdog_device *wdd)
> +{
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> + mutex_lock(&wdt->lock);
> +
> + outb(1, wdt->io_base + WDT_RELOAD_PORT);
> +
> + mutex_unlock(&wdt->lock);
> + return 0;
> +}
> +
> +static unsigned int ni7018_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + u8 count;
> +
> + mutex_lock(&wdt->lock);
> +
> + count = inb(wdt->io_base + WDT_COUNT) & 0xF;
> +
> + mutex_unlock(&wdt->lock);
> +
> + if (!count)
> + return 0;
> +
> + return ni7018_timeout_ms(wdt->period_ms, count) / 1000;
> +}
> +
> +static acpi_status
> +ni7018_resources(struct acpi_resource *res, void *data)
> +{
> + struct ni7018_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 (io_size < WDT_IO_SIZE) {
> + dev_err(wdt->dev, "memory region too small\n");
> + return AE_ERROR;
> + }
> +
> + if (!devm_request_region(wdt->dev, wdt->io_base, io_size,
> + KBUILD_MODNAME)) {
> + dev_err(wdt->dev, "failed to get memory region\n");
> + return AE_ERROR;
> + }
> +
> + return AE_OK;
> +
> + default:
> + /* Ignore unsupported resources */
> + return AE_OK;
> + }
> +}
> +
> +static const struct watchdog_info ni7018_wdd_info = {
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> + .identity = "NI 7018 Watchdog",
> +};
> +
> +static const struct watchdog_ops ni7018_wdd_ops = {
> + .owner = THIS_MODULE,
> + .start = ni7018_start,
> + .stop = ni7018_stop,
> + .ping = ni7018_ping,
> + .set_timeout = ni7018_set_timeout,
> + .get_timeleft = ni7018_get_timeleft,
> +};
> +
> +struct ni7018_attr {
> + struct device_attribute dev_attr;
> + u8 offset;
> + u8 bit;
> +};
> +#define to_ni7018_attr(_attr) \
> + container_of((_attr), struct ni7018_attr, dev_attr)
> +
> +static ssize_t wdt_attr_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + struct ni7018_attr *attr = to_ni7018_attr(da);
> + u8 control;
> + size_t ret;
> +
> + mutex_lock(&wdt->lock);
> +
> + control = inb(wdt->io_base + attr->offset);
> +
> + mutex_unlock(&wdt->lock);
> +
> + ret = sprintf(buf, "%u\n", !!(control & attr->bit));
> + return ret;
> +}
> +
> +static ssize_t wdt_attr_store(struct device *dev,
> + struct device_attribute *da,
> + const char *buf,
> + size_t size)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + struct ni7018_attr *attr = to_ni7018_attr(da);
> + unsigned long val;
> + u8 control;
> +
> + int ret = kstrtoul(buf, 10, &val);
> +
> + if (ret)
> + return ret;
> +
> + if (val < 0 || val > 1)
> + return -EINVAL;
> +
> + mutex_lock(&wdt->lock);
> +
> + control = inb(wdt->io_base + attr->offset);
> + if (val)
> + outb(control | attr->bit, wdt->io_base + attr->offset);
> + else
> + outb(control & ~attr->bit, wdt->io_base + attr->offset);
> +
> + mutex_unlock(&wdt->lock);
> + return size;
> +}
> +
> +#define WDT_ATTR(_name, _offset, _bit) \
> + struct ni7018_attr dev_attr_##_name = { \
> + .offset = _offset, \
> + .bit = _bit, \
> + .dev_attr = \
> + __ATTR(_name, S_IWUSR | S_IRUGO, \
> + wdt_attr_show, wdt_attr_store), \
> + }
> +
> +static WDT_ATTR(enable_reset, WDT_CTRL, WDT_CTRL_RESET_EN);
> +static WDT_ATTR(enable_soft_ping, WDT_RELOAD_CTRL, WDT_RELOAD_PORT_EN);
> +static WDT_ATTR(trigger_polarity, WDT_CTRL, WDT_CTRL_TRIG_POLARITY);
> +static WDT_ATTR(keepalive_trigger_polarity, WDT_RELOAD_CTRL,
> + WDT_RELOAD_TRIG_POLARITY);
> +
> +static ssize_t wdt_trig_show(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + struct ni7018_attr *attr = to_ni7018_attr(da);
> + u8 control;
> + size_t ret;
> +
> + mutex_lock(&wdt->lock);
> +
> + control = inb(wdt->io_base + attr->offset);
> +
> + mutex_unlock(&wdt->lock);
> +
> + if (control & attr->bit)
> + ret = sprintf(buf, "trig%u\n", (control & attr->bit) - 1);
> + else
> + ret = sprintf(buf, "none\n");
> +
> + return ret;
> +}
> +
> +static ssize_t wdt_trig_store(struct device *dev,
> + struct device_attribute *da,
> + const char *buf,
> + size_t size)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct ni7018_wdt *wdt = watchdog_get_drvdata(wdd);
> + struct ni7018_attr *attr = to_ni7018_attr(da);
> + unsigned long val;
> + u8 control;
> + int ret;
> +
> + if (!strncmp(buf, "trig", 4)) {
> + ret = kstrtoul(buf + 4, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val < WDT_MIN_TRIG_NUM || val > WDT_MAX_TRIG_NUM)
> + return -EINVAL;
> +
> + mutex_lock(&wdt->lock);
> +
> + control = inb(wdt->io_base + attr->offset);
> + outb((control & ~attr->bit) | (val + 1),
> + wdt->io_base + attr->offset);
> +
> + mutex_unlock(&wdt->lock);
> +
> + } else if (!strncmp(buf, "none", 4)) {
> + mutex_lock(&wdt->lock);
> +
> + control = inb(wdt->io_base + attr->offset);
> + outb(control & ~attr->bit, wdt->io_base + attr->offset);
> +
> + mutex_unlock(&wdt->lock);
> +
> + } else {
> + return -EINVAL;
> + }
> +
> + return size;
> +}
> +
> +#define WDT_TRIG_ATTR(_name, _offset, _bit) \
> + struct ni7018_attr dev_attr_##_name = { \
> + .offset = _offset, \
> + .bit = _bit, \
> + .dev_attr = \
> + __ATTR(_name, S_IWUSR | S_IRUGO, \
> + wdt_trig_show, wdt_trig_store), \
> + }
> +
> +static WDT_TRIG_ATTR(trigger, WDT_CTRL, WDT_CTRL_TRIG);
> +static WDT_TRIG_ATTR(keepalive_trigger, WDT_RELOAD_CTRL, WDT_RELOAD_TRIG);
> +
> +static struct attribute *ni7018_wdt_attrs[] = {
> + &dev_attr_enable_reset.dev_attr.attr,
> + &dev_attr_enable_soft_ping.dev_attr.attr,
> + &dev_attr_trigger_polarity.dev_attr.attr,
> + &dev_attr_keepalive_trigger_polarity.dev_attr.attr,
> + &dev_attr_trigger.dev_attr.attr,
> + &dev_attr_keepalive_trigger.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(ni7018_wdt);
> +
> +static int ni7018_add(struct acpi_device *device)
> +{
> + struct device *dev = &device->dev;
> + struct watchdog_device *wdd;
> + struct ni7018_wdt *wdt;
> + acpi_status status;
> + int ret;
> +
> + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + device->driver_data = wdt;
> + wdt->dev = dev;
> +
> + status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> + ni7018_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 = &ni7018_wdd_info;
> + wdd->ops = &ni7018_wdd_ops;
> + wdd->min_timeout = WDT_MIN_TIMEOUT;
> + wdd->max_timeout = WDT_MAX_TIMEOUT;
> + wdd->timeout = WDT_DEFAULT_TIMEOUT;
> + wdd->parent = dev;
> + wdd->groups = ni7018_wdt_groups;
> +
> + mutex_init(&wdt->lock);
> + 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, using default\n");
> +
> + /* Unlock WDT register */
> + outb(UNLOCK, wdt->io_base + WDT_REG_LOCK);
> +
> + ret = watchdog_register_device(wdd);
> + if (ret) {
> + dev_err(dev, "failed to register watchdog\n");
> + goto err_out;
> + }
> +
> + dev_dbg(dev, "io_base=0x%04X, timeout=%d, nowayout=%d\n",
> + wdt->io_base, timeout, nowayout);
> + return 0;
> +
> +err_out:
> + mutex_destroy(&wdt->lock);
> + return ret;
> +}
> +
> +static int ni7018_remove(struct acpi_device *device)
> +{
> + struct ni7018_wdt *wdt = acpi_driver_data(device);
> +
> + ni7018_stop(&wdt->wdd);
> + watchdog_unregister_device(&wdt->wdd);
> +
> + /* Lock WDT register */
> + outb(LOCK, wdt->io_base + WDT_REG_LOCK);
> +
> + mutex_destroy(&wdt->lock);
> + return 0;
> +}
> +
> +static const struct acpi_device_id ni7018_device_ids[] = {
> + {"NIC7018", 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, ni7018_device_ids);
> +
> +static struct acpi_driver ni7018_acpi_driver = {
> + .name = KBUILD_MODNAME,
> + .ids = ni7018_device_ids,
> + .ops = {
> + .add = ni7018_add,
> + .remove = ni7018_remove,
> + },
> +};
> +
> +module_acpi_driver(ni7018_acpi_driver);
> +
> +MODULE_DESCRIPTION("NI 7018 Watchdog");
> +MODULE_AUTHOR("Hui Chun Ong <hui.chun.ong@xxxxxx>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
