[PATCH v3 1/2] Add binding documentation for Zodiac Watchdog Timer

[PATCH v3 1/2] Add binding documentation for Zodiac Watchdog Timer

[Martyn Welch]

This patchs adds documentation for the binding of the Zodiac RAVE
Switch Watchdog Processor. This is an i2c based watchdog.

Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Martyn Welch <martyn.welch-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---

v2: Addition of optional properties.
v3: Correct naming of reset-duration-msec -> reset-duration-ms

 .../devicetree/bindings/watchdog/ziirave-wdt.txt      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
new file mode 100644
index 0000000..3d87818
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
@@ -0,0 +1,19 @@
+Zodiac RAVE Watchdog Timer
+
+Required properties:
+- compatible: must be "zii,rave-wdt"
+- reg: i2c slave address of device, usually 0x38
+
+Optional Properties:
+- timeout-sec: Watchdog timeout value in seconds.
+- reset-duration-ms: Duration of the pulse generated when the watchdog times
+  out. Value in milliseconds.
+
+Example:
+
+	watchdog@38 {
+		compatible = "zii,rave-wdt";
+		reg = <0x38>;
+		timeout-sec = <30>;
+		reset-duration-ms = <30>;
+	};
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] Add binding documentation for Zodiac Watchdog Timer

[Martyn Welch]

This patchs adds documentation for the binding of the Zodiac RAVE
Switch Watchdog Processor. This is an i2c based watchdog.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

v2: Addition of optional properties.
v3: Correct naming of reset-duration-msec -> reset-duration-ms

 .../devicetree/bindings/watchdog/ziirave-wdt.txt      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
new file mode 100644
index 0000000..3d87818
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
@@ -0,0 +1,19 @@
+Zodiac RAVE Watchdog Timer
+
+Required properties:
+- compatible: must be "zii,rave-wdt"
+- reg: i2c slave address of device, usually 0x38
+
+Optional Properties:
+- timeout-sec: Watchdog timeout value in seconds.
+- reset-duration-ms: Duration of the pulse generated when the watchdog times
+  out. Value in milliseconds.
+
+Example:
+
+	watchdog@38 {
+		compatible = "zii,rave-wdt";
+		reg = <0x38>;
+		timeout-sec = <30>;
+		reset-duration-ms = <30>;
+	};
-- 
2.1.4

[PATCH v3 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

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@roeck-us.net>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

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@collabora.co.uk>
+ *
+ * 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"};
+
+#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
+
+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;
+}
+
+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");
+
+	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;
+
+	if (duration_parm) {
+		duration = duration_parm;
+	} else {
+		/* 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))
+		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;
+	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;
+	} 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;
+
+	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@collabora.co.uk");
+MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
+MODULE_LICENSE("GPL");
+
-- 
2.1.4

Re: [PATCH v3 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck]

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@roeck-us.net>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>
> 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@collabora.co.uk>
> + *
> + * 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 ?

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

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

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

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

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

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

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

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

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

> +	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@collabora.co.uk");
> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
> +MODULE_LICENSE("GPL");
> +
>

Re: [PATCH v3 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

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@roeck-us.net>
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>
>> 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@collabora.co.uk>
>> + *
>> + * 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@collabora.co.uk");
>> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor 
>> Driver");
>> +MODULE_LICENSE("GPL");
>> +
>>
>

Re: [PATCH v3 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck]

Hi Martyn,

On 11/25/2015 07:36 AM, Martyn Welch wrote:
>
[ ... ]

>>> +
>>> +#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?
>
I thought you wanted to check if I really read your code :-).

Yes, padding would probably be the simplest solution, though question is
what to do with undefined values. Maybe pad with NULL and also check if
ziirave_reasons[x] is not NULL ?


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

Maybe add the following ?

	if (val < ZIIRAVE_TIMEOUT_MIN)
		return -ENODEV;

Thanks,
Guenter

Re: [PATCH v3 2/2] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

On 25/11/15 16:10, Guenter Roeck wrote:
> Hi Martyn,
>
> On 11/25/2015 07:36 AM, Martyn Welch wrote:
>>
> [ ... ]
>
>>>> +
>>>> +#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?
>>
> I thought you wanted to check if I really read your code :-).
>
> Yes, padding would probably be the simplest solution, though question is
> what to do with undefined values. Maybe pad with NULL and also check if
> ziirave_reasons[x] is not NULL ?
>

I'd just added a string "error" then needed to do a strcmp() to check, 
NULL is a much better idea.

>
>>>> +    /*
>>>> +     * 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.
>>
>
> Maybe add the following ?
>
>      if (val < ZIIRAVE_TIMEOUT_MIN)
>          return -ENODEV;
>

Might as well.

Martyn

> Thanks,
> Guenter
>

[PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

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@roeck-us.net>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

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

v4:
 - Correct number of entries in ziirave_reasons array.
 - Remove unused defines and arrays.
 - Merge revision functions.
 - Remove unneeded range check.
 - Remove unneeded variables & initialisations.
 - Remove unused flags from i2c_check_functionality().
 - Move more initialisation before registration.
 - Check returned timeout value.

 drivers/watchdog/Kconfig       |  11 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/ziirave_wdt.c | 379 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 391 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..32078b9
--- /dev/null
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -0,0 +1,379 @@
+/*
+ * Copyright (C) 2015 Zodiac Inflight Innovations
+ *
+ * Author: Martyn Welch <martyn.welch@collabora.co.uk>
+ *
+ * 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_MAX	255
+
+#define ZIIRAVE_PING_VALUE	0x0
+
+#define ZIIRAVE_STATE_INITIAL	0x0
+#define ZIIRAVE_STATE_OFF	0x1
+#define ZIIRAVE_STATE_ON	0x2
+
+static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
+				  "host request", NULL, "illegal configuration",
+				  "illegal instruction", "illegal trap",
+				  "unknown"};
+
+#define ZIIRAVE_WDT_FIRM_VER_MAJOR	0x1
+#define ZIIRAVE_WDT_BOOT_VER_MAJOR	0x3
+#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
+
+struct ziirave_wdt_rev {
+	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_revision(struct i2c_client *client,
+				    struct ziirave_wdt_rev *rev, u8 command)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, command);
+	if (ret < 0)
+		return ret;
+
+	rev->major = ret;
+
+	ret = i2c_smbus_read_byte_data(client, command + 1);
+	if (ret < 0)
+		return ret;
+
+	rev->minor = ret;
+
+	return 0;
+}
+
+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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
+
+	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)
+{
+	int ret;
+
+	if (!duration) {
+		/* See if the reset pulse duration is provided in an of_node */
+		if (client->dev.of_node == NULL)
+			return -ENODEV;
+
+		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);
+
+	return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
+					 duration);
+}
+
+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_SMBUS_BYTE_DATA))
+		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.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;
+
+		if (val < ZIIRAVE_TIMEOUT_MIN)
+			return -ENODEV;
+
+		w_priv->wdd.timeout = val;
+	} 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 = 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_revision(client, &w_priv->firmware_rev,
+				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
+	if (ret)
+		return ret;
+
+	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
+				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
+	if (ret)
+		return ret;
+
+	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
+						ZIIRAVE_WDT_RESET_REASON);
+	if (w_priv->reset_reason < 0)
+		return w_priv->reset_reason;
+
+	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons)
+	    || ziirave_reasons[w_priv->reset_reason] == NULL)
+		return -ENODEV;
+
+	ret = watchdog_register_device(&w_priv->wdd);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
+				 &ziirave_wdt_sysfs_files);
+	if (ret) {
+		watchdog_unregister_device(&w_priv->wdd);
+
+		return ret;
+	}
+
+	return 0;
+}
+
+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@collabora.co.uk");
+MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
+MODULE_LICENSE("GPL");
+
-- 
2.1.4

