On 25/11/15 14:52, Guenter Roeck wrote:
Hi Martyn,
On 11/25/2015 04:03 AM, Martyn Welch wrote:
This patch adds a driver for the Zodiac Aerospace RAVE Watchdog
Procesor.
This device implements a watchdog timer, accessible over I2C.
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Signed-off-by: Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx>
---
v2:
- Improved error handling.
- Correct indentation of split lines.
- Remove unnecessary checks, casts and bracketing.
- Remove sysfs entries provided by standard mechanisms.
- Remove simple access functions.
- Allocate wdd as a part of w_priv.
- Add ability to set timeout from module parameter or device tree
property
rather than through a sysfs entry.
- Add ability to set reset duration from module parameter or device
tree
property rather than through a sysfs entry.
v3:
- Correct naming of reset-duration-msec -> reset-duration-ms
- Improved timeout logic (wasn't falling back to default, whoops.)
drivers/watchdog/Kconfig | 11 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/ziirave_wdt.c | 428
+++++++++++++++++++++++++++++++++++++++++
3 files changed, 440 insertions(+)
create mode 100644 drivers/watchdog/ziirave_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7a8a6c6..816b5fb 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -161,6 +161,17 @@ config XILINX_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called of_xilinx_wdt.
+config ZIIRAVE_WATCHDOG
+ tristate "Zodiac RAVE Watchdog Timer"
+ depends on I2C
+ select WATCHDOG_CORE
+ help
+ Watchdog driver for the Zodiac Aerospace RAVE Switch Watchdog
+ Processor.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ziirave_wdt.
+
# ALPHA Architecture
# ARM Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 53d4827..05ba23a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -190,5 +190,6 @@ obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o
obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o
obj-$(CONFIG_MAX63XX_WATCHDOG) += max63xx_wdt.o
+obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o
obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
diff --git a/drivers/watchdog/ziirave_wdt.c
b/drivers/watchdog/ziirave_wdt.c
new file mode 100644
index 0000000..3d337ab
--- /dev/null
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -0,0 +1,428 @@
+/*
+ * Copyright (C) 2015 Zodiac Inflight Innovations
+ *
+ * Author: Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx>
+ *
+ * Based on twl4030_wdt.c by Timo Kokkonen <timo.t.kokkonen at
nokia.com>:
+ *
+ * Copyright (C) Nokia Corporation
+ *
+ * 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/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/watchdog.h>
+#include <linux/i2c.h>
+#include <linux/version.h>
+#include <linux/sysfs.h>
+
+#define ZIIRAVE_TIMEOUT_MIN 3
+#define ZIIRAVE_TIMEOUT_DEFAULT 30
+#define ZIIRAVE_TIMEOUT_MAX 255
+
+#define ZIIRAVE_PING_VALUE 0x0
+#define ZIIRAVE_RESET_VALUE 0x1
+
+#define ZIIRAVE_STATE_INITIAL 0x0
+#define ZIIRAVE_STATE_OFF 0x1
+#define ZIIRAVE_STATE_ON 0x2
+#define ZIIRAVE_STATE_TRIGGERED 0x3
+
+static char *ziirave_states[] = {"unconfigured", "disabled", "enabled",
+ "triggered"};
+
+#define ZIIRAVE_REASON_POWER_UP 0x0
+#define ZIIRAVE_REASON_HW_WDT 0x1
+#define ZIIRAVE_REASON_HOST_REQ 0x4
+#define ZIIRAVE_REASON_IGL_CFG 0x6
+#define ZIIRAVE_REASON_IGL_INST 0x7
+#define ZIIRAVE_REASON_IGL_TRAP 0x8
+#define ZIIRAVE_REASON_UNKNOWN 0x9
+
+static char *ziirave_reasons[] = {"power cycle", "triggered", "host
request",
+ "illegal configuration",
+ "illegal instruction", "illegal trap",
+ "unknown"};
+
I don't see mapping code from the reason values to the array.
"host request" is array index 2 but the defined value is 0x4.
Am I missing something ?
Whoops, missed that. I guess the simplest thing to do is pad out the array?
+#define ZIIRAVE_WDT_FIRM_VER_MAJOR 0x1
+#define ZIIRAVE_WDT_FIRM_VER_MINOR 0x2
+#define ZIIRAVE_WDT_BOOT_VER_MAJOR 0x3
+#define ZIIRAVE_WDT_BOOT_VER_MINOR 0x4
+#define ZIIRAVE_WDT_RESET_REASON 0x5
+#define ZIIRAVE_WDT_STATE 0x6
+#define ZIIRAVE_WDT_TIMEOUT 0x7
+#define ZIIRAVE_WDT_TIME_LEFT 0x8
+#define ZIIRAVE_WDT_PING 0x9
+#define ZIIRAVE_WDT_RESET_DURATION 0xa
+#define ZIIRAVE_WDT_RESET_WDT 0xb
+#define ZIIRAVE_WDT_GOTO_BOOTLOADER 0xc
+#define ZIIRAVE_WDT_FORCE_RESET_HOST 0xd
+
Do you plan to use the unused defines in a later patch ?
No, will remove.
+struct ziirave_wdt_rev {
+ unsigned char part;
+ unsigned char major;
+ unsigned char minor;
+};
+
+struct ziirave_wdt_data {
+ struct watchdog_device wdd;
+ struct ziirave_wdt_rev bootloader_rev;
+ struct ziirave_wdt_rev firmware_rev;
+ int reset_reason;
+};
+
+static int wdt_timeout;
+module_param(wdt_timeout, int, 0);
+MODULE_PARM_DESC(wdt_timeout, "Watchdog timeout in seconds");
+
+static int reset_duration;
+module_param(reset_duration, int, 0);
+MODULE_PARM_DESC(reset_duration,
+ "Watchdog reset pulse duration in milliseconds");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static int ziirave_wdt_firmware_rev(struct i2c_client *client,
+ struct ziirave_wdt_rev *rev)
+{
+ int ret;
+
+ rev->part = 2;
+ ret = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_FIRM_VER_MAJOR);
+ if (ret < 0)
+ return ret;
+
+ rev->major = ret;
+
+ ret = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_FIRM_VER_MINOR);
+ if (ret < 0)
+ return ret;
+
+ rev->minor = ret;
+
+ return 0;
+}
+
+static int ziirave_wdt_bootloader_rev(struct i2c_client *client,
+ struct ziirave_wdt_rev *rev)
+{
+ int ret;
+
+ rev->part = 1;
+ ret = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_BOOT_VER_MAJOR);
+ if (ret < 0)
+ return ret;
+
+ rev->major = ret;
+
+ ret = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_BOOT_VER_MINOR);
+
+ if (ret < 0)
+ return ret;
+
+ rev->minor = ret;
+
+ return 0;
+}
You could merge those two functions into one by also specifying
the registers as parameters. 'part' does not really provide any value -
you could hard-code it in the print function.
Ok.
+
+static int ziirave_wdt_set_state(struct watchdog_device *wdd, int
state)
+{
+ struct i2c_client *client = to_i2c_client(wdd->parent);
+
+ return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_STATE, state);
+}
+
+static int ziirave_wdt_start(struct watchdog_device *wdd)
+{
+ return ziirave_wdt_set_state(wdd, ZIIRAVE_STATE_ON);
+}
+
+static int ziirave_wdt_stop(struct watchdog_device *wdd)
+{
+ return ziirave_wdt_set_state(wdd, ZIIRAVE_STATE_OFF);
+}
+
+static int ziirave_wdt_ping(struct watchdog_device *wdd)
+{
+ struct i2c_client *client = to_i2c_client(wdd->parent);
+
+ return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_PING,
+ ZIIRAVE_PING_VALUE);
+}
+
+static int ziirave_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ struct i2c_client *client = to_i2c_client(wdd->parent);
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_TIMEOUT,
+ timeout);
+ if (!ret)
+ wdd->timeout = timeout;
+
+ return ret;
+}
+
+static unsigned int ziirave_wdt_get_timeleft(struct watchdog_device
*wdd)
+{
+ struct i2c_client *client = to_i2c_client(wdd->parent);
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIME_LEFT);
+ if (ret < 0)
+ ret = 0;
+
+ return ret;
+}
+
+static const struct watchdog_info ziirave_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
WDIOF_KEEPALIVEPING,
+ .identity = "Zodiac RAVE Watchdog",
+};
+
+static const struct watchdog_ops ziirave_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = ziirave_wdt_start,
+ .stop = ziirave_wdt_stop,
+ .ping = ziirave_wdt_ping,
+ .set_timeout = ziirave_wdt_set_timeout,
+ .get_timeleft = ziirave_wdt_get_timeleft,
+};
+
+static ssize_t ziirave_wdt_sysfs_show_firm(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
+
+ return sprintf(buf, "%02u.%02u.%02u", w_priv->firmware_rev.part,
+ w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
+}
+
+static DEVICE_ATTR(firmware_version, S_IRUGO,
ziirave_wdt_sysfs_show_firm,
+ NULL);
+
+static ssize_t ziirave_wdt_sysfs_show_boot(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
+
+ return sprintf(buf, "%02u.%02u.%02u", w_priv->bootloader_rev.part,
+ w_priv->bootloader_rev.major,
+ w_priv->bootloader_rev.minor);
+}
+
+static DEVICE_ATTR(bootloader_version, S_IRUGO,
ziirave_wdt_sysfs_show_boot,
+ NULL);
+
+static ssize_t ziirave_wdt_sysfs_show_reason(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
+
+ if (w_priv->reset_reason < 0 ||
+ w_priv->reset_reason >= ARRAY_SIZE(ziirave_states))
+ return sprintf(buf, "error");
+
The probe function bails out on error, and if the reason is out of range,
so this range check is unnecessary.
Ok.
+ return sprintf(buf, "%s", ziirave_reasons[w_priv->reset_reason]);
+}
+
+static DEVICE_ATTR(reset_reason, S_IRUGO,
ziirave_wdt_sysfs_show_reason,
+ NULL);
+
+static struct attribute *ziirave_wdt_attrs[] = {
+ &dev_attr_firmware_version.attr,
+ &dev_attr_bootloader_version.attr,
+ &dev_attr_reset_reason.attr,
+ NULL
+};
+
+static const struct attribute_group ziirave_wdt_sysfs_files = {
+ .attrs = ziirave_wdt_attrs,
+};
+
+static int ziirave_wdt_init_duration(struct i2c_client *client,
+ int duration_parm)
+{
+ int duration = 0;
+ int ret = 0;
Unnecessary initialization.
Ok.
+
+ if (duration_parm) {
+ duration = duration_parm;
+ } else {
Since you are initializing duration, you might as well use it directly as
parameter.
..., int duration)
{
..
if (!duration) {
Good point.
+ /* See if the reset pulse duration is provided in an of_node */
+ if (client->dev.of_node == NULL)
+ return ret;
+
+ ret = of_property_read_u32(client->dev.of_node,
+ "reset-duration-ms", &duration);
+ if (ret)
+ return ret;
+ }
+
+ if (duration < 1 || duration > 255)
+ return -EINVAL;
+
+ dev_info(&client->dev, "Setting reset duration to %dms", duration);
+
+ ret = i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
+ duration);
+
+ return ret;
+}
+
+static int ziirave_wdt_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int ret;
+ struct ziirave_wdt_data *w_priv;
+ int val;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
+ | I2C_FUNC_SMBUS_BYTE_DATA
+ | I2C_FUNC_SMBUS_READ_I2C_BLOCK))
Why 2C_FUNC_SMBUS_READ_I2C_BLOCK, and why I2C_FUNC_I2C ?
Unless I am missing something, you only need I2C_FUNC_SMBUS_BYTE_DATA.
Left over from some early goes at implementing it and hadn't noticed it
again to take it out.
+ return -ENODEV;
+
+ w_priv = devm_kzalloc(&client->dev, sizeof(*w_priv), GFP_KERNEL);
+ if (!w_priv)
+ return -ENOMEM;
+
+ w_priv->wdd.info = &ziirave_wdt_info;
+ w_priv->wdd.ops = &ziirave_wdt_ops;
+ w_priv->wdd.status = 0;
Unnecessary initialization.
Ok.
+ w_priv->wdd.min_timeout = ZIIRAVE_TIMEOUT_MIN;
+ w_priv->wdd.max_timeout = ZIIRAVE_TIMEOUT_MAX;
+ w_priv->wdd.parent = &client->dev;
+
+ ret = watchdog_init_timeout(&w_priv->wdd, wdt_timeout,
&client->dev);
+ if (ret) {
+ dev_info(&client->dev,
+ "Unable to select timeout value, using default\n");
+ }
+
+ /*
+ * The default value set in the watchdog should be perfectly
valid, so
+ * pass that in if we haven't provided one via the module
parameter or
+ * of property.
+ */
+ if (w_priv->wdd.timeout == 0) {
+ val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT);
+ if (val < 0)
+ return val;
+
+ w_priv->wdd.timeout = val;
What if the returned value is 0 ?
Minimum timeout is 3, device won't accept a value below that and should
never return a value
below that.
+ } else {
+ ret = ziirave_wdt_set_timeout(&w_priv->wdd,
w_priv->wdd.timeout);
+ if (ret)
+ return ret;
+
+ dev_info(&client->dev, "Timeout set to %ds.",
w_priv->wdd.timeout);
+ }
+
+ watchdog_set_nowayout(&w_priv->wdd, nowayout);
+
+ i2c_set_clientdata(client, w_priv);
+
+ /* If in unconfigured state, set to stopped */
+ val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE);
+ if (val < 0)
+ return val;
+
+ if (val == ZIIRAVE_STATE_INITIAL)
+ ziirave_wdt_stop(&w_priv->wdd);
+
+ ret = watchdog_register_device(&w_priv->wdd);
+ if (ret)
+ return ret;
+
+ ret = ziirave_wdt_init_duration(client, reset_duration);
+ if (ret) {
+ dev_info(&client->dev,
+ "Unable to set reset pulse duration, using default\n");
+ }
+
+ ret = ziirave_wdt_firmware_rev(client, &w_priv->firmware_rev);
+ if (ret)
+ goto err;
+
+ ret = ziirave_wdt_bootloader_rev(client, &w_priv->bootloader_rev);
+ if (ret)
+ goto err;
+
+ w_priv->reset_reason = i2c_smbus_read_byte_data(client,
+ ZIIRAVE_WDT_RESET_REASON);
+ if (w_priv->reset_reason < 0
+ || w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons))
+ goto err;
+
Would it make sense to call those functions prior to registering the
watchdog ?
This would simplify error handling, and avoid a registration followed
by an
unregistration if the i2c functions return an error.
Ah, yes - the helper functions I was using took wdd, so the device
needed to be register for them to work. Not the case any more so this
can be done prior to registration.
+ ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
+ &ziirave_wdt_sysfs_files);
+ if (ret)
+ goto err;
+
+ return 0;
+err:
+ watchdog_unregister_device(&w_priv->wdd);
+
+ return ret;
+}
+
+static int ziirave_wdt_remove(struct i2c_client *client)
+{
+ struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
+
+ sysfs_remove_group(&client->dev.kobj, &ziirave_wdt_sysfs_files);
+
+ watchdog_unregister_device(&w_priv->wdd);
+
+ return 0;
+}
+
+static struct i2c_device_id ziirave_wdt_id[] = {
+ { "ziirave-wdt", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ziirave_wdt_id);
+
+static const struct of_device_id zrv_wdt_of_match[] = {
+ { .compatible = "zii,rave-wdt", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, zrv_wdt_of_match);
+
+static struct i2c_driver ziirave_wdt_driver = {
+ .driver = {
+ .name = "ziirave_wdt",
+ .of_match_table = zrv_wdt_of_match,
+ },
+ .probe = ziirave_wdt_probe,
+ .remove = ziirave_wdt_remove,
+ .id_table = ziirave_wdt_id,
+};
+
+module_i2c_driver(ziirave_wdt_driver);
+
+MODULE_AUTHOR("Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx");
+MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor
Driver");
+MODULE_LICENSE("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