Re: [PATCH v3 1/2] Add binding documentation for Zodiac Watchdog Timer

[Rob Herring]

On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
> This patchs adds documentation for the binding of the Zodiac RAVE
> Switch Watchdog Processor. This is an i2c based watchdog.
> 
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Martyn Welch <martyn.welch-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> ---
> 
> v2: Addition of optional properties.
> v3: Correct naming of reset-duration-msec -> reset-duration-ms

I acked v2, so please add it.

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> 
>  .../devicetree/bindings/watchdog/ziirave-wdt.txt      | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
> new file mode 100644
> index 0000000..3d87818
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
> @@ -0,0 +1,19 @@
> +Zodiac RAVE Watchdog Timer
> +
> +Required properties:
> +- compatible: must be "zii,rave-wdt"
> +- reg: i2c slave address of device, usually 0x38
> +
> +Optional Properties:
> +- timeout-sec: Watchdog timeout value in seconds.
> +- reset-duration-ms: Duration of the pulse generated when the watchdog times
> +  out. Value in milliseconds.
> +
> +Example:
> +
> +	watchdog@38 {
> +		compatible = "zii,rave-wdt";
> +		reg = <0x38>;
> +		timeout-sec = <30>;
> +		reset-duration-ms = <30>;
> +	};
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] Add binding documentation for Zodiac Watchdog Timer

[Rob Herring]

On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
> This patchs adds documentation for the binding of the Zodiac RAVE
> Switch Watchdog Processor. This is an i2c based watchdog.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
> 
> v2: Addition of optional properties.
> v3: Correct naming of reset-duration-msec -> reset-duration-ms

I acked v2, so please add it.

Acked-by: Rob Herring <robh@kernel.org>

> 
>  .../devicetree/bindings/watchdog/ziirave-wdt.txt      | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
> new file mode 100644
> index 0000000..3d87818
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/ziirave-wdt.txt
> @@ -0,0 +1,19 @@
> +Zodiac RAVE Watchdog Timer
> +
> +Required properties:
> +- compatible: must be "zii,rave-wdt"
> +- reg: i2c slave address of device, usually 0x38
> +
> +Optional Properties:
> +- timeout-sec: Watchdog timeout value in seconds.
> +- reset-duration-ms: Duration of the pulse generated when the watchdog times
> +  out. Value in milliseconds.
> +
> +Example:
> +
> +	watchdog@38 {
> +		compatible = "zii,rave-wdt";
> +		reg = <0x38>;
> +		timeout-sec = <30>;
> +		reset-duration-ms = <30>;
> +	};
> -- 
> 2.1.4
> 

Re: [PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

Hi Guenter,

Was wondering if you had any comments WRT v4?

Martyn

On 25/11/15 17:15, 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@roeck-us.net>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>
> 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.)
>
> v4:
>   - Correct number of entries in ziirave_reasons array.
>   - Remove unused defines and arrays.
>   - Merge revision functions.
>   - Remove unneeded range check.
>   - Remove unneeded variables & initialisations.
>   - Remove unused flags from i2c_check_functionality().
>   - Move more initialisation before registration.
>   - Check returned timeout value.
>
>   drivers/watchdog/Kconfig       |  11 ++
>   drivers/watchdog/Makefile      |   1 +
>   drivers/watchdog/ziirave_wdt.c | 379 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 391 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..32078b9
> --- /dev/null
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -0,0 +1,379 @@
> +/*
> + * Copyright (C) 2015 Zodiac Inflight Innovations
> + *
> + * Author: Martyn Welch <martyn.welch@collabora.co.uk>
> + *
> + * 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_MAX	255
> +
> +#define ZIIRAVE_PING_VALUE	0x0
> +
> +#define ZIIRAVE_STATE_INITIAL	0x0
> +#define ZIIRAVE_STATE_OFF	0x1
> +#define ZIIRAVE_STATE_ON	0x2
> +
> +static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
> +				  "host request", NULL, "illegal configuration",
> +				  "illegal instruction", "illegal trap",
> +				  "unknown"};
> +
> +#define ZIIRAVE_WDT_FIRM_VER_MAJOR	0x1
> +#define ZIIRAVE_WDT_BOOT_VER_MAJOR	0x3
> +#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
> +
> +struct ziirave_wdt_rev {
> +	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_revision(struct i2c_client *client,
> +				    struct ziirave_wdt_rev *rev, u8 command)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, command);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev->major = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, command + 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev->minor = ret;
> +
> +	return 0;
> +}
> +
> +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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
> +
> +	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)
> +{
> +	int ret;
> +
> +	if (!duration) {
> +		/* See if the reset pulse duration is provided in an of_node */
> +		if (client->dev.of_node == NULL)
> +			return -ENODEV;
> +
> +		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);
> +
> +	return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
> +					 duration);
> +}
> +
> +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_SMBUS_BYTE_DATA))
> +		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.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;
> +
> +		if (val < ZIIRAVE_TIMEOUT_MIN)
> +			return -ENODEV;
> +
> +		w_priv->wdd.timeout = val;
> +	} 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 = 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_revision(client, &w_priv->firmware_rev,
> +				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
> +	if (ret)
> +		return ret;
> +
> +	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
> +				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
> +	if (ret)
> +		return ret;
> +
> +	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
> +						ZIIRAVE_WDT_RESET_REASON);
> +	if (w_priv->reset_reason < 0)
> +		return w_priv->reset_reason;
> +
> +	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons)
> +	    || ziirave_reasons[w_priv->reset_reason] == NULL)
> +		return -ENODEV;
> +
> +	ret = watchdog_register_device(&w_priv->wdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
> +				 &ziirave_wdt_sysfs_files);
> +	if (ret) {
> +		watchdog_unregister_device(&w_priv->wdd);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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@collabora.co.uk");
> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
> +MODULE_LICENSE("GPL");
> +
>

Re: [PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck]

On 11/30/2015 01:30 AM, Martyn Welch wrote:
> Hi Guenter,
>
> Was wondering if you had any comments WRT v4?
>

Sorry, I didn't have time to go through it yet. Hopefully this week.

Guenter

> Martyn
>
> On 25/11/15 17:15, 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@roeck-us.net>
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>
>> 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.)
>>
>> v4:
>>   - Correct number of entries in ziirave_reasons array.
>>   - Remove unused defines and arrays.
>>   - Merge revision functions.
>>   - Remove unneeded range check.
>>   - Remove unneeded variables & initialisations.
>>   - Remove unused flags from i2c_check_functionality().
>>   - Move more initialisation before registration.
>>   - Check returned timeout value.
>>
>>   drivers/watchdog/Kconfig       |  11 ++
>>   drivers/watchdog/Makefile      |   1 +
>>   drivers/watchdog/ziirave_wdt.c | 379 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 391 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..32078b9
>> --- /dev/null
>> +++ b/drivers/watchdog/ziirave_wdt.c
>> @@ -0,0 +1,379 @@
>> +/*
>> + * Copyright (C) 2015 Zodiac Inflight Innovations
>> + *
>> + * Author: Martyn Welch <martyn.welch@collabora.co.uk>
>> + *
>> + * 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_MAX    255
>> +
>> +#define ZIIRAVE_PING_VALUE    0x0
>> +
>> +#define ZIIRAVE_STATE_INITIAL    0x0
>> +#define ZIIRAVE_STATE_OFF    0x1
>> +#define ZIIRAVE_STATE_ON    0x2
>> +
>> +static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
>> +                  "host request", NULL, "illegal configuration",
>> +                  "illegal instruction", "illegal trap",
>> +                  "unknown"};
>> +
>> +#define ZIIRAVE_WDT_FIRM_VER_MAJOR    0x1
>> +#define ZIIRAVE_WDT_BOOT_VER_MAJOR    0x3
>> +#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
>> +
>> +struct ziirave_wdt_rev {
>> +    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_revision(struct i2c_client *client,
>> +                    struct ziirave_wdt_rev *rev, u8 command)
>> +{
>> +    int ret;
>> +
>> +    ret = i2c_smbus_read_byte_data(client, command);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    rev->major = ret;
>> +
>> +    ret = i2c_smbus_read_byte_data(client, command + 1);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    rev->minor = ret;
>> +
>> +    return 0;
>> +}
>> +
>> +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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
>> +
>> +    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)
>> +{
>> +    int ret;
>> +
>> +    if (!duration) {
>> +        /* See if the reset pulse duration is provided in an of_node */
>> +        if (client->dev.of_node == NULL)
>> +            return -ENODEV;
>> +
>> +        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);
>> +
>> +    return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
>> +                     duration);
>> +}
>> +
>> +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_SMBUS_BYTE_DATA))
>> +        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.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;
>> +
>> +        if (val < ZIIRAVE_TIMEOUT_MIN)
>> +            return -ENODEV;
>> +
>> +        w_priv->wdd.timeout = val;
>> +    } 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 = 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_revision(client, &w_priv->firmware_rev,
>> +                   ZIIRAVE_WDT_FIRM_VER_MAJOR);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
>> +                   ZIIRAVE_WDT_BOOT_VER_MAJOR);
>> +    if (ret)
>> +        return ret;
>> +
>> +    w_priv->reset_reason = i2c_smbus_read_byte_data(client,
>> +                        ZIIRAVE_WDT_RESET_REASON);
>> +    if (w_priv->reset_reason < 0)
>> +        return w_priv->reset_reason;
>> +
>> +    if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons)
>> +        || ziirave_reasons[w_priv->reset_reason] == NULL)
>> +        return -ENODEV;
>> +
>> +    ret = watchdog_register_device(&w_priv->wdd);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
>> +                 &ziirave_wdt_sysfs_files);
>> +    if (ret) {
>> +        watchdog_unregister_device(&w_priv->wdd);
>> +
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +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@collabora.co.uk");
>> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
>> +MODULE_LICENSE("GPL");
>> +
>>
>

Re: [PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck]

Hi Martyn,

On 11/25/2015 09:15 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.
>
Mostly there, still a few nitpicks and a problem with DT properties.

Thanks,
Guenter

> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>
> 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.)
>
> v4:
>   - Correct number of entries in ziirave_reasons array.
>   - Remove unused defines and arrays.
>   - Merge revision functions.
>   - Remove unneeded range check.
>   - Remove unneeded variables & initialisations.
>   - Remove unused flags from i2c_check_functionality().
>   - Move more initialisation before registration.
>   - Check returned timeout value.
>
>   drivers/watchdog/Kconfig       |  11 ++
>   drivers/watchdog/Makefile      |   1 +
>   drivers/watchdog/ziirave_wdt.c | 379 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 391 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..32078b9
> --- /dev/null
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -0,0 +1,379 @@
> +/*
> + * Copyright (C) 2015 Zodiac Inflight Innovations
> + *
> + * Author: Martyn Welch <martyn.welch@collabora.co.uk>
> + *
> + * 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>

Please order alphabetically.

> +
> +#define ZIIRAVE_TIMEOUT_MIN	3
> +#define ZIIRAVE_TIMEOUT_MAX	255
> +
> +#define ZIIRAVE_PING_VALUE	0x0
> +
> +#define ZIIRAVE_STATE_INITIAL	0x0
> +#define ZIIRAVE_STATE_OFF	0x1
> +#define ZIIRAVE_STATE_ON	0x2
> +
> +static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
> +				  "host request", NULL, "illegal configuration",
> +				  "illegal instruction", "illegal trap",
> +				  "unknown"};
> +
> +#define ZIIRAVE_WDT_FIRM_VER_MAJOR	0x1
> +#define ZIIRAVE_WDT_BOOT_VER_MAJOR	0x3
> +#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
> +
> +struct ziirave_wdt_rev {
> +	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_revision(struct i2c_client *client,
> +				    struct ziirave_wdt_rev *rev, u8 command)

Please align continuation lines with '(', unless that would result in lines longer
than 80 columns.

> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, command);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev->major = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, command + 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev->minor = ret;
> +
> +	return 0;
> +}
> +
> +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);

Please no unnecessary continuation lines.

> +	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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
> +
> +	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)
> +{
> +	int ret;
> +
> +	if (!duration) {
> +		/* See if the reset pulse duration is provided in an of_node */
> +		if (client->dev.of_node == NULL)
> +			return -ENODEV;
> +
Please write as "if (!client->dev.of_node)" (see checkpatch --strict).

Also, is it really necessary to mandate DT support ? There is no mandatory
property to be read, so this driver would (or should) work fine on a non-DT
system.

> +		ret = of_property_read_u32(client->dev.of_node,
> +					   "reset-duration-ms", &duration);

reset-duration-ms is listed as optional property, thus it should not be an error
if it is not provided. Can you set some default in this case ? Or do nothing ?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (duration < 1 || duration > 255)
> +		return -EINVAL;
> +
> +	dev_info(&client->dev, "Setting reset duration to %dms", duration);
> +
> +	return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
> +					 duration);
> +}
> +
> +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_SMBUS_BYTE_DATA))
> +		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.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;
> +
> +		if (val < ZIIRAVE_TIMEOUT_MIN)
> +			return -ENODEV;
> +
> +		w_priv->wdd.timeout = val;
> +	} 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 = 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_revision(client, &w_priv->firmware_rev,
> +				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
> +	if (ret)
> +		return ret;
> +
> +	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
> +				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
> +	if (ret)
> +		return ret;
> +
> +	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
> +						ZIIRAVE_WDT_RESET_REASON);
> +	if (w_priv->reset_reason < 0)
> +		return w_priv->reset_reason;
> +
> +	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons)
> +	    || ziirave_reasons[w_priv->reset_reason] == NULL)
> +		return -ENODEV;

Per checkpatch:

CHECK: Logical continuations should be on the previous line
CHECK: Comparison to NULL could be written "!ziirave_reasons[w_priv->reset_reason]"

> +
> +	ret = watchdog_register_device(&w_priv->wdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
> +				 &ziirave_wdt_sysfs_files);
> +	if (ret) {
> +		watchdog_unregister_device(&w_priv->wdd);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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@collabora.co.uk");
> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
> +MODULE_LICENSE("GPL");
> +
>

Re: [PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

On 30/11/15 15:55, Guenter Roeck wrote:
> Hi Martyn,
>
> On 11/25/2015 09:15 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.
>>
> Mostly there, still a few nitpicks and a problem with DT properties.
>
> Thanks,
> Guenter
>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>
>> 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.)
>>
>> v4:
>>   - Correct number of entries in ziirave_reasons array.
>>   - Remove unused defines and arrays.
>>   - Merge revision functions.
>>   - Remove unneeded range check.
>>   - Remove unneeded variables & initialisations.
>>   - Remove unused flags from i2c_check_functionality().
>>   - Move more initialisation before registration.
>>   - Check returned timeout value.
>>
>>   drivers/watchdog/Kconfig       |  11 ++
>>   drivers/watchdog/Makefile      |   1 +
>>   drivers/watchdog/ziirave_wdt.c | 379
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 391 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..32078b9
>> --- /dev/null
>> +++ b/drivers/watchdog/ziirave_wdt.c
>> @@ -0,0 +1,379 @@
>> +/*
>> + * Copyright (C) 2015 Zodiac Inflight Innovations
>> + *
>> + * Author: Martyn Welch <martyn.welch@collabora.co.uk>
>> + *
>> + * 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>
>
> Please order alphabetically.
>

Will do.

>> +
>> +#define ZIIRAVE_TIMEOUT_MIN    3
>> +#define ZIIRAVE_TIMEOUT_MAX    255
>> +
>> +#define ZIIRAVE_PING_VALUE    0x0
>> +
>> +#define ZIIRAVE_STATE_INITIAL    0x0
>> +#define ZIIRAVE_STATE_OFF    0x1
>> +#define ZIIRAVE_STATE_ON    0x2
>> +
>> +static char *ziirave_reasons[] = {"power cycle", "triggered", NULL,
>> NULL,
>> +                  "host request", NULL, "illegal configuration",
>> +                  "illegal instruction", "illegal trap",
>> +                  "unknown"};
>> +
>> +#define ZIIRAVE_WDT_FIRM_VER_MAJOR    0x1
>> +#define ZIIRAVE_WDT_BOOT_VER_MAJOR    0x3
>> +#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
>> +
>> +struct ziirave_wdt_rev {
>> +    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_revision(struct i2c_client *client,
>> +                    struct ziirave_wdt_rev *rev, u8 command)
>
> Please align continuation lines with '(', unless that would result in
> lines longer
> than 80 columns.
>

Opps - sorry missed one.

>> +{
>> +    int ret;
>> +
>> +    ret = i2c_smbus_read_byte_data(client, command);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    rev->major = ret;
>> +
>> +    ret = i2c_smbus_read_byte_data(client, command + 1);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    rev->minor = ret;
>> +
>> +    return 0;
>> +}
>> +
>> +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);
>
> Please no unnecessary continuation lines.
>

ok

>> +    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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
>> +
>> +    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)
>> +{
>> +    int ret;
>> +
>> +    if (!duration) {
>> +        /* See if the reset pulse duration is provided in an of_node */
>> +        if (client->dev.of_node == NULL)
>> +            return -ENODEV;
>> +
> Please write as "if (!client->dev.of_node)" (see checkpatch --strict).
>

ok

> Also, is it really necessary to mandate DT support ? There is no mandatory
> property to be read, so this driver would (or should) work fine on a non-DT
> system.
>

The error doesn't cause the probe to fail. If it takes that path, it 
results in a message that the default value is being used and continues 
with that.

>> +        ret = of_property_read_u32(client->dev.of_node,
>> +                       "reset-duration-ms", &duration);
>
> reset-duration-ms is listed as optional property, thus it should not be
> an error
> if it is not provided. Can you set some default in this case ? Or do
> nothing ?
>

It takes module parameter, DT entry or falls back to built in default.

>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    if (duration < 1 || duration > 255)
>> +        return -EINVAL;
>> +
>> +    dev_info(&client->dev, "Setting reset duration to %dms", duration);
>> +
>> +    return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
>> +                     duration);
>> +}
>> +
>> +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_SMBUS_BYTE_DATA))
>> +        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.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;
>> +
>> +        if (val < ZIIRAVE_TIMEOUT_MIN)
>> +            return -ENODEV;
>> +
>> +        w_priv->wdd.timeout = val;
>> +    } 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 = 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_revision(client, &w_priv->firmware_rev,
>> +                   ZIIRAVE_WDT_FIRM_VER_MAJOR);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
>> +                   ZIIRAVE_WDT_BOOT_VER_MAJOR);
>> +    if (ret)
>> +        return ret;
>> +
>> +    w_priv->reset_reason = i2c_smbus_read_byte_data(client,
>> +                        ZIIRAVE_WDT_RESET_REASON);
>> +    if (w_priv->reset_reason < 0)
>> +        return w_priv->reset_reason;
>> +
>> +    if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons)
>> +        || ziirave_reasons[w_priv->reset_reason] == NULL)
>> +        return -ENODEV;
>
> Per checkpatch:
>
> CHECK: Logical continuations should be on the previous line
> CHECK: Comparison to NULL could be written
> "!ziirave_reasons[w_priv->reset_reason]"
>

ok

>> +
>> +    ret = watchdog_register_device(&w_priv->wdd);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
>> +                 &ziirave_wdt_sysfs_files);
>> +    if (ret) {
>> +        watchdog_unregister_device(&w_priv->wdd);
>> +
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +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@collabora.co.uk");
>> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor
>> Driver");
>> +MODULE_LICENSE("GPL");
>> +
>>
>

Re: [PATCH v4] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck]

On 11/30/2015 09:26 AM, Martyn Welch wrote:
> On 30/11/15 15:55, Guenter Roeck wrote:
>> Hi Martyn,
>>
>> On 11/25/2015 09:15 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.
>>>
>> Mostly there, still a few nitpicks and a problem with DT properties.
>>
[ ... ]

>>> +
>>> +static int ziirave_wdt_init_duration(struct i2c_client *client, int
>>> duration)
>>> +{

Separate note: The "duration" parameter here doesn't really add value.
It just reflects the module parameter. It would only make sense if the code
to obtain the duration (either from reset_duration or from DT)
would be in a separate function, such as in

	duration = ziirave_get_duration(client);
	if (duration < 0)
		return duration;
	if (duration) {
		ret = ziirave_wdt_init_duration(client, duration);
		if (ret)
			return ret;
	}

or similar.

>>> +    int ret;
>>> +
>>> +    if (!duration) {
>>> +        /* See if the reset pulse duration is provided in an of_node */
>>> +        if (client->dev.of_node == NULL)
>>> +            return -ENODEV;
>>> +
>> Please write as "if (!client->dev.of_node)" (see checkpatch --strict).
>>
>
> ok
>
>> Also, is it really necessary to mandate DT support ? There is no mandatory
>> property to be read, so this driver would (or should) work fine on a non-DT
>> system.
>>
>
> The error doesn't cause the probe to fail. If it takes that path, it results in a message that the default value is being used and continues with that.
>
>>> +        ret = of_property_read_u32(client->dev.of_node,
>>> +                       "reset-duration-ms", &duration);
>>
>> reset-duration-ms is listed as optional property, thus it should not be
>> an error
>> if it is not provided. Can you set some default in this case ? Or do
>> nothing ?
>>
>
> It takes module parameter, DT entry or falls back to built in default.
>

Hmm, you are right. Problem with that though is that it also accepts _invalid_
parameters.

I think it would be better to only return an error if the value is really wrong,
and abort in that case. Otherwise a duration of, say, 1000, will only result
in "unable to set duration", which is likely going to be overlooked.

The current code not distinguish between "no duration provided", "bad duration",
and "duration not accepted by hardware". It seems to me that the latter two
should be fatal and not just be ignored.

Thanks,
Guenter

[PATCH v5] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

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@roeck-us.net>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

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

v4:
 - Correct number of entries in ziirave_reasons array.
 - Remove unused defines and arrays.
 - Merge revision functions.
 - Remove unneeded range check.
 - Remove unneeded variables & initialisations.
 - Remove unused flags from i2c_check_functionality().
 - Move more initialisation before registration.
 - Check returned timeout value.

v5:
 - Correct missed bad indentation & error checking.
 - Treat lack of DT/module param for duration differently from a failure
   to set or bad value.
 - Reorder includes.

 drivers/watchdog/Kconfig       |  11 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/ziirave_wdt.c | 382 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 394 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..fe8b07f
--- /dev/null
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -0,0 +1,382 @@
+/*
+ * Copyright (C) 2015 Zodiac Inflight Innovations
+ *
+ * Author: Martyn Welch <martyn.welch@collabora.co.uk>
+ *
+ * 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/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/version.h>
+#include <linux/watchdog.h>
+
+#define ZIIRAVE_TIMEOUT_MIN	3
+#define ZIIRAVE_TIMEOUT_MAX	255
+
+#define ZIIRAVE_PING_VALUE	0x0
+
+#define ZIIRAVE_STATE_INITIAL	0x0
+#define ZIIRAVE_STATE_OFF	0x1
+#define ZIIRAVE_STATE_ON	0x2
+
+static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
+				  "host request", NULL, "illegal configuration",
+				  "illegal instruction", "illegal trap",
+				  "unknown"};
+
+#define ZIIRAVE_WDT_FIRM_VER_MAJOR	0x1
+#define ZIIRAVE_WDT_BOOT_VER_MAJOR	0x3
+#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
+
+struct ziirave_wdt_rev {
+	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_revision(struct i2c_client *client,
+				struct ziirave_wdt_rev *rev, u8 command)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, command);
+	if (ret < 0)
+		return ret;
+
+	rev->major = ret;
+
+	ret = i2c_smbus_read_byte_data(client, command + 1);
+	if (ret < 0)
+		return ret;
+
+	rev->minor = ret;
+
+	return 0;
+}
+
+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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
+
+	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 ret;
+
+	if (!reset_duration) {
+		/* See if the reset pulse duration is provided in an of_node */
+		if (!client->dev.of_node)
+			ret = -ENODEV;
+		else
+			ret = of_property_read_u32(client->dev.of_node,
+						   "reset-duration-ms",
+						   &reset_duration);
+		if (ret) {
+			dev_info(&client->dev,
+				 "Unable to set reset pulse duration, using default\n");
+			return 0;
+		}
+	}
+
+	if (reset_duration < 1 || reset_duration > 255)
+		return -EINVAL;
+
+	dev_info(&client->dev, "Setting reset duration to %dms",
+		 reset_duration);
+
+	return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
+					 reset_duration);
+}
+
+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_SMBUS_BYTE_DATA))
+		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.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;
+
+		if (val < ZIIRAVE_TIMEOUT_MIN)
+			return -ENODEV;
+
+		w_priv->wdd.timeout = val;
+	} 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 = ziirave_wdt_init_duration(client);
+	if (ret)
+		return ret;
+
+	ret = ziirave_wdt_revision(client, &w_priv->firmware_rev,
+				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
+	if (ret)
+		return ret;
+
+	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
+				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
+	if (ret)
+		return ret;
+
+	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
+						ZIIRAVE_WDT_RESET_REASON);
+	if (w_priv->reset_reason < 0)
+		return w_priv->reset_reason;
+
+	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons) ||
+	    !ziirave_reasons[w_priv->reset_reason])
+		return -ENODEV;
+
+	ret = watchdog_register_device(&w_priv->wdd);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
+				 &ziirave_wdt_sysfs_files);
+	if (ret) {
+		watchdog_unregister_device(&w_priv->wdd);
+
+		return ret;
+	}
+
+	return 0;
+}
+
+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@collabora.co.uk");
+MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
+MODULE_LICENSE("GPL");
+
-- 
2.1.4

Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer

[Guenter Roeck]

On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
> This patchs adds documentation for the binding of the Zodiac RAVE
> Switch Watchdog Processor. This is an i2c based watchdog.
> 
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Martyn Welch <martyn.welch-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer

[Guenter Roeck]

On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
> This patchs adds documentation for the binding of the Zodiac RAVE
> Switch Watchdog Processor. This is an i2c based watchdog.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v5] Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Guenter Roeck]

On 12/01/2015 02:13 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@roeck-us.net>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>

Hi Martyn,

Nitpick:

Applying: Zodiac Aerospace RAVE Switch Watchdog Processor Driver
/home/groeck/src/linux-staging/.git/rebase-apply/patch:461: new blank line at EOF.

Also, the subject line should start with "watchdog: ".

Other than that,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Please fix and resend with my Reviewed-by:.

Thanks,
Guenter

[PATCH v6] watchdog: Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Martyn Welch]

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@roeck-us.net>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---

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

v4:
 - Correct number of entries in ziirave_reasons array.
 - Remove unused defines and arrays.
 - Merge revision functions.
 - Remove unneeded range check.
 - Remove unneeded variables & initialisations.
 - Remove unused flags from i2c_check_functionality().
 - Move more initialisation before registration.
 - Check returned timeout value.

v5:
 - Correct missed bad indentation & error checking.
 - Treat lack of DT/module param for duration differently from a failure
   to set or bad value.
 - Reorder includes.

v6:
 - Remove trailing blank line.
 - Correct comment subject.

 drivers/watchdog/Kconfig       |  11 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/ziirave_wdt.c | 381 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 393 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..b498fdc
--- /dev/null
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -0,0 +1,381 @@
+/*
+ * Copyright (C) 2015 Zodiac Inflight Innovations
+ *
+ * Author: Martyn Welch <martyn.welch@collabora.co.uk>
+ *
+ * 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/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/version.h>
+#include <linux/watchdog.h>
+
+#define ZIIRAVE_TIMEOUT_MIN	3
+#define ZIIRAVE_TIMEOUT_MAX	255
+
+#define ZIIRAVE_PING_VALUE	0x0
+
+#define ZIIRAVE_STATE_INITIAL	0x0
+#define ZIIRAVE_STATE_OFF	0x1
+#define ZIIRAVE_STATE_ON	0x2
+
+static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
+				  "host request", NULL, "illegal configuration",
+				  "illegal instruction", "illegal trap",
+				  "unknown"};
+
+#define ZIIRAVE_WDT_FIRM_VER_MAJOR	0x1
+#define ZIIRAVE_WDT_BOOT_VER_MAJOR	0x3
+#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
+
+struct ziirave_wdt_rev {
+	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_revision(struct i2c_client *client,
+				struct ziirave_wdt_rev *rev, u8 command)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, command);
+	if (ret < 0)
+		return ret;
+
+	rev->major = ret;
+
+	ret = i2c_smbus_read_byte_data(client, command + 1);
+	if (ret < 0)
+		return ret;
+
+	rev->minor = ret;
+
+	return 0;
+}
+
+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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
+
+	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 ret;
+
+	if (!reset_duration) {
+		/* See if the reset pulse duration is provided in an of_node */
+		if (!client->dev.of_node)
+			ret = -ENODEV;
+		else
+			ret = of_property_read_u32(client->dev.of_node,
+						   "reset-duration-ms",
+						   &reset_duration);
+		if (ret) {
+			dev_info(&client->dev,
+				 "Unable to set reset pulse duration, using default\n");
+			return 0;
+		}
+	}
+
+	if (reset_duration < 1 || reset_duration > 255)
+		return -EINVAL;
+
+	dev_info(&client->dev, "Setting reset duration to %dms",
+		 reset_duration);
+
+	return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
+					 reset_duration);
+}
+
+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_SMBUS_BYTE_DATA))
+		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.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;
+
+		if (val < ZIIRAVE_TIMEOUT_MIN)
+			return -ENODEV;
+
+		w_priv->wdd.timeout = val;
+	} 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 = ziirave_wdt_init_duration(client);
+	if (ret)
+		return ret;
+
+	ret = ziirave_wdt_revision(client, &w_priv->firmware_rev,
+				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
+	if (ret)
+		return ret;
+
+	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
+				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
+	if (ret)
+		return ret;
+
+	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
+						ZIIRAVE_WDT_RESET_REASON);
+	if (w_priv->reset_reason < 0)
+		return w_priv->reset_reason;
+
+	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons) ||
+	    !ziirave_reasons[w_priv->reset_reason])
+		return -ENODEV;
+
+	ret = watchdog_register_device(&w_priv->wdd);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
+				 &ziirave_wdt_sysfs_files);
+	if (ret) {
+		watchdog_unregister_device(&w_priv->wdd);
+
+		return ret;
+	}
+
+	return 0;
+}
+
+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@collabora.co.uk");
+MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer

[Martyn Welch]

On 01/12/15 15:15, Guenter Roeck wrote:
> On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
>> This patchs adds documentation for the binding of the Zodiac RAVE
>> Switch Watchdog Processor. This is an i2c based watchdog.
>>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
>> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>> Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
>> Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Martyn Welch <martyn.welch-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>

Would I be right in thinking this will be going via the watchdog tree as 
well?

Martyn
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer

[Martyn Welch]

On 01/12/15 15:15, Guenter Roeck wrote:
> On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
>> This patchs adds documentation for the binding of the Zodiac RAVE
>> Switch Watchdog Processor. This is an i2c based watchdog.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> Acked-by: Rob Herring <robh@kernel.org>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>

Would I be right in thinking this will be going via the watchdog tree as 
well?

Martyn

Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer

[Guenter Roeck]

On Tue, Dec 01, 2015 at 03:38:38PM +0000, Martyn Welch wrote:
> 
> 
> On 01/12/15 15:15, Guenter Roeck wrote:
> >On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
> >>This patchs adds documentation for the binding of the Zodiac RAVE
> >>Switch Watchdog Processor. This is an i2c based watchdog.
> >>
> >>Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> >>Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> >>Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> >>Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> >>Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>Signed-off-by: Martyn Welch <martyn.welch-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> >>Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> >Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >
> 
> Would I be right in thinking this will be going via the watchdog tree as
> well?
> 
I would think so. I'll queue it in my tree and send it to Wim in my next pull
request.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3,1/2] Add binding documentation for Zodiac Watchdog Timer

[Guenter Roeck]

On Tue, Dec 01, 2015 at 03:38:38PM +0000, Martyn Welch wrote:
> 
> 
> On 01/12/15 15:15, Guenter Roeck wrote:
> >On Wed, Nov 25, 2015 at 12:03:34PM +0000, Martyn Welch wrote:
> >>This patchs adds documentation for the binding of the Zodiac RAVE
> >>Switch Watchdog Processor. This is an i2c based watchdog.
> >>
> >>Cc: Rob Herring <robh+dt@kernel.org>
> >>Cc: Pawel Moll <pawel.moll@arm.com>
> >>Cc: Mark Rutland <mark.rutland@arm.com>
> >>Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> >>Cc: Kumar Gala <galak@codeaurora.org>
> >>Cc: devicetree@vger.kernel.org
> >>Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> >>Acked-by: Rob Herring <robh@kernel.org>
> >
> >Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >
> 
> Would I be right in thinking this will be going via the watchdog tree as
> well?
> 
I would think so. I'll queue it in my tree and send it to Wim in my next pull
request.

Guenter

Re: [PATCH v6] watchdog: Zodiac Aerospace RAVE Switch Watchdog Processor Driver

[Wim Van Sebroeck]

Hi Martyn,

> 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@roeck-us.net>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 
> 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.)
> 
> v4:
>  - Correct number of entries in ziirave_reasons array.
>  - Remove unused defines and arrays.
>  - Merge revision functions.
>  - Remove unneeded range check.
>  - Remove unneeded variables & initialisations.
>  - Remove unused flags from i2c_check_functionality().
>  - Move more initialisation before registration.
>  - Check returned timeout value.
> 
> v5:
>  - Correct missed bad indentation & error checking.
>  - Treat lack of DT/module param for duration differently from a failure
>    to set or bad value.
>  - Reorder includes.
> 
> v6:
>  - Remove trailing blank line.
>  - Correct comment subject.
> 
>  drivers/watchdog/Kconfig       |  11 ++
>  drivers/watchdog/Makefile      |   1 +
>  drivers/watchdog/ziirave_wdt.c | 381 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 393 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..b498fdc
> --- /dev/null
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -0,0 +1,381 @@
> +/*
> + * Copyright (C) 2015 Zodiac Inflight Innovations
> + *
> + * Author: Martyn Welch <martyn.welch@collabora.co.uk>
> + *
> + * 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/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/version.h>
> +#include <linux/watchdog.h>
> +
> +#define ZIIRAVE_TIMEOUT_MIN	3
> +#define ZIIRAVE_TIMEOUT_MAX	255
> +
> +#define ZIIRAVE_PING_VALUE	0x0
> +
> +#define ZIIRAVE_STATE_INITIAL	0x0
> +#define ZIIRAVE_STATE_OFF	0x1
> +#define ZIIRAVE_STATE_ON	0x2
> +
> +static char *ziirave_reasons[] = {"power cycle", "triggered", NULL, NULL,
> +				  "host request", NULL, "illegal configuration",
> +				  "illegal instruction", "illegal trap",
> +				  "unknown"};
> +
> +#define ZIIRAVE_WDT_FIRM_VER_MAJOR	0x1
> +#define ZIIRAVE_WDT_BOOT_VER_MAJOR	0x3
> +#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
> +
> +struct ziirave_wdt_rev {
> +	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_revision(struct i2c_client *client,
> +				struct ziirave_wdt_rev *rev, u8 command)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, command);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev->major = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, command + 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev->minor = ret;
> +
> +	return 0;
> +}
> +
> +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, "02.%02u.%02u", 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, "01.%02u.%02u", 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);
> +
> +	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 ret;
> +
> +	if (!reset_duration) {
> +		/* See if the reset pulse duration is provided in an of_node */
> +		if (!client->dev.of_node)
> +			ret = -ENODEV;
> +		else
> +			ret = of_property_read_u32(client->dev.of_node,
> +						   "reset-duration-ms",
> +						   &reset_duration);
> +		if (ret) {
> +			dev_info(&client->dev,
> +				 "Unable to set reset pulse duration, using default\n");
> +			return 0;
> +		}
> +	}
> +
> +	if (reset_duration < 1 || reset_duration > 255)
> +		return -EINVAL;
> +
> +	dev_info(&client->dev, "Setting reset duration to %dms",
> +		 reset_duration);
> +
> +	return i2c_smbus_write_byte_data(client, ZIIRAVE_WDT_RESET_DURATION,
> +					 reset_duration);
> +}
> +
> +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_SMBUS_BYTE_DATA))
> +		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.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;
> +
> +		if (val < ZIIRAVE_TIMEOUT_MIN)
> +			return -ENODEV;
> +
> +		w_priv->wdd.timeout = val;
> +	} 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 = ziirave_wdt_init_duration(client);
> +	if (ret)
> +		return ret;
> +
> +	ret = ziirave_wdt_revision(client, &w_priv->firmware_rev,
> +				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
> +	if (ret)
> +		return ret;
> +
> +	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
> +				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
> +	if (ret)
> +		return ret;
> +
> +	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
> +						ZIIRAVE_WDT_RESET_REASON);
> +	if (w_priv->reset_reason < 0)
> +		return w_priv->reset_reason;
> +
> +	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons) ||
> +	    !ziirave_reasons[w_priv->reset_reason])
> +		return -ENODEV;
> +
> +	ret = watchdog_register_device(&w_priv->wdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
> +				 &ziirave_wdt_sysfs_files);
> +	if (ret) {
> +		watchdog_unregister_device(&w_priv->wdd);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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@collabora.co.uk");
> +MODULE_DESCRIPTION("Zodiac Aerospace RAVE Switch Watchdog Processor Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.1.4
> 

This new driver (including DT patch) has been added to linux-watchdog-next.

Kind regards,
Wim.